Skip to content

refactor(token): collapse token provider to concrete type#235

Merged
appleboy merged 1 commit into
mainfrom
refactor/concrete-token-provider
Jun 2, 2026
Merged

refactor(token): collapse token provider to concrete type#235
appleboy merged 1 commit into
mainfrom
refactor/concrete-token-provider

Conversation

@appleboy

@appleboy appleboy commented Jun 2, 2026

Copy link
Copy Markdown
Member

Summary

Removes the core.TokenProvider / core.IDTokenProvider interface abstraction and collapses every consumer onto the single concrete *token.LocalTokenProvider. There was exactly one implementation and the only other "implementation" — the generated MockTokenProvider — was completely unused (every test constructs a real token.NewLocalTokenProvider). This is a pure internal refactor: no config, API, or token-output change.

AI Authorship

  • AI was used. Details:
    • Tool / model: Claude Opus 4.8 via Claude Code
    • AI-authored files: all 12 changed files
    • Human line-by-line reviewed: none yet — relying on the passing test suite + lint and a 4-agent automated cleanup pass. Reviewer line-by-line attention requested on the core files below.

Change classification

  • Core code (broad impact — needs line-by-line review)

    Touches the token-issuance path (internal/core/token.go, internal/token/, internal/services/token*.go). Although behavior is unchanged, this is the auth/token-signing core, so the stricter review bar applies.

Plan reference

Executed from plan.md"Remove the pluggable TokenProvider interface (collapse to concrete LocalTokenProvider)". Goal: the interface abstraction is gone, every consumer holds the concrete provider, the dead mock is deleted, docs no longer describe a pluggable token backend, and make generate && build && test && lint all pass with no behavior change to issued tokens.

Verification

  • Existing unit/integration tests pass (make test green; services, bootstrap, token, core re-run individually green)
  • make generate && make build && make lint all pass (lint: 0 issues)
  • No new tests added — intentional. This is a pure type-level refactor; the plan relies on the existing suite proving no behavior drift. The three plan e2e checks are already covered by existing tests:
    1. Happy path — OIDC ID token still issued on authorization_code + openid (token-exchange tests)
    2. JWKS for HS256 returns an empty key set (concrete PublicKey() → nil)
    3. Refresh & client_credentials grants still issue correct aud/scope
  • Manual verification for reviewer: grep -rn "core.TokenProvider\|core.IDTokenProvider\|MockTokenProvider" --include="*.go" . → no matches.

Verifiability check

  • Inputs/outputs unchanged — issued JWTs, ID tokens, and JWKS output are byte-for-byte identical
  • Reviewer can judge correctness from the type-only diff + green suite
  • Any drift would surface as a test failure (full suite exercises all four grants)

Security check

Pure internal refactor — no external interface, input-validation, or auth-decision change. No secrets in the diff. The type claim / aud binding logic and signing code (token/local.go) are untouched except for removing a dead var _ core.TokenProvider assertion and an unused import.

Risk & rollback

  • Risk: Low. Compiler-enforced — any missed reference would have failed the build. No data, migration, or config impact.
  • Rollback: Single self-contained commit on a feature branch; git revert restores the interface.

Reviewer guide

  • Read carefully:
    • internal/services/token_exchange.go — the ID-token block lost its core.IDTokenProvider type assertion; the if scopeSet := …; scopeSet["openid"] guard was re-indented (confirm the re-indent is behavior-preserving).
    • internal/core/token.go — confirm only the two interfaces were deleted; the four result/param structs are kept.
    • internal/bootstrap/handlers.gobuildJWKSHandler now calls the concrete Algorithm()/KeyID()/PublicKey(); NewOIDCHandler is passed a literal true for ID-token support (the bool param is now vestigial — noted as a follow-up, not changed here).
  • Spot-check OK: the mechanical core.TokenProvider*token.LocalTokenProvider retypes in bootstrap/{bootstrap,services}.go, services/token.go, token/idtoken.go, token/local.go; the mock_token.go deletion + generate.go directive removal; the CLAUDE.md / ARCHITECTURE.md doc updates.

🤖 Generated with Claude Code

- Remove the TokenProvider and IDTokenProvider interfaces in favor of the single concrete LocalTokenProvider
- Inject the concrete provider directly into services, bootstrap, and the JWKS handler
- Replace the ID token provider type assertion with a direct method call
- Delete the unused generated token provider mock and its generate directive
- Update architecture docs and guidelines to describe the concrete provider
Copilot AI review requested due to automatic review settings June 2, 2026 15:18

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes the unused core.TokenProvider / core.IDTokenProvider interface layer and updates the codebase to inject/use the single concrete implementation (*token.LocalTokenProvider) everywhere. This simplifies the token issuance and JWKS/ID-token capability wiring without changing token behavior or external APIs.

Changes:

  • Deleted core.TokenProvider and core.IDTokenProvider (and the unused generated GoMock token mocks).
  • Updated services/bootstrap/handlers to depend on *token.LocalTokenProvider directly (including JWKS + ID-token support wiring).
  • Updated docs to reflect that token generation is intentionally concrete (not pluggable via interface).

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated no comments.

Show a summary per file
File Description
internal/token/local.go Removes now-dead interface assertion/import; keeps provider concrete.
internal/token/idtoken.go Removes re-export of the removed IDTokenProvider alias; keeps IDTokenParams alias.
internal/services/token.go Changes TokenService to hold *token.LocalTokenProvider instead of core.TokenProvider.
internal/services/token_exchange.go Removes optional ID-token-provider assertion and calls GenerateIDToken directly under openid scope gate.
internal/mocks/mock_token.go Deletes unused generated mocks for the removed interfaces.
internal/mocks/generate.go Removes the go:generate directive for the deleted token mocks.
internal/core/token.go Removes the interfaces; keeps shared param/result structs in core.
internal/bootstrap/services.go Updates service initialization wiring to accept *token.LocalTokenProvider.
internal/bootstrap/handlers.go JWKS handler now built directly from LocalTokenProvider; ID-token support flag set to true.
internal/bootstrap/bootstrap.go Updates Application.TokenProvider field type to *token.LocalTokenProvider.
docs/ARCHITECTURE.md Updates architecture docs to describe the concrete token provider and its RFC 8707-shaped signatures.
CLAUDE.md Updates contributor guidance to reflect “intentionally concrete” token provider design.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@codecov

codecov Bot commented Jun 2, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 77.08333% with 11 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
internal/services/token_exchange.go 84.09% 6 Missing and 1 partial ⚠️
internal/bootstrap/handlers.go 0.00% 4 Missing ⚠️

📢 Thoughts on this report? Let us know!

@appleboy appleboy merged commit beed71f into main Jun 2, 2026
18 checks passed
@appleboy appleboy deleted the refactor/concrete-token-provider branch June 3, 2026 12:58
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.

2 participants