diff --git a/api/v1alpha1/codermcpserver_types.go b/api/v1alpha1/codermcpserver_types.go index 8d4478c0..5a4d3464 100644 --- a/api/v1alpha1/codermcpserver_types.go +++ b/api/v1alpha1/codermcpserver_types.go @@ -258,7 +258,10 @@ type CoderMCPServerOAuth2Auth struct { type CoderMCPServerAccess struct { // Groups are Coder group names in the server's organization. Everyone is // the group of all organization members. An empty list leaves the server - // to organization admins. Coder reserves the names new and create. + // to organization admins. Coder reserves the names new and create. While a + // listed group does not exist, the controller removes the access of every + // group that this list does not name, adds none, disables the server if + // the spec disables it, and waits with GroupNotFound for the rest. // +kubebuilder:validation:MaxItems=64 // +kubebuilder:validation:XValidation:rule="self.all(g, g != 'new' && g != 'create')",message="access.groups must not hold new or create" // +kubebuilder:validation:items:MinLength=1 diff --git a/config/crd/dormant/coder.com_codermcpservers.yaml b/config/crd/dormant/coder.com_codermcpservers.yaml index 47f1654d..d653518b 100644 --- a/config/crd/dormant/coder.com_codermcpservers.yaml +++ b/config/crd/dormant/coder.com_codermcpservers.yaml @@ -76,7 +76,10 @@ spec: description: |- Groups are Coder group names in the server's organization. Everyone is the group of all organization members. An empty list leaves the server - to organization admins. Coder reserves the names new and create. + to organization admins. Coder reserves the names new and create. While a + listed group does not exist, the controller removes the access of every + group that this list does not name, adds none, disables the server if + the spec disables it, and waits with GroupNotFound for the rest. items: maxLength: 255 minLength: 1 diff --git a/internal/controller/codermcpserver_acl.go b/internal/controller/codermcpserver_acl.go index 01c9e1be..89d1e1e7 100644 --- a/internal/controller/codermcpserver_acl.go +++ b/internal/controller/codermcpserver_acl.go @@ -32,6 +32,12 @@ var mcpServerGroupNamePattern = regexp.MustCompile(`^[a-zA-Z0-9]+(?:-[a-zA-Z0-9] // it reads the ACL, and sends one sparse PATCH only when the ACL differs. A // nil access means the controller does not own the ACL, so it sends nothing. // It returns nil when the ACL matches. +// +// When a name does not resolve (GroupNotFound), it fails closed (#313): it +// still removes every entry whose group name access.groups does not list, +// by the names in Coder's ACL answer, and every user entry. It adds nothing, +// so access never widens while a group is missing, and it returns the +// GroupNotFound step. The missing groups are added once they exist. func syncMCPServerACL( ctx context.Context, sdk *codersdk.Client, orgID, configID uuid.UUID, access *coderv1alpha1.CoderMCPServerAccess, ) (*mcpServerStep, error) { @@ -59,9 +65,15 @@ func syncMCPServerACLPass( return nil, nil } want, step := resolveMCPServerGroups(ctx, sdk, orgID, configID, access.Groups) - if step != nil { + if step != nil && step.reason != "GroupNotFound" { return step, nil } + // A missing group: keep only the entries that access.groups names (#313). + byName := step != nil + listed := make(map[string]bool, len(access.Groups)) + for _, name := range access.Groups { + listed[name] = true + } acl, err := sdk.MCPServerConfigACL(ctx, orgID, configID) if err != nil { return mcpServerWait(mcpServerRetry, "CoderUnavailable", "get the ACL of MCP server config %s: %s", configID, coderErrorSummary(err)), nil @@ -72,14 +84,21 @@ func syncMCPServerACLPass( return mcpServerStalled(mcpServerResync, "CoderAnswerMismatch", "assertion failed: Coder answered an ACL group %s of organization %s for MCP server config %s", g.ID, g.OrganizationID, configID), nil } - if _, ok := want[g.ID]; !ok { + keep := false + if byName { + // Everyone has the organization ID (mcpServerEveryoneGroup). + keep = g.ID == orgID && listed[mcpServerEveryoneGroup] || g.ID != orgID && listed[g.Name] + } else { + _, keep = want[g.ID] + } + if !keep { patch.GroupRoles[g.ID.String()] = codersdk.MCPServerConfigRoleDeleted } else if g.Role != codersdk.MCPServerConfigRoleRead { patch.GroupRoles[g.ID.String()] = codersdk.MCPServerConfigRoleRead } delete(want, g.ID) } - if !removeOnly { + if !removeOnly && !byName { for id := range want { patch.GroupRoles[id.String()] = codersdk.MCPServerConfigRoleRead } @@ -92,7 +111,7 @@ func syncMCPServerACLPass( patch.UserRoles[u.ID.String()] = codersdk.MCPServerConfigRoleDeleted } if len(patch.GroupRoles) == 0 && len(patch.UserRoles) == 0 { - return nil, nil + return step, nil } err = sdk.UpdateMCPServerConfigACL(ctx, orgID, configID, patch) switch { @@ -104,7 +123,7 @@ func syncMCPServerACLPass( // A sparse PATCH sets absolute roles, so sending it again is safe. return mcpServerWait(mcpServerRetry, "CoderUnavailable", "update the ACL of MCP server config %s: %s", configID, coderErrorSummary(err)), nil } - return nil, nil + return step, nil } // resolveMCPServerGroups maps group names to IDs. Everyone needs no call. diff --git a/internal/controller/codermcpserver_acl_test.go b/internal/controller/codermcpserver_acl_test.go index f9a3b004..af82f391 100644 --- a/internal/controller/codermcpserver_acl_test.go +++ b/internal/controller/codermcpserver_acl_test.go @@ -115,8 +115,10 @@ func TestMCPServerACLNotOwned(t *testing.T) { require.Equal(t, before, env.fake.totalRequests(), "no read and no write") } -// A name that resolves to no group, or that Coder's search rejects, stops -// the sync before the ACL read, so nothing is written. +// A name that resolves to no group, or that Coder's search rejects, gives +// GroupNotFound. The sync fails closed (#313): it still removes every entry +// that access.groups does not name, here the default Everyone entry and the +// user entry, and it adds nothing. func TestMCPServerACLGroupNotFound(t *testing.T) { t.Parallel() for _, tc := range []struct { @@ -152,7 +154,8 @@ func TestMCPServerACLGroupNotFound(t *testing.T) { require.True(t, got.Stalled) require.Equal(t, tc.requeue, got.Requeue) require.Contains(t, got.Message, tc.message) - require.Equal(t, mcpACLCalls{available: tc.available}, calls, "no ACL read and no write") + require.Equal(t, mcpACLCalls{available: tc.available, get: 1, patch: 1}, calls, "one ACL read and one remove-only write") + env.requireACL(t) }) } } @@ -163,13 +166,13 @@ func TestMCPServerACLRejectsNamesOutsideThePattern(t *testing.T) { t.Parallel() env := newMCPACLEnv(t) for _, name := range []string{"data science", "role:admin", "50%", "data_science", `data\`, `"data"`, "-data", "data-", strings.Repeat("a", 256), ""} { - before := env.fake.totalRequests() + before := env.calls().available got, err := controller.SyncMCPServerACL(context.Background(), env.sdk, env.orgID, env.configID, &coderv1alpha1.CoderMCPServerAccess{Groups: []string{"data", name}}) require.NoError(t, err) require.Equal(t, "GroupNotFound", got.Reason, name) require.True(t, got.Stalled) require.Zero(t, got.Requeue, "only a spec change helps") - require.Equal(t, 1, env.fake.totalRequests()-before, "only the search for data ran: %q", name) + require.Equal(t, 1, env.calls().available-before, "only the search for data ran: %q", name) } got, _ := env.sync(t, strings.Repeat("a", 255)) require.Equal(t, "GroupNotFound", got.Reason) @@ -302,3 +305,31 @@ func TestMCPServerACLComparesSets(t *testing.T) { bodies := env.fake.mcpRequestBodies(routeUpdateMCPServerACL) require.NotContains(t, string(bodies[len(bodies)-1]), `""`, "the PATCH sets read and removes nothing") } + +// #313: while a listed group is missing, the sync keeps the entries of the +// groups that access.groups names, by their names in Coder's ACL answer, +// removes the others, and adds no group. Once the group exists, the next +// sync adds it. +func TestMCPServerACLMissingGroupNeverWidens(t *testing.T) { + t.Parallel() + env := newMCPACLEnv(t) + got, _ := env.sync(t, "Everyone", "data", "analytics") + require.Empty(t, got.Reason) + env.requireACL(t, env.orgID, env.data, env.analytic) + + got, calls := env.sync(t, "data", "data-science", "missing") + require.Equal(t, "GroupNotFound", got.Reason) + require.Equal(t, mcpACLCalls{available: 3, get: 1, patch: 1}, calls) + env.requireACL(t, env.data) + patches := env.fake.mcpRequestBodies(routeUpdateMCPServerACL) + require.JSONEq(t, `{"group_roles":{"`+env.orgID.String()+`":"","`+env.analytic.String()+`":""}}`, string(patches[len(patches)-1]), + "only removes: data-science resolves, but nothing is added while missing is missing") + + _, calls = env.sync(t, "data", "data-science", "missing") + require.Equal(t, mcpACLCalls{available: 3, get: 1}, calls, "nothing left to remove") + + missing := env.fake.addGroup(env.orgID, "missing", "") + got, _ = env.sync(t, "data", "data-science", "missing") + require.Empty(t, got.Reason) + env.requireACL(t, env.data, env.science, missing) +} diff --git a/internal/controller/codermcpserver_apply.go b/internal/controller/codermcpserver_apply.go index 1493fa50..da810f65 100644 --- a/internal/controller/codermcpserver_apply.go +++ b/internal/controller/codermcpserver_apply.go @@ -96,6 +96,11 @@ func (r *CoderMCPServerReconciler) syncMCPServer( if req != (codersdk.UpdateMCPServerConfigRequest{}) { if kill == nil { if step, err := narrowMCPServerACL(ctx, sdk, orgID, id, server.Spec.Access); step != nil || err != nil { + if step != nil && step.reason == "GroupNotFound" && req.Enabled != nil && !*req.Enabled { + // Fail closed (#313): a generation that disables the + // server disables it now, while a group is missing. + return r.disableForMissingGroup(ctx, sdk, server, orgID, id, step), false, nil + } return step, false, err } } @@ -136,6 +141,26 @@ func (r *CoderMCPServerReconciler) syncMCPServer( return mcpServerReady(server), false, nil } +// disableForMissingGroup sends a PATCH that holds only enabled false, for a +// generation that disables the server while one of its access.groups does +// not resolve (#313). The generation stays unapplied, so the rest of it +// follows once the group exists. It returns the GroupNotFound step, or a +// wait when the PATCH failed. +func (r *CoderMCPServerReconciler) disableForMissingGroup( + ctx context.Context, sdk *codersdk.Client, server *coderv1alpha1.CoderMCPServer, orgID, id uuid.UUID, missing *mcpServerStep, +) *mcpServerStep { + disabled := false + updated, err := sdk.UpdateMCPServerConfig(ctx, orgID, id, codersdk.UpdateMCPServerConfigRequest{Enabled: &disabled}) + if err != nil { + return mcpServerWait(mcpServerRetry, missing.reason, "MCP server config %s is still enabled: the disable failed: %s. %s", id, coderErrorSummary(err), missing.message) + } + if updated.ID != id || updated.OrganizationID != orgID || updated.Slug != server.Spec.Slug || updated.Enabled { + return mcpServerWait(mcpServerRetry, "CoderAnswerMismatch", + "Coder answered the disable of MCP server config %s with another or an enabled config. The controller disables it again.", id) + } + return missing +} + // mcpServerCredentialSet says which credential fields an auth type uses. type mcpServerCredentialSet struct{ apiKey, headers, secret bool } diff --git a/internal/controller/codermcpserver_apply_test.go b/internal/controller/codermcpserver_apply_test.go index 8e5af373..498d46b2 100644 --- a/internal/controller/codermcpserver_apply_test.go +++ b/internal/controller/codermcpserver_apply_test.go @@ -166,7 +166,9 @@ func TestCoderMCPServerApplyAccess(t *testing.T) { } // A group that does not resolve keeps the new config disabled with the nonce, -// and sends no PATCH. Once the group exists, the next reconcile applies. +// and sends no config PATCH. The ACL fails closed (#313): the default +// Everyone entry goes, and nothing is added. Once the group exists, the next +// reconcile applies. func TestCoderMCPServerApplyGroupNotFound(t *testing.T) { t.Parallel() ctx := context.Background() @@ -179,7 +181,9 @@ func TestCoderMCPServerApplyGroupNotFound(t *testing.T) { stored := env.requireCreated(t, got) require.False(t, stored.view.Enabled) require.Equal(t, "coder-k8s:"+string(got.UID), stored.view.DisplayName) - require.Zero(t, env.fake.requestCount(routeUpdateMCPServer)+env.fake.requestCount(routeUpdateMCPServerACL)) + require.Zero(t, env.fake.requestCount(routeUpdateMCPServer)) + groups, _ := env.fake.mcpACL(stored.view.ID) + require.Empty(t, groups, "only the remove-only ACL write: Everyone goes") require.Zero(t, got.Status.AppliedGeneration) env.fake.addGroup(env.orgID, "data", "") @@ -386,3 +390,67 @@ func TestCoderMCPServerApplyAuthTypeChange(t *testing.T) { }) } } + +// #313: a new generation names a group that does not exist yet. The ACL +// fails closed: entries that the new access.groups does not list go, and +// nothing is added. When the same generation disables the server, one PATCH +// with only enabled false disables it now. The rest of the generation +// follows once the group exists. +func TestCoderMCPServerApplyMissingGroupFailsClosed(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + name string + disable bool + }{{name: "access only"}, {name: "also disabled", disable: true}} { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctx := context.Background() + env := newMCPServerEnv(ctx, t, true) + env.keySecret(ctx, t, testMCPKey) + data := env.fake.addGroup(env.orgID, "data", "") + got, result := env.reconcile(ctx, t, env.create(ctx, t, func(s *coderv1alpha1.CoderMCPServerSpec) { + s.Access = &coderv1alpha1.CoderMCPServerAccess{Groups: []string{"Everyone", "data"}} + })) + requireMCPServerApplied(t, got) + id := env.requireCreated(t, got).view.ID + + require.NoError(t, k8sClient.Get(ctx, client.ObjectKeyFromObject(got), got)) + got.Spec.Access.Groups = []string{"data", "ops"} + got.Spec.Description = "new" + if tc.disable { + got.Spec.Enabled = ptrTo(false) + } + require.NoError(t, k8sClient.Update(ctx, got)) + mark := len(env.writeRoutes()) + got, result = env.reconcile(ctx, t, got) + requireMCPServerStep(t, got, true, "GroupNotFound") + require.Equal(t, 10*time.Minute, result.RequeueAfter) + require.NotEqual(t, got.Generation, got.Status.AppliedGeneration) + groups, _ := env.fake.mcpACL(id) + require.Equal(t, []uuid.UUID{data}, groups, "Everyone goes, ops is not added") + writes := env.writesSince(mark) + stored := env.requireCreated(t, got) + require.Empty(t, stored.view.Description, "the rest of the generation waits") + if tc.disable { + require.Len(t, writes, 2) + require.Equal(t, routeUpdateMCPServerACL, writes[0].route) + require.Equal(t, routeUpdateMCPServer, writes[1].route) + require.JSONEq(t, `{"enabled":false}`, string(writes[1].body)) + require.False(t, stored.view.Enabled) + } else { + require.Len(t, writes, 1) + require.Equal(t, routeUpdateMCPServerACL, writes[0].route) + require.True(t, stored.view.Enabled) + } + + ops := env.fake.addGroup(env.orgID, "ops", "") + got, _ = env.reconcile(ctx, t, got) + requireMCPServerApplied(t, got) + groups, _ = env.fake.mcpACL(id) + require.ElementsMatch(t, []uuid.UUID{data, ops}, groups) + stored = env.requireCreated(t, got) + require.Equal(t, "new", stored.view.Description) + require.Equal(t, !tc.disable, stored.view.Enabled) + }) + } +}