feat(abac): add durable service and MCP IDs

api/api.go

@@ -70,11 +70,10 @@ type MCPClientManager interface {
 	GetConfig() mcp.Config
 
 	RegisterPluginServer(cfg mcp.PluginServerConfig)
-	UpdatePluginServer(cfg mcp.PluginServerConfig)
+	UpdatePluginServerAdminFields(pluginID string, enabled bool, toolConfigs []mcp.ToolConfig) (mcp.PluginServerConfig, bool)
 	UnregisterPluginServer(pluginID string)
 	ListPluginServers() []mcp.PluginServerConfig
 	GetPluginServer(pluginID string) (mcp.PluginServerConfig, bool)
-	IsPluginRegistered(pluginID string) bool
 
 	DiscoverPluginServerTools(ctx context.Context, userID string, cfg mcp.PluginServerConfig) ([]mcp.ToolInfo, error)
 }
@@ -83,6 +82,10 @@ type MCPClientManager interface {
 type ConfigStore interface {
 	GetConfig() (*config.Config, error)
 	SaveConfig(cfg config.Config) error
+	// UpdateConfig atomically reads the active config, applies transform, and
+	// persists the result under the config advisory lock. A transform error
+	// aborts the update and is returned as-is.
+	UpdateConfig(transform func(prev *config.Config) (config.Config, error)) (config.Config, error)
 }
 
 // AgentStore provides CRUD access to user-created agents in the database.

api/api_admin.go

@@ -13,6 +13,7 @@ import (
 
 	"github.com/gin-gonic/gin"
 	"github.com/mattermost/mattermost-plugin-agents/v2/audit"
+	"github.com/mattermost/mattermost-plugin-agents/v2/config"
 	"github.com/mattermost/mattermost-plugin-agents/v2/indexer"
 	"github.com/mattermost/mattermost-plugin-agents/v2/mcp"
 	"github.com/mattermost/mattermost-plugin-agents/v2/mmapi"
@@ -284,6 +285,8 @@ type MCPServerInfo struct {
 	Enabled    bool   `json:"enabled"`
 	// ToolConfigs is populated for plugin rows only.
 	ToolConfigs []mcp.ToolConfig `json:"toolConfigs,omitempty"`
+	// ID is the stable ABAC policy identity when present.
+	ID string `json:"id,omitempty"`
 }
 
 // MCPToolsResponse represents the response structure for MCP tools endpoint
@@ -362,6 +365,7 @@ func (a *API) buildMCPDiscoveryRows(ctx context.Context, userID string) []mcpDis
 				// Embedded MCP is always available after PR #617, even if older
 				// configs still have the legacy toggle stored as false.
 				Enabled: true,
+				ID:      embeddedConfig.ID,
 			},
 			discover: func() ([]MCPToolInfo, error) {
 				return a.discoverEmbeddedServerTools(ctx, userID, embeddedConfig, embeddedServer)
@@ -389,6 +393,7 @@ func (a *API) buildMCPDiscoveryRows(ctx context.Context, userID string) []mcpDis
 				Tools:      []MCPToolInfo{},
 				ServerType: "remote",
 				Enabled:    serverConfig.Enabled,
+				ID:         serverConfig.ID,
 			},
 		}
 		if message, conflicting := conflictMessages[i]; conflicting {
@@ -402,12 +407,10 @@ func (a *API) buildMCPDiscoveryRows(ctx context.Context, userID string) []mcpDis
 	}
 
 	// Render disabled plugin entries (with an empty tool list) so the admin UI
-	// can re-enable them. Skip orphans hydrated from persisted config without
-	// a live source-plugin registration; surfacing them as "session not found"
-	// rows is misleading, and their persisted policy is preserved on disk
-	// regardless of whether they appear here.
+	// can re-enable them. Skip config-only orphans without a live source-plugin
+	// registration; their persisted identity and policy remain on disk.
 	for _, cfg := range a.mcpClientManager.ListPluginServers() {
-		if !a.mcpClientManager.IsPluginRegistered(cfg.PluginID) {
+		if !isPluginMCPServerRegistered(a.mcpClientManager, cfg.PluginID) {
 			continue
 		}
 
@@ -419,6 +422,7 @@ func (a *API) buildMCPDiscoveryRows(ctx context.Context, userID string) []mcpDis
 				ServerType:  "plugin",
 				Enabled:     cfg.Enabled,
 				ToolConfigs: cfg.ToolConfigs,
+				ID:          cfg.ID,
 			},
 		}
 		if cfg.Enabled {
@@ -432,6 +436,21 @@ func (a *API) buildMCPDiscoveryRows(ctx context.Context, userID string) []mcpDis
 	return rows
 }
 
