feat: generic OAuth2 handler with pluggable identity provider

[Copilot]

OIDCHandler is tied to providers that issue id_tokens (OIDC discovery + JWT verification). Providers like GitHub implement OAuth2 but not OIDC, requiring a separate abstraction to avoid per-provider rewrites.

Core abstraction

Single interface to implement per provider:

type OAuth2IdentityProvider interface {
    FetchUserInfo(ctx context.Context, token *oauth2.Token) (*OAuth2UserInfo, error)
}

OAuth2Handler handles the full flow (PKCE, CSRF, code exchange, session/refresh tokens, account linking) generically — callers only implement FetchUserInfo.

New files

• handler/oauth2.go — OAuth2Handler with Validate, Login, Callback, CreateLinkNonce, Link; mirrors OIDCHandler field-for-field. Adds LoginRedirect string to configure the post-login query param (default oauth2_login=1).
• handler/oauth2_providers.go — built-in GitHubProvider (calls /user + /user/emails, prefixes subjects github:<id> to avoid cross-provider collisions) and GoogleOAuth2Provider (userinfo endpoint fallback).
• handler/oauth2_common.go — package-level shared helpers extracted from OIDCHandler: findOrCreateUser, linkSubjectBestEffort, signLinkState, parseLinkState, consumeLinkNonce, handleLinkCallback. Both handlers delegate to these instead of duplicating.
• handler/oauth2_test.go — full test coverage for all flows.
• docs/handler/oauth2.md — provider interface docs, subject-prefix convention, built-ins, linking flow, status codes.

Refactors / fixes

• handler/oidc.go — existing methods become thin wrappers over the shared package-level functions.
• handler/oidc_test.go — fixed broken TestOIDCLink_storeError that referenced undefined h.linkNonces / linkNonce types (pre-existing compile error).
• docs/handler/oidc.md — corrected pre-existing doc error: User not found is only for ErrNotFound; transient DB errors on FindByID produce Link verification failed.

Usage

h := &handler.OAuth2Handler{
    Users:         userStore,
    JWT:           jwtMgr,
    OAuthConfig:   oauth2.Config{Endpoint: github.Endpoint, Scopes: []string{"read:user", "user:email"}, ...},
    Provider:      &handler.GitHubProvider{},
    CookieName:    "session",
    SecureCookies: true,
    LoginRedirect: "github_login=1",
}
// Any provider: implement OAuth2IdentityProvider for Discord, Slack, etc.

Greptile Summary

This PR introduces OAuth2Handler, a generic OAuth2 login/linking flow that mirrors OIDCHandler but delegates identity resolution to a pluggable OAuth2IdentityProvider interface, along with built-in GitHubProvider and GoogleOAuth2Provider implementations. Shared helpers (findOrCreateUser, handleLinkCallback, link-state signing) are extracted into oauth2_common.go so both handlers delegate to the same logic, eliminating duplication.

Confidence Score: 5/5

Safe to merge; the single finding is a P2 observability improvement in an edge-case retry path that still returns an error correctly.

No P0 or P1 issues found. The one flagged item (race-retry swallowing non-ErrNotFound DB errors) is P2 — it doesn't change the error outcome (a 500 is still returned), only loses diagnostic context. All slog calls include context per the custom rule, Validate() correctly guards all required fields including Provider, and both providers correctly prefix subjects.

handler/oauth2_common.go — race-retry error handling in findOrCreateUser

Important Files Changed

Filename	Overview
handler/oauth2.go	Core handler: Validate() correctly guards Provider/Users/JWT/Sessions+RefreshCookieName; CSRF+PKCE flow, link state signing, and email-verification bypass for link flows all look correct.
handler/oauth2_common.go	Shared helpers extracted from OIDCHandler; race-retry path in findOrCreateUser silently swallows non-ErrNotFound DB errors (P2). All slog calls correctly include context.
handler/oauth2_providers.go	Both GitHubProvider and GoogleOAuth2Provider correctly prefix subjects (github: / google:). HTTP clients, error handling, and JSON decoding look correct.
handler/oauth2_test.go	Comprehensive coverage of all flows including race-retry, link flows, custom redirect, and error paths. Fake token server pattern is clean.
handler/oidc.go	Correctly thinned to delegate findOrCreateUser, consumeLinkNonce, signLinkState, parseLinkState, and handleLinkCallback to the new shared package-level functions.
docs/handler/oauth2.md	Accurate documentation of the interface, subject-prefix convention, built-in providers, link flow, and HTTP status code table.

Sequence Diagram

