From 0e2c34219112db3a6b7801530a946fd5b1bbb111 Mon Sep 17 00:00:00 2001 From: tobi <31960611+tsmethurst@users.noreply.github.com> Date: Fri, 1 Dec 2023 15:27:15 +0100 Subject: [PATCH] [bugfix/chore] `Announce` reliability updates (#2405) * [bugfix/chore] `Announce` updates * test update * fix tests * TestParseAnnounce * update comments * don't lock/unlock, change function signature * naming stuff * don't check domain block twice * UnwrapIfBoost * beep boop --- internal/federation/dereferencing/announce.go | 131 +++++++++------ .../federation/federatingdb/announce_test.go | 12 +- internal/gtsmodel/status.go | 1 + internal/processing/common/status.go | 19 +++ internal/processing/fedi/status.go | 10 ++ internal/processing/status/boost.go | 153 ++++++++++-------- internal/processing/status/fave.go | 32 +++- internal/processing/workers/fromfediapi.go | 40 +++-- .../processing/workers/fromfediapi_test.go | 4 +- internal/typeutils/astointernal.go | 81 ++++------ internal/typeutils/astointernal_test.go | 40 +++++ internal/typeutils/internal.go | 122 +++++++------- internal/typeutils/internaltoas_test.go | 3 +- internal/typeutils/internaltofrontend.go | 54 +++---- internal/web/thread.go | 8 + 15 files changed, 425 insertions(+), 285 deletions(-) diff --git a/internal/federation/dereferencing/announce.go b/internal/federation/dereferencing/announce.go index 22c33685a..8e880dad5 100644 --- a/internal/federation/dereferencing/announce.go +++ b/internal/federation/dereferencing/announce.go @@ -20,66 +20,107 @@ import ( "context" "errors" - "fmt" "net/url" "github.com/superseriousbusiness/gotosocial/internal/config" + "github.com/superseriousbusiness/gotosocial/internal/db" + "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" + "github.com/superseriousbusiness/gotosocial/internal/id" ) -func (d *Dereferencer) DereferenceAnnounce(ctx context.Context, announce *gtsmodel.Status, requestingUsername string) error { - if announce.BoostOf == nil { - // we can't do anything unfortunately - return errors.New("DereferenceAnnounce: no URI to dereference") +// EnrichAnnounce enriches the given boost wrapper status +// by either fetching from the DB or dereferencing the target +// status, populating the boost wrapper's fields based on the +// target status, and then storing the wrapper in the database. +// The wrapper is then returned to the caller. +// +// The provided boost wrapper status must have BoostOfURI set. +func (d *Dereferencer) EnrichAnnounce( + ctx context.Context, + boost *gtsmodel.Status, + requestUser string, +) (*gtsmodel.Status, error) { + targetURI := boost.BoostOfURI + if targetURI == "" { + // We can't do anything. + return nil, gtserror.Newf("no URI to dereference") } - // Parse the boosted status' URI - boostedURI, err := url.Parse(announce.BoostOf.URI) + // Parse the boost target status URI. + targetURIObj, err := url.Parse(targetURI) if err != nil { - return fmt.Errorf("DereferenceAnnounce: couldn't parse boosted status URI %s: %s", announce.BoostOf.URI, err) + return nil, gtserror.Newf( + "couldn't parse boost target status URI %s: %w", + targetURI, err, + ) } - // Check whether the originating status is from a blocked host - if blocked, err := d.state.DB.IsDomainBlocked(ctx, boostedURI.Host); blocked || err != nil { - return fmt.Errorf("DereferenceAnnounce: domain %s is blocked", boostedURI.Host) - } + // Fetch/deref status being boosted. + var target *gtsmodel.Status - var boostedStatus *gtsmodel.Status - - if boostedURI.Host == config.GetHost() { + if targetURIObj.Host == config.GetHost() { // This is a local status, fetch from the database - status, err := d.state.DB.GetStatusByURI(ctx, boostedURI.String()) - if err != nil { - return fmt.Errorf("DereferenceAnnounce: error fetching local status %q: %v", announce.BoostOf.URI, err) - } - - // Set boosted status - boostedStatus = status + target, err = d.state.DB.GetStatusByURI(ctx, targetURI) } else { - // This is a boost of a remote status, we need to dereference it. - status, _, err := d.GetStatusByURI(ctx, requestingUsername, boostedURI) - if err != nil { - return fmt.Errorf("DereferenceAnnounce: error dereferencing remote status with id %s: %s", announce.BoostOf.URI, err) - } - - // Set boosted status - boostedStatus = status + // This is a remote status, we need to dereference it. + // + // d.GetStatusByURI will handle domain block checking for us, + // so we don't try to deref an announce target on a blocked host. + target, _, err = d.GetStatusByURI(ctx, requestUser, targetURIObj) } - announce.Content = boostedStatus.Content - announce.ContentWarning = boostedStatus.ContentWarning - announce.ActivityStreamsType = boostedStatus.ActivityStreamsType - announce.Sensitive = boostedStatus.Sensitive - announce.Language = boostedStatus.Language - announce.Text = boostedStatus.Text - announce.BoostOfID = boostedStatus.ID - announce.BoostOfAccountID = boostedStatus.AccountID - announce.Visibility = boostedStatus.Visibility - announce.Federated = boostedStatus.Federated - announce.Boostable = boostedStatus.Boostable - announce.Replyable = boostedStatus.Replyable - announce.Likeable = boostedStatus.Likeable - announce.BoostOf = boostedStatus + if err != nil { + return nil, gtserror.Newf( + "error getting boost target status %s: %w", + targetURI, err, + ) + } - return nil + // Generate an ID for the boost wrapper status. + boost.ID, err = id.NewULIDFromTime(boost.CreatedAt) + if err != nil { + return nil, gtserror.Newf("error generating id: %w", err) + } + + // Populate remaining fields on + // the boost wrapper using target. + boost.Content = target.Content + boost.ContentWarning = target.ContentWarning + boost.ActivityStreamsType = target.ActivityStreamsType + boost.Sensitive = target.Sensitive + boost.Language = target.Language + boost.Text = target.Text + boost.BoostOfID = target.ID + boost.BoostOf = target + boost.BoostOfAccountID = target.AccountID + boost.BoostOfAccount = target.Account + boost.Visibility = target.Visibility + boost.Federated = target.Federated + boost.Boostable = target.Boostable + boost.Replyable = target.Replyable + boost.Likeable = target.Likeable + + // Store the boost wrapper status. + switch err = d.state.DB.PutStatus(ctx, boost); { + case err == nil: + // All good baby. + + case errors.Is(err, db.ErrAlreadyExists): + // DATA RACE! We likely lost out to another goroutine + // in a call to db.Put(Status). Look again in DB by URI. + boost, err = d.state.DB.GetStatusByURI(ctx, boost.URI) + if err != nil { + err = gtserror.Newf( + "error getting boost wrapper status %s from database after race: %w", + boost.URI, err, + ) + } + + default: + // Proper database error. + err = gtserror.Newf("db error inserting status: %w", err) + } + + return boost, err } diff --git a/internal/federation/federatingdb/announce_test.go b/internal/federation/federatingdb/announce_test.go index ae5213f66..d8de2e49c 100644 --- a/internal/federation/federatingdb/announce_test.go +++ b/internal/federation/federatingdb/announce_test.go @@ -50,8 +50,10 @@ func (suite *AnnounceTestSuite) TestNewAnnounce() { suite.True(ok) suite.Equal(announcingAccount.ID, boost.AccountID) - // only the URI will be set on the boosted status because it still needs to be dereferenced - suite.NotEmpty(boost.BoostOf.URI) + // only the URI will be set for the boosted status + // because it still needs to be dereferenced + suite.Nil(boost.BoostOf) + suite.Equal("http://example.org/users/Some_User/statuses/afaba698-5740-4e32-a702-af61aa543bc1", boost.BoostOfURI) } func (suite *AnnounceTestSuite) TestAnnounceTwice() { @@ -81,8 +83,10 @@ func (suite *AnnounceTestSuite) TestAnnounceTwice() { return nil }) - // only the URI will be set on the boosted status because it still needs to be dereferenced - suite.NotEmpty(boost.BoostOf.URI) + // only the URI will be set for the boosted status + // because it still needs to be dereferenced + suite.Nil(boost.BoostOf) + suite.Equal("http://example.org/users/Some_User/statuses/afaba698-5740-4e32-a702-af61aa543bc1", boost.BoostOfURI) ctx2 := createTestContext(receivingAccount2, announcingAccount) announce2 := suite.testActivities["announce_forwarded_1_turtle"] diff --git a/internal/gtsmodel/status.go b/internal/gtsmodel/status.go index 9b93e34a1..79c67c933 100644 --- a/internal/gtsmodel/status.go +++ b/internal/gtsmodel/status.go @@ -50,6 +50,7 @@ type Status struct { InReplyTo *Status `bun:"-"` // status corresponding to inReplyToID InReplyToAccount *Account `bun:"rel:belongs-to"` // account corresponding to inReplyToAccountID BoostOfID string `bun:"type:CHAR(26),nullzero"` // id of the status this status is a boost of + BoostOfURI string `bun:"-"` // URI of the status this status is a boost of; field not inserted in the db, just for dereferencing purposes. BoostOfAccountID string `bun:"type:CHAR(26),nullzero"` // id of the account that owns the boosted status BoostOf *Status `bun:"-"` // status that corresponds to boostOfID BoostOfAccount *Account `bun:"rel:belongs-to"` // account that corresponds to boostOfAccountID diff --git a/internal/processing/common/status.go b/internal/processing/common/status.go index 233c1c867..0a1f495fb 100644 --- a/internal/processing/common/status.go +++ b/internal/processing/common/status.go @@ -119,6 +119,25 @@ func (p *Processor) GetVisibleTargetStatus( return target, nil } +// UnwrapIfBoost "unwraps" the given status if +// it's a boost wrapper, by returning the boosted +// status it targets (pending visibility checks). +// +// Just returns the input status if it's not a boost. +func (p *Processor) UnwrapIfBoost( + ctx context.Context, + requester *gtsmodel.Account, + status *gtsmodel.Status, +) (*gtsmodel.Status, gtserror.WithCode) { + if status.BoostOfID == "" { + return status, nil + } + + return p.GetVisibleTargetStatus(ctx, + requester, status.BoostOfID, + ) +} + // GetAPIStatus fetches the appropriate API status model for target. func (p *Processor) GetAPIStatus( ctx context.Context, diff --git a/internal/processing/fedi/status.go b/internal/processing/fedi/status.go index 8082c79bc..1c1af9cb4 100644 --- a/internal/processing/fedi/status.go +++ b/internal/processing/fedi/status.go @@ -51,6 +51,11 @@ func (p *Processor) StatusGet(ctx context.Context, requestedUser string, statusI return nil, gtserror.NewErrorNotFound(errors.New(text)) } + if status.BoostOfID != "" { + const text = "status is a boost wrapper" + return nil, gtserror.NewErrorNotFound(errors.New(text)) + } + visible, err := p.filter.StatusVisible(ctx, requester, status) if err != nil { return nil, gtserror.NewErrorInternalError(err) @@ -106,6 +111,11 @@ func (p *Processor) StatusRepliesGet( return nil, gtserror.NewErrorNotFound(errors.New(text)) } + if status.BoostOfID != "" { + const text = "status is a boost wrapper" + return nil, gtserror.NewErrorNotFound(errors.New(text)) + } + // Parse replies collection ID from status' URI with onlyOtherAccounts param. onlyOtherAccStr := "only_other_accounts=" + strconv.FormatBool(onlyOtherAccounts) collectionID, err := url.Parse(status.URI + "/replies?" + onlyOtherAccStr) diff --git a/internal/processing/status/boost.go b/internal/processing/status/boost.go index 76a0a75bc..2062fb802 100644 --- a/internal/processing/status/boost.go +++ b/internal/processing/status/boost.go @@ -30,106 +30,125 @@ "github.com/superseriousbusiness/gotosocial/internal/messages" ) -// BoostCreate processes the boost/reblog of a given status, returning the newly-created boost if all is well. -func (p *Processor) BoostCreate(ctx context.Context, requestingAccount *gtsmodel.Account, application *gtsmodel.Application, targetStatusID string) (*apimodel.Status, gtserror.WithCode) { - targetStatus, err := p.state.DB.GetStatusByID(ctx, targetStatusID) +// BoostCreate processes the boost/reblog of target +// status, returning the newly-created boost. +func (p *Processor) BoostCreate( + ctx context.Context, + requester *gtsmodel.Account, + application *gtsmodel.Application, + targetID string, +) (*apimodel.Status, gtserror.WithCode) { + // Get target status and ensure it's not a boost. + target, errWithCode := p.c.GetVisibleTargetStatus( + ctx, + requester, + targetID, + ) + if errWithCode != nil { + return nil, errWithCode + } + + target, errWithCode = p.c.UnwrapIfBoost( + ctx, + requester, + target, + ) + if errWithCode != nil { + return nil, errWithCode + } + + // Ensure valid boost target. + boostable, err := p.filter.StatusBoostable(ctx, + requester, + target, + ) if err != nil { - return nil, gtserror.NewErrorNotFound(fmt.Errorf("error fetching status %s: %s", targetStatusID, err)) - } - if targetStatus.Account == nil { - return nil, gtserror.NewErrorNotFound(fmt.Errorf("no status owner for status %s", targetStatusID)) + err := gtserror.Newf("error seeing if status %s is boostable: %w", target.ID, err) + return nil, gtserror.NewErrorInternalError(err) } - // if targetStatusID refers to a boost, then we should redirect - // the target to being the status that was boosted; if we don't - // do this, then we end up in weird situations where people - // boost boosts, and it looks absolutely bizarre in the UI - if targetStatus.BoostOfID != "" { - if targetStatus.BoostOf == nil { - b, err := p.state.DB.GetStatusByID(ctx, targetStatus.BoostOfID) - if err != nil { - return nil, gtserror.NewErrorNotFound(fmt.Errorf("couldn't fetch boosted status %s", targetStatus.BoostOfID)) - } - targetStatus.BoostOf = b - } - targetStatus = targetStatus.BoostOf + if !boostable { + err := gtserror.New("status is not boostable") + return nil, gtserror.NewErrorNotFound(err) } - boostable, err := p.filter.StatusBoostable(ctx, requestingAccount, targetStatus) - if err != nil { - return nil, gtserror.NewErrorNotFound(fmt.Errorf("error seeing if status %s is boostable: %s", targetStatus.ID, err)) - } else if !boostable { - return nil, gtserror.NewErrorNotFound(errors.New("status is not boostable")) - } - - // it's visible! it's boostable! so let's boost the FUCK out of it - boostWrapperStatus, err := p.converter.StatusToBoost(ctx, targetStatus, requestingAccount) + // Status is visible and boostable. + boost, err := p.converter.StatusToBoost(ctx, + target, + requester, + application.ID, + ) if err != nil { return nil, gtserror.NewErrorInternalError(err) } - boostWrapperStatus.CreatedWithApplicationID = application.ID - boostWrapperStatus.BoostOfAccount = targetStatus.Account - - // put the boost in the database - if err := p.state.DB.PutStatus(ctx, boostWrapperStatus); err != nil { + // Store the new boost. + if err := p.state.DB.PutStatus(ctx, boost); err != nil { return nil, gtserror.NewErrorInternalError(err) } - // send it back to the processor for async processing + // Process side effects asynchronously. p.state.Workers.EnqueueClientAPI(ctx, messages.FromClientAPI{ APObjectType: ap.ActivityAnnounce, APActivityType: ap.ActivityCreate, - GTSModel: boostWrapperStatus, - OriginAccount: requestingAccount, - TargetAccount: targetStatus.Account, + GTSModel: boost, + OriginAccount: requester, + TargetAccount: target.Account, }) - return p.c.GetAPIStatus(ctx, requestingAccount, boostWrapperStatus) + return p.c.GetAPIStatus(ctx, requester, boost) } -// BoostRemove processes the unboost/unreblog of a given status, returning the status if all is well. -func (p *Processor) BoostRemove(ctx context.Context, requestingAccount *gtsmodel.Account, application *gtsmodel.Application, targetStatusID string) (*apimodel.Status, gtserror.WithCode) { - targetStatus, err := p.state.DB.GetStatusByID(ctx, targetStatusID) - if err != nil { - return nil, gtserror.NewErrorNotFound(fmt.Errorf("error fetching status %s: %s", targetStatusID, err)) - } - if targetStatus.Account == nil { - return nil, gtserror.NewErrorNotFound(fmt.Errorf("no status owner for status %s", targetStatusID)) +// BoostRemove processes the unboost/unreblog of +// target status, returning the target status. +func (p *Processor) BoostRemove( + ctx context.Context, + requester *gtsmodel.Account, + application *gtsmodel.Application, + targetID string, +) (*apimodel.Status, gtserror.WithCode) { + // Get target status and ensure it's not a boost. + target, errWithCode := p.c.GetVisibleTargetStatus( + ctx, + requester, + targetID, + ) + if errWithCode != nil { + return nil, errWithCode } - visible, err := p.filter.StatusVisible(ctx, requestingAccount, targetStatus) - if err != nil { - return nil, gtserror.NewErrorNotFound(fmt.Errorf("error seeing if status %s is visible: %s", targetStatus.ID, err)) - } - if !visible { - return nil, gtserror.NewErrorNotFound(errors.New("status is not visible")) + target, errWithCode = p.c.UnwrapIfBoost( + ctx, + requester, + target, + ) + if errWithCode != nil { + return nil, errWithCode } - // Check whether the requesting account has boosted the given status ID. - boost, err := p.state.DB.GetStatusBoost(ctx, targetStatusID, requestingAccount.ID) - if err != nil { - return nil, gtserror.NewErrorNotFound(fmt.Errorf("error checking status boost %s: %w", targetStatusID, err)) + // Check whether requester has actually + // boosted target, by trying to get the boost. + boost, err := p.state.DB.GetStatusBoost(ctx, + target.ID, + requester.ID, + ) + if err != nil && !errors.Is(err, db.ErrNoEntries) { + err = gtserror.Newf("db error getting boost of %s: %w", target.ID, err) + return nil, gtserror.NewErrorInternalError(err) } if boost != nil { - // pin some stuff onto the boost while we have it out of the db - boost.Account = requestingAccount - boost.BoostOf = targetStatus - boost.BoostOfAccount = targetStatus.Account - boost.BoostOf.Account = targetStatus.Account - - // send it back to the processor for async processing + // Status was boosted. Process unboost side effects asynchronously. p.state.Workers.EnqueueClientAPI(ctx, messages.FromClientAPI{ APObjectType: ap.ActivityAnnounce, APActivityType: ap.ActivityUndo, GTSModel: boost, - OriginAccount: requestingAccount, - TargetAccount: targetStatus.Account, + OriginAccount: requester, + TargetAccount: target.Account, }) } - return p.c.GetAPIStatus(ctx, requestingAccount, targetStatus) + return p.c.GetAPIStatus(ctx, requester, target) } // StatusBoostedBy returns a slice of accounts that have boosted the given status, filtered according to privacy settings. diff --git a/internal/processing/status/fave.go b/internal/processing/status/fave.go index a16fb6620..dbeba7fe9 100644 --- a/internal/processing/status/fave.go +++ b/internal/processing/status/fave.go @@ -33,24 +33,46 @@ "github.com/superseriousbusiness/gotosocial/internal/uris" ) -func (p *Processor) getFaveableStatus(ctx context.Context, requestingAccount *gtsmodel.Account, targetStatusID string) (*gtsmodel.Status, *gtsmodel.StatusFave, gtserror.WithCode) { - targetStatus, errWithCode := p.c.GetVisibleTargetStatus(ctx, requestingAccount, targetStatusID) +func (p *Processor) getFaveableStatus( + ctx context.Context, + requester *gtsmodel.Account, + targetID string, +) ( + *gtsmodel.Status, + *gtsmodel.StatusFave, + gtserror.WithCode, +) { + // Get target status and ensure it's not a boost. + target, errWithCode := p.c.GetVisibleTargetStatus( + ctx, + requester, + targetID, + ) if errWithCode != nil { return nil, nil, errWithCode } - if !*targetStatus.Likeable { + target, errWithCode = p.c.UnwrapIfBoost( + ctx, + requester, + target, + ) + if errWithCode != nil { + return nil, nil, errWithCode + } + + if !*target.Likeable { err := errors.New("status is not faveable") return nil, nil, gtserror.NewErrorForbidden(err, err.Error()) } - fave, err := p.state.DB.GetStatusFave(ctx, requestingAccount.ID, targetStatusID) + fave, err := p.state.DB.GetStatusFave(ctx, requester.ID, target.ID) if err != nil && !errors.Is(err, db.ErrNoEntries) { err = fmt.Errorf("getFaveTarget: error checking existing fave: %w", err) return nil, nil, gtserror.NewErrorInternalError(err) } - return targetStatus, fave, nil + return target, fave, nil } // FaveCreate adds a fave for the requestingAccount, targeting the given status (no-op if fave already exists). diff --git a/internal/processing/workers/fromfediapi.go b/internal/processing/workers/fromfediapi.go index 4c5c85666..d04e4ab8d 100644 --- a/internal/processing/workers/fromfediapi.go +++ b/internal/processing/workers/fromfediapi.go @@ -26,7 +26,6 @@ "github.com/superseriousbusiness/gotosocial/internal/gtscontext" "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" - "github.com/superseriousbusiness/gotosocial/internal/id" "github.com/superseriousbusiness/gotosocial/internal/log" "github.com/superseriousbusiness/gotosocial/internal/messages" "github.com/superseriousbusiness/gotosocial/internal/processing/account" @@ -332,45 +331,44 @@ func (p *fediAPI) CreateLike(ctx context.Context, fMsg messages.FromFediAPI) err } func (p *fediAPI) CreateAnnounce(ctx context.Context, fMsg messages.FromFediAPI) error { - status, ok := fMsg.GTSModel.(*gtsmodel.Status) + boost, ok := fMsg.GTSModel.(*gtsmodel.Status) if !ok { return gtserror.Newf("%T not parseable as *gtsmodel.Status", fMsg.GTSModel) } // Dereference status that this boosts, note - // that this will handle dereferencing the status + // that this will handle storing the boost in + // the db, and dereferencing the target status // ancestors / descendants where appropriate. - if err := p.federate.DereferenceAnnounce(ctx, - status, + var err error + boost, err = p.federate.EnrichAnnounce( + ctx, + boost, fMsg.ReceivingAccount.Username, - ); err != nil { + ) + if err != nil { + if gtserror.IsUnretrievable(err) { + // Boosted status domain blocked, nothing to do. + log.Debugf(ctx, "skipping announce: %v", err) + return nil + } + + // Actual error. return gtserror.Newf("error dereferencing announce: %w", err) } - // Generate an ID for the boost wrapper status. - statusID, err := id.NewULIDFromTime(status.CreatedAt) - if err != nil { - return gtserror.Newf("error generating id: %w", err) - } - status.ID = statusID - - // Store the boost wrapper status. - if err := p.state.DB.PutStatus(ctx, status); err != nil { - return gtserror.Newf("db error inserting status: %w", err) - } - // Timeline and notify the announce. - if err := p.surface.timelineAndNotifyStatus(ctx, status); err != nil { + if err := p.surface.timelineAndNotifyStatus(ctx, boost); err != nil { log.Errorf(ctx, "error timelining and notifying status: %v", err) } - if err := p.surface.notifyAnnounce(ctx, status); err != nil { + if err := p.surface.notifyAnnounce(ctx, boost); err != nil { log.Errorf(ctx, "error notifying announce: %v", err) } // Interaction counts changed on the original status; // uncache the prepared version from all timelines. - p.surface.invalidateStatusFromTimelines(ctx, status.ID) + p.surface.invalidateStatusFromTimelines(ctx, boost.BoostOfID) return nil } diff --git a/internal/processing/workers/fromfediapi_test.go b/internal/processing/workers/fromfediapi_test.go index 952c008cc..799eaf2dc 100644 --- a/internal/processing/workers/fromfediapi_test.go +++ b/internal/processing/workers/fromfediapi_test.go @@ -45,9 +45,7 @@ func (suite *FromFediAPITestSuite) TestProcessFederationAnnounce() { boostingAccount := suite.testAccounts["remote_account_1"] announceStatus := >smodel.Status{} announceStatus.URI = "https://example.org/some-announce-uri" - announceStatus.BoostOf = >smodel.Status{ - URI: boostedStatus.URI, - } + announceStatus.BoostOfURI = boostedStatus.URI announceStatus.CreatedAt = time.Now() announceStatus.UpdatedAt = time.Now() announceStatus.AccountID = boostingAccount.ID diff --git a/internal/typeutils/astointernal.go b/internal/typeutils/astointernal.go index ad42a687d..5bcb35d75 100644 --- a/internal/typeutils/astointernal.go +++ b/internal/typeutils/astointernal.go @@ -527,29 +527,15 @@ func (c *Converter) ASBlockToBlock(ctx context.Context, blockable ap.Blockable) }, nil } -// ASAnnounceToStatus converts an activitystreams 'announce' into a status. -// -// The returned bool indicates whether this status is new (true) or not new (false). -// -// In other words, if the status is already in the database with the ID set on the announceable, then that will be returned, -// the returned bool will be false, and no further processing is necessary. If the returned bool is true, indicating -// that this is a new announce, then further processing will be necessary, because the returned status will be bareboned and -// require further dereferencing. -// -// This is useful when multiple users on an instance might receive the same boost, and we only want to process the boost once. -// -// NOTE -- this is different from one status being boosted multiple times! In this case, new boosts should indeed be created. -// -// Implementation note: this function creates and returns a boost WRAPPER -// status which references the boosted status in its BoostOf field. No -// dereferencing is done on the boosted status by this function. Callers -// should look at `status.BoostOf` to see the status being boosted, and do -// dereferencing on it as appropriate. -// -// The returned boolean indicates whether or not the boost has already been -// seen before by this instance. If it was, then status.BoostOf should be a -// fully filled-out status. If not, then only status.BoostOf.URI will be set. -func (c *Converter) ASAnnounceToStatus(ctx context.Context, announceable ap.Announceable) (*gtsmodel.Status, bool, error) { +// ASAnnounceToStatus converts an activitystreams 'announce' into a boost +// wrapper status. The returned bool indicates whether this boost is new +// (true) or not. If new, callers should use `status.BoostOfURI` to see the +// status being boosted, and do dereferencing on it as appropriate. If not +// new, then the boost has already been fully processed and can be ignored. +func (c *Converter) ASAnnounceToStatus( + ctx context.Context, + announceable ap.Announceable, +) (*gtsmodel.Status, bool, error) { // Default assume // we already have. isNew := false @@ -565,21 +551,21 @@ func (c *Converter) ASAnnounceToStatus(ctx context.Context, announceable ap.Anno uri := uriObj.String() // Check if we already have this boost in the database. - status, err := c.state.DB.GetStatusByURI(ctx, uri) + boost, err := c.state.DB.GetStatusByURI(ctx, uri) if err != nil && !errors.Is(err, db.ErrNoEntries) { err = gtserror.Newf("db error trying to get status with uri %s: %w", uri, err) return nil, isNew, err } - if status != nil { - // We already have this status, + if boost != nil { + // We already have this boost, // no need to proceed further. - return status, isNew, nil + return boost, isNew, nil } - // Create DB status with URI - status = new(gtsmodel.Status) - status.URI = uri + // Create boost with URI + boost = new(gtsmodel.Status) + boost.URI = uri isNew = true // Get the URI of the boosted status. @@ -590,22 +576,21 @@ func (c *Converter) ASAnnounceToStatus(ctx context.Context, announceable ap.Anno } // Set the URI of the boosted status on - // the new status, for later dereferencing. - status.BoostOf = new(gtsmodel.Status) - status.BoostOf.URI = boostOf[0].String() + // the boost, for later dereferencing. + boost.BoostOfURI = boostOf[0].String() // Extract published time for the boost, // zero-time will fall back to db defaults. if pub := ap.GetPublished(announceable); !pub.IsZero() { - status.CreatedAt = pub - status.UpdatedAt = pub + boost.CreatedAt = pub + boost.UpdatedAt = pub } else { log.Warnf(ctx, "unusable published property on %s", uri) } // Extract and load the boost actor account, // (this MUST already be in database by now). - status.Account, err = c.getASActorAccount(ctx, + boost.Account, err = c.getASActorAccount(ctx, uri, announceable, ) @@ -614,13 +599,13 @@ func (c *Converter) ASAnnounceToStatus(ctx context.Context, announceable ap.Anno } // Set the related status<->account fields. - status.AccountURI = status.Account.URI - status.AccountID = status.Account.ID + boost.AccountURI = boost.Account.URI + boost.AccountID = boost.Account.ID // Calculate intended visibility of the boost. - status.Visibility, err = ap.ExtractVisibility( + boost.Visibility, err = ap.ExtractVisibility( announceable, - status.Account.FollowersURI, + boost.Account.FollowersURI, ) if err != nil { err := gtserror.Newf("error extracting status visibility for %s: %w", uri, err) @@ -629,15 +614,15 @@ func (c *Converter) ASAnnounceToStatus(ctx context.Context, announceable ap.Anno // Below IDs will all be included in the // boosted status, so set them empty here. - status.AttachmentIDs = make([]string, 0) - status.TagIDs = make([]string, 0) - status.MentionIDs = make([]string, 0) - status.EmojiIDs = make([]string, 0) + boost.AttachmentIDs = make([]string, 0) + boost.TagIDs = make([]string, 0) + boost.MentionIDs = make([]string, 0) + boost.EmojiIDs = make([]string, 0) - // Remaining fields on the boost status will be taken - // from the boosted status; it's not our job to do all - // that dereferencing here. - return status, isNew, nil + // Remaining fields on the boost will be + // taken from the target status; it's not + // our job to do all that dereferencing here. + return boost, isNew, nil } // ASFlagToReport converts a remote activitystreams 'flag' representation into a gts model report. diff --git a/internal/typeutils/astointernal_test.go b/internal/typeutils/astointernal_test.go index 40bf4c855..4be2434e5 100644 --- a/internal/typeutils/astointernal_test.go +++ b/internal/typeutils/astointernal_test.go @@ -463,6 +463,46 @@ func (suite *ASToInternalTestSuite) TestParseFlag6() { suite.Equal(report.Comment, "misinformation") } +func (suite *ASToInternalTestSuite) TestParseAnnounce() { + // Boost a status that belongs to a local account + boostingAccount := suite.testAccounts["remote_account_1"] + targetStatus := suite.testStatuses["local_account_2_status_1"] + receivingAccount := suite.testAccounts["local_account_1"] + + raw := `{ + "@context": "https://www.w3.org/ns/activitystreams", + "actor": "` + boostingAccount.URI + `", + "id": "http://fossbros-anonymous.io/db22128d-884e-4358-9935-6a7c3940535d", + "object": ["` + targetStatus.URI + `"], + "type": "Announce", + "to": "` + receivingAccount.URI + `" + }` + + t := suite.jsonToType(raw) + asAnnounce, ok := t.(ap.Announceable) + if !ok { + suite.FailNow("type not coercible") + } + + boost, isNew, err := suite.typeconverter.ASAnnounceToStatus(context.Background(), asAnnounce) + if err != nil { + suite.FailNow(err.Error()) + } + + suite.True(isNew) + suite.NotNil(boost) + suite.Equal(boostingAccount.ID, boost.AccountID) + suite.NotNil(boost.Account) + + // Of the 'BoostOf' fields, only BoostOfURI will be set. + // Others are set in dereferencing.EnrichAnnounceSafely. + suite.Equal(targetStatus.URI, boost.BoostOfURI) + suite.Empty(boost.BoostOfID) + suite.Nil(boost.BoostOf) + suite.Empty(boost.BoostOfAccountID) + suite.Nil(boost.BoostOfAccount) +} + func TestASToInternalTestSuite(t *testing.T) { suite.Run(t, new(ASToInternalTestSuite)) } diff --git a/internal/typeutils/internal.go b/internal/typeutils/internal.go index 1824a4421..095e2b121 100644 --- a/internal/typeutils/internal.go +++ b/internal/typeutils/internal.go @@ -19,90 +19,86 @@ import ( "context" - "time" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" "github.com/superseriousbusiness/gotosocial/internal/id" "github.com/superseriousbusiness/gotosocial/internal/uris" + "github.com/superseriousbusiness/gotosocial/internal/util" ) -// FollowRequestToFollow just converts a follow request into a follow, that's it! No bells and whistles. -func (c *Converter) FollowRequestToFollow(ctx context.Context, f *gtsmodel.FollowRequest) *gtsmodel.Follow { - showReblogs := *f.ShowReblogs - notify := *f.Notify +// FollowRequestToFollow just converts a follow request +// into a follow, that's it! No bells and whistles. +func (c *Converter) FollowRequestToFollow( + ctx context.Context, + fr *gtsmodel.FollowRequest, +) *gtsmodel.Follow { return >smodel.Follow{ - ID: f.ID, - CreatedAt: f.CreatedAt, - UpdatedAt: f.UpdatedAt, - AccountID: f.AccountID, - TargetAccountID: f.TargetAccountID, - ShowReblogs: &showReblogs, - URI: f.URI, - Notify: ¬ify, + ID: fr.ID, + CreatedAt: fr.CreatedAt, + UpdatedAt: fr.UpdatedAt, + AccountID: fr.AccountID, + TargetAccountID: fr.TargetAccountID, + ShowReblogs: util.Ptr(*fr.ShowReblogs), + URI: fr.URI, + Notify: util.Ptr(*fr.Notify), } } -// StatusToBoost wraps the given status into a boosting status. -func (c *Converter) StatusToBoost(ctx context.Context, s *gtsmodel.Status, boostingAccount *gtsmodel.Account) (*gtsmodel.Status, error) { - // the wrapper won't use the same ID as the boosted status so we generate some new UUIDs - accountURIs := uris.GenerateURIsForAccount(boostingAccount.Username) - boostWrapperStatusID := id.NewULID() - boostWrapperStatusURI := accountURIs.StatusesURI + "/" + boostWrapperStatusID - boostWrapperStatusURL := accountURIs.StatusesURL + "/" + boostWrapperStatusID +// StatusToBoost wraps the target status into a +// boost wrapper status owned by the requester. +func (c *Converter) StatusToBoost( + ctx context.Context, + target *gtsmodel.Status, + booster *gtsmodel.Account, + applicationID string, +) (*gtsmodel.Status, error) { + // The boost won't use the same IDs as the + // target so we need to generate new ones. + boostID := id.NewULID() + accountURIs := uris.GenerateURIsForAccount(booster.Username) - local := true - if boostingAccount.Domain != "" { - local = false - } + boost := >smodel.Status{ + ID: boostID, + URI: accountURIs.StatusesURI + "/" + boostID, + URL: accountURIs.StatusesURL + "/" + boostID, - sensitive := *s.Sensitive - federated := *s.Federated - boostable := *s.Boostable - replyable := *s.Replyable - likeable := *s.Likeable + // Inherit some fields from the booster account. + Local: util.Ptr(booster.IsLocal()), + AccountID: booster.ID, + Account: booster, + AccountURI: booster.URI, + CreatedWithApplicationID: applicationID, - boostWrapperStatus := >smodel.Status{ - ID: boostWrapperStatusID, - URI: boostWrapperStatusURI, - URL: boostWrapperStatusURL, - - // the boosted status is not created now, but the boost certainly is - CreatedAt: time.Now(), - UpdatedAt: time.Now(), - Local: &local, - AccountID: boostingAccount.ID, - AccountURI: boostingAccount.URI, - - // replies can be boosted, but boosts are never replies + // Replies can be boosted, but + // boosts are never replies. InReplyToID: "", InReplyToAccountID: "", - // these will all be wrapped in the boosted status so set them empty here + // These will all be wrapped in the + // boosted status so set them empty. AttachmentIDs: []string{}, TagIDs: []string{}, MentionIDs: []string{}, EmojiIDs: []string{}, - // the below fields will be taken from the target status - Content: s.Content, - ContentWarning: s.ContentWarning, - ActivityStreamsType: s.ActivityStreamsType, - Sensitive: &sensitive, - Language: s.Language, - Text: s.Text, - BoostOfID: s.ID, - BoostOfAccountID: s.AccountID, - Visibility: s.Visibility, - Federated: &federated, - Boostable: &boostable, - Replyable: &replyable, - Likeable: &likeable, - - // attach these here for convenience -- the boosted status/account won't go in the DB - // but they're needed in the processor and for the frontend. Since we have them, we can - // attach them so we don't need to fetch them again later (save some DB calls) - BoostOf: s, + // Remaining fields all + // taken from boosted status. + Content: target.Content, + ContentWarning: target.ContentWarning, + ActivityStreamsType: target.ActivityStreamsType, + Sensitive: util.Ptr(*target.Sensitive), + Language: target.Language, + Text: target.Text, + BoostOfID: target.ID, + BoostOf: target, + BoostOfAccountID: target.AccountID, + BoostOfAccount: target.Account, + Visibility: target.Visibility, + Federated: util.Ptr(*target.Federated), + Boostable: util.Ptr(*target.Boostable), + Replyable: util.Ptr(*target.Replyable), + Likeable: util.Ptr(*target.Likeable), } - return boostWrapperStatus, nil + return boost, nil } diff --git a/internal/typeutils/internaltoas_test.go b/internal/typeutils/internaltoas_test.go index 0e0607279..cbeaf3c8c 100644 --- a/internal/typeutils/internaltoas_test.go +++ b/internal/typeutils/internaltoas_test.go @@ -724,10 +724,11 @@ func (suite *InternalToASTestSuite) TestSelfBoostFollowersOnlyToAS() { testStatus := suite.testStatuses["local_account_1_status_5"] testAccount := suite.testAccounts["local_account_1"] - boostWrapperStatus, err := suite.typeconverter.StatusToBoost(ctx, testStatus, testAccount) + boostWrapperStatus, err := suite.typeconverter.StatusToBoost(ctx, testStatus, testAccount, "") suite.NoError(err) suite.NotNil(boostWrapperStatus) + // Set some fields to predictable values for the test. boostWrapperStatus.ID = "01G74JJ1KS331G2JXHRMZCE0ER" boostWrapperStatus.URI = "http://localhost:8080/users/the_mighty_zork/statuses/01G74JJ1KS331G2JXHRMZCE0ER" boostWrapperStatus.CreatedAt = testrig.TimeMustParse("2022-06-09T13:12:00Z") diff --git a/internal/typeutils/internaltofrontend.go b/internal/typeutils/internaltofrontend.go index cd53d9ae6..fe5c8ec8f 100644 --- a/internal/typeutils/internaltofrontend.go +++ b/internal/typeutils/internaltofrontend.go @@ -733,11 +733,18 @@ func (c *Converter) statusToFrontend( s *gtsmodel.Status, requestingAccount *gtsmodel.Account, ) (*apimodel.Status, error) { + // Try to populate status struct pointer fields. + // We can continue in many cases of partial failure, + // but there are some fields we actually need. if err := c.state.DB.PopulateStatus(ctx, s); err != nil { - // Ensure author account present + correct; - // can't really go further without this! if s.Account == nil { - return nil, gtserror.Newf("error(s) populating status, cannot continue: %w", err) + err = gtserror.Newf("error(s) populating status, cannot continue (status.Account not set): %w", err) + return nil, err + } + + if s.BoostOfID != "" && s.BoostOf == nil { + err = gtserror.Newf("error(s) populating status, cannot continue (status.BoostOfID set, but status.Boost not set): %w", err) + return nil, err } log.Errorf(ctx, "error(s) populating status, will continue: %v", err) @@ -794,12 +801,12 @@ func (c *Converter) statusToFrontend( apiStatus := &apimodel.Status{ ID: s.ID, CreatedAt: util.FormatISO8601(s.CreatedAt), - InReplyToID: nil, - InReplyToAccountID: nil, + InReplyToID: nil, // Set below. + InReplyToAccountID: nil, // Set below. Sensitive: *s.Sensitive, SpoilerText: s.ContentWarning, Visibility: c.VisToAPIVis(ctx, s.Visibility), - Language: nil, + Language: nil, // Set below. URI: s.URI, URL: s.URL, RepliesCount: repliesCount, @@ -811,56 +818,47 @@ func (c *Converter) statusToFrontend( Reblogged: interacts.Reblogged, Pinned: interacts.Pinned, Content: s.Content, - Reblog: nil, - Application: nil, + Reblog: nil, // Set below. + Application: nil, // Set below. Account: apiAuthorAccount, MediaAttachments: apiAttachments, Mentions: apiMentions, Tags: apiTags, Emojis: apiEmojis, Card: nil, // TODO: implement cards - Poll: nil, // TODO: implement polls Text: s.Text, } // Nullable fields. - if s.InReplyToID != "" { - apiStatus.InReplyToID = func() *string { i := s.InReplyToID; return &i }() + apiStatus.InReplyToID = util.Ptr(s.InReplyToID) } if s.InReplyToAccountID != "" { - apiStatus.InReplyToAccountID = func() *string { i := s.InReplyToAccountID; return &i }() + apiStatus.InReplyToAccountID = util.Ptr(s.InReplyToAccountID) } if s.Language != "" { - apiStatus.Language = func() *string { i := s.Language; return &i }() + apiStatus.Language = util.Ptr(s.Language) } if s.BoostOf != nil { - apiBoostOf, err := c.StatusToAPIStatus(ctx, s.BoostOf, requestingAccount) + reblog, err := c.StatusToAPIStatus(ctx, s.BoostOf, requestingAccount) if err != nil { return nil, gtserror.Newf("error converting boosted status: %w", err) } - apiStatus.Reblog = &apimodel.StatusReblogged{Status: apiBoostOf} + apiStatus.Reblog = &apimodel.StatusReblogged{reblog} } - if appID := s.CreatedWithApplicationID; appID != "" { - app := s.CreatedWithApplication - if app == nil { - app, err = c.state.DB.GetApplicationByID(ctx, appID) - if err != nil { - return nil, gtserror.Newf("error getting application %s: %w", appID, err) - } - } - - apiApp, err := c.AppToAPIAppPublic(ctx, app) + if app := s.CreatedWithApplication; app != nil { + apiStatus.Application, err = c.AppToAPIAppPublic(ctx, app) if err != nil { - return nil, gtserror.Newf("error converting application %s: %w", appID, err) + return nil, gtserror.Newf( + "error converting application %s: %w", + s.CreatedWithApplicationID, err, + ) } - - apiStatus.Application = apiApp } if s.Poll != nil { diff --git a/internal/web/thread.go b/internal/web/thread.go index 523cf7579..20145cfcd 100644 --- a/internal/web/thread.go +++ b/internal/web/thread.go @@ -20,6 +20,7 @@ import ( "context" "encoding/json" + "errors" "fmt" "net/http" "strings" @@ -124,6 +125,13 @@ func (m *Module) threadGETHandler(c *gin.Context) { return } + // Don't render boosts/reblogs as top-level statuses. + if status.Reblog != nil { + err := errors.New("status is a boost wrapper / reblog") + apiutil.WebErrorHandler(c, gtserror.NewErrorNotFound(err), instanceGet) + return + } + // Fill in the rest of the thread context. context, errWithCode := m.processor.Status().WebContextGet(ctx, targetStatusID) if errWithCode != nil {