Protocol.receive and ATProto.send: detect CRUD of object without id

fixes https://console.cloud.google.com/errors/detail/CO_yiJ_PrYSuvQE;locations=global;time=P30D?project=bridgy-federated&inv=1&invt=AbjgeA

atproto.py

@@ -578,9 +578,12 @@ class ATProto(User, Protocol):
         base_obj = obj
         base_obj_as1 = obj.as1
         allow_opt_out = (type == 'delete')
-        if type in ('post', 'update', 'delete', 'undo'):
+        if type in as1.CRUD_VERBS:
             base_obj_as1 = as1.get_object(obj.as1)
-            base_id = base_obj_as1['id']
+            base_id = base_obj_as1.get('id')
+            if not base_id:
+                logger.info(f'{type} object has no id!')
+                return False
             base_obj = PROTOCOLS[obj.source_protocol].load(base_id, remote=False)
             if type not in ('delete', 'undo'):
                 if not base_obj:  # probably a new repo

protocol.py

@@ -917,6 +917,11 @@ class Protocol:
         obj.add('users', from_user.key)
 
         inner_obj_as1 = as1.get_object(obj.as1)
+        inner_obj_id = inner_obj_as1.get('id')
+        if obj.type in as1.CRUD_VERBS | set(('like', 'share')):
+            if not inner_obj_id:
+                error(f'{obj.type} object has no id!')
+
         if obj.type in as1.CRUD_VERBS:
             if inner_owner := as1.get_owner(inner_obj_as1):
                 if inner_owner_key := from_cls.key_for(inner_owner):
@@ -926,7 +931,6 @@ class Protocol:
         obj.put()
 
         # store inner object
-        inner_obj_id = inner_obj_as1.get('id')
         if obj.type in ('post', 'update') and inner_obj_as1.keys() > set(['id']):
             Object.get_or_create(inner_obj_id, our_as1=inner_obj_as1,
                                  source_protocol=from_cls.LABEL, authed_as=actor)
@@ -959,16 +963,10 @@ class Protocol:
             # TODO: do we convert stop-following to webmention 410 of original
             # follow?
 
-        elif obj.type in ('update', 'like', 'share'):  # require object
-            if not inner_obj_id:
-                error("Couldn't find id of object to update")
-
             # fall through to deliver to followers
 
         elif obj.type in ('delete', 'undo'):
-            if not inner_obj_id:
-                error("Couldn't find id of object to delete")
-
+            assert inner_obj_id
             logger.info(f'Marking Object {inner_obj_id} deleted')
             Object.get_or_create(inner_obj_id, deleted=True, authed_as=authed_as)
 

tests/test_atproto.py

@@ -2320,6 +2320,21 @@ Sed tortor neque, aliquet quis posuere aliquam […]
             'https://chat.local/xrpc/chat.bsky.convo.getConvoForMembers?members=did%3Aplc%3Aalice',
             json=None, data=None, headers=ANY)
 
+    def test_send_object_without_id(self):
+        user = self.make_user_and_repo()
+
+        # got an activity like this from Pandacap
+        # https://github.com/IsaacSchemm/Pandacap
+        dm = Object(id='fake:post', source_protocol='fake', our_as1={
+            'objectType': 'activity',
+            'verb': 'post',
+            'object': [{
+                'Item1': 'id',
+                'Item2': 'https://pandacap.azurewebsites.net/#transient-abc-123',
+            }],
+        })
+        self.assertFalse(ATProto.send(dm, 'https://bsky.brid.gy/'))
+
     def test_datastore_client_get_record_datastore_object(self):
         self.make_user_and_repo()
         post = {

tests/test_protocol.py

@@ -982,6 +982,26 @@ class ProtocolReceiveTest(TestCase):
             ('other:bob:target', obj.as1),
         ], OtherFake.sent)
 
+    def test_create_post_object_missing_id(self):
+        self.make_followers()
+
+        # got an activity like this from Pandacap
+        # https://github.com/IsaacSchemm/Pandacap
+        create_as1 = {
+            'id': 'fake:create',
+            'objectType': 'activity',
+            'verb': 'post',
+            'actor': 'fake:user',
+            'object': [{
+                'Item1': 'id',
+                'Item2': 'https://pandacap.azurewebsites.net/#transient-abc-123',
+            }],
+        }
+        with self.assertRaises(ErrorButDoNotRetryTask):
+            Fake.receive_as1(create_as1)
+
+        self.assertEqual([], OtherFake.sent)
+
     def test_create_post_bare_object(self):
         self.make_followers()
 

tests/test_web.py

@@ -1120,9 +1120,10 @@ class WebTest(TestCase):
         mock_get.return_value = requests_response(
             NOTE_HTML.replace('<a href="http://localhost/"></a>', """
   <a class="u-like-of" href="https://alice.com/post"></a>
-  <a class="u-bookmark-of" href="http://bob.com/post"></a>
+  <a class="u-repost-of" href="http://bob.com/post"></a>
+  <a class="u-url" href=""></a>
   <a href="http://localhost/"></a>
-"""), url='https://user.com/post')
+"""))
 
         got = self.post('/queue/webmention', data={
             'source': 'https://user.com/multiple',
@@ -1131,21 +1132,19 @@ class WebTest(TestCase):
         self.assertEqual(202, got.status_code)
 
         inboxes = ['https://inbox', 'https://public/inbox', 'https://shared/inbox']
-        expected = {
-            **NOTE_AS2,
-            'attributedTo': None,
-            'type': 'Create',
+        self.assert_deliveries(mock_post, inboxes, {
+            '@context': ['https://www.w3.org/ns/activitystreams'],
+            'type': 'Announce',
+            'id': 'http://localhost/r/https://user.com/multiple',
             'actor': 'http://localhost/user.com',
-            'content': 'hello i am a post',
-            # TODO: this is an awkward wart left over from the multi-type mf2.
-            # remove it eventually.
-            'object': {
-                'targetUrl': 'http://bob.com/post',
-                'to': ['https://www.w3.org/ns/activitystreams#Public'],
-            },
-        }
-        del expected['contentMap']
-        self.assert_deliveries(mock_post, inboxes, expected)
+            'content': '<p>hello i am a post</p>',
+            'contentMap': {'en': '<p>hello i am a post</p>'},
+            'name': 'hello i am a post',
+            'object': 'http://localhost/r/http://bob.com/post',
+            'to': ['https://www.w3.org/ns/activitystreams#Public'],
+            'cc': ['http://localhost/user.com',
+                   'https://www.w3.org/ns/activitystreams#Public'],
+        })
 
     def test_create_default_url_to_wm_source(self, mock_get, mock_post):
         """Source post has no u-url. AS2 id should default to webmention source."""

tests/testutil.py

@@ -158,8 +158,11 @@ class Fake(User, protocol.Protocol):
         from_ = PROTOCOLS.get(obj.source_protocol)
         if (from_ and from_ != to and to.HAS_COPIES
                 and obj.type not in ('update', 'delete')):
+            obj_id = as1.get_object(obj.as1).get('id')
+            if not obj_id:
+                return False
             if obj.type == 'post':
-                obj = Object.get_by_id(as1.get_object(obj.as1)['id'])
+                obj = Object.get_by_id(obj_id)
             copy_id = ids.translate_object_id(
                 id=obj.key.id(), from_=from_, to=to)
             util.add(obj.copies, Target(uri=copy_id, protocol=to.LABEL))