mirror of
https://github.com/coder/coder.git
synced 2026-09-21 12:44:32 +08:00
fix: harden org-scoped MCP config chat gating, updates, and visibility (#28065)
Hardens the org-scoped MCP server config surface from #27942 with fixes and regression pins that are independent of the core cutover: - Keep chats sendable after a selected MCP server is disabled (persisted selections are exempt from message-time rejection). - Keep cached MCP selections usable after a background refetch error (gate the composer on missing data, not `isSuccess`). - Merge PATCH updates onto the current row so unset fields are not clobbered. - Give auditors the full management view of MCP configs their audit logs reference (site and org auditor roles). - Regression pins: frozen OAuth2 callback path, cross-org concealment for item routes, OAuth callback token binding, and disconnect responses indistinguishable from nonexistent config IDs. - Storybook: semantic textbox queries in MCP loading stories; docs note that the MCP settings page needs deployment access. ## Stack context Part of the MCP org-separation stack (CODAGT-711 org scope -> apidocs -> hardening -> CODAGT-717 audit -> CODAGT-712 ACLs -> CODAGT-806 token RBAC). Split out of #27942 to keep the core cutover reviewable; each change here builds on the org-scoped routes and chat gating introduced below it. > Mux (AI agent) authored this PR on Mike's behalf. <!-- mux-attribution: model=claude-fable-5 thinking=high -->
This commit is contained in:
+25
-6
@@ -2146,16 +2146,30 @@ func TestMCPServerOAuth2PKCE(t *testing.T) {
|
||||
require.NotEmpty(t, query.Get("code_challenge"),
|
||||
"connect redirect must include a code_challenge")
|
||||
|
||||
// A verifier cookie must be set.
|
||||
var verifierCookie *http.Cookie
|
||||
// The callback path is frozen because it is registered as a
|
||||
// redirect URI with external authorization servers.
|
||||
frozenCallbackPath := "/api/experimental/mcp/servers/" + created.ID.String() + "/oauth2/callback"
|
||||
redirectURI, err := url.Parse(query.Get("redirect_uri"))
|
||||
require.NoError(t, err)
|
||||
require.Equal(t, frozenCallbackPath, redirectURI.Path,
|
||||
"outbound redirect_uri must use the frozen callback path")
|
||||
|
||||
var stateCookie, verifierCookie *http.Cookie
|
||||
for _, c := range res.Cookies() {
|
||||
if c.Name == "mcp_oauth2_verifier_"+created.ID.String() {
|
||||
switch c.Name {
|
||||
case "mcp_oauth2_state_" + created.ID.String():
|
||||
stateCookie = c
|
||||
case "mcp_oauth2_verifier_" + created.ID.String():
|
||||
verifierCookie = c
|
||||
break
|
||||
}
|
||||
}
|
||||
require.NotNil(t, stateCookie, "response must set a state cookie")
|
||||
require.Equal(t, frozenCallbackPath, stateCookie.Path,
|
||||
"state cookie must be scoped to the frozen callback path")
|
||||
require.NotNil(t, verifierCookie, "response must set a PKCE verifier cookie")
|
||||
require.NotEmpty(t, verifierCookie.Value)
|
||||
require.Equal(t, frozenCallbackPath, verifierCookie.Path,
|
||||
"verifier cookie must be scoped to the frozen callback path")
|
||||
|
||||
// Verify the code_challenge matches SHA256(verifier).
|
||||
h := sha256.Sum256([]byte(verifierCookie.Value))
|
||||
@@ -2260,12 +2274,17 @@ func TestMCPServerOAuth2PKCE(t *testing.T) {
|
||||
"token exchange must send the PKCE code_verifier")
|
||||
|
||||
// Verify the verifier cookie is cleared in the response.
|
||||
var clearedVerifier *http.Cookie
|
||||
for _, c := range res.Cookies() {
|
||||
if c.Name == "mcp_oauth2_verifier_"+created.ID.String() {
|
||||
require.Equal(t, -1, c.MaxAge,
|
||||
"verifier cookie must be cleared after callback")
|
||||
clearedVerifier = c
|
||||
}
|
||||
}
|
||||
require.NotNil(t, clearedVerifier, "callback must clear the verifier cookie")
|
||||
require.Equal(t, -1, clearedVerifier.MaxAge,
|
||||
"verifier cookie must be cleared after callback")
|
||||
require.Equal(t, callbackURL.Path, clearedVerifier.Path,
|
||||
"cleared verifier cookie must be scoped to the frozen callback path")
|
||||
})
|
||||
|
||||
t.Run("CallbackWithoutVerifierStillWorks", func(t *testing.T) {
|
||||
|
||||
@@ -1159,8 +1159,7 @@ func OrgMemberPermissions(org OrgSettings) OrgRolePermissions {
|
||||
ResourceOrganization.Type: {policy.ActionRead},
|
||||
// Can read available roles.
|
||||
ResourceAssignOrgRole.Type: {policy.ActionRead},
|
||||
// TODO(mafredri): Remove once CODAGT-712 replaces this grant with
|
||||
// per-config ACL evaluation.
|
||||
// TODO(mafredri): remove once CODAGT-712 adds per-config ACL evaluation.
|
||||
ResourceMCPServerConfig.Type: {policy.ActionRead},
|
||||
}
|
||||
|
||||
@@ -1239,8 +1238,7 @@ func OrgServiceAccountPermissions(org OrgSettings) OrgRolePermissions {
|
||||
ResourceOrganization.Type: {policy.ActionRead},
|
||||
// Can read available roles.
|
||||
ResourceAssignOrgRole.Type: {policy.ActionRead},
|
||||
// TODO(mafredri): Remove once CODAGT-712 replaces this grant with
|
||||
// per-config ACL evaluation.
|
||||
// TODO(mafredri): remove once CODAGT-712 adds per-config ACL evaluation.
|
||||
ResourceMCPServerConfig.Type: {policy.ActionRead},
|
||||
}
|
||||
|
||||
|
||||
@@ -174,7 +174,9 @@ wins.
|
||||
| View enabled servers | Organization member |
|
||||
| OAuth2 connect and disconnect | Organization member |
|
||||
|
||||
Creating or updating a server with `auth_type` set to `user_oidc` also requires the `deployment_config:update` permission.
|
||||
|
||||
Members only see enabled servers in their own organizations. Sensitive fields
|
||||
such as API keys and client secrets are redacted in API responses.
|
||||
|
||||
The **MCP servers** settings page is part of deployment settings, so opening it in the dashboard also requires permission to edit deployment configuration.
|
||||
Organization admins without that permission can manage servers through the API.
|
||||
Creating or updating a server with `auth_type` set to `user_oidc` also requires the `deployment_config:update` permission.
|
||||
|
||||
@@ -1,13 +1,20 @@
|
||||
package coderd_test
|
||||
|
||||
import (
|
||||
"encoding/json"
|
||||
"fmt"
|
||||
"io"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
"testing"
|
||||
|
||||
"github.com/google/uuid"
|
||||
"github.com/stretchr/testify/require"
|
||||
|
||||
"github.com/coder/coder/v2/coderd/coderdtest"
|
||||
"github.com/coder/coder/v2/coderd/database"
|
||||
"github.com/coder/coder/v2/coderd/database/dbauthz"
|
||||
"github.com/coder/coder/v2/coderd/rbac"
|
||||
"github.com/coder/coder/v2/coderd/util/ptr"
|
||||
"github.com/coder/coder/v2/codersdk"
|
||||
"github.com/coder/coder/v2/enterprise/coderd/coderdenttest"
|
||||
@@ -135,4 +142,123 @@ func TestMCPServerConfigItemCrossOrganizationConcealment(t *testing.T) {
|
||||
requireMCPServerConfigRequestStatus(t, otherClient, test.method, test.path, test.body, wantStatus)
|
||||
})
|
||||
}
|
||||
|
||||
// Compare raw responses because SDK decoding can hide body differences
|
||||
// that reveal whether the config exists.
|
||||
t.Run("OAuthDisconnectBodyMatchesNonexistent", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
rawDisconnect := func(id uuid.UUID) (int, string) {
|
||||
res, err := otherClient.Request(ctx, http.MethodDelete,
|
||||
"/api/experimental/mcp/servers/"+id.String()+"/oauth2/disconnect", nil)
|
||||
require.NoError(t, err)
|
||||
defer res.Body.Close()
|
||||
body, err := io.ReadAll(res.Body)
|
||||
require.NoError(t, err)
|
||||
return res.StatusCode, string(body)
|
||||
}
|
||||
|
||||
hiddenStatus, hiddenBody := rawDisconnect(config.ID)
|
||||
missingStatus, missingBody := rawDisconnect(uuid.New())
|
||||
require.Equal(t, missingStatus, hiddenStatus)
|
||||
require.Equal(t, missingBody, hiddenBody)
|
||||
|
||||
var disconnect codersdk.MCPServerOAuth2DisconnectResponse
|
||||
require.NoError(t, json.Unmarshal([]byte(hiddenBody), &disconnect))
|
||||
require.False(t, disconnect.TokenRevoked)
|
||||
require.Empty(t, disconnect.TokenRevocationError)
|
||||
})
|
||||
}
|
||||
|
||||
func TestMCPServerConfigsOAuth2CallbackTokenBinding(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
client, db, firstUser := coderdenttest.NewWithDatabase(t, &coderdenttest.Options{
|
||||
LicenseOptions: &coderdenttest.LicenseOptions{
|
||||
Features: license.Features{
|
||||
codersdk.FeatureMultipleOrganizations: 1,
|
||||
},
|
||||
},
|
||||
})
|
||||
secondOrg := coderdenttest.CreateOrganization(t, client, coderdenttest.CreateOrganizationOptions{})
|
||||
memberClient, member := coderdtest.CreateAnotherUser(t, client, firstUser.OrganizationID, rbac.ScopedRoleOrgMember(secondOrg.ID))
|
||||
|
||||
newTokenServer := func(accessToken string) *httptest.Server {
|
||||
srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
|
||||
w.Header().Set("Content-Type", "application/json")
|
||||
_, _ = fmt.Fprintf(w,
|
||||
`{"access_token":%q,"token_type":"Bearer","expires_in":3600,"refresh_token":"refresh-%s"}`,
|
||||
accessToken, accessToken,
|
||||
)
|
||||
}))
|
||||
t.Cleanup(srv.Close)
|
||||
return srv
|
||||
}
|
||||
createOAuthConfig := func(organizationID uuid.UUID, tokenURL string) codersdk.MCPServerConfig {
|
||||
t.Helper()
|
||||
config, err := client.CreateMCPServerConfig(ctx, organizationID, codersdk.CreateMCPServerConfigRequest{
|
||||
DisplayName: "Callback Binding",
|
||||
Slug: "callback-binding",
|
||||
Transport: "streamable_http",
|
||||
URL: "https://mcp.example.com/callback-binding",
|
||||
AuthType: "oauth2",
|
||||
OAuth2ClientID: "client-" + organizationID.String(),
|
||||
OAuth2AuthURL: "https://auth.example.com/authorize",
|
||||
OAuth2TokenURL: tokenURL,
|
||||
Availability: "default_on",
|
||||
Enabled: true,
|
||||
ToolAllowList: []string{},
|
||||
ToolDenyList: []string{},
|
||||
})
|
||||
require.NoError(t, err)
|
||||
return config
|
||||
}
|
||||
completeCallback := func(config codersdk.MCPServerConfig) {
|
||||
t.Helper()
|
||||
state := "state-" + config.ID.String()
|
||||
callbackURL, err := memberClient.URL.Parse(
|
||||
"/api/experimental/mcp/servers/" + config.ID.String() + "/oauth2/callback",
|
||||
)
|
||||
require.NoError(t, err)
|
||||
query := callbackURL.Query()
|
||||
query.Set("code", "auth-code-"+config.ID.String())
|
||||
query.Set("state", state)
|
||||
callbackURL.RawQuery = query.Encode()
|
||||
req, err := http.NewRequestWithContext(ctx, http.MethodGet, callbackURL.String(), nil)
|
||||
require.NoError(t, err)
|
||||
req.AddCookie(&http.Cookie{Name: codersdk.SessionTokenCookie, Value: memberClient.SessionToken()})
|
||||
req.AddCookie(&http.Cookie{Name: "mcp_oauth2_state_" + config.ID.String(), Value: state})
|
||||
res, err := memberClient.HTTPClient.Do(req)
|
||||
require.NoError(t, err)
|
||||
defer res.Body.Close()
|
||||
require.Equal(t, http.StatusOK, res.StatusCode)
|
||||
}
|
||||
tokenRow := func(configID uuid.UUID) database.MCPServerUserToken {
|
||||
t.Helper()
|
||||
//nolint:gocritic // Verifying persisted state requires system access.
|
||||
row, err := db.GetMCPServerUserToken(dbauthz.AsSystemRestricted(ctx), database.GetMCPServerUserTokenParams{
|
||||
MCPServerConfigID: configID,
|
||||
UserID: member.ID,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
return row
|
||||
}
|
||||
|
||||
// The same slug in both organizations proves tokens bind to the config
|
||||
// ID, not the slug.
|
||||
firstConfig := createOAuthConfig(firstUser.OrganizationID, newTokenServer("org-one-access-token").URL)
|
||||
secondConfig := createOAuthConfig(secondOrg.ID, newTokenServer("org-two-access-token").URL)
|
||||
|
||||
completeCallback(firstConfig)
|
||||
firstToken := tokenRow(firstConfig.ID)
|
||||
require.Equal(t, "org-one-access-token", firstToken.AccessToken)
|
||||
|
||||
completeCallback(secondConfig)
|
||||
firstToken = tokenRow(firstConfig.ID)
|
||||
secondToken := tokenRow(secondConfig.ID)
|
||||
require.Equal(t, "org-one-access-token", firstToken.AccessToken)
|
||||
require.Equal(t, "org-two-access-token", secondToken.AccessToken)
|
||||
require.NotEqual(t, firstToken.ID, secondToken.ID)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user