fix: address multi-user review findings — race conditions, validation, and edge cases

- Add userRolesMu (sync.RWMutex) to protect concurrent read/write of
  userRoles and disabledCmds on the Engine
- Add ValidateRoleInputs() and call it from management API PATCH /users
  to prevent duplicate user IDs, multiple wildcards, and invalid default_role
- Fix empty UserID bypassing role-based ACL and rate limiting — now
  resolves to default/wildcard role instead of falling back to less
  restrictive project-level policies
- Extract buildUserRoleManager() helper in main.go to eliminate code
  duplication between initial setup and config reload
- Add 7 new tests for validation and empty UserID behavior

cmd/cc-connect/main.go

@@ -237,36 +237,7 @@ func main() {
 
 		// Wire per-user role-based policies
 		if proj.Users != nil {
-			urm := core.NewUserRoleManager()
-			var roles []core.RoleInput
-			for name, rc := range proj.Users.Roles {
-				var rlCfg *core.RateLimitCfg
-				if rc.RateLimit != nil {
-					maxMsg, windowSecs := 20, 60
-					if rc.RateLimit.MaxMessages != nil {
-						maxMsg = *rc.RateLimit.MaxMessages
-					}
-					if rc.RateLimit.WindowSecs != nil {
-						windowSecs = *rc.RateLimit.WindowSecs
-					}
-					rlCfg = &core.RateLimitCfg{
-						MaxMessages: maxMsg,
-						Window:      time.Duration(windowSecs) * time.Second,
-					}
-				}
-				roles = append(roles, core.RoleInput{
-					Name:             name,
-					UserIDs:          rc.UserIDs,
-					DisabledCommands: rc.DisabledCommands,
-					RateLimit:        rlCfg,
-				})
-			}
-			defaultRole := "member"
-			if proj.Users.DefaultRole != "" {
-				defaultRole = proj.Users.DefaultRole
-			}
-			urm.Configure(defaultRole, roles)
-			engine.SetUserRoles(urm)
+			engine.SetUserRoles(buildUserRoleManager(proj.Users))
 		}
 
 		// Wire display truncation settings
@@ -953,36 +924,7 @@ func reloadConfig(configPath, projName string, engine *core.Engine) (*core.Confi
 
 	// Reload per-user role-based policies
 	if proj.Users != nil {
-		urm := core.NewUserRoleManager()
-		var roles []core.RoleInput
-		for name, rc := range proj.Users.Roles {
-			var rlCfg *core.RateLimitCfg
-			if rc.RateLimit != nil {
-				maxMsg, windowSecs := 20, 60
-				if rc.RateLimit.MaxMessages != nil {
-					maxMsg = *rc.RateLimit.MaxMessages
-				}
-				if rc.RateLimit.WindowSecs != nil {
-					windowSecs = *rc.RateLimit.WindowSecs
-				}
-				rlCfg = &core.RateLimitCfg{
-					MaxMessages: maxMsg,
-					Window:      time.Duration(windowSecs) * time.Second,
-				}
-			}
-			roles = append(roles, core.RoleInput{
-				Name:             name,
-				UserIDs:          rc.UserIDs,
-				DisabledCommands: rc.DisabledCommands,
-				RateLimit:        rlCfg,
-			})
-		}
-		defaultRole := "member"
-		if proj.Users.DefaultRole != "" {
-			defaultRole = proj.Users.DefaultRole
-		}
-		urm.Configure(defaultRole, roles)
-		engine.SetUserRoles(urm)
+		engine.SetUserRoles(buildUserRoleManager(proj.Users))
 	} else {
 		engine.SetUserRoles(nil)
 	}
@@ -991,6 +933,39 @@ func reloadConfig(configPath, projName string, engine *core.Engine) (*core.Confi
 	return result, nil
 }
 
+func buildUserRoleManager(uc *config.UsersConfig) *core.UserRoleManager {
+	var roles []core.RoleInput
+	for name, rc := range uc.Roles {
+		var rlCfg *core.RateLimitCfg
+		if rc.RateLimit != nil {
+			maxMsg, windowSecs := 20, 60
+			if rc.RateLimit.MaxMessages != nil {
+				maxMsg = *rc.RateLimit.MaxMessages
+			}
+			if rc.RateLimit.WindowSecs != nil {
+				windowSecs = *rc.RateLimit.WindowSecs
+			}
+			rlCfg = &core.RateLimitCfg{
+				MaxMessages: maxMsg,
+				Window:      time.Duration(windowSecs) * time.Second,
+			}
+		}
+		roles = append(roles, core.RoleInput{
+			Name:             name,
+			UserIDs:          rc.UserIDs,
+			DisabledCommands: rc.DisabledCommands,
+			RateLimit:        rlCfg,
+		})
+	}
+	defaultRole := "member"
+	if uc.DefaultRole != "" {
+		defaultRole = uc.DefaultRole
+	}
+	urm := core.NewUserRoleManager()
+	urm.Configure(defaultRole, roles)
+	return urm
+}
+
 func buildHeartbeatConfig(hc config.HeartbeatConfig) core.HeartbeatConfig {
 	cfg := core.HeartbeatConfig{
 		IntervalMins: 30,

core/engine.go

@@ -162,6 +162,7 @@ type Engine struct {
 	disabledCmds map[string]bool
 	adminFrom    string // comma-separated user IDs for privileged commands; "*" = all allowed users; "" = deny
 	userRoles    *UserRoleManager // nil = legacy mode (no per-user policies)
+	userRolesMu  sync.RWMutex    // protects userRoles and disabledCmds
 
 	rateLimiter      *RateLimiter
 	streamPreview    StreamPreviewCfg
@@ -456,11 +457,15 @@ func resolveDisabledCmds(cmds []string) map[string]bool {
 
 // SetDisabledCommands sets the list of command IDs that are disabled for this project.
 func (e *Engine) SetDisabledCommands(cmds []string) {
+	e.userRolesMu.Lock()
+	defer e.userRolesMu.Unlock()
 	e.disabledCmds = resolveDisabledCmds(cmds)
 }
 
 // SetUserRoles configures per-user role-based policies. Pass nil to disable.
 func (e *Engine) SetUserRoles(urm *UserRoleManager) {
+	e.userRolesMu.Lock()
+	defer e.userRolesMu.Unlock()
 	if e.userRoles != nil {
 		e.userRoles.Stop()
 	}
@@ -472,7 +477,10 @@ func (e *Engine) SetUserRoles(urm *UserRoleManager) {
 // Empty string means privileged commands are denied for everyone.
 func (e *Engine) SetAdminFrom(adminFrom string) {
 	e.adminFrom = strings.TrimSpace(adminFrom)
-	if e.adminFrom == "" && !e.disabledCmds["shell"] {
+	e.userRolesMu.RLock()
+	shellDisabled := e.disabledCmds["shell"]
+	e.userRolesMu.RUnlock()
+	if e.adminFrom == "" && !shellDisabled {
 		slog.Warn("admin_from is not set — privileged commands (/shell, /dir, /restart, /upgrade) are blocked. "+
 			"Set admin_from in config to enable them, or use disabled_commands to hide them.",
 			"project", e.name)
@@ -528,13 +536,22 @@ func (e *Engine) SetRateLimitCfg(cfg RateLimitCfg) {
 // checkRateLimit returns true if the message is allowed, false if rate-limited.
 // It checks per-user role-based limits first, then falls back to the global limiter.
 func (e *Engine) checkRateLimit(msg *Message) bool {
+	e.userRolesMu.RLock()
+	urm := e.userRoles
+	e.userRolesMu.RUnlock()
+
 	// Try role-specific rate limit first
-	if e.userRoles != nil && msg.UserID != "" {
-		allowed, handled := e.userRoles.AllowRate(msg.UserID)
+	if urm != nil {
+		// Use userID if available, else fall back to sessionKey for unidentified users
+		rateKey := msg.UserID
+		if rateKey == "" {
+			rateKey = msg.SessionKey
+		}
+		allowed, handled := urm.AllowRate(rateKey)
 		if handled {
 			return allowed
 		}
-		// Role has no rate_limit config — fall through to global, keyed by userID
+		// Role has no rate_limit config — fall through to global, keyed by user
 	}
 	// Global rate limiter
 	if e.rateLimiter == nil {
@@ -542,7 +559,7 @@ func (e *Engine) checkRateLimit(msg *Message) bool {
 	}
 	// When users config active: key by userID (per-user); otherwise sessionKey (legacy)
 	key := msg.SessionKey
-	if e.userRoles != nil && msg.UserID != "" {
+	if urm != nil && msg.UserID != "" {
 		key = msg.UserID
 	}
 	return e.rateLimiter.Allow(key)
@@ -821,9 +838,11 @@ func (e *Engine) Stop() error {
 	if e.rateLimiter != nil {
 		e.rateLimiter.Stop()
 	}
+	e.userRolesMu.Lock()
 	if e.userRoles != nil {
 		e.userRoles.Stop()
 	}
+	e.userRolesMu.Unlock()
 
 	if err := e.agent.Stop(); err != nil {
 		errs = append(errs, fmt.Errorf("stop agent %s: %w", e.agent.Name(), err))
@@ -2007,9 +2026,12 @@ func (e *Engine) handleCommand(p Platform, msg *Message, raw string) bool {
 	cmdID := matchPrefix(cmd, builtinCommands)
 
 	// Resolve effective disabled commands: role-based if available, else project-level
+	e.userRolesMu.RLock()
 	disabledCmds := e.disabledCmds
-	if e.userRoles != nil && msg.UserID != "" {
-		if role := e.userRoles.ResolveRole(msg.UserID); role != nil {
+	urm := e.userRoles
+	e.userRolesMu.RUnlock()
+	if urm != nil {
+		if role := urm.ResolveRole(msg.UserID); role != nil {
 			disabledCmds = role.DisabledCmds
 		}
 	}
@@ -3542,6 +3564,10 @@ func (e *Engine) renderHelpGroupCard(groupKey string) *Card {
 func (e *Engine) GetAllCommands() []BotCommandInfo {
 	var commands []BotCommandInfo
 
+	e.userRolesMu.RLock()
+	disabledCmds := e.disabledCmds
+	e.userRolesMu.RUnlock()
+
 	// Collect built-in  commands (use primary name, first in names list)
 	seenCmds := make(map[string]bool)
 	for _, c := range builtinCommands {
@@ -3556,7 +3582,7 @@ func (e *Engine) GetAllCommands() []BotCommandInfo {
 		seenCmds[primaryName] = true
 
 		// Skip disabled commands
-		if e.disabledCmds[c.id] {
+		if disabledCmds[c.id] {
 			continue
 		}
 

core/engine_test.go

@@ -613,13 +613,15 @@ func TestEngine_RoleBasedACL_MemberBlocked(t *testing.T) {
 	}
 }
 
-func TestEngine_RoleBasedACL_NoUserID_Fallback(t *testing.T) {
+func TestEngine_RoleBasedACL_NoUserID_UsesDefaultRole(t *testing.T) {
 	e := newTestEngine()
 	e.SetDisabledCommands([]string{"help"}) // project-level disables /help
 
+	// Default role "member" has wildcard with disabled_commands=["*"]
 	urm := NewUserRoleManager()
 	urm.Configure("member", []RoleInput{
-		{Name: "member", UserIDs: []string{"*"}, DisabledCommands: []string{}},
+		{Name: "admin", UserIDs: []string{"admin1"}, DisabledCommands: []string{}},
+		{Name: "member", UserIDs: []string{"*"}, DisabledCommands: []string{"*"}},
 	})
 	e.SetUserRoles(urm)
 
@@ -627,9 +629,9 @@ func TestEngine_RoleBasedACL_NoUserID_Fallback(t *testing.T) {
 	msg := &Message{SessionKey: "test:anon", UserID: "", ReplyCtx: "ctx"} // no UserID
 	e.handleCommand(p, msg, "/help")
 
-	// No UserID → falls back to project-level disabledCmds which has /help disabled
+	// Empty UserID resolves to default/wildcard role, which disables all commands
 	if len(p.sent) != 1 || (!strings.Contains(p.sent[0], "disabled") && !strings.Contains(p.sent[0], "禁用")) {
-		t.Errorf("empty UserID should use project-level disabled_commands, got: %v", p.sent)
+		t.Errorf("empty UserID should resolve to default role ACL, got: %v", p.sent)
 	}
 }
 

core/management.go

@@ -468,7 +468,10 @@ func (m *ManagementServer) handleProjectDetail(w http.ResponseWriter, r *http.Re
 func (m *ManagementServer) handleProjectUsers(w http.ResponseWriter, r *http.Request, e *Engine) {
 	switch r.Method {
 	case http.MethodGet:
-		mgmtJSON(w, http.StatusOK, e.userRoles.Snapshot())
+		e.userRolesMu.RLock()
+		urm := e.userRoles
+		e.userRolesMu.RUnlock()
+		mgmtJSON(w, http.StatusOK, urm.Snapshot())
 
 	case http.MethodPatch:
 		var body struct {
@@ -513,6 +516,11 @@ func (m *ManagementServer) handleProjectUsers(w http.ResponseWriter, r *http.Req
 			defaultRole = "member"
 		}
 
+		if err := ValidateRoleInputs(defaultRole, roles); err != nil {
+			mgmtError(w, http.StatusBadRequest, "invalid users config: "+err.Error())
+			return
+		}
+
 		urm := NewUserRoleManager()
 		urm.Configure(defaultRole, roles)
 		e.SetUserRoles(urm)

core/user_roles.go

@@ -1,6 +1,7 @@
 package core
 
 import (
+	"fmt"
 	"sort"
 	"strings"
 	"sync"
@@ -198,6 +199,42 @@ func (m *UserRoleManager) Snapshot() map[string]any {
 	}
 }
 
+// ValidateRoleInputs checks role inputs for consistency: duplicate user IDs,
+// multiple wildcards, empty user_ids, and default_role existence.
+func ValidateRoleInputs(defaultRole string, roles []RoleInput) error {
+	if len(roles) == 0 {
+		return fmt.Errorf("no roles defined")
+	}
+	wildcardCount := 0
+	seenUserIDs := make(map[string]string) // userID → role name
+	roleNames := make(map[string]bool, len(roles))
+	for _, ri := range roles {
+		roleNames[ri.Name] = true
+		if len(ri.UserIDs) == 0 {
+			return fmt.Errorf("role %q has empty user_ids", ri.Name)
+		}
+		for _, uid := range ri.UserIDs {
+			if uid == "*" {
+				wildcardCount++
+				continue
+			}
+			if prev, dup := seenUserIDs[uid]; dup {
+				return fmt.Errorf("user %q appears in both role %q and %q", uid, prev, ri.Name)
+			}
+			seenUserIDs[uid] = ri.Name
+		}
+	}
+	if wildcardCount > 1 {
+		return fmt.Errorf("wildcard user_ids=[\"*\"] appears in multiple roles")
+	}
+	if defaultRole != "" {
+		if !roleNames[defaultRole] {
+			return fmt.Errorf("default_role %q does not match any defined role", defaultRole)
+		}
+	}
+	return nil
+}
+
 // Stop terminates all per-role rate limiter goroutines. Nil-receiver safe.
 func (m *UserRoleManager) Stop() {
 	if m == nil {

core/user_roles_test.go

@@ -215,3 +215,59 @@ func TestUserRoleManager_Snapshot_Nil(t *testing.T) {
 		t.Error("nil manager snapshot should have configured=false")
 	}
 }
+
+func TestUserRoleManager_ResolveRole_EmptyUserID(t *testing.T) {
+	m := NewUserRoleManager()
+	m.Configure("member", testRoles())
+
+	// Empty userID should still resolve to default/wildcard role
+	role := m.ResolveRole("")
+	if role == nil || role.Name != "member" {
+		t.Errorf("empty userID should resolve to default role, got %+v", role)
+	}
+}
+
+func TestValidateRoleInputs_DuplicateUserIDs(t *testing.T) {
+	err := ValidateRoleInputs("admin", []RoleInput{
+		{Name: "admin", UserIDs: []string{"user1"}},
+		{Name: "member", UserIDs: []string{"user1"}},
+	})
+	if err == nil {
+		t.Error("expected error for duplicate user IDs")
+	}
+}
+
+func TestValidateRoleInputs_MultipleWildcards(t *testing.T) {
+	err := ValidateRoleInputs("admin", []RoleInput{
+		{Name: "admin", UserIDs: []string{"*"}},
+		{Name: "member", UserIDs: []string{"*"}},
+	})
+	if err == nil {
+		t.Error("expected error for multiple wildcards")
+	}
+}
+
+func TestValidateRoleInputs_InvalidDefaultRole(t *testing.T) {
+	err := ValidateRoleInputs("nonexistent", []RoleInput{
+		{Name: "admin", UserIDs: []string{"user1"}},
+	})
+	if err == nil {
+		t.Error("expected error for invalid default_role")
+	}
+}
+
+func TestValidateRoleInputs_EmptyUserIDs(t *testing.T) {
+	err := ValidateRoleInputs("admin", []RoleInput{
+		{Name: "admin", UserIDs: []string{}},
+	})
+	if err == nil {
+		t.Error("expected error for empty user_ids")
+	}
+}
+
+func TestValidateRoleInputs_Valid(t *testing.T) {
+	err := ValidateRoleInputs("member", testRoles())
+	if err != nil {
+		t.Errorf("expected no error for valid config, got %v", err)
+	}
+}