Merge lp:~gingerchris/endroid/nonmemberkick into lp:endroid

[ChrisD]

Add support for users to be kicked from rooms if they are not members of the room.

Add support for users to be kicked from rooms if they are not members of the room.
This is needed because some webex servers don't support mamber-only rooms.

As part of this:

 * Add a new JID class in usermanagement with the aim of this become the common representation of JIDs (users and rooms)
 * Remove any use of parse that I came accross (it's not needed)
 * Make the the set_available methods take this new JID object as an argument.
 * Override set_avail and unavail in the Room roster class to support the kicking (also means more accurate debugging can be added)
 * Add a tiny kick method on the UM object and migrate the other use of kick to this.
 * Rename um.JOIN_ATTEMPTS_MAX so its less shouty
 * Acknowledge that it is not ideal that UM is reading roomowner config.
 * Allow a rooms member list to be specified in the roomowner section of config [Though more thought needed on this e.g. how EnDroid should support joining rooms it doesn't own]
 * Remove unnecessary set_registration_list method on UM (it was only called from UM methods which can call the roster method directly).

[Matthew Earl (matthew-earl)]

line 21 (src/endroid/rosterhandler.py):
    logging.info("Available from {} '{}' priority: '{}'".format(str(jid), show, priority))

    Superfluous use of str?

[Martin Morrison (isoschiz)]

The presence of an explicit fetch of the roomowner plugin's config in the core seems horrible. Can't we just move that config up to be core config? We can then move it back out to plugin config once we've cleaned up the interactions throughout.

Some of your docstrings don't have a newline after the opening triple quotes. Please change, as it is easier to read the other way.

[ChrisD (gingerchris)]

> line 21 (src/endroid/rosterhandler.py):
> logging.info("Available from {} '{}' priority: '{}'".format(str(jid),
> show, priority))
>
> Superfluous use of str?

Meh

[ChrisD (gingerchris)]

> The presence of an explicit fetch of the roomowner plugin's config in the core
> seems horrible. Can't we just move that config up to be core config? We can
> then move it back out to plugin config once we've cleaned up the interactions
> throughout.

There are two cases:
 * The password
 * The user list

The password was already there and will be a pain to move.
The user one was added by me - I can remove that but if I do it means you need two room sections:

[room : <email address hidden>] - with the user list
and
[room : <email address hidden> : plugin : endroid.plugins.roomowner] - with the name and description

when owning a room. This is a pain.

While the password config read hack is still there, having the user list hack seems sensible. Removing the password config hack opens the larger can of worms.

>
> Some of your docstrings don't have a newline after the opening triple quotes.
> Please change, as it is easier to read the other way.

Done.

[ChrisD (gingerchris)]

> > The presence of an explicit fetch of the roomowner plugin's config in the
> core
> > seems horrible. Can't we just move that config up to be core config? We can
> > then move it back out to plugin config once we've cleaned up the
> interactions
> > throughout.
>
> There are two cases:
> * The password
> * The user list
>
> The password was already there and will be a pain to move.
> The user one was added by me - I can remove that but if I do it means you need
> two room sections:
>
> [room : <email address hidden>] - with the user list
> and
> [room : <email address hidden> : plugin : endroid.plugins.roomowner] - with the name
> and description
>
> when owning a room. This is a pain.
>
> While the password config read hack is still there, having the user list hack
> seems sensible. Removing the password config hack opens the larger can of
> worms.

As per discussion; will leave this for now. See my advanced-roomowner blueprint for some thoughts on sorting this:

https://blueprints.launchpad.net/endroid/+spec/advanced-roomowner

>
>
> >
> > Some of your docstrings don't have a newline after the opening triple
> quotes.
> > Please change, as it is easier to read the other way.
>
> Done.

[Martin Morrison (isoschiz)]

review: Approve

[Martin Morrison (isoschiz)]

More than one proposal found for merge of lp:~gingerchris/endroid/bugfix into lp:endroid, which is not Superseded.

[Martin Morrison (isoschiz)]

More than one proposal found for merge of lp:~gingerchris/endroid/bugfix into lp:endroid, which is not Superseded.

[Martin Morrison (isoschiz)]

More than one proposal found for merge of lp:~gingerchris/endroid/bugfix into lp:endroid, which is not Superseded.

Unmerged revisions

=== modified file 'src/endroid/rosterhandler.py'
--- src/endroid/rosterhandler.py	2013-08-21 09:56:42 +0000
+++ src/endroid/rosterhandler.py	2013-09-06 10:08:14 +0000
@@ -6,7 +6,7 @@
 
 import logging
 
-from twisted.words.protocols.jabber.jid import JID
+from endroid.usermanagement import JID
 from wokkel.xmppim import RosterClientProtocol, PresenceClientProtocol, RosterItem
 
 
@@ -81,23 +81,34 @@
         self.available()
 
     def availableReceived(self, entity, show=None, statuses=None, priority=0):
-        userhost, full_jid = entity.userhost(), entity.full()
-        logging.info("Available from {} '{}' priority: '{}'".format(full_jid, show, priority))
+        jid = JID(entity.full())
+        userhost = jid.userhost()
+        logging.info("Available from {} '{}' priority: '{}'".format(jid, show, priority))
         # entity has come online - update our online set
         if userhost in self.um.users():
-            self.um.set_available(full_jid, show=show, priority=priority)
+            self.um.set_available(jid, show=show, priority=priority)
             # make them available in their groups
             for group in self.um.get_groups(userhost):
-                self.um.set_available(full_jid, group, show=show, priority=priority)
+                self.um.set_available(jid, group, show=show, priority=priority)
+        else:
+            # Probably a user entering a room EnDroid is in.
+            # If this is the case EnDroid will find out via a userJoinedRoom call
+            # on wokkelhandler
+            pass
 
     def unavailableReceived(self, entity, statuses=None):
-        userhost, full_jid = entity.userhost(), entity.full()
-        logging.info("Unavailable from {}".format(full_jid))
+        jid = JID(entity.full())
+        userhost = jid.userhost()
+        logging.info("Unavailable from {}".format(str(jid)))
         # entity has gone offline - update our online set
         if userhost in self.um.available_users():
-            self.um.set_unavailable(full_jid)
+            self.um.set_unavailable(jid)
             # make them unavailable in their groups
             for group in self.um.get_groups(userhost):
-                self.um.set_unavailable(full_jid, group)
-
+                self.um.set_unavailable(jid, group)
+        else:
+            # Probably a user leaving a room EnDroid is in.
+            # If this is the case EnDroid will find out via a userLeftRoom call
+            # on wokkelhandler
+            pass
 

=== modified file 'src/endroid/usermanagement.py'
--- src/endroid/usermanagement.py	2013-08-21 17:00:41 +0000
+++ src/endroid/usermanagement.py	2013-09-06 10:08:14 +0000
@@ -5,7 +5,8 @@
 # -----------------------------------------
 
 import logging
-from twisted.words.protocols.jabber.jid import JID, parse, InvalidFormat
+import twisted.words.protocols.jabber.jid
+from twisted.words.protocols.jabber.jid import InvalidFormat
 from twisted.words.protocols.jabber.error import StanzaError
 from endroid.pluginmanager import PluginManager
 from random import choice
@@ -24,6 +25,38 @@
 
 Place = namedtuple("Place", ("type", "name"))
 
+
+class JID(twisted.words.protocols.jabber.jid.JID):
+    """
+    Wrapper around twisteds JID class to provide more functionality.
+
+
+    Attributes:
+        Inherited from twisted:
+          user - the user part of the JID
+          host - the host part of the JID
+          resource - the resource part of the JID
+        Added by this class:
+          nick - nickname of this JID if in a room
+
+    Methods:
+        Inherited from twisted:
+          full - string representation of the JID - user str(JID) instead
+          userhost - string representation of just the user and host
+                     parts of this JID
+          userhostJID - a new JID object representing just the user and 
+                        host parts of this JID
+          
+    """
+
+    def __init__(self, str=None, tuple=None, nick=None, **kwargs):
+        self.nick = nick
+        super(JID, self).__init__(str=str, tuple=tuple, **kwargs)
+
+    def __str__(self):
+        return self.full()
+
+
 class Roster(object):
     """
     Provides functions for maintaining sets of users registered with and
@@ -67,17 +100,29 @@
         self._members.pop(name, None)
         self.deregistration_cb(name, self.name)
 
-    def set_available(self, full_jid, show=None, priority=0):
-        userhost = UserManagement.get_userhost(full_jid)
-        logging.debug("{} came online as {}".format(userhost, full_jid))
-        if userhost in self._members:
-            self._members[userhost][full_jid] = Resource(show, priority)
-
-    def set_unavailable(self, full_jid):
-        userhost = UserManagement.get_userhost(full_jid)
-        logging.debug("{} went offline as {}".format(userhost, full_jid))
-        if userhost in self._members:
-            self._members[userhost].pop(full_jid, None)
+    def set_available(self, jid, show=None, priority=0):
+        """
+        Mark a JID as available.
+
+        jid should be a JID object
+
+        """
+
+        logging.debug("{} available at resource {}".format(jid.userhost(), jid.resource))
+        if jid.userhost() in self._members:
+            self._members[jid.userhost()][jid.full()] = Resource(show, priority)
+
+    def set_unavailable(self, jid):
+        """
+        Mark a JID as unavailable.
+
+        jid should be a JID object
+
+        """
+
+        logging.debug("{} unavailable at resource {}".format(jid.userhost(), jid.resource))
+        if jid.userhost() in self._members:
+            self._members[jid.userhost()].pop(jid.full(), None)
 
     def __repr__(self):
         name = self.name or "contacts"
@@ -119,14 +164,48 @@
             options[MUC + "passwordprotectedroom"] = True
         return options
 
-    def __init__(self, name, nick, password, *args, **kwargs):
+    def __init__(self, name, nick, password, um, *args, **kwargs):
         self.nick = nick
         self.password = password
-        # our add/remove user deferreds can only be run after we join the room,
-        # so keep this list to store those generated before we do so
+
+        # Keep a reference to the usermanagement object
+        # @@@ Evil hack until the roomowner plugin can register for
+        # presence notifications - the UM ref is needed to kick
+        # unwanted members
+        self.um = um
+
+        # Initialise a list to keep membership changes to be made
+        # once EnDroid has joined the room
         self.deferreds = []
         super(Room, self).__init__(name, *args, **kwargs)
 
+    def set_available(self, user):
+        """
+        Mark a user as being available in a room.
+
+        user is a JID object.
+        If the user isn't registered with the room they are kicked.
+
+        """
+
+        logging.debug("{} ({}) joined room {}".format(
+                      user.nick, user, self.name))
+        if user.userhost() in self._members:
+            self._members[user.userhost()][user.full()] = Resource(False, 1)
+        else:
+            # This person shouldn't be here, kick
+            self.um.kick(room=self.name, nick=user.nick, reason="You are "
+                         "not on the memberlist, sorry.")
+
+    def set_unavailable(self, user):
+        """Process a user leaving a room."""
+
+        logging.debug("{} ({}) left room {}".format(
+                      user.nick, user, self.name))
+        if user.userhost() in self._members:
+            self._members[user.userhost()].pop(user.full(), None)
+
+
 class Resource(object):
     __slots__ = ("show", "priority")
 
@@ -153,6 +232,7 @@
         self._pms = {}  # a dict of {room/group names : pluginmanager objects}
 
         self.jid = None
+        self.jid_obj = None
         # our contact list and room list
         self._users = Roster(None, self._cb_add_contact, self._cb_rem_contact)
         self._rooms = Roster()
@@ -163,38 +243,48 @@
         self.conf = config
         self._read_config(config)
 
+    def _allowed_users(self, r_g, name):
+        """Get the set of allowed users for this room or group."""
+    
+        # The config get could return an empty list e.g. if 'users='
+        users = set(self.conf.get(r_g, name, "users", default=self.users()))
+        return users & self.users()
+
     def _read_config(self, config):
-        def allowed_users(r_g, name):
-            # get may return an empty string (if the line is 'users='), so use
-            # or to make sure we have a set
-            users = set(config.get(r_g, name, "users", default=self.users()))
-            return users & self.users()
-
-        # set our contact list and room list
+        # Set our contact list and room list
         self.jid = config.get("setup", "jid")
+        self.jid_obj = JID(self.jid)
         self._users.set_registration_list(config.get("setup", "users", default=[]))
         self._rooms.set_registration_list(config.get("setup", "rooms", default=[]))
 
-        self.JOIN_ATTEMPTS_MAX = config.get("setup", "join_attempts", default=5)
+        self.join_attempts = config.get("setup", "join_attempts", 
+                                        default=self.JOIN_ATTEMPTS_MAX)
 
-        # set contact lists for our rooms
+        nick = config.get("setup", "nick", default=self.jid_obj.user)
+        # Set contact lists for our rooms
         for room in config.get("setup", "rooms", default=[]):
-            # what we need to join the room
-            nick = config.get("setup", "nick", default=parse(self.jid)[0])
+            # What we need to join the room
+            # @@@ Evil hack until roomowner can be made a global plugin
+            # that triggers room joins
             password = config.get("room", room, "plugin", 
                                   "endroid.plugins.roomowner", "password",
                                   default="")
-            # user data
-            self.room_rosters[room] = Room(room, nick, password, 
+            try:
+                users = config.get("room", room, "plugin",
+                                   "endroid.plugins.roomowner", "users")
+                users = set(users) & self.users()
+            except KeyError:
+                # User list may have been specified old style:
+                users = self._allowed_users('room', room)
+            self.room_rosters[room] = Room(room, nick, password, self, 
                                            self._cb_add_room_contact,
                                            self._cb_rem_room_contact)
-            users = allowed_users("room", room)
-            self.set_registration_list(users, room)
+            self.room_rosters[room].set_registration_list(users)
 
         for group in config.get("setup", "groups", default=['all']):
             self.group_rosters[group] = Roster(group)
-            users = allowed_users("group", group)
-            self.set_registration_list(users, group)
+            users = self._allowed_users("group", group)
+            self.group_rosters[group].set_registration_list(users)
 
         # rosterhandler receives presence notifications and gives them to us
         self.rh.set_presence_handler(self)
@@ -317,31 +407,18 @@
         or if place is None (default), the user part of the jid.
 
         """
-        user = self.get_userhost(user)
+        user_jid = JID(user)
         if place is None or place in self.group_rosters:
-            return parse(user)[0]
+            return user_jid.user
         elif place in self.room_rosters:
             for (nick, rosteritem) in self.wh._rooms[JID(place)].roster.items():
-                if user == rosteritem.entity.userhost():
+                if user_jid.userhost() == rosteritem.entity.userhost():
                     return nick
         return "unknown"
     get_nickname = nickname
 
     ### Functions for managing contact lists ###
 
-    def set_registration_list(self, names, place=None):
-        """
-        Set the 'member' list for our contacts (place = None) or in a group or
-        room. This specifies who is allowed in said place.
-
-        """
-        if place is None:
-            self._users.set_registration_list(names)
-        elif place in self.group_rosters:
-            self.group_rosters[place].set_registration_list(names)
-        elif place in self.room_rosters:
-            self.room_rosters[place].set_registration_list(names)
-
     def register_user(self, name, place=None):
         """
         Add a user to the 'member' list for our contacts (place=None) or in a
@@ -368,31 +445,41 @@
         elif place in self.room_rosters:
             self.room_rosters[place].deregister_user(name)
 
-    def set_available(self, name, place=None, show=None, priority=0):
-        """
-        Add a user to the 'available' list for our contacts (place=None) or in a
-        group or room.
-
-        """
-        if place is None:
-            self._users.set_available(name, show, priority)
-        elif place in self.group_rosters:
-            self.group_rosters[place].set_available(name, show, priority)
-        elif place in self.room_rosters:
-            self.room_rosters[place].set_available(name, show, priority)
-
-    def set_unavailable(self, name, place=None):
-        """
-        Add a user to the 'available' list for our contacts (place=None) or in a
-        group or room.
-
-        """
-        if place is None:
-            self._users.set_unavailable(name)
-        elif place in self.group_rosters:
-            self.group_rosters[place].set_unavailable(name)
-        elif place in self.room_rosters:
-            self.room_rosters[place].set_unavailable(name)
+    def set_available(self, user, place=None, show=None, priority=0):
+        """
+        Add a user to the 'available' list for our contacts (place=None) or in a
+        group or room.
+
+        user should be a JID object
+
+        """
+        if place is None:
+            self._users.set_available(user, show, priority)
+        elif place in self.group_rosters:
+            self.group_rosters[place].set_available(user, show, priority)
+        elif place in self.room_rosters:
+            self.room_rosters[place].set_available(user)
+        else:
+            logging.error("User {} available but {} not in any roster".format(
+                          user, place))
+
+    def set_unavailable(self, user, place=None):
+        """
+        Add a user to the 'available' list for our contacts (place=None) or in a
+        group or room.
+
+        user should be a JID object
+
+        """
+        if place is None:
+            self._users.set_unavailable(user)
+        elif place in self.group_rosters:
+            self.group_rosters[place].set_unavailable(user)
+        elif place in self.room_rosters:
+            self.room_rosters[place].set_unavailable(user)
+        else:
+            logging.error("User {} unavailable but {} not in any roster".format(
+                          user, room))
 
     # callbacks for adding/removing from our main contact list
     def _cb_add_contact(self, name, _):
@@ -443,6 +530,8 @@
                 members.append(user)
             else:
                 nones.append(user)
+        # Reset the list as we don't need it anymore
+        self.room_rosters[name].deferreds = []
 
         fmt_add = "Added {} to {}"
         fmt_rem = "Removed {} from {}"
@@ -465,20 +554,21 @@
     def join_room(self, name, attempts=0):
         """
         Attempt to join the room 'name'. On failure will try again with
-        different nicknames until attempts == JOIN_ATTEMPTS_MAX succes.
+        different nicknames until attempts == join_attempts.
 
         """
         if not name in self.room_rosters:
             return "Failed to join room {}: not allowed".format(name)
 
         def retry(failure, room, nick, attempts):
-            if attempts < self.JOIN_ATTEMPTS_MAX:
+            if attempts < self.join_attempts:
                 return self.join_room(room, attempts+1)
             else:
                 return "Failed to join room {} as {}".format(room, nick)
         def joined(_, room, nick, attempts):
             if attempts:
-                self.wh.kick(JID(room), nick, "Begone thou who taketh my name!")
+                self.kick(room, nick, "You've taken EnDroid's nick, "
+                          "please reconnect with another nick")
                 self.wh.nick(JID(room), nick)
             return "Joined room {} as {}".format(room, nick)
 
@@ -493,11 +583,16 @@
         
         return d
 
+    def kick(self, room, nick, reason=None):
+        """Kick the user with the specified nick from the room."""
+        logging.debug("Kicking {} from {}".format(nick, room))
+        self.wh.kick(JID(room), nick, reason)
+
     def joined_room(self, name):
         logging.info("Joined room {}".format(name))
         self._do_room_contacts(name)
-        self._rooms.set_available(name)
-        if not name in self._pms:
+        self._rooms.set_available(JID(name))
+        if name not in self._pms:
             self.start_pm(None, "room", name)
 
     def joined_group(self, name):

=== modified file 'src/endroid/wokkelhandler.py'
--- src/endroid/wokkelhandler.py	2013-08-28 17:22:30 +0000
+++ src/endroid/wokkelhandler.py	2013-09-06 10:08:14 +0000
@@ -5,12 +5,12 @@
 # -----------------------------------------
 
 from endroid.messagehandler import Message
+from endroid.usermanagement import JID
 from endroid.cron import Cron
 import logging
 
 from wokkel.muc import MUCClient
 from wokkel.xmppim import MessageProtocol
-from twisted.words.protocols.jabber.jid import JID
 from twisted.internet.task import LoopingCall
 
 
@@ -106,17 +106,18 @@
     # MUCClient methods to be overridden
 
     def userJoinedRoom(self, room, user):
-        room_name = room.roomJID.userhost()
-        user_name = user.entity.full()
-        if user_name == self.jid:
+        user_obj = JID(user.entity.full(), nick=user.nick)
+        if user_obj.full() == self.jid:
             # This is EnDroid joining a room nothing to do
             pass
         else:
-            logging.info(user_name + " joined " + room_name)
-            self.usermanagement.set_available(user, room)
+            self.usermanagement.set_available(user_obj, 
+                                              room.roomJID.userhost())
 
     def userLeftRoom(self, room, user):
-        self.usermanagement.set_unavailable(user, room)
+        user_obj = JID(user.entity.full(), nick=user.nick)
+        self.usermanagement.set_unavailable(user_obj,
+                                            room.roomJID.userhost())
 
     def userUpdatedStatus(self, room, user, show, status):
         pass