Skip to content

feat(token): emit server-attested uid claim from User.Username#184

Merged
appleboy merged 2 commits intomainfrom
worktree-uid
May 5, 2026
Merged

feat(token): emit server-attested uid claim from User.Username#184
appleboy merged 2 commits intomainfrom
worktree-uid

Conversation

@appleboy
Copy link
Copy Markdown
Member

@appleboy appleboy commented May 3, 2026

Summary

Adds a server-attested <prefix>_uid private claim (default extra_uid) to access and refresh tokens, sourced from User.Username and re-resolved on every issuance and refresh so admin renames propagate to the next token. The claim is suppressed for the client_credentials grant where no real user exists. This is Stage 1 of a larger design — a future Stage 2 (RFC 7523 JWT Bearer Assertion grant for external services that have already authenticated users) was discussed and intentionally deferred to a separate PR.

Mirrors the pattern of <prefix>_domain from #181 / b2dc884.

AI Authorship

  • No AI was used in this PR
  • AI was used. Details:
    • Tool / model: Claude Opus 4.7 (1M context) via Claude Code
    • AI-authored files: All 12 files in this PR were written by Claude
    • Human line-by-line reviewed: None — design was reviewed (decisions on Client Credentials suppression, Stage 1 scoping, Option A vs C trust models, simplify pass), but the diff itself was not walked line-by-line by a human

Reviewer note: Treat this as fully AI-authored, line-by-line review needed.

Change classification

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

The change touches token issuance, which affects every JWT minted by AuthGate. Downstream resource servers consume these claims; a defect here ships to every consumer.

Plan reference

This PR implements Stage 1 of the plan at ~/.claude/plans/token-post-oauth-token-synchronous-planet.md (local plan file, not in repo).

Goal: every access/refresh JWT issued via POST /oauth/token for authorization_code, device_code, and refresh_token grants carries <prefix>_uid: <User.Username>. client_credentials grant must omit. Username re-resolves from DB on every issuance/refresh.

Locked design decisions (resolved with the user during planning):

  1. Client Credentials → omit <prefix>_uid entirely; downstream M2M consumers can use client_id or <prefix>_service_account for machine identity.
  2. Refresh re-resolves username from DB every time (mirrors live JWT_DOMAIN re-resolution); on lookup failure, log and omit the claim — never fail issuance.

Plan-scope deviation (disclose)

The plan's must-not-modify list included internal/handlers/, internal/services/audit.go, and internal/services/token_introspect.go. The simplify pass touched all three to consolidate the "client:" sentinel that was duplicated across 6 production sites:

  • Added MachineUserIDPrefix const + MachineUserID(clientID) / IsMachineUserID(userID) helpers in internal/services/token_client_credentials.go.
  • Replaced bare "client:" + clientID and strings.HasPrefix(userID, "client:") literals in services/token_introspect.go, services/audit.go, handlers/token.go, and the new services/token.go resolver.

Rationale: introducing the helper without updating the existing 4 sites would have left the new code adding a 6th duplicate of the literal. Removing duplication felt mandatory once the helper existed. Reviewer should sanity-check that this consolidation hasn't changed behavior at any of the 4 pre-existing call sites — the refactor is mechanical but each call site is in a distinct hot path (audit logging, introspection, handler subjectType discrimination).

Verification

  • Unit tests — TestBuildServerClaims table extended for the new username dimension (5 cases, including custom prefix)
  • Integration tests — new file internal/services/token_uid_test.go with 4 tests:
    1. Happy path: TestAuthCodeFlow_EmitsUidClaim — auth_code + device_code subtests, both grants emit <prefix>_uid on access AND refresh tokens
    2. Omission: TestClientCredentialsFlow_OmitsUidClaim — claim absent on M2M tokens; explicitly contrasts the parallel domain-claim test that asserts presence
    3. Live re-resolution: TestRefresh_ReResolvesUidAfterUsernameChange — admin rename between issuance and refresh propagates to the next refreshed token
    4. Failure mode: TestUidClaim_OmittedWhenUserLookupFails — issuance with a UserID that has no matching user row succeeds with claim absent
  • Defense-in-depth: existing TestPrivateClaimRegistryDrift (config) auto-covers the new uid registry entry without test edits; existing reserved-key / strip tests cascade through BuildReservedClaimKeys and computeStripList so callers cannot inject extra_uid, bare uid, or default-prefixed variants
  • Stress / soak test — N/A (token issuance is a bounded path; the only new work is one DB roundtrip per issuance, comparable to existing client lookup)
  • Manual verification: see Risk & rollback for the manual decode steps

