mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
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.
137 lines
3.7 KiB
Go
137 lines
3.7 KiB
Go
package mcpclient_test
|
|
|
|
import (
|
|
"context"
|
|
"database/sql"
|
|
"encoding/json"
|
|
"net/http"
|
|
"net/http/httptest"
|
|
"strings"
|
|
"testing"
|
|
"time"
|
|
|
|
"github.com/google/uuid"
|
|
"github.com/stretchr/testify/require"
|
|
"golang.org/x/oauth2"
|
|
"golang.org/x/xerrors"
|
|
|
|
"cdr.dev/slog/v3/sloggers/slogtest"
|
|
"github.com/coder/coder/v2/coderd/database"
|
|
"github.com/coder/coder/v2/coderd/x/chatd/mcpclient"
|
|
)
|
|
|
|
func TestIsPermanentRefreshError(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
retrieveErr := func(code string, status int) error {
|
|
body, err := json.Marshal(map[string]string{"error": code})
|
|
require.NoError(t, err)
|
|
return &oauth2.RetrieveError{
|
|
Response: &http.Response{StatusCode: status},
|
|
Body: body,
|
|
ErrorCode: code,
|
|
}
|
|
}
|
|
|
|
cases := []struct {
|
|
name string
|
|
err error
|
|
permanent bool
|
|
}{
|
|
{"InvalidGrant", retrieveErr("invalid_grant", http.StatusBadRequest), true},
|
|
{"BadRefreshToken", retrieveErr("bad_refresh_token", http.StatusOK), true},
|
|
{"WrappedInvalidGrant", xerrors.Errorf("refresh: %w", retrieveErr("invalid_grant", http.StatusBadRequest)), true},
|
|
{"InvalidClient", retrieveErr("invalid_client", http.StatusUnauthorized), false},
|
|
{"UnauthorizedClient", retrieveErr("unauthorized_client", http.StatusBadRequest), false},
|
|
{"ServerError", retrieveErr("", http.StatusInternalServerError), false},
|
|
{"RateLimited", retrieveErr("", http.StatusTooManyRequests), false},
|
|
{"PlainError", xerrors.New("connection refused"), false},
|
|
{"Nil", nil, false},
|
|
}
|
|
for _, tc := range cases {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
t.Parallel()
|
|
require.Equal(t, tc.permanent, mcpclient.IsPermanentRefreshError(tc.err))
|
|
})
|
|
}
|
|
}
|
|
|
|
func TestRefreshFailureReason(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
require.Equal(t, "boom", mcpclient.RefreshFailureReason(xerrors.New("boom")))
|
|
|
|
long := strings.Repeat("x", 1000)
|
|
reason := mcpclient.RefreshFailureReason(xerrors.New(long))
|
|
require.Len(t, reason, 400)
|
|
}
|
|
|
|
func TestRefreshOAuth2TokenInvalidGrant(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
tokenSrv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
|
|
w.Header().Set("Content-Type", "application/json")
|
|
w.WriteHeader(http.StatusBadRequest)
|
|
_, _ = w.Write([]byte(`{"error":"invalid_grant","error_description":"grant revoked"}`))
|
|
}))
|
|
defer tokenSrv.Close()
|
|
|
|
cfg := database.MCPServerConfig{
|
|
OAuth2ClientID: "cid",
|
|
OAuth2TokenURL: tokenSrv.URL,
|
|
}
|
|
tok := database.MCPServerUserToken{
|
|
AccessToken: "expired",
|
|
RefreshToken: "refresh",
|
|
TokenType: "Bearer",
|
|
Expiry: sql.NullTime{Time: time.Now().Add(-time.Hour), Valid: true},
|
|
}
|
|
|
|
_, err := mcpclient.RefreshOAuth2Token(context.Background(), cfg, tok)
|
|
require.Error(t, err)
|
|
require.True(t, mcpclient.IsPermanentRefreshError(err))
|
|
}
|
|
|
|
func TestBuildAuthHeadersSkipsFailedToken(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
logger := slogtest.Make(t, nil)
|
|
cfg := database.MCPServerConfig{
|
|
ID: uuid.New(),
|
|
Slug: "revoked",
|
|
AuthType: "oauth2",
|
|
}
|
|
|
|
t.Run("FailureReasonSet", func(t *testing.T) {
|
|
t.Parallel()
|
|
headers := mcpclient.BuildAuthHeadersForTest(
|
|
context.Background(), logger, cfg,
|
|
map[uuid.UUID]database.MCPServerUserToken{
|
|
cfg.ID: {
|
|
MCPServerConfigID: cfg.ID,
|
|
AccessToken: "leftover",
|
|
OauthRefreshFailureReason: "invalid_grant",
|
|
},
|
|
},
|
|
uuid.New(), nil,
|
|
)
|
|
require.NotContains(t, headers, "Authorization")
|
|
})
|
|
|
|
t.Run("HealthyToken", func(t *testing.T) {
|
|
t.Parallel()
|
|
headers := mcpclient.BuildAuthHeadersForTest(
|
|
context.Background(), logger, cfg,
|
|
map[uuid.UUID]database.MCPServerUserToken{
|
|
cfg.ID: {
|
|
MCPServerConfigID: cfg.ID,
|
|
AccessToken: "valid",
|
|
TokenType: "Bearer",
|
|
},
|
|
},
|
|
uuid.New(), nil,
|
|
)
|
|
require.Equal(t, "Bearer valid", headers["Authorization"])
|
|
})
|
|
}
|