From 9140318f4a6342d28e863acb42b3ffd8d3ea0732 Mon Sep 17 00:00:00 2001 From: Ryan Barrett Date: Mon, 26 Jun 2023 20:22:06 -0700 Subject: [PATCH] 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 | 11 ++-- models.py | 16 +++--- protocol.py | 110 +++++++++++++++++++++++--------------- tests/test_activitypub.py | 78 +++++++++++++++++---------- tests/test_models.py | 5 +- tests/test_pages.py | 7 ++- tests/test_protocol.py | 43 ++++++++++++++- tests/testutil.py | 14 +++-- web.py | 2 +- 9 files changed, 193 insertions(+), 93 deletions(-) diff --git a/activitypub.py b/activitypub.py index 8ec9272..c6a6cde 100644 --- a/activitypub.py +++ b/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 { diff --git a/models.py b/models.py index b2bdb05..2465582 100644 --- a/models.py +++ b/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) diff --git a/protocol.py b/protocol.py index b337201..f1a693b 100644 --- a/protocol.py +++ b/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): diff --git a/tests/test_activitypub.py b/tests/test_activitypub.py index be0d495..8cc17a6 100644 --- a/tests/test_activitypub.py +++ b/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('') 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']]) diff --git a/tests/test_models.py b/tests/test_models.py index cc48104..a828e90 100644 --- a/tests/test_models.py +++ b/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"})) diff --git a/tests/test_pages.py b/tests/test_pages.py index ac71b10..255d9fe 100644 --- a/tests/test_pages.py +++ b/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( diff --git a/tests/test_protocol.py b/tests/test_protocol.py index 6c499ee..96960d7 100644 --- a/tests/test_protocol.py +++ b/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']) diff --git a/tests/testutil.py b/tests/testutil.py index ad11e31..89cfddc 100644 --- a/tests/testutil.py +++ b/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, diff --git a/web.py b/web.py index dc09b9d..45e4d0a 100644 --- a/web.py +++ b/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}')