Merge pull request #5760 from matrix-org/babolivier/access-rules-publ…

…ic-restricted

Force the access rule to be "restricted" if the join rule is "public"

changelog.d/5760.feature

@@ -0,0 +1 @@
+Force the access rule to be "restricted" if the join rule is "public".

synapse/third_party_rules/access_rules.py

@@ -17,7 +17,7 @@
 
 from twisted.internet import defer
 
-from synapse.api.constants import EventTypes
+from synapse.api.constants import EventTypes, JoinRules, RoomCreationPreset
 from synapse.api.errors import SynapseError
 from synapse.config._base import ConfigError
 from synapse.types import get_domain_from_id
@@ -94,35 +94,43 @@ def on_create_room(self, requester, config, is_requester_admin):
         default rule to the initial state.
         """
         is_direct = config.get("is_direct")
-        rule = None
+        preset = config.get("preset")
+        access_rule = None
+        join_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:
-                rule = event["content"].get("rule")
+                access_rule = event["content"].get("rule")
 
                 # Make sure the event has a valid content.
-                if rule is None:
+                if access_rule is None:
                     raise SynapseError(400, "Invalid access rule")
 
                 # Make sure the rule name is valid.
-                if rule not in VALID_ACCESS_RULES:
+                if access_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)
-                    or (rule == ACCESS_RULE_DIRECT and not is_direct)
+                    (is_direct and access_rule != ACCESS_RULE_DIRECT)
+                    or (access_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 rule:
+            if event["type"] == EventTypes.JoinRules:
+                join_rule = event["content"].get("join_rule")
+
+        if access_rule is None:
+            # If there's no access rules event in the initial state, create one with the
+            # default setting.
             if is_direct:
                 default_rule = ACCESS_RULE_DIRECT
             else:
+                # If the default value for non-direct chat changes, we should make another
+                # case here for rooms created with either a "public" join_rule or the
+                # "public_chat" preset to make sure those keep defaulting to "restricted"
                 default_rule = ACCESS_RULE_RESTRICTED
 
             if not config.get("initial_state"):
@@ -136,19 +144,32 @@ def on_create_room(self, requester, config, is_requester_admin):
                 }
             })
 
-            rule = default_rule
+            access_rule = default_rule
+
+        # Check that the preset or the join rule in use is compatible with the access
+        # rule, whether it's a user-defined one or the default one (i.e. if it involves
+        # a "public" join rule, the access rule must be "restricted").
+        if (
+            (
+                join_rule == JoinRules.PUBLIC
+                or preset == RoomCreationPreset.PUBLIC_CHAT
+            ) and access_rule != ACCESS_RULE_RESTRICTED
+        ):
+            raise SynapseError(400, "Invalid access 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,
+            config.get("power_level_content_override", {}), access_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)
+                allowed = self._is_power_level_content_allowed(
+                    event["content"], access_rule
+                )
                 if not allowed:
                     raise SynapseError(400, "Invalid power levels content")
 
@@ -213,6 +234,9 @@ def check_event_allowed(self, event, state_events):
         if event.type == EventTypes.Member or event.type == EventTypes.ThirdPartyInvite:
             return self._on_membership_or_invite(event, rule, state_events)
 
+        if event.type == EventTypes.JoinRules:
+            return self._on_join_rule_change(event, rule)
+
         return True
 
     def _on_rules_change(self, event, state_events):
@@ -232,6 +256,12 @@ def _on_rules_change(self, event, state_events):
         if new_rule not in VALID_ACCESS_RULES:
             return False
 
+        # We must not allow rooms with the "public" join rule to be given any other access
+        # rule than "restricted".
+        join_rule = self._get_join_rule_from_state(state_events)
+        if join_rule == JoinRules.PUBLIC and new_rule != ACCESS_RULE_RESTRICTED:
+            return False
+
         # Make sure we don't apply "direct" if the room has more than two members.
         if new_rule == ACCESS_RULE_DIRECT:
             existing_members, threepid_tokens = self._get_members_and_tokens_from_state(
@@ -381,7 +411,6 @@ def _is_power_level_content_allowed(self, content, access_rule):
             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:
@@ -405,6 +434,33 @@ def _is_power_level_content_allowed(self, content, access_rule):
 
         return True
 
+    def _on_join_rule_change(self, event, rule):
+        """Check whether a join rule change is allowed. A join rule change is always
+        allowed unless the new join rule is "public" and the current access rule isn't
+        "restricted".
+        The rationale is that external users (those whose server would be denied access
+        to rooms enforcing the "restricted" access rule) should always rely on non-
+        external users for access to rooms, therefore they shouldn't be able to access
+        rooms that don't require an invite to be joined.
+
+        Note that we currently rely on the default access rule being "restricted": during
+        room creation, the m.room.join_rules event will be sent *before* the
+        im.vector.room.access_rules one, so the access rule that will be considered here
+        in this case will be the default "restricted" one. This is fine since the
+        "restricted" access rule allows any value for the join rule, but we should keep
+        that in mind if we need to change the default access rule in the future.
+
+        Args:
+            event (synapse.events.EventBase): The event to check.
+            rule (str): The name of the rule to apply.
+        Returns:
+            bool, True if the event can be allowed, False otherwise.
+        """
+        if event.content.get('join_rule') == JoinRules.PUBLIC:
+            return rule == ACCESS_RULE_RESTRICTED
+
+        return True
+
     @staticmethod
     def _get_rule_from_state(state_events):
         """Extract the rule to be applied from the given set of state events.
