refactor(http): readability + maintainability sweep

.mockery.yaml

@@ -22,3 +22,17 @@ packages:
           pkgname: authkitmocks
           dir: mocks/authkit
           filename: principal_resolver.go
+      PrincipalAuthenticator:
+        config:
+          template: testify
+          structname: PrincipalAuthenticator
+          pkgname: authkitmocks
+          dir: mocks/authkit
+          filename: principal_authenticator.go
+      Authorizer:
+        config:
+          template: testify
+          structname: Authorizer
+          pkgname: authkitmocks
+          dir: mocks/authkit
+          filename: authorizer.go

http/auth/helpers_test.go

@@ -0,0 +1,128 @@
+package auth_test
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/mock"
+	"github.com/stretchr/testify/require"
+
+	"github.com/meigma/authkit"
+	"github.com/meigma/authkit/http/auth"
+	authkitmocks "github.com/meigma/authkit/mocks/authkit"
+)
+
+const testAuthenticatorName = "test"
+
+func newMiddleware(t *testing.T, pipeline *authkit.Pipeline) *auth.Middleware {
+	t.Helper()
+
+	middleware, err := auth.NewMiddleware(pipeline)
+	require.NoError(t, err)
+
+	return middleware
+}
+
+// newTestPipelineWithAuthorizer composes a pipeline with an allow-everything principal
+// authenticator and the supplied authorizer.
+func newTestPipelineWithAuthorizer(t *testing.T, authorizer authkit.Authorizer) *authkit.Pipeline {
+	t.Helper()
+
+	return newTestPipelineWithPrincipalAuthenticator(t, newAllowPrincipalAuthenticator(t), authorizer)
+}
+
+// newTestPipelineWithPrincipalAuthenticator composes a pipeline from the supplied authenticator
+// and authorizer.
+func newTestPipelineWithPrincipalAuthenticator(
+	t *testing.T,
+	authenticator authkit.PrincipalAuthenticator,
+	authorizer authkit.Authorizer,
+) *authkit.Pipeline {
+	t.Helper()
+
+	pipeline, err := authkit.NewPipeline(authkit.PipelineOptions{
+		PrincipalAuthenticators: []authkit.PrincipalAuthenticator{authenticator},
+		Authorizer:              authorizer,
+	})
+	require.NoError(t, err)
+
+	return pipeline
+}
+
+// newAllowPrincipalAuthenticator returns a mock authenticator that accepts every request and
+// returns testPrincipal().
+func newAllowPrincipalAuthenticator(t *testing.T) *authkitmocks.PrincipalAuthenticator {
+	t.Helper()
+
+	authenticator := authkitmocks.NewPrincipalAuthenticator(t)
+	authenticator.EXPECT().Name().Return(testAuthenticatorName)
+	authenticator.EXPECT().
+		AuthenticatePrincipal(mock.Anything, mock.Anything).
+		Return(&authkit.PrincipalAuthentication{Principal: testPrincipal()}, nil)
+
+	return authenticator
+}
+
+// newDenyPrincipalAuthenticator returns a mock authenticator that wraps ErrUnauthenticated. The
+// pipeline does not call Name() on the ErrUnauthenticated path, so Name() is not expected.
+func newDenyPrincipalAuthenticator(t *testing.T) *authkitmocks.PrincipalAuthenticator {
+	t.Helper()
+
+	authenticator := authkitmocks.NewPrincipalAuthenticator(t)
+	authenticator.EXPECT().
+		AuthenticatePrincipal(mock.Anything, mock.Anything).
+		Return(nil, fmt.Errorf("%w: credential missing", authkit.ErrUnauthenticated))
+
+	return authenticator
+}
+
+// newFailingPrincipalAuthenticator returns a mock authenticator whose AuthenticatePrincipal call
+// returns err. The pipeline wraps the failure with Name() in the ErrInternal message, so Name()
+// is expected to be called.
+func newFailingPrincipalAuthenticator(t *testing.T, err error) *authkitmocks.PrincipalAuthenticator {
+	t.Helper()
+
+	authenticator := authkitmocks.NewPrincipalAuthenticator(t)
+	authenticator.EXPECT().Name().Return(testAuthenticatorName)
+	authenticator.EXPECT().
+		AuthenticatePrincipal(mock.Anything, mock.Anything).
+		Return(nil, err)
+
+	return authenticator
+}
+
+func assertAuthenticationContext(ctx context.Context, t *testing.T) {
+	t.Helper()
+
+	authentication, ok := auth.AuthenticationFromContext(ctx)
+	require.True(t, ok)
+	assert.Equal(t, testAuthentication(), authentication)
+
+	principal, ok := auth.PrincipalFromContext(ctx)
+	require.True(t, ok)
+	assert.Equal(t, testPrincipal(), principal)
+}
+
+func testAuthentication() authkit.Authentication {
+	return authkit.Authentication{
+		AuthenticatorName: testAuthenticatorName,
+		Principal:         testPrincipal(),
+	}
+}
+
+func testPrincipal() authkit.Principal {
+	return authkit.Principal{
+		ID:          "principal_1",
+		Kind:        authkit.PrincipalKindUser,
+		DisplayName: "Ada Lovelace",
+	}
+}
+
+func testResource(id string) authkit.Resource {
+	return authkit.Resource{
+		Type: "note",
+		ID:   id,
+	}
+}

http/auth/middleware.go

@@ -108,6 +108,8 @@ func (m *Middleware) RequireAuthorization(
 			}
 
 			authenticatedReq := req.WithContext(contextWithAuthentication(req.Context(), authentication))
+			// A nil extractor is a caller-construction bug, not an authorization decision; report
+			// as ErrInternal so the renderer maps it to 500 rather than 403.
 			if extract == nil {
 				m.renderer(
 					w,
@@ -119,6 +121,8 @@ func (m *Middleware) RequireAuthorization(
 			}
 
 			authorizationRequest, err := extract(authenticatedReq)
+			// Extractor failures are caller-side bugs (e.g. malformed routing data) rather than
+			// authorization denials; wrap as ErrInternal to keep the 401/403 envelope honest.
 			if err != nil {
 				m.renderer(
 					w,