Skip to content

refactor(access): readability + maintainability sweep + introduce mockery#61

Merged
jmgilman merged 7 commits into
masterfrom
session-037/access-refactor
May 27, 2026
Merged

refactor(access): readability + maintainability sweep + introduce mockery#61
jmgilman merged 7 commits into
masterfrom
session-037/access-refactor

Conversation

@jmgilman
Copy link
Copy Markdown
Contributor

Summary

First slice of a multi-session refactor sweep across the codebase, targeting readability, maintainability, and architectural purity (hexagonal, strong typing, strong contractual boundaries). Scope is the two packages under access/: access/jwt/ and access/middleware/. Explicitly non-functional — no schema, port, or behavioural changes.

Highlights

access/jwt/

  • Rename IssuedToken.PlaintextIssuedToken.JWT (cascaded through consumers in exchange/, http/compose/, testkit/, authz/role/). Plaintext falsely implied "unencrypted body" in a crypto package; the field carries the signed compact JWS serialization.
  • Rename the matching VerifyToken parameter to jwt and document it as accepting a compact JWS.
  • Add godocs to every private helper across keys.go, issuer.go, verifier.go.
  • New validation.go absorbs the cross-file validateRequiredString so Issuer and Verifier share the helper from a neutral location.
  • Annotate validateProtectedHeaders as the JWS security gate, with one comment per check naming the attack class it defends against (multi-signature, token-type confusion, ambiguous kid, alg substitution, unrecognised crit per RFC 7515 §4.1.11).
  • Split the 600+ line monolithic jwt_test.go into issuer_test.go, verifier_test.go, and helpers_test.go. No tests added, removed, or reorganised.

access/middleware/

  • Compile-time assertion: var _ authkit.PrincipalAuthenticator = (*Authenticator)(nil). Makes the port relationship explicit.
  • Godocs on bearerToken and unauthenticated; inline comments on the security-relevant branches (existence-leak defence, defensive principal.ID check, RFC 7235 case-insensitive bearer scheme).
  • Test refactor: replace memory.Store with the generated authkitmocks.PrincipalFinder. Add three previously-missing rejection cases (cancelled context, nil request, internal finder error) plus a regression test for the principal.ID == "" defence.

Mockery introduction (first time in this repo)

  • Add github.com/vektra/mockery/v3 v3.7.0 as a go.mod tool dependency so go tool mockery works with no system install.
  • .mockery.yaml configures generation; today it only covers authkit.PrincipalFinder. Future passes append interfaces as their consumer tests get refactored.
  • Generated mock lands at mocks/authkit/principal_finder.go in package authkitmocks.
  • New generate Moon task wraps the invocation; CI drift-detection is a follow-up once more mocks land.

Deliberately NOT changed

  • No new port interfaces. *jwt.Verifier is the single concrete adapter; access/middleware is its single consumer. Adding an interface today would be a "might-be-useful-later" abstraction explicitly forbidden by the refactor brief.
  • validateProtectedHeaders is not split. Inline comments are the right tool for explaining each security check; splitting would scatter the gate.
  • unauthenticated helpers stay duplicated across jwt and middleware. Two three-line copies; a shared package would add API surface for trivial savings.
  • apikey.IssuedToken.Plaintext stays. That's proof/apikey/'s pass, not this one.

Test plan

  • moon run root:check --summary minimal — format, lint, build, unit tests, integration tests with Testcontainers. All green.
  • go test ./access/... — all 28 sub-tests pass (jwt + middleware).
  • go vet ./... clean across the module.
  • go doc -all github.com/meigma/authkit/access/jwt diff against master: only IssuedToken.Plaintext removed and IssuedToken.JWT added.
  • go doc -all github.com/meigma/authkit/access/middleware diff against master: empty (the interface assertion is package-private from the doc perspective).
  • Reviewer functional check: testkit pastebin login flow if anything looks suspicious in the rename cascade.

🤖 Generated with Claude Code

jmgilman and others added 7 commits May 27, 2026 13:29
"Plaintext" implied "unencrypted body" in a crypto context. The field
carries the signed compact JWS serialization, so JWT names it accurately.
Mechanical rename across jwt.IssuedToken consumers; apikey.IssuedToken
keeps its own Plaintext naming until proof/apikey's own refactor pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ity gate

- New validation.go absorbs validateRequiredString from issuer.go so Issuer and Verifier
  share the helper from a neutral location.
- Add godocs to every private helper in keys.go, issuer.go, and verifier.go.
- Annotate validateProtectedHeaders as the JWS security gate with one comment per check
  naming the attack class it defends against (multi-signature, token-type confusion,
  ambiguous kid, alg substitution, unrecognised crit per RFC 7515 §4.1.11).
- Rename VerifyToken's parameter plaintext to jwt; the value is a compact JWS, not an
  unencrypted body.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the per-domain pattern set by PRs #56-#58 on store/. jwt_test.go grew to 600+
lines mixing issuer behaviour, verifier behaviour, and a large helper surface.

- issuer_test.go: option validation and issuance behaviour.
- verifier_test.go: option validation, rejection cases, storage-backed authorization.
- helpers_test.go: shared constants, fixtures, token-construction helpers, RSA key
  factories. Helper parameter "plaintext" follows the production rename to "jwt".

No tests added, removed, or reorganised; the split is mechanical.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…principal-authenticator port

- Add a compile-time assertion that *Authenticator satisfies authkit.PrincipalAuthenticator,
  making the port relationship explicit rather than relying on duck typing.
- Document bearerToken (note RFC 7235 case-insensitive scheme) and unauthenticated.
- Annotate the ErrPrincipalNotFound branch as a security-relevant existence-leak defence
  and the post-finder principal.ID == "" branch as a defence against a misbehaving
  PrincipalFinder.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…palFinder

The go-testing skill mandates mockery at port boundaries. This pass introduces the
tooling so consumer-package refactors (starting with access/middleware, eventually
exchange/ and http/compose/) can use generated mocks instead of standing up real
storage adapters.

- Add mockery v3.7.0 as a go.mod tool dependency so generation works through
  `go tool mockery` with no extra system install.
- .mockery.yaml at the repo root drives generation; today it only configures
  authkit.PrincipalFinder. Future passes append new interfaces here.
- Generated mock lands at mocks/authkit/principal_finder.go in package authkitmocks.
- Add a `generate` task to moon.yml. CI is not yet wired to fail on drift; that's
  a follow-up once more mocks land and the policy is worth enforcing.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…on cases

- Replace memory.Store with the generated authkitmocks.PrincipalFinder so the test
  asserts the Authenticator's contract with its port, not the store's behaviour.
- Collapse plumbing into a testContext fixture per the go-testing skill's pattern.
- Add three rejection cases the prior suite missed:
  * cancelled context returns context.Canceled untouched
  * nil request returns ErrUnauthenticated
  * principal finder returning a generic error wraps with ErrInternal
- Add coverage for the principal.ID == "" defensive branch in AuthenticatePrincipal.

Issuer and Verifier remain real concrete adapters (they are pure domain logic with no
port boundary). Per-case "wrong issuer / wrong audience / expired" tokens reuse the
shared signing key so the rejection happens at iss/aud/exp validation, not signature.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
godoclint requires [errors.Is] form so pkg.go.dev renders a hyperlink.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant