[chore] chore rationalise http return codes for activitypub handlers (#…

…2540)

* some small code fixups and changes

* add check in ResolveIncomingActivity for transient activity types (i.e. activity ID is nil)

* update test to handle new transient behaviour

internal/ap/resolve.go

@@ -56,8 +56,9 @@ func putMap(m map[string]any) {
 	mapPool.Put(m)
 }
 
-// ResolveActivity is a util function for pulling a pub.Activity type out of an incoming request body.
-func ResolveIncomingActivity(r *http.Request) (pub.Activity, gtserror.WithCode) {
+// ResolveActivity is a util function for pulling a pub.Activity type out of an incoming request body,
+// returning the resolved activity type, error and whether to accept activity (false = transient i.e. ignore).
+func ResolveIncomingActivity(r *http.Request) (pub.Activity, bool, gtserror.WithCode) {
 	// Get "raw" map
 	// destination.
 	raw := getMap()
@@ -68,33 +69,37 @@ func ResolveIncomingActivity(r *http.Request) (pub.Activity, gtserror.WithCode)
 	// Decode the JSON body stream into "raw" map.
 	if err := json.NewDecoder(r.Body).Decode(&raw); err != nil {
 		err := gtserror.Newf("error decoding json: %w", err)
-		return nil, gtserror.NewErrorInternalError(err)
+		return nil, false, gtserror.NewErrorInternalError(err)
 	}
 
 	// Resolve "raw" JSON to vocab.Type.
 	t, err := streams.ToType(r.Context(), raw)
 	if err != nil {
 		if !streams.IsUnmatchedErr(err) {
 			err := gtserror.Newf("error matching json to type: %w", err)
-			return nil, gtserror.NewErrorInternalError(err)
+			return nil, false, gtserror.NewErrorInternalError(err)
 		}
 
 		// Respond with bad request; we just couldn't
 		// match the type to one that we know about.
 		const text = "body json not resolvable as ActivityStreams type"
-		return nil, gtserror.NewErrorBadRequest(errors.New(text), text)
+		return nil, false, gtserror.NewErrorBadRequest(errors.New(text), text)
 	}
 
 	// Ensure this is an Activity type.
 	activity, ok := t.(pub.Activity)
 	if !ok {
 		text := fmt.Sprintf("cannot resolve vocab type %T as pub.Activity", t)
-		return nil, gtserror.NewErrorBadRequest(errors.New(text), text)
+		return nil, false, gtserror.NewErrorBadRequest(errors.New(text), text)
 	}
 
 	if activity.GetJSONLDId() == nil {
-		const text = "missing ActivityStreams id property"
-		return nil, gtserror.NewErrorBadRequest(errors.New(text), text)
+		// missing ID indicates a transient ID as per:
+		//
+		// all objects distributed by the ActivityPub protocol MUST have unique global identifiers,
+		// unless they are intentionally transient (short lived activities that are not intended to
+		// be able to be looked up, such as some kinds of chat messages or game notifications).
+		return nil, false, nil
 	}
 
 	// Normalize any Statusable, Accountable, Pollable fields found.
@@ -104,7 +109,7 @@ func ResolveIncomingActivity(r *http.Request) (pub.Activity, gtserror.WithCode)
 	// Release.
 	putMap(raw)
 
-	return activity, nil
+	return activity, true, nil
 }
 
 // ResolveStatusable tries to resolve the given bytes into an ActivityPub Statusable representation.

internal/api/activitypub/users/inboxpost_test.go

@@ -478,15 +478,17 @@ func (suite *InboxPostTestSuite) TestPostEmptyCreate() {
 		targetAccount     = suite.testAccounts["local_account_1"]
 	)
 
-	// Post a create with no object.
+	// Post a create with no object, this
+	// should get accepted and silently dropped
+	// as the lack of ID marks it as transient.
 	create := streams.NewActivityStreamsCreate()
 
 	suite.inboxPost(
 		create,
 		requestingAccount,
 		targetAccount,
-		http.StatusBadRequest,
-		`{"error":"Bad Request: missing ActivityStreams id property"}`,
+		http.StatusAccepted,
+		`{"status":"Accepted"}`,
 		suite.signatureCheck,
 	)
 }

internal/federation/federatingactor.go

@@ -143,10 +143,12 @@ func (f *federatingActor) PostInboxScheme(ctx context.Context, w http.ResponseWr
 		have not yet applied authorization (ie., blocks).
 	*/
 
-	// Obtain the activity; reject unknown activities.
-	activity, errWithCode := ap.ResolveIncomingActivity(r)
+	// Resolve the activity, rejecting badly formatted / transient.
+	activity, ok, errWithCode := ap.ResolveIncomingActivity(r)
 	if errWithCode != nil {
 		return false, errWithCode
+	} else if !ok { // transient
+		return false, nil
 	}
 
 	// Set additional context data. Primarily this means

internal/federation/federatingdb/reject.go

@@ -45,16 +45,11 @@ func (f *federatingDB) Reject(ctx context.Context, reject vocab.ActivityStreamsR
 		return nil // Already processed.
 	}
 
-	rejectObject := reject.GetActivityStreamsObject()
-	if rejectObject == nil {
-		return errors.New("Reject: no object set on vocab.ActivityStreamsReject")
-	}
+	for _, obj := range ap.ExtractObjects(reject) {
 
-	for iter := rejectObject.Begin(); iter != rejectObject.End(); iter = iter.Next() {
-		// check if the object is an IRI
-		if iter.IsIRI() {
+		if obj.IsIRI() {
 			// we have just the URI of whatever is being rejected, so we need to find out what it is
-			rejectedObjectIRI := iter.GetIRI()
+			rejectedObjectIRI := obj.GetIRI()
 			if uris.IsFollowPath(rejectedObjectIRI) {
 				// REJECT FOLLOW
 				followReq, err := f.state.DB.GetFollowRequestByURI(ctx, rejectedObjectIRI.String())
@@ -78,15 +73,10 @@ func (f *federatingDB) Reject(ctx context.Context, reject vocab.ActivityStreamsR
 			}
 		}
 
-		// check if iter is an AP object / type
-		if iter.GetType() == nil {
-			continue
-		}
-
-		if iter.GetType().GetTypeName() == ap.ActivityFollow {
+		if t := obj.GetType(); t != nil {
 			// we have the whole object so we can figure out what we're rejecting
 			// REJECT FOLLOW
-			asFollow, ok := iter.GetType().(vocab.ActivityStreamsFollow)
+			asFollow, ok := t.(vocab.ActivityStreamsFollow)
 			if !ok {
 				return errors.New("Reject: couldn't parse follow into vocab.ActivityStreamsFollow")
 			}

internal/federation/federatingdb/undo.go

@@ -78,7 +78,7 @@ func (f *federatingDB) Undo(ctx context.Context, undo vocab.ActivityStreamsUndo)
 		}
 	}
 
-	return nil
+	return errs.Combine()
 }
 
 func (f *federatingDB) undoFollow(