sequenceDiagram
    participant Browser
    participant OAuth2Handler
    participant Provider as OAuth2IdentityProvider
    participant OAuth2Server as OAuth2 Provider
    participant Users as UserStore

    Note over Browser,Users: Login flow
    Browser->>OAuth2Handler: GET /login
    OAuth2Handler->>Browser: Set state+verifier cookies, 302 to OAuth2Server

    Browser->>OAuth2Server: Authorize (PKCE)
    OAuth2Server->>Browser: 302 to /callback?state=&code=

    Browser->>OAuth2Handler: GET /callback?state=&code=
    OAuth2Handler->>OAuth2Handler: Validate CSRF state cookie
    OAuth2Handler->>OAuth2Server: Exchange code (PKCE verifier)
    OAuth2Server-->>OAuth2Handler: access_token
    OAuth2Handler->>Provider: FetchUserInfo(ctx, token)
    Provider-->>OAuth2Handler: OAuth2UserInfo{Subject, Email, ...}
    OAuth2Handler->>Users: FindByOIDCSubject / FindByEmail / CreateOIDCUser
    Users-->>OAuth2Handler: auth.User
    OAuth2Handler->>Browser: Set JWT cookie, 302 to /?LoginRedirect

    Note over Browser,Users: Link flow
    Browser->>OAuth2Handler: POST /link-nonce (authenticated)
    OAuth2Handler->>Browser: {nonce: ...}
    Browser->>OAuth2Handler: GET /link?nonce=...
    OAuth2Handler->>OAuth2Handler: consumeLinkNonce to userID
    OAuth2Handler->>OAuth2Handler: signLinkState(JWT, rand, userID)
    OAuth2Handler->>Browser: Set signed-state cookie, 302 to OAuth2Server
    OAuth2Server->>Browser: 302 to /callback?state=signedState&code=
    Browser->>OAuth2Handler: GET /callback (signedState)
    OAuth2Handler->>OAuth2Handler: parseLinkState to linkUserID
    OAuth2Handler->>Users: FindByID, FindByOIDCSubject, LinkOIDCSubject
    OAuth2Handler->>Browser: 302 to /?oauth2_linked=true

Loading

Comments Outside Diff (1)

1. handler/oauth2.go, line 293-298 (link)

   Validate() does not check Provider for nil

   Provider is the single required field unique to OAuth2Handler, yet Validate() skips it. If a caller forgets to set Provider and still calls h.Validate() (which returns nil), the server starts cleanly — then panics with a nil pointer dereference the first time Callback reaches h.Provider.FetchUserInfo(r.Context(), token).

   Prompt To Fix With AI

   This is a comment left during a code review.
   Path: handler/oauth2.go
   Line: 293-298
   
   Comment:
   **`Validate()` does not check `Provider` for nil**
   
   `Provider` is the single required field unique to `OAuth2Handler`, yet `Validate()` skips it. If a caller forgets to set `Provider` and still calls `h.Validate()` (which returns nil), the server starts cleanly — then panics with a nil pointer dereference the first time `Callback` reaches `h.Provider.FetchUserInfo(r.Context(), token)`.
   
   
   
   How can I resolve this? If you propose a fix, please make it concise.

   

Prompt To Fix All With AI

Fix the following 1 code review issue. Work through them one at a time, proposing concise fixes.

---

### Issue 1 of 1
handler/oauth2_common.go:53-60
**Race-retry swallows non-`ErrNotFound` DB errors**

In the race-retry block, both `FindByOIDCSubject` and `FindByEmail` use bare `err == nil` guards, so any transient DB error (e.g. connection timeout) silently falls through to the generic `"failed to resolve OIDC user"` sentinel, discarding the real cause. Callers get a 500 with no actionable log message — compare to the first-pass lookups on lines 36–44, which properly return non-`ErrNotFound` errors.

```suggestion
	if u, err := users.FindByOIDCSubject(ctx, subject); err == nil {
		return u, nil
	} else if !errors.Is(err, auth.ErrNotFound) {
		return nil, fmt.Errorf("race-retry find by subject: %w", err)
	}
	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("race-retry find by email: %w", err)
	}
	return nil, fmt.Errorf("failed to resolve OIDC user after race retry")
```

_{Reviews (3): Last reviewed commit: "fix(handler): address PR review comments..." | Re-trigger Greptile}

[Copilot]

Pull request overview

Adds a generic OAuth2-based authentication handler (for non-OIDC providers like GitHub) and refactors OIDC linking/login logic into shared helpers so both OIDC and OAuth2 flows use the same user-resolution and account-linking implementation.

Changes:

• Introduces OAuth2Handler with login/callback/linking flows driven by a pluggable OAuth2IdentityProvider.
• Adds shared helper functions (findOrCreateUser, link state signing/parsing, nonce consumption, shared link callback handling) and refactors OIDCHandler to delegate to them.
• Adds built-in OAuth2 providers (GitHub + Google userinfo fallback), comprehensive OAuth2 tests, and new/updated handler documentation.

Show a summary per file

File	Description
handler/oauth2.go	New generic OAuth2 handler implementing login, callback, and account-linking flows.
handler/oauth2_common.go	New shared helpers extracted for reuse between OIDC and OAuth2 flows.
handler/oauth2_providers.go	New built-in OAuth2 identity providers (GitHub, Google userinfo fallback).
handler/oauth2_test.go	New OAuth2 handler test suite covering validation, login/callback/linking flows.
handler/oidc.go	Refactor to delegate linking/user-resolution logic to shared helpers.
handler/oidc_test.go	Fixes a broken linking test to use the nonce store abstraction.
docs/handler/oauth2.md	New OAuth2Handler documentation and behavior/status-code details.
docs/handler/oidc.md	Corrects documented meaning of link error redirect values.

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.go:128

• The Callback doc comment says “All error responses are JSON”, but in link flows handleLinkCallback(...) redirects on failure via /?oauth2_link_error=.... Consider updating the comment to reflect that link-flow outcomes are communicated via redirects while non-link failures return JSON.

// Callback handles the OAuth2 provider redirect. It validates the CSRF state
// and PKCE verifier, exchanges the authorisation code, fetches the user's
// identity via Provider.FetchUserInfo, and issues JWT/session cookies.
//
// On success it redirects to "/?<LoginRedirect>". All error responses are JSON.
func (h *OAuth2Handler) Callback(w http.ResponseWriter, r *http.Request) {

• Files reviewed: 6/6 changed files
• Comments generated: 3