generalize Protocol.accept_follow, other bug fixes in Protocol.receive

for #529, fixes https://console.cloud.google.com/errors/detail/CMbJ6a6j56LErwE;time=P30D?project=bridgy-federated

activitypub.py

@@ -627,13 +627,12 @@ def actor(protocol, domain):
         error('', status=404)
 
     cls = PROTOCOLS[protocol]
-    g.user = cls.get_by_id(domain)
-    if not g.user:
+    g.user = cls.get_or_create(domain)
+    if not g.user.obj or not g.user.obj.as1:
         try:
-            obj = cls.load(f'https://{domain}/', gateway=True)
-        except NoMicroformats:
-            obj = None
-        g.user = cls.get_or_create(id=domain, obj=obj)
+            g.user.obj = cls.load(f'https://{domain}/', gateway=True)
+        except NoMicroformats as e:
+            pass
 
     # TODO: unify with common.actor()
     actor = g.user.as2() or {

models.py

@@ -382,13 +382,13 @@ class Object(StringIdModel):
         # if bool(self.as2) + bool(self.bsky) + bool(self.mf2) > 1:
         #     logger.warning(f'{self.key} has multiple! {bool(self.as2)} {bool(self.bsky)} {bool(self.mf2)}')
 
-        if self.our_as1 is not None:
+        if self.our_as1:
             return redirect_unwrap(self.our_as1)
-        elif self.as2 is not None:
+        elif self.as2:
             return as2.to_as1(redirect_unwrap(self.as2))
-        elif self.bsky is not None:
+        elif self.bsky:
             return bluesky.to_as1(self.bsky)
-        elif self.mf2 is not None:
+        elif self.mf2:
             return microformats2.json_to_object(self.mf2,
                                                 rel_urls=self.mf2.get('rel-urls'))
 
@@ -496,9 +496,11 @@ class Object(StringIdModel):
             # outbound; show a nice link to the user
             return g.user.user_page_link()
 
-        actor = (util.get_first(self.as1, 'actor')
-                 or util.get_first(self.as1, 'author')
-                 or {})
+        actor = {}
+        if self.as1:
+            actor = (util.get_first(self.as1, 'actor')
+                     or util.get_first(self.as1, 'author')
+                     or {})
         if isinstance(actor, str):
             return common.pretty_link(actor, attrs=attrs)
 

protocol.py

@@ -179,6 +179,7 @@ class Protocol:
         if util.is_web(id):
             by_domain = Protocol.for_domain(id)
             if by_domain:
+                logger.info(f'  {by_domain.__name__} owns {id}')
                 return by_domain
 
         # step 2: check if any Protocols say conclusively that they own it
@@ -189,17 +190,19 @@ class Protocol:
         for protocol in protocols:
             owns = protocol.owns_id(id)
             if owns:
+                logger.info(f'  {protocol.__name__} owns {id}')
                 return protocol
             elif owns is not False:
                 candidates.append(protocol)
 
         if len(candidates) == 1:
+            logger.info(f'  {candidates[0].__name__} owns {id}')
             return candidates[0]
 
         # step 3: look for existing Objects in the datastore
         obj = Protocol.load(id, remote=False)
         if obj and obj.source_protocol:
-            logger.info(f'{obj.key} has source_protocol {obj.source_protocol}')
+            logger.info(f'  {obj.key} owned by source_protocol {obj.source_protocol}')
             return PROTOCOLS[obj.source_protocol]
 
         # step 4: fetch over the network
@@ -207,8 +210,9 @@ class Protocol:
             logger.info(f'Trying {protocol.__name__}')
             try:
                 protocol.load(id, local=False, remote=True)
+                logger.info(f'  {protocol.__name__} owns {id}')
                 return protocol
-            except werkzeug.exceptions.HTTPException:
+            except werkzeug.exceptions.HTTPException as e:
                 # internal error we generated ourselves; try next protocol
                 pass
             except Exception as e:
@@ -352,13 +356,15 @@ class Protocol:
             error(f'Sorry, {obj.type} activities are not supported yet.', status=501)
 
         # store inner object
-        inner_obj = as1.get_object(obj.as1)
-        inner_obj_id = inner_obj.get('id')
-        if obj.type in ('post', 'create', 'update') and inner_obj.keys() > set(['id']):
-            to_update = (Object.get_by_id(inner_obj_id)
-                         or Object(id=inner_obj_id))
-            to_update.populate(as2=obj.as2['object'], source_protocol=from_cls.LABEL)
-            to_update.put()
+        inner_obj_as1 = as1.get_object(obj.as1)
+        inner_obj_id = inner_obj_as1.get('id')
+        inner_obj = None
+        if (obj.type in ('post', 'create', 'update')
+              and inner_obj_as1.keys() > set(['id'])):
+            inner_obj = Object.get_or_insert(inner_obj_id)
+            inner_obj.populate(our_as1=inner_obj_as1,
+                               source_protocol=from_cls.LABEL)
+            inner_obj.put()
 
         actor = as1.get_object(obj.as1, 'actor')
         actor_id = actor.get('id')
@@ -425,11 +431,16 @@ class Protocol:
 
         # fetch actor if necessary so we have name, profile photo, etc
         if actor and actor.keys() == set(['id']):
-            actor = obj.as2['actor'] = from_cls.load(actor['id']).as2
+            actor_obj = from_cls.load(actor['id'])
+            if actor_obj.as1:
+                obj.our_as1 = {**obj.as1, 'actor': actor_obj.as1}
 
         # fetch object if necessary so we can render it in feeds
-        if obj.type == 'share' and inner_obj.keys() == set(['id']):
-            inner_obj = obj.as2['object'] = from_cls.load(inner_obj_id).as_as2()
+        if obj.type == 'share' and inner_obj_as1.keys() == set(['id']):
+            if not inner_obj:
+                inner_obj = from_cls.load(inner_obj_id)
+            if inner_obj.as1:
+                obj.our_as1 = {**obj.as1, 'object': inner_obj.as1}
 
         if obj.type == 'follow':
             from_cls.accept_follow(obj)
@@ -439,11 +450,11 @@ class Protocol:
 
         # deliver original posts and reposts to followers
         is_reply = (obj.type == 'comment' or
-                    (inner_obj and inner_obj.get('inReplyTo')))
+                    (inner_obj_as1 and inner_obj_as1.get('inReplyTo')))
         if ((obj.type == 'share' or obj.type in ('create', 'post') and not is_reply)
-                and actor and actor_id):
+                and actor_id):
             logger.info(f'Delivering to followers of {actor_id}')
