Merge pull request #4875 from matrix-org/erikj/spam_checker dinsic_2019-03-20

Extend spam checking rules

6 files changed, 326 insertions, 43 deletions

diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py
index 633e068eb8..e4fc988cfc 100644
--- a/synapse/events/spamcheck.py
+++ b/synapse/events/spamcheck.py
@@ -46,13 +46,18 @@ class SpamChecker(object):
 
         return self.spam_checker.check_event_for_spam(event)
 
-    def user_may_invite(self, inviter_userid, invitee_userid, room_id):
+    def user_may_invite(self, inviter_userid, invitee_userid, room_id, new_room):
         """Checks if a given user may send an invite
 
         If this method returns false, the invite will be rejected.
 
         Args:
-            userid (string): The sender's user ID
+            inviter_userid (str)
+            invitee_userid (str)
+            room_id (str)
+            new_room (bool): Wether the user is being invited to the room as
+                part of a room creation, if so the invitee would have been
+                included in the call to `user_may_create_room`.
 
         Returns:
             bool: True if the user may send an invite, otherwise False
@@ -60,15 +65,21 @@ class SpamChecker(object):
         if self.spam_checker is None:
             return True
 
-        return self.spam_checker.user_may_invite(inviter_userid, invitee_userid, room_id)
+        return self.spam_checker.user_may_invite(
+            inviter_userid, invitee_userid, room_id, new_room,
+        )
 
-    def user_may_create_room(self, userid):
+    def user_may_create_room(self, userid, invite_list, cloning):
         """Checks if a given user may create a room
 
         If this method returns false, the creation request will be rejected.
 
         Args:
             userid (string): The sender's user ID
+            invite_list (list[str]): List of user IDs that would be invited to
+                the new room.
+            cloning (bool): Whether the user is cloning an existing room, e.g.
+                upgrading a room.
 
         Returns:
             bool: True if the user may create a room, otherwise False
@@ -76,7 +87,7 @@ class SpamChecker(object):
         if self.spam_checker is None:
             return True
 
-        return self.spam_checker.user_may_create_room(userid)
+        return self.spam_checker.user_may_create_room(userid, invite_list, cloning)
 
     def user_may_create_room_alias(self, userid, room_alias):
         """Checks if a given user may create a room alias
@@ -111,3 +122,21 @@ class SpamChecker(object):
             return True
 
         return self.spam_checker.user_may_publish_room(userid, room_id)
+
+    def user_may_join_room(self, userid, room_id, is_invited):
+        """Checks if a given users is allowed to join a room.
+
+        Is not called when the user creates a room.
+
+        Args:
+            userid (str)
+            room_id (str)
+            is_invited (bool): Whether the user is invited into the room
+
+        Returns:
+            bool: Whether the user may join the room
+        """
+        if self.spam_checker is None:
+            return True
+
+        return self.spam_checker.user_may_join_room(userid, room_id, is_invited)
diff --git a/synapse/handlers/federation.py b/synapse/handlers/federation.py
index f80486102a..a222d67190 100644
--- a/synapse/handlers/federation.py
+++ b/synapse/handlers/federation.py
@@ -1299,7 +1299,7 @@ class FederationHandler(BaseHandler):
             raise SynapseError(403, "This server does not accept room invites")
 
         if not self.spam_checker.user_may_invite(
-            event.sender, event.state_key, event.room_id,
+            event.sender, event.state_key, event.room_id, new_room=False,
         ):
             raise SynapseError(
                 403, "This user is not permitted to send invites to this server/user"
diff --git a/synapse/handlers/room.py b/synapse/handlers/room.py
index eb4b437ce8..581cff9526 100644
--- a/synapse/handlers/room.py
+++ b/synapse/handlers/room.py
@@ -81,6 +81,8 @@ class RoomCreationHandler(BaseHandler):
         # linearizer to stop two upgrades happening at once
         self._upgrade_linearizer = Linearizer("room_upgrade_linearizer")
 
+        self._server_notices_mxid = hs.config.server_notices_mxid
+
     @defer.inlineCallbacks
     def upgrade_room(self, requester, old_room_id, new_version):
         """Replace a room with a new room with a different version
