From d36a876d2d4571321e8a989c5bfb73a3b2e1dc1f Mon Sep 17 00:00:00 2001 From: Brendan Abolivier Date: Tue, 18 Jun 2019 14:53:33 +0100 Subject: Incorporate review --- synapse/third_party_rules/access_rules.py | 147 ++++++++++++++---------------- 1 file changed, 69 insertions(+), 78 deletions(-) diff --git a/synapse/third_party_rules/access_rules.py b/synapse/third_party_rules/access_rules.py index 07a1649584..096e23e149 100644 --- a/synapse/third_party_rules/access_rules.py +++ b/synapse/third_party_rules/access_rules.py @@ -18,14 +18,21 @@ import email.utils from twisted.internet import defer from synapse.api.constants import EventTypes +from synapse.api.errors import SynapseError from synapse.config._base import ConfigError -from synapse.rulecheck.domain_rule_checker import DomainRuleChecker +from synapse.types import get_domain_from_id ACCESS_RULES_TYPE = "im.vector.room.access_rules" ACCESS_RULE_RESTRICTED = "restricted" ACCESS_RULE_UNRESTRICTED = "unrestricted" ACCESS_RULE_DIRECT = "direct" +VALID_ACCESS_RULES = ( + ACCESS_RULE_DIRECT, + ACCESS_RULE_RESTRICTED, + ACCESS_RULE_UNRESTRICTED, +) + class RoomAccessRules(object): """Implementation of the ThirdPartyEventRules module API that allows federation admins @@ -75,7 +82,7 @@ class RoomAccessRules(object): rules_in_initial_state = False # If there's a rules event in the initial state, check if it complies with the - # spec for im.vector.room.access_rules and fix it if not. + # 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 @@ -84,27 +91,27 @@ class RoomAccessRules(object): # Make sure the event has a valid content. if rule is None: - event["content"] = { - "rule": self._on_create_room_default_rule(is_direct) - } + raise SynapseError(400, "Invalid access rule",) # Make sure the rule name is valid. - if not self._is_rule_name_valid(rule): - event["content"]["rule"] = self._on_create_room_default_rule( - is_direct, - ) + if rule not in VALID_ACCESS_RULES: + 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: - event["content"]["rule"] = ACCESS_RULE_DIRECT - - # Make sure the rule is not "direct" if the room isn't a direct chat. - if rule == ACCESS_RULE_DIRECT and not is_direct: - event["content"]["rule"] = ACCESS_RULE_RESTRICTED + if ( + (is_direct and rule != ACCESS_RULE_DIRECT) + or (rule == ACCESS_RULE_DIRECT and not is_direct) + ): + 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 is_direct: + default_rule = ACCESS_RULE_DIRECT + else: + default_rule = ACCESS_RULE_RESTRICTED + if not config.get("initial_state"): config["initial_state"] = [] @@ -112,25 +119,10 @@ class RoomAccessRules(object): "type": ACCESS_RULES_TYPE, "state_key": "", "content": { - "rule": self._on_create_room_default_rule(is_direct), + "rule": default_rule, } }) - @staticmethod - def _on_create_room_default_rule(is_direct): - """Returns the default rule to set. - - Args: - is_direct (bool): Is the room created with "is_direct" set to True. - - Returns: - str, the name of the rule tu use as the default. - """ - if is_direct: - return ACCESS_RULE_DIRECT - else: - return ACCESS_RULE_RESTRICTED - @defer.inlineCallbacks def check_threepid_can_be_invited(self, medium, address, state_events): """Implements synapse.events.ThirdPartyEventRules.check_threepid_can_be_invited @@ -145,7 +137,9 @@ class RoomAccessRules(object): defer.returnValue(False) if rule != ACCESS_RULE_RESTRICTED: - # Only "restricted" requires filtering 3PID invites. + # Only "restricted" requires filtering 3PID invites. We don't need to do + # anything for "direct" here, because only "restricted" requires filtering + # based on the HS the address is mapped to. defer.returnValue(True) parsed_address = email.utils.parseaddr(address)[1] @@ -211,17 +205,16 @@ class RoomAccessRules(object): new_rule = event.content.get("rule") # Check for invalid values. - if not self._is_rule_name_valid(new_rule): + if new_rule not in VALID_ACCESS_RULES: return False # Make sure we don't apply "direct" if the room has more than two members. if new_rule == ACCESS_RULE_DIRECT: - member_events_count = 0 - for key, event in state_events.items(): - if key[0] == EventTypes.Member: - member_events_count += 1 + existing_members, threepid_tokens = self._get_members_and_tokens_from_state( + state_events, + ) - if member_events_count > 2: + if len(existing_members) > 2 or len(threepid_tokens) > 1: return False prev_rules_event = state_events.get((ACCESS_RULES_TYPE, "")) @@ -251,7 +244,7 @@ class RoomAccessRules(object): # included in a limited list of domains. if event.type != EventTypes.Member and event.type != EventTypes.ThirdPartyInvite: return True - invitee_domain = DomainRuleChecker._get_domain_from_id(event.state_key) + invitee_domain = get_domain_from_id(event.state_key) return invitee_domain not in self.domains_forbidden_when_restricted def _apply_unrestricted(self): @@ -279,26 +272,21 @@ class RoomAccessRules(object): if event.type != EventTypes.Member and event.type != EventTypes.ThirdPartyInvite: return True - # Get the m.room.member and m.room.third_party_invite events from the room's - # state. - member_events = [] - threepid_invite_events = [] - for key, event in state_events.items(): - if key[0] == EventTypes.Member: - member_events.append(event) - if key[0] == EventTypes.ThirdPartyInvite: - threepid_invite_events.append(event) + # 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, + ) # There should never be more than one 3PID invite in the room state: if the second # original user came and left, and we're inviting them using their email address, # given we know they have a Matrix account binded to the address (so they could # join the first time), Synapse will successfully look it up before attempting to # store an invite on the IS. - if len(threepid_invite_events) == 1 and event.type == EventTypes.ThirdPartyInvite: + if len(threepid_tokens) == 1 and event.type == EventTypes.ThirdPartyInvite: # If we already have a 3PID invite in flight, don't accept another one. return False - if len(member_events) == 2: + if len(existing_members) == 2: # If the user was within the two initial user of the room, Synapse would have # looked it up successfully and thus sent a m.room.member here instead of # m.room.third_party_invite. @@ -308,16 +296,11 @@ class RoomAccessRules(object): # We can only have m.room.member events here. The rule in this case is to only # allow the event if its target is one of the initial two members in the room, # i.e. the state key of one of the two m.room.member states in the room. - target = event.state_key - for e in member_events: - if e.state_key == target: - return True - - return False + return event.state_key in existing_members # We're alone in the room (and always have been) and there's one 3PID invite in # flight. - if len(member_events) == 1 and len(threepid_invite_events) == 1: + if len(existing_members) == 1 and len(threepid_tokens) == 1: # We can only have m.room.member events here. In this case, we can only allow # the event if it's either a m.room.member from the joined user (we can assume # that the only m.room.member event is a join otherwise we wouldn't be able to @@ -325,10 +308,9 @@ class RoomAccessRules(object): # user. target = event.state_key is_from_threepid_invite = self._is_invite_from_threepid( - event, threepid_invite_events[0], - + event, threepid_tokens[0], ) - if is_from_threepid_invite or target == member_events[0].state_key: + if is_from_threepid_invite or target == existing_members[0]: return True return False @@ -341,7 +323,7 @@ class RoomAccessRules(object): Args: state_events (dict[tuple[event type, state key], EventBase]): The set of state - events + events. Returns: str, the name of the rule (either "direct", "restricted" or "unrestricted") """ @@ -353,28 +335,37 @@ class RoomAccessRules(object): return rule @staticmethod - def _is_invite_from_threepid(invite, threepid_invite): - """Checks whether the given invite follows the given 3PID invite. + def _get_members_and_tokens_from_state(state_events): + """Retrieves from a list of state events the list of users that have a + m.room.member event in the room, and the tokens of 3PID invites in the room. Args: - invite (EventBase): The m.room.member event with "invite" membership. - threepid_invite (EventBase): The m.room.third_party_invite event. + state_events (dict[tuple[event type, state key], EventBase]): The set of state + events. + Returns: + existing_members (list[str]): List of targets of the m.room.member events in + the state. + threepid_invite_tokens (list[str]): List of tokens of the 3PID invites in the + state. """ - token = invite.content.get("third_party_signed", {}).get("token", "") - return token == threepid_invite.state_key + existing_members = [] + threepid_invite_tokens = [] + for key, event in state_events.items(): + if key[0] == EventTypes.Member: + existing_members.append(event.state_key) + if key[0] == EventTypes.ThirdPartyInvite: + threepid_invite_tokens.append(event.state_key) + + return existing_members, threepid_invite_tokens + @staticmethod - def _is_rule_name_valid(rule): - """Returns whether the given rule name is within the allowed values ("direct", - "restricted" or "unrestricted"). + def _is_invite_from_threepid(invite, threepid_invite_token): + """Checks whether the given invite follows the given 3PID invite. Args: - rule (str): The name of the rule. - Returns: - bool, True if the name is valid, False otherwise. + invite (EventBase): The m.room.member event with "invite" membership. + threepid_invite_token (str): The state key from the 3PID invite. """ - return ( - rule == ACCESS_RULE_DIRECT - or rule == ACCESS_RULE_RESTRICTED - or rule == ACCESS_RULE_UNRESTRICTED - ) + token = invite.content.get("third_party_signed", {}).get("token", "") + return token == threepid_invite_token -- cgit 1.4.1