From 67c3a1d3a0387a6ddea2488526511306f6a4b0b7 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Wed, 14 Oct 2026 14:18:48 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=A4=96=20fix:=20fail=20closed=20on=20a=20?= =?UTF-8?q?missing=20CoderMCPServer=20access=20group?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When a name in spec.access.groups does not resolve in Coder (GroupNotFound), the ACL sync now still removes every entry that the list does not name, by the group names in Coder's ACL answer, and every user entry. It adds nothing, so access never widens while a group is missing. A generation that also disables the server sends one PATCH with only enabled: false. The rest of the generation and the missing groups follow once the groups exist. The access.groups field doc says so, and the dormant CRD is regenerated. Fixes #313 Refs #155 Change-Id: I9a98f53a214e5ed6049b01abf64e703015600a8b Signed-off-by: Thomas Kosiewski --- api/v1alpha1/codermcpserver_types.go | 5 +- .../dormant/coder.com_codermcpservers.yaml | 5 +- internal/controller/codermcpserver_acl.go | 29 ++++++-- .../controller/codermcpserver_acl_test.go | 41 +++++++++-- internal/controller/codermcpserver_apply.go | 25 +++++++ .../controller/codermcpserver_apply_test.go | 72 ++++++++++++++++++- 6 files changed, 163 insertions(+), 14 deletions(-) diff --git a/api/v1alpha1/codermcpserver_types.go b/api/v1alpha1/codermcpserver_types.go index 8d4478c0b..5a4d34640 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 47f1654d7..d653518bd 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 01c9e1be5..89d1e1e72 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 f9a3b0047..af82f3916 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 1493fa50f..da810f65f 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 8e5af3731..498d46b21 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) + }) + } +}