mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
feat: handle revoked OAuth grants for MCP servers gracefully (#27264)
Closes [CODAGT-792](https://linear.app/codercom/issue/CODAGT-792/handle-revoked-oauth-grants-for-mcp-servers-gracefully). When a user revokes an upstream OAuth grant for an MCP server used by Coder Agents, Coder kept treating the cached token as valid: `invalid_grant` refresh failures were logged and swallowed, the dead bearer token kept being attached, the list endpoints re-attempted the refresh on every call, and the UI kept showing the server as authenticated. ## Changes Backend, mirroring the `external_auth_links` prior art: - New migration adds `mcp_server_user_tokens.oauth_refresh_failure_reason`. `UpsertMCPServerUserToken` clears it, so completing the OAuth flow again recovers the row. - New `MarkMCPServerUserTokenRefreshFailure` query records the failure and clears all token material, guarded by an `updated_at` optimistic lock so a stale failure never clobbers a concurrently refreshed token (on a lock miss the winner's row is used). - `mcpclient.IsPermanentRefreshError` classifies `*oauth2.RetrieveError` codes: only `invalid_grant` and `bad_refresh_token` are permanent. Client/config errors (`invalid_client`, `unauthorized_client`, ...) stay transient for the user row since reconnecting cannot fix them. - chatd token refresh and the MCP list/get endpoints persist permanent failures, return cleared tokens for the in-flight request, and skip provider calls for already-failed rows. - `buildAuthHeaders` no longer attaches an Authorization header for failed tokens, so chat degrades by omitting that server's tools instead of sending a dead bearer. API and UI: - No new API surface. A permanently failed token simply reports `auth_connected: false`, so the existing "Auth" button and "Not authenticated" tooltip appear and the user re-runs the same OAuth flow to recover. An earlier revision added an `auth_status` enum (`connected` / `not_connected` / `reconnect_required`) with a dedicated "Reconnect" button; it was collapsed to keep the API minimal since both states lead to the identical re-auth action. Out of scope (follow-up): typed 401-on-connect detection and forced refresh. mcp-go exposes no stable typed 401 signal in the static-header path, so a revocation while the access token still looks valid locally stays undetected until expiry triggers a refresh. ## Testing - Unit and integration tests: classifier, chatd refresh paths (permanent/transient/race/persist-failure), API endpoints (revoked, transient, no-retry caching, re-auth recovery, stale-lock), dbauthz, dbcrypt, migrations. - Dogfood UAT against a dev instance with a mock IdP returning `invalid_grant`: revoked grant detected on refresh and persisted once (no repeated IdP calls), chat with the revoked server selected completes with the server's tools omitted, and re-auth restores the connected state. > This PR was authored by Mux, working on Mike's behalf.
This commit is contained in:
Michael Suchacz
Generated
+57
-3
@@ -16864,7 +16864,7 @@ func (q *sqlQuerier) GetMCPServerConfigsByIDs(ctx context.Context, ids []uuid.UU
|
||||
|
||||
const getMCPServerUserToken = `-- name: GetMCPServerUserToken :one
|
||||
SELECT
|
||||
id, mcp_server_config_id, user_id, access_token, access_token_key_id, refresh_token, refresh_token_key_id, token_type, expiry, created_at, updated_at
|
||||
id, mcp_server_config_id, user_id, access_token, access_token_key_id, refresh_token, refresh_token_key_id, token_type, expiry, created_at, updated_at, oauth_refresh_failure_reason
|
||||
FROM
|
||||
mcp_server_user_tokens
|
||||
WHERE
|
||||
@@ -16892,13 +16892,14 @@ func (q *sqlQuerier) GetMCPServerUserToken(ctx context.Context, arg GetMCPServer
|
||||
&i.Expiry,
|
||||
&i.CreatedAt,
|
||||
&i.UpdatedAt,
|
||||
&i.OauthRefreshFailureReason,
|
||||
)
|
||||
return i, err
|
||||
}
|
||||
|
||||
const getMCPServerUserTokensByUserID = `-- name: GetMCPServerUserTokensByUserID :many
|
||||
SELECT
|
||||
id, mcp_server_config_id, user_id, access_token, access_token_key_id, refresh_token, refresh_token_key_id, token_type, expiry, created_at, updated_at
|
||||
id, mcp_server_config_id, user_id, access_token, access_token_key_id, refresh_token, refresh_token_key_id, token_type, expiry, created_at, updated_at, oauth_refresh_failure_reason
|
||||
FROM
|
||||
mcp_server_user_tokens
|
||||
WHERE
|
||||
@@ -16926,6 +16927,7 @@ func (q *sqlQuerier) GetMCPServerUserTokensByUserID(ctx context.Context, userID
|
||||
&i.Expiry,
|
||||
&i.CreatedAt,
|
||||
&i.UpdatedAt,
|
||||
&i.OauthRefreshFailureReason,
|
||||
); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
@@ -17098,6 +17100,54 @@ func (q *sqlQuerier) InsertMCPServerConfig(ctx context.Context, arg InsertMCPSer
|
||||
return i, err
|
||||
}
|
||||
|
||||
const markMCPServerUserTokenRefreshFailure = `-- name: MarkMCPServerUserTokenRefreshFailure :one
|
||||
UPDATE mcp_server_user_tokens
|
||||
SET
|
||||
access_token = '',
|
||||
access_token_key_id = NULL,
|
||||
refresh_token = '',
|
||||
refresh_token_key_id = NULL,
|
||||
expiry = NULL,
|
||||
oauth_refresh_failure_reason = $1::text,
|
||||
updated_at = NOW()
|
||||
WHERE
|
||||
id = $2::uuid
|
||||
AND updated_at = $3::timestamptz
|
||||
RETURNING
|
||||
id, mcp_server_config_id, user_id, access_token, access_token_key_id, refresh_token, refresh_token_key_id, token_type, expiry, created_at, updated_at, oauth_refresh_failure_reason
|
||||
`
|
||||
|
||||
type MarkMCPServerUserTokenRefreshFailureParams struct {
|
||||
OauthRefreshFailureReason string `db:"oauth_refresh_failure_reason" json:"oauth_refresh_failure_reason"`
|
||||
ID uuid.UUID `db:"id" json:"id"`
|
||||
UpdatedAt time.Time `db:"updated_at" json:"updated_at"`
|
||||
}
|
||||
|
||||
// Records a permanent refresh failure (e.g. revoked grant) and clears
|
||||
// the dead token material so it is never attached to a request again.
|
||||
// The updated_at predicate provides optimistic concurrency: if another
|
||||
// request refreshed or replaced the token since it was read, this
|
||||
// update matches zero rows and returns sql.ErrNoRows.
|
||||
func (q *sqlQuerier) MarkMCPServerUserTokenRefreshFailure(ctx context.Context, arg MarkMCPServerUserTokenRefreshFailureParams) (MCPServerUserToken, error) {
|
||||
row := q.db.QueryRowContext(ctx, markMCPServerUserTokenRefreshFailure, arg.OauthRefreshFailureReason, arg.ID, arg.UpdatedAt)
|
||||
var i MCPServerUserToken
|
||||
err := row.Scan(
|
||||
&i.ID,
|
||||
&i.MCPServerConfigID,
|
||||
&i.UserID,
|
||||
&i.AccessToken,
|
||||
&i.AccessTokenKeyID,
|
||||
&i.RefreshToken,
|
||||
&i.RefreshTokenKeyID,
|
||||
&i.TokenType,
|
||||
&i.Expiry,
|
||||
&i.CreatedAt,
|
||||
&i.UpdatedAt,
|
||||
&i.OauthRefreshFailureReason,
|
||||
)
|
||||
return i, err
|
||||
}
|
||||
|
||||
const updateMCPServerConfig = `-- name: UpdateMCPServerConfig :one
|
||||
UPDATE
|
||||
mcp_server_configs
|
||||
@@ -17258,9 +17308,12 @@ ON CONFLICT (mcp_server_config_id, user_id) DO UPDATE SET
|
||||
refresh_token_key_id = $6::text,
|
||||
token_type = $7::text,
|
||||
expiry = $8::timestamptz,
|
||||
-- New token material means the user re-authenticated, so any
|
||||
-- cached permanent refresh failure no longer applies.
|
||||
oauth_refresh_failure_reason = '',
|
||||
updated_at = NOW()
|
||||
RETURNING
|
||||
id, mcp_server_config_id, user_id, access_token, access_token_key_id, refresh_token, refresh_token_key_id, token_type, expiry, created_at, updated_at
|
||||
id, mcp_server_config_id, user_id, access_token, access_token_key_id, refresh_token, refresh_token_key_id, token_type, expiry, created_at, updated_at, oauth_refresh_failure_reason
|
||||
`
|
||||
|
||||
type UpsertMCPServerUserTokenParams struct {
|
||||
@@ -17298,6 +17351,7 @@ func (q *sqlQuerier) UpsertMCPServerUserToken(ctx context.Context, arg UpsertMCP
|
||||
&i.Expiry,
|
||||
&i.CreatedAt,
|
||||
&i.UpdatedAt,
|
||||
&i.OauthRefreshFailureReason,
|
||||
)
|
||||
return i, err
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user