ATProto: switch from #tombstone to #account active=False for deleting…

… accounts

...including reactivating with #account active=True. it works!

for #1130, #1119

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:

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`.

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

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()
 

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 @[email protected] tombstones the Bluesky repo.
+    def test_activitypub_block_bsky_bot_user_deactivates_atproto_repo(self, mock_get):
+        """AP Block of @[email protected] 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("""\
 <html>
   <body class="h-card"><a rel="me" href="/">me</a> #nobridge</body>
 </html>""", 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