From 42d8011ff4f10664a853c483685db8e97b7a3118 Mon Sep 17 00:00:00 2001 From: kim <89579420+NyaaaWhatsUpDoc@users.noreply.github.com> Date: Tue, 21 Nov 2023 10:35:30 +0000 Subject: [PATCH] [chore/security] refactor AuthenticateFederatedRequest() to handle account deref + suspension checks (#2371) * refactor AuthenticateFederatedRequest() to handle account suspension + fetching of owner * small fixups * small changes * revert to 'IsEitherBlocked' instead of just 'IsBlocked" :grimace: * update code comment to indicate that AuthenticateFederatedRequest() will handle account + instance dereferencing --- internal/federation/authenticate.go | 224 ++++++++++++------ .../federation/dereferencing/handshake.go | 5 +- internal/federation/federatingprotocol.go | 67 +----- internal/processing/admin/domainkeysexpire.go | 4 +- internal/processing/fedi/common.go | 33 +-- internal/processing/fedi/status.go | 1 - internal/processing/fedi/user.go | 69 ++---- 7 files changed, 205 insertions(+), 198 deletions(-) diff --git a/internal/federation/authenticate.go b/internal/federation/authenticate.go index a42633b8f..fe611af8c 100644 --- a/internal/federation/authenticate.go +++ b/internal/federation/authenticate.go @@ -40,12 +40,7 @@ ) var ( - errUnsigned = errors.New("http request wasn't signed or http signature was invalid") - signingAlgorithms = []httpsig.Algorithm{ - httpsig.RSA_SHA256, // Prefer common RSA_SHA256. - httpsig.RSA_SHA512, // Fall back to less common RSA_SHA512. - httpsig.ED25519, // Try ED25519 as a long shot. - } + errUnsigned = errors.New("http request wasn't signed or http signature was invalid") ) // PubKeyAuth models authorization information for a remote @@ -67,21 +62,18 @@ type PubKeyAuth struct { // OwnerURI is the ActivityPub id of the owner of // the public key used to sign the request we're - // now authenticating. This will always be set - // even if Owner isn't, so that callers can use - // this URI to go fetch the Owner from remote. + // now authenticating. This will always be set. OwnerURI *url.URL - // Owner is the account corresponding to OwnerURI. - // - // Owner will only be defined if the account who - // owns the public key was already cached in the - // database when we received the request we're now - // authenticating (ie., we've seen it before). - // - // If it's not defined, callers should use OwnerURI - // to go and dereference it. + // Owner is the account corresponding to + // OwnerURI. This will always be set UNLESS + // the PubKeyAuth.Handshaking field is set.. Owner *gtsmodel.Account + + // Handshaking indicates that uncached owner + // account was NOT dereferenced due to an ongoing + // handshake with another instance. + Handshaking bool } // AuthenticateFederatedRequest authenticates any kind of incoming federated @@ -103,12 +95,12 @@ type PubKeyAuth struct { // know that this is the user making the dereferencing request, and they can decide // to allow or deny the request depending on their settings. // +// This function will handle dereferencing and storage of any new remote accounts +// and / or instances. The returned PubKeyAuth{}.Owner account will ONLY ever be +// nil in the case that there is an ongoing handshake involving this account. +// // Note that it is also valid to pass in an empty string here, in which case the // keys of the instance account will be used. -// -// Also note that this function *does not* dereference the remote account that -// the signature key is associated with. Other functions should use the returned -// URL to dereference the remote account, if required. func (f *Federator) AuthenticateFederatedRequest(ctx context.Context, requestedUsername string) (*PubKeyAuth, gtserror.WithCode) { // Thanks to the signature check middleware, // we should already have an http signature @@ -142,7 +134,7 @@ func (f *Federator) AuthenticateFederatedRequest(ctx context.Context, requestedU var ( pubKeyIDStr = pubKeyID.String() - local = (pubKeyID.Host == config.GetHost()) + isLocal = (pubKeyID.Host == config.GetHost()) pubKeyAuth *PubKeyAuth errWithCode gtserror.WithCode ) @@ -154,7 +146,7 @@ func (f *Federator) AuthenticateFederatedRequest(ctx context.Context, requestedU {"pubKeyID", pubKeyIDStr}, }...) - if local { + if isLocal { l.Trace("public key is local, no dereference needed") pubKeyAuth, errWithCode = f.derefPubKeyDBOnly(ctx, pubKeyIDStr) } else { @@ -166,7 +158,7 @@ func (f *Federator) AuthenticateFederatedRequest(ctx context.Context, requestedU return nil, errWithCode } - if local && pubKeyAuth == nil { + if isLocal && pubKeyAuth == nil { // We signed this request, apparently, but // local lookup didn't find anything. This // is an almost impossible error condition! @@ -175,37 +167,61 @@ func (f *Federator) AuthenticateFederatedRequest(ctx context.Context, requestedU return nil, gtserror.NewErrorInternalError(err) } - // Try to authenticate using permitted algorithms in - // order of most -> least common, checking each defined - // pubKey for this Actor. Return OK as soon as one passes. - for _, pubKey := range [2]*rsa.PublicKey{ - pubKeyAuth.FetchedPubKey, - pubKeyAuth.CachedPubKey, - } { - if pubKey == nil { - continue + // Attempt to verify auth with both fetched and cached keys. + if !verifyAuth(&l, verifier, pubKeyAuth.CachedPubKey) && + !verifyAuth(&l, verifier, pubKeyAuth.FetchedPubKey) { + + const format = "authentication NOT PASSED for public key %s; tried algorithms %+v; signature value was '%s'" + text := fmt.Sprintf(format, pubKeyIDStr, signingAlgorithms, signature) + return nil, gtserror.NewErrorUnauthorized(errors.New(text), text) + } + + if pubKeyAuth.Owner == nil { + // Ensure we have instance stored in + // database for the account at URI. + err := f.fetchAccountInstance(ctx, + requestedUsername, + pubKeyAuth.OwnerURI, + ) + if err != nil { + return nil, gtserror.NewErrorInternalError(err) } - for _, algo := range signingAlgorithms { - l.Tracef("trying %s", algo) + // If we're currently handshaking with another instance, return + // without derefing the owner, the only possible time we do this. + // This prevents deadlocks when GTS instances mutually deref. + if f.Handshaking(requestedUsername, pubKeyAuth.OwnerURI) { + log.Warnf(ctx, "network race during %s handshake", pubKeyAuth.OwnerURI) + pubKeyAuth.Handshaking = true + return pubKeyAuth, nil + } - err := verifier.Verify(pubKey, algo) - if err == nil { - l.Tracef("authentication PASSED with %s", algo) - return pubKeyAuth, nil + // Dereference the account located at owner URI. + pubKeyAuth.Owner, _, err = f.GetAccountByURI(ctx, + requestedUsername, + pubKeyAuth.OwnerURI, + ) + if err != nil { + if gtserror.StatusCode(err) == http.StatusGone { + // This can happen here instead of the pubkey 'gone' + // checks due to: the server sending account deletion + // notifications out, we start processing, the request above + // succeeds, and *then* the profile is removed and starts + // returning 410 Gone, at which point _this_ request fails. + return nil, gtserror.NewErrorGone(err) } - l.Tracef("authentication NOT PASSED with %s: %q", algo, err) + err := gtserror.Newf("error dereferencing account %s: %w", pubKeyAuth.OwnerURI, err) + return nil, gtserror.NewErrorInternalError(err) } } - // At this point no algorithms passed. - err := gtserror.Newf( - "authentication NOT PASSED for public key %s; tried algorithms %+v; signature value was '%s'", - pubKeyIDStr, signature, signingAlgorithms, - ) + if !pubKeyAuth.Owner.SuspendedAt.IsZero() { + const text = "requesting account suspended" + return nil, gtserror.NewErrorForbidden(errors.New(text)) + } - return nil, gtserror.NewErrorUnauthorized(err, err.Error()) + return pubKeyAuth, nil } // derefPubKeyDBOnly tries to dereference the given @@ -218,22 +234,27 @@ func (f *Federator) AuthenticateFederatedRequest(ctx context.Context, requestedU func (f *Federator) derefPubKeyDBOnly( ctx context.Context, pubKeyIDStr string, -) (*PubKeyAuth, gtserror.WithCode) { +) ( + *PubKeyAuth, + gtserror.WithCode, +) { + // Look for pubkey ID owner in the database. owner, err := f.db.GetAccountByPubkeyID(ctx, pubKeyIDStr) - if err != nil { - if errors.Is(err, db.ErrNoEntries) { - // We don't have this - // account stored (yet). - return nil, nil - } - + if err != nil && !errors.Is(err, db.ErrNoEntries) { err = gtserror.Newf("db error getting account with pubKeyID %s: %w", pubKeyIDStr, err) return nil, gtserror.NewErrorInternalError(err) } + if owner == nil { + // We don't have this + // account stored (yet). + return nil, nil + } + + // Parse owner account URI as URL obj. ownerURI, err := url.Parse(owner.URI) if err != nil { - err = gtserror.Newf("error parsing account uri with pubKeyID %s: %w", pubKeyIDStr, err) + err := gtserror.Newf("error parsing account uri with pubKeyID %s: %w", pubKeyIDStr, err) return nil, gtserror.NewErrorInternalError(err) } @@ -253,7 +274,10 @@ func (f *Federator) derefPubKey( requestedUsername string, pubKeyIDStr string, pubKeyID *url.URL, -) (*PubKeyAuth, gtserror.WithCode) { +) ( + *PubKeyAuth, + gtserror.WithCode, +) { l := log. WithContext(ctx). WithFields(kv.Fields{ @@ -316,7 +340,7 @@ func (f *Federator) derefPubKey( // Extract the key and the owner from the response. pubKey, pubKeyOwner, err := parsePubKeyBytes(ctx, pubKeyBytes, pubKeyID) if err != nil { - err := fmt.Errorf("error parsing public key (%s): %w", pubKeyID, err) + err := gtserror.Newf("error parsing public key (%s): %w", pubKeyID, err) return nil, gtserror.NewErrorUnauthorized(err) } @@ -337,16 +361,12 @@ func (f *Federator) derefPubKey( // had an owner stored for it locally. Since // we now successfully refreshed the pub key, // we should update the account to reflect that. - ownerAcct := pubKeyAuth.Owner - ownerAcct.PublicKey = pubKeyAuth.FetchedPubKey - ownerAcct.PublicKeyExpiresAt = time.Time{} - - l.Info("obtained a new public key to replace expired key, caching now; " + - "authorization for this request will be attempted with both old and new keys") - + owner := pubKeyAuth.Owner + owner.PublicKey = pubKeyAuth.FetchedPubKey + owner.PublicKeyExpiresAt = time.Time{} if err := f.db.UpdateAccount( ctx, - ownerAcct, + owner, "public_key", "public_key_expires_at", ); err != nil { @@ -354,6 +374,8 @@ func (f *Federator) derefPubKey( return nil, gtserror.NewErrorInternalError(err) } + l.Info("obtained new public key to replace expired; attempting auth with old / new") + // Return both new and cached (now // expired) keys, authentication // will be attempted with both. @@ -406,6 +428,47 @@ func (f *Federator) callForPubKey( return nil, gtserror.NewErrorInternalError(err) } +// fetchAccountInstance ensures that an instance model exists in +// the database for the given account URI, deref'ing if necessary. +func (f *Federator) fetchAccountInstance( + ctx context.Context, + requestedUsername string, + accountURI *url.URL, +) error { + // Look for an existing entry for instance in database. + instance, err := f.db.GetInstance(ctx, accountURI.Host) + if err != nil && !errors.Is(err, db.ErrNoEntries) { + return gtserror.Newf("error getting instance from database: %w", err) + } + + if instance != nil { + // already fetched. + return nil + } + + // We don't have an entry for this + // instance yet; go dereference it. + instance, err = f.GetRemoteInstance( + gtscontext.SetFastFail(ctx), + requestedUsername, + &url.URL{ + Scheme: accountURI.Scheme, + Host: accountURI.Host, + }, + ) + if err != nil { + return gtserror.Newf("error dereferencing instance %s: %w", accountURI.Host, err) + } + + // Insert new instance into the datbase. + err = f.db.PutInstance(ctx, instance) + if err != nil && !errors.Is(err, db.ErrAlreadyExists) { + return gtserror.Newf("error inserting instance %s into database: %w", accountURI.Host, err) + } + + return nil +} + // parsePubKeyBytes extracts an rsa public key from the // given pubKeyBytes by trying to parse the pubKeyBytes // as an ActivityPub type. It will return the public key @@ -439,3 +502,32 @@ func parsePubKeyBytes( return pubKey, pubKeyOwnerID, nil } + +var signingAlgorithms = []httpsig.Algorithm{ + httpsig.RSA_SHA256, // Prefer common RSA_SHA256. + httpsig.RSA_SHA512, // Fall back to less common RSA_SHA512. + httpsig.ED25519, // Try ED25519 as a long shot. +} + +// verifyAuth verifies auth using generated verifier, according to pubkey and our supported signing algorithms. +func verifyAuth(l *log.Entry, verifier httpsig.Verifier, pubKey *rsa.PublicKey) bool { + if pubKey == nil { + return false + } + + // Loop through all supported algorithms. + for _, algo := range signingAlgorithms { + + // Verify according to pubkey and algo. + err := verifier.Verify(pubKey, algo) + if err != nil { + l.Tracef("authentication NOT PASSED with %s: %v", algo, err) + continue + } + + l.Tracef("authenticated PASSED with %s", algo) + return true + } + + return false +} diff --git a/internal/federation/dereferencing/handshake.go b/internal/federation/dereferencing/handshake.go index 1180ff140..253c5f2da 100644 --- a/internal/federation/dereferencing/handshake.go +++ b/internal/federation/dereferencing/handshake.go @@ -38,8 +38,11 @@ func (d *Dereferencer) Handshaking(username string, remoteAccountID *url.URL) bo return false } + // Calculate remote account ID str once. + remoteIDStr := remoteAccountID.String() + for _, id := range remoteIDs { - if id.String() == remoteAccountID.String() { + if id.String() == remoteIDStr { // We are currently handshaking // with the remote account. return true diff --git a/internal/federation/federatingprotocol.go b/internal/federation/federatingprotocol.go index 802a8b1dc..f3ab1ae3c 100644 --- a/internal/federation/federatingprotocol.go +++ b/internal/federation/federatingprotocol.go @@ -232,72 +232,17 @@ func (f *Federator) AuthenticatePostInbox(ctx context.Context, w http.ResponseWr } } - pubKeyOwnerURI := pubKeyAuth.OwnerURI - - // Authentication has passed, check if we need to create a - // new instance entry for the Host of the requesting account. - if _, err := f.db.GetInstance(ctx, pubKeyOwnerURI.Host); err != nil { - if !errors.Is(err, db.ErrNoEntries) { - // There's been an actual error. - err = gtserror.Newf("error getting instance %s: %w", pubKeyOwnerURI.Host, err) - return ctx, false, err - } - - // We don't have an entry for this - // instance yet; go dereference it. - instance, err := f.GetRemoteInstance( - gtscontext.SetFastFail(ctx), - username, - &url.URL{ - Scheme: pubKeyOwnerURI.Scheme, - Host: pubKeyOwnerURI.Host, - }, - ) - if err != nil { - err = gtserror.Newf("error dereferencing instance %s: %w", pubKeyOwnerURI.Host, err) - return nil, false, err - } - - if err := f.db.PutInstance(ctx, instance); err != nil && !errors.Is(err, db.ErrAlreadyExists) { - err = gtserror.Newf("error inserting instance entry for %s: %w", pubKeyOwnerURI.Host, err) - return nil, false, err - } - } - - // We know the public key owner URI now, so we can - // dereference the remote account (or just get it - // from the db if we already have it). - requestingAccount, _, err := f.GetAccountByURI( - gtscontext.SetFastFail(ctx), - username, - pubKeyOwnerURI, - ) - if err != nil { - if gtserror.StatusCode(err) == http.StatusGone { - // This is the same case as the http.StatusGone check above. - // It can happen here and not there because there's a race - // where the sending server starts sending account deletion - // notifications out, we start processing, the request above - // succeeds, and *then* the profile is removed and starts - // returning 410 Gone, at which point _this_ request fails. - w.WriteHeader(http.StatusAccepted) - return ctx, false, nil - } - - err = gtserror.Newf("couldn't get requesting account %s: %w", pubKeyOwnerURI, err) - return nil, false, err - } - - if !requestingAccount.SuspendedAt.IsZero() { - // Account was marked as suspended by a - // local admin action. Stop request early. - w.WriteHeader(http.StatusForbidden) + if pubKeyAuth.Handshaking { + // There is a mutal handshake occurring between us and + // the owner URI. Return 202 and leave as we can't do + // much else until the handshake procedure has finished. + w.WriteHeader(http.StatusAccepted) return ctx, false, nil } // We have everything we need now, set the requesting // and receiving accounts on the context for later use. - ctx = gtscontext.SetRequestingAccount(ctx, requestingAccount) + ctx = gtscontext.SetRequestingAccount(ctx, pubKeyAuth.Owner) ctx = gtscontext.SetReceivingAccount(ctx, receivingAccount) return ctx, true, nil } diff --git a/internal/processing/admin/domainkeysexpire.go b/internal/processing/admin/domainkeysexpire.go index 886da8b2f..9853becbd 100644 --- a/internal/processing/admin/domainkeysexpire.go +++ b/internal/processing/admin/domainkeysexpire.go @@ -71,9 +71,7 @@ func (p *Processor) domainKeysExpireSideEffects(ctx context.Context, domain stri // the public key and update the account. if err := p.rangeDomainAccounts(ctx, domain, func(account *gtsmodel.Account) { account.PublicKeyExpiresAt = expiresAt - - if err := p.state.DB.UpdateAccount( - ctx, + if err := p.state.DB.UpdateAccount(ctx, account, "public_key_expires_at", ); err != nil { diff --git a/internal/processing/fedi/common.go b/internal/processing/fedi/common.go index f395ec3cf..a02779e73 100644 --- a/internal/processing/fedi/common.go +++ b/internal/processing/fedi/common.go @@ -22,7 +22,6 @@ "errors" "github.com/superseriousbusiness/gotosocial/internal/db" - "github.com/superseriousbusiness/gotosocial/internal/gtscontext" "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/gtsmodel" ) @@ -45,8 +44,6 @@ func (p *Processor) authenticate(ctx context.Context, requestedUser string) ( return nil, nil, gtserror.NewErrorNotFound(err) } - var requester *gtsmodel.Account - // Ensure request signed, and use signature URI to // get requesting account, dereferencing if necessary. pubKeyAuth, errWithCode := p.federator.AuthenticateFederatedRequest(ctx, requestedUser) @@ -54,33 +51,23 @@ func (p *Processor) authenticate(ctx context.Context, requestedUser string) ( return nil, nil, errWithCode } - if requester = pubKeyAuth.Owner; requester == nil { - requester, _, err = p.federator.GetAccountByURI( - gtscontext.SetFastFail(ctx), - requestedUser, - pubKeyAuth.OwnerURI, - ) - if err != nil { - err = gtserror.Newf("error getting account %s: %w", pubKeyAuth.OwnerURI, err) - return nil, nil, gtserror.NewErrorUnauthorized(err) - } + if pubKeyAuth.Handshaking { + // This should happen very rarely, we are in the middle of handshaking. + err := gtserror.Newf("network race handshaking %s", pubKeyAuth.OwnerURI) + return nil, nil, gtserror.NewErrorInternalError(err) } - if !requester.SuspendedAt.IsZero() { - // Account was marked as suspended by a - // local admin action. Stop request early. - const text = "requesting account is suspended" - return nil, nil, gtserror.NewErrorForbidden(errors.New(text)) - } + // Get requester from auth. + requester := pubKeyAuth.Owner - // Ensure no block exists between requester + requested. + // Check that block does not exist between receiver and requester. blocked, err := p.state.DB.IsEitherBlocked(ctx, receiver.ID, requester.ID) if err != nil { - err = gtserror.Newf("db error getting checking block: %w", err) + err := gtserror.Newf("error checking block: %w", err) return nil, nil, gtserror.NewErrorInternalError(err) } else if blocked { - err = gtserror.Newf("block exists between accounts %s and %s", requester.ID, receiver.ID) - return nil, nil, gtserror.NewErrorForbidden(err) + const text = "block exists between accounts" + return nil, nil, gtserror.NewErrorForbidden(errors.New(text)) } return requester, receiver, nil diff --git a/internal/processing/fedi/status.go b/internal/processing/fedi/status.go index b8b75841c..8082c79bc 100644 --- a/internal/processing/fedi/status.go +++ b/internal/processing/fedi/status.go @@ -35,7 +35,6 @@ // StatusGet handles the getting of a fedi/activitypub representation of a local status. // It performs appropriate authentication before returning a JSON serializable interface. func (p *Processor) StatusGet(ctx context.Context, requestedUser string, statusID string) (interface{}, gtserror.WithCode) { - // Authenticate using http signature. // Authenticate the incoming request, getting related user accounts. requester, receiver, errWithCode := p.authenticate(ctx, requestedUser) if errWithCode != nil { diff --git a/internal/processing/fedi/user.go b/internal/processing/fedi/user.go index 17663a8f4..bf14554cf 100644 --- a/internal/processing/fedi/user.go +++ b/internal/processing/fedi/user.go @@ -26,7 +26,6 @@ "github.com/superseriousbusiness/activity/streams/vocab" "github.com/superseriousbusiness/gotosocial/internal/ap" "github.com/superseriousbusiness/gotosocial/internal/db" - "github.com/superseriousbusiness/gotosocial/internal/gtscontext" "github.com/superseriousbusiness/gotosocial/internal/gtserror" "github.com/superseriousbusiness/gotosocial/internal/uris" ) @@ -35,7 +34,7 @@ // performing authentication before returning a JSON serializable interface to the caller. func (p *Processor) UserGet(ctx context.Context, requestedUsername string, requestURL *url.URL) (interface{}, gtserror.WithCode) { // (Try to) get the requested local account from the db. - requestedAccount, err := p.state.DB.GetAccountByUsernameDomain(ctx, requestedUsername, "") + receiver, err := p.state.DB.GetAccountByUsernameDomain(ctx, requestedUsername, "") if err != nil { if errors.Is(err, db.ErrNoEntries) { // Account just not found w/ this username. @@ -54,8 +53,9 @@ func (p *Processor) UserGet(ctx context.Context, requestedUsername string, reque // the bare minimum user profile needed for the pubkey. // // TODO: https://github.com/superseriousbusiness/gotosocial/issues/1186 - minimalPerson, err := p.converter.AccountToASMinimal(ctx, requestedAccount) + minimalPerson, err := p.converter.AccountToASMinimal(ctx, receiver) if err != nil { + err := gtserror.Newf("error converting to minimal account: %w", err) return nil, gtserror.NewErrorInternalError(err) } @@ -72,56 +72,39 @@ func (p *Processor) UserGet(ctx context.Context, requestedUsername string, reque } // Auth passed, generate the proper AP representation. - person, err := p.converter.AccountToAS(ctx, requestedAccount) + person, err := p.converter.AccountToAS(ctx, receiver) if err != nil { + err := gtserror.Newf("error converting account: %w", err) return nil, gtserror.NewErrorInternalError(err) } - // If we are currently handshaking with the remote account - // making the request, then don't be coy: just serve the AP - // representation of the target account. - // - // This handshake check ensures that we don't get stuck in - // a loop with another GtS instance, where each instance is - // trying repeatedly to dereference the other account that's - // making the request before it will reveal its own account. - // - // Instead, we end up in an 'I'll show you mine if you show me - // yours' situation, where we sort of agree to reveal each - // other's profiles at the same time. - if p.federator.Handshaking(requestedUsername, pubKeyAuth.OwnerURI) { + if pubKeyAuth.Handshaking { + // If we are currently handshaking with the remote account + // making the request, then don't be coy: just serve the AP + // representation of the target account. + // + // This handshake check ensures that we don't get stuck in + // a loop with another GtS instance, where each instance is + // trying repeatedly to dereference the other account that's + // making the request before it will reveal its own account. + // + // Instead, we end up in an 'I'll show you mine if you show me + // yours' situation, where we sort of agree to reveal each + // other's profiles at the same time. return data(person) } - // We're not currently handshaking with the requestingAccountURI, - // so fetch its details and ensure it's up to date + not blocked. - requestingAccount, _, err := p.federator.GetAccountByURI( - // On a hot path so fail quickly. - gtscontext.SetFastFail(ctx), - requestedUsername, - pubKeyAuth.OwnerURI, - ) - if err != nil { - err := gtserror.Newf("error getting account %s: %w", pubKeyAuth.OwnerURI, err) - return nil, gtserror.NewErrorUnauthorized(err) - } + // Get requester from auth. + requester := pubKeyAuth.Owner - if !requestingAccount.SuspendedAt.IsZero() { - // Account was marked as suspended by a - // local admin action. Stop request early. - err = fmt.Errorf("account %s marked as suspended", requestingAccount.ID) - return nil, gtserror.NewErrorForbidden(err) - } - - blocked, err := p.state.DB.IsBlocked(ctx, requestedAccount.ID, requestingAccount.ID) + // Check that block does not exist between receiver and requester. + blocked, err := p.state.DB.IsBlocked(ctx, receiver.ID, requester.ID) if err != nil { - err := gtserror.Newf("error checking block from account %s to account %s: %w", requestedAccount.ID, requestingAccount.ID, err) + err := gtserror.Newf("error checking block: %w", err) return nil, gtserror.NewErrorInternalError(err) - } - - if blocked { - err := fmt.Errorf("account %s blocks account %s", requestedAccount.ID, requestingAccount.ID) - return nil, gtserror.NewErrorForbidden(err) + } else if blocked { + const text = "block exists between accounts" + return nil, gtserror.NewErrorForbidden(errors.New(text)) } return data(person)