@@ -254,7 +256,21 @@ class RoomCreationHandler(BaseHandler):
         """
         user_id = requester.user.to_string()
 
-        if not self.spam_checker.user_may_create_room(user_id):
+        if (self._server_notices_mxid is not None and
+                requester.user.to_string() == self._server_notices_mxid):
+            # allow the server notices mxid to create rooms
+            is_requester_admin = True
+
+        else:
+            is_requester_admin = yield self.auth.is_server_admin(
+                requester.user,
+            )
+
+        if not is_requester_admin and not self.spam_checker.user_may_create_room(
+            user_id,
+            invite_list=[],
+            cloning=True,
+        ):
             raise SynapseError(403, "You are not permitted to create rooms")
 
         creation_content = {
@@ -475,7 +491,22 @@ class RoomCreationHandler(BaseHandler):
 
         yield self.auth.check_auth_blocking(user_id)
 
-        if not self.spam_checker.user_may_create_room(user_id):
+        invite_list = config.get("invite", [])
+
+        if (self._server_notices_mxid is not None and
+                requester.user.to_string() == self._server_notices_mxid):
+            # allow the server notices mxid to create rooms
+            is_requester_admin = True
+        else:
+            is_requester_admin = yield self.auth.is_server_admin(
+                requester.user,
+            )
+
+        if not is_requester_admin and not self.spam_checker.user_may_create_room(
+            user_id,
+            invite_list=invite_list,
+            cloning=False,
+        ):
             raise SynapseError(403, "You are not permitted to create rooms")
 
         if ratelimit:
@@ -518,7 +549,6 @@ class RoomCreationHandler(BaseHandler):
         else:
             room_alias = None
 
-        invite_list = config.get("invite", [])
         for i in invite_list:
             try:
                 UserID.from_string(i)
@@ -615,6 +645,7 @@ class RoomCreationHandler(BaseHandler):
                 "invite",
                 ratelimit=False,
                 content=content,
+                new_room=True,
             )
 
         for invite_3pid in invite_3pid_list:
@@ -699,6 +730,7 @@ class RoomCreationHandler(BaseHandler):
             "join",
             ratelimit=False,
             content=creator_join_profile,
+            new_room=True,
         )
 
         # We treat the power levels override specially as this needs to be one
diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py
index 190ea2c7b1..dcf30327cd 100644
--- a/synapse/handlers/room_member.py
+++ b/synapse/handlers/room_member.py
@@ -301,7 +301,30 @@ class RoomMemberHandler(object):
             third_party_signed=None,
             ratelimit=True,
             content=None,
+            new_room=False,
     ):
+        """Update a users membership in a room
+
+        Args:
+            requester (Requester)
+            target (UserID)
+            room_id (str)
+            action (str): The "action" the requester is performing against the
+                target. One of join/leave/kick/ban/invite/unban.
+            txn_id (str|None): The transaction ID associated with the request,
+                or None not provided.
+            remote_room_hosts (list[str]|None): List of remote servers to try
+                and join via if server isn't already in the room.
+            third_party_signed (dict|None): The signed object for third party
+                invites.
+            ratelimit (bool): Whether to apply ratelimiting to this request.
+            content (dict|None): Fields to include in the new events content.
+            new_room (bool): Whether these membership changes are happening
+                as part of a room creation (e.g. initial joins and invites)
+
+        Returns:
+            Deferred[FrozenEvent]
+        """
         key = (room_id,)
 
         with (yield self.member_linearizer.queue(key)):
@@ -315,6 +338,7 @@ class RoomMemberHandler(object):
                 third_party_signed=third_party_signed,
                 ratelimit=ratelimit,
                 content=content,
+                new_room=new_room,
             )
 
         defer.returnValue(result)
@@ -331,6 +355,7 @@ class RoomMemberHandler(object):
             third_party_signed=None,
             ratelimit=True,
             content=None,
+            new_room=False,
     ):
         content_specified = bool(content)
         if content is None:
@@ -392,6 +417,7 @@ class RoomMemberHandler(object):
 
                 if not self.spam_checker.user_may_invite(
                     requester.user.to_string(), target.to_string(), room_id,
+                    new_room=new_room,
                 ):
                     logger.info("Blocking invite due to spam checker")
                     block_invite = True
@@ -461,8 +487,29 @@ class RoomMemberHandler(object):
                     # so don't really fit into the general auth process.
                     raise AuthError(403, "Guest access not allowed")
 
+            if (self._server_notices_mxid is not None and
+                    requester.user.to_string() == self._server_notices_mxid):
+                # allow the server notices mxid to join rooms
+                is_requester_admin = True
+
+            else:
+                is_requester_admin = yield self.auth.is_server_admin(
+                    requester.user,
+                )
+
+            inviter = yield self._get_inviter(target.to_string(), room_id)
+            if not is_requester_admin:
+                # We assume that if the spam checker allowed the user to create
+                # a room then they're allowed to join it.
+                if not new_room and not self.spam_checker.user_may_join_room(
+                    target.to_string(), room_id,
+                    is_invited=inviter is not None,
+                ):
+                    raise SynapseError(
+                        403, "Not allowed to join this room",
+                    )
+
             if not is_host_in_room:
-                inviter = yield self._get_inviter(target.to_string(), room_id)
                 if inviter and not self.hs.is_mine(inviter):
                     remote_room_hosts.append(inviter.domain)
 
diff --git a/synapse/rulecheck/domain_rule_checker.py b/synapse/rulecheck/domain_rule_checker.py
index 3caa6b34cb..410757041b 100644
--- a/synapse/rulecheck/domain_rule_checker.py
+++ b/synapse/rulecheck/domain_rule_checker.py
@@ -34,7 +34,17 @@ class DomainRuleChecker(object):
             "inviter_domain": [ "invitee_domain_permitted", "other_domain_permitted" ]
             "other_inviter_domain": [ "invitee_domain_permitted" ]
           default: False
-        }
+
+          # Only let local users join rooms if they were explicitly invited.
+          can_only_join_rooms_with_invite: false
+
+          # Only let local users create rooms if they are inviting only one
+          # other user, and that user matches the rules above.
+          can_only_create_one_to_one_rooms: false
+
+          # Only let local users invite during room creation, regardless of the
+          # domain mapping rules above.
+          can_only_invite_during_room_creation: false
 
     Don't forget to consider if you can invite users from your own domain.
     """
