From 0ebdde87399e4195b0f385f7f2511ef16893b07b Mon Sep 17 00:00:00 2001
From: Andrew Morgan <1342360+anoadragon453@users.noreply.github.com>
Date: Thu, 29 Oct 2020 10:48:29 +0000
Subject: [PATCH] Override any missing default power level keys with DINUM's
 defaults when creating a room (#68)

The createRoom flow in DINUM's Synapse (through the AccessRules module which has hooks for all of this) already rejects a power levels content dict if it doesn't have high enough power levels to satisfy DINUM's [requirements](https://github.com/matrix-org/synapse-dinsic/blob/ac50ed353b5fdbdf9f853be0d94b6fccaf33973e/synapse/third_party_rules/access_rules.py#L233-L252).

This PR ensures that any keys that aren't provided are replaced with the defaults, instead of just assuming the whole dict was correct (and thus those keys were set to mainline Synapse's default instead).
---
 changelog.d/68.misc                         |  1 +
 synapse/third_party_rules/access_rules.py   | 55 ++++++++++++++-------
 tests/rest/client/test_room_access_rules.py | 47 ++++++++++++++++++
 3 files changed, 85 insertions(+), 18 deletions(-)
 create mode 100644 changelog.d/68.misc

diff --git a/changelog.d/68.misc b/changelog.d/68.misc
new file mode 100644
index 0000000000..99cc5f7483
--- /dev/null
+++ b/changelog.d/68.misc
@@ -0,0 +1 @@
+Override any missing default power level keys with DINUM's defaults when creating a room.
\ No newline at end of file
diff --git a/synapse/third_party_rules/access_rules.py b/synapse/third_party_rules/access_rules.py
index 2519e05ae0..f158ab6676 100644
--- a/synapse/third_party_rules/access_rules.py
+++ b/synapse/third_party_rules/access_rules.py
@@ -189,34 +189,40 @@ class RoomAccessRules(object):
         ) and access_rule == AccessRules.DIRECT:
             raise SynapseError(400, "Invalid access rule")
 
+        default_power_levels = self._get_default_power_levels(
+            requester.user.to_string()
+        )
+
         # Check if the creator can override values for the power levels.
         allowed = self._is_power_level_content_allowed(
-            config.get("power_level_content_override", {}), access_rule
+            config.get("power_level_content_override", {}),
+            access_rule,
+            default_power_levels,
         )
         if not allowed:
             raise SynapseError(400, "Invalid power levels content override")
 
-        use_default_power_levels = True
-        if config.get("power_level_content_override"):
-            use_default_power_levels = False
+        custom_user_power_levels = config.get("power_level_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"], access_rule
+                    event["content"], access_rule, default_power_levels
                 )
                 if not allowed:
                     raise SynapseError(400, "Invalid power levels content")
 
-                use_default_power_levels = False
-
-        # If power levels were not overridden by the user, override with DINUM's preferred
-        # defaults instead
-        if use_default_power_levels:
-            config["power_level_content_override"] = self._get_default_power_levels(
-                requester.user.to_string()
-            )
+                custom_user_power_levels = event["content"]
+        if custom_user_power_levels:
+            # If the user is using their own power levels, but failed to provide an expected
+            # key in the power levels content dictionary, fill it in from the defaults instead
+            for key, value in default_power_levels.items():
+                custom_user_power_levels.setdefault(key, value)
+        else:
+            # If power levels were not overridden by the user, completely override with the
+            # defaults instead
+            config["power_level_content_override"] = default_power_levels
 
         return True
 