-            for f in Follower.query(Follower.to == from_cls(id=actor_id).key,
+            for f in Follower.query(Follower.to == from_cls.key_for(actor_id),
                                     Follower.status == 'active'):
                 if f.from_ not in obj.users:
                     obj.users.append(f.from_)
@@ -460,42 +471,57 @@ class Protocol:
         Args:
           obj: :class:`Object`, follow activity
         """
-        logger.info('Replying to Follow with Accept')
+        logger.info('Got follow. Loading users, storing Follow, sending accept')
 
-        followee_id = as1.get_object(obj.as1).get('id')
-        follower_as1 = as1.get_object(obj.as1, 'actor')
-        follower_id = follower_as1.get('id')
-        if not followee_id or not follower_id:
+        # Extract follower/followee objects and ids
+        from_as1 = as1.get_object(obj.as1, 'actor')
+        from_id = from_as1.get('id')
+        to_as1 = as1.get_object(obj.as1)
+        to_id = to_as1.get('id')
+        if not to_id or not from_id:
             error(f'Follow activity requires object and actor. Got: {obj.as1}')
 
-        # store Follower and follower User
-        #
-        # If followee user is already direct, follower may not know they're
-        # interacting with a bridge. If followee user is indirect though,
-        # follower should know, so they're direct.
-        follower_obj = cls.load(follower_id)
-        if not follower_obj.as1:
-            follower_obj.our_as1 = follower_as1
-            follower_obj.put()
+        # Store follower/followee Objects
+        from_cls = cls
+        from_obj = from_cls.load(from_id)
+        if not from_obj.as1:
+            from_obj.our_as1 = from_as1
+            from_obj.put()
 
-        target = cls.target_for(follower_obj)
-        if not target or not follower_id:
-            error(f"Couldn't find delivery target for follow actor {follower_obj}")
+        to_cls = Protocol.for_id(to_id)
+        to_obj = to_cls.load(to_id)
+        if not to_obj.as1:
+            to_obj.our_as1 = to_as1
+            to_obj.put()
 
-        from_ = cls.get_or_create(id=follower_id, obj=follower_obj,
-                                  direct=not g.user.direct)
-        follower_obj = Follower.get_or_create(to=g.user, from_=from_, follow=obj.key,
-                                              status='active')
+        target = from_cls.target_for(from_obj)
+        if not target:
+            error(f"Couldn't find delivery target for follower {from_obj}")
 
-        # send accept
-        accept = {
-            'id': common.host_url(g.user.user_page_path(f'followers#accept-{obj.key.id()}')),
+        # If followee user is alread direct, follower may not know they're
+        # interacting with a bridge. f followee user is indirect though,
+        # follower should know, so the're direct.
+        to_key = to_cls.key_for(to_id)
+        to_user = to_cls.get_or_create(id=to_key.id(), obj=to_obj, direct=False)
+
+        from_key = from_cls.key_for(from_id)
+        from_user = from_cls.get_or_create(id=from_key.id(), obj=from_obj,
+                                           direct=not to_user.direct)
+
+        follower_obj = Follower.get_or_create(to=to_user, from_=from_user,
+                                              follow=obj.key, status='active')
+
+        # send Accept
+        id = common.host_url(to_user.user_page_path(
+            f'followers#accept-{obj.key.id()}'))
+        accept = Object.get_or_insert(id, our_as1={
+            'id': id,
             'objectType': 'activity',
             'verb': 'accept',
-            'actor': followee_id,
+            'actor': to_id,
             'object': obj.as1,
-        }
-        return cls.send(Object(our_as1=accept), target)
+        })
+        return cls.send(accept, target)
 
     @classmethod
     def deliver(cls, obj):

tests/test_activitypub.py

@@ -275,7 +275,7 @@ class ActivityPubTest(TestCase):
         self.request_context.push()
 
         self.user = self.make_user('user.com', has_hcard=True, has_redirects=True,
-                                   obj_as2=ACTOR)
+                                   obj_as2={**ACTOR, 'id': 'https://user.com/'})
 
         ACTOR_BASE['publicKey']['publicKeyPem'] = self.user.public_pem().decode()
 
@@ -290,7 +290,9 @@ class ActivityPubTest(TestCase):
         self.key_id_obj.put()
 
     def assert_object(self, id, **props):
-        return super().assert_object(id, delivered_protocol='web', **props)
+        ignore = ['as2'] if 'our_as1' in props and 'as2' not in props else []
+        return super().assert_object(id, delivered_protocol='web',
+                                     ignore=ignore, **props)
 
     def sign(self, path, body):
         """Constructs HTTP Signature, returns headers."""
@@ -358,7 +360,10 @@ class ActivityPubTest(TestCase):
         self.assertEqual(404, got.status_code)
 
     def test_actor_new_user_fetch(self, _, mock_get, __):
+        self.user.obj_key.delete()
         self.user.key.delete()
+        protocol.objects_cache.clear()
+
         mock_get.return_value = requests_response(test_web.ACTOR_HTML)
 
         got = self.client.get('/user.com')
@@ -366,7 +371,10 @@ class ActivityPubTest(TestCase):
         self.assert_equals(ACTOR_BASE_FULL, got.json, ignore=['publicKeyPem'])
 
     def test_actor_new_user_fetch_no_mf2(self, _, mock_get, __):
+        self.user.obj_key.delete()
         self.user.key.delete()
+        protocol.objects_cache.clear()
+
         mock_get.return_value = requests_response('<html></html>')
 
         got = self.client.get('/user.com')
@@ -426,7 +434,7 @@ class ActivityPubTest(TestCase):
                                mock_head, mock_get, mock_post)
         self.assert_object(REPLY_OBJECT['id'],
                            source_protocol='activitypub',
-                           as2=REPLY_OBJECT,
+                           our_as1=as2.to_as1(REPLY_OBJECT),
                            type='comment')
 
     def _test_inbox_reply(self, reply, expected_props, mock_head, mock_get, mock_post):
@@ -507,22 +515,27 @@ class ActivityPubTest(TestCase):
 
         got = self.post(path, json=NOTE)
         self.assertEqual(200, got.status_code, got.get_data(as_text=True))
-        expected_as2 = common.redirect_unwrap({
-            **NOTE,
-            'actor': ACTOR,
+
+        expected_obj = as2.to_as1(NOTE_OBJECT)
+        expected_obj['author'] = {'id': 'https://masto.foo/@author'}
+        self.assert_object(NOTE_OBJECT['id'],
+                           source_protocol='activitypub',
+                           our_as1=expected_obj,
+                           type='note')
+
+        expected_create = as2.to_as1(common.redirect_unwrap(NOTE))
+        expected_create.update({
+            'actor': as2.to_as1(ACTOR),
+            'object': expected_obj,
         })
 
         self.assert_object('http://mas.to/note/as2',
                            source_protocol='activitypub',
-                           as2=expected_as2,
+                           our_as1=expected_create,
                            users=[self.user.key, Fake(id='http://baz').key],
                            type='post',
                            labels=['activity', 'feed'],
                            object_ids=[NOTE_OBJECT['id']])
-        self.assert_object(NOTE_OBJECT['id'],
-                           source_protocol='activitypub',
-                           as2=NOTE_OBJECT,
-                           type='note')
 
     def test_repost_of_indieweb(self, mock_head, mock_get, mock_post):
         mock_head.return_value = requests_response(url='https://user.com/orig')
@@ -561,7 +574,7 @@ class ActivityPubTest(TestCase):
         self.assert_object(REPOST_FULL['id'],
                            source_protocol='activitypub',
                            status='complete',
-                           as2=repost,
+                           our_as1=as2.to_as1(repost),
                            users=[self.user.key],
                            delivered=['https://user.com/orig'],
                            type='share',
@@ -595,7 +608,7 @@ class ActivityPubTest(TestCase):
         self.assert_object(REPOST['id'],
                            source_protocol='activitypub',
                            status='ignored',
-                           as2=REPOST_FULL,
+                           our_as1=as2.to_as1(REPOST_FULL),
                            users=[self.user.key, Fake(id='http://baz').key],
                            type='share',
                            labels=['activity', 'feed'],
@@ -620,7 +633,10 @@ class ActivityPubTest(TestCase):
                            users=[],
                            source_protocol='activitypub',
                            status='ignored',
-                           as2={**LIKE_WITH_ACTOR, 'object': 'http://nope.com/post'},
+                           our_as1=as2.to_as1({
+                               **LIKE_WITH_ACTOR,
+                               'object': 'http://nope.com/post',
+                           }),
                            type='like',
                            labels=['activity'],
                            object_ids=['http://nope.com/post'])
@@ -667,7 +683,7 @@ class ActivityPubTest(TestCase):
         expected_as2['tag'][1]['href'] = 'https://tar.get/'
         self.assert_object(MENTION_OBJECT['id'],
                            source_protocol='activitypub',
-                           as2=expected_as2,
+                           our_as1=as2.to_as1(expected_as2),
                            type='note')
 
     def _test_inbox_mention(self, mention, expected_props, mock_head, mock_get, mock_post):
@@ -733,7 +749,7 @@ class ActivityPubTest(TestCase):
                            users=[self.user.key],
                            source_protocol='activitypub',
                            status='complete',
-                           as2=LIKE_WITH_ACTOR,
+                           our_as1=as2.to_as1(LIKE_WITH_ACTOR),
                            delivered=['https://user.com/post'],
                            type='like',
                            labels=['notification', 'activity'],
@@ -760,7 +776,7 @@ class ActivityPubTest(TestCase):
                            users=[self.user.key],
                            source_protocol='activitypub',
                            status='complete',
-                           as2=follow,
+                           our_as1=as2.to_as1(follow),
                            delivered=['https://user.com/'],
                            type='follow',
                            labels=['notification', 'activity'],
@@ -784,7 +800,7 @@ class ActivityPubTest(TestCase):
                            users=[self.user.key],
                            source_protocol='activitypub',
                            status='complete',
-                           as2=follow,
+                           our_as1=as2.to_as1(follow),
                            delivered=['https://user.com/'],
                            type='follow',
                            labels=['notification', 'activity'],
@@ -806,7 +822,7 @@ class ActivityPubTest(TestCase):
                            users=[self.user.key],
                            source_protocol='activitypub',
                            status='complete',
-                           as2=follow,
+                           our_as1=as2.to_as1(follow),
                            delivered=[],
                            type='follow',
                            labels=['notification', 'activity'],
@@ -857,9 +873,10 @@ class ActivityPubTest(TestCase):
             Follower.query().fetch(),
             ignore=['created', 'updated'])
 
-        self.assert_user(ActivityPub, ACTOR['id'],
-                         obj_as2=ACCEPT['object']['actor'],
-                         direct=True)
+        self.assert_user(ActivityPub, 'https://mas.to/users/swentel',
+                         obj_as2=ACTOR, direct=True)
+        self.assert_user(Web, 'user.com', direct=False,
+                         has_hcard=True, has_redirects=True)
 
     def test_inbox_follow_use_instead_strip_www(self, mock_head, mock_get, mock_post):
         self.make_user('www.user.com', use_instead=self.user.key)
@@ -1139,11 +1156,18 @@ class ActivityPubTest(TestCase):
         resp = self.post('/ap/sharedInbox', json=UPDATE_NOTE)
         self.assertEqual(200, resp.status_code)
 
-        obj = UPDATE_NOTE['object']
-        self.assert_object('https://a/note', type='note', as2=obj,
+        self.assert_object('https://a/note',
+                           type='note',
+                           our_as1=as2.to_as1({
+                               **UPDATE_NOTE['object'],
+                               'author': {'id': 'https://mas.to/users/swentel'},
+                           }),
                            source_protocol='activitypub')
-        self.assert_object(UPDATE_NOTE['id'], source_protocol='activitypub',
-                           type='update', status='complete', as2=UPDATE_NOTE,
+        self.assert_object(UPDATE_NOTE['id'],
+                           source_protocol='activitypub',
+                           type='update',
+                           status='complete',
+                           as2=UPDATE_NOTE,
                            labels=['activity'])
 
         self.assert_entities_equal(Object.get_by_id('https://a/note'),
@@ -1182,7 +1206,7 @@ class ActivityPubTest(TestCase):
                            users=[self.user.key],
                            source_protocol='activitypub',
                            status='complete',
-                           as2=LIKE_WITH_ACTOR,
+                           our_as1=as2.to_as1(LIKE_WITH_ACTOR),
                            type='like',
                            labels=['activity', 'notification'],
                            object_ids=[LIKE['object']])

tests/test_models.py

@@ -208,8 +208,9 @@ class ObjectTest(TestCase):
               'icon': {'type': 'Image', 'url': 'http://pic'},
           }}),
         ):
-            obj = Object(id='x', as2=as2)
-            self.assert_multiline_in(expected, obj.actor_link())
+            with self.subTest(expected=expected, as2=as2):
+                obj = Object(id='x', as2=as2)
+                self.assert_multiline_in(expected, obj.actor_link())
 
     def test_actor_link_user(self):
         g.user = Fake(id='user.com', obj=Object(id='a', as2={"name": "Alice"}))

tests/test_pages.py

@@ -56,10 +56,11 @@ class PagesTest(TestCase):
 
     def test_user_web_custom_username_doesnt_redirect(self):
         """https://github.com/snarfed/bridgy-fed/issues/534"""
-        self.user.obj_key = Object(id='a', as2={
+        self.user.obj = Object(id='a', as2={
             **ACTOR_AS2,
             'url': 'acct:baz@user.com',
-        }).put()
+        })
+        self.user.obj.put()
         self.user.put()
         self.assertEqual('baz', self.user.username())
 
@@ -148,6 +149,7 @@ class PagesTest(TestCase):
             to=self.user,
             from_=self.make_user('unused', cls=Fake, obj_as2={
                 **ACTOR,
+                'id': 'unused',
                 'url': 'http://stored/users/follow',
             }))
         Follower.get_or_create(
@@ -186,6 +188,7 @@ class PagesTest(TestCase):
             from_=self.user,
             to=self.make_user('unused', cls=Fake, obj_as2={
                 **ACTOR,
+                'id': 'unused',
                 'url': 'http://stored/users/follow',
             }))
         Follower.get_or_create(

tests/test_protocol.py

@@ -2,6 +2,7 @@
 from unittest.mock import patch
 
 from flask import g
+from granary import as2
 from oauth_dropins.webutil.testutil import requests_response
 import requests
 
@@ -25,7 +26,7 @@ REPLY = {
     'actor': ACTOR,
     'object': {
         **REPLY['object'],
-        'author': ACTOR,
+        'attributedTo': ACTOR,
     },
 }
 
@@ -135,7 +136,7 @@ class ProtocolTest(TestCase):
                            source_protocol='fake',
                            )
         self.assert_object(REPLY['object']['id'],
-                           as2=REPLY['object'],
+                           our_as1=as2.to_as1(REPLY['object']),
                            type='comment',
                            source_protocol='fake',
                            )
