From 064921fdd1cf542bfb4464818e127909570ed163 Mon Sep 17 00:00:00 2001 From: Thomas Sileo Date: Tue, 26 Jul 2022 21:10:59 +0200 Subject: [PATCH] Handle out-of-order Delete activity --- app/activitypub.py | 2 +- app/boxes.py | 77 +++++++++++++++++++++++------ tests/factories.py | 31 ++++++++++++ tests/test_inbox.py | 115 +++++++++++++++++++++++++++++++++++++++++++- tests/utils.py | 16 ++++++ 5 files changed, 222 insertions(+), 19 deletions(-) diff --git a/app/activitypub.py b/app/activitypub.py index 60a6f9c..b48bc9e 100644 --- a/app/activitypub.py +++ b/app/activitypub.py @@ -261,7 +261,7 @@ async def get_object(activity: RawObject) -> RawObject: def wrap_object(activity: RawObject) -> RawObject: - # TODO(ts): improve Create VS Update + # TODO(tsileo): improve Create VS Update with a `update=True` flag if "updated" in activity: return { "@context": AS_EXTENDED_CTX, diff --git a/app/boxes.py b/app/boxes.py index bd39908..d3339d2 100644 --- a/app/boxes.py +++ b/app/boxes.py @@ -654,6 +654,25 @@ async def get_inbox_object_by_ap_id( ).scalar_one_or_none() # type: ignore +async def get_inbox_delete_for_activity_object_ap_id( + db_session: AsyncSession, activity_object_ap_id: str +) -> models.InboxObject | None: + return ( + await db_session.execute( + select(models.InboxObject) + .where( + models.InboxObject.ap_type == "Delete", + models.InboxObject.activity_object_ap_id == activity_object_ap_id, + ) + .options( + joinedload(models.InboxObject.actor), + joinedload(models.InboxObject.relates_to_inbox_object), + joinedload(models.InboxObject.relates_to_outbox_object), + ) + ) + ).scalar_one_or_none() # type: ignore + + async def get_outbox_object_by_ap_id( db_session: AsyncSession, ap_id: str ) -> models.OutboxObject | None: @@ -695,8 +714,16 @@ async def _handle_delete_activity( db_session: AsyncSession, from_actor: models.Actor, delete_activity: models.InboxObject, - ap_object_to_delete: models.InboxObject, + ap_object_to_delete: models.InboxObject | None, ) -> None: + if ap_object_to_delete is None: + logger.info( + "Received Delete for an unknown object " + f"{delete_activity.activity_object_ap_id}" + ) + # TODO(tsileo): support deleting actor + return + if from_actor.ap_id != ap_object_to_delete.actor.ap_id: logger.warning( "Actor mismatch between the activity and the object: " @@ -724,11 +751,18 @@ async def _handle_delete_activity( logger.info(f"Deleting {ap_object_to_delete.ap_type}/{ap_object_to_delete.ap_id}") ap_object_to_delete.is_deleted = True + await _revert_side_effect_for_deleted_object(db_session, ap_object_to_delete) + + +async def _revert_side_effect_for_deleted_object( + db_session: AsyncSession, + deleted_ap_object: models.InboxObject, +) -> None: # Decrement the replies counter if needed - if ap_object_to_delete.in_reply_to: + if deleted_ap_object.in_reply_to: replied_object = await get_anybox_object_by_ap_id( db_session, - ap_object_to_delete.in_reply_to, + deleted_ap_object.in_reply_to, ) if replied_object: if replied_object.is_from_outbox: @@ -928,6 +962,24 @@ async def _handle_create_activity( raise ValueError("Object actor does not match activity") ro = RemoteObject(wrapped_object, actor=from_actor) + + # Check if we already received a delete for this object (happens often + # with forwarded replies) + delete_object = await get_inbox_delete_for_activity_object_ap_id( + db_session, + ro.ap_id, + ) + if delete_object: + if delete_object.actor.ap_id != from_actor.ap_id: + logger.warning( + f"Got a Delete for {ro.ap_id} from {delete_object.actor.ap_id}??" + ) + else: + logger.info("Got a Delete for this object, deleting activity") + create_activity.is_deleted = True + await db_session.flush() + return None + await _process_note_object(db_session, create_activity, from_actor, ro) @@ -1251,19 +1303,12 @@ async def save_to_inbox( elif activity_ro.ap_type == "Update": await _handle_update_activity(db_session, actor, inbox_object) elif activity_ro.ap_type == "Delete": - if relates_to_inbox_object: - await _handle_delete_activity( - db_session, - actor, - inbox_object, - relates_to_inbox_object, - ) - else: - # TODO(ts): handle delete actor - logger.info( - "Received a Delete for an unknown object: " - f"{activity_ro.activity_object_ap_id}" - ) + await _handle_delete_activity( + db_session, + actor, + inbox_object, + relates_to_inbox_object, + ) elif activity_ro.ap_type == "Follow": await _handle_follow_follow_activity(db_session, actor, inbox_object) elif activity_ro.ap_type == "Undo": diff --git a/tests/factories.py b/tests/factories.py index d489da4..5f57a80 100644 --- a/tests/factories.py +++ b/tests/factories.py @@ -36,6 +36,24 @@ def build_follow_activity( } +def build_delete_activity( + from_remote_actor: actor.RemoteActor | models.Actor, + deleted_object_ap_id: str, + outbox_public_id: str | None = None, +) -> ap.RawObject: + return { + "@context": ap.AS_CTX, + "type": "Delete", + "id": ( + from_remote_actor.ap_id # type: ignore + + "/follow/" + + (outbox_public_id or uuid4().hex) + ), + "actor": from_remote_actor.ap_id, + "object": deleted_object_ap_id, + } + + def build_accept_activity( from_remote_actor: actor.RemoteActor, for_remote_object: RemoteObject, @@ -80,6 +98,19 @@ def build_note_object( } +def build_create_activity(obj: ap.RawObject) -> ap.RawObject: + return { + "@context": ap.AS_EXTENDED_CTX, + "actor": obj["attributedTo"], + "to": obj.get("to", []), + "cc": obj.get("cc", []), + "id": obj["id"] + "/activity", + "object": ap.remove_context(obj), + "published": obj["published"], + "type": "Create", + } + + class BaseModelMeta: sqlalchemy_session = _Session sqlalchemy_session_persistence = "commit" diff --git a/tests/test_inbox.py b/tests/test_inbox.py index 0f53d72..a4fd783 100644 --- a/tests/test_inbox.py +++ b/tests/test_inbox.py @@ -3,6 +3,7 @@ from uuid import uuid4 import httpx import respx from fastapi.testclient import TestClient +from sqlalchemy import select from sqlalchemy.orm import Session from app import activitypub as ap @@ -13,7 +14,9 @@ from app.incoming_activities import process_next_incoming_activity from tests import factories from tests.utils import mock_httpsig_checker from tests.utils import run_async +from tests.utils import setup_inbox_delete from tests.utils import setup_remote_actor +from tests.utils import setup_remote_actor_as_follower def test_inbox_requires_httpsig( @@ -41,7 +44,7 @@ def test_inbox_follow_request( ) respx_mock.get(ra.ap_id).mock(return_value=httpx.Response(200, json=ra.ap_actor)) - # When sending a Follow activity + # When receiving a Follow activity follow_activity = RemoteObject( factories.build_follow_activity( from_remote_actor=ra, @@ -108,7 +111,7 @@ def test_inbox_accept_follow_request( follow_id, follow_from_outbox ) - # When sending a Accept activity + # When receiving a Accept activity accept_activity = RemoteObject( factories.build_accept_activity( from_remote_actor=ra, @@ -137,3 +140,111 @@ def test_inbox_accept_follow_request( # And a following entry was created internally following = db.query(models.Following).one() assert following.ap_actor_id == actor_in_db.ap_id + + +def test_inbox__create_from_follower( + db: Session, + client: TestClient, + respx_mock: respx.MockRouter, +) -> None: + # Given a remote actor + ra = setup_remote_actor(respx_mock) + + # Who is also a follower + setup_remote_actor_as_follower(ra) + + create_activity = factories.build_create_activity( + factories.build_note_object( + from_remote_actor=ra, + outbox_public_id=str(uuid4()), + content="Hello", + to=[LOCAL_ACTOR.ap_id], + ) + ) + + # When receiving a Create activity + ro = RemoteObject(create_activity, ra) + + with mock_httpsig_checker(ra): + response = client.post( + "/inbox", + headers={"Content-Type": ap.AS_CTX}, + json=ro.ap_object, + ) + + # Then the server returns a 204 + assert response.status_code == 202 + + # And when processing the incoming activity + run_async(process_next_incoming_activity) + + # Then the Create activity was saved + create_activity_from_inbox: models.InboxObject | None = db.execute( + select(models.InboxObject).where(models.InboxObject.ap_type == "Create") + ).scalar_one_or_none() + assert create_activity_from_inbox + assert create_activity_from_inbox.ap_id == ro.ap_id + + # And the Note object was created + note_activity_from_inbox: models.InboxObject | None = db.execute( + select(models.InboxObject).where(models.InboxObject.ap_type == "Note") + ).scalar_one_or_none() + assert note_activity_from_inbox + assert note_activity_from_inbox.ap_id == ro.activity_object_ap_id + + +def test_inbox__create_already_deleted_object( + db: Session, + client: TestClient, + respx_mock: respx.MockRouter, +) -> None: + # Given a remote actor + ra = setup_remote_actor(respx_mock) + + # Who is also a follower + follower = setup_remote_actor_as_follower(ra) + + # And a Create activity for a Note object + create_activity = factories.build_create_activity( + factories.build_note_object( + from_remote_actor=ra, + outbox_public_id=str(uuid4()), + content="Hello", + to=[LOCAL_ACTOR.ap_id], + ) + ) + ro = RemoteObject(create_activity, ra) + + # And a Delete activity received for the create object + setup_inbox_delete(follower.actor, ro.activity_object_ap_id) # type: ignore + + # When receiving a Create activity + with mock_httpsig_checker(ra): + response = client.post( + "/inbox", + headers={"Content-Type": ap.AS_CTX}, + json=ro.ap_object, + ) + + # Then the server returns a 204 + assert response.status_code == 202 + + # And when processing the incoming activity + run_async(process_next_incoming_activity) + + # Then the Create activity was saved + create_activity_from_inbox: models.InboxObject | None = db.execute( + select(models.InboxObject).where(models.InboxObject.ap_type == "Create") + ).scalar_one_or_none() + assert create_activity_from_inbox + assert create_activity_from_inbox.ap_id == ro.ap_id + # But it has the deleted flag + assert create_activity_from_inbox.is_deleted is True + + # And the Note wasn't created + assert ( + db.execute( + select(models.InboxObject).where(models.InboxObject.ap_type == "Note") + ).scalar_one_or_none() + is None + ) diff --git a/tests/utils.py b/tests/utils.py index a2b51b2..2dd53e0 100644 --- a/tests/utils.py +++ b/tests/utils.py @@ -73,6 +73,22 @@ def setup_remote_actor_as_follower(ra: actor.RemoteActor) -> models.Follower: return follower +def setup_inbox_delete( + actor: models.Actor, deleted_object_ap_id: str +) -> models.InboxObject: + follow_from_inbox = RemoteObject( + factories.build_delete_activity( + from_remote_actor=actor, + deleted_object_ap_id=deleted_object_ap_id, + ), + actor, + ) + inbox_object = factories.InboxObjectFactory.from_remote_object( + follow_from_inbox, actor + ) + return inbox_object + + def run_async(func, *args, **kwargs): async def _func(): async with async_session() as db: