kopia lustrzana https://github.com/snarfed/bridgy-fed
Protocol.receive: return HTTP 299 for deterministic errors to prevent retries
for https://github.com/snarfed/bridgy-fed/issues/1063pull/1175/head
rodzic
89e2372922
commit
06f3694c9d
30
protocol.py
30
protocol.py
|
@ -19,6 +19,7 @@ from oauth_dropins.webutil import models
|
|||
from oauth_dropins.webutil import util
|
||||
from oauth_dropins.webutil.util import json_dumps, json_loads
|
||||
import werkzeug.exceptions
|
||||
from werkzeug.exceptions import BadGateway, HTTPException
|
||||
|
||||
import common
|
||||
from common import (
|
||||
|
@ -26,7 +27,6 @@ from common import (
|
|||
DOMAIN_BLOCKLIST,
|
||||
DOMAIN_RE,
|
||||
DOMAINS,
|
||||
error,
|
||||
PRIMARY_DOMAIN,
|
||||
PROTOCOL_DOMAINS,
|
||||
report_error,
|
||||
|
@ -62,6 +62,20 @@ seen_ids_lock = Lock()
|
|||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
def error(*args, status=299, **kwargs):
|
||||
"""Default HTTP status code to 299 to prevent retrying task."""
|
||||
return common.error(*args, status=status, **kwargs)
|
||||
|
||||
|
||||
class ErrorButDoNotRetryTask(HTTPException):
|
||||
code = 299
|
||||
description = 'ErrorButDoNotRetryTask'
|
||||
|
||||
# https://github.com/pallets/flask/issues/1837#issuecomment-304996942
|
||||
werkzeug.exceptions.default_exceptions.setdefault(299, ErrorButDoNotRetryTask)
|
||||
werkzeug.exceptions._aborter.mapping.setdefault(299, ErrorButDoNotRetryTask)
|
||||
|
||||
|
||||
class Protocol:
|
||||
"""Base protocol class. Not to be instantiated; classmethods only.
|
||||
|
||||
|
@ -311,11 +325,11 @@ class Protocol:
|
|||
if protocol.load(id, local=False, remote=True):
|
||||
logger.info(f' {protocol.LABEL} owns id {id}')
|
||||
return protocol
|
||||
except werkzeug.exceptions.BadGateway:
|
||||
except BadGateway:
|
||||
# we tried and failed fetching the id over the network.
|
||||
# this depends on ActivityPub.fetch raising this!
|
||||
return None
|
||||
except werkzeug.exceptions.HTTPException as e:
|
||||
except HTTPException as e:
|
||||
# internal error we generated ourselves; try next protocol
|
||||
pass
|
||||
except Exception as e:
|
||||
|
@ -721,7 +735,7 @@ class Protocol:
|
|||
"""Handles an incoming activity.
|
||||
|
||||
If ``obj``'s key is unset, ``obj.as1``'s id field is used. If both are
|
||||
unset, raises :class:`werkzeug.exceptions.BadRequest`.
|
||||
unset, returns HTTP 299.
|
||||
|
||||
Args:
|
||||
obj (models.Object)
|
||||
|
@ -739,8 +753,6 @@ class Protocol:
|
|||
assert isinstance(obj, Object), obj
|
||||
logger.info(f'From {from_cls.LABEL}: {obj.type} {obj.key} AS1: {json_dumps(obj.as1, indent=2)}')
|
||||
|
||||
# TODO: return 204 for all of these errors so we don't retry them
|
||||
# https://cloud.google.com/tasks/docs/creating-appengine-handlers
|
||||
if not obj.as1:
|
||||
error('No object data provided')
|
||||
|
||||
|
@ -789,7 +801,7 @@ class Protocol:
|
|||
# https://www.w3.org/wiki/ActivityPub/Primer/Authentication_Authorization
|
||||
actor = as1.get_owner(obj.as1)
|
||||
if not actor:
|
||||
error('Activity missing actor or author', status=400)
|
||||
error('Activity missing actor or author')
|
||||
elif from_cls.owns_id(actor) is False:
|
||||
error(f"{from_cls.LABEL} doesn't own actor {actor}, this is probably a bridged activity. Skipping.", status=204)
|
||||
|
||||
|
@ -800,7 +812,7 @@ class Protocol:
|
|||
if actor != authed_as:
|
||||
report_error("Auth: receive: authed_as doesn't match owner",
|
||||
user=f'{id} authed_as {authed_as} owner {actor}')
|
||||
error(f"actor {actor} isn't authed user {authed_as}", status=403)
|
||||
error(f"actor {actor} isn't authed user {authed_as}")
|
||||
|
||||
# update copy ids to originals
|
||||
obj.normalize_ids()
|
||||
|
@ -984,7 +996,7 @@ class Protocol:
|
|||
|
||||
from_obj = from_cls.load(from_id)
|
||||
if not from_obj:
|
||||
error(f"Couldn't load {from_id}")
|
||||
error(f"Couldn't load {from_id}", status=502)
|
||||
|
||||
if not from_obj.as1:
|
||||
from_obj.our_as1 = from_as1
|
||||
|
|
|
@ -1065,7 +1065,7 @@ class ActivityPubTest(TestCase):
|
|||
'actor': ACTOR['id'],
|
||||
'object': None,
|
||||
})
|
||||
self.assertEqual(400, got.status_code)
|
||||
self.assertEqual(299, got.status_code)
|
||||
|
||||
def test_inbox_follow_accept_with_id(self, *mocks):
|
||||
self._test_inbox_follow_accept(FOLLOW_WRAPPED, ACCEPT, *mocks)
|
||||
|
@ -1548,7 +1548,7 @@ class ActivityPubTest(TestCase):
|
|||
})
|
||||
headers = sign('/ap/sharedInbox', body, key_id=ACTOR['id'])
|
||||
got = self.client.post('/ap/sharedInbox', data=body, headers=headers)
|
||||
self.assertEqual(403, got.status_code, got.get_data(as_text=True))
|
||||
self.assertEqual(299, got.status_code, got.get_data(as_text=True))
|
||||
|
||||
def test_inbox_NO_AUTH_DOMAINS(self, *_):
|
||||
id = 'https://a.gup.pe/a-group'
|
||||
|
|
|
@ -17,7 +17,7 @@ from oauth_dropins.webutil.appengine_config import ndb_client
|
|||
from oauth_dropins.webutil.flask_util import NoContent
|
||||
from oauth_dropins.webutil.testutil import NOW, requests_response
|
||||
import requests
|
||||
from werkzeug.exceptions import BadRequest, Forbidden
|
||||
from werkzeug.exceptions import BadRequest
|
||||
|
||||
# import first so that Fake is defined before URL routes are registered
|
||||
from .testutil import ExplicitEnableFake, Fake, OtherFake, TestCase
|
||||
|
@ -28,7 +28,7 @@ from atproto import ATProto
|
|||
import common
|
||||
from models import Follower, Object, PROTOCOLS, Target, User
|
||||
import protocol
|
||||
from protocol import Protocol
|
||||
from protocol import ErrorButDoNotRetryTask, Protocol
|
||||
from ui import UIProtocol
|
||||
from web import Web
|
||||
|
||||
|
@ -1188,7 +1188,7 @@ class ProtocolReceiveTest(TestCase):
|
|||
}
|
||||
self.store_object(id='fake:post', our_as1=post_as1, source_protocol='fake')
|
||||
|
||||
with self.assertRaises(Forbidden):
|
||||
with self.assertRaises(ErrorButDoNotRetryTask):
|
||||
Fake.receive_as1({
|
||||
'id': 'fake:update',
|
||||
'objectType': 'activity',
|
||||
|
@ -1580,7 +1580,7 @@ class ProtocolReceiveTest(TestCase):
|
|||
self.assertEqual([(like_obj.key.id(), 'fake:post:target')], Fake.sent)
|
||||
|
||||
def test_like_no_object_error(self):
|
||||
with self.assertRaises(BadRequest):
|
||||
with self.assertRaises(ErrorButDoNotRetryTask):
|
||||
Fake.receive_as1({
|
||||
'id': 'fake:like',
|
||||
'objectType': 'activity',
|
||||
|
@ -1590,7 +1590,7 @@ class ProtocolReceiveTest(TestCase):
|
|||
})
|
||||
|
||||
def test_share_no_object_error(self):
|
||||
with self.assertRaises(BadRequest):
|
||||
with self.assertRaises(ErrorButDoNotRetryTask):
|
||||
Fake.receive_as1({
|
||||
'id': 'fake:share',
|
||||
'objectType': 'activity',
|
||||
|
@ -1959,7 +1959,7 @@ class ProtocolReceiveTest(TestCase):
|
|||
self.assertEqual([], OtherFake.sent)
|
||||
|
||||
def test_follow_no_actor(self):
|
||||
with self.assertRaises(BadRequest):
|
||||
with self.assertRaises(ErrorButDoNotRetryTask):
|
||||
Fake.receive_as1({
|
||||
'id': 'fake:follow',
|
||||
'objectType': 'activity',
|
||||
|
@ -1971,7 +1971,7 @@ class ProtocolReceiveTest(TestCase):
|
|||
self.assertEqual([], Fake.sent)
|
||||
|
||||
def test_follow_no_object(self):
|
||||
with self.assertRaises(BadRequest):
|
||||
with self.assertRaises(ErrorButDoNotRetryTask):
|
||||
Fake.receive_as1({
|
||||
'id': 'fake:follow',
|
||||
'objectType': 'activity',
|
||||
|
@ -1983,7 +1983,7 @@ class ProtocolReceiveTest(TestCase):
|
|||
self.assertEqual([], Fake.sent)
|
||||
|
||||
def test_follow_object_unknown_protocol(self):
|
||||
with self.assertRaises(BadRequest):
|
||||
with self.assertRaises(ErrorButDoNotRetryTask):
|
||||
Fake.receive_as1({
|
||||
'id': 'fake:follow',
|
||||
'objectType': 'activity',
|
||||
|
@ -2233,7 +2233,7 @@ class ProtocolReceiveTest(TestCase):
|
|||
})
|
||||
|
||||
def test_activity_id_blocklisted(self):
|
||||
with self.assertRaises(BadRequest):
|
||||
with self.assertRaises(ErrorButDoNotRetryTask):
|
||||
Fake.receive_as1({
|
||||
'objectType': 'activity',
|
||||
'verb': 'delete',
|
||||
|
@ -2551,7 +2551,7 @@ class ProtocolReceiveTest(TestCase):
|
|||
|
||||
self.addCleanup(reset_seen_ids)
|
||||
|
||||
# pretend like the two threads below are in different processes
|
||||
# pretend the two threads below are in different processes
|
||||
protocol.seen_ids = TTLCache(maxsize=10, ttl=0)
|
||||
|
||||
Follower.get_or_create(to=self.user, from_=self.alice)
|
||||
|
@ -2812,7 +2812,7 @@ class ProtocolReceiveTest(TestCase):
|
|||
'obj': obj.key.urlsafe(),
|
||||
'authed_as': 'foo.com',
|
||||
})
|
||||
self.assertEqual(403, got.status_code)
|
||||
self.assertEqual(299, got.status_code)
|
||||
self.assertIsNone(Object.get_by_id('http://bar.com/post#bridgy-fed-create'))
|
||||
|
||||
def test_receive_task_handler_not_authed_as(self):
|
||||
|
@ -2826,7 +2826,7 @@ class ProtocolReceiveTest(TestCase):
|
|||
'obj': obj.key.urlsafe(),
|
||||
'authed_as': 'fake:eve',
|
||||
})
|
||||
self.assertEqual(403, got.status_code)
|
||||
self.assertEqual(299, got.status_code)
|
||||
self.assertIsNone(Object.get_by_id('fake:post#bridgy-fed-create'))
|
||||
|
||||
def test_like_not_authed_as_actor(self):
|
||||
|
@ -2835,7 +2835,7 @@ class ProtocolReceiveTest(TestCase):
|
|||
'author': 'fake:bob',
|
||||
}
|
||||
|
||||
with self.assertRaises(Forbidden):
|
||||
with self.assertRaises(ErrorButDoNotRetryTask):
|
||||
Fake.receive_as1({
|
||||
'id': 'fake:like',
|
||||
'objectType': 'activity',
|
||||
|
|
Ładowanie…
Reference in New Issue