@@ -43,14 +53,28 @@ class DomainRuleChecker(object):
         self.domain_mapping = config["domain_mapping"] or {}
         self.default = config["default"]
 
+        self.can_only_join_rooms_with_invite = config.get(
+            "can_only_join_rooms_with_invite", False,
+        )
+        self.can_only_create_one_to_one_rooms = config.get(
+            "can_only_create_one_to_one_rooms", False,
+        )
+        self.can_only_invite_during_room_creation = config.get(
+            "can_only_invite_during_room_creation", False,
+        )
+
     def check_event_for_spam(self, event):
         """Implements synapse.events.SpamChecker.check_event_for_spam
         """
         return False
 
-    def user_may_invite(self, inviter_userid, invitee_userid, room_id):
+    def user_may_invite(self, inviter_userid, invitee_userid, room_id,
+                        new_room):
         """Implements synapse.events.SpamChecker.user_may_invite
         """
+        if self.can_only_invite_during_room_creation and not new_room:
+            return False
+
         inviter_domain = self._get_domain_from_id(inviter_userid)
         invitee_domain = self._get_domain_from_id(invitee_userid)
 
@@ -59,9 +83,16 @@ class DomainRuleChecker(object):
 
         return invitee_domain in self.domain_mapping[inviter_domain]
 
-    def user_may_create_room(self, userid):
+    def user_may_create_room(self, userid, invite_list, cloning):
         """Implements synapse.events.SpamChecker.user_may_create_room
         """
+
+        if cloning:
+            return True
+
+        if self.can_only_create_one_to_one_rooms and len(invite_list) != 1:
+            return False
+
         return True
 
     def user_may_create_room_alias(self, userid, room_alias):
@@ -74,6 +105,14 @@ class DomainRuleChecker(object):
         """
         return True
 
+    def user_may_join_room(self, userid, room_id, is_invited):
+        """Implements synapse.events.SpamChecker.user_may_join_room
+        """
+        if self.can_only_join_rooms_with_invite and not is_invited:
+            return False
+
+        return True
+
     @staticmethod
     def parse_config(config):
         """Implements synapse.events.SpamChecker.parse_config