@@ -302,3 +303,41 @@ class ProtocolTest(TestCase):
     def test_remote_false_local_false_assert(self):
         with self.assertRaises(AssertionError):
             Fake.load('nope', local=False, remote=False)
+
+
+class ProtocolReceiveTest(TestCase):
+
+    def setUp(self):
+        super().setUp()
+        g.user = self.make_user('fake:user', cls=Fake, obj_id='fake:user')
+        self.alice = self.make_user('fake:alice', cls=Fake, obj_id='fake:alice')
+        self.bob = self.make_user('fake:bob', cls=Fake, obj_id='fake:bob')
+
+    def assert_object(self, id, **props):
+        return super().assert_object(id, delivered_protocol='fake', **props)
+
+    def test_follow_no_g_user(self):
+        """No user from request, eg delivered to our ActivityPub shared inbox."""
+        g.user = None
+
+        follow_as1 = {
+            'objectType': 'activity',
+            'verb': 'follow',
+            'id': 'fake:follow',
+            'actor': 'fake:alice',
+            'object': 'fake:bob',
+        }
+        self.assertEqual('OK', Fake.receive('fake:follow', our_as1=follow_as1))
+
+        obj = self.assert_object('fake:follow',
+                                 our_as1=follow_as1,
+                                 type='follow',
+                                 source_protocol='fake',
+                                 labels=['activity'],
+                                 status='ignored',
+                                 )
+        self.assert_entities_equal(
+            Follower(to=self.bob.key, from_=self.alice.key, status='active',
+                     follow=obj.key),
+            Follower.query().get(),
+            ignore=['created', 'updated'])

tests/testutil.py

@@ -90,7 +90,7 @@ class Fake(User, protocol.Protocol):
     @classmethod
     def target_for(cls, obj, shared=False):
         assert obj.source_protocol in (cls.LABEL, cls.ABBREV, 'ui', None)
-        return 'shared target' if shared else f'target: {self.key.id()}'
+        return 'shared:target' if shared else f'{obj.key.id()}:target'
 
 
 # used in TestCase.make_user() to reuse keys across Users since they're
@@ -191,10 +191,16 @@ class TestCase(unittest.TestCase, testutil.Asserts):
     def make_user(self, id, cls=Web, **kwargs):
         """Reuse RSA key across Users because generating it is expensive."""
         obj_key = None
-        obj_as2 = kwargs.pop('obj_as2', None)
-        if obj_as2:
-            obj_key = Object(id=str(self.last_make_user_id), as2=obj_as2).put()
+
+        obj_as2 = kwargs.pop('obj_as2', None) or {}
+        obj_mf2 = kwargs.pop('obj_mf2', None) or {}
+        obj_id = kwargs.pop('obj_id', None)
+        if not obj_id:
+            obj_id = (obj_as2.get('id')
+                      or util.get_url(obj_mf2, 'properties')
+                      or str(self.last_make_user_id))
             self.last_make_user_id += 1
+        obj_key = Object(id=obj_id, as2=obj_as2, mf2=obj_mf2).put()
 
         user = cls(id=id,
                    direct=True,

web.py

@@ -129,7 +129,7 @@ class Web(User, Protocol):
                 if url and url.startswith('acct:'):
                     try:
                         urluser, urldomain = util.parse_acct_uri(url)
-                    except ValueError:
+                    except ValueError as e:
                         continue
                     if urldomain == id:
                         logger.info(f'Found custom username: {urluser}')