Verifiability check

  • Inputs and outputs are documented (registry doc in internal/token/types.go, function-level docs in internal/services/token.go)
  • Reviewer can judge correctness without reading every line (4 e2e tests + table-driven helper test cover the user-observable contract)
  • Failures will surface — [Token] uid claim: GetUserByID failed user_id=... log line on lookup miss

Security check

This PR touches token issuance, which is auth-adjacent. Security checklist:

  • No secrets in code
  • No new external inputs — <prefix>_uid is fully server-derived from User.Username
  • Caller cannot impersonate the claim — registry entry adds uid / extra_uid to reserved keys (rejected at the parser) AND to the strip list (defense-in-depth at sign time); reserved-key tests are auto-covered by the registry-driven derivation
  • No new permissions / rate limits needed (issuance flow already protected)
  • Errors don't leak internals — lookup failure logs at server side, claim is silently omitted in the JWT (no error returned to the client)
  • OIDC discovery: <prefix>_uid is intentionally NOT advertised in claims_supported (matches the precedent for <prefix>_domain set in fix(oidc): drop non-OIDC claims from discovery metadata #180 — prefix-namespaced server-attested claims are AuthGate private extensions, not OIDC standard)

Risk & rollback

Risks:

  • Extra DB roundtrip per token issuance — each user-bearing grant now does one s.store.GetUserByID(userID). Bypasses the user cache in UserService because TokenService calls store directly. Acknowledged as parity with the existing client lookup; future optimization can route through UserService when DI is restructured. Refresh hot path order is correct: GetUserByID runs AFTER all short-circuit checks (IsRefreshToken, IsActive, IsExpired, ClientID mismatch, scope subset).
  • Auth_code + openid+profile/email scopes does the same User fetch twice — once in composeIssuanceClaims for uid, once in ExchangeAuthorizationCode for ID-token claims. Worth a focused follow-up to context-cache via models.SetUserContext; not in scope here.
  • Cached tokens carry stale username after rename — token cache only invalidates on revocation; rename takes effect on next issuance, matching the <prefix>_domain semantics.
  • Downstream consumer trusts <prefix>_uid for auth decisions — the registry doc explicitly says it's a handle, not authority-attested identity. Reviewers should sanity-check the doc framing in internal/token/types.go.

Manual verification before merge:

# Start server
make generate && make build && ./bin/authgate server &

# Auth code / device code flow → user token, decode, expect extra_uid: <username>
# Client credentials → expect NO extra_uid:
curl -s -X POST http://localhost:8080/oauth/token \
  -d grant_type=client_credentials -d client_id=$CID -d client_secret=$CS \
  | jq -r .access_token | cut -d. -f2 | base64 -d 2>/dev/null | jq .

Rollback: Single revert. Existing token consumers must already handle the claim's absence (since it's a new claim), so reverting drops the claim cleanly. Tokens already issued with the claim remain valid — JWT verification doesn't reject unknown claims.

Reviewer guide

Read carefully (line-by-line — fully AI-authored, not human-reviewed):

  • internal/services/token.gobuildServerClaims (signature changed), resolveUsernameForUID (new), composeIssuanceClaims (signature changed). The trust model and short-circuit logic for the client: UserID belongs here.
  • internal/services/token_client_credentials.go — new MachineUserIDPrefix / MachineUserID / IsMachineUserID exports. These are now consumed cross-package by handlers/token.go; verify export and naming sit well in this file.
  • internal/services/token_uid_test.go — assertion semantics: assertPrivateClaim(t, cfg, raw, logical, "") asserts absence. Verify test logic matches the omission-vs-emission expectations stated in each test docstring.
  • internal/token/types.go — registry entry (single line) plus updated trust-level docstring. The docstring is the authoritative reference for downstream consumers' trust assumptions.

