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

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

Co-authored-by: Patrick Cloke <clokep@users.noreply.github.com>

changelog.d/11621.feature

+Remove account data (including client config, push rules and ignored users) upon user deactivation.
\ No newline at end of file

docs/admin_api/user_admin_api.md

@@ -353,6 +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*. 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`:
@@ -366,7 +371,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 @@ class DeactivateAccountHandler:
         # 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

@@ -26,6 +26,7 @@ from synapse.storage.database import (
     LoggingTransaction,
 )
 from synapse.storage.databases.main.cache import CacheInvalidationWorkerStore
+from synapse.storage.databases.main.push_rule import PushRulesWorkerStore
 from synapse.storage.engines import PostgresEngine
 from synapse.storage.util.id_generators import (
     AbstractStreamIdGenerator,
@@ -44,7 +45,7 @@ if TYPE_CHECKING:
 logger = logging.getLogger(__name__)
 
 
-class AccountDataWorkerStore(CacheInvalidationWorkerStore):
+class AccountDataWorkerStore(PushRulesWorkerStore, CacheInvalidationWorkerStore):
     def __init__(
         self,
         database: DatabasePool,
@@ -179,7 +180,7 @@ class AccountDataWorkerStore(CacheInvalidationWorkerStore):
         else:
             return None
 
-    @cached(num_args=2)
+    @cached(num_args=2, tree=True)
     async def get_account_data_for_room(
         self, user_id: str, room_id: str
     ) -> Dict[str, JsonDict]:
@@ -546,6 +547,74 @@ class AccountDataWorkerStore(CacheInvalidationWorkerStore):
         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 the account data for a user.
+
+        This is intended to be used upon user deactivation and also removes any
+        derived information from account data (e.g. push rules and ignored users).
+
+        Args:
+            user_id: The user ID to remove data for.
+        """
+
+        def purge_account_data_for_user_txn(txn: LoggingTransaction) -> None:
+            # 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.
+            #      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}
+            )
+
+            # 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)
+
+        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

+# 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 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
+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 _deactivate_my_account(self):
+        """
+        Deactivates the account `self.user` using `self.token` and asserts
+        that it returns a 200 success code.
+        """
+        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, 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(
+                    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()
+        )