[chore] Refactor AP authentication, other small bits of tidying up (#1874)

9 files changed, 43 insertions, 79 deletions

diff --git a/internal/federation/federatingdb/accept.go b/internal/federation/federatingdb/accept.go
index 1c39bc134..7d3e16d4e 100644
--- a/internal/federation/federatingdb/accept.go
+++ b/internal/federation/federatingdb/accept.go
@@ -41,12 +41,9 @@ func (f *federatingDB) Accept(ctx context.Context, accept vocab.ActivityStreamsA
 		l.Debug("entering Accept")
 	}
 
-	receivingAccount, _ := extractFromCtx(ctx)
-	if receivingAccount == nil {
-		// If the receiving account  wasn't set on the context, that means this request didn't pass
-		// through the API, but came from inside GtS as the result of another activity on this instance. That being so,
-		// we can safely just ignore this activity, since we know we've already processed it elsewhere.
-		return nil
+	receivingAccount, _, internal := extractFromCtx(ctx)
+	if internal {
+		return nil // Already processed.
 	}
 
 	acceptObject := accept.GetActivityStreamsObject()
diff --git a/internal/federation/federatingdb/announce.go b/internal/federation/federatingdb/announce.go
index 88b150cf0..ab230cdfb 100644
--- a/internal/federation/federatingdb/announce.go
+++ b/internal/federation/federatingdb/announce.go
@@ -39,12 +39,9 @@ func (f *federatingDB) Announce(ctx context.Context, announce vocab.ActivityStre
 		l.Debug("entering Announce")
 	}
 
-	receivingAccount, _ := extractFromCtx(ctx)
-	if receivingAccount == nil {
-		// If the receiving account wasn't set on the context, that means this request didn't pass
-		// through the API, but came from inside GtS as the result of another activity on this instance. That being so,
-		// we can safely just ignore this activity, since we know we've already processed it elsewhere.
-		return nil
+	receivingAccount, _, internal := extractFromCtx(ctx)
+	if internal {
+		return nil // Already processed.
 	}
 
 	boost, isNew, err := f.typeConverter.ASAnnounceToStatus(ctx, announce)
diff --git a/internal/federation/federatingdb/create.go b/internal/federation/federatingdb/create.go
index f68279d37..3650f8c3d 100644
--- a/internal/federation/federatingdb/create.go
+++ b/internal/federation/federatingdb/create.go
@@ -57,12 +57,9 @@ func (f *federatingDB) Create(ctx context.Context, asType vocab.Type) error {
 		l.Trace("entering Create")
 	}
 
-	receivingAccount, requestingAccount := extractFromCtx(ctx)
-	if receivingAccount == nil {
-		// If the receiving account wasn't set on the context, that means this request didn't pass
-		// through the API, but came from inside GtS as the result of another activity on this instance. That being so,
-		// we can safely just ignore this activity, since we know we've already processed it elsewhere.
-		return nil
+	receivingAccount, requestingAccount, internal := extractFromCtx(ctx)
+	if internal {
+		return nil // Already processed.
 	}
 
 	switch asType.GetTypeName() {
diff --git a/internal/federation/federatingdb/delete.go b/internal/federation/federatingdb/delete.go
index 7a8b51329..95f9be354 100644
--- a/internal/federation/federatingdb/delete.go
+++ b/internal/federation/federatingdb/delete.go
@@ -40,12 +40,9 @@ func (f *federatingDB) Delete(ctx context.Context, id *url.URL) error {
 		}...)
 	l.Debug("entering Delete")
 
-	receivingAccount, requestingAccount := extractFromCtx(ctx)
-	if receivingAccount == nil {
-		// If the receiving account wasn't set on the context, that means this request didn't pass
-		// through the API, but came from inside GtS as the result of another activity on this instance. That being so,
-		// we can safely just ignore this activity, since we know we've already processed it elsewhere.
-		return nil
+	receivingAccount, requestingAccount, internal := extractFromCtx(ctx)
+	if internal {
+		return nil // Already processed.
 	}
 
 	// in a delete we only get the URI, we can't know if we have a status or a profile or something else,
diff --git a/internal/federation/federatingdb/federatingdb_test.go b/internal/federation/federatingdb/federatingdb_test.go
index 6a6cb9262..6a8754519 100644
--- a/internal/federation/federatingdb/federatingdb_test.go
+++ b/internal/federation/federatingdb/federatingdb_test.go
@@ -21,9 +21,9 @@ import (
 	"context"
 
 	"github.com/stretchr/testify/suite"
-	"github.com/superseriousbusiness/gotosocial/internal/ap"
 	"github.com/superseriousbusiness/gotosocial/internal/db"
 	"github.com/superseriousbusiness/gotosocial/internal/federation/federatingdb"
+	"github.com/superseriousbusiness/gotosocial/internal/gtscontext"
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
 	"github.com/superseriousbusiness/gotosocial/internal/messages"
 	"github.com/superseriousbusiness/gotosocial/internal/state"
@@ -107,7 +107,7 @@ func (suite *FederatingDBTestSuite) TearDownTest() {
 
 func createTestContext(receivingAccount *gtsmodel.Account, requestingAccount *gtsmodel.Account) context.Context {
 	ctx := context.Background()
-	ctx = context.WithValue(ctx, ap.ContextReceivingAccount, receivingAccount)
-	ctx = context.WithValue(ctx, ap.ContextRequestingAccount, requestingAccount)
+	ctx = gtscontext.SetReceivingAccount(ctx, receivingAccount)
+	ctx = gtscontext.SetRequestingAccount(ctx, requestingAccount)
 	return ctx
 }
diff --git a/internal/federation/federatingdb/reject.go b/internal/federation/federatingdb/reject.go
index 17f0f84d0..10882db83 100644
--- a/internal/federation/federatingdb/reject.go
+++ b/internal/federation/federatingdb/reject.go
@@ -40,12 +40,9 @@ func (f *federatingDB) Reject(ctx context.Context, reject vocab.ActivityStreamsR
 		l.Debug("entering Reject")
 	}
 
-	receivingAccount, _ := extractFromCtx(ctx)
-	if receivingAccount == nil {
-		// If the receiving account or federator channel wasn't set on the context, that means this request didn't pass
-		// through the API, but came from inside GtS as the result of another activity on this instance. That being so,
-		// we can safely just ignore this activity, since we know we've already processed it elsewhere.
-		return nil
+	receivingAccount, _, internal := extractFromCtx(ctx)
+	if internal {
+		return nil // Already processed.
 	}
 
 	rejectObject := reject.GetActivityStreamsObject()
diff --git a/internal/federation/federatingdb/undo.go b/internal/federation/federatingdb/undo.go
index c17bd2e90..704ca5502 100644
--- a/internal/federation/federatingdb/undo.go
+++ b/internal/federation/federatingdb/undo.go
@@ -43,12 +43,9 @@ func (f *federatingDB) Undo(ctx context.Context, undo vocab.ActivityStreamsUndo)
 		l.Debug("entering Undo")
 	}
 
-	receivingAccount, _ := extractFromCtx(ctx)
-	if receivingAccount == nil {
-		// If the receiving account wasn't set on the context, that means this request didn't pass
-		// through the API, but came from inside GtS as the result of another activity on this instance. That being so,
-		// we can safely just ignore this activity, since we know we've already processed it elsewhere.
-		return nil
+	receivingAccount, _, internal := extractFromCtx(ctx)
+	if internal {
+		return nil // Already processed.
 	}
 
 	undoObject := undo.GetActivityStreamsObject()
diff --git a/internal/federation/federatingdb/update.go b/internal/federation/federatingdb/update.go
index 5c8785c08..aad386085 100644
--- a/internal/federation/federatingdb/update.go
+++ b/internal/federation/federatingdb/update.go
@@ -52,28 +52,14 @@ func (f *federatingDB) Update(ctx context.Context, asType vocab.Type) error {
 		l.Debug("entering Update")
 	}
 
-	receivingAccount, _ := extractFromCtx(ctx)
-	if receivingAccount == nil {
-		// If the receiving account wasn't set on the context, that means
-		// this request didn't pass through the API, but came from inside
-		// GtS as the result of another activity on this instance. As such,
-		// we must have already processed it in order to reach this stage.
-		return nil
-	}
-
-	requestingAcctI := ctx.Value(ap.ContextRequestingAccount)
-	if requestingAcctI == nil {
-		return errors.New("Update: requesting account wasn't set on context")
-	}
-
-	requestingAcct, ok := requestingAcctI.(*gtsmodel.Account)
-	if !ok {
-		return errors.New("Update: requesting account was set on context but couldn't be parsed")
+	receivingAccount, requestingAccount, internal := extractFromCtx(ctx)
+	if internal {
+		return nil // Already processed.
 	}
 
 	switch asType.GetTypeName() {
 	case ap.ActorApplication, ap.ActorGroup, ap.ActorOrganization, ap.ActorPerson, ap.ActorService:
-		return f.updateAccountable(ctx, receivingAccount, requestingAcct, asType)
+		return f.updateAccountable(ctx, receivingAccount, requestingAccount, asType)
 	}
 
 	return nil
diff --git a/internal/federation/federatingdb/util.go b/internal/federation/federatingdb/util.go
index 8e9f67c59..d46451e21 100644
--- a/internal/federation/federatingdb/util.go
+++ b/internal/federation/federatingdb/util.go
@@ -30,6 +30,7 @@ import (
 	"github.com/superseriousbusiness/gotosocial/internal/ap"
 	"github.com/superseriousbusiness/gotosocial/internal/config"
 	"github.com/superseriousbusiness/gotosocial/internal/db"
+	"github.com/superseriousbusiness/gotosocial/internal/gtscontext"
 	"github.com/superseriousbusiness/gotosocial/internal/gtsmodel"
 	"github.com/superseriousbusiness/gotosocial/internal/id"
 	"github.com/superseriousbusiness/gotosocial/internal/log"
@@ -296,30 +297,23 @@ func (f *federatingDB) collectIRIs(ctx context.Context, iris []*url.URL) (vocab.
 	return collection, nil
 }
 
-// extractFromCtx extracts some useful values from a context passed into the federatingDB via the API:
-//   - The target account that owns the inbox or URI being interacted with.
-//   - The requesting account that posted to the inbox.
-//   - A channel that messages for the processor can be placed into.
+// extractFromCtx extracts some useful values from a context passed into the federatingDB:
 //
-// If a value is not present, nil will be returned for it. It's up to the caller to check this and respond appropriately.
-func extractFromCtx(ctx context.Context) (receivingAccount, requestingAccount *gtsmodel.Account) {
-	receivingAccountI := ctx.Value(ap.ContextReceivingAccount)
-	if receivingAccountI != nil {
-		var ok bool
-		receivingAccount, ok = receivingAccountI.(*gtsmodel.Account)
-		if !ok {
-			log.Panicf(ctx, "context entry with key %s could not be asserted to *gtsmodel.Account", ap.ContextReceivingAccount)
-		}
-	}
+//   - The account that owns the inbox or URI being interacted with.
+//   - The account that POSTed a request to the inbox.
+//   - Whether this is an internal request (one originating not from
+//     the API but from inside the instance).
+//
+// If the request is internal, the caller can assume that the activity has
+// already been processed elsewhere, and should return with no further action.
+func extractFromCtx(ctx context.Context) (receivingAccount *gtsmodel.Account, requestingAccount *gtsmodel.Account, internal bool) {
+	receivingAccount = gtscontext.ReceivingAccount(ctx)
+	requestingAccount = gtscontext.RequestingAccount(ctx)
 
-	requestingAcctI := ctx.Value(ap.ContextRequestingAccount)
-	if requestingAcctI != nil {
-		var ok bool
-		requestingAccount, ok = requestingAcctI.(*gtsmodel.Account)
-		if !ok {
-			log.Panicf(ctx, "context entry with key %s could not be asserted to *gtsmodel.Account", ap.ContextRequestingAccount)
-		}
-	}
+	// If the receiving account wasn't set on the context, that
+	// means this request didn't pass through the API, but
+	// came from inside GtS as the result of a local activity.
+	internal = receivingAccount == nil
 
 	return
 }
@@ -329,9 +323,11 @@ func marshalItem(item vocab.Type) (string, error) {
 	if err != nil {
 		return "", err
 	}
+
 	b, err := json.Marshal(m)
 	if err != nil {
 		return "", err
 	}
+
 	return string(b), nil
 }