[chore] Refactor federatingDB.Undo, avoid 500 errors on Undo Like (#3310)

internal/federation/federatingactor.go

@@ -170,14 +170,22 @@ func (f *federatingActor) PostInboxScheme(ctx context.Context, w http.ResponseWr
 	//
 	// Post the activity to the Actor's inbox and trigger side effects.
 	if err := f.sideEffectActor.PostInbox(ctx, inboxID, activity); err != nil {
-		// Special case: We know it is a bad request if the object or target
-		// props needed to be populated, or we failed parsing activity details.
-		// Send the rejection to the peer.
-		if errors.Is(err, pub.ErrObjectRequired) ||
-			errors.Is(err, pub.ErrTargetRequired) ||
-			gtserror.IsMalformed(err) {
+		// Check if a function in the federatingDB
+		// has returned an explicit errWithCode for us.
+		if errWithCode, ok := err.(gtserror.WithCode); ok {
+			return false, errWithCode
+		}
+
+		// Check if it's a bad request because the
+		// object or target props weren't populated,
+		// or we failed parsing activity details.
+		//
+		// Log such activities to help debug, then
+		// return the rejection (400) to the peer.
+		if gtserror.IsMalformed(err) ||
+			errors.Is(err, pub.ErrObjectRequired) ||
+			errors.Is(err, pub.ErrTargetRequired) {
 
-			// Log malformed activities to help debug.
 			l = l.WithField("activity", activity)
 			l.Warnf("malformed incoming activity: %v", err)
 
@@ -185,7 +193,7 @@ func (f *federatingActor) PostInboxScheme(ctx context.Context, w http.ResponseWr
 			return false, gtserror.NewErrorBadRequest(errors.New(text), text)
 		}
 
-		// There's been some real error.
+		// Default: there's been some real error.
 		err := gtserror.Newf("error calling sideEffectActor.PostInbox: %w", err)
 		return false, gtserror.NewErrorInternalError(err)
 	}

internal/federation/federatingdb/undo.go

@@ -33,14 +33,13 @@ import (
 )
 
 func (f *federatingDB) Undo(ctx context.Context, undo vocab.ActivityStreamsUndo) error {
-	l := log.WithContext(ctx)
-
 	if log.Level() >= level.DEBUG {
 		i, err := marshalItem(undo)
 		if err != nil {
 			return err
 		}
-		l = l.WithField("undo", i)
+		l := log.WithContext(ctx).
+			WithField("undo", i)
 		l.Debug("entering Undo")
 	}
 
@@ -52,80 +51,129 @@ func (f *federatingDB) Undo(ctx context.Context, undo vocab.ActivityStreamsUndo)
 	requestingAcct := activityContext.requestingAcct
 	receivingAcct := activityContext.receivingAcct
 
-	var errs gtserror.MultiError
 	for _, object := range ap.ExtractObjects(undo) {
 		// Try to get object as vocab.Type,
 		// else skip handling (likely) IRI.
-		objType := object.GetType()
-		if objType == nil {
+		asType := object.GetType()
+		if asType == nil {
 			continue
 		}
 
-		switch objType.GetTypeName() {
+		// Check and handle any
+		// vocab.Type objects.
+		switch asType.GetTypeName() {
+
+		// UNDO FOLLOW
 		case ap.ActivityFollow:
-			if err := f.undoFollow(ctx, receivingAcct, requestingAcct, undo, objType); err != nil {
-				errs.Appendf("error undoing follow: %w", err)
+			if err := f.undoFollow(
+				ctx,
+				receivingAcct,
+				requestingAcct,
+				undo,
+				asType,
+			); err != nil {
+				return err
 			}
+
+		// UNDO LIKE
 		case ap.ActivityLike:
-			if err := f.undoLike(ctx, receivingAcct, requestingAcct, undo, objType); err != nil {
-				errs.Appendf("error undoing like: %w", err)
+			if err := f.undoLike(
+				ctx,
+				receivingAcct,
+				requestingAcct,
+				undo,
+				asType,
+			); err != nil {
+				return err
 			}
+
+		// UNDO BLOCK
+		case ap.ActivityBlock:
+			if err := f.undoBlock(
+				ctx,
+				receivingAcct,
+				requestingAcct,
+				undo,
+				asType,
+			); err != nil {
+				return err
+			}
+
+		// UNDO ANNOUNCE
 		case ap.ActivityAnnounce:
 			// TODO: actually handle this!
 			log.Warn(ctx, "skipped undo announce")
-		case ap.ActivityBlock:
-			if err := f.undoBlock(ctx, receivingAcct, requestingAcct, undo, objType); err != nil {
-				errs.Appendf("error undoing block: %w", err)
-			}
 		}
 	}
 
-	return errs.Combine()
+	return nil
 }
 
 func (f *federatingDB) undoFollow(
 	ctx context.Context,
-	receivingAccount *gtsmodel.Account,
-	requestingAccount *gtsmodel.Account,
+	receivingAcct *gtsmodel.Account,
+	requestingAcct *gtsmodel.Account,
 	undo vocab.ActivityStreamsUndo,
 	t vocab.Type,
 ) error {
-	Follow, ok := t.(vocab.ActivityStreamsFollow)
+	asFollow, ok := t.(vocab.ActivityStreamsFollow)
 	if !ok {
-		return errors.New("undoFollow: couldn't parse vocab.Type into vocab.ActivityStreamsFollow")
+		err := fmt.Errorf("%T not parseable as vocab.ActivityStreamsFollow", t)
+		return gtserror.SetMalformed(err)
 	}
 
-	// Make sure the undo actor owns the target.
-	if !sameActor(undo.GetActivityStreamsActor(), Follow.GetActivityStreamsActor()) {
+	// Make sure the Undo
+	// actor owns the target.
+	if !sameActor(
+		undo.GetActivityStreamsActor(),
+		asFollow.GetActivityStreamsActor(),
+	) {
 		// Ignore this Activity.
 		return nil
 	}
 
-	follow, err := f.converter.ASFollowToFollow(ctx, Follow)
-	if err != nil {
-		return fmt.Errorf("undoFollow: error converting ActivityStreams Follow to follow: %w", err)
+	// Convert AS Follow to barebones *gtsmodel.Follow,
+	// retrieving origin + target accts from the db.
+	follow, err := f.converter.ASFollowToFollow(
+		gtscontext.SetBarebones(ctx),
+		asFollow,
+	)
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
+		err := gtserror.Newf("error converting AS Follow to follow: %w", err)
+		return err
+	}
+
+	// We were missing origin or
+	// target for this Follow, so
+	// we cannot Undo anything.
+	if follow == nil {
+		return nil
 	}
 
 	// Ensure addressee is follow target.
-	if follow.TargetAccountID != receivingAccount.ID {
-		// Ignore this Activity.
-		return nil
+	if follow.TargetAccountID != receivingAcct.ID {
+		const text = "receivingAcct was not Follow target"
+		return gtserror.NewErrorForbidden(errors.New(text), text)
 	}
 
 	// Ensure requester is follow origin.
-	if follow.AccountID != requestingAccount.ID {
-		// Ignore this Activity.
-		return nil
+	if follow.AccountID != requestingAcct.ID {
+		const text = "requestingAcct was not Follow origin"
+		return gtserror.NewErrorForbidden(errors.New(text), text)
 	}
 
 	// Delete any existing follow with this URI.
-	if err := f.state.DB.DeleteFollowByURI(ctx, follow.URI); err != nil && !errors.Is(err, db.ErrNoEntries) {
-		return fmt.Errorf("undoFollow: db error removing follow: %w", err)
+	err = f.state.DB.DeleteFollowByURI(ctx, follow.URI)
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
+		err := gtserror.Newf("db error deleting follow: %w", err)
+		return err
 	}
 
 	// Delete any existing follow request with this URI.
-	if err := f.state.DB.DeleteFollowRequestByURI(ctx, follow.URI); err != nil && !errors.Is(err, db.ErrNoEntries) {
-		return fmt.Errorf("undoFollow: db error removing follow request: %w", err)
+	err = f.state.DB.DeleteFollowRequestByURI(ctx, follow.URI)
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
+		err := gtserror.Newf("db error deleting follow request: %w", err)
+		return err
 	}
 
 	log.Debug(ctx, "Follow undone")
@@ -134,58 +182,89 @@ func (f *federatingDB) undoFollow(
 
 func (f *federatingDB) undoLike(
 	ctx context.Context,
-	receivingAccount *gtsmodel.Account,
-	requestingAccount *gtsmodel.Account,
+	receivingAcct *gtsmodel.Account,
+	requestingAcct *gtsmodel.Account,
 	undo vocab.ActivityStreamsUndo,
 	t vocab.Type,
 ) error {
-	Like, ok := t.(vocab.ActivityStreamsLike)
+	asLike, ok := t.(vocab.ActivityStreamsLike)
 	if !ok {
-		return errors.New("undoLike: couldn't parse vocab.Type into vocab.ActivityStreamsLike")
+		err := fmt.Errorf("%T not parseable as vocab.ActivityStreamsLike", t)
+		return gtserror.SetMalformed(err)
 	}
 
-	// Make sure the undo actor owns the target.
-	if !sameActor(undo.GetActivityStreamsActor(), Like.GetActivityStreamsActor()) {
+	// Make sure the Undo
+	// actor owns the target.
+	if !sameActor(
+		undo.GetActivityStreamsActor(),
+		asLike.GetActivityStreamsActor(),
+	) {
 		// Ignore this Activity.
 		return nil
 	}
 
-	fave, err := f.converter.ASLikeToFave(ctx, Like)
-	if err != nil {
-		return fmt.Errorf("undoLike: error converting ActivityStreams Like to fave: %w", err)
+	// Convert AS Like to barebones *gtsmodel.StatusFave,
+	// retrieving liking acct and target status from the DB.
+	fave, err := f.converter.ASLikeToFave(
+		gtscontext.SetBarebones(ctx),
+		asLike,
+	)
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
+		err := gtserror.Newf("error converting AS Like to fave: %w", err)
+		return err
+	}
+
+	// We were missing status, account,
+	// or other for this Like, so we
+	// cannot Undo anything.
+	if fave == nil {
+		return nil
 	}
 
 	// Ensure addressee is fave target.
-	if fave.TargetAccountID != receivingAccount.ID {
-		// Ignore this Activity.
-		return nil
+	if fave.TargetAccountID != receivingAcct.ID {
+		const text = "receivingAcct was not Fave target"
+		return gtserror.NewErrorForbidden(errors.New(text), text)
 	}
 
 	// Ensure requester is fave origin.
-	if fave.AccountID != requestingAccount.ID {
-		// Ignore this Activity.
-		return nil
+	if fave.AccountID != requestingAcct.ID {
+		const text = "requestingAcct was not Fave origin"
+		return gtserror.NewErrorForbidden(errors.New(text), text)
 	}
 
+	// Fetch fave from the DB so we know the ID to delete it.
+	//
 	// Ignore URI on Likes, since we often get multiple Likes
 	// with the same target and account ID, but differing URIs.
 	// Instead, we'll select using account and target status.
+	//
 	// Regardless of the URI, we can read an Undo Like to mean
 	// "I don't want to fave this post anymore".
-	fave, err = f.state.DB.GetStatusFave(gtscontext.SetBarebones(ctx), fave.AccountID, fave.StatusID)
-	if err != nil {
-		if errors.Is(err, db.ErrNoEntries) {
-			// We didn't have a like/fave
-			// for this combo anyway, ignore.
-			return nil
-		}
-		// Real error.
-		return fmt.Errorf("undoLike: db error getting fave from %s targeting %s: %w", fave.AccountID, fave.StatusID, err)
+	fave, err = f.state.DB.GetStatusFave(
+		gtscontext.SetBarebones(ctx),
+		fave.AccountID,
+		fave.StatusID,
+	)
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
+		err := gtserror.Newf(
+			"db error getting fave from %s targeting %s: %w",
+			fave.AccountID, fave.StatusID, err,
+		)
+		return err
 	}
 
-	// Delete the status fave.
+	if fave == nil {
+		// We didn't have this fave
+		// stored anyway, so we can't
+		// Undo it, just ignore.
+		return nil
+	}
+
+	// Delete the fave.
 	if err := f.state.DB.DeleteStatusFaveByID(ctx, fave.ID); err != nil {
-		return fmt.Errorf("undoLike: db error deleting fave %s: %w", fave.ID, err)
+		err := gtserror.Newf("db error deleting fave %s: %w", fave.ID, err)
+		return err
 	}
 
 	log.Debug(ctx, "Like undone")
@@ -194,42 +273,62 @@ func (f *federatingDB) undoLike(
 
 func (f *federatingDB) undoBlock(
 	ctx context.Context,
-	receivingAccount *gtsmodel.Account,
-	requestingAccount *gtsmodel.Account,
+	receivingAcct *gtsmodel.Account,
+	requestingAcct *gtsmodel.Account,
 	undo vocab.ActivityStreamsUndo,
 	t vocab.Type,
 ) error {
-	Block, ok := t.(vocab.ActivityStreamsBlock)
+	asBlock, ok := t.(vocab.ActivityStreamsBlock)
 	if !ok {
-		return errors.New("undoBlock: couldn't parse vocab.Type into vocab.ActivityStreamsBlock")
+		err := fmt.Errorf("%T not parseable as vocab.ActivityStreamsBlock", t)
+		return gtserror.SetMalformed(err)
 	}
 
-	// Make sure the undo actor owns the target.
-	if !sameActor(undo.GetActivityStreamsActor(), Block.GetActivityStreamsActor()) {
+	// Make sure the Undo
+	// actor owns the target.
+	if !sameActor(
+		undo.GetActivityStreamsActor(),
+		asBlock.GetActivityStreamsActor(),
+	) {
 		// Ignore this Activity.
 		return nil
 	}
 
-	block, err := f.converter.ASBlockToBlock(ctx, Block)
-	if err != nil {
-		return fmt.Errorf("undoBlock: error converting ActivityStreams Block to block: %w", err)
+	// Convert AS Block to barebones *gtsmodel.Block,
+	// retrieving origin + target accts from the DB.
+	block, err := f.converter.ASBlockToBlock(
+		gtscontext.SetBarebones(ctx),
+		asBlock,
+	)
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
+		err := gtserror.Newf("error converting AS Block to block: %w", err)
+		return err
+	}
+
+	// We were missing origin or
+	// target for this Block, so
+	// we cannot Undo anything.
+	if block == nil {
+		return nil
 	}
 
 	// Ensure addressee is block target.
-	if block.TargetAccountID != receivingAccount.ID {
-		// Ignore this Activity.
-		return nil
+	if block.TargetAccountID != receivingAcct.ID {
+		const text = "receivingAcct was not Block target"
+		return gtserror.NewErrorForbidden(errors.New(text), text)
 	}
 
 	// Ensure requester is block origin.
-	if block.AccountID != requestingAccount.ID {
-		// Ignore this Activity.
-		return nil
+	if block.AccountID != requestingAcct.ID {
+		const text = "requestingAcct was not Block origin"
+		return gtserror.NewErrorForbidden(errors.New(text), text)
 	}
 
-	// Delete any existing BLOCK
-	if err := f.state.DB.DeleteBlockByURI(ctx, block.URI); err != nil && !errors.Is(err, db.ErrNoEntries) {
-		return fmt.Errorf("undoBlock: db error removing block: %w", err)
+	// Delete any existing block.
+	err = f.state.DB.DeleteBlockByURI(ctx, block.URI)
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
+		err := gtserror.Newf("db error deleting block: %w", err)
+		return err
 	}
 
 	log.Debug(ctx, "Block undone")

internal/typeutils/astointernal.go

@@ -861,7 +861,7 @@ func (c *Converter) getASObjectStatus(ctx context.Context, id string, with ap.Wi
 	// Check for status in database with provided object URI.
 	status, err := c.state.DB.GetStatusByURI(ctx, object[0].String())
 	if err != nil {
-		return nil, gtserror.Newf("error getting object account from database: %w", err)
+		return nil, gtserror.Newf("error getting object status from database: %w", err)
 	}
 
 	return status, nil