diff --git a/synapse/events/spamcheck.py b/synapse/events/spamcheck.py index 633e068eb8d5..e4fc988cfc3e 100644 --- a/synapse/events/spamcheck.py +++ b/synapse/events/spamcheck.py @@ -46,13 +46,18 @@ def check_event_for_spam(self, event): 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 @@ def user_may_invite(self, inviter_userid, invitee_userid, room_id): 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 @@ def user_may_create_room(self, userid): 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 @@ def user_may_publish_room(self, userid, room_id): 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 f80486102ae2..a222d67190d6 100644 --- a/synapse/handlers/federation.py +++ b/synapse/handlers/federation.py @@ -1299,7 +1299,7 @@ def on_invite_request(self, origin, pdu): 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 eb4b437ce857..581cff9526d7 100644 --- a/synapse/handlers/room.py +++ b/synapse/handlers/room.py @@ -81,6 +81,8 @@ def __init__(self, hs): # 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 @@ def clone_existing_room( """ 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 @@ def create_room(self, requester, config, ratelimit=True, 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 @@ def create_room(self, requester, config, ratelimit=True, else: room_alias = None - invite_list = config.get("invite", []) for i in invite_list: try: UserID.from_string(i) @@ -615,6 +645,7 @@ def create_room(self, requester, config, ratelimit=True, "invite", ratelimit=False, content=content, + new_room=True, ) for invite_3pid in invite_3pid_list: @@ -699,6 +730,7 @@ def send(etype, content, **kwargs): "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 190ea2c7b14b..dcf30327cd01 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -301,7 +301,30 @@ def update_membership( 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 @@ def update_membership( third_party_signed=third_party_signed, ratelimit=ratelimit, content=content, + new_room=new_room, ) defer.returnValue(result) @@ -331,6 +355,7 @@ def _update_membership( third_party_signed=None, ratelimit=True, content=None, + new_room=False, ): content_specified = bool(content) if content is None: @@ -392,6 +417,7 @@ def _update_membership( 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 @@ def _update_membership( # 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 3caa6b34cb6c..410757041be9 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 @@ def __init__(self, config): 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 @@ def user_may_invite(self, inviter_userid, invitee_userid, room_id): 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 @@ def user_may_publish_room(self, userid, room_id): """ 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 702862f78b08..de89f95e3c77 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 @@ def test_disallowed(self): "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 @@ def test_config_parse_failure(self): 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