From 4d095fa3d9a5309a74444f5de8adeca79a7f365e Mon Sep 17 00:00:00 2001 From: Ryan Barrett Date: Wed, 15 Nov 2023 14:23:08 -0800 Subject: [PATCH] misc cleanup * remove obsolete TODOs, commented out code * remove obsolete circular imports of per-protocol modules * minimize Object put in Protocol.load * remove duplicated Protocol.load tests in test_activitypub * re-enable rest of ActivityPubUtilsTest.test_postprocess_as2_idempotent * drop default cls=Web in TestCase.make_user --- activitypub.py | 4 -- app.py | 13 +----- atproto.py | 2 - ids.py | 3 +- models.py | 12 +----- protocol.py | 12 +++--- scripts/activity_model.py | 1 - templates/docs.html | 18 -------- tests/test_activitypub.py | 88 ++++----------------------------------- tests/test_atproto.py | 2 +- tests/test_convert.py | 7 ++-- tests/test_follow.py | 11 ++--- tests/test_ids.py | 4 +- tests/test_pages.py | 4 +- tests/test_protocol.py | 14 +++++-- tests/test_redirect.py | 2 +- tests/test_web.py | 6 +-- tests/test_webfinger.py | 2 +- tests/testutil.py | 3 +- 19 files changed, 49 insertions(+), 159 deletions(-) diff --git a/activitypub.py b/activitypub.py index 8b03306..d104c53 100644 --- a/activitypub.py +++ b/activitypub.py @@ -156,10 +156,6 @@ class ActivityPub(User, Protocol): @classmethod def target_for(cls, obj, shared=False): """Returns ``obj``'s or its author's/actor's inbox, if available.""" - # TODO: we have entities in prod that fail this, eg - # https://indieweb.social/users/bismark has source_protocol webmention - # assert obj.source_protocol in (cls.LABEL, cls.ABBREV, 'ui', None), str(obj) - if not obj.as1: return None diff --git a/app.py b/app.py index 190bdfd..2ff3369 100644 --- a/app.py +++ b/app.py @@ -6,18 +6,7 @@ registered. from flask_app import app # import all modules to register their Flask handlers -import ( - activitypub, - atproto, - convert, - follow, - pages, - redirect, - superfeedr, - ui, - webfinger, - web, -) +import activitypub, atproto, convert, follow, pages, redirect, superfeedr, ui, webfinger, web import models models.reset_protocol_properties() diff --git a/atproto.py b/atproto.py index 340aa49..b19ba93 100644 --- a/atproto.py +++ b/atproto.py @@ -449,7 +449,6 @@ def poll_notifications(): for cls in set(PROTOCOLS.values()) if cls and cls != ATProto)) - # TODO: convert to Session for connection pipelining! client = Client(f'https://{os.environ["APPVIEW_HOST"]}', headers={'User-Agent': USER_AGENT}) @@ -504,7 +503,6 @@ def poll_posts(): for cls in set(PROTOCOLS.values()) if cls and cls != ATProto)) - # TODO: convert to Session for connection pipelining! client = Client(f'https://{os.environ["APPVIEW_HOST"]}', headers={'User-Agent': USER_AGENT}) diff --git a/ids.py b/ids.py index 25aaadc..ab2dc74 100644 --- a/ids.py +++ b/ids.py @@ -111,7 +111,8 @@ def translate_handle(*, handle, from_proto, to_proto): case 'activitypub', 'web': user, instance = handle.lstrip('@').split('@') - return f'instance/@user' # TODO + # TODO: get this from the actor object's url field? + return f'https://{instance}/@{user}' case _, 'web': return handle diff --git a/models.py b/models.py index 2fb3a26..83cd512 100644 --- a/models.py +++ b/models.py @@ -544,8 +544,7 @@ class Object(StringIdModel): status = ndb.StringProperty(choices=STATUSES) # choices is populated in app, after all User subclasses are created, # so that PROTOCOLS is fully populated - # TODO: remove? is this redundant with the protocol-specific data fields below? - # TODO: otherwise, nail down whether this is ABBREV or LABEL + # TODO: nail down whether this is ABBREV or LABEL source_protocol = ndb.StringProperty(choices=[]) labels = ndb.StringProperty(repeated=True, choices=LABELS) @@ -588,13 +587,6 @@ class Object(StringIdModel): @ComputedJsonProperty def as1(self): - # TODO: bring back log or assert? we have prod entities that currently - # fail this though. - # assert (self.as2 is not None) ^ (self.bsky is not None) ^ (self.mf2 is not None), \ - # f'{self.as2} {self.bsky} {self.mf2}' - # 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: obj = self.our_as1 @@ -603,7 +595,7 @@ class Object(StringIdModel): elif self.bsky: owner, _, _ = parse_at_uri(self.key.id()) - ATProto = PROTOCOLS['atproto'] # TODO: circular import :( ??? + ATProto = PROTOCOLS['atproto'] handle = ATProto(id=owner).handle obj = bluesky.to_as1(self.bsky, repo_did=owner, repo_handle=handle, uri=self.key.id(), pds=ATProto.target_for(self)) diff --git a/protocol.py b/protocol.py index 3c06007..0dcb85c 100644 --- a/protocol.py +++ b/protocol.py @@ -50,7 +50,6 @@ objects_cache_lock = Lock() logger = logging.getLogger(__name__) -# TODO: merge Protocol and User classes? class Protocol: """Base protocol class. Not to be instantiated; classmethods only. @@ -652,10 +651,8 @@ class Protocol: error(f'Undo of Follow requires actor id and object id. Got: {actor_id} {inner_obj_id} {obj.as1}') # deactivate Follower - # TODO: avoid import? - from web import Web from_ = from_cls.key_for(actor_id) - to_cls = Protocol.for_id(inner_obj_id) or Web + to_cls = Protocol.for_id(inner_obj_id) to = to_cls.key_for(inner_obj_id) follower = Follower.query(Follower.to == to, Follower.from_ == from_, @@ -1149,9 +1146,10 @@ class Protocol: if obj.new is False: obj.changed = obj.activity_changed(orig_as1) - obj.source_protocol = cls.LABEL - # TODO: drop this? - obj.put() + if obj.source_protocol not in (cls.LABEL, cls.ABBREV): + assert not obj.source_protocol + obj.source_protocol = cls.LABEL + obj.put() with objects_cache_lock: objects_cache[id] = obj diff --git a/scripts/activity_model.py b/scripts/activity_model.py index 4eecbd5..ce510a9 100644 --- a/scripts/activity_model.py +++ b/scripts/activity_model.py @@ -27,7 +27,6 @@ class Activity(StringIdModel): source_atom = ndb.TextProperty() target_as2 = ndb.TextProperty() # JSON - # TODO: uncomment created = ndb.DateTimeProperty(auto_now_add=True) updated = ndb.DateTimeProperty(auto_now=True) diff --git a/templates/docs.html b/templates/docs.html index 534d06f..1c40c6f 100644 --- a/templates/docs.html +++ b/templates/docs.html @@ -136,24 +136,6 @@ RewriteRule ^.well-known/(host-meta|webfinger).* https://fed.brid.gy/$0 [redire - -
  • Add webmention support to your site. This is strongly recommended, but technically optional. You don't have to automate the webmentions to Bridgy Fed to federate your posts, and you don't have to accept the inbound webmentions that Bridgy Fed sends, but you'll have a much better experience if you do. Check out the IndieWeb wiki for instructions for your web server.
    • diff --git a/tests/test_activitypub.py b/tests/test_activitypub.py index 6519ae1..c6bfdbe 100644 --- a/tests/test_activitypub.py +++ b/tests/test_activitypub.py @@ -299,7 +299,8 @@ class ActivityPubTest(TestCase): super().setUp() self.request_context.push() - self.user = self.make_user('user.com', has_hcard=True, has_redirects=True, + self.user = self.make_user('user.com', cls=Web, has_hcard=True, + has_redirects=True, obj_as2={**ACTOR, 'id': 'https://user.com/'}) self.swentel_key = ndb.Key(ActivityPub, 'https://mas.to/users/swentel') self.masto_actor_key = ndb.Key(ActivityPub, 'https://mas.to/actor') @@ -952,7 +953,7 @@ class ActivityPubTest(TestCase): 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) + self.make_user('www.user.com', cls=Web, use_instead=self.user.key) mock_head.return_value = requests_response(url='https://www.user.com/') mock_get.side_effect = [ @@ -1573,7 +1574,7 @@ class ActivityPubTest(TestCase): class ActivityPubUtilsTest(TestCase): def setUp(self): super().setUp() - g.user = self.make_user('user.com', has_hcard=True, obj_as2=ACTOR) + g.user = self.make_user('user.com', cls=Web, has_hcard=True, obj_as2=ACTOR) def test_put_validates_id(self, *_): for bad in ( @@ -1764,80 +1765,6 @@ class ActivityPubUtilsTest(TestCase): self.assertEqual(['https://masto.foo/@other'], postprocess_as2(obj)['cc']) - # TODO: make these generic and use Fake - @patch('requests.get') - def test_load_http(self, mock_get): - mock_get.return_value = AS2 - - id = 'http://the/id' - self.assertIsNone(Object.get_by_id(id)) - - # first time fetches over HTTP - got = ActivityPub.load(id) - self.assert_equals(id, got.key.id()) - self.assert_equals(AS2_OBJ, got.as2) - mock_get.assert_has_calls([self.as2_req(id)]) - - # second time is in cache - got.key.delete() - mock_get.reset_mock() - - got = ActivityPub.load(id) - self.assert_equals(id, got.key.id()) - self.assert_equals(AS2_OBJ, got.as2) - mock_get.assert_not_called() - - @patch('requests.get') - def test_load_datastore(self, mock_get): - id = 'http://the/id' - stored = Object(id=id, as2=AS2_OBJ) - stored.put() - protocol.objects_cache.clear() - - # first time loads from datastore - got = ActivityPub.load(id) - self.assert_entities_equal(stored, got) - mock_get.assert_not_called() - - # second time is in cache - stored.key.delete() - got = ActivityPub.load(id) - self.assert_entities_equal(stored, got) - mock_get.assert_not_called() - - @patch('requests.get') - def test_load_preserves_fragment(self, mock_get): - stored = Object(id='http://the/id#frag', as2=AS2_OBJ) - stored.put() - protocol.objects_cache.clear() - - got = ActivityPub.load('http://the/id#frag') - self.assert_entities_equal(stored, got) - mock_get.assert_not_called() - - @patch('requests.get') - def test_load_datastore_no_as2(self, mock_get): - """If the stored Object has no as2, we should fall back to HTTP.""" - id = 'http://the/id' - stored = Object(id=id, as2={}, status='in progress') - stored.put() - protocol.objects_cache.clear() - - mock_get.return_value = AS2 - got = ActivityPub.load(id) - mock_get.assert_has_calls([self.as2_req(id)]) - - self.assert_equals(id, got.key.id()) - self.assert_equals(AS2_OBJ, got.as2) - mock_get.assert_has_calls([self.as2_req(id)]) - - self.assert_object(id, - as2=AS2_OBJ, - as1={**AS2_OBJ, 'id': id}, - source_protocol='activitypub', - # check that it reused our original Object - status='in progress') - @patch('requests.get') def test_signed_get_redirects_manually_with_new_sig_headers(self, mock_get): mock_get.side_effect = [ @@ -2005,15 +1932,14 @@ class ActivityPubUtilsTest(TestCase): }, ActivityPub.convert(obj)) def test_postprocess_as2_idempotent(self): - g.user = self.make_user('foo.com') + g.user = self.make_user('foo.com', cls=Web) for obj in (ACTOR, REPLY_OBJECT, REPLY_OBJECT_WRAPPED, REPLY, NOTE_OBJECT, NOTE, MENTION_OBJECT, MENTION, LIKE, LIKE_WRAPPED, REPOST, FOLLOW, FOLLOW_WRAPPED, ACCEPT, UNDO_FOLLOW_WRAPPED, DELETE, UPDATE_NOTE, - # TODO: these currently fail - # LIKE_WITH_ACTOR, REPOST_FULL, FOLLOW_WITH_ACTOR, - # FOLLOW_WRAPPED_WITH_ACTOR, FOLLOW_WITH_OBJECT, UPDATE_PERSON, + LIKE_WITH_ACTOR, REPOST_FULL, FOLLOW_WITH_ACTOR, + FOLLOW_WRAPPED_WITH_ACTOR, FOLLOW_WITH_OBJECT, UPDATE_PERSON, ): with self.subTest(obj=obj): obj = copy.deepcopy(obj) diff --git a/tests/test_atproto.py b/tests/test_atproto.py index 2fe89cb..a9be5ad 100644 --- a/tests/test_atproto.py +++ b/tests/test_atproto.py @@ -967,7 +967,7 @@ class ATProtoTest(TestCase): self.assert_task(mock_create_task, 'receive', '/queue/receive', obj=post_obj.key.urlsafe(), authed_as='did:plc:a') - # TODO + # TODO: https://github.com/snarfed/bridgy-fed/issues/728 # repost_obj = Object.get_by_id('at://did:plc:d/app.bsky.feed.post/456') # self.assertEqual(repost, repost_obj.bsky) # self.assert_task(mock_create_task, 'receive', '/queue/receive', diff --git a/tests/test_convert.py b/tests/test_convert.py index dd845af..62bb91d 100644 --- a/tests/test_convert.py +++ b/tests/test_convert.py @@ -15,6 +15,7 @@ from .testutil import Fake, OtherFake from activitypub import ActivityPub from common import CONTENT_TYPE_HTML +from web import Web COMMENT_AS2 = { **as2.from_as1(COMMENT), @@ -271,7 +272,7 @@ A ☕ reply def test_web_to_activitypub_object(self, mock_get): mock_get.return_value = requests_response(HTML) - self.make_user('user.com') + self.make_user('user.com', cls=Web) url = 'https://user.com/bar?baz=baj&biff' Object(id=url, mf2=parse_mf2(HTML)['items'][0]).put() @@ -284,7 +285,7 @@ A ☕ reply def test_web_to_activitypub_fetch(self, mock_get): mock_get.return_value = requests_response(HTML) url = 'https://user.com/bar?baz=baj&biff' - self.make_user('user.com') + self.make_user('user.com', cls=Web) Object(id=url, mf2=parse_mf2(HTML)['items'][0]).put() @@ -306,7 +307,7 @@ A ☕ reply """https://github.com/snarfed/bridgy-fed/issues/581""" mock_get.return_value = requests_response(HTML) - self.make_user('user.com') + self.make_user('user.com', cls=Web) self.store_object(id='http://user.com/a#b', mf2=parse_mf2(HTML)['items'][0]) resp = self.client.get(f'/convert/ap/http://user.com/a%23b', diff --git a/tests/test_follow.py b/tests/test_follow.py index 0beeb52..c2914d8 100644 --- a/tests/test_follow.py +++ b/tests/test_follow.py @@ -15,6 +15,7 @@ from .testutil import Fake, TestCase from activitypub import ActivityPub from models import Follower, Object +from web import Web WEBFINGER = requests_response({ 'subject': 'acct:foo@bar', @@ -61,7 +62,7 @@ class RemoteFollowTest(TestCase): def setUp(self): super().setUp() - self.make_user('user.com') + self.make_user('user.com', cls=Web) def test_no_domain(self, _): got = self.client.post('/remote-follow?address=@foo@bar&protocol=web') @@ -135,7 +136,7 @@ class FollowTest(TestCase): def setUp(self): super().setUp() - self.user = self.make_user('alice.com') + self.user = self.make_user('alice.com', cls=Web) self.state = { 'endpoint': 'http://auth/endpoint', 'me': 'https://alice.com', @@ -289,7 +290,7 @@ class FollowTest(TestCase): self.assertEqual(400, resp.status_code) def test_callback_user_use_instead(self, mock_get, mock_post): - user = self.make_user('www.alice.com') + user = self.make_user('www.alice.com', cls=Web) self.user.use_instead = user.key self.user.put() @@ -395,7 +396,7 @@ class UnfollowTest(TestCase): def setUp(self): super().setUp() - self.user = self.make_user('alice.com') + self.user = self.make_user('alice.com', cls=Web) self.follower = Follower.get_or_create( from_=self.user, to=self.make_user('https://bar/id', cls=ActivityPub, obj_as2=FOLLOWEE), @@ -484,7 +485,7 @@ class UnfollowTest(TestCase): self.assertEqual('https://alice.com', session['indieauthed-me']) def test_callback_user_use_instead(self, mock_get, mock_post): - user = self.make_user('www.alice.com') + user = self.make_user('www.alice.com', cls=Web) self.user.use_instead = user.key self.user.put() diff --git a/tests/test_ids.py b/tests/test_ids.py index 8e6653a..c6aaebf 100644 --- a/tests/test_ids.py +++ b/tests/test_ids.py @@ -4,8 +4,8 @@ from atproto import ATProto from flask_app import app from ids import translate_handle, translate_object_id, translate_user_id from models import Target -from web import Web from .testutil import Fake, TestCase +from web import Web class IdsTest(TestCase): @@ -92,7 +92,7 @@ class IdsTest(TestCase): (ActivityPub, '@user@instance', ActivityPub, '@user@instance'), (ActivityPub, '@user@instance', ATProto, 'user.instance.ap.brid.gy'), (ActivityPub, '@user@instance', Fake, 'fake:handle:@user@instance'), - (ActivityPub, '@user@instance', Web, 'instance/@user'), + (ActivityPub, '@user@instance', Web, 'https://instance/@user'), # # # TODO: enhanced # (ActivityPub, '@user@instance', Web, 'https://instance/user'), # (ActivityPub, '@user@instance', Fake, diff --git a/tests/test_pages.py b/tests/test_pages.py index 8bc6f7b..288b59f 100644 --- a/tests/test_pages.py +++ b/tests/test_pages.py @@ -43,7 +43,7 @@ class PagesTest(TestCase): def setUp(self): super().setUp() - self.user = self.make_user('user.com') + self.user = self.make_user('user.com', cls=Web) def test_user(self): got = self.client.get('/web/user.com', base_url='https://fed.brid.gy/') @@ -114,7 +114,7 @@ class PagesTest(TestCase): self.assert_equals('/web/user.com', got.headers['Location']) def test_user_use_instead(self): - self.make_user('bar.com', use_instead=self.user.key) + self.make_user('bar.com', cls=Web, use_instead=self.user.key) got = self.client.get('/web/bar.com') self.assert_equals(302, got.status_code) diff --git a/tests/test_protocol.py b/tests/test_protocol.py index 91ae46e..e9be943 100644 --- a/tests/test_protocol.py +++ b/tests/test_protocol.py @@ -12,6 +12,7 @@ from oauth_dropins.webutil import appengine_info from oauth_dropins.webutil.flask_util import CLOUD_TASKS_QUEUE_HEADER, NoContent from oauth_dropins.webutil.testutil import requests_response import requests +from werkzeug.exceptions import BadRequest # import first so that Fake is defined before URL routes are registered from .testutil import Fake, OtherFake, TestCase @@ -25,7 +26,6 @@ import protocol from protocol import Protocol from ui import UIProtocol from web import Web -from werkzeug.exceptions import BadRequest from .test_activitypub import ACTOR from .test_atproto import DID_DOC @@ -36,7 +36,7 @@ class ProtocolTest(TestCase): def setUp(self): super().setUp() - self.user = self.make_user('foo.com', has_hcard=True) + self.user = self.make_user('foo.com', cls=Web, has_hcard=True) g.user = None def tearDown(self): @@ -210,12 +210,14 @@ class ProtocolTest(TestCase): def test_load_remote_true_existing_empty(self): Fake.fetchable['foo'] = {'x': 'y'} - Object(id='foo').put() + Object(id='foo', our_as1={}, status='in progress').put() loaded = Fake.load('foo', remote=True) self.assertEqual({'id': 'foo', 'x': 'y'}, loaded.as1) self.assertTrue(loaded.changed) self.assertFalse(loaded.new) + # check that it merged in fields like status + self.assertEqual('in progress', loaded.status) self.assertEqual(['foo'], Fake.fetched) def test_load_remote_true_new_empty(self): @@ -316,6 +318,12 @@ class ProtocolTest(TestCase): 'object': 'other:bob', }, obj.our_as1) + def test_load_preserves_fragment(self): + stored = self.store_object(id='http://the/id#frag', our_as1={'foo': 'bar'}) + got = ActivityPub.load('http://the/id#frag') + self.assert_entities_equal(stored, got) + self.assertEqual([], Fake.fetched) + def test_actor_key(self): user = self.make_user(id='fake:a', cls=Fake) a_key = user.key diff --git a/tests/test_redirect.py b/tests/test_redirect.py index 561ccf5..efd8c66 100644 --- a/tests/test_redirect.py +++ b/tests/test_redirect.py @@ -33,7 +33,7 @@ class RedirectTest(testutil.TestCase): def setUp(self): super().setUp() - self.user = self.make_user('user.com') + self.user = self.make_user('user.com', cls=Web) def test_redirect(self): got = self.client.get('/r/https://user.com/bar?baz=baj&biff') diff --git a/tests/test_web.py b/tests/test_web.py index fe3d432..5f404a5 100644 --- a/tests/test_web.py +++ b/tests/test_web.py @@ -396,7 +396,7 @@ class WebTest(TestCase): obj = Object(id='https://user.com/', mf2=ACTOR_MF2, source_protocol='web') obj.put() - g.user = self.make_user('user.com', has_redirects=True, obj=obj) + g.user = self.make_user('user.com', cls=Web, has_redirects=True, obj=obj) self.mrs_foo = ndb.Key(ActivityPub, 'https://mas.to/mrs-foo') @@ -1113,7 +1113,7 @@ class WebTest(TestCase): }, } g.user.obj.put() - self.make_user('www.user.com', use_instead=g.user.key) + self.make_user('www.user.com', cls=Web, use_instead=g.user.key) self.make_followers() note_html = NOTE_HTML.replace('https://user.com/', 'https://www.user.com/') @@ -1774,7 +1774,7 @@ http://this/404s }) def test_verify_www_redirect(self, mock_get, _): - www_user = self.make_user('www.user.com') + www_user = self.make_user('www.user.com', cls=Web) empty = requests_response('') mock_get.side_effect = [ diff --git a/tests/test_webfinger.py b/tests/test_webfinger.py index 985f745..a5d409c 100644 --- a/tests/test_webfinger.py +++ b/tests/test_webfinger.py @@ -141,7 +141,7 @@ class WebfingerTest(TestCase): def setUp(self): super().setUp() - self.user = self.make_user('user.com', has_hcard=True, obj_as2={ + self.user = self.make_user('user.com', cls=Web, has_hcard=True, obj_as2={ '@context': 'https://www.w3.org/ns/activitystreams', 'type': 'Person', 'url': 'https://user.com/about-me', diff --git a/tests/testutil.py b/tests/testutil.py index f8a2748..dadad6f 100644 --- a/tests/testutil.py +++ b/tests/testutil.py @@ -276,8 +276,7 @@ class TestCase(unittest.TestCase, testutil.Asserts): kwargs.setdefault('headers', {})[flask_util.CLOUD_TASKS_QUEUE_HEADER] = '' return client.post(url, **kwargs) - # TODO: switch default to Fake, start using that more - def make_user(self, id, cls=Web, **kwargs): + def make_user(self, id, cls, **kwargs): """Reuse RSA key across Users because generating it is expensive.""" obj_key = None