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

[ChrisD]

RoomOwner fixes

Following roomowner changes:

 * Mark and sweep MUC member list before changing any member affiliations (#1222857)
 * Change room owner to only configure MUCs that EnDroid owns (#1222858)

In addition:

 * As a result of moving MUC affiliation changes to the roomowner plugin, the functions are removed from usermanagement that performed these actions.
 * Now that affiliation is only changed once EnDroid has joined the room the old 'deferreds' list is no longer required so that is removed
 * Correct the error handling in configure_room to use trap correctly.

The following APIs are added to UM:

 * room_modify_affiliation
 * room_memberlist_change
 * is_room_owner
 * get_configured_room_memberlist
 * get_room_memberlist

It is noted that these are room-centric API's on an object/class called 'usermanagement'. Though there is precedence here (e.g. configure_room) this isn't ideal. Open to suggestions (blueprints) on how to re-org.

Note: this goes some way to implementing BP advanced-roomowner however it falls short in a few key areas:

 * MUC memberlist config is being read by both core EnDroid and roomowner (and indeed can be specified in both config sections).
 * EnDroid core is still joing all rooms and roomowner assumes it owns all rooms specified in config.
 * No additional support for non-standard MUC implementations (and supported config).

[Martin Morrison (isoschiz)]

get_room_memberlist and is_room_owner should guarantee to return a Deferred, rather than returning None if bad args are given. Using defer.fail would make sense here.

I think owner_result and parse_memberlist in the RoomOwner plugin should be inner funcs as they are just locally used callbacks (this would match the pattn used elsewhere in Enadroid, including further down in this diff).

review: Needs Fixing

[lp:~gingerchris/endroid/roomownerfixes]

62. By Chris Davidson <email address hidden> on 2013-09-10

Markups

63. By Chris Davidson <email address hidden> on 2013-09-10

Windows mount commit

[ChrisD (gingerchris)]

> get_room_memberlist and is_room_owner should guarantee to return a Deferred,
> rather than returning None if bad args are given. Using defer.fail would make
> sense here.

Done.

>
> I think owner_result and parse_memberlist in the RoomOwner plugin should be
> inner funcs as they are just locally used callbacks (this would match the
> pattn used elsewhere in Enadroid, including further down in this diff).

k. Have basically reworked the methods here in the process:
 * Made private
 * Daisy changed better
 * Split out to make a bit easier to read

Let me know if that works.

[Martin Morrison (isoschiz)]

review: Approve

=== modified file 'src/endroid/plugins/roomowner.py'
--- src/endroid/plugins/roomowner.py	2013-07-31 15:39:20 +0000
+++ src/endroid/plugins/roomowner.py	2013-09-10 15:35:09 +0000
@@ -1,9 +1,65 @@
+import logging
 from endroid.pluginmanager import Plugin
 
+
 class RoomOwner(Plugin):
+
     def endroid_init(self):
-        if self.place == "room":
-            # note that configure_room internally sanitises anything it gets
-            # - allowing only predefined keys and ignoring the rest (so giving it
+        if self.place != "room":
+            return
+
+        def owner_result(is_owner):
+            if not is_owner:
+                logging.error("EnDroid doesn't appear to be the owner for "
+                              "room {}".format(self.place_name))
+                return
+
+            logging.debug("EnDroid owns room {}".format(self.place_name))
+
+            self._update_memberlist()
+
+            # Apply the config
+            # @@@Config mark and sweep should be added here
+            # configure_room internally sanitises anything it gets
+            # allowing only predefined keys and ignoring the rest (so giving it
             # our whole .vars dictionary is safe (and easy))
-            self.usermanagement.configure_room(self.place_name, self.vars)
+            self.usermanagement.configure_room(self.place_name, self.vars)            
+
+        # First check if EnDroid owns this room
+        d = self.usermanagement.is_room_owner(self.place_name)
+        d.addCallback(owner_result)
+
+    def _get_configured_memberlist(self):
+        if 'users' in self.vars:
+            memberlist = set(self.vars['users'])
+        else:
+            # Check for old-style config
+            memberlist = self.usermanagement.get_configured_room_memberlist(
+                self.place_name)
+
+        return memberlist
+
+    def _update_memberlist(self):
+        """Update the member list of this room based on config."""
+
+        def parse_memberlist(memberlist):
+            current_memberlist = set(memberlist)
+            conf_memberlist = self._get_configured_memberlist()
+                
+            logging.debug('Configured memberlist for room {}: {}'.format(
+                          self.place_name, ", ".join(conf_memberlist)))
+            logging.debug('Current memberlist for room {}: {}'.format(
+                          self.place_name, ", ".join(current_memberlist)))
+
+            new_members = conf_memberlist - current_memberlist
+            dead_members = current_memberlist - conf_memberlist
+            if new_members:
+                self.usermanagement.room_memberlist_change(
+                    self.place_name, new_members)
+            if dead_members:
+                self.usermanagement.room_memberlist_change(
+                    self.place_name, dead_members, remove=True)
+
+        # Get the current memberlist to determine what changes need to be made
+        d = self.usermanagement.get_room_memberlist(self.place_name)
+        d.addCallback(parse_memberlist)

=== modified file 'src/endroid/usermanagement.py'
--- src/endroid/usermanagement.py	2013-09-06 10:06:52 +0000
+++ src/endroid/usermanagement.py	2013-09-10 15:35:09 +0000
@@ -174,9 +174,6 @@
         # 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):
@@ -276,9 +273,7 @@
             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)
+            self.room_rosters[room] = Room(room, nick, password, self)
             self.room_rosters[room].set_registration_list(users)
 
         for group in config.get("setup", "groups", default=['all']):
@@ -494,58 +489,6 @@
         self.rh.removeItem(name)  # sets the subscription to None both ways
         logging.info("Removed {} from {}".format(name, "contacts"))
 
-    # callbacks for adding/removing from rooms - can only be called after we are
-    # in the room ie if room is in self._wh.rooms. otherwise append the callback
-    # to a list to be called when we join (from userJoinedRoom in wokkelhandler)
-    def _cb_add_room_contact(self, name, room):
-        fmt_add = "Added {} to {}"
-        fmt_fail = "Failed to add {} to {}"
-
-        if room in self.wh._rooms:
-            d = self.wh.modifyAffiliationList(JID(room), [JID(name)], "member")
-            d.addCallback(lambda _: logging.info(fmt_add.format(name, room)))
-            d.addErrback(lambda _: logging.info(fmt_fail.format(name, room)))
-            logging.debug("Adding {} to {}".format(name, room))
-            return d
-        else:
-            self.room_rosters[room].deferreds.append((name, 1))
-
-    def _cb_rem_room_contact(self, name, room):
-        fmt_rem = "Removed {} from {}"
-        fmt_fail = "Failed to remove {} from {}"
-
-        if room in self.wh._rooms:
-            d = self.wh.modifyAffiliationList(JID(room), [JID(name)], "none")
-            d.addCallback(lambda _: logging.info(fmt_rem.format(name, room)))
-            d.addErrback(lambda _: logging.info(fmt_fail.format(name, room)))
-            logging.debug("Removing {} from {}".format(name, room))
-            return d
-        else:
-            self.room_rosters[room].deferreds.append((name, 0))
-
-    def _do_room_contacts(self, name):
-        members, nones = [], []
-        for (user, add) in self.room_rosters[name].deferreds:
-            if add:
-                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 {}"
-
-        if members:
-            string = fmt_add.format(', '.join(members), name)
-            d1 = self.wh.modifyAffiliationList(JID(name), map(JID, members), "member")
-            d1.addCallback(lambda _: logging.info(string))
-        if nones:
-            string = fmt_rem.format(', '.join(nones), name)
-            d2 = self.wh.modifyAffiliationList(JID(name), map(JID, nones), "none")
-            d2.addCallback(lambda _: logging.info(string))
-
-
     ### Room functions
 
     # @@@ at the moment this assumes that if we failed to join a room it was because
@@ -590,7 +533,6 @@
 
     def joined_room(self, name):
         logging.info("Joined room {}".format(name))
-        self._do_room_contacts(name)
         self._rooms.set_available(JID(name))
         if name not in self._pms:
             self.start_pm(None, "room", name)
@@ -617,10 +559,38 @@
             d = self.join_room(room)
             d.addCallback(lambda _, r: self.joined_room(r), room)
 
+    # @@@ This function is in the wrong place. This is the room section
     def join_all_groups(self):
         for group in self.get_groups():
             self.joined_group(group)
 
+    def room_modify_affiliation(self, room, users, affiliation):
+        """Update the affiliation list for this room."""
+
+        # Check that only EnDroid's contacts can be added to rooms
+        user_set = set(users)
+        un_reg_users = user_set - self.users()
+        if len(un_reg_users) > 0 and affiliation != 'none':
+            logging.warning('Attempt to allow {} unregistered user(s) '
+                            'access to {}'.format(len(un_reg_users), room))
+
+        allowed_users = user_set & self.users()
+        
+        jids = [JID(user) for user in allowed_users]
+        d = self.wh.modifyAffiliationList(JID(room), jids, affiliation)
+        d.addCallback(lambda _: logging.info("Modified affiliation for "
+                                            "room {}.".format(room)))
+        d.addErrback(lambda _: logging.error("Failed to modify room {}".format(
+                                             room)))
+        logging.debug("Changing affiliation for {} users in room {} to "
+                      "{}".format(len(users), room, affiliation))
+        return d
+
+    def room_memberlist_change(self, room, users, remove=False):
+        if remove:
+            return self.room_modify_affiliation(room, users, 'none')
+        else:
+            return self.room_modify_affiliation(room, users, 'member')
 
     def configure_room(self, name, options):
         """
@@ -630,21 +600,22 @@
         description.
 
         """
-        def cb(value, name):
-            try:
-                failure = value.trap(StanzaError)
-                if failure == StanzaError:
-                    return "Configured {}: not all options set".format(name)
-            except:
-                return "Configured {}".format(name)
+        def conf_failed(failure):
+            failure.trap(StanzaError)
+            logging.error("Failure configuring {}: code {}".format(name,
+                          failure.value.code))
+
+        def cb(value):
+            logging.info("Configured {}".format(name))
 
         mucoptions = Room.config_default.copy()
         # options is a human-readable dict, we intersect with Room.config_options
         # and translate into muc#roomconfig_xxx terms in translate_options
         mucoptions.update(Room.translate_options(options))
         d = self.wh.configure(JID(name), mucoptions)
-        d.addBoth(cb, name)
-        return d.addCallback(logging.info)
+        d.addCallback(cb)
+        d.addErrback(conf_failed)
+        return d
 
     def get_configuration(self, name):
         """
@@ -660,6 +631,71 @@
             d = self.wh.getConfiguration(JID(name))
             return d.addCallback(form_to_string)
 
+    def is_room_owner(self, room):
+        """Find out if EnDroid is an owner for the specified room. """
+
+        def determine_if_owner(owner_list):
+            """
+            Search the list of AdminItems to see if EnDroid's JID is there.
+
+            Should resource be listened to here?
+
+            """
+
+            return self.jid_obj.userhost() in (owner.entity.userhost() 
+                                               for owner in owner_list)
+
+        def error_determining_ownership(failure):
+            # Re-raise the failure if its not a StanzaError
+            failure.trap(StanzaError)
+            
+            if failure.value.code == '403':
+                # This is the forbidden code - so no EnDroid doesn't own the
+                # room
+                return False
+            else:
+                # If it's any other error let the caller deal with it
+                logging.error("Error determining ownership of {}. Code "
+                              "{}".format(room, failure.value.code))
+                return failure
+
+        if room in self.get_rooms():
+            return self.wh.getOwnerList(JID(room)).addCallbacks(
+                determine_if_owner, error_determining_ownership)
+        else:
+            # Always return a deferred
+            return twisted.internet.defer.fail()
+
+    def get_configured_room_memberlist(self, room):
+        """
+        Return the list (if any) of configured members for this room.
+
+        This is to support old style config where the user list was owned by
+        EnDroid core.
+
+        """
+        return self._allowed_users('room', room)
+
+    def get_room_memberlist(self, room):
+        """
+        Get the member list for a room.
+
+        Note: EnDroid must be room owner for this to work.
+
+        """
+
+        def convert_admin_list(member_list):
+            """Convert the list of AdminItems into userhosts. """
+
+            return [member.entity.userhost() for member in member_list]
+
+        if room in self.get_rooms():
+            return self.wh.getMemberList(JID(room)).addCallback(
+                convert_admin_list)
+        else:
+            # Always return a deferred
+            return twisted.internet.defer.fail()
+
     def invite(self, user, room, reason=None):
         """
         Invite a user to a room.