diff options
| -rw-r--r-- | changelog.d/5576.bugfix | 1 | ||||
| -rw-r--r-- | changelog.d/5610.feature | 1 | ||||
| -rw-r--r-- | synapse/handlers/room_member.py | 1 | ||||
| -rw-r--r-- | synapse/third_party_rules/access_rules.py | 154 | ||||
| -rw-r--r-- | tests/handlers/test_identity.py | 108 | ||||
| -rw-r--r-- | tests/rest/client/test_room_access_rules.py | 56 |
6 files changed, 283 insertions, 38 deletions
diff --git a/changelog.d/5576.bugfix b/changelog.d/5576.bugfix new file mode 100644 index 0000000000..c1ba5581f2 --- /dev/null +++ b/changelog.d/5576.bugfix @@ -0,0 +1 @@ +Fix a bug that would cause invited users to receive several emails for a single 3PID invite in case the inviter is rate limited. diff --git a/changelog.d/5610.feature b/changelog.d/5610.feature new file mode 100644 index 0000000000..b99514f97e --- /dev/null +++ b/changelog.d/5610.feature @@ -0,0 +1 @@ +Implement new custom event rules for power levels. diff --git a/synapse/handlers/room_member.py b/synapse/handlers/room_member.py index 1517907898..e940e4183b 100644 --- a/synapse/handlers/room_member.py +++ b/synapse/handlers/room_member.py @@ -935,6 +935,7 @@ class RoomMemberHandler(object): "sender": user.to_string(), "state_key": token, }, + ratelimit=False, txn_id=txn_id, ) diff --git a/synapse/third_party_rules/access_rules.py b/synapse/third_party_rules/access_rules.py index d13f8de888..c8698c66cc 100644 --- a/synapse/third_party_rules/access_rules.py +++ b/synapse/third_party_rules/access_rules.py @@ -33,6 +33,21 @@ VALID_ACCESS_RULES = ( ACCESS_RULE_UNRESTRICTED, ) +# Rules to which we need to apply the power levels restrictions. +# +# These are all of the rules that neither: +# * forbid users from joining based on a server blacklist (which means that there +# is no need to apply power level restrictions), nor +# * target direct chats (since we allow both users to be room admins in this case). +# +# The power-level restrictions, when they are applied, prevent the following: +# * the default power level for users (users_default) being set to anything other than 0. +# * a non-default power level being assigned to any user which would be forbidden from +# joining a restricted room. +RULES_WITH_RESTRICTED_POWER_LEVELS = ( + ACCESS_RULE_UNRESTRICTED, +) + class RoomAccessRules(object): """Implementation of the ThirdPartyEventRules module API that allows federation admins @@ -79,34 +94,32 @@ class RoomAccessRules(object): default rule to the initial state. """ is_direct = config.get("is_direct") - rules_in_initial_state = False + rule = None # If there's a rules event in the initial state, check if it complies with the # spec for im.vector.room.access_rules and deny the request if not. for event in config.get("initial_state", []): if event["type"] == ACCESS_RULES_TYPE: - rules_in_initial_state = True - rule = event["content"].get("rule") # Make sure the event has a valid content. if rule is None: - raise SynapseError(400, "Invalid access rule",) + raise SynapseError(400, "Invalid access rule") # Make sure the rule name is valid. if rule not in VALID_ACCESS_RULES: - raise SynapseError(400, "Invalid access rule", ) + raise SynapseError(400, "Invalid access rule") # Make sure the rule is "direct" if the room is a direct chat. if ( (is_direct and rule != ACCESS_RULE_DIRECT) or (rule == ACCESS_RULE_DIRECT and not is_direct) ): - raise SynapseError(400, "Invalid access rule",) + raise SynapseError(400, "Invalid access rule") # If there's no rules event in the initial state, create one with the default # setting. - if not rules_in_initial_state: + if not rule: if is_direct: default_rule = ACCESS_RULE_DIRECT else: @@ -123,6 +136,22 @@ class RoomAccessRules(object): } }) + rule = default_rule + + # Check if the creator can override values for the power levels. + allowed = self._is_power_level_content_allowed( + config.get("power_level_content_override", {}), rule, + ) + if not allowed: + raise SynapseError(400, "Invalid power levels content override") + + # Second loop for events we need to know the current rule to process. + for event in config.get("initial_state", []): + if event["type"] == EventTypes.PowerLevels: + allowed = self._is_power_level_content_allowed(event["content"], rule) + if not allowed: + raise SynapseError(400, "Invalid power levels content") + @defer.inlineCallbacks def check_threepid_can_be_invited(self, medium, address, state_events): """Implements synapse.events.ThirdPartyEventRules.check_threepid_can_be_invited @@ -172,24 +201,19 @@ class RoomAccessRules(object): Checks the event's type and the current rule and calls the right function to determine whether the event can be allowed. """ - # Special-case the access rules event. if event.type == ACCESS_RULES_TYPE: return self._on_rules_change(event, state_events) + # We need to know the rule to apply when processing the event types below. rule = self._get_rule_from_state(state_events) - if rule == ACCESS_RULE_RESTRICTED: - ret = self._apply_restricted(event) - elif rule == ACCESS_RULE_UNRESTRICTED: - ret = self._apply_unrestricted() - elif rule == ACCESS_RULE_DIRECT: - ret = self._apply_direct(event, state_events) - else: - # We currently apply the default (restricted) if we don't know the rule, we - # might want to change that in the future. - ret = self._apply_restricted(event) + if event.type == EventTypes.PowerLevels: + return self._is_power_level_content_allowed(event.content, rule) - return ret + if event.type == EventTypes.Member or event.type == EventTypes.ThirdPartyInvite: + return self._on_membership_or_invite(event, rule, state_events) + + return True def _on_rules_change(self, event, state_events): """Implement the checks and behaviour specified on allowing or forbidding a new @@ -232,35 +256,67 @@ class RoomAccessRules(object): return False - def _apply_restricted(self, event): + def _on_membership_or_invite(self, event, rule, state_events): + """Applies the correct rule for incoming m.room.member and + m.room.third_party_invite events. + + Args: + event (synapse.events.EventBase): The event to check. + rule (str): The name of the rule to apply. + state_events (dict[tuple[event type, state key], EventBase]): The state of the + room before the event was sent. + Returns: + bool, True if the event can be allowed, False otherwise. + """ + if rule == ACCESS_RULE_RESTRICTED: + ret = self._on_membership_or_invite_restricted(event) + elif rule == ACCESS_RULE_UNRESTRICTED: + ret = self._on_membership_or_invite_unrestricted() + elif rule == ACCESS_RULE_DIRECT: + ret = self._on_membership_or_invite_direct(event, state_events) + else: + # We currently apply the default (restricted) if we don't know the rule, we + # might want to change that in the future. + ret = self._on_membership_or_invite_restricted(event) + + return ret + + def _on_membership_or_invite_restricted(self, event): """Implements the checks and behaviour specified for the "restricted" rule. + "restricted" currently means that users can only invite users if their server is + included in a limited list of domains. + Args: event (synapse.events.EventBase): The event to check. Returns: bool, True if the event can be allowed, False otherwise. """ - # "restricted" currently means that users can only invite users if their server is - # included in a limited list of domains. - # We're not filtering on m.room.third_party_member events here because the - # filtering on threepids is done in check_threepid_can_be_invited. - if event.type != EventTypes.Member: + # We're not applying the rules on m.room.third_party_member events here because + # the filtering on threepids is done in check_threepid_can_be_invited, which is + # called before check_event_allowed. + if event.type == EventTypes.ThirdPartyInvite: return True invitee_domain = get_domain_from_id(event.state_key) return invitee_domain not in self.domains_forbidden_when_restricted - def _apply_unrestricted(self): + def _on_membership_or_invite_unrestricted(self): """Implements the checks and behaviour specified for the "unrestricted" rule. + "unrestricted" currently means that every event is allowed. + Returns: bool, True if the event can be allowed, False otherwise. """ - # "unrestricted" currently means that every event is allowed. return True - def _apply_direct(self, event, state_events): + def _on_membership_or_invite_direct(self, event, state_events): """Implements the checks and behaviour specified for the "direct" rule. + "direct" currently means that no member is allowed apart from the two initial + members the room was created for (i.e. the room's creator and their first + invitee). + Args: event (synapse.events.EventBase): The event to check. state_events (dict[tuple[event type, state key], EventBase]): The state of the @@ -268,12 +324,6 @@ class RoomAccessRules(object): Returns: bool, True if the event can be allowed, False otherwise. """ - # "direct" currently means that no member is allowed apart from the two initial - # members the room was created for (i.e. the room's creator and their first - # invitee). - if event.type != EventTypes.Member and event.type != EventTypes.ThirdPartyInvite: - return True - # Get the room memberships and 3PID invite tokens from the room's state. existing_members, threepid_tokens = self._get_members_and_tokens_from_state( state_events, @@ -319,6 +369,42 @@ class RoomAccessRules(object): return True + def _is_power_level_content_allowed(self, content, access_rule): + """Check if a given power levels event is permitted under the given access rule. + + It shouldn't be allowed if it either changes the default PL to a non-0 value or + gives a non-0 PL to a user that would have been forbidden from joining the room + under a more restrictive access rule. + + Args: + content (dict[]): The content of the m.room.power_levels event to check. + access_rule (str): The access rule in place in this room. + Returns: + bool, True if the event can be allowed, False otherwise. + + """ + # Check if we need to apply the restrictions with the current rule. + if access_rule not in RULES_WITH_RESTRICTED_POWER_LEVELS: + return True + + # If users_default is explicitly set to a non-0 value, deny the event. + users_default = content.get('users_default', 0) + if users_default: + return False + + users = content.get('users', {}) + for user_id, power_level in users.items(): + server_name = get_domain_from_id(user_id) + # Check the domain against the blacklist. If found, and the PL isn't 0, deny + # the event. + if ( + server_name in self.domains_forbidden_when_restricted + and power_level != 0 + ): + return False + + return True + @staticmethod def _get_rule_from_state(state_events): """Extract the rule to be applied from the given set of state events. diff --git a/tests/handlers/test_identity.py b/tests/handlers/test_identity.py new file mode 100644 index 0000000000..99ce94db52 --- /dev/null +++ b/tests/handlers/test_identity.py @@ -0,0 +1,108 @@ +# -*- coding: utf-8 -*- +# Copyright 2019 New Vector Ltd +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +from mock import Mock + +from twisted.internet import defer + +import synapse.rest.admin +from synapse.rest.client.v1 import login +from synapse.rest.client.v2_alpha import account + +from tests import unittest + + +class ThreepidISRewrittenURLTestCase(unittest.HomeserverTestCase): + + servlets = [ + synapse.rest.admin.register_servlets_for_client_rest_resource, + login.register_servlets, + account.register_servlets, + ] + + def make_homeserver(self, reactor, clock): + self.address = "test@test" + self.is_server_name = "testis" + self.rewritten_is_url = "int.testis" + + config = self.default_config() + config["trusted_third_party_id_servers"] = [ + self.is_server_name, + ] + config["rewrite_identity_server_urls"] = { + self.is_server_name: self.rewritten_is_url, + } + + mock_http_client = Mock(spec=[ + "post_urlencoded_get_json", + ]) + mock_http_client.post_urlencoded_get_json.return_value = defer.succeed({ + "address": self.address, + "medium": "email", + }) + + self.hs = self.setup_test_homeserver( + config=config, + simple_http_client=mock_http_client, + ) + + return self.hs + + def prepare(self, reactor, clock, hs): + self.user_id = self.register_user("kermit", "monkey") + + def test_rewritten_id_server(self): + """ + Tests that, when validating a 3PID association while rewriting the IS's server + name: + * the bind request is done against the rewritten hostname + * the original, non-rewritten, server name is stored in the database + """ + handler = self.hs.get_handlers().identity_handler + post_urlenc_get_json = self.hs.get_simple_http_client().post_urlencoded_get_json + store = self.hs.get_datastore() + + creds = { + "sid": "123", + "client_secret": "some_secret", + } + + # Make sure processing the mocked response goes through. + data = self.get_success(handler.bind_threepid( + { + "id_server": self.is_server_name, + "client_secret": creds["client_secret"], + "sid": creds["sid"], + }, + self.user_id, + )) + self.assertEqual(data.get("address"), self.address) + + # Check that the request was done against the rewritten server name. + post_urlenc_get_json.assert_called_once_with( + "https://%s/_matrix/identity/api/v1/3pid/bind" % self.rewritten_is_url, + { + 'sid': creds['sid'], + 'client_secret': creds["client_secret"], + 'mxid': self.user_id, + } + ) + + # Check that the original server name is saved in the database instead of the + # rewritten one. + id_servers = self.get_success(store.get_id_servers_user_bound( + self.user_id, "email", self.address + )) + self.assertEqual(id_servers, [self.is_server_name]) diff --git a/tests/rest/client/test_room_access_rules.py b/tests/rest/client/test_room_access_rules.py index 5c46dabf32..df48a89e93 100644 --- a/tests/rest/client/test_room_access_rules.py +++ b/tests/rest/client/test_room_access_rules.py @@ -22,6 +22,7 @@ from mock import Mock from twisted.internet import defer +from synapse.api.constants import EventTypes from synapse.rest import admin from synapse.rest.client.v1 import login, room from synapse.third_party_rules.access_rules import ( @@ -147,12 +148,11 @@ class RoomAccessTestCase(unittest.HomeserverTestCase): self.assertEqual(rule, ACCESS_RULE_UNRESTRICTED) def test_create_room_invalid_rule(self): - """Tests that creating a room with an invalid rule will set the default value.""" + """Tests that creating a room with an invalid rule will set fail.""" self.create_room(rule=ACCESS_RULE_DIRECT, expected_code=400) def test_create_room_direct_invalid_rule(self): - """Tests that creating a direct room with an invalid rule will set the default - value. + """Tests that creating a direct room with an invalid rule will fail. """ self.create_room(direct=True, rule=ACCESS_RULE_RESTRICTED, expected_code=400) @@ -277,7 +277,9 @@ class RoomAccessTestCase(unittest.HomeserverTestCase): ) def test_unrestricted(self): - """Tests that, in unrestricted mode, we can invite whoever we want. + """Tests that, in unrestricted mode, we can invite whoever we want, but we can + only change the power level of users that wouldn't be forbidden in restricted + mode. """ # We can invite self.helper.invite( @@ -311,6 +313,52 @@ class RoomAccessTestCase(unittest.HomeserverTestCase): expected_code=200, ) + # We can send a power level event that doesn't redefine the default PL or set a + # non-default PL for a user that would be forbidden in restricted mode. + self.helper.send_state( + room_id=self.unrestricted_room, + event_type=EventTypes.PowerLevels, + body={ + "users": { + self.user_id: 100, + "@test:not_forbidden_domain": 10, + }, + }, + tok=self.tok, + expect_code=200, + ) + + # We can't send a power level event that redefines the default PL and doesn't set + # a non-default PL for a user that would be forbidden in restricted mode. + self.helper.send_state( + room_id=self.unrestricted_room, + event_type=EventTypes.PowerLevels, + body={ + "users": { + self.user_id: 100, + "@test:not_forbidden_domain": 10, + }, + "users_default": 10, + }, + tok=self.tok, + expect_code=403, + ) + + # We can't send a power level event that doesn't redefines the default PL but sets + # a non-default PL for a user that would be forbidden in restricted mode. + self.helper.send_state( + room_id=self.unrestricted_room, + event_type=EventTypes.PowerLevels, + body={ + "users": { + self.user_id: 100, + "@test:forbidden_domain": 10, + }, + }, + tok=self.tok, + expect_code=403, + ) + def test_change_rules(self): """Tests that we can only change the current rule from restricted to unrestricted. |