@@ -422,6 +478,21 @@ def _get_rule_from_state(state_events):
             rule = access_rules.content.get("rule")
         return rule
 
+    @staticmethod
+    def _get_join_rule_from_state(state_events):
+        """Extract the room's join rule from the given set of state events.
+
+        Args:
+            state_events (dict[tuple[event type, state key], EventBase]): The set of state
+                events.
+        Returns:
+            str, the name of the join rule (either "public", or "invite")
+        """
+        join_rule_event = state_events.get((EventTypes.JoinRules, ""))
+        if join_rule_event is None:
+            return None
+        return join_rule_event.content.get("join_rule")
+
     @staticmethod
     def _get_members_and_tokens_from_state(state_events):
         """Retrieves from a list of state events the list of users that have a

tests/rest/client/test_room_access_rules.py

@@ -22,7 +22,7 @@
 
 from twisted.internet import defer
 
-from synapse.api.constants import EventTypes
+from synapse.api.constants import EventTypes, JoinRules, RoomCreationPreset
 from synapse.rest import admin
 from synapse.rest.client.v1 import login, room
 from synapse.third_party_rules.access_rules import (
@@ -156,6 +156,84 @@ def test_create_room_direct_invalid_rule(self):
         """
         self.create_room(direct=True, rule=ACCESS_RULE_RESTRICTED, expected_code=400)
 
