fix(oauth): re-check user identity on code redemption

[appleboy]

Summary

A user could be disabled (IsActive=false) or hard-deleted by an admin in the window after they authorize a device/auth code but before the client redeems it. The redemption path only checked client.IsActive() — never the user — so a disabled user still got a fresh access+refresh token pair, and a deleted user got tokens whose sub/user_id pointed at a non-existent account.

This re-verifies the authorizing user exists and is active immediately before consuming the single-use code on both code-redemption grants, rejecting with access_denied and minting no tokens. It is a sibling to the existing client.IsActive() checks already on all four grants.

What changed (production)

• ExchangeDeviceCode (token_exchange.go): re-check user before DeleteDeviceCodeByID, so a doomed redemption does not burn the device code. Records oauth_device_code_validation{result="invalid"}.
• AuthorizationService.ExchangeCode (authorization.go): re-check user before MarkAuthorizationCodeUsed, joining the existing RFC 8707 pre-consume guard block.
• Handler (token.go): map services.ErrAccessDenied → OAuth access_denied on the authorization_code grant (the device-code grant already mapped it; the auth-code switch did not, so it would have fallen through to invalid_grant).
• Both rejection paths emit a SUSPICIOUS_ACTIVITY (WARNING) audit event with client_id. Disabled vs. deleted both collapse to access_denied so we never leak whether an account exists.

The refresh_token and client_credentials grants are intentionally untouched: disabling a user already revokes their tokens, so the refresh grant fails its existing revoked/IsActive() check.

AI Authorship

• AI was used.
  • Tool / model: Claude Code (Opus 4.8), executing a pre-written plan.md, then refined by an automated 4-agent /simplify review pass.
  • AI-authored files: all files in this PR.
  • Human line-by-line reviewed: author to confirm before merge.

Change classification

• Core code — OAuth token redemption / auth. Failure is system-wide (privilege persistence after account disablement/deletion). Needs line-by-line review.

Plan reference

Plan goal: re-check user identity between authorization and token redemption (Batch C #6). Done = both redemption grants re-verify the user exists and is active immediately before consuming the single-use code; a missing/inactive user is rejected with access_denied and the code/token pair is never minted.

Two disclosed deviations from the plan's "may modify" list (both intentional):

1. internal/handlers/token.go — the plan assumed the handler already mapped ErrAccessDenied to access_denied for the auth-code grant; it did not. Adding the one-line case was explicitly authorized by the plan's own Risk-mitigation note and confirmed with the author.
2. token_resource_test.go / token_uid_test.go — seeded real active users at their device-code call sites, exactly as the plan's Risk section instructed ("grep for other call sites … seed real users there too").

Verification

• Unit/service tests — 6 new (3 per grant: happy path + disabled + deleted, plus an audit-event assertion per grant via a recording audit logger)
• Handler test — authorization_code grant returns access_denied (HTTP 400) for a disabled user
• Disabled-user tests prove the single-use code is not consumed (re-enable + redeem again succeeds)
• Stress / soak test: N/A (no long-running/async code added)
• Manual verification: make generate && make fmt && make lint && make test — all clean (lint 0 issues; internal/services and internal/handlers pass with -count=1).

Verifiability check

• Inputs/outputs documented in code comments and test names
• Reviewer can judge correctness from test names + the small production diff
• Failures surface via the SUSPICIOUS_ACTIVITY audit event and the device-code invalid metric

Security check

• No secrets in the diff (test-only fixed strings)
• External input (user state) validated at the redemption chokepoint
• Rejection is uniform (access_denied) — does not leak disabled vs. deleted
• Reject happens before single-use-code consumption (no code-burn on rejection)

Risk & rollback

• Risk: low. One extra GetUserByID read per redemption (not a hot path; refresh is, and is untouched). No schema/migration change.
• Rollback: pure code revert — revert the two service edits + the handler line + test changes; no state to undo.

Reviewer guide

• Read carefully: internal/services/token_exchange.go (placement before DeleteDeviceCodeByID), internal/services/authorization.go (placement before MarkAuthorizationCodeUsed), internal/handlers/token.go (the new access_denied case).
• Spot-check OK: the *_test.go files — test-helper seeding and the new test cases; signatures + names convey intent.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[appleboy]

🔎 Max-effort code review (9 finder angles + verify + sweep)

Correctness (angles A–E, line-by-line / removed-behavior / cross-file / Go-pitfalls): no confirmed bugs.

• User re-check is correctly placed before code consumption on both grants (DeleteDeviceCodeByID / MarkAuthorizationCodeUsed) — a disabled/deleted user never burns the single-use code. ✔
• err shadowing in both := blocks is safe (outer err already returned). Short-circuit err != nil || user == nil || !user.IsActive has no nil-deref. ✔
• New handler case errors.Is(err, services.ErrAccessDenied) is reachable and not shadowed; ExchangeCode returns the token.go ErrAccessDenied sentinel that the handler matches. ✔
• No test regressions: the early-failing tests (expiry/secret/PKCE/redirect/auth-pending) reject before the new check; modified tests all seed real users. ✔

Fixed in c9e147f (audit-consistency, Angle E):

1. Both rejection paths now emit their SUSPICIOUS_ACTIVITY event via LogSync instead of async Log — matching the codebase's only other SUSPICIOUS_ACTIVITY emitter (revokeTokenFamilyWithAudit), so the security event survives a crash instead of being lost in the async batch.
2. The device-code rejection event now carries ResourceType: ResourceDeviceCode + ResourceID: dc.DeviceCodeID (was ResourceToken, no ID), mirroring EventDeviceCodeAuthorized so admins can trace the blocked redemption to its device code.

Skipped (with rationale):

• Double GetUserByID on the authorization_code + openid+profile/email path (ExchangeCode re-check, then ExchangeAuthorizationCode re-fetch for ID-token claims). Real but eliminating it requires threading the user across the ExchangeCode → handler → ExchangeAuthorizationCode boundary (an exported-API change), and redemption is not a hot path — the plan explicitly accepted one extra read.
• Extract a shared validateUserIsActive helper for the two re-check blocks. The blocks diverge (2- vs 3-value returns, device-only metric, different ResourceType/ResourceID); a shared helper would need a callback/param-soup that hurts readability.
• user == nil guard "dead code". Kept — s.store is the core.Store interface; the never-(nil,nil) guarantee is only the concrete type's, so the guard is defensive, not dead.
• Re-check refresh_token / client_credentials grants too. Correct by design (disabling a user revokes their tokens, so refresh fails its existing revoked check; CC has no user). Documented in the PR body.

✅ make fmt / make lint (0 issues) / make test (services + handlers) all green.

[Copilot]

Pull request overview

This PR strengthens OAuth code redemption by re-checking that the authorizing user still exists and is active before allowing device-code or authorization-code redemption to proceed.

Changes:

• Adds user existence/active checks and audit events in device-code and auth-code redemption paths.
• Maps auth-code ErrAccessDenied to OAuth access_denied.
• Updates and adds tests for disabled/deleted users, audit events, and helper seeding.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
internal/services/token_exchange.go	Adds device-code redemption user re-check before consuming the device code.
internal/services/authorization.go	Adds authorization-code redemption user re-check before marking the code used.
internal/handlers/token.go	Maps auth-code access denial to OAuth access_denied.
internal/handlers/token_authcode_user_recheck_test.go	Adds handler coverage for disabled-user auth-code redemption.
internal/services/token_test.go	Adds device-code user re-check tests and supporting helpers.
internal/services/authorization_test.go	Adds auth-code user re-check tests and updates existing tests to seed users.
internal/services/token_resource_test.go	Seeds a real user for authorized device-code resource tests.
internal/services/token_uid_test.go	Updates UID-claim helper to mutate the seeded user instead of recreating it.

---

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