diff --git a/tests/rulecheck/test_domainrulecheck.py b/tests/rulecheck/test_domainrulecheck.py
index 702862f78b..de89f95e3c 100644
--- a/tests/rulecheck/test_domainrulecheck.py
+++ b/tests/rulecheck/test_domainrulecheck.py
@@ -14,29 +14,35 @@
 # limitations under the License.
 
 
+import json
+
 from synapse.config._base import ConfigError
+from synapse.rest.client.v1 import admin, login, room
 from synapse.rulecheck.domain_rule_checker import DomainRuleChecker
 
 from tests import unittest
+from tests.server import make_request, render
 
 
 class DomainRuleCheckerTestCase(unittest.TestCase):
-
     def test_allowed(self):
         config = {
             "default": False,
             "domain_mapping": {
                 "source_one": ["target_one", "target_two"],
-                "source_two": ["target_two"]
-            }
+                "source_two": ["target_two"],
+            },
         }
         check = DomainRuleChecker(config)
-        self.assertTrue(check.user_may_invite("test:source_one",
-                                              "test:target_one", "room"))
-        self.assertTrue(check.user_may_invite("test:source_one",
-                                              "test:target_two", "room"))
-        self.assertTrue(check.user_may_invite("test:source_two",
-                                              "test:target_two", "room"))
+        self.assertTrue(
+            check.user_may_invite("test:source_one", "test:target_one", "room", False)
+        )
+        self.assertTrue(
+            check.user_may_invite("test:source_one", "test:target_two", "room", False)
+        )
+        self.assertTrue(
+            check.user_may_invite("test:source_two", "test:target_two", "room", False)
+        )
 
     def test_disallowed(self):
         config = {
@@ -44,50 +50,56 @@ class DomainRuleCheckerTestCase(unittest.TestCase):
             "domain_mapping": {
                 "source_one": ["target_one", "target_two"],
                 "source_two": ["target_two"],
-                "source_four": []
-            }
+                "source_four": [],
+            },
         }
         check = DomainRuleChecker(config)
-        self.assertFalse(check.user_may_invite("test:source_one",
-                                               "test:target_three", "room"))
-        self.assertFalse(check.user_may_invite("test:source_two",
-                                               "test:target_three", "room"))
-        self.assertFalse(check.user_may_invite("test:source_two",
-                                               "test:target_one", "room"))
-        self.assertFalse(check.user_may_invite("test:source_four",
-                                               "test:target_one", "room"))
+        self.assertFalse(
+            check.user_may_invite("test:source_one", "test:target_three", "room", False)
+        )
+        self.assertFalse(
+            check.user_may_invite("test:source_two", "test:target_three", "room", False)
+        )
+        self.assertFalse(
+            check.user_may_invite("test:source_two", "test:target_one", "room", False)
+        )
+        self.assertFalse(
+            check.user_may_invite("test:source_four", "test:target_one", "room", False)
+        )
 
     def test_default_allow(self):
         config = {
             "default": True,
             "domain_mapping": {
                 "source_one": ["target_one", "target_two"],
-                "source_two": ["target_two"]
-            }
+                "source_two": ["target_two"],
+            },
         }
         check = DomainRuleChecker(config)
-        self.assertTrue(check.user_may_invite("test:source_three",
-                                              "test:target_one", "room"))
+        self.assertTrue(
+            check.user_may_invite("test:source_three", "test:target_one", "room", False)
+        )
 
     def test_default_deny(self):
         config = {
             "default": False,
             "domain_mapping": {
                 "source_one": ["target_one", "target_two"],
-                "source_two": ["target_two"]
-            }
+                "source_two": ["target_two"],
+            },
         }
         check = DomainRuleChecker(config)
-        self.assertFalse(check.user_may_invite("test:source_three",
-                                               "test:target_one", "room"))
+        self.assertFalse(
+            check.user_may_invite("test:source_three", "test:target_one", "room", False)
+        )
 
     def test_config_parse(self):
         config = {
             "default": False,
             "domain_mapping": {
                 "source_one": ["target_one", "target_two"],
-                "source_two": ["target_two"]
-            }
+                "source_two": ["target_two"],
+            },
         }
         self.assertEquals(config, DomainRuleChecker.parse_config(config))
 
@@ -95,7 +107,131 @@ class DomainRuleCheckerTestCase(unittest.TestCase):
         config = {
             "domain_mapping": {
                 "source_one": ["target_one", "target_two"],
-                "source_two": ["target_two"]
+                "source_two": ["target_two"],
             }
         }
         self.assertRaises(ConfigError, DomainRuleChecker.parse_config, config)
+
+
+class DomainRuleCheckerRoomTestCase(unittest.HomeserverTestCase):
+    servlets = [
+        admin.register_servlets,
+        room.register_servlets,
+        login.register_servlets,
+    ]
+
+    hijack_auth = False
+
+    def make_homeserver(self, reactor, clock):
+        config = self.default_config()
+
+        config.spam_checker = (DomainRuleChecker, {
+            "default": True,
+            "domain_mapping": {},
+            "can_only_join_rooms_with_invite": True,
+            "can_only_create_one_to_one_rooms": True,
+            "can_only_invite_during_room_creation": True,
+        })
+
+        hs = self.setup_test_homeserver(config=config)
+        return hs
+
+    def prepare(self, reactor, clock, hs):
+        self.admin_user_id = self.register_user("admin_user", "pass", admin=True)
+        self.admin_access_token = self.login("admin_user", "pass")
+
+        self.normal_user_id = self.register_user("normal_user", "pass", admin=False)
+        self.normal_access_token = self.login("normal_user", "pass")
+
+        self.other_user_id = self.register_user("other_user", "pass", admin=False)
+
+    def test_admin_can_create_room(self):
+        channel = self._create_room(self.admin_access_token)
+        assert channel.result["code"] == b"200", channel.result
+
+    def test_normal_user_cannot_create_empty_room(self):
+        channel = self._create_room(self.normal_access_token)
+        assert channel.result["code"] == b"403", channel.result
+
+    def test_normal_user_cannot_create_room_with_multiple_invites(self):
+        channel = self._create_room(self.normal_access_token, content={
+            "invite": [self.other_user_id, self.admin_user_id],
+        })
+        assert channel.result["code"] == b"403", channel.result
+
+    def test_normal_user_can_room_with_single_invites(self):
+        channel = self._create_room(self.normal_access_token, content={
+            "invite": [self.other_user_id],
+        })
+        assert channel.result["code"] == b"200", channel.result
+
+    def test_cannot_join_public_room(self):
+        channel = self._create_room(self.admin_access_token)
+        assert channel.result["code"] == b"200", channel.result
+
+        room_id = channel.json_body["room_id"]
+
+        self.helper.join(
+            room_id, self.normal_user_id,
+            tok=self.normal_access_token,
+            expect_code=403,
+        )
+
+    def test_can_join_invited_room(self):
+        channel = self._create_room(self.admin_access_token)
+        assert channel.result["code"] == b"200", channel.result
+
+        room_id = channel.json_body["room_id"]
+
+        self.helper.invite(
+            room_id,
+            src=self.admin_user_id,
+            targ=self.normal_user_id,
+            tok=self.admin_access_token,
+        )
+
+        self.helper.join(
+            room_id, self.normal_user_id,
+            tok=self.normal_access_token,
+            expect_code=200,
+        )
+
+    def test_cannot_invite(self):
+        channel = self._create_room(self.admin_access_token)
+        assert channel.result["code"] == b"200", channel.result
+
+        room_id = channel.json_body["room_id"]
+
+        self.helper.invite(
+            room_id,
+            src=self.admin_user_id,
+            targ=self.normal_user_id,
+            tok=self.admin_access_token,
+        )
+
+        self.helper.join(
+            room_id, self.normal_user_id,
+            tok=self.normal_access_token,
+            expect_code=200,
+        )
+
+        self.helper.invite(
+            room_id,
+            src=self.normal_user_id,
+            targ=self.other_user_id,
+            tok=self.normal_access_token,
+            expect_code=403,
+        )
+
+    def _create_room(self, token, content={}):
+        path = "/_matrix/client/r0/createRoom?access_token=%s" % (
+            token,
+        )
+
+        request, channel = make_request(
+            self.hs.get_reactor(), "POST", path,
+            content=json.dumps(content).encode("utf8"),
+        )
+        render(request, self.resource, self.hs.get_reactor())
+
+        return channel