feat(security): implement OAuth2 authorization server with database s…#18
feat(security): implement OAuth2 authorization server with database s…#18
Conversation
…upport - Add OAuthServer for handling OAuth2 flows including authorization, token exchange, and client registration. - Introduce DatabaseAuthenticator for persisting clients and authorization codes. - Implement SQL procedures for client registration, code saving, and token introspection. - Support for external OAuth2 providers and PKCE (Proof Key for Code Exchange).
There was a problem hiding this comment.
Pull request overview
Implements an OAuth 2.1 + PKCE authorization server within pkg/security, adds database persistence (clients, auth codes, token introspection/revocation) via stored procedures, and wires the server into pkg/resolvemcp so MCP clients can auto-discover and authenticate.
Changes:
- Added
security.OAuthServerHTTP handlers for metadata, dynamic client registration, authorization, token exchange, revocation, and introspection. - Introduced
DatabaseAuthenticatorpersistence methods + SQL schema additions for OAuth clients/codes and token introspection/revocation. - Integrated the OAuth server into
resolvemcp.Handlerand expanded documentation.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 20 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/security/sql_names.go | Adds SQLNames entries for new OAuth server stored procedures. |
| pkg/security/README.md | Documents OAuthServer endpoints, config, persistence, and usage patterns. |
| pkg/security/oauth_server.go | New OAuth 2.1 + PKCE authorization server implementation and HTTP routing. |
| pkg/security/oauth_server_db.go | Adds DatabaseAuthenticator methods for OAuth server persistence/introspection/revocation. |
| pkg/security/database_schema.sql | Adds oauth_clients/oauth_codes tables and stored procedures for OAuth server persistence + introspection/revocation. |
| pkg/resolvemcp/README.md | Documents MCP OAuth2 auth setup and modes for resolvemcp. |
| pkg/resolvemcp/oauth2.go | Adds handler wiring for OAuth server + legacy OAuth2 cookie-flow routes and auth-wrapped transports. |
| pkg/resolvemcp/oauth2_server.go | Adds EnableOAuthServer and provider registration on the resolvemcp.Handler. |
| pkg/resolvemcp/handler.go | Extends Handler struct to store OAuth server + OAuth2 registrations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func oauthSliceContains(slice []string, s string) bool { | ||
| for _, v := range slice { | ||
| if strings.EqualFold(v, s) { | ||
| return true | ||
| } | ||
| } | ||
| return false |
There was a problem hiding this comment.
redirect_uri matching should be an exact, case-sensitive string comparison per OAuth2 (RFC 6749). Using strings.EqualFold in oauthSliceContains can allow non-identical redirect URIs (e.g., path case changes) to pass validation, weakening redirect URI verification.
pkg/security/oauth_server.go
Outdated
| func writeOAuthToken(w http.ResponseWriter, accessToken, refreshToken string, scopes []string) { | ||
| resp := map[string]interface{}{ | ||
| "access_token": accessToken, | ||
| "token_type": "Bearer", | ||
| "expires_in": 86400, | ||
| } | ||
| if refreshToken != "" { | ||
| resp["refresh_token"] = refreshToken | ||
| } | ||
| if len(scopes) > 0 { | ||
| resp["scope"] = strings.Join(scopes, " ") | ||
| } | ||
| w.Header().Set("Content-Type", "application/json") | ||
| w.Header().Set("Cache-Control", "no-store") | ||
| w.Header().Set("Pragma", "no-cache") | ||
| json.NewEncoder(w).Encode(resp) //nolint:errcheck | ||
| } |
There was a problem hiding this comment.
writeOAuthToken hardcodes expires_in=86400, ignoring OAuthServerConfig.AccessTokenTTL (and any underlying session/token TTL). This can cause clients to cache tokens longer/shorter than they are actually valid; compute expires_in from the configured TTL or the authenticated session’s expiry.
| CodeChallengeMethod string | ||
| ProviderName string // empty = password login | ||
| ExpiresAt time.Time | ||
| SessionToken string // set after authentication completes |
There was a problem hiding this comment.
pendingAuth only persists SessionToken and Scopes. Since OAuth2HandleCallback/Login can return a RefreshToken, the authorization_code exchange currently cannot return a refresh_token and the refresh_token grant becomes unusable. Persist the refresh token (or enough info to mint/lookup one) alongside the auth code and return it from /oauth/token.
| SessionToken string // set after authentication completes | |
| SessionToken string // set after authentication completes | |
| RefreshToken string // set after authentication completes when refresh tokens are issued |
| if s.auth != nil { | ||
| s.auth.OAuthRevokeToken(r.Context(), token) //nolint:errcheck | ||
| } | ||
| w.WriteHeader(http.StatusOK) |
There was a problem hiding this comment.
revokeHandler only revokes when s.auth != nil. In external-provider-only mode (auth passed as nil), /oauth/revoke becomes a no-op even though issued access tokens are local session tokens stored in DB by provider authenticators. Consider allowing revocation via a persistence authenticator (e.g., first provider auth) or require a non-nil DB auth even when using only external providers.
pkg/security/oauth_server.go
Outdated
| if token == "" || s.auth == nil { | ||
| w.Write([]byte(`{"active":false}`)) //nolint:errcheck | ||
| return | ||
| } | ||
|
|
||
| info, err := s.auth.OAuthIntrospectToken(r.Context(), token) | ||
| if err != nil { | ||
| w.Write([]byte(`{"active":false}`)) //nolint:errcheck | ||
| return | ||
| } | ||
| json.NewEncoder(w).Encode(info) //nolint:errcheck | ||
| } |
There was a problem hiding this comment.
introspectHandler returns {"active":false} whenever s.auth == nil. In external-provider-only mode, this makes RFC 7662 introspection unusable even for valid session tokens created by external providers. Consider routing introspection to a configured persistence DatabaseAuthenticator (or a provider authenticator) rather than requiring s.auth.
pkg/resolvemcp/oauth2.go
Outdated
| // | ||
| // auth := security.NewGoogleAuthenticator(...) | ||
| // handler.RegisterOAuth2(auth, cfg) | ||
| // handler.EnableOAuthServer(resolvemcp.OAuthServerConfig{Issuer: "https://api.example.com"}) |
There was a problem hiding this comment.
The HTTPHandler doc comment example calls EnableOAuthServer with resolvemcp.OAuthServerConfig, but the actual API takes security.OAuthServerConfig. This is misleading for users and will not compile as written; please update the example type/package reference.
| // handler.EnableOAuthServer(resolvemcp.OAuthServerConfig{Issuer: "https://api.example.com"}) | |
| // handler.EnableOAuthServer(security.OAuthServerConfig{Issuer: "https://api.example.com"}) |
pkg/security/database_schema.sql
Outdated
| CREATE TABLE IF NOT EXISTS oauth_codes ( | ||
| id SERIAL PRIMARY KEY, | ||
| code VARCHAR(255) NOT NULL UNIQUE, | ||
| client_id VARCHAR(255) NOT NULL REFERENCES oauth_clients(client_id) ON DELETE CASCADE, |
There was a problem hiding this comment.
oauth_codes.client_id has a FOREIGN KEY to oauth_clients(client_id). This means codes cannot be persisted unless the corresponding client row exists in oauth_clients. If the application allows PersistCodes without PersistClients (as OAuthServerConfig does), authorization will fail at code-save time due to the FK constraint. Consider documenting/enforcing that PersistCodes requires PersistClients, or relaxing the FK if you intend codes to outlive in-memory client registrations.
| CREATE TABLE IF NOT EXISTS oauth_codes ( | |
| id SERIAL PRIMARY KEY, | |
| code VARCHAR(255) NOT NULL UNIQUE, | |
| client_id VARCHAR(255) NOT NULL REFERENCES oauth_clients(client_id) ON DELETE CASCADE, | |
| -- Note: client_id is stored without a foreign key so codes can be persisted even | |
| -- when OAuth clients are managed in memory rather than persisted in oauth_clients. | |
| CREATE TABLE IF NOT EXISTS oauth_codes ( | |
| id SERIAL PRIMARY KEY, | |
| code VARCHAR(255) NOT NULL UNIQUE, | |
| client_id VARCHAR(255) NOT NULL, |
pkg/security/oauth_server.go
Outdated
| ProviderCallbackPath string | ||
|
|
||
| // LoginTitle is shown on the built-in login form when the server acts as its own | ||
| // identity provider. Defaults to "MCP Login". |
There was a problem hiding this comment.
OAuthServerConfig.LoginTitle doc comment says it defaults to "MCP Login", but NewOAuthServer sets it to "Sign in". Please align the comment (or the default) so users aren’t misled.
| // identity provider. Defaults to "MCP Login". | |
| // identity provider. Defaults to "Sign in". |
| | `GET` | `/oauth/authorize` | OAuth 2.1 — start authorization / provider selection | | ||
| | `POST` | `/oauth/authorize` | OAuth 2.1 — login form submission | | ||
| | `POST` | `/oauth/token` | OAuth 2.1 — code exchange + refresh | | ||
| | `POST` | `/oauth/revoke` | RFC 7009 — token revocation | | ||
| | `POST` | `/oauth/introspect` | RFC 7662 — token introspection | | ||
| | `GET` | `{ProviderCallbackPath}` | External provider redirect target | | ||
|
|
||
| ### Config | ||
|
|
||
| ```go | ||
| cfg := security.OAuthServerConfig{ | ||
| Issuer: "https://example.com", // Required — token issuer URL | ||
| ProviderCallbackPath: "/oauth/provider/callback", // External provider redirect target | ||
| LoginTitle: "My App Login", // HTML login page title | ||
| PersistClients: true, // Store clients in DB (multi-instance safe) | ||
| PersistCodes: true, // Store codes in DB (multi-instance safe) | ||
| DefaultScopes: []string{"openid", "profile"}, // Returned when no scope requested | ||
| AccessTokenTTL: time.Hour, | ||
| AuthCodeTTL: 5 * time.Minute, | ||
| } | ||
| ``` | ||
|
|
||
| | Field | Default | Notes | | ||
| |-------|---------|-------| | ||
| | `Issuer` | — | Required | | ||
| | `ProviderCallbackPath` | `/oauth/provider/callback` | | | ||
| | `LoginTitle` | `"Login"` | | | ||
| | `PersistClients` | `false` | Set `true` for multi-instance | | ||
| | `PersistCodes` | `false` | Set `true` for multi-instance | | ||
| | `DefaultScopes` | `nil` | | | ||
| | `AccessTokenTTL` | `1h` | | | ||
| | `AuthCodeTTL` | `5m` | | | ||
|
|
||
| ### Operating Modes | ||
|
|
||
| **Mode 1 — Direct login (username/password form)** | ||
|
|
||
| Pass a `*DatabaseAuthenticator` to `NewOAuthServer`. The server renders a login form at `GET /oauth/authorize` and issues tokens via the stored session after login. | ||
|
|
||
| ```go | ||
| auth := security.NewDatabaseAuthenticator(db) | ||
| srv := security.NewOAuthServer(cfg, auth) | ||
| ``` | ||
|
|
||
| **Mode 2 — External provider federation** | ||
|
|
||
| Pass `nil` as auth and register external providers. The authorize page shows a provider selection UI. | ||
|
|
||
| ```go | ||
| srv := security.NewOAuthServer(cfg, nil) | ||
| srv.RegisterExternalProvider(googleAuth, "google") | ||
| srv.RegisterExternalProvider(githubAuth, "github") | ||
| ``` |
There was a problem hiding this comment.
Docs say the authorize page “shows a provider selection UI” when external providers are registered, but oauth_server.go currently redirects immediately to the first (or specified) provider and never renders a selection page. Either implement the documented selection UI or update the docs to reflect the current redirect behavior (e.g., require a provider query param to choose).
| // RegisterExternalProvider adds an external OAuth2 provider (Google, GitHub, Microsoft, etc.) | ||
| // that handles user authentication via redirect. The DatabaseAuthenticator must have been | ||
| // configured with WithOAuth2(providerName, ...) before calling this. | ||
| // Multiple providers can be registered; the first is used as the default. | ||
| func (s *OAuthServer) RegisterExternalProvider(auth *DatabaseAuthenticator, providerName string) { | ||
| s.providers = append(s.providers, externalProvider{auth: auth, providerName: providerName}) | ||
| } |
There was a problem hiding this comment.
RegisterExternalProvider appends to s.providers without any synchronization, while request handlers read s.providers concurrently. If providers can be registered after the server starts serving, this is a data race. Either document/enforce “register providers before serving” or protect providers with the existing mutex (or a separate one).
…feature-auth-mcp
|
@copilot Fix the issues you mentioned. Keep in mind, I did update code |
Agent-Logs-Url: https://github.com/bitechdev/ResolveSpec/sessions/e886b781-c910-425f-aa6f-06d13c46dcc7 Co-authored-by: warkanum <208308+warkanum@users.noreply.github.com>
…issuer normalization Agent-Logs-Url: https://github.com/bitechdev/ResolveSpec/sessions/e886b781-c910-425f-aa6f-06d13c46dcc7 Co-authored-by: warkanum <208308+warkanum@users.noreply.github.com>
All review issues have been addressed in commits 850ad2b and ca0545e:
|
…upport