+    def test_public_room(self):
+        """Tests that it's not possible to have a room with the public join rule and an
+        access rule that's not restricted.
+        """
+        # Creating a room with the public_chat preset should succeed and set the access
+        # rule to restricted.
+        preset_room_id = self.create_room(preset=RoomCreationPreset.PUBLIC_CHAT)
+        self.assertEqual(
+            self.current_rule_in_room(preset_room_id), ACCESS_RULE_RESTRICTED,
+        )
+
+        # Creating a room with the public join rule in its initial state should succeed
+        # and set the access rule to restricted.
+        init_state_room_id = self.create_room(initial_state=[{
+            "type": "m.room.join_rules",
+            "content": {
+                "join_rule": JoinRules.PUBLIC,
+            },
+        }])
+        self.assertEqual(
+            self.current_rule_in_room(init_state_room_id), ACCESS_RULE_RESTRICTED,
+        )
+
+        # Changing access rule to unrestricted should fail.
+        self.change_rule_in_room(
+            preset_room_id, ACCESS_RULE_UNRESTRICTED, expected_code=403,
+        )
+        self.change_rule_in_room(
+            init_state_room_id, ACCESS_RULE_UNRESTRICTED, expected_code=403,
+        )
+
+        # Changing access rule to direct should fail.
+        self.change_rule_in_room(
+            preset_room_id, ACCESS_RULE_DIRECT, expected_code=403,
+        )
+        self.change_rule_in_room(
+            init_state_room_id, ACCESS_RULE_DIRECT, expected_code=403,
+        )
+
+        # Changing join rule to public in an unrestricted room should fail.
+        self.change_join_rule_in_room(
+            self.unrestricted_room, JoinRules.PUBLIC, expected_code=403,
+        )
+        # Changing join rule to public in an direct room should fail.
+        self.change_join_rule_in_room(
+            self.direct_rooms[0], JoinRules.PUBLIC, expected_code=403,
+        )
+
+        # Creating a new room with the public_chat preset and an access rule that isn't
+        # restricted should fail.
+        self.create_room(
+            preset=RoomCreationPreset.PUBLIC_CHAT, rule=ACCESS_RULE_UNRESTRICTED,
+            expected_code=400,
+        )
+        self.create_room(
+            preset=RoomCreationPreset.PUBLIC_CHAT, rule=ACCESS_RULE_DIRECT,
+            expected_code=400,
+        )
+
+        # Creating a room with the public join rule in its initial state and an access
+        # rule that isn't restricted should fail.
+        self.create_room(
+            initial_state=[{
+                "type": "m.room.join_rules",
+                "content": {
+                    "join_rule": JoinRules.PUBLIC,
+                },
+            }], rule=ACCESS_RULE_UNRESTRICTED, expected_code=400,
+        )
+        self.create_room(
+            initial_state=[{
+                "type": "m.room.join_rules",
+                "content": {
+                    "join_rule": JoinRules.PUBLIC,
+                },
+            }], rule=ACCESS_RULE_DIRECT, expected_code=400,
+        )
+
     def test_restricted(self):
         """Tests that in restricted mode we're unable to invite users from blacklisted
         servers but can invite other users.
@@ -405,9 +483,13 @@ def test_change_rules(self):
             expected_code=403,
         )
 
-    def create_room(self, direct=False, rule=None, expected_code=200):
+    def create_room(
+        self, direct=False, rule=None, preset=RoomCreationPreset.TRUSTED_PRIVATE_CHAT,
+        initial_state=None, expected_code=200,
+    ):
         content = {
             "is_direct": direct,
+            "preset": preset,
         }
 
         if rule:
@@ -419,6 +501,12 @@ def create_room(self, direct=False, rule=None, expected_code=200):
                 }
             }]
 
+        if initial_state:
+            if "initial_state" not in content:
+                content["initial_state"] = []
+
+            content["initial_state"] += initial_state
+
         request, channel = self.make_request(
             "POST",
             "/_matrix/client/r0/createRoom",
@@ -457,6 +545,20 @@ def change_rule_in_room(self, room_id, new_rule, expected_code=200):
 
         self.assertEqual(channel.code, expected_code, channel.result)
 
+    def change_join_rule_in_room(self, room_id, new_join_rule, expected_code=200):
+        data = {
+            "join_rule": new_join_rule,
+        }
+        request, channel = self.make_request(
+            "PUT",
+            "/_matrix/client/r0/rooms/%s/state/%s" % (room_id, EventTypes.JoinRules),
+            json.dumps(data),
+            access_token=self.tok,
+        )
+        self.render(request)
+
+        self.assertEqual(channel.code, expected_code, channel.result)
+
     def send_threepid_invite(self, address, room_id, expected_code=200):
         params = {
             "id_server": "testis",