refactor(http): readability + maintainability sweep#64
Merged
Conversation
The Spec interface had exactly one production implementation (accessJWTAuthenticatorSpec) — a premature port by the rule established in PRs #61 and #62 (no port without a second implementation). Replace with a function type so AccessJWT returns a closure and the accessJWTAuthenticatorSpec struct + BuildPrincipalAuthenticator method disappear. buildPrincipalAuthenticators now calls spec() instead of spec.BuildPrincipalAuthenticator(). Consumers (testkit/internal/authflow/runtime.go) compile unchanged: []compose.PrincipalAuthenticatorSpec{compose.AccessJWT(...)} works identically whether PrincipalAuthenticatorSpec is an interface or a function type. The test fixture principalAuthenticatorSpecFunc, which existed solely to wrap a func into the interface, is removed; tests now pass inline closures.
Godocs: - http/auth/options.go: options (private struct) and defaultOptions (private function), matching the PR #62 bar. - http/compose/http.go: buildPrincipalAuthenticators (private function). Inline comments — match the access/middleware/authenticator.go:76-88 style. Each comment names what the check defends against and why the error envelope is what it is, not what the code does: - http/auth/middleware.go::RequireAuthorization: annotate the extract == nil branch and the wrapped extractor-failure branch. Both report ErrInternal because they are caller-side construction bugs (nil extractor) or caller-supplied function bugs (extraction error), not authorization denials. ErrInternal maps to 500 in the default renderer; ErrUnauthorized maps to 403. Choosing one over the other affects the public API contract; the choice is worth spelling out at the call site. - http/compose/http.go::buildPrincipalAuthenticators: annotate the two nil branches (nil spec, nil-authenticator-from-non-erroring-spec) as contract violations that abort composition. No new comments where the code already reads obviously (defaultErrorRenderer status mapping, empty-struct context key, per-helper nil-request guards in http/facts/facts.go). Package godocs already cover those.
Mechanical lift only — no test logic changes. http/auth/helpers_test.go: newMiddleware, newTestPipeline, newTestPipelineWithAuthorizer, newTestPipelineWithPrincipalAuthenticator, newTestPipelineWithOptions, assertAuthenticationContext, testAuthentication, testPrincipal, testResource. http/compose/helpers_test.go: newTestPrincipalAuthenticator, requestWithBearer, newAccessJWTIssuerAndVerifier. The hand-rolled testPrincipalAuthenticator, testAuthorizer, and the allow/deny/failingPrincipalAuthenticator + allowAuthorizer constructors intentionally stay in middleware_test.go and http_test.go for this commit. They get replaced by mockery in the next commit and the lift would be churn. Matches the helpers_test.go precedent from PRs #61 (access/jwt) and #63 (exchange).
Add authkit.PrincipalAuthenticator and authkit.Authorizer to .mockery.yaml. The generated mocks land at mocks/authkit/principal_authenticator.go and mocks/authkit/authorizer.go in package authkitmocks. Both are root-package ports that http/auth and http/compose tests faked by hand — twice over. http/auth/middleware_test.go and http/auth/helpers_test.go: - Delete hand-rolled testPrincipalAuthenticator and testAuthorizer types. - Delete allow/deny/failingPrincipalAuthenticator and allowAuthorizer constructor helpers. - Replace per-test with authkitmocks.PrincipalAuthenticator and authkitmocks.Authorizer; keep typed helpers (newAllowPrincipalAuthenticator, newDenyPrincipalAuthenticator, newFailingPrincipalAuthenticator) in helpers_test.go so the Name()/AuthenticatePrincipal expectation patterns stay in one place. - Tests whose short-circuit path never reaches the authorizer now pass an unused authkitmocks.NewAuthorizer(t) (no expectations set); a stray Can() call would panic the test. http/compose/http_test.go and http/compose/helpers_test.go: - Delete hand-rolled testPrincipalAuthenticator and testAuthorizer. - Delete newTestPrincipalAuthenticator factory. - Migrate the five tests that drove behaviour through the hand-rolled fakes to authkitmocks; the two AccessJWT integration tests keep their real-implementation chain and gain an unused authkitmocks.NewAuthorizer for the slot the pipeline does not exercise. Matches the access/middleware (PR #61), authz/role (PR #62), and exchange (PR #63) migration shape.
golangci-lint's golines formatter (configured in .golangci.yml) wraps a few long lines that the mockery migration introduced. Pure formatting; no logic change.
This was referenced May 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fourth pass of the multi-session refactor sweep (after
access/PR #61,authz/PR #62,exchange/PR #63). Same 10-criteria bar applied tohttp/{auth,facts,compose}. Non-goals: bugs, correctness, performance.The headline architectural fix:
compose.PrincipalAuthenticatorSpecis demoted from interface to function type — it had exactly one production implementation (accessJWTAuthenticatorSpec), a premature port by the rule established in earlier passes. Consumers (testkit/) compile unchanged because the call-site literal is identical either way.Changes per commit
refactor(compose): demote PrincipalAuthenticatorSpec to a function type—accessJWTAuthenticatorSpecand its method disappear;AccessJWTreturns a closure;buildPrincipalAuthenticatorscallsspec()instead ofspec.BuildPrincipalAuthenticator(). Test fixtureprincipalAuthenticatorSpecFuncdeleted (it existed solely to wrap a func into the old interface).refactor(http): add godocs and security/defensive inline comments— godocs onhttp/auth/options.go(options,defaultOptions) andhttp/compose/http.go(buildPrincipalAuthenticators); inline comments onRequireAuthorization's twoErrInternalbranches (nil extractor and extractor failure are caller-side contract violations, not authorization denials) andbuildPrincipalAuthenticators' two nil checks. Matchesaccess/middleware/authenticator.gostyle.test(http): split shared helpers into helpers_test.go—http/auth/helpers_test.goandhttp/compose/helpers_test.gocreated with the shared constants, helpers, and the fakes that will survive commit 4. Mechanical lift only.test(http): migrate root-port fakes to mockery— addauthkit.PrincipalAuthenticatorandauthkit.Authorizerto.mockery.yaml, regenerate, swap hand-rolled fakes forauthkitmocks.PrincipalAuthenticator/authkitmocks.Authorizer. Tests whose short-circuit path never reaches the authorizer use an unusedauthkitmocks.NewAuthorizer(t)(no expectations set) so a strayCan()would panic the test.chore(http): apply golines wrapping to long test lines—golangci-lint fmt(golines formatter) wrapped a few long lines the mockery migration introduced.Test plan
moon run root:check --summary minimal— format, lint, build, unit, Testcontainers integration. All green.go test ./http/... -count=1 -v— all 18 tests pass; one redundant test was already absent (none dropped this pass).PrincipalAuthenticatorSpecis now a function type. Consumer source (testkit/internal/{authflow,httpui}) compiles unchanged because[]compose.PrincipalAuthenticatorSpec{compose.AccessJWT(...)}works either way.go list -f '{{.Name}}' ./http/{auth,facts,compose}— all three packages list cleanly.Deliberately deferred
http/factsquery/queryKey helpers — feature gap, not a readability defect; the package is opt-in and zero-consumer..mockery.yaml— still unwired. Worth a dedicated task once a few more entries land.🤖 Generated with Claude Code