refactor(exchange): readability + maintainability sweep

.mockery.yaml

@@ -15,3 +15,10 @@ packages:
           pkgname: authkitmocks
           dir: mocks/authkit
           filename: principal_action_resolver.go
+      PrincipalResolver:
+        config:
+          template: testify
+          structname: PrincipalResolver
+          pkgname: authkitmocks
+          dir: mocks/authkit
+          filename: principal_resolver.go

exchange/apitoken.go

@@ -3,12 +3,50 @@ package exchange
 import (
 	"context"
 	"errors"
-	"fmt"
 
 	"github.com/meigma/authkit"
 	"github.com/meigma/authkit/access/jwt"
+	"github.com/meigma/authkit/proof/apikey"
 )
 
+// APITokenVerifier verifies opaque API tokens and returns their authenticated metadata.
+// Implementations must return authkit.ErrUnauthenticated (directly or wrapped) when plaintext is
+// not a currently valid token; other errors are treated as internal failures.
+type APITokenVerifier interface {
+	// VerifyAPIToken verifies plaintext and returns its authenticated token metadata.
+	VerifyAPIToken(ctx context.Context, plaintext string) (apikey.VerifiedToken, error)
+}
+
+// APITokenOptions configures an APITokenExchanger.
+type APITokenOptions struct {
+	// APITokens verifies opaque API tokens.
+	APITokens APITokenVerifier
+
+	// Principals loads principals authenticated by API tokens.
+	Principals authkit.PrincipalFinder
+
+	// AccessTokens issues authkit access JWTs.
+	AccessTokens AccessTokenIssuer
+}
+
+// APITokenRequest describes an API-token exchange request.
+type APITokenRequest struct {
+	// Plaintext is the opaque API token presented for exchange.
+	Plaintext string
+}
+
+// APITokenResult describes a completed API-token exchange.
+type APITokenResult struct {
+	// APIToken is the verified opaque API token metadata.
+	APIToken apikey.VerifiedToken
+
+	// Principal is the principal authenticated by APIToken.
+	Principal authkit.Principal
+
+	// AccessToken is the authkit access JWT issued for Principal.
+	AccessToken jwt.IssuedToken
+}
+
 // APITokenExchanger exchanges opaque API tokens for authkit access JWTs.
 type APITokenExchanger struct {
 	apiTokens    APITokenVerifier
@@ -47,6 +85,8 @@ func (e *APITokenExchanger) Exchange(ctx context.Context, req APITokenRequest) (
 	}
 
 	principal, err := e.principals.FindPrincipal(ctx, apiToken.PrincipalID)
+	// Missing principal collapses to the generic unauthenticated response so the API does not
+	// leak whether a specific PrincipalID exists.
 	if errors.Is(err, authkit.ErrPrincipalNotFound) {
 		return APITokenResult{}, unauthenticated("principal not found")
 	}
@@ -67,22 +107,3 @@ func (e *APITokenExchanger) Exchange(ctx context.Context, req APITokenRequest) (
 		AccessToken: accessToken,
 	}, nil
 }
-
-func exchangeError(operation string, err error) error {
-	if isContextError(err) {
-		return err
-	}
-	if errors.Is(err, authkit.ErrUnauthenticated) || errors.Is(err, authkit.ErrUnresolvedIdentity) {
-		return err
-	}
-
-	return fmt.Errorf("%w: %s: %w", authkit.ErrInternal, operation, err)
-}
-
-func unauthenticated(reason string) error {
-	return fmt.Errorf("%w: %s", authkit.ErrUnauthenticated, reason)
-}
-
-func isContextError(err error) bool {
-	return errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded)
-}

exchange/apitoken_test.go

@@ -7,24 +7,19 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 
 	"github.com/meigma/authkit"
-	"github.com/meigma/authkit/access/jwt"
 	"github.com/meigma/authkit/exchange"
-	"github.com/meigma/authkit/internal/authtest"
+	authkitmocks "github.com/meigma/authkit/mocks/authkit"
 	"github.com/meigma/authkit/proof/apikey"
 	"github.com/meigma/authkit/store/memory"
 )
 
