From 0567b319c6de0cc964bcf0b2891bdd58e887bf68 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Mon, 16 Sep 2024 17:49:40 +0200 Subject: [PATCH] [chore] Refactor federatingDB.Undo, avoid 500 errors on Undo Like (#3310) --- internal/federation/federatingactor.go | 26 ++- internal/federation/federatingdb/undo.go | 259 ++++++++++++++++------- internal/typeutils/astointernal.go | 2 +- 3 files changed, 197 insertions(+), 90 deletions(-) diff --git a/internal/federation/federatingactor.go b/internal/federation/federatingactor.go index b9b2c8001..d19c97c3e 100644 --- a/internal/federation/federatingactor.go +++ b/internal/federation/federatingactor.go @@ -168,16 +168,24 @@ func (f *federatingActor) PostInboxScheme(ctx context.Context, w http.ResponseWr // the POST request is authentic (properly signed) and authorized // (permitted to interact with the target inbox). // - // Post the activity to the Actor's inbox and trigger side effects . + // 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) } diff --git a/internal/federation/federatingdb/undo.go b/internal/federation/federatingdb/undo.go index b3d158ba6..71e248aac 100644 --- a/internal/federation/federatingdb/undo.go +++ b/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 } - case ap.ActivityAnnounce: - // TODO: actually handle this ! - log.Warn(ctx, "skipped undo announce") + + // UNDO BLOCK case ap.ActivityBlock: - if err := f.undoBlock(ctx, receivingAcct, requestingAcct, undo, objType); err != nil { - errs.Appendf("error undoing block: %w", err) + 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") } } - 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") diff --git a/internal/typeutils/astointernal.go b/internal/typeutils/astointernal.go index 5ff60b09c..9dd18dbb2 100644 --- a/internal/typeutils/astointernal.go +++ b/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