Spot-check OK (mechanical refactors):

  • internal/handlers/token.go, internal/services/audit.go, internal/services/token_introspect.go"client:" literal → helper call. Behavior unchanged; tests cover.
  • internal/services/token_refresh.go, internal/services/token_client_credentials.go (call site) — single-line userID plumbing into composeIssuanceClaims.
  • internal/services/token_extra_claims_test.go, internal/services/token_domain_test.go, internal/services/token_private_claim_prefix_test.go — signature-fix updates and assertPrivateClaim unification.
  • internal/config/config.go — single-line drift-guard parallel registry update.

Suggested reviewer count: 2+ (core code, fully AI-authored). Include the token-issuance module owner (recent recent token-stack contributors per git log internal/token internal/services/token*.go).

- Add `<prefix>_uid` private claim to access and refresh tokens, sourced from User.Username and re-resolved on every issuance and refresh
- Suppress the claim on the client_credentials grant where no real user exists
- Log and omit when the user lookup fails so token issuance never fails on a missing or deleted user
- Extract MachineUserID/IsMachineUserID helpers and replace the "client:" sentinel literal that was duplicated across audit, introspect, handlers, and token services
- Unify the per-claim test assertion helpers behind a single assertPrivateClaim so future private-claim tests reuse it

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 3, 2026 07:40
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Adds a new server-attested private JWT claim for user identity (<prefix>_uid, default extra_uid) emitted on access/refresh tokens and re-resolved from User.Username on each issuance/refresh, while also consolidating the client-credentials synthetic user ID sentinel into shared helpers.

Changes:

  • Register new private claim logical name uid and emit it from User.Username in token issuance/refresh flows (omitted for client_credentials and on user-lookup failure).
  • Refactor "client:" + clientID / HasPrefix("client:") usages into MachineUserID(...) / IsMachineUserID(...) helpers.
  • Add/extend unit + integration tests covering emission/omission and refresh-time re-resolution semantics.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
internal/token/types.go Adds uid to the private-claim registry and documents trust model/semantics.
internal/config/config.go Adds uid to the config-side drift-guard list of private claim logical names.
internal/services/token.go Implements username resolution and emits <prefix>_uid as a server-attested claim during issuance.
internal/services/token_refresh.go Ensures refresh-time claim composition includes the refresh token’s UserID for uid re-resolution.
internal/services/token_client_credentials.go Introduces MachineUserID* helpers and ensures client-credentials tokens omit uid.
internal/services/token_introspect.go Uses MachineUserID(...) for audit actor identity instead of inline "client:" concatenation.
internal/services/audit.go Uses IsMachineUserID(...) when deciding whether to DB-resolve actor usernames.
internal/handlers/token.go Uses services.IsMachineUserID(...) for subject-type discrimination and introspection username enrichment.
internal/services/token_extra_claims_test.go Extends buildServerClaims tests to cover username/uid behavior and custom prefixes.
internal/services/token_domain_test.go Generalizes claim assertion helper for reuse across private-claim tests and updates buildServerClaims signature.
internal/services/token_private_claim_prefix_test.go Updates to new buildServerClaims signature to keep reserved/override tests accurate.
internal/services/token_uid_test.go Adds end-to-end style tests for uid emission, omission on client credentials, and refresh-time re-resolution.

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

Comment thread internal/services/token_client_credentials.go Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented May 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

… Username

- Correct the IsMachineUserID doc-comment to reference AccessToken.UserID and the User.ID UUID invariant rather than User.Username, which is not what the function checks

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

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


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

@appleboy appleboy merged commit d5d28de into main May 5, 2026
21 checks passed
@appleboy appleboy deleted the worktree-uid branch May 5, 2026 05:15
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