From aaf49e0f37425c417843877668d9e4de2174c039 Mon Sep 17 00:00:00 2001 From: Ryan Barrett Date: Tue, 22 Oct 2024 13:49:18 -0700 Subject: [PATCH] ATProto: switch from #tombstone to #account active=False for deleting accounts ...including reactivating with #account active=True. it works! for #1130, #1119 --- atproto.py | 14 ++++++++++---- protocol.py | 4 +++- tests/test_atproto.py | 35 ++++++++++++++++++++++++++++++---- tests/test_atproto_firehose.py | 4 +++- tests/test_integrations.py | 22 +++++++++------------ 5 files changed, 56 insertions(+), 23 deletions(-) diff --git a/atproto.py b/atproto.py index 98b43fb5..c77be31b 100644 --- a/atproto.py +++ b/atproto.py @@ -370,6 +370,9 @@ def is_blocklisted(url, allow_internal=False): def create_for(cls, user): """Creates an ATProto repo and profile for a non-ATProto user. + If the repo already exists, reactivates it by emitting an #account event + with active: True. + Args: user (models.User) @@ -379,7 +382,10 @@ def create_for(cls, user): """ assert not isinstance(user, ATProto) - if user.get_copy(ATProto): + if copy_did := user.get_copy(ATProto): + repo = arroba.server.storage.load_repo(copy_did) + arroba.server.storage.activate_repo(repo) + common.create_task(queue='atproto-commit') return # create new DID, repo @@ -556,7 +562,7 @@ def send(to_cls, obj, url, from_user=None, orig_obj_id=None): repo.callback = lambda _: common.create_task(queue='atproto-commit') # non-commit operations: - # * delete actor => tombstone repo + # * delete actor => deactivate repo # * flag => send report to mod service # * stop-following => delete follow record (prepared above) # * dm => chat message @@ -566,8 +572,8 @@ def send(to_cls, obj, url, from_user=None, orig_obj_id=None): else ids.translate_user_id(from_=from_cls, to=to_cls, id=base_id)) if atp_base_id == did: - logger.info(f'Deleting bridged ATProto account {did} by tombstoning repo!') - arroba.server.storage.tombstone_repo(repo) + logger.info(f'Deactivating bridged ATProto account {did} !') + arroba.server.storage.deactivate_repo(repo) return True if not record: diff --git a/protocol.py b/protocol.py index e827e49e..e0590275 100644 --- a/protocol.py +++ b/protocol.py @@ -460,10 +460,12 @@ def bot_user_id(cls): @classmethod def create_for(cls, user): - """Creates a copy user in this protocol. + """Creates or re-activate a copy user in this protocol. Should add the copy user to :attr:`copies`. + If the copy user already exists and active, should do nothing. + Args: user (models.User): original source user. Shouldn't already have a copy user for this protocol in :attr:`copies`. diff --git a/tests/test_atproto.py b/tests/test_atproto.py index b771da72..e1c5a530 100644 --- a/tests/test_atproto.py +++ b/tests/test_atproto.py @@ -1108,6 +1108,31 @@ def test_create_for_bad_handle(self): with self.assertRaises(ValueError): ATProto.create_for(Fake(id=bad)) + @patch.object(tasks_client, 'create_task', return_value=Task(name='my task')) + def test_create_for_already_exists(self, mock_create_task): + """Should mostly be a noop, but should emit an active: True #account event.""" + self.make_user_and_repo() + repo = arroba.server.storage.load_repo('did:plc:user') + arroba.server.storage.deactivate_repo(repo) + + ATProto.create_for(self.user) + + # check repo + repo = arroba.server.storage.load_repo('did:plc:user') + self.assertIsNone(repo.status) + + # check #account event + seq = self.storage.last_seq(SUBSCRIBE_REPOS_NSID) + self.assertEqual({ + '$type': 'com.atproto.sync.subscribeRepos#account', + 'seq': seq, + 'did': 'did:plc:user', + 'time': NOW.isoformat(), + 'active': True, + }, next(self.storage.read_events_by_seq(seq))) + + mock_create_task.assert_called() # atproto-commit + @patch('atproto.DEBUG', new=False) @patch.object(google.cloud.dns.client.ManagedZone, 'changes') @patch.object(atproto.dns_discovery_api, 'resourceRecordSets') @@ -1238,7 +1263,7 @@ def test_send_new_repo(self, mock_post, mock_create_task, _): }, genesis_op) # check atproto-commit task - self.assertEqual(2, mock_create_task.call_count) + self.assertEqual(4, mock_create_task.call_count) self.assert_task(mock_create_task, 'atproto-commit') @patch('requests.get', return_value=requests_response( @@ -1748,16 +1773,18 @@ def test_send_delete_actor(self, mock_create_task): self.assertTrue(ATProto.send(delete, 'https://bsky.brid.gy/', from_user=user)) + self.assertFalse(self.user.is_enabled(ATProto)) did = self.user.key.get().get_copy(ATProto) - with self.assertRaises(arroba.util.TombstonedRepo): - self.storage.load_repo(did) + assert did seq = self.storage.last_seq(SUBSCRIBE_REPOS_NSID) self.assertEqual({ - '$type': 'com.atproto.sync.subscribeRepos#tombstone', + '$type': 'com.atproto.sync.subscribeRepos#account', 'seq': seq, 'did': did, 'time': NOW.isoformat(), + 'active': False, + 'status': 'deactivated', }, next(self.storage.read_events_by_seq(seq))) mock_create_task.assert_called() # atproto-commit diff --git a/tests/test_atproto_firehose.py b/tests/test_atproto_firehose.py index 89901ec8..1ecd15f9 100644 --- a/tests/test_atproto_firehose.py +++ b/tests/test_atproto_firehose.py @@ -404,11 +404,13 @@ def test_load_dids_atprepo(self): }) self.assertIn('did:plc:eve', atproto_firehose.bridged_dids) - def test_load_dids_tombstoned_atprepo(self): + def test_load_dids_tombstoned_deactivated_atprepos(self): FakeWebsocketClient.to_receive = [({'op': 1, 't': '#info'}, {})] AtpRepo(id='did:plc:eve', head='', signing_key_pem=b'', status=arroba.util.TOMBSTONED).put() + AtpRepo(id='did:plc:frank', head='', signing_key_pem=b'', + status=arroba.util.DEACTIVATED).put() self.subscribe() diff --git a/tests/test_integrations.py b/tests/test_integrations.py index f106f670..3707f667 100644 --- a/tests/test_integrations.py +++ b/tests/test_integrations.py @@ -6,7 +6,7 @@ from arroba.datastore_storage import DatastoreStorage from arroba.repo import Repo -from arroba.util import dag_cbor_cid, TombstonedRepo +from arroba.util import dag_cbor_cid from dns.resolver import NXDOMAIN from granary import as2, bluesky from granary.tests.test_bluesky import ACTOR_PROFILE_BSKY, POST_BSKY @@ -617,8 +617,8 @@ def test_atproto_block_ap_bot_user_disables_protocol_deletes_actor( @patch('requests.get', side_effect=[ requests_response('blob', headers={'Content-Type': 'image/jpeg'}), # http://pic/ ]) - def test_activitypub_block_bsky_bot_user_tombstones_atproto_repo(self, mock_get): - """AP Block of @bsky.brid.gy@bsky.brid.gy tombstones the Bluesky repo. + def test_activitypub_block_bsky_bot_user_deactivates_atproto_repo(self, mock_get): + """AP Block of @bsky.brid.gy@bsky.brid.gy deactivates the Bluesky repo. ActivityPub user @alice@inst , https://inst/alice , did:plc:alice Block is https://inst/block @@ -641,15 +641,14 @@ def test_activitypub_block_bsky_bot_user_tombstones_atproto_repo(self, mock_get) user = ActivityPub.get_by_id('https://inst/alice') self.assertFalse(user.is_enabled(ATProto)) - with self.assertRaises(TombstonedRepo): - self.storage.load_repo('did:plc:alice') + self.assertEqual('deactivated', self.storage.load_repo('did:plc:alice').status) @patch('requests.get', side_effect=[ requests_response('blob', headers={'Content-Type': 'image/jpeg'}), # http://pic/ ]) - def test_activitypub_delete_user_tombstones_atproto_repo(self, mock_get): - """AP Delete of user tombstones the Bluesky repo. + def test_activitypub_delete_user_deactivates_atproto_repo(self, mock_get): + """AP Delete of user deactivates the Bluesky repo. ActivityPub user @alice@inst , https://inst/alice , did:plc:alice Delete is https://inst/block @@ -672,16 +671,14 @@ def test_activitypub_delete_user_tombstones_atproto_repo(self, mock_get): user = ActivityPub.get_by_id('https://inst/alice') self.assertFalse(user.is_enabled(ATProto)) - with self.assertRaises(TombstonedRepo): - self.storage.load_repo('did:plc:alice') - + self.assertEqual('deactivated', self.storage.load_repo('did:plc:alice').status) @patch('requests.post', return_value=requests_response('')) @patch('requests.get', return_value=requests_response("""\ me #nobridge """, url='https://alice.com/')) - def test_web_nobridge_refresh_profile_deletes_user_tombstones_atproto_repo( + def test_web_nobridge_refresh_profile_deletes_user_deactivates_atproto_repo( self, mock_get, mock_post): """Web user adds #nobridge and refreshes their profile. @@ -705,8 +702,7 @@ def test_web_nobridge_refresh_profile_deletes_user_tombstones_atproto_repo( # should be deleted everywhere self.assertEqual('opt-out', alice.key.get().status) - with self.assertRaises(TombstonedRepo): - self.storage.load_repo('did:plc:alice') + self.assertEqual('deactivated', self.storage.load_repo('did:plc:alice').status) self.assertEqual(1, mock_post.call_count) args, kwargs = mock_post.call_args