ATProto: when disabling, also remove DNS entry

for #1268

atproto.py

@@ -453,8 +453,8 @@ def create_for(cls, user):
 
         user.put()
 
-    @staticmethod
-    def set_dns(handle, did):
+    @classmethod
+    def set_dns(cls, handle, did):
         """Create _atproto DNS record for handle resolution.
 
         https://atproto.com/specs/handle#handle-resolution
@@ -478,25 +478,48 @@ def set_dns(handle, did):
         zone = dns_client.zone(DNS_ZONE)
         changes = zone.changes()
 
+        logger.info('Checking for existing record')
+        ATProto.remove_dns(handle)
+
+        changes.add_record_set(zone.resource_record_set(name=name, record_type='TXT',
+                                                        ttl=DNS_TTL, rrdatas=[val]))
+        changes.create()
+        logger.info('done!')
+
+    @classmethod
+    def remove_dns(cls, handle):
+        """Removes an _atproto DNS record.
+
+        https://atproto.com/specs/handle#handle-resolution
+
+        Args:
+          handle (str): Bluesky handle, eg ``snarfed.org.web.brid.gy``
+        """
+        name = f'_atproto.{handle}.'
+        logger.info(f'removing GCP DNS TXT record for {name}')
+        if DEBUG:
+            logger.info('  skipped since DEBUG is true')
+            return
+
+        # https://cloud.google.com/python/docs/reference/dns/latest
+        # https://cloud.google.com/dns/docs/reference/rest/v1/
+        zone = dns_client.zone(DNS_ZONE)
+        changes = zone.changes()
+
         # sadly can't check if the record exists with the google.cloud.dns API
         # because it doesn't support list_resource_record_sets's name param.
         # heed to use the generic discovery-based API instead.
         # https://cloud.google.com/python/docs/reference/dns/latest/zone#listresourcerecordsetsmaxresultsnone-pagetokennone-clientnone
         # https://github.com/googleapis/python-dns/issues/31#issuecomment-1595105412
         # https://cloud.google.com/apis/docs/client-libraries-explained
         # https://googleapis.github.io/google-api-python-client/docs/dyn/dns_v1.resourceRecordSets.html
-        logger.info('Checking for existing record')
         resp = dns_discovery_api.resourceRecordSets().list(
             project=DNS_GCP_PROJECT, managedZone=DNS_ZONE, type='TXT', name=name,
         ).execute()
         for existing in resp.get('rrsets', []):
             logger.info(f'  deleting {existing}')
             changes.delete_record_set(ResourceRecordSet.from_api_repr(existing, zone=zone))
-
-        changes.add_record_set(zone.resource_record_set(name=name, record_type='TXT',
-                                                        ttl=DNS_TTL, rrdatas=[val]))
         changes.create()
-        logger.info('done!')
 
     @classmethod
     def set_username(to_cls, user, username):
@@ -616,6 +639,7 @@ def send(to_cls, obj, url, from_user=None, orig_obj_id=None):
             if atp_base_id == did:
                 logger.info(f'Deactivating bridged ATProto account {did} !')
                 arroba.server.storage.deactivate_repo(repo)
+                to_cls.remove_dns(user.handle_as('atproto'))
                 return True
 
         if not record:

tests/test_atproto.py

@@ -1905,10 +1905,29 @@ def test_send_skips_add_to_collection(self, mock_create_task):
         self.assertEqual(0, AtpRepo.query().count())
         mock_create_task.assert_not_called()
 
+    @patch('atproto.DEBUG', new=False)
+    @patch.object(google.cloud.dns.client.ManagedZone, 'changes')
+    @patch.object(atproto.dns_discovery_api, 'resourceRecordSets')
     @patch.object(tasks_client, 'create_task', return_value=Task(name='my task'))
-    def test_send_delete_actor(self, mock_create_task):
+    def test_send_delete_actor(self, mock_create_task, mock_rrsets, mock_changes):
         user = self.make_user_and_repo()
 
+        mock_changes.return_value = changes = MagicMock()
+        mock_rrsets.return_value = rrsets = MagicMock()
+        rrsets.list.return_value = list_ = MagicMock()
+
+        dns_name = '_atproto.ha.nl.'
+        list_.execute.return_value = {
+            'rrsets': [{
+                'name': dns_name,
+                'type': 'TXT',
+                'ttl': 300,
+                'rrdatas': ['"did=did:abc:xyz"'],
+                'kind': 'dns#resourceRecordSet',
+            }],
+            'kind': 'dns#resourceRecordSetsListResponse',
+        }
+
         delete = self.store_object(id='fake:delete', source_protocol='fake', our_as1={
             'objectType': 'activity',
             'verb': 'delete',
@@ -1934,6 +1953,16 @@ def test_send_delete_actor(self, mock_create_task):
 
         mock_create_task.assert_called()  # atproto-commit
 
+        rrsets.list.assert_called_with(
+            project=DNS_GCP_PROJECT, managedZone=DNS_ZONE, type='TXT', name=dns_name)
+        changes.delete_record_set.assert_called_once()
+        rrset = changes.delete_record_set.call_args[0][0]
+        self.assertEqual(DNS_ZONE, rrset.zone.name)
+        self.assertEqual(dns_name, rrset.name)
+        self.assertEqual('TXT', rrset.record_type)
+        self.assertEqual(300, rrset.ttl)
+        self.assertEqual(['"did=did:abc:xyz"'], rrset.rrdatas)
+
     @patch.object(tasks_client, 'create_task', return_value=Task(name='my task'))
     def test_send_from_deleted_actor(self, mock_create_task):
         self.make_user_and_repo()