+func isPluginMCPServerRegistered(manager MCPClientManager, pluginID string) bool {
+	registry, ok := manager.(interface {
+		IsPluginRegistered(string) bool
+	})
+	if ok {
+		return registry.IsPluginRegistered(pluginID)
+	}
+	for _, cfg := range manager.ListPluginServers() {
+		if cfg.PluginID == pluginID {
+			return true
+		}
+	}
+	return false
+}
+
 func applyMCPDiscoveryResult(info *MCPServerInfo, tools []MCPToolInfo, err error) {
 	if err == nil {
 		info.Tools = tools
@@ -556,7 +575,7 @@ func (a *API) handleUpdatePluginServer(c *gin.Context) {
 	audit.AddParam(auditRec(c), "tool_configs_changed", req.ToolConfigs != nil)
 
 	live, foundLive := a.mcpClientManager.GetPluginServer(pluginID)
-	if !foundLive {
+	if !foundLive || !isPluginMCPServerRegistered(a.mcpClientManager, pluginID) {
 		c.AbortWithError(http.StatusNotFound, fmt.Errorf("plugin MCP server %q is not registered", pluginID))
 		return
 	}
@@ -569,40 +588,57 @@ func (a *API) handleUpdatePluginServer(c *gin.Context) {
 		updated.ToolConfigs = *req.ToolConfigs
 	}
 
-	// Effective final value after partial-update merge, not the raw request.
+	// Best-effort fallback; overwritten below when persisted state is available.
 	audit.AddParam(auditRec(c), "enabled", updated.Enabled)
 
-	existing, getErr := a.configStore.GetConfig()
-	if getErr != nil {
-		c.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to load config for plugin-server save: %w", getErr))
-		return
-	}
-	if existing == nil {
-		c.AbortWithError(http.StatusInternalServerError, errors.New("no plugin configuration available"))
-		return
-	}
-	// Clone to avoid mutating the store's cached pointer.
-	cfg := existing.Clone()
-
-	// Merge by PluginID against the persisted list rather than overwriting
-	// with the in-memory snapshot, which would silently drop entries for
-	// plugins that are persisted but currently inactive in memory.
-	merged := append([]mcp.PluginServerConfig(nil), cfg.MCP.PluginServers...)
-	mergedIdx := -1
-	for i := range merged {
-		if merged[i].PluginID == updated.PluginID {
-			mergedIdx = i
-			break
+	// Apply the request to the freshest persisted entry inside the UpdateConfig
+	// transform; rebasing onto the live snapshot read above would let two
+	// concurrent field updates overwrite each other.
+	saved, err := a.configStore.UpdateConfig(func(prev *config.Config) (config.Config, error) {
+		if prev == nil {
+			// Replacing a nil persisted config with a zero-value baseline
+			// would clobber unrelated settings on the next save.
+			return config.Config{}, errors.New("no plugin configuration available")
 		}
-	}
-	if mergedIdx >= 0 {
-		merged[mergedIdx] = updated
-	} else {
-		merged = append(merged, updated)
-	}
-	cfg.MCP.PluginServers = merged
+		// Clone to avoid mutating the store's cached pointer.
+		cfg := prev.Clone()
+
+		// Merge by PluginID against the persisted list rather than overwriting
+		// with the in-memory snapshot, which would silently drop entries for
+		// plugins that are persisted but currently inactive in memory.
+		merged := append([]mcp.PluginServerConfig(nil), cfg.MCP.PluginServers...)
+		mergedIdx := -1
+		for i := range merged {
+			if merged[i].PluginID == pluginID {
+				mergedIdx = i
+				break
+			}
+		}
+
+		// Live entry owns Name/Path/ExposeExternal; persisted admin fields
+		// (Enabled, ToolConfigs, ID) overlay via the shared merge helper.
+		base := live
+		if mergedIdx >= 0 {
+			base = mcp.ApplyPersistedPluginServerFields(base, merged[mergedIdx])
+		}
+		if req.Enabled != nil {
+			base.Enabled = *req.Enabled
+		}
+		if req.ToolConfigs != nil {
+			base.ToolConfigs = *req.ToolConfigs
+		}
+		updated = base
+		audit.AddParam(auditRec(c), "enabled", updated.Enabled)
 
-	if err := a.configStore.SaveConfig(*cfg); err != nil {
+		if mergedIdx >= 0 {
+			merged[mergedIdx] = base
+		} else {
+			merged = append(merged, base)
+		}
+		cfg.MCP.PluginServers = merged
+		return *cfg, nil
+	})
+	if err != nil {
 		c.AbortWithError(http.StatusInternalServerError, fmt.Errorf("failed to save plugin-server config: %w", err))
 		return
 	}
@@ -616,8 +652,13 @@ func (a *API) handleUpdatePluginServer(c *gin.Context) {
 		return
 	}
 
-	a.mcpClientManager.UpdatePluginServer(updated)
-	a.configUpdater.Update(cfg)
+	// Patch admin fields rather than replace the whole object, so plugin-owned
+	// fields survive a concurrent re-registration. A missing entry means the
+	// plugin unregistered; it is hydrated again on its next registration.
+	if patched, ok := a.mcpClientManager.UpdatePluginServerAdminFields(pluginID, updated.Enabled, updated.ToolConfigs); ok {
+		updated = patched
+	}
+	a.configUpdater.Update(&saved)
 
 	// Rebuild when either old or new state was external so removed tools
 	// disappear from the aggregate server.