Incorporate review

1 files 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