Merge branch 'develop' into rei/add_job

docs/admin_api/user_admin_api.md

@@ -353,7 +353,11 @@ The following actions are performed when deactivating an user:
 - Remove the user from the user directory
 - Reject all pending invites
 - Remove all account validity information related to the user
-- Remove the arbitrary data store known as *account data* (list of ignored users, push rules, etc.)
+- Remove the arbitrary data store known as *account data*. For example, this includes:
+    - list of ignored users;
+    - push rules;
+    - secret storage keys; and
+    - cross-signing keys.
 
 The following additional actions are performed during deactivation if `erase`
 is set to `true`:

synapse/storage/databases/main/account_data.py

@@ -573,11 +573,15 @@ def _purge_account_data_for_user_txn(
         """
         See `purge_account_data_for_user`.
         """
-        # Purge from the primary account_data table.
+        # Purge from the primary account_data tables.
         self.db_pool.simple_delete_txn(
             txn, table="account_data", keyvalues={"user_id": user_id}
         )
 
+        self.db_pool.simple_delete_txn(
+            txn, table="room_account_data", keyvalues={"user_id": user_id}
+        )
+
         # Purge from ignored_users where this user is the ignorer.
         # N.B. We don't purge where this user is the ignoree, because that
         #      interferes with other users' account data.
@@ -597,6 +601,29 @@ def _purge_account_data_for_user_txn(
             txn, table="push_rules_stream", keyvalues={"user_id": user_id}
         )
 
+        # Invalidate caches as appropriate
+        self._invalidate_cache_and_stream(
+            txn, self.get_account_data_for_room_and_type, (user_id,)
+        )
+        self._invalidate_cache_and_stream(
+            txn, self.get_account_data_for_user, (user_id,)
+        )
+        self._invalidate_cache_and_stream(
+            txn, self.get_global_account_data_by_type_for_user, (user_id,)
+        )
+        self._invalidate_cache_and_stream(
+            txn, self.get_account_data_for_room, (user_id,)
+        )
+        self._invalidate_cache_and_stream(
+            txn, self.get_push_rules_for_user, (user_id,)
+        )
+        self._invalidate_cache_and_stream(
+            txn, self.get_push_rules_enabled_for_user, (user_id,)
+        )
+        # This user might be contained in the ignored_by cache for other users,
+        # so we have to invalidate it all.
+        self._invalidate_all_cache_and_stream(txn, self.ignored_by)
+
     async def _delete_account_data_for_deactivated_users(
         self, progress: dict, batch_size: int
     ) -> int:

tests/handlers/test_deactivate_account.py

@@ -11,9 +11,13 @@
 # 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 http import HTTPStatus
+from typing import Any, Dict
+
 from twisted.test.proto_helpers import MemoryReactor
 
 from synapse.api.constants import AccountDataTypes
+from synapse.push.rulekinds import PRIORITY_CLASS_MAP
 from synapse.rest import admin
 from synapse.rest.client import account, login
 from synapse.server import HomeServer
@@ -30,26 +34,16 @@ class DeactivateAccountTestCase(HomeserverTestCase):
     ]
 
     def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None:
-        super(DeactivateAccountTestCase, self).prepare(reactor, clock, hs)
         self._store = hs.get_datastore()
 
         self.user = self.register_user("user", "pass")
         self.token = self.login("user", "pass")
 
-    def test_account_data_deleted_upon_deactivation(self) -> None:
+    def _deactivate_my_account(self):
         """
-        Tests that account data is removed upon deactivation.
+        Deactivates the account `self.user` using `self.token` and asserts
+        that it returns a 200 success code.
         """
-        # Add some account data
-        self.get_success(
-            self._store.add_account_data_for_user(
-                self.user,
-                AccountDataTypes.DIRECT,
-                {"@someone:remote": ["!somewhere:remote"]},
-            )
-        )
-
-        # Request the deactivation of our account
         req = self.get_success(
             self.make_request(
                 "POST",
@@ -65,18 +59,166 @@ def test_account_data_deleted_upon_deactivation(self) -> None:
                 access_token=self.token,
             )
         )