@@ -710,7 +716,11 @@ class RoomAccessRules(object):
         return True
 
     def _is_power_level_content_allowed(
-        self, content: Dict, access_rule: str, on_room_creation: bool = True
+        self,
+        content: Dict,
+        access_rule: str,
+        default_power_levels: Optional[Dict] = None,
+        on_room_creation: bool = True,
     ) -> bool:
         """Check if a given power levels event is permitted under the given access rule.
 
@@ -721,6 +731,8 @@ class RoomAccessRules(object):
         Args:
             content: The content of the m.room.power_levels event to check.
             access_rule: The access rule in place in this room.
+            default_power_levels: The default power levels when a room is created with
+                the specified access rule. Required if on_room_creation is True.
             on_room_creation: True if this call is happening during a room's
                 creation, False otherwise.
 
@@ -733,12 +745,19 @@ class RoomAccessRules(object):
         # have a special circumstance, but still want to encourage a certain pattern during
         # room creation.
         if on_room_creation:
-            # If invite requirements are <PL50
-            if content.get("invite", 50) < 50:
+            # We specifically don't fail if "invite" or "state_default" are None, as those
+            # values should be replaced with our "default" power level values anyways,
+            # which are compliant
+
+            invite = default_power_levels["invite"]
+            state_default = default_power_levels["state_default"]
+
+            # If invite requirements are less than our required defaults
+            if content.get("invite", invite) < invite:
                 return False
 
-            # If "other" state requirements are <PL100
-            if content.get("state_default", 100) < 100:
+            # If "other" state requirements are less than our required defaults
+            if content.get("state_default", state_default) < state_default:
                 return False
 
         # Check if we need to apply the restrictions with the current rule.
diff --git a/tests/rest/client/test_room_access_rules.py b/tests/rest/client/test_room_access_rules.py
index de7856fba9..f233b79de5 100644
--- a/tests/rest/client/test_room_access_rules.py
+++ b/tests/rest/client/test_room_access_rules.py
@@ -185,6 +185,49 @@ class RoomAccessTestCase(unittest.HomeserverTestCase):
             expected_code=400,
         )
 
+    def test_create_room_with_missing_power_levels_use_default_values(self):
+        """
+        Tests that a room created with custom power levels, but without defining invite or state_default
+        succeeds, but the missing values are replaced with the defaults.
+        """
+
+        # Attempt to create a room without defining "invite" or "state_default"
+        modified_power_levels = RoomAccessRules._get_default_power_levels(self.user_id)
+        del modified_power_levels["invite"]
+        del modified_power_levels["state_default"]
+        room_id = self.create_room(
+            direct=True,
+            rule=AccessRules.DIRECT,
+            initial_state=[
+                {"type": "m.room.power_levels", "content": modified_power_levels}
+            ],
+        )
+
+        # This should succeed, but the defaults should be put in place instead
+        room_power_levels = self.helper.get_state(
+            room_id, "m.room.power_levels", self.tok
+        )
+        self.assertEqual(room_power_levels["invite"], 50)
+        self.assertEqual(room_power_levels["state_default"], 100)
+
+        # And now the same test, but using power_levels_content_override instead
+        # of initial_state (which takes a slightly different codepath)
+        modified_power_levels = RoomAccessRules._get_default_power_levels(self.user_id)
+        del modified_power_levels["invite"]
+        del modified_power_levels["state_default"]
+        room_id = self.create_room(
+            direct=True,
+            rule=AccessRules.DIRECT,
+            power_levels_content_override=modified_power_levels,
+        )
+
+        # This should succeed, but the defaults should be put in place instead
+        room_power_levels = self.helper.get_state(
+            room_id, "m.room.power_levels", self.tok
+        )
+        self.assertEqual(room_power_levels["invite"], 50)
+        self.assertEqual(room_power_levels["state_default"], 100)
+
     def test_existing_room_can_change_power_levels(self):
         """Tests that a room created with default power levels can have their power levels
         dropped after room creation
@@ -975,6 +1018,7 @@ class RoomAccessTestCase(unittest.HomeserverTestCase):
         rule=None,
         preset=RoomCreationPreset.TRUSTED_PRIVATE_CHAT,
         initial_state=None,
+        power_levels_content_override=None,
         expected_code=200,
     ):
         content = {"is_direct": direct, "preset": preset}
@@ -990,6 +1034,9 @@ class RoomAccessTestCase(unittest.HomeserverTestCase):
 
             content["initial_state"] += initial_state
 
+        if power_levels_content_override:
+            content["power_levels_content_override"] = power_levels_content_override
+
         request, channel = self.make_request(
             "POST", "/_matrix/client/r0/createRoom", content, access_token=self.tok,
         )