From 68b663d60f59a4ae4d2906f1a648ca2af13d11ab Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 21 Dec 2021 11:39:25 +0000 Subject: [PATCH 01/23] Remove account data upon user deactivation --- synapse/handlers/deactivate_account.py | 3 ++ .../storage/databases/main/account_data.py | 39 +++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/synapse/handlers/deactivate_account.py b/synapse/handlers/deactivate_account.py index bee62cf36088..7a13d76a687f 100644 --- a/synapse/handlers/deactivate_account.py +++ b/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: diff --git a/synapse/storage/databases/main/account_data.py b/synapse/storage/databases/main/account_data.py index 32a553fdd7bd..83066a9c3baf 100644 --- a/synapse/storage/databases/main/account_data.py +++ b/synapse/storage/databases/main/account_data.py @@ -546,6 +546,45 @@ 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 cache table + and the push_rules cache tables. + """ + + 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} + ) + + # 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_name": user_id} + ) + + await self.db_pool.runInteraction( + "purge_account_data_for_user_txn", + purge_account_data_for_user_txn, + ) + class AccountDataStore(AccountDataWorkerStore): pass From 13fec0d8d86324ab1c74f60bc4085b95bbdb0163 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 21 Dec 2021 11:39:49 +0000 Subject: [PATCH 02/23] Document that account data is removed upon user deactivation --- docs/admin_api/user_admin_api.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/admin_api/user_admin_api.md b/docs/admin_api/user_admin_api.md index ba574d795fcc..25048a95e05e 100644 --- a/docs/admin_api/user_admin_api.md +++ b/docs/admin_api/user_admin_api.md @@ -352,6 +352,7 @@ 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.) The following additional actions are performed during deactivation if `erase` is set to `true`: From c1fed495ce23fee1aee924262e2723ed2dd0f320 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 21 Dec 2021 11:41:07 +0000 Subject: [PATCH 03/23] Newsfile Signed-off-by: Olivier Wilkinson (reivilibre) --- changelog.d/11621.feature | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/11621.feature diff --git a/changelog.d/11621.feature b/changelog.d/11621.feature new file mode 100644 index 000000000000..dc426fb658ac --- /dev/null +++ b/changelog.d/11621.feature @@ -0,0 +1 @@ +Remove account data (including client config, push rules and ignored users) upon user deactivation. \ No newline at end of file From 4da869ea1af50f92d5ae29d63882571e55954811 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 21 Dec 2021 11:39:25 +0000 Subject: [PATCH 04/23] Remove account data upon user deactivation --- synapse/storage/databases/main/account_data.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/account_data.py b/synapse/storage/databases/main/account_data.py index 83066a9c3baf..4ebc44308b78 100644 --- a/synapse/storage/databases/main/account_data.py +++ b/synapse/storage/databases/main/account_data.py @@ -577,7 +577,7 @@ def purge_account_data_for_user_txn(txn: LoggingTransaction) -> None: txn, table="push_rules_enable", keyvalues={"user_name": user_id} ) self.db_pool.simple_delete_txn( - txn, table="push_rules_stream", keyvalues={"user_name": user_id} + txn, table="push_rules_stream", keyvalues={"user_id": user_id} ) await self.db_pool.runInteraction( From b132bbab3a8e67f5b17018db852df1d6cc0d29e8 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Wed, 29 Dec 2021 13:19:00 +0000 Subject: [PATCH 05/23] Test the removal of account data upon deactivation --- tests/handlers/test_deactivate_account.py | 78 +++++++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 tests/handlers/test_deactivate_account.py diff --git a/tests/handlers/test_deactivate_account.py b/tests/handlers/test_deactivate_account.py new file mode 100644 index 000000000000..34b98f079a7e --- /dev/null +++ b/tests/handlers/test_deactivate_account.py @@ -0,0 +1,78 @@ +# 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 twisted.test.proto_helpers import MemoryReactor + +from synapse.api.constants import AccountDataTypes +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: + 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: + """ + Tests that 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"]}, + ) + ) + + # 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) + + # 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, + ) + ), + ) From ab970037502d3bd129f935ede033ffbc689aaec3 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Tue, 4 Jan 2022 14:12:41 +0000 Subject: [PATCH 06/23] Clarify and document what is included in account data --- docs/admin_api/user_admin_api.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/admin_api/user_admin_api.md b/docs/admin_api/user_admin_api.md index 25048a95e05e..47e23a8afd17 100644 --- a/docs/admin_api/user_admin_api.md +++ b/docs/admin_api/user_admin_api.md @@ -352,7 +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* (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`: @@ -366,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 From 5d4c6ca5ef176b6c77af37a92e2f6c4a09ca211d Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 13 Jan 2022 14:39:04 +0000 Subject: [PATCH 07/23] Clarify why we purge ignored users and push rules --- synapse/storage/databases/main/account_data.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/databases/main/account_data.py b/synapse/storage/databases/main/account_data.py index 4ebc44308b78..0013583c1b09 100644 --- a/synapse/storage/databases/main/account_data.py +++ b/synapse/storage/databases/main/account_data.py @@ -551,8 +551,8 @@ 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 cache table - and the push_rules cache tables. + 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: From 6d7e9acb65cf9305a5ace3fce84c6f33a37bf54f Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 13 Jan 2022 14:39:57 +0000 Subject: [PATCH 08/23] Remove needless super call --- tests/handlers/test_deactivate_account.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/handlers/test_deactivate_account.py b/tests/handlers/test_deactivate_account.py index 34b98f079a7e..a34c43de4dc2 100644 --- a/tests/handlers/test_deactivate_account.py +++ b/tests/handlers/test_deactivate_account.py @@ -30,7 +30,6 @@ 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") From d73be6c1a166a3dd98e030ce891c8f169d29e9b7 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 13 Jan 2022 16:32:39 +0000 Subject: [PATCH 09/23] Also delete room account data --- synapse/storage/databases/main/account_data.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/synapse/storage/databases/main/account_data.py b/synapse/storage/databases/main/account_data.py index 0013583c1b09..b0ab054c1348 100644 --- a/synapse/storage/databases/main/account_data.py +++ b/synapse/storage/databases/main/account_data.py @@ -561,6 +561,10 @@ def purge_account_data_for_user_txn(txn: LoggingTransaction) -> None: 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. From e58ec5d1182b276e8d3a90dd9c00db7f40fc04c5 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 13 Jan 2022 16:41:48 +0000 Subject: [PATCH 10/23] Ensure the test actually does something --- tests/handlers/test_deactivate_account.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/tests/handlers/test_deactivate_account.py b/tests/handlers/test_deactivate_account.py index a34c43de4dc2..8e7eae63eecf 100644 --- a/tests/handlers/test_deactivate_account.py +++ b/tests/handlers/test_deactivate_account.py @@ -35,9 +35,9 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: self.user = self.register_user("user", "pass") self.token = self.login("user", "pass") - def test_account_data_deleted_upon_deactivation(self) -> None: + def test_global_account_data_deleted_upon_deactivation(self) -> None: """ - Tests that account data is removed upon deactivation. + Tests that global account data is removed upon deactivation. """ # Add some account data self.get_success( @@ -48,6 +48,16 @@ def test_account_data_deleted_upon_deactivation(self) -> None: ) ) + # 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( @@ -66,6 +76,9 @@ def test_account_data_deleted_upon_deactivation(self) -> None: ) 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( From 00f9033f8ee72b122588f4bab51a5f486fe89095 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 13 Jan 2022 16:41:58 +0000 Subject: [PATCH 11/23] Add a test for room account data --- tests/handlers/test_deactivate_account.py | 55 +++++++++++++++++++++++ 1 file changed, 55 insertions(+) diff --git a/tests/handlers/test_deactivate_account.py b/tests/handlers/test_deactivate_account.py index 8e7eae63eecf..5273a74996f8 100644 --- a/tests/handlers/test_deactivate_account.py +++ b/tests/handlers/test_deactivate_account.py @@ -88,3 +88,58 @@ def test_global_account_data_deleted_upon_deactivation(self) -> None: ) ), ) + + 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" + ) + ), + ) From 21234353c2e1889857904ca53aed145e345a4e87 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Mon, 17 Jan 2022 10:10:05 +0000 Subject: [PATCH 12/23] Add a test for push rules --- tests/handlers/test_deactivate_account.py | 76 +++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/tests/handlers/test_deactivate_account.py b/tests/handlers/test_deactivate_account.py index 5273a74996f8..a0ac8071bc4a 100644 --- a/tests/handlers/test_deactivate_account.py +++ b/tests/handlers/test_deactivate_account.py @@ -11,9 +11,12 @@ # 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 @@ -143,3 +146,76 @@ def test_room_account_data_deleted_upon_deactivation(self) -> None: ) ), ) + + 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) From a1a8c68910b20f3077bfc79dbb2c763947cdfe83 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Mon, 17 Jan 2022 10:24:47 +0000 Subject: [PATCH 13/23] Add a test for ignored users --- tests/handlers/test_deactivate_account.py | 48 +++++++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/tests/handlers/test_deactivate_account.py b/tests/handlers/test_deactivate_account.py index a0ac8071bc4a..71bea77f86b9 100644 --- a/tests/handlers/test_deactivate_account.py +++ b/tests/handlers/test_deactivate_account.py @@ -219,3 +219,51 @@ def test_push_rules_deleted_upon_account_deactivation(self) -> None: 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() + ) From 3edab888765ea5c9ca1a47cc1d908205dd00bd79 Mon Sep 17 00:00:00 2001 From: reivilibre Date: Thu, 20 Jan 2022 17:01:52 +0000 Subject: [PATCH 14/23] Update synapse/storage/databases/main/account_data.py Co-authored-by: Patrick Cloke --- synapse/storage/databases/main/account_data.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/synapse/storage/databases/main/account_data.py b/synapse/storage/databases/main/account_data.py index b0ab054c1348..bd406183264f 100644 --- a/synapse/storage/databases/main/account_data.py +++ b/synapse/storage/databases/main/account_data.py @@ -548,11 +548,13 @@ def _add_account_data_for_user( 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. + Removes the account data for a user. - Also purges the user from the ignored_users table and the push_rules tables, - because those are derived from account data. + 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: From 249b05b9c1901238c4fc3908f641091c9cc2890e Mon Sep 17 00:00:00 2001 From: reivilibre Date: Thu, 20 Jan 2022 17:01:57 +0000 Subject: [PATCH 15/23] Update synapse/storage/databases/main/account_data.py Co-authored-by: Patrick Cloke --- synapse/storage/databases/main/account_data.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/account_data.py b/synapse/storage/databases/main/account_data.py index bd406183264f..19c95db36031 100644 --- a/synapse/storage/databases/main/account_data.py +++ b/synapse/storage/databases/main/account_data.py @@ -558,7 +558,7 @@ async def purge_account_data_for_user(self, user_id: str) -> None: """ def purge_account_data_for_user_txn(txn: LoggingTransaction) -> None: - # 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} ) From 37dc2e1dbbed9b0dda472e4492ac9e481323f83f Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 20 Jan 2022 17:05:30 +0000 Subject: [PATCH 16/23] Extract account deactivation test helper --- tests/handlers/test_deactivate_account.py | 91 +++++++---------------- 1 file changed, 27 insertions(+), 64 deletions(-) diff --git a/tests/handlers/test_deactivate_account.py b/tests/handlers/test_deactivate_account.py index 71bea77f86b9..e0823c6cfe07 100644 --- a/tests/handlers/test_deactivate_account.py +++ b/tests/handlers/test_deactivate_account.py @@ -11,6 +11,7 @@ # 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 @@ -38,6 +39,28 @@ def prepare(self, reactor: MemoryReactor, clock: Clock, hs: HomeServer) -> None: 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. @@ -62,22 +85,7 @@ def test_global_account_data_deleted_upon_deactivation(self) -> None: ) # 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) + self._deactivate_my_account() # Clear the cache (for testing) self._store.get_global_account_data_by_type_for_user.invalidate_all() @@ -118,22 +126,7 @@ def test_room_account_data_deleted_upon_deactivation(self) -> None: ) # 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) + self._deactivate_my_account() # Clear the cache (for testing) self._store.get_account_data_for_room_and_type.invalidate_all() @@ -195,22 +188,7 @@ def test_push_rules_deleted_upon_account_deactivation(self) -> None: ) # 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) + self._deactivate_my_account() # Test the rule no longer exists (after clearing the cache) self._store.get_push_rules_for_user.invalidate_all() @@ -243,22 +221,7 @@ def test_ignored_users_deleted_upon_deactivation(self) -> None: ) # 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) + self._deactivate_my_account() # Invalidate the cache self._store.ignored_by.invalidate_all() From 81a2863e73fe605b79ec33a4cb12b8b0272ed3fd Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 20 Jan 2022 17:06:55 +0000 Subject: [PATCH 17/23] Antilint --- synapse/storage/databases/main/account_data.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/synapse/storage/databases/main/account_data.py b/synapse/storage/databases/main/account_data.py index 19c95db36031..628c4022afb2 100644 --- a/synapse/storage/databases/main/account_data.py +++ b/synapse/storage/databases/main/account_data.py @@ -553,8 +553,8 @@ async def purge_account_data_for_user(self, user_id: str) -> None: 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. + Args: + user_id: The user ID to remove data for. """ def purge_account_data_for_user_txn(txn: LoggingTransaction) -> None: From 87473f4cdf2b757c8e9d68d555e67e08648f0a01 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 20 Jan 2022 17:35:23 +0000 Subject: [PATCH 18/23] Don't invalidate caches just for tests --- tests/handlers/test_deactivate_account.py | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/tests/handlers/test_deactivate_account.py b/tests/handlers/test_deactivate_account.py index e0823c6cfe07..3eedef891adc 100644 --- a/tests/handlers/test_deactivate_account.py +++ b/tests/handlers/test_deactivate_account.py @@ -87,9 +87,6 @@ def test_global_account_data_deleted_upon_deactivation(self) -> None: # Request the deactivation of our account self._deactivate_my_account() - # 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( @@ -128,9 +125,6 @@ def test_room_account_data_deleted_upon_deactivation(self) -> None: # Request the deactivation of our account self._deactivate_my_account() - # 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( @@ -190,8 +184,6 @@ def test_push_rules_deleted_upon_account_deactivation(self) -> None: # Request the deactivation of our account self._deactivate_my_account() - # 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)) @@ -223,9 +215,6 @@ def test_ignored_users_deleted_upon_deactivation(self) -> None: # Request the deactivation of our account self._deactivate_my_account() - # 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() From e13780e25b1846dfe1b4ebe16207b12ec7bdb076 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Thu, 20 Jan 2022 17:43:07 +0000 Subject: [PATCH 19/23] Handle change in arg order for get_global_account_data_by_type_for_user --- tests/handlers/test_deactivate_account.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/handlers/test_deactivate_account.py b/tests/handlers/test_deactivate_account.py index 3eedef891adc..3da597768c62 100644 --- a/tests/handlers/test_deactivate_account.py +++ b/tests/handlers/test_deactivate_account.py @@ -78,8 +78,7 @@ def test_global_account_data_deleted_upon_deactivation(self) -> None: self.assertIsNotNone( self.get_success( self._store.get_global_account_data_by_type_for_user( - AccountDataTypes.DIRECT, - self.user, + self.user, AccountDataTypes.DIRECT ) ), ) @@ -91,8 +90,7 @@ def test_global_account_data_deleted_upon_deactivation(self) -> None: self.assertIsNone( self.get_success( self._store.get_global_account_data_by_type_for_user( - AccountDataTypes.DIRECT, - self.user, + self.user, AccountDataTypes.DIRECT ) ), ) From 75f1e2f200d9196ae8230545e82b22abba0e93a5 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 21 Jan 2022 08:51:40 +0000 Subject: [PATCH 20/23] Invalidate caches properly when purging account data --- .../storage/databases/main/account_data.py | 26 ++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/account_data.py b/synapse/storage/databases/main/account_data.py index 1a682c009b78..b362e943f6ad 100644 --- a/synapse/storage/databases/main/account_data.py +++ b/synapse/storage/databases/main/account_data.py @@ -26,6 +26,7 @@ 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 @@ logger = logging.getLogger(__name__) -class AccountDataWorkerStore(CacheInvalidationWorkerStore): +class AccountDataWorkerStore(PushRulesWorkerStore, CacheInvalidationWorkerStore): def __init__( self, database: DatabasePool, @@ -586,6 +587,29 @@ def purge_account_data_for_user_txn(txn: LoggingTransaction) -> None: 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, From 1b28e2fef75ceb90b1b7d6c565f8a592829684c3 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 21 Jan 2022 08:52:48 +0000 Subject: [PATCH 21/23] Drive-by type annotations --- synapse/storage/databases/main/push_rule.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index e01c94930aed..cf0227dff041 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -32,6 +32,7 @@ AbstractStreamIdTracker, StreamIdGenerator, ) +from synapse.types import JsonDict from synapse.util import json_encoder from synapse.util.caches.descriptors import cached, cachedList from synapse.util.caches.stream_change_cache import StreamChangeCache @@ -126,7 +127,7 @@ def get_max_push_rules_stream_id(self): raise NotImplementedError() @cached(max_entries=5000) - async def get_push_rules_for_user(self, user_id): + async def get_push_rules_for_user(self, user_id: str) -> List[JsonDict]: rows = await self.db_pool.simple_select_list( table="push_rules", keyvalues={"user_name": user_id}, @@ -150,7 +151,7 @@ async def get_push_rules_for_user(self, user_id): return _load_rules(rows, enabled_map, use_new_defaults) @cached(max_entries=5000) - async def get_push_rules_enabled_for_user(self, user_id) -> Dict[str, bool]: + async def get_push_rules_enabled_for_user(self, user_id: str) -> Dict[str, bool]: results = await self.db_pool.simple_select_list( table="push_rules_enable", keyvalues={"user_name": user_id}, From a68607c7319ac59fa9934a416fbc7f6242d4ec38 Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 21 Jan 2022 08:53:05 +0000 Subject: [PATCH 22/23] Make `get_account_data_for_room` a tree cache (missed one) --- synapse/storage/databases/main/account_data.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/synapse/storage/databases/main/account_data.py b/synapse/storage/databases/main/account_data.py index b362e943f6ad..5bfa408f74b0 100644 --- a/synapse/storage/databases/main/account_data.py +++ b/synapse/storage/databases/main/account_data.py @@ -180,7 +180,7 @@ async def get_global_account_data_by_type_for_user( 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]: From 97ceb01f36dde31532cce4d1f14a048b0bfcddeb Mon Sep 17 00:00:00 2001 From: "Olivier Wilkinson (reivilibre)" Date: Fri, 21 Jan 2022 13:43:50 +0000 Subject: [PATCH 23/23] Revert "Drive-by type annotations" It's never that easy... This reverts commit 1b28e2fef75ceb90b1b7d6c565f8a592829684c3. --- synapse/storage/databases/main/push_rule.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/synapse/storage/databases/main/push_rule.py b/synapse/storage/databases/main/push_rule.py index cf0227dff041..e01c94930aed 100644 --- a/synapse/storage/databases/main/push_rule.py +++ b/synapse/storage/databases/main/push_rule.py @@ -32,7 +32,6 @@ AbstractStreamIdTracker, StreamIdGenerator, ) -from synapse.types import JsonDict from synapse.util import json_encoder from synapse.util.caches.descriptors import cached, cachedList from synapse.util.caches.stream_change_cache import StreamChangeCache @@ -127,7 +126,7 @@ def get_max_push_rules_stream_id(self): raise NotImplementedError() @cached(max_entries=5000) - async def get_push_rules_for_user(self, user_id: str) -> List[JsonDict]: + async def get_push_rules_for_user(self, user_id): rows = await self.db_pool.simple_select_list( table="push_rules", keyvalues={"user_name": user_id}, @@ -151,7 +150,7 @@ async def get_push_rules_for_user(self, user_id: str) -> List[JsonDict]: return _load_rules(rows, enabled_map, use_new_defaults) @cached(max_entries=5000) - async def get_push_rules_enabled_for_user(self, user_id: str) -> Dict[str, bool]: + async def get_push_rules_enabled_for_user(self, user_id) -> Dict[str, bool]: results = await self.db_pool.simple_select_list( table="push_rules_enable", keyvalues={"user_name": user_id},