Clean up JWT API surface and eliminate redundant subject claims

[Copilot]

This change tightens a few API/design edges called out in the issue set: the JWT manager exposed unused context.Context parameters, JWTs encoded the user ID twice via two sub claim sources, OAuth2 race-retry fallback hid underlying datastore failures, and one email-verification log key drifted from the repo’s structured logging convention.

• JWT API surface

  • Removed unused context.Context parameters from:
    • JWTManager.CreateToken
    • JWTManager.CreateTokenWithSession
    • JWTManager.ValidateToken
  • Updated all internal call sites, tests, and docs to match the narrower API.

• JWT claims model

  • Removed the custom Claims.UserID field and now use jwt.RegisteredClaims.Subject as the single source of truth for the authenticated user ID.
  • Updated token consumers (middleware, logout/session handling, tests, docs) to read claims.Subject.

• OAuth2/OIDC user resolution

  • Preserved underlying datastore failures in the findOrCreateUser race-retry path instead of collapsing them into a generic "failed to resolve OIDC user" error.
  • The retry still handles expected ErrNotFound cases, but operational failures now retain context via wrapped errors.

• Structured logging consistency

  • Standardized the email verification handler log field from userID to user_id to match the rest of the handlers.

token, err := jwtMgr.CreateToken(userID)
claims, err := jwtMgr.ValidateToken(tokenString)

userID := claims.Subject
sessionID := claims.ID

Greptile Summary

This PR cleans up the JWTManager API by removing unused context.Context parameters from CreateToken, CreateTokenWithSession, and ValidateToken, and eliminates a long-standing sub claim duplication where both Claims.UserID (with json:\"sub\" tag) and jwt.RegisteredClaims.Subject serialized to the same JWT field. It also tightens error propagation in the OAuth2 race-retry path and standardizes a log key in the email verification handler.

• JWT claims model: Removes the redundant Claims.UserID field — previously the Go JSON embedding rules meant only UserID (at depth 1) was marshaled/unmarshaled while RegisteredClaims.Subject was silently ignored. Now claims.Subject is the single authoritative field, and all consumers (middleware, logout, tests, docs) have been updated accordingly.
• OAuth2 race-retry: The retry path after ErrEmailExists now propagates wrapped store errors instead of silently returning a generic message, and two focused tests cover both new error paths.
• Structured logging: The email_verification.go log key is renamed from userID to user_id to match the rest of the codebase.

Confidence Score: 5/5

Safe to merge — the JWT wire format is unchanged (sub still maps to RegisteredClaims.Subject), all consumer call sites are updated consistently, and the race-retry error paths now correctly propagate store failures.

The sub-claim fix is backward-compatible: old tokens carry sub in the JSON payload and the new Claims struct parses it into RegisteredClaims.Subject just as before. Context parameters were unused in the JWT methods, so removing them cannot change runtime behavior. The OAuth2 error-propagation change is additive — it surfaces previously swallowed errors and is covered by new tests. No auth logic or token-validation rules were altered.

No files require special attention.

Important Files Changed

Filename	Overview
auth/jwt.go	Removes unused context params from all three public JWTManager methods and drops the redundant UserID field; Subject is now the sole sub-claim source. Logic is correct and backward-compatible.
handler/oauth2_common.go	Race-retry path now returns wrapped errors for non-ErrNotFound store failures; two new tests cover both added error branches. Consistent with the initial lookup paths.
handler/helpers.go	tokenCreator interface and issueTokens call sites updated to drop context params; no logic changes.
handler/email_verification.go	Single log key renamed from userID to user_id to match structured logging convention; all other slog calls already used context variants.
auth/middleware.go	resolveUser updated to call ValidateToken without context and read claims.Subject instead of claims.UserID; logic unchanged.
handler/auth.go	Logout handler updated to use claims.Subject instead of claims.UserID for session deletion; correct single-line change.
handler/oauth2_test.go	Two new tests added for the race-retry error paths, correctly exercising wrapped error propagation via errors.Is.
auth/jwt_test.go	All test call sites updated: context params removed and assertions moved from claims.UserID to claims.Subject throughout.

Sequence Diagram

sequenceDiagram
    participant C as Client
    participant H as Handler (issueTokens)
    participant J as JWTManager
    participant M as Middleware (resolveUser)

    Note over J: CreateToken(userID) — no ctx
    H->>J: CreateToken(userID)
    J-->>H: "signedJWT (sub=userID)"

    Note over J: CreateTokenWithSession(userID, sessID) — no ctx
    H->>J: CreateTokenWithSession(userID, sessID)
    J-->>H: "signedJWT (sub=userID, jti=sessID)"

    C->>M: Request + Bearer token
    Note over M: ValidateToken(token) — no ctx
    M->>J: ValidateToken(tokenString)
    J-->>M: "*Claims{RegisteredClaims}"
    Note over M: userID = claims.Subject
    Note over M: sessID = claims.ID

Loading

_{Reviews (2): Last reviewed commit: "test(handler): add unit tests for race-r..." | Re-trigger Greptile}

[Copilot]

Pull request overview

This PR refines the auth surface area by simplifying the JWT manager API (removing unused context.Context params), consolidating JWT user identity into the single standard sub claim (jwt.RegisteredClaims.Subject), improving error preservation in the OAuth2/OIDC user-resolution race-retry path, and aligning one log field name with the repo’s structured logging conventions.

Changes:

• Removed unused context.Context parameters from JWTManager methods and updated call sites + tests/docs accordingly.
• Removed the redundant custom Claims.UserID field and migrated consumers to claims.Subject.
• Improved OAuth2/OIDC race-retry error handling to preserve underlying datastore failures; standardized one email verification log key to user_id.

Show a summary per file

File	Description
README.md	Updates JWT usage example to the new JWTManager signatures and Subject-based user ID.
docs/auth/jwt.md	Updates JWT documentation to reflect the narrower API and Subject-only user identity.
auth/jwt.go	Removes ctx params, drops custom Claims.UserID, and uses RegisteredClaims.Subject for user ID.
auth/jwt_test.go	Updates tests for new JWTManager signatures and Subject-based assertions.
auth/middleware.go	Switches JWT resolution to ValidateToken(token) and returns claims.Subject.
auth/middleware_test.go	Updates middleware tests to use the new JWTManager method signatures.
auth/rbac_test.go	Updates RBAC tests to use the new CreateToken signature.
handler/helpers.go	Updates tokenCreator interface and issuance flow to the new JWTManager signatures.
handler/helpers_test.go	Updates tokenCreator test double to match the new interface.
handler/magiclink_test.go	Updates magiclink token-creation mock to match the new signature.
handler/auth.go	Uses claims.Subject for session revocation on logout.
handler/auth_test.go	Updates logout tests to use the new JWTManager signatures.
handler/oauth2_common.go	Preserves datastore failures in the race-retry lookup path via wrapped errors.
handler/email_verification.go	Renames the log key from userID to user_id for consistency.

Copilot's findings

Tip

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

Comments suppressed due to low confidence (1)

handler/oauth2_common.go:63

• New branch returns a wrapped error when the race-retry FindByEmail call fails with a non-ErrNotFound. There doesn’t appear to be a test covering this branch, so the intended behavior (preserving the datastore failure instead of falling through to the generic "failed to resolve OIDC user") isn’t locked in. Add a unit test that forces FindByEmail to return a non-ErrNotFound error in the race-retry path and assert the returned error wraps the underlying failure.

	if u, err := users.FindByEmail(ctx, email); err == nil {
		linkSubjectBestEffort(ctx, users, u.ID, subject, "race_retry")
		return u, nil
	} else if !errors.Is(err, auth.ErrNotFound) {
		return nil, fmt.Errorf("look up user by email after email race: %w", err)
	}

• Files reviewed: 14/14 changed files
• Comments generated: 1