Merge pull request #501 from matrix-org/daniel/unban

Require unbanning before other membership changes

6 files changed, 116 insertions, 58 deletions

diff --git a/synapse/api/errors.py b/synapse/api/errors.py
index ce0fc53668..b106fbed6d 100644
--- a/synapse/api/errors.py
+++ b/synapse/api/errors.py
@@ -29,6 +29,7 @@ class Codes(object):
     USER_IN_USE = "M_USER_IN_USE"
     ROOM_IN_USE = "M_ROOM_IN_USE"
     BAD_PAGINATION = "M_BAD_PAGINATION"
+    BAD_STATE = "M_BAD_STATE"
     UNKNOWN = "M_UNKNOWN"
     NOT_FOUND = "M_NOT_FOUND"
     MISSING_TOKEN = "M_MISSING_TOKEN"
diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index 2f6359c768..26402ea9cd 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -1693,7 +1693,7 @@ class FederationHandler(BaseHandler):
             self.auth.check(event, context.current_state)
             yield self._validate_keyserver(event, auth_events=context.current_state)
             member_handler = self.hs.get_handlers().room_member_handler
-            yield member_handler.change_membership(event, context)
+            yield member_handler.send_membership_event(event, context)
         else:
             destinations = set([x.split(":", 1)[-1] for x in (sender, room_id)])
             yield self.replication_layer.forward_third_party_invite(
@@ -1722,7 +1722,7 @@ class FederationHandler(BaseHandler):
         # TODO: Make sure the signatures actually are correct.
         event.signatures.update(returned_invite.signatures)
         member_handler = self.hs.get_handlers().room_member_handler
-        yield member_handler.change_membership(event, context)
+        yield member_handler.send_membership_event(event, context)
 
     @defer.inlineCallbacks
     def add_display_name_to_third_party_invite(self, event_dict, event, context):
diff --git a/synapse/handlers/message.py b/synapse/handlers/message.py
index 5805190ce8..4c7bf2bef3 100644
--- a/synapse/handlers/message.py
+++ b/synapse/handlers/message.py
@@ -174,30 +174,25 @@ class MessageHandler(BaseHandler):
         defer.returnValue(chunk)
 
     @defer.inlineCallbacks
-    def create_and_send_event(self, event_dict, ratelimit=True,
-                              token_id=None, txn_id=None, is_guest=False):
-        """ Given a dict from a client, create and handle a new event.
+    def create_event(self, event_dict, token_id=None, txn_id=None):
+        """
+        Given a dict from a client, create a new event.
 
         Creates an FrozenEvent object, filling out auth_events, prev_events,
         etc.
 
         Adds display names to Join membership events.
 
-        Persists and notifies local clients and federation.
-
         Args:
             event_dict (dict): An entire event
+
+        Returns:
+            Tuple of created event (FrozenEvent), Context
         """
         builder = self.event_builder_factory.new(event_dict)
 
         self.validator.validate_new(builder)
 
-        if ratelimit:
-            self.ratelimit(builder.user_id)
-        # TODO(paul): Why does 'event' not have a 'user' object?
-        user = UserID.from_string(builder.user_id)
-        assert self.hs.is_mine(user), "User must be our own: %s" % (user,)
-
         if builder.type == EventTypes.Member:
             membership = builder.content.get("membership", None)
             if membership == Membership.JOIN:
@@ -216,6 +211,25 @@ class MessageHandler(BaseHandler):
         event, context = yield self._create_new_client_event(
             builder=builder,
         )
+        defer.returnValue((event, context))
+
+    @defer.inlineCallbacks
+    def send_event(self, event, context, ratelimit=True, is_guest=False):
+        """
+        Persists and notifies local clients and federation of an event.
+
+        Args:
+            event (FrozenEvent) the event to send.
+            context (Context) the context of the event.
+            ratelimit (bool): Whether to rate limit this send.
+            is_guest (bool): Whether the sender is a guest.
+        """
+        user = UserID.from_string(event.sender)
+
+        assert self.hs.is_mine(user), "User must be our own: %s" % (user,)
+
+        if ratelimit:
+            self.ratelimit(event.sender)
 
         if event.is_state():
             prev_state = context.current_state.get((event.type, event.state_key))
@@ -229,7 +243,7 @@ class MessageHandler(BaseHandler):
 
         if event.type == EventTypes.Member:
             member_handler = self.hs.get_handlers().room_member_handler
-            yield member_handler.change_membership(event, context, is_guest=is_guest)
+            yield member_handler.send_membership_event(event, context, is_guest=is_guest)
         else:
             yield self.handle_new_client_event(
                 event=event,
@@ -241,6 +255,25 @@ class MessageHandler(BaseHandler):
             with PreserveLoggingContext():
                 presence.bump_presence_active_time(user)
 
+    @defer.inlineCallbacks
+    def create_and_send_event(self, event_dict, ratelimit=True,
+                              token_id=None, txn_id=None, is_guest=False):
+        """
+        Creates an event, then sends it.
+
+        See self.create_event and self.send_event.
+        """
+        event, context = yield self.create_event(
+            event_dict,
+            token_id=token_id,
+            txn_id=txn_id
+        )
+        yield self.send_event(
+            event,
+            context,
+            ratelimit=ratelimit,
+            is_guest=is_guest
+        )
         defer.returnValue(event)
 
     @defer.inlineCallbacks
diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py
index a410e4394c..a1baf9d200 100644
--- a/synapse/handlers/room.py
+++ b/synapse/handlers/room.py
@@ -22,7 +22,7 @@ from synapse.types import UserID, RoomAlias, RoomID
 from synapse.api.constants import (
     EventTypes, Membership, JoinRules, RoomCreationPreset,
 )
-from synapse.api.errors import AuthError, StoreError, SynapseError
+from synapse.api.errors import AuthError, StoreError, SynapseError, Codes
 from synapse.util import stringutils, unwrapFirstError
 from synapse.util.async import run_on_reactor
 
@@ -397,7 +397,58 @@ class RoomMemberHandler(BaseHandler):
                     remotedomains.add(member.domain)
 
     @defer.inlineCallbacks
-    def change_membership(self, event, context, is_guest=False):
+    def update_membership(self, requester, target, room_id, action, txn_id=None):
+        effective_membership_state = action
+        if action in ["kick", "unban"]:
+            effective_membership_state = "leave"
+        elif action == "forget":
+            effective_membership_state = "leave"
+
+        msg_handler = self.hs.get_handlers().message_handler
+
+        content = {"membership": unicode(effective_membership_state)}
+        if requester.is_guest:
+            content["kind"] = "guest"
+
+        event, context = yield msg_handler.create_event(
+            {
+                "type": EventTypes.Member,
+                "content": content,
+                "room_id": room_id,
+                "sender": requester.user.to_string(),
+                "state_key": target.to_string(),
+            },
+            token_id=requester.access_token_id,
+            txn_id=txn_id,
+        )
+
+        old_state = context.current_state.get((EventTypes.Member, event.state_key))
+        old_membership = old_state.content.get("membership") if old_state else None
+        if action == "unban" and old_membership != "ban":
+            raise SynapseError(
+                403,
+                "Cannot unban user who was not banned (membership=%s)" % old_membership,
+                errcode=Codes.BAD_STATE
+            )
+        if old_membership == "ban" and action != "unban":
+            raise SynapseError(
+                403,
+                "Cannot %s user who was is banned" % (action,),
+                errcode=Codes.BAD_STATE
+            )
+
+        yield msg_handler.send_event(
+            event,
+            context,
+            ratelimit=True,
+            is_guest=requester.is_guest
+        )
+
+        if action == "forget":
+            yield self.forget(requester.user, room_id)
+
+    @defer.inlineCallbacks
+    def send_membership_event(self, event, context, is_guest=False):
         """ Change the membership status of a user in a room.
 
         Args:
diff --git a/synapse/rest/client/v1/room.py b/synapse/rest/client/v1/room.py
index 8b1b2b852d..85b9f253e3 100644
--- a/synapse/rest/client/v1/room.py
+++ b/synapse/rest/client/v1/room.py
@@ -442,7 +442,7 @@ class RoomMembershipRestServlet(ClientV1RestServlet):
     def register(self, http_server):
         # /rooms/$roomid/[invite|join|leave]
         PATTERNS = ("/rooms/(?P<room_id>[^/]*)/"
-                    "(?P<membership_action>join|invite|leave|ban|kick|forget)")
+                    "(?P<membership_action>join|invite|leave|ban|unban|kick|forget)")
         register_txn_path(self, PATTERNS, http_server)
 
     @defer.inlineCallbacks
@@ -451,9 +451,6 @@ class RoomMembershipRestServlet(ClientV1RestServlet):
             request,
             allow_guest=True,
         )
-        user = requester.user
-
-        effective_membership_action = membership_action
 
         if requester.is_guest and membership_action not in {
             Membership.JOIN,
@@ -463,13 +460,10 @@ class RoomMembershipRestServlet(ClientV1RestServlet):
 
         content = _parse_json(request)
 
-        # target user is you unless it is an invite
-        state_key = user.to_string()
-
         if membership_action == "invite" and self._has_3pid_invite_keys(content):
             yield self.handlers.room_member_handler.do_3pid_invite(
                 room_id,
-                user,
+                requester.user,
                 content["medium"],
                 content["address"],
                 content["id_server"],
@@ -478,42 +472,21 @@ class RoomMembershipRestServlet(ClientV1RestServlet):
             )
             defer.returnValue((200, {}))
             return
-        elif membership_action in ["invite", "ban", "kick"]:
-            if "user_id" in content:
-                state_key = content["user_id"]
-            else:
-                raise SynapseError(400, "Missing user_id key.")
-
-            # make sure it looks like a user ID; it'll throw if it's invalid.
-            UserID.from_string(state_key)
 
-            if membership_action == "kick":
-                effective_membership_action = "leave"
-        elif membership_action == "forget":
-            effective_membership_action = "leave"
-
-        msg_handler = self.handlers.message_handler
-
-        content = {"membership": unicode(effective_membership_action)}
-        if requester.is_guest:
-            content["kind"] = "guest"
+        target = requester.user
+        if membership_action in ["invite", "ban", "unban", "kick"]:
+            if "user_id" not in content:
+                raise SynapseError(400, "Missing user_id key.")
+            target = UserID.from_string(content["user_id"])
 
-        yield msg_handler.create_and_send_event(
-            {
-                "type": EventTypes.Member,
-                "content": content,
-                "room_id": room_id,
-                "sender": user.to_string(),
-                "state_key": state_key,
-            },
-            token_id=requester.access_token_id,
+        yield self.handlers.room_member_handler.update_membership(
+            requester=requester,
+            target=target,
+            room_id=room_id,
+            action=membership_action,
             txn_id=txn_id,
-            is_guest=requester.is_guest,
         )
 
-        if membership_action == "forget":
-            yield self.handlers.room_member_handler.forget(user, room_id)
-
         defer.returnValue((200, {}))
 
     def _has_3pid_invite_keys(self, content):
diff --git a/tests/handlers/test_room.py b/tests/handlers/test_room.py
index 97491848a3..e7a12a2ba2 100644
--- a/tests/handlers/test_room.py
+++ b/tests/handlers/test_room.py
@@ -156,7 +156,7 @@ class RoomMemberHandlerTestCase(unittest.TestCase):
             builder
         )
 
-        yield room_handler.change_membership(event, context)
+        yield room_handler.send_membership_event(event, context)
 
         self.state_handler.compute_event_context.assert_called_once_with(
             builder
@@ -232,7 +232,7 @@ class RoomMemberHandlerTestCase(unittest.TestCase):
         )
 
         # Actual invocation
-        yield room_handler.change_membership(event, context)
+        yield room_handler.send_membership_event(event, context)
 
         self.federation.handle_new_event.assert_called_once_with(
             event, destinations=set()
@@ -312,7 +312,7 @@ class RoomMemberHandlerTestCase(unittest.TestCase):
         self.distributor.observe("user_left_room", leave_signal_observer)
 
         # Actual invocation
-        yield room_handler.change_membership(event, context)
+        yield room_handler.send_membership_event(event, context)
 
         self.federation.handle_new_event.assert_called_once_with(
             event, destinations=set(['red'])