Remove account data (including client config, push rules and ignored users) upon user deactivation.

changelog.d/11621.feature

@@ -0,0 +1 @@
+Remove account data (including client config, push rules and ignored users) upon user deactivation.

docs/admin_api/user_admin_api.md

@@ -352,6 +352,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*. 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`:
@@ -365,7 +370,6 @@ The following actions are **NOT** performed. The list may be incomplete.
 - Remove mappings of SSO IDs
 - [Delete media uploaded](#delete-media-uploaded-by-a-user) by user (included avatar images)
 - Delete sent and received messages
-- Delete E2E cross-signing keys
 - Remove the user's creation (registration) timestamp
 - [Remove rate limit overrides](#override-ratelimiting-for-users)
 - Remove from monthly active users

synapse/handlers/deactivate_account.py

@@ -157,6 +157,9 @@ async def deactivate_account(
         # Mark the user as deactivated.
         await self.store.set_user_deactivated_status(user_id, True)
 
+        # Remove account data (including ignored users and push rules).
+        await self.store.purge_account_data_for_user(user_id)
+
         return identity_server_supports_unbinding
 
     async def _reject_pending_invites_for_user(self, user_id: str) -> None:

synapse/storage/databases/main/account_data.py

@@ -546,6 +546,49 @@ def _add_account_data_for_user(
         for ignored_user_id in previously_ignored_users ^ currently_ignored_users:
             self._invalidate_cache_and_stream(txn, self.ignored_by, (ignored_user_id,))
 
+    async def purge_account_data_for_user(self, user_id: str) -> None:
+        """
+        Removes ALL the account data for a user.
+        Intended to be used upon user deactivation.
+
+        Also purges the user from the ignored_users table and the push_rules tables,
+        because those are derived from account data.
+        """
+
+        def purge_account_data_for_user_txn(txn: LoggingTransaction) -> None:
+            # Purge from the primary account_data table.
+            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.
+            #      It's also not this user's data to delete!
+            self.db_pool.simple_delete_txn(
+                txn, table="ignored_users", keyvalues={"ignorer_user_id": user_id}
+            )
+
+            # Remove the push rules
+            self.db_pool.simple_delete_txn(
+                txn, table="push_rules", keyvalues={"user_name": user_id}
+            )
+            self.db_pool.simple_delete_txn(
+                txn, table="push_rules_enable", keyvalues={"user_name": user_id}
+            )
+            self.db_pool.simple_delete_txn(
+                txn, table="push_rules_stream", keyvalues={"user_id": user_id}
+            )
+
+        await self.db_pool.runInteraction(
+            "purge_account_data_for_user_txn",
+            purge_account_data_for_user_txn,
+        )
+
 
 class AccountDataStore(AccountDataWorkerStore):
     pass

tests/handlers/test_deactivate_account.py

@@ -0,0 +1,269 @@
+# Copyright 2021 The Matrix.org Foundation C.I.C.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# 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 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
+from synapse.util import Clock
+
+from tests.unittest import HomeserverTestCase
+
+
+class DeactivateAccountTestCase(HomeserverTestCase):
+    servlets = [
+        login.register_servlets,
+        admin.register_servlets,
+        account.register_servlets,
+    ]
+
+    def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None:
+        self._store = hs.get_datastore()
+
+        self.user = self.register_user("user", "pass")
+        self.token = self.login("user", "pass")
+
+    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(
+                    AccountDataTypes.DIRECT,
+                    self.user,
+                )
+            ),
+        )
+
+        # Request the deactivation of our account
+        req = self.get_success(
+            self.make_request(
+                "POST",
+                "account/deactivate",
+                {
+                    "auth": {
+                        "type": "m.login.password",
+                        "user": self.user,
+                        "password": "pass",
+                    },
+                    "erase": True,
+                },
+                access_token=self.token,
+            )
+        )
+        self.assertEqual(req.code, 200, req)
+
+        # Clear the cache (for testing)
+        self._store.get_global_account_data_by_type_for_user.invalidate_all()
+
+        # 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,
+                )
+            ),
+        )
+
+    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
+        req = self.get_success(
+            self.make_request(
+                "POST",
+                "account/deactivate",
+                {
+                    "auth": {
+                        "type": "m.login.password",
+                        "user": self.user,
+                        "password": "pass",
+                    },
+                    "erase": True,
+                },
+                access_token=self.token,
+            )
+        )
+        self.assertEqual(req.code, 200, req)
+
+        # Clear the cache (for testing)
+        self._store.get_account_data_for_room_and_type.invalidate_all()
+
+        # 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
+        req = self.get_success(
+            self.make_request(
+                "POST",
+                "account/deactivate",
+                {
+                    "auth": {
+                        "type": "m.login.password",
+                        "user": self.user,
+                        "password": "pass",
+                    },
+                    "erase": True,
+                },
+                access_token=self.token,
+            )
+        )
+        self.assertEqual(req.code, 200, req)
+
+        # Test the rule no longer exists (after clearing the cache)
+        self._store.get_push_rules_for_user.invalidate_all()
+        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
+        req = self.get_success(
+            self.make_request(
+                "POST",
+                "account/deactivate",
+                {
+                    "auth": {
+                        "type": "m.login.password",
+                        "user": self.user,
+                        "password": "pass",
+                    },
+                    "erase": True,
+                },
+                access_token=self.token,
+            )
+        )
+        self.assertEqual(req.code, 200, req)
+
+        # Invalidate the cache
+        self._store.ignored_by.invalidate_all()
+
+        # 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()
+        )