Skip to content

Commit

Permalink
[bugfix] Don't return Account or Status if new and dereferencing fail…
Browse files Browse the repository at this point in the history 
…ed, other small fixes (#2563)

* tidy up account, status, webfingering logic a wee bit

* go fmt

* invert published check

* alter resp initialization

* get Published from account in typeutils

* don't instantiate error for no darn good reason

* shadow err

* don't repeat error codes in wrapped errors

* don't wrap error unnecessarily
  • Loading branch information
@tsmethurst
tsmethurst committed Feb 14, 2024
1 parent ad6f756 commit a3aa604
Show file tree
Hide file tree
Showing 12 changed files with 445 additions and 194 deletions.
1 change: 1 addition & 0 deletions internal/ap/interfaces.go
View file
Expand Up @@ -163,6 +163,7 @@ type Accountable interface {
WithManuallyApprovesFollowers
WithEndpoints
WithTag
WithPublished
}

// Statusable represents the minimum activitypub interface for representing a 'status'.
Expand Down
188 changes: 137 additions & 51 deletions internal/federation/dereferencing/account.go
View file
Expand Up @@ -52,7 +52,7 @@ func accountUpToDate(account *gtsmodel.Account, force bool) bool {
return true
}

if !account.CreatedAt.IsZero() && account.IsInstance() {
if account.IsInstance() && !account.IsNew() {
// Existing instance account. No need for update.
return true
}
Expand Down Expand Up @@ -232,8 +232,8 @@ func (d *Dereferencer) getAccountByUsernameDomain(
// Create and pass-through a new bare-bones model for dereferencing.
account, accountable, err := d.enrichAccountSafely(ctx, requestUser, nil, &gtsmodel.Account{
ID: id.NewULID(),
Username: username,
Domain: domain,
Username: username,
}, nil)
if err != nil {
return nil, nil, err
Expand Down Expand Up @@ -372,17 +372,18 @@ func (d *Dereferencer) enrichAccountSafely(

// By default use account.URI
// as the per-URI deref lock.
uriStr := account.URI

if uriStr == "" {
var lockKey string
if account.URI != "" {
lockKey = account.URI
} else {
// No URI is set yet, instead generate a faux-one from user+domain.
uriStr = "https://" + account.Domain + "/user/" + account.Username
lockKey = "https://" + account.Domain + "/users/" + account.Username
}

// Acquire per-URI deref lock, wraping unlock
// to safely defer in case of panic, while still
// performing more granular unlocks when needed.
unlock := d.state.FedLocks.Lock(uriStr)
unlock := d.state.FedLocks.Lock(lockKey)
unlock = doOnce(unlock)
defer unlock()

Expand All @@ -394,10 +395,30 @@ func (d *Dereferencer) enrichAccountSafely(
accountable,
)

if gtserror.StatusCode(err) >= 400 {
// Update fetch-at to slow re-attempts.
if code := gtserror.StatusCode(err); code >= 400 {
// No matter what, log the error
// so instance admins have an idea
// why something isn't working.
log.Info(ctx, err)

if account.IsNew() {
// This was a new account enrich
// attempt which failed before we
// got to store it, so we can't
// return anything useful.
return nil, nil, err
}

// We had this account stored already
// before this enrichment attempt.
//
// Update fetched_at to slow re-attempts
// but don't return early. We can still
// return the model we had stored already.
account.FetchedAt = time.Now()
_ = d.state.DB.UpdateAccount(ctx, account, "fetched_at")
if err := d.state.DB.UpdateAccount(ctx, account, "fetched_at"); err != nil {
log.Errorf(ctx, "error updating account fetched_at: %v", err)
}
}

// Unlock now
Expand All @@ -414,7 +435,7 @@ func (d *Dereferencer) enrichAccountSafely(
// in a call to db.Put(Account). Look again in DB by URI.
latest, err = d.state.DB.GetAccountByURI(ctx, account.URI)
if err != nil {
err = gtserror.Newf("error getting account %s from database after race: %w", uriStr, err)
err = gtserror.Newf("error getting account %s from database after race: %w", lockKey, err)
}
}

Expand All @@ -440,32 +461,44 @@ func (d *Dereferencer) enrichAccount(
if account.Username != "" {
// A username was provided so we can attempt a webfinger, this ensures up-to-date accountdomain info.
accDomain, accURI, err := d.fingerRemoteAccount(ctx, tsport, account.Username, account.Domain)
if err != nil {
if account.URI == "" {
// this is a new account (to us) with username@domain
// but failed webfinger, nothing more we can do.
err := gtserror.Newf("error webfingering account: %w", err)
return nil, nil, gtserror.SetUnretrievable(err)
}
switch {

// Simply log this error and move on, we already have an account URI.
log.Errorf(ctx, "error webfingering[1] remote account %s@%s: %v", account.Username, account.Domain, err)
}
case err != nil && account.URI == "":
// This is a new account (to us) with username@domain
// but failed webfinger, nothing more we can do.
err := gtserror.Newf("error webfingering account: %w", err)
return nil, nil, gtserror.SetUnretrievable(err)

if err == nil {
if account.Domain != accDomain {
// After webfinger, we now have correct account domain from which we can do a final DB check.
alreadyAccount, err := d.state.DB.GetAccountByUsernameDomain(ctx, account.Username, accDomain)
if err != nil && !errors.Is(err, db.ErrNoEntries) {
return nil, nil, gtserror.Newf("db err looking for account again after webfinger: %w", err)
}
case err != nil:
// Simply log this error and move on,
// we already have an account URI.
log.Errorf(ctx,
"error webfingering[1] remote account %s@%s: %v",
account.Username, account.Domain, err,
)

case err == nil && account.Domain != accDomain:
// After webfinger, we now have correct account domain from which we can do a final DB check.
alreadyAcct, err := d.state.DB.GetAccountByUsernameDomain(ctx, account.Username, accDomain)
if err != nil && !errors.Is(err, db.ErrNoEntries) {
return nil, nil, gtserror.Newf("db err looking for account again after webfinger: %w", err)
}

if alreadyAccount != nil {
// Enrich existing account.
account = alreadyAccount
}
if alreadyAcct != nil {
// We had this account stored under
// the discovered accountDomain.
// Proceed with this account.
account = alreadyAcct
}

// Whether we had the account or not, we
// now have webfinger info relevant to the
// account, so fallthrough to set webfinger
// info on either the account we just found,
// or the stub account we were passed.
fallthrough

case err == nil:
// Update account with latest info.
account.URI = accURI.String()
account.Domain = accDomain
Expand All @@ -474,13 +507,31 @@ func (d *Dereferencer) enrichAccount(
}

if uri == nil {
// No URI provided / found, must parse from account.
// No URI provided / found,
// must parse from account.
uri, err = url.Parse(account.URI)
if err != nil {
return nil, nil, gtserror.Newf("invalid uri %q: %w", account.URI, err)
return nil, nil, gtserror.Newf(
"invalid uri %q: %w",
account.URI, gtserror.SetUnretrievable(err),
)
}

if uri.Scheme != "http" && uri.Scheme != "https" {
err = errors.New("account URI scheme must be http or https")
return nil, nil, gtserror.Newf(
"invalid uri %q: %w",
account.URI, gtserror.SetUnretrievable(err),
)
}
}

/*
BY THIS POINT we must have an account URI set,
either provided, pinned to the account, or
obtained via webfinger call.
*/

// Check whether this account URI is a blocked domain / subdomain.
if blocked, err := d.state.DB.IsDomainBlocked(ctx, uri.Host); err != nil {
return nil, nil, gtserror.Newf("error checking blocked domain: %w", err)
Expand All @@ -493,27 +544,45 @@ func (d *Dereferencer) enrichAccount(
defer d.stopHandshake(requestUser, uri)

if apubAcc == nil {
// We were not given any (partial) ActivityPub
// version of this account as a parameter.
// Dereference latest version of the account.
b, err := tsport.Dereference(ctx, uri)
if err != nil {
err := gtserror.Newf("error deferencing %s: %w", uri, err)
err := gtserror.Newf("error dereferencing %s: %w", uri, err)
return nil, nil, gtserror.SetUnretrievable(err)
}

// Attempt to resolve ActivityPub acc from data.
apubAcc, err = ap.ResolveAccountable(ctx, b)
if err != nil {
return nil, nil, gtserror.Newf("error resolving accountable from data for account %s: %w", uri, err)
// ResolveAccountable will set Unretrievable/WrongType
// on the returned error, so we don't need to do it here.
err = gtserror.Newf("error resolving accountable from data for account %s: %w", uri, err)
return nil, nil, err
}
}

/*
BY THIS POINT we must have the ActivityPub
representation of the account, either provided,
or obtained via a dereference call.
*/

// Convert the dereferenced AP account object to our GTS model.
//
// We put this in the variable latestAcc because we might want
// to compare the provided account model with this fresh version,
// in order to check if anything changed since we last saw it.
latestAcc, err := d.converter.ASRepresentationToAccount(ctx,
apubAcc,
account.Domain,
)
if err != nil {
return nil, nil, gtserror.Newf("error converting accountable to gts model for account %s: %w", uri, err)
// ASRepresentationToAccount will set Malformed on the
// returned error, so we don't need to do it here.
err = gtserror.Newf("error converting accountable to gts model for account %s: %w", uri, err)
return nil, nil, err
}

if account.Username == "" {
Expand All @@ -528,14 +597,15 @@ func (d *Dereferencer) enrichAccount(
// from example.org to somewhere.else: we want to take somewhere.else
// as the accountDomain then, not the example.org we were redirected from.

// Assume the host from the returned ActivityPub representation.
idProp := apubAcc.GetJSONLDId()
if idProp == nil || !idProp.IsIRI() {
// Assume the host from the returned
// ActivityPub representation.
id := ap.GetJSONLDId(apubAcc)
if id == nil {
return nil, nil, gtserror.New("no id property found on person, or id was not an iri")
}

// Get IRI host value.
accHost := idProp.GetIRI().Host
accHost := id.Host

latestAcc.Domain, _, err = d.fingerRemoteAccount(ctx,
tsport,
Expand All @@ -553,7 +623,7 @@ func (d *Dereferencer) enrichAccount(
}

if latestAcc.Domain == "" {
// ensure we have a domain set by this point,
// Ensure we have a domain set by this point,
// otherwise it gets stored as a local user!
//
// TODO: there is probably a more granular way
Expand All @@ -562,7 +632,16 @@ func (d *Dereferencer) enrichAccount(
return nil, nil, gtserror.Newf("empty domain for %s", uri)
}

// Ensure ID is set and update fetch time.
/*
BY THIS POINT we have more or less a fullly-formed
representation of the target account, derived from
a combination of webfinger lookups and dereferencing.
Further fetching beyond this point is for peripheral
things like account avatar, header, emojis.
*/

// Ensure internal db ID is
// set and update fetch time.
latestAcc.ID = account.ID
latestAcc.FetchedAt = time.Now()

Expand All @@ -581,12 +660,14 @@ func (d *Dereferencer) enrichAccount(
log.Errorf(ctx, "error fetching remote emojis for account %s: %v", uri, err)
}

if account.CreatedAt.IsZero() {
// CreatedAt will be zero if no local copy was
// found in one of the GetAccountBy___() functions.
//
// Set time of creation from the last-fetched date.
latestAcc.CreatedAt = latestAcc.FetchedAt
if account.IsNew() {
// Prefer published/created time from
// apubAcc, fall back to FetchedAt value.
if latestAcc.CreatedAt.IsZero() {
latestAcc.CreatedAt = latestAcc.FetchedAt
}

// Set time of update from the last-fetched date.
latestAcc.UpdatedAt = latestAcc.FetchedAt

// This is new, put it in the database.
Expand All @@ -595,11 +676,16 @@ func (d *Dereferencer) enrichAccount(
return nil, nil, gtserror.Newf("error putting in database: %w", err)
}
} else {
// Prefer published time from apubAcc,
// fall back to previous stored value.
if latestAcc.CreatedAt.IsZero() {
latestAcc.CreatedAt = account.CreatedAt
}

// Set time of update from the last-fetched date.
latestAcc.UpdatedAt = latestAcc.FetchedAt

// Use existing account values.
latestAcc.CreatedAt = account.CreatedAt
// Carry over existing account language.
latestAcc.Language = account.Language

// This is an existing account, update the model in the database.
Expand Down

0 comments on commit a3aa604

Please sign in to comment.