[chore] Rationalize HTTP return codes for fedi endpoints, other tidying up (#4503)

# Description

> If this is a code change, please include a summary of what you've coded, and link to the issue(s) it closes/implements.
>
> If this is a documentation change, please briefly describe what you've changed and why.

This pull request does some refactoring of the fedi API endpoints and processing functions, and the authenticate + pub key deref functions, to try to return fewer silly HTTP codes like 410 Gone (when a *remote* account is gone, not a local one), and 500 errors where something isn't really an error.

Also does some general tidying up and renaming for consistency.

## Checklist

Please put an x inside each checkbox to indicate that you've read and followed it: `[ ]` -> `[x]`

If this is a documentation change, only the first checkbox must be filled (you can delete the others if you want).

- [x] I/we have read the [GoToSocial contribution guidelines](https://codeberg.org/superseriousbusiness/gotosocial/src/branch/main/CONTRIBUTING.md).
- [x] I/we have discussed the proposed changes already, either in an issue on the repository, or in the Matrix chat.
- [x] I/we have not leveraged AI to create the proposed changes.
- [x] I/we have performed a self-review of added code.
- [x] I/we have written code that is legible and maintainable by others.
- [x] I/we have commented the added code, particularly in hard-to-understand areas.
- [ ] I/we have made any necessary changes to documentation.
- [ ] I/we have added tests that cover new code.
- [x] I/we have run tests and they pass locally with the changes.
- [x] I/we have run `go fmt ./...` and `golangci-lint run`.

Reviewed-on: https://codeberg.org/superseriousbusiness/gotosocial/pulls/4503
Co-authored-by: tobi <tobi.smethurst@protonmail.com>
Co-committed-by: tobi <tobi.smethurst@protonmail.com>

1 files changed, 47 insertions, 26 deletions

diff --git a/internal/federation/federatingprotocol.go b/internal/federation/federatingprotocol.go
index e1ec86b32..2f6953257 100644
--- a/internal/federation/federatingprotocol.go
+++ b/internal/federation/federatingprotocol.go
@@ -198,39 +198,56 @@ func (f *Federator) AuthenticatePostInbox(ctx context.Context, w http.ResponseWr
 	// account by parsing username from `/users/{username}/inbox`.
 	username, err := uris.ParseInboxPath(r.URL)
 	if err != nil {
-		err = gtserror.Newf("could not parse %s as inbox path: %w", r.URL.String(), err)
+		err := gtserror.Newf("could not parse %s as inbox path: %w", r.URL.String(), err)
 		return nil, false, err
 	}
 
 	if username == "" {
-		err = gtserror.New("inbox username was empty")
+		err := gtserror.New("inbox username was empty")
 		return nil, false, err
 	}
 
-	receivingAccount, err := f.db.GetAccountByUsernameDomain(ctx, username, "")
-	if err != nil {
-		err = gtserror.Newf("could not fetch receiving account %s: %w", username, err)
+	// Get the receiving local account inbox
+	// owner with given username from database.
+	receiver, err := f.db.GetAccountByUsernameDomain(ctx, username, "")
+	if err != nil && !errors.Is(err, db.ErrNoEntries) {
+		err := gtserror.Newf("db error getting receiving account %s: %w", username, err)
 		return nil, false, err
 	}
 
+	if receiver == nil {
+		// Maybe we had this account at some point and someone
+		// manually deleted it from the DB. Just return not found.
+		err := gtserror.Newf("receiving account %s not found in the db", username)
+		errWithCode := gtserror.NewErrorNotFound(err)
+		return ctx, false, errWithCode
+	}
+
 	// Check who's trying to deliver to us by inspecting the http signature.
-	pubKeyAuth, errWithCode := f.AuthenticateFederatedRequest(ctx, receivingAccount.Username)
+	pubKeyAuth, errWithCode := f.AuthenticateFederatedRequest(ctx, receiver.Username)
 	if errWithCode != nil {
-		switch errWithCode.Code() {
-		case http.StatusUnauthorized, http.StatusForbidden, http.StatusBadRequest:
-			// If codes 400, 401, or 403, obey the go-fed
-			// interface by writing the header and bailing.
-			w.WriteHeader(errWithCode.Code())
-		case http.StatusGone:
-			// If the requesting account's key has gone
-			// (410) then likely inbox post was a delete.
+
+		// Check if we got an error code from a remote
+		// instance while trying to dereference the pub
+		// key owner who's trying to post to this inbox.
+		if gtserror.StatusCode(errWithCode) == http.StatusGone {
+			// If the pub key owner's key/account has gone
+			// (410) then likely inbox post was a Delete.
 			//
-			// We can just write 202 and leave: we didn't
-			// know about the account anyway, so we can't
-			// do any further processing.
+			// If so, we can just write 202 and leave, as
+			// either we'll have already processed any Deletes
+			// sent by this account, or we never met the account
+			// in the first place so we don't have any of their
+			// stuff stored to actually delete.
 			w.WriteHeader(http.StatusAccepted)
+			return ctx, false, nil
 		}
 
+		// In all other cases, obey the go-fed
+		// interface by writing the status
+		// code from the returned ErrWithCode.
+		w.WriteHeader(errWithCode.Code())
+
 		// We still return the error
 		// for later request logging.
 		return ctx, false, errWithCode
@@ -247,7 +264,11 @@ func (f *Federator) AuthenticatePostInbox(ctx context.Context, w http.ResponseWr
 	// We have everything we need now, set the requesting
 	// and receiving accounts on the context for later use.
 	ctx = gtscontext.SetRequestingAccount(ctx, pubKeyAuth.Owner)
-	ctx = gtscontext.SetReceivingAccount(ctx, receivingAccount)
+	ctx = gtscontext.SetReceivingAccount(ctx, receiver)
+
+	// Note: we do not check here yet whether requesting
+	// account has been suspended or self-deleted, as that
+	// is handled in *federatingActor.PostInboxScheme
 	return ctx, true, nil
 }
 
@@ -290,7 +311,7 @@ func (f *Federator) Blocked(ctx context.Context, actorIRIs []*url.URL) (bool, er
 	// then we can save some work.
 	blocked, err := f.db.AreURIsBlocked(ctx, actorIRIs)
 	if err != nil {
-		err = gtserror.Newf("error checking domain blocks of actorIRIs: %w", err)
+		err := gtserror.Newf("error checking domain blocks of actorIRIs: %w", err)
 		return false, err
 	}
 
@@ -302,7 +323,7 @@ func (f *Federator) Blocked(ctx context.Context, actorIRIs []*url.URL) (bool, er
 	// Now user level blocks. Receiver should not block requester.
 	blocked, err = f.db.IsBlocked(ctx, receivingAccount.ID, requestingAccount.ID)
 	if err != nil {
-		err = gtserror.Newf("db error checking block between receiver and requester: %w", err)
+		err := gtserror.Newf("db error checking block between receiver and requester: %w", err)
 		return false, err
 	}
 
@@ -358,7 +379,7 @@ func (f *Federator) Blocked(ctx context.Context, actorIRIs []*url.URL) (bool, er
 		)
 		if err != nil && !errors.Is(err, db.ErrNoEntries) {
 			// Real db error.
-			err = gtserror.Newf("db error trying to get %s as account: %w", iriStr, err)
+			err := gtserror.Newf("db error trying to get %s as account: %w", iriStr, err)
 			return false, err
 		} else if err == nil {
 			// IRI is for an account.
@@ -373,7 +394,7 @@ func (f *Federator) Blocked(ctx context.Context, actorIRIs []*url.URL) (bool, er
 		)
 		if err != nil && !errors.Is(err, db.ErrNoEntries) {
 			// Real db error.
-			err = gtserror.Newf("db error trying to get %s as status: %w", iriStr, err)
+			err := gtserror.Newf("db error trying to get %s as status: %w", iriStr, err)
 			return false, err
 		} else if err == nil {
 			// IRI is for a status.
@@ -395,9 +416,9 @@ func (f *Federator) Blocked(ctx context.Context, actorIRIs []*url.URL) (bool, er
 		// account they have blocked. In this case, it's v. unlikely
 		// they care to see the boost in their timeline, so there's
 		// no point in us processing it.
-		blocked, err = f.db.IsBlocked(ctx, receivingAccount.ID, accountID)
+		blocked, err := f.db.IsBlocked(ctx, receivingAccount.ID, accountID)
 		if err != nil {
-			err = gtserror.Newf("db error checking block between receiver and other account: %w", err)
+			err := gtserror.Newf("db error checking block between receiver and other account: %w", err)
 			return false, err
 		}
 
@@ -418,9 +439,9 @@ func (f *Federator) Blocked(ctx context.Context, actorIRIs []*url.URL) (bool, er
 		// accounts are gossiping about + trying to tag a third account
 		// who has one or the other of them blocked.
 		if iriHost == ourHost {
-			blocked, err = f.db.IsBlocked(ctx, accountID, requestingAccount.ID)
+			blocked, err := f.db.IsBlocked(ctx, accountID, requestingAccount.ID)
 			if err != nil {
-				err = gtserror.Newf("db error checking block between other account and requester: %w", err)
+				err := gtserror.Newf("db error checking block between other account and requester: %w", err)
 				return false, err
 			}