🤖 fix: fail closed on a missing CoderMCPServer access group

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

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

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.

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)
+}

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 }
 

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)
+		})
+	}
+}