-        self.assertEqual(req.code, 200, req)
+        self.assertEqual(req.code, HTTPStatus.OK, req)
+
+    def test_global_account_data_deleted_upon_deactivation(self) -> None:
+        """
+        Tests that global account data is removed upon deactivation.
+        """
+        # Add some account data
+        self.get_success(
+            self._store.add_account_data_for_user(
+                self.user,
+                AccountDataTypes.DIRECT,
+                {"@someone:remote": ["!somewhere:remote"]},
+            )
+        )
+
+        # Check that we actually added some.
+        self.assertIsNotNone(
+            self.get_success(
+                self._store.get_global_account_data_by_type_for_user(
+                    self.user, AccountDataTypes.DIRECT
+                )
+            ),
+        )
+
+        # Request the deactivation of our account
+        self._deactivate_my_account()
 
         # Check that the account data does not persist.
         self.assertIsNone(
             self.get_success(
                 self._store.get_global_account_data_by_type_for_user(
-                    AccountDataTypes.DIRECT,
-                    self.user,
+                    self.user, AccountDataTypes.DIRECT
+                )
+            ),
+        )
+
+    def test_room_account_data_deleted_upon_deactivation(self) -> None:
+        """
+        Tests that room account data is removed upon deactivation.
+        """
+        room_id = "!room:test"
+
+        # Add some room account data
+        self.get_success(
+            self._store.add_account_data_to_room(
+                self.user,
+                room_id,
+                "m.fully_read",
+                {"event_id": "$aaaa:test"},
+            )
+        )
+
+        # Check that we actually added some.
+        self.assertIsNotNone(
+            self.get_success(
+                self._store.get_account_data_for_room_and_type(
+                    self.user, room_id, "m.fully_read"
+                )
+            ),
+        )
+
+        # Request the deactivation of our account
+        self._deactivate_my_account()
+
+        # Check that the account data does not persist.
+        self.assertIsNone(
+            self.get_success(
+                self._store.get_account_data_for_room_and_type(
+                    self.user, room_id, "m.fully_read"
                 )
             ),
         )
 
+    def _is_custom_rule(self, push_rule: Dict[str, Any]) -> bool:
+        """
+        Default rules start with a dot: such as .m.rule and .im.vector.
+        This function returns true iff a rule is custom (not default).
+        """
+        return "/." not in push_rule["rule_id"]
+
+    def test_push_rules_deleted_upon_account_deactivation(self) -> None:
+        """
+        Push rules are a special case of account data.
+        They are stored separately but get sent to the client as account data in /sync.
+        This tests that deactivating a user deletes push rules along with the rest
+        of their account data.
+        """
+
+        # Add a push rule
+        self.get_success(
+            self._store.add_push_rule(
+                self.user,
+                "personal.override.rule1",
+                PRIORITY_CLASS_MAP["override"],
+                [],
+                [],
+            )
+        )
+
+        # Test the rule exists
+        push_rules = self.get_success(self._store.get_push_rules_for_user(self.user))
+        # Filter out default rules; we don't care
+        push_rules = list(filter(self._is_custom_rule, push_rules))
+        # Check our rule made it
+        self.assertEqual(
+            push_rules,
+            [
+                {
+                    "user_name": "@user:test",
+                    "rule_id": "personal.override.rule1",
+                    "priority_class": 5,
+                    "priority": 0,
+                    "conditions": [],
+                    "actions": [],
+                    "default": False,
+                }
+            ],
+            push_rules,
+        )
+
+        # Request the deactivation of our account
+        self._deactivate_my_account()
+
+        push_rules = self.get_success(self._store.get_push_rules_for_user(self.user))
+        # Filter out default rules; we don't care
+        push_rules = list(filter(self._is_custom_rule, push_rules))
+        # Check our rule no longer exists
+        self.assertEqual(push_rules, [], push_rules)
+
+    def test_ignored_users_deleted_upon_deactivation(self) -> None:
+        """
+        Ignored users are a special case of account data.
+        They get denormalised into the `ignored_users` table upon being stored as
+        account data.
+        Test that a user's list of ignored users is deleted upon deactivation.
+        """
+
+        # Add an ignored user
+        self.get_success(
+            self._store.add_account_data_for_user(
+                self.user,
+                AccountDataTypes.IGNORED_USER_LIST,
+                {"ignored_users": {"@sheltie:test": {}}},
+            )
+        )
+
+        # Test the user is ignored
+        self.assertEqual(
+            self.get_success(self._store.ignored_by("@sheltie:test")), {self.user}
+        )
+
+        # Request the deactivation of our account
+        self._deactivate_my_account()
+
+        # Test the user is no longer ignored by the user that was deactivated
+        self.assertEqual(
+            self.get_success(self._store.ignored_by("@sheltie:test")), set()
+        )
+
+
     def _rerun_retroactive_account_data_deletion_job(self) -> None:
         # Reset the 'all done' flag
         self._store.db_pool.updates._all_done = False
@@ -93,6 +235,7 @@ def _rerun_retroactive_account_data_deletion_job(self) -> None:
 
         self.wait_for_background_updates()
 
+
     def test_account_data_deleted_retroactively_by_background_job_if_deactivated(
         self,
     ) -> None:
@@ -156,6 +299,7 @@ def test_account_data_deleted_retroactively_by_background_job_if_deactivated(
             ),
         )
 
+
     def test_account_data_preserved_by_background_job_if_not_deactivated(self) -> None:
         """
         Tests that the background job does not scrub account data for users that have