-const (
-	testPrincipalID = "principal_1"
-	testTokenID     = "access-token-123"
-)
-
 func TestNewAPITokenExchangerValidatesDependencies(t *testing.T) {
 	apiTokens := fakeAPITokenVerifier{}
-	principals := fakePrincipalFinder{}
+	principals := authkitmocks.NewPrincipalFinder(t)
 	accessTokens := fakeAccessTokenIssuer{}
 
 	tests := []struct {
@@ -104,40 +99,11 @@ func TestAPITokenExchangerExchangesTokenForAccessJWT(t *testing.T) {
 	assert.Equal(t, principal.ID, verified.PrincipalID)
 }
 
-func TestAPITokenExchangerDoesNotRequireIdentityLink(t *testing.T) {
-	ctx := context.Background()
-	store := memory.NewStore()
-	principal, err := store.CreatePrincipal(ctx, authkit.CreatePrincipalRequest{
-		Kind:        authkit.PrincipalKindService,
-		DisplayName: "notes service",
-	})
-	require.NoError(t, err)
-	apiTokens, err := apikey.NewService(store, apikey.WithClock(fixedTime))
-	require.NoError(t, err)
-	apiToken, err := apiTokens.IssueToken(ctx, apikey.IssueRequest{
-		PrincipalID: principal.ID,
-		Name:        "bootstrap token",
-		ExpiresAt:   fixedTime().Add(time.Hour),
-	})
-	require.NoError(t, err)
-	accessTokens, _ := newAccessJWTIssuerAndVerifier(t)
-	exchanger := newAPITokenExchanger(t, exchange.APITokenOptions{
-		APITokens:    apiTokens,
-		Principals:   store,
-		AccessTokens: accessTokens,
-	})
-
-	result, err := exchanger.Exchange(ctx, exchange.APITokenRequest{
-		Plaintext: apiToken.Plaintext,
-	})
-
-	require.NoError(t, err)
-	assert.Equal(t, principal.ID, result.Principal.ID)
-}
-
 func TestAPITokenExchangerRejectsMissingPrincipal(t *testing.T) {
-	ctx := context.Background()
-	store := memory.NewStore()
+	principals := authkitmocks.NewPrincipalFinder(t)
+	principals.EXPECT().
+		FindPrincipal(mock.Anything, "missing").
+		Return(authkit.Principal{}, authkit.ErrPrincipalNotFound)
 	accessTokens, _ := newAccessJWTIssuerAndVerifier(t)
 	exchanger := newAPITokenExchanger(t, exchange.APITokenOptions{
 		APITokens: fakeAPITokenVerifier{
@@ -147,11 +113,11 @@ func TestAPITokenExchangerRejectsMissingPrincipal(t *testing.T) {
 				ExpiresAt:   fixedTime().Add(time.Hour),
 			},
 		},
-		Principals:   store,
+		Principals:   principals,
 		AccessTokens: accessTokens,
 	})
 
-	result, err := exchanger.Exchange(ctx, exchange.APITokenRequest{
+	result, err := exchanger.Exchange(context.Background(), exchange.APITokenRequest{
 		Plaintext: "ak_token_secret",
 	})
 
@@ -193,43 +159,47 @@ func TestAPITokenExchangerWrapsInternalFailures(t *testing.T) {
 	}
 
 	tests := []struct {
-		name string
-		opts exchange.APITokenOptions
-		want error
+		name      string
+		setupOpts func(t *testing.T) exchange.APITokenOptions
+		want      error
 	}{
 		{
 			name: "principal finder failure",
-			opts: exchange.APITokenOptions{
-				APITokens: fakeAPITokenVerifier{
-					token: apiToken,
-				},
-				Principals: fakePrincipalFinder{
-					err: storeErr,
-				},
-				AccessTokens: fakeAccessTokenIssuer{},
+			setupOpts: func(t *testing.T) exchange.APITokenOptions {
+				t.Helper()
+				principals := authkitmocks.NewPrincipalFinder(t)
+				principals.EXPECT().
+					FindPrincipal(mock.Anything, testPrincipalID).
+					Return(authkit.Principal{}, storeErr)
+				return exchange.APITokenOptions{
+					APITokens:    fakeAPITokenVerifier{token: apiToken},
+					Principals:   principals,
+					AccessTokens: fakeAccessTokenIssuer{},
+				}
 			},
 			want: storeErr,
 		},
 		{
 			name: "access token issuer failure",
-			opts: exchange.APITokenOptions{
-				APITokens: fakeAPITokenVerifier{
-					token: apiToken,
-				},
-				Principals: fakePrincipalFinder{
-					principal: principal,
-				},
-				AccessTokens: fakeAccessTokenIssuer{
-					err: issuerErr,
-				},
+			setupOpts: func(t *testing.T) exchange.APITokenOptions {
+				t.Helper()
+				principals := authkitmocks.NewPrincipalFinder(t)
+				principals.EXPECT().
+					FindPrincipal(mock.Anything, testPrincipalID).
+					Return(principal, nil)
+				return exchange.APITokenOptions{
+					APITokens:    fakeAPITokenVerifier{token: apiToken},
+					Principals:   principals,
+					AccessTokens: fakeAccessTokenIssuer{err: issuerErr},
+				}
 			},
 			want: issuerErr,
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			exchanger := newAPITokenExchanger(t, tt.opts)
+			exchanger := newAPITokenExchanger(t, tt.setupOpts(t))
 
 			result, err := exchanger.Exchange(context.Background(), exchange.APITokenRequest{
 				Plaintext: "ak_token_secret",
@@ -245,7 +215,7 @@ func TestAPITokenExchangerWrapsInternalFailures(t *testing.T) {
 func TestAPITokenExchangerPassesThroughContextErrors(t *testing.T) {
 	exchanger := newAPITokenExchanger(t, exchange.APITokenOptions{
 		APITokens:    fakeAPITokenVerifier{},
-		Principals:   fakePrincipalFinder{},
+		Principals:   authkitmocks.NewPrincipalFinder(t),
 		AccessTokens: fakeAccessTokenIssuer{},
 	})
 	ctx, cancel := context.WithCancel(context.Background())
@@ -259,63 +229,3 @@ func TestAPITokenExchangerPassesThroughContextErrors(t *testing.T) {
 	require.NotErrorIs(t, err, authkit.ErrInternal)
 	assert.Empty(t, result)
 }
-
-func newAPITokenExchanger(t *testing.T, opts exchange.APITokenOptions) *exchange.APITokenExchanger {
-	t.Helper()
-
-	exchanger, err := exchange.NewAPITokenExchanger(opts)
-	require.NoError(t, err)
-
-	return exchanger
-}
-
-func newAccessJWTIssuerAndVerifier(t *testing.T) (*jwt.Issuer, *jwt.Verifier) {
-	t.Helper()
-
-	return authtest.NewAccessJWTIssuerAndVerifier(
-		t,
-		authtest.WithAccessJWTTokenID(func() (string, error) {
-			return testTokenID, nil
-		}),
-	)
-}
-
-type fakeAPITokenVerifier struct {
-	token apikey.VerifiedToken
-	err   error
-}
-
-func (f fakeAPITokenVerifier) VerifyAPIToken(
-	context.Context,
-	string,
-) (apikey.VerifiedToken, error) {
-	return f.token, f.err
-}
-
-type fakePrincipalFinder struct {
-	principal authkit.Principal
-	err       error
-}
-
-func (f fakePrincipalFinder) FindPrincipal(
-	context.Context,
-	string,
-) (authkit.Principal, error) {
-	return f.principal, f.err
-}
-
-type fakeAccessTokenIssuer struct {
-	token jwt.IssuedToken
-	err   error
-}
-
-func (f fakeAccessTokenIssuer) IssueToken(
-	context.Context,
-	jwt.IssueRequest,
-) (jwt.IssuedToken, error) {
-	return f.token, f.err
-}
-
-func fixedTime() time.Time {
-	return authtest.FixedTime()
-}

exchange/errors.go

@@ -0,0 +1,39 @@
+package exchange
+
+import (
+	"context"
+	"errors"
+	"fmt"
+
+	"github.com/meigma/authkit"
+)
+
+// exchangeError classifies err raised during operation. Context-cancellation errors and authkit
+// sentinel errors (ErrUnauthenticated, ErrUnresolvedIdentity) are returned unchanged so callers
+// can detect them with [errors.Is]; anything else is wrapped as ErrInternal with operation as
+// breadcrumb.
+func exchangeError(operation string, err error) error {
+	// Context errors are returned unchanged so callers can distinguish cancellation/deadline from
+	// a legitimate collaborator failure.
+	if isContextError(err) {
+		return err
+	}
+	// Authentication and identity-resolution sentinels are returned unchanged so callers can
+	// branch on them via [errors.Is]; wrapping would hide the sentinel behind ErrInternal.
+	if errors.Is(err, authkit.ErrUnauthenticated) || errors.Is(err, authkit.ErrUnresolvedIdentity) {
+		return err
+	}
+
+	return fmt.Errorf("%w: %s: %w", authkit.ErrInternal, operation, err)
+}
+
+// unauthenticated wraps reason with authkit.ErrUnauthenticated so callers can recognise the
+// failure via [errors.Is].
+func unauthenticated(reason string) error {
+	return fmt.Errorf("%w: %s", authkit.ErrUnauthenticated, reason)
+}
+
+// isContextError reports whether err is a context cancellation or deadline error.
+func isContextError(err error) bool {
+	return errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded)
+}