Skip to content

refactor(authz): readability + maintainability sweep#62

Merged
jmgilman merged 4 commits into
masterfrom
session-037/authz-refactor
May 27, 2026
Merged

refactor(authz): readability + maintainability sweep#62
jmgilman merged 4 commits into
masterfrom
session-037/authz-refactor

Conversation

@jmgilman
Copy link
Copy Markdown
Contributor

Summary

Second slice of the multi-session refactor sweep. PR #61 brought access/ up to the bar; this PR applies the same 10 criteria to authz/role/ and authz/casbin/. Same non-goals: bugs, correctness, performance.

Highlights

authz/role/

  • Compile-time assertion: var _ authkit.Authorizer = (*Authorizer)(nil).
  • Tightened Authorizer godoc into a contract statement; documented the deniedReason constant.
  • Inline comment on the post-resolver ctx.Err() re-check explaining the cancellation-mid-resolution rationale.
  • Replaced the hand-rolled fakeActionResolver with the generated authkitmocks.PrincipalActionResolver. Each table case now configures its mock expectations explicitly. The TestAuthorizerAllowsHTTPMiddlewareThroughAccessJWT end-to-end test still uses memory.Store — the right shape for an integration test (matches access pass's TestVerifiedTokenUsesStorageBackedAuthorization).

authz/casbin/

  • Compile-time port assertion (same shape as role).
  • Reason-string cleanup: "casbin policy denied""policy denied". The adapter's implementation name does not belong in the API response; grep confirms no consumer asserts on the literal.
  • Godocs on every private symbol (deniedReason, resourceObject, options, defaultOptions); expanded WithRequestBuilder godoc to call out the nil-leaves-default behaviour.
  • Tightened Can godoc.
  • Inline annotations on the security-relevant branches: fail-closed input validation in DefaultRequestBuilder, the post-projection ctx.Err() re-check, and the type-only-resource form in resourceObject.
  • Test file split: 433-line authorizer_test.goauthorizer_test.go (Can + NewAuthorizer cases), request_builder_test.go (DefaultRequestBuilder cases), helpers_test.go (test fixtures + real Casbin enforcer helper). Mechanical lift; every test function preserved verbatim.

Mockery

  • Appended authkit.PrincipalActionResolver to .mockery.yaml. The pattern is now: one entry per root-package port that consumer tests need to mock. Adapter-local ports (e.g. casbin.Enforcer) stay hand-rolled.
  • Generated mocks/authkit/principal_action_resolver.go.

Deliberately NOT changed

  • No new ports in either package. casbin.Enforcer and casbin.RequestBuilder already serve as extension points; promoting them to a ports.go file would mirror nothing else in the codebase.
  • No mock for casbin.Enforcer. Adapter-local port for a third-party library; the hand-rolled testEnforcer is small and clear.
  • No merge of casbin/options.go into authorizer.go. Five other packages in the repo use a dedicated options.go (proof/oidc/, proof/apikey/, proof/passkey/session/, http/auth/, this one). Consistency wins.
  • No deduplication of TestDefaultRequestBuilderValidatesRequiredInputs vs TestAuthorizerCanReturnsDefaultProjectionErrors. They cover the same input rules at different layers; the second test also asserts the enforcer is never called — defense in depth for a security-critical path.
  • No rename of role.deniedReason ("action not granted"). Already neutral and accurate.

Test plan

  • moon run root:check --summary minimal — format, lint, build, unit, Testcontainers integration. All green.
  • go test -v ./authz/... — all sub-tests pass (role + casbin), same case count as pre-split for casbin.
  • go tool mockery is idempotent; second run produces no diff.
  • Public-surface diff: only godoc text changes for role.Authorizer.Can and casbin.Authorizer.Can, plus one observable string change (Decision.Reason from "casbin policy denied""policy denied").

🤖 Generated with Claude Code

jmgilman and others added 4 commits May 27, 2026 13:58
Extends the mockery config introduced in PR #61 to cover the second root-package
port consumed by an adapter under refactor (authz/role). The pattern is now:
one entry per root interface that consumer-package tests need to mock.

Adapter-local ports (e.g. casbin.Enforcer) stay hand-rolled; mockery covers root
ports only.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…s, and mock the resolver

- Add compile-time assertion that *Authorizer satisfies authkit.Authorizer.
- Tighten the Authorizer godoc into a contract statement ("decides authorization
  checks by matching check.Action against the principal's effective actions").
- Document the deniedReason constant.
- Annotate the post-resolver ctx.Err() check explaining why we re-check after
  the external call (cancellation surfaces as cancellation, not stale decision).
- Replace the hand-rolled fakeActionResolver with the generated
  authkitmocks.PrincipalActionResolver. Each table case now configures its mock
  expectations explicitly; the previous test's tt.resolver.requests assertion is
  subsumed by mockery's AssertExpectations on Cleanup.

The TestAuthorizerAllowsHTTPMiddlewareThroughAccessJWT integration test still
uses memory.Store as the resolver — that's the right shape for an end-to-end
test (matching access pass's TestVerifiedTokenUsesStorageBackedAuthorization).

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

- Add compile-time assertion that *Authorizer satisfies authkit.Authorizer.
- Change deniedReason from "casbin policy denied" to "policy denied". The
  adapter's implementation name does not belong in the API response; consumers
  asserting on Decision.Reason should not learn the backend through a string
  literal. (No code in the repo grepped for the old literal.)
- Document deniedReason, resourceObject, options, defaultOptions; expand the
  WithRequestBuilder godoc to call out the nil-leaves-default behaviour.
- Tighten Can's godoc into a contract statement.
- Annotate fail-closed input validation in DefaultRequestBuilder; comment why
  the post-projection ctx.Err() re-check exists; explain the type-only resource
  form in resourceObject.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mirrors the per-type split applied to access/jwt in PR #61. The monolithic 433-line
file mixed three concerns:

- authorizer_test.go: NewAuthorizer validation + Can() behaviour (allow, deny,
  custom builder, projection/enforcer/context errors, real Casbin integration).
- request_builder_test.go: DefaultRequestBuilder projection + validation.
- helpers_test.go: test fixtures (testPrincipal, testCheck, testResource,
  testEnforcer + allow/deny helpers, newAuthorizer, newCasbinEnforcer).

Mechanical lift: every test function moves verbatim with its imports. No tests
added, removed, or reorganised internally.

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