From 8c9ed6c5fa13b7873c6a69945d3fd3ffdbf70786 Mon Sep 17 00:00:00 2001 From: sean2077 <37324769+sean2077@users.noreply.github.com> Date: Sun, 15 Mar 2026 12:46:55 +0800 Subject: [PATCH 1/3] feat: add multi-user mode with per-user rate limits, role-based ACL, and audit logging Add project-level multi-user support via [projects.users] config section: - Role-based command ACL with disabled_commands wildcard ("*") support - Per-user rate limiting keyed by role configuration - Structured audit logging for command execution and blocking events - Management API endpoints (GET/PATCH /api/v1/projects/{name}/users) - Fully backward compatible: no [projects.users] = unchanged behavior Closes #149 --- cmd/cc-connect/main.go | 70 +++++++++++++ config.example.toml | 23 +++++ config/config.go | 53 ++++++++++ core/engine.go | 86 ++++++++++++++-- core/engine_test.go | 218 ++++++++++++++++++++++++++++++++++++++++ core/management.go | 63 ++++++++++++ core/user_roles.go | 211 ++++++++++++++++++++++++++++++++++++++ core/user_roles_test.go | 217 +++++++++++++++++++++++++++++++++++++++ 8 files changed, 931 insertions(+), 10 deletions(-) create mode 100644 core/user_roles.go create mode 100644 core/user_roles_test.go diff --git a/cmd/cc-connect/main.go b/cmd/cc-connect/main.go index e54932a..f138d8f 100644 --- a/cmd/cc-connect/main.go +++ b/cmd/cc-connect/main.go @@ -235,6 +235,40 @@ func main() { // Wire admin allowlist for privileged commands engine.SetAdminFrom(proj.AdminFrom) + // 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) + } + // Wire display truncation settings { dcfg := core.DisplayCfg{ @@ -917,6 +951,42 @@ func reloadConfig(configPath, projName string, engine *core.Engine) (*core.Confi // Reload admin allowlist engine.SetAdminFrom(proj.AdminFrom) + // 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) + } else { + engine.SetUserRoles(nil) + } + slog.Info("config reloaded", "project", projName) return result, nil } diff --git a/config.example.toml b/config.example.toml index a21a370..2f9744a 100644 --- a/config.example.toml +++ b/config.example.toml @@ -415,6 +415,29 @@ name = "my-backend" # 仅在个人单用户部署中使用此配置,确保所有允许的用户均可信任。 # admin_from = "*" +# Multi-user mode — per-user rate limits and role-based command ACL +# 多用户模式 — 按用户速率限制和基于角色的命令权限控制 +# +# When [projects.users] is configured, each user is matched to a role. +# Roles control which commands are available and the rate limit for that user. +# If not configured, behavior is unchanged (backward compatible). +# +# 配置 [projects.users] 后,每个用户会被分配到一个角色。 +# 角色控制该用户可用的命令和速率限制。未配置时行为不变(向后兼容)。 +# +# [projects.users] +# default_role = "member" +# +# [projects.users.roles.admin] +# user_ids = ["platform_user_id_1", "platform_user_id_2"] +# disabled_commands = [] # no restrictions +# rate_limit = { max_messages = 50, window_secs = 60 } +# +# [projects.users.roles.member] +# user_ids = ["*"] # matches all unassigned users +# disabled_commands = ["*"] # disable all built-in commands +# rate_limit = { max_messages = 10, window_secs = 60 } + # Heartbeat — periodic awareness check / 心跳 — 周期性巡检 # The agent wakes up at a fixed interval to check the environment. # Agent 会按固定间隔唤醒,检查环境状态。 diff --git a/config/config.go b/config/config.go index 7e93fcb..c6ce582 100644 --- a/config/config.go +++ b/config/config.go @@ -88,6 +88,19 @@ type RateLimitConfig struct { WindowSecs *int `toml:"window_secs"` // window size in seconds; default 60 } +// UsersConfig controls per-user role assignments and policies within a project. +type UsersConfig struct { + DefaultRole string `toml:"default_role,omitempty"` // role for unmatched users; default "member" + Roles map[string]RoleConfig `toml:"roles,omitempty"` +} + +// RoleConfig defines policies for a user role. +type RoleConfig struct { + UserIDs []string `toml:"user_ids"` + DisabledCommands []string `toml:"disabled_commands,omitempty"` + RateLimit *RateLimitConfig `toml:"rate_limit,omitempty"` // nil = inherit global +} + // SpeechConfig configures speech-to-text for voice messages. type SpeechConfig struct { Enabled bool `toml:"enabled"` @@ -156,6 +169,7 @@ type ProjectConfig struct { InjectSender *bool `toml:"inject_sender,omitempty"` // prepend sender identity (platform + user ID) to each message sent to the agent DisabledCommands []string `toml:"disabled_commands,omitempty"` // commands to disable for this project (e.g. ["restart", "upgrade"]) AdminFrom string `toml:"admin_from,omitempty"` // comma-separated user IDs allowed to run privileged commands; "*" = all allowed users + Users *UsersConfig `toml:"users,omitempty"` // per-user role config; nil = legacy behavior } type AgentConfig struct { @@ -252,6 +266,45 @@ func (c *Config) validate() error { return fmt.Errorf("project %q: multi-workspace mode conflicts with agent work_dir (use base_dir instead)", proj.Name) } } + if err := validateUsersConfig(prefix, proj.Users); err != nil { + return err + } + } + return nil +} + +// validateUsersConfig checks the [projects.users] section for consistency. +func validateUsersConfig(prefix string, u *UsersConfig) error { + if u == nil { + return nil + } + if len(u.Roles) == 0 { + return fmt.Errorf("config: %s.users has no roles defined", prefix) + } + wildcardCount := 0 + seenUserIDs := make(map[string]string) // userID → role name + for roleName, rc := range u.Roles { + if len(rc.UserIDs) == 0 { + return fmt.Errorf("config: %s.users.roles.%s has empty user_ids", prefix, roleName) + } + for _, uid := range rc.UserIDs { + if uid == "*" { + wildcardCount++ + continue + } + if prev, dup := seenUserIDs[uid]; dup { + return fmt.Errorf("config: %s.users: user %q appears in both role %q and %q", prefix, uid, prev, roleName) + } + seenUserIDs[uid] = roleName + } + } + if wildcardCount > 1 { + return fmt.Errorf("config: %s.users: wildcard user_ids=[\"*\"] appears in multiple roles", prefix) + } + if u.DefaultRole != "" { + if _, ok := u.Roles[u.DefaultRole]; !ok { + return fmt.Errorf("config: %s.users.default_role %q does not match any defined role", prefix, u.DefaultRole) + } } return nil } diff --git a/core/engine.go b/core/engine.go index e579acc..9db0470 100644 --- a/core/engine.go +++ b/core/engine.go @@ -161,6 +161,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) rateLimiter *RateLimiter streamPreview StreamPreviewCfg @@ -432,20 +433,38 @@ func (e *Engine) ClearAliases() { e.aliases = make(map[string]string) } -// SetDisabledCommands sets the list of command IDs that are disabled for this project. -func (e *Engine) SetDisabledCommands(cmds []string) { +// resolveDisabledCmds resolves a list of command names (including "*" wildcard) +// to a set of canonical command IDs. +func resolveDisabledCmds(cmds []string) map[string]bool { m := make(map[string]bool, len(cmds)) for _, c := range cmds { c = strings.ToLower(strings.TrimPrefix(c, "/")) - // Resolve alias names to canonical IDs - id := matchPrefix(c, builtinCommands) - if id != "" { + if c == "*" { + for _, bc := range builtinCommands { + m[bc.id] = true + } + return m + } + if id := matchPrefix(c, builtinCommands); id != "" { m[id] = true } else { m[c] = true } } - e.disabledCmds = m + return m +} + +// SetDisabledCommands sets the list of command IDs that are disabled for this project. +func (e *Engine) SetDisabledCommands(cmds []string) { + e.disabledCmds = resolveDisabledCmds(cmds) +} + +// SetUserRoles configures per-user role-based policies. Pass nil to disable. +func (e *Engine) SetUserRoles(urm *UserRoleManager) { + if e.userRoles != nil { + e.userRoles.Stop() + } + e.userRoles = urm } // SetAdminFrom sets the admin allowlist for privileged commands. @@ -506,6 +525,29 @@ func (e *Engine) SetRateLimitCfg(cfg RateLimitCfg) { e.rateLimiter = NewRateLimiter(cfg.MaxMessages, cfg.Window) } +// 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 { + // Try role-specific rate limit first + if e.userRoles != nil && msg.UserID != "" { + allowed, handled := e.userRoles.AllowRate(msg.UserID) + if handled { + return allowed + } + // Role has no rate_limit config — fall through to global, keyed by userID + } + // Global rate limiter + if e.rateLimiter == nil { + return true + } + // When users config active: key by userID (per-user); otherwise sessionKey (legacy) + key := msg.SessionKey + if e.userRoles != nil && msg.UserID != "" { + key = msg.UserID + } + return e.rateLimiter.Allow(key) +} + // SetStreamPreviewCfg configures the streaming preview behavior. func (e *Engine) SetStreamPreviewCfg(cfg StreamPreviewCfg) { e.streamPreview = cfg @@ -779,6 +821,9 @@ func (e *Engine) Stop() error { if e.rateLimiter != nil { e.rateLimiter.Stop() } + if e.userRoles != nil { + e.userRoles.Stop() + } if err := e.agent.Stop(); err != nil { errs = append(errs, fmt.Errorf("stop agent %s: %w", e.agent.Name(), err)) @@ -853,9 +898,10 @@ func (e *Engine) handleMessage(p Platform, msg *Message) { content = e.resolveAlias(content) msg.Content = content - // Rate limit check - if e.rateLimiter != nil && !e.rateLimiter.Allow(msg.SessionKey) { - slog.Info("message rate limited", "session", msg.SessionKey, "user", msg.UserName) + // Rate limit check (per-user role-based, then global fallback) + if !e.checkRateLimit(msg) { + slog.Info("message rate limited", + "session", msg.SessionKey, "user_id", msg.UserID, "user", msg.UserName) e.reply(p, msg.ReplyCtx, e.i18n.T(MsgRateLimited)) return } @@ -1960,16 +2006,36 @@ func (e *Engine) handleCommand(p Platform, msg *Message, raw string) bool { cmdID := matchPrefix(cmd, builtinCommands) - if cmdID != "" && e.disabledCmds[cmdID] { + // Resolve effective disabled commands: role-based if available, else project-level + disabledCmds := e.disabledCmds + if e.userRoles != nil && msg.UserID != "" { + if role := e.userRoles.ResolveRole(msg.UserID); role != nil { + disabledCmds = role.DisabledCmds + } + } + + if cmdID != "" && disabledCmds[cmdID] { + slog.Info("audit: command_blocked", + "user_id", msg.UserID, "platform", msg.Platform, + "project", e.name, "command", cmdID, "reason", "disabled") e.reply(p, msg.ReplyCtx, fmt.Sprintf(e.i18n.T(MsgCommandDisabled), "/"+cmdID)) return true } if cmdID != "" && privilegedCommands[cmdID] && !e.isAdmin(msg.UserID) { + slog.Info("audit: command_blocked", + "user_id", msg.UserID, "platform", msg.Platform, + "project", e.name, "command", cmdID, "reason", "unauthorized") e.reply(p, msg.ReplyCtx, fmt.Sprintf(e.i18n.T(MsgAdminRequired), "/"+cmdID)) return true } + if cmdID != "" { + slog.Info("audit: command_executed", + "user_id", msg.UserID, "platform", msg.Platform, + "project", e.name, "command", cmdID) + } + switch cmdID { case "new": e.cmdNew(p, msg, args) diff --git a/core/engine_test.go b/core/engine_test.go index 9208315..41b71f0 100644 --- a/core/engine_test.go +++ b/core/engine_test.go @@ -380,6 +380,58 @@ func TestEngine_DisabledCommandsWithSlash(t *testing.T) { } } +func TestResolveDisabledCmds_Wildcard(t *testing.T) { + m := resolveDisabledCmds([]string{"*"}) + for _, bc := range builtinCommands { + if !m[bc.id] { + t.Errorf("wildcard should disable %q", bc.id) + } + } +} + +func TestResolveDisabledCmds_Specific(t *testing.T) { + m := resolveDisabledCmds([]string{"upgrade", "/restart", "Help"}) + if !m["upgrade"] { + t.Error("upgrade should be disabled") + } + if !m["restart"] { + t.Error("restart should be disabled (slash stripped)") + } + if !m["help"] { + t.Error("help should be disabled (case insensitive)") + } + if m["shell"] { + t.Error("shell should not be disabled") + } +} + +func TestResolveDisabledCmds_Empty(t *testing.T) { + m1 := resolveDisabledCmds(nil) + if len(m1) != 0 { + t.Errorf("nil input should produce empty map, got %d entries", len(m1)) + } + m2 := resolveDisabledCmds([]string{}) + if len(m2) != 0 { + t.Errorf("empty input should produce empty map, got %d entries", len(m2)) + } +} + +func TestEngine_DisabledCommandsWildcard(t *testing.T) { + e := newTestEngine() + e.SetDisabledCommands([]string{"*"}) + + p := &stubPlatformEngine{n: "test"} + msg := &Message{SessionKey: "test:u1", UserID: "user1", ReplyCtx: "ctx"} + + e.handleCommand(p, msg, "/help") + if len(p.sent) != 1 { + t.Fatalf("expected 1 reply, got %d", len(p.sent)) + } + if !strings.Contains(p.sent[0], "disabled") && !strings.Contains(p.sent[0], "禁用") { + t.Errorf("expected disabled message, got: %s", p.sent[0]) + } +} + // --- admin_from tests --- func TestEngine_AdminFrom_DenyByDefault(t *testing.T) { @@ -514,6 +566,172 @@ func TestEngine_AdminFrom_AdminCanRunShell(t *testing.T) { } } +// --- role-based ACL tests --- + +func TestEngine_RoleBasedACL_AdminCanRunAll(t *testing.T) { + e := newTestEngine() + e.SetDisabledCommands([]string{"help", "status"}) // project-level disables + + urm := NewUserRoleManager() + urm.Configure("member", []RoleInput{ + {Name: "admin", UserIDs: []string{"admin1"}, DisabledCommands: []string{}}, + {Name: "member", UserIDs: []string{"*"}, DisabledCommands: []string{"*"}}, + }) + e.SetUserRoles(urm) + + p := &stubPlatformEngine{n: "test"} + msg := &Message{SessionKey: "test:a1", UserID: "admin1", ReplyCtx: "ctx"} + e.handleCommand(p, msg, "/help") + + // Admin role has disabled_commands=[], so /help should NOT be blocked + for _, s := range p.sent { + if strings.Contains(s, "disabled") || strings.Contains(s, "禁用") { + t.Errorf("admin should not have /help disabled, got: %s", s) + } + } +} + +func TestEngine_RoleBasedACL_MemberBlocked(t *testing.T) { + e := newTestEngine() + + urm := NewUserRoleManager() + urm.Configure("member", []RoleInput{ + {Name: "admin", UserIDs: []string{"admin1"}, DisabledCommands: []string{}}, + {Name: "member", UserIDs: []string{"*"}, DisabledCommands: []string{"*"}}, + }) + e.SetUserRoles(urm) + + p := &stubPlatformEngine{n: "test"} + msg := &Message{SessionKey: "test:u1", UserID: "user1", ReplyCtx: "ctx"} + e.handleCommand(p, msg, "/help") + + if len(p.sent) != 1 { + t.Fatalf("expected 1 reply, got %d", len(p.sent)) + } + if !strings.Contains(p.sent[0], "disabled") && !strings.Contains(p.sent[0], "禁用") { + t.Errorf("member should have /help disabled, got: %s", p.sent[0]) + } +} + +func TestEngine_RoleBasedACL_NoUserID_Fallback(t *testing.T) { + e := newTestEngine() + e.SetDisabledCommands([]string{"help"}) // project-level disables /help + + urm := NewUserRoleManager() + urm.Configure("member", []RoleInput{ + {Name: "member", UserIDs: []string{"*"}, DisabledCommands: []string{}}, + }) + e.SetUserRoles(urm) + + p := &stubPlatformEngine{n: "test"} + 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 + 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) + } +} + +func TestEngine_RoleBasedACL_NoUsersConfig_Legacy(t *testing.T) { + e := newTestEngine() + e.SetDisabledCommands([]string{"help"}) + // No SetUserRoles — legacy mode + + p := &stubPlatformEngine{n: "test"} + msg := &Message{SessionKey: "test:u1", UserID: "user1", ReplyCtx: "ctx"} + e.handleCommand(p, msg, "/help") + + if len(p.sent) != 1 || (!strings.Contains(p.sent[0], "disabled") && !strings.Contains(p.sent[0], "禁用")) { + t.Errorf("legacy mode should use project-level disabled_commands, got: %v", p.sent) + } +} + +// --- role-based rate limit tests --- + +func TestEngine_RateLimit_RoleSpecific(t *testing.T) { + e := newTestEngine() + + urm := NewUserRoleManager() + urm.Configure("member", []RoleInput{ + {Name: "admin", UserIDs: []string{"admin1"}, DisabledCommands: []string{}, + RateLimit: &RateLimitCfg{MaxMessages: 50, Window: time.Minute}}, + {Name: "member", UserIDs: []string{"*"}, DisabledCommands: []string{}, + RateLimit: &RateLimitCfg{MaxMessages: 2, Window: time.Minute}}, + }) + e.SetUserRoles(urm) + + // Member should be limited after 2 messages + msg := &Message{SessionKey: "test:u1", UserID: "user1"} + if !e.checkRateLimit(msg) { + t.Error("1st message should be allowed") + } + if !e.checkRateLimit(msg) { + t.Error("2nd message should be allowed") + } + if e.checkRateLimit(msg) { + t.Error("3rd message should be rate-limited") + } + + // Admin should still be allowed + adminMsg := &Message{SessionKey: "test:a1", UserID: "admin1"} + if !e.checkRateLimit(adminMsg) { + t.Error("admin should not be rate-limited") + } +} + +func TestEngine_RateLimit_NoUsersConfig_Legacy(t *testing.T) { + e := newTestEngine() + e.SetRateLimitCfg(RateLimitCfg{MaxMessages: 2, Window: time.Minute}) + + msg := &Message{SessionKey: "test:session1", UserID: "user1"} + if !e.checkRateLimit(msg) { + t.Error("1st should be allowed") + } + if !e.checkRateLimit(msg) { + t.Error("2nd should be allowed") + } + if e.checkRateLimit(msg) { + t.Error("3rd should be rate-limited") + } + + // Different session key should be independent (legacy keying) + msg2 := &Message{SessionKey: "test:session2", UserID: "user1"} + if !e.checkRateLimit(msg2) { + t.Error("different session key should have independent bucket in legacy mode") + } +} + +func TestEngine_RateLimit_GlobalFallback(t *testing.T) { + e := newTestEngine() + e.SetRateLimitCfg(RateLimitCfg{MaxMessages: 2, Window: time.Minute}) + + // User roles configured but role has no rate_limit + urm := NewUserRoleManager() + urm.Configure("member", []RoleInput{ + {Name: "member", UserIDs: []string{"*"}, DisabledCommands: []string{}}, + // No RateLimit on this role + }) + e.SetUserRoles(urm) + + msg := &Message{SessionKey: "test:s1", UserID: "user1"} + if !e.checkRateLimit(msg) { + t.Error("1st should be allowed") + } + if !e.checkRateLimit(msg) { + t.Error("2nd should be allowed") + } + if e.checkRateLimit(msg) { + t.Error("3rd should be rate-limited by global limiter") + } + + // Same user, different session → should share limit (keyed by userID when users config active) + msg2 := &Message{SessionKey: "test:s2", UserID: "user1"} + if e.checkRateLimit(msg2) { + t.Error("same user from different session should still be rate-limited") + } +} + // --- permission prompt card tests --- func TestSendPermissionPrompt_CardPlatform(t *testing.T) { diff --git a/core/management.go b/core/management.go index 01100a8..b91dff4 100644 --- a/core/management.go +++ b/core/management.go @@ -363,6 +363,8 @@ func (m *ManagementServer) handleProjectRoutes(w http.ResponseWriter, r *http.Re m.handleProjectModel(w, r, engine) case "heartbeat": m.handleProjectHeartbeat(w, r, projName, rest) + case "users": + m.handleProjectUsers(w, r, engine) default: mgmtError(w, http.StatusNotFound, "not found") } @@ -461,6 +463,67 @@ func (m *ManagementServer) handleProjectDetail(w http.ResponseWriter, r *http.Re mgmtError(w, http.StatusMethodNotAllowed, "GET or PATCH only") } +// ── Users endpoints ────────────────────────────────────────── + +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()) + + case http.MethodPatch: + var body struct { + DefaultRole string `json:"default_role"` + Roles map[string]json.RawMessage `json:"roles"` + } + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + mgmtError(w, http.StatusBadRequest, "invalid JSON: "+err.Error()) + return + } + + var roles []RoleInput + for name, raw := range body.Roles { + var rc struct { + UserIDs []string `json:"user_ids"` + DisabledCommands []string `json:"disabled_commands"` + RateLimit *struct { + MaxMessages int `json:"max_messages"` + WindowSecs int `json:"window_secs"` + } `json:"rate_limit"` + } + if err := json.Unmarshal(raw, &rc); err != nil { + mgmtError(w, http.StatusBadRequest, fmt.Sprintf("invalid role %q: %s", name, err)) + return + } + ri := RoleInput{ + Name: name, + UserIDs: rc.UserIDs, + DisabledCommands: rc.DisabledCommands, + } + if rc.RateLimit != nil { + ri.RateLimit = &RateLimitCfg{ + MaxMessages: rc.RateLimit.MaxMessages, + Window: time.Duration(rc.RateLimit.WindowSecs) * time.Second, + } + } + roles = append(roles, ri) + } + + defaultRole := body.DefaultRole + if defaultRole == "" { + defaultRole = "member" + } + + urm := NewUserRoleManager() + urm.Configure(defaultRole, roles) + e.SetUserRoles(urm) + + mgmtOK(w, "users config updated") + + default: + mgmtError(w, http.StatusMethodNotAllowed, "GET or PATCH only") + } +} + // ── Session endpoints ───────────────────────────────────────── func (m *ManagementServer) handleProjectSessions(w http.ResponseWriter, r *http.Request, projName string, e *Engine, rest string) { diff --git a/core/user_roles.go b/core/user_roles.go new file mode 100644 index 0000000..91b6770 --- /dev/null +++ b/core/user_roles.go @@ -0,0 +1,211 @@ +package core + +import ( + "sort" + "strings" + "sync" + "time" +) + +// UserRole holds the resolved policy for a single role. +type UserRole struct { + Name string + DisabledCmds map[string]bool // resolved command IDs (including "*" wildcard) + RateLimitCfg *RateLimitCfg // nil = no role-specific limit; use global fallback +} + +// RoleInput is the configuration data used to build a UserRoleManager. +type RoleInput struct { + Name string + UserIDs []string + DisabledCommands []string + RateLimit *RateLimitCfg +} + +// UserRoleManager resolves user IDs to roles and manages per-role rate limiters. +type UserRoleManager struct { + mu sync.RWMutex + roles []roleEntry // ordered list for iteration + defaultRole string // fallback role name + roleMap map[string]*UserRole // role name → resolved policy + limiters map[string]*RateLimiter // role name → shared per-role rate limiter +} + +type roleEntry struct { + roleName string + userIDs map[string]bool // normalized user IDs; nil when wildcard + wildcard bool // true if user_ids contains "*" +} + +// NewUserRoleManager creates an empty manager. Call Configure() to populate. +func NewUserRoleManager() *UserRoleManager { + return &UserRoleManager{ + roleMap: make(map[string]*UserRole), + limiters: make(map[string]*RateLimiter), + } +} + +// Configure replaces the role configuration. Should be called on a fresh manager +// before passing to Engine.SetUserRoles(). +func (m *UserRoleManager) Configure(defaultRole string, roles []RoleInput) { + m.mu.Lock() + defer m.mu.Unlock() + + // Stop any existing limiters + for _, rl := range m.limiters { + rl.Stop() + } + + m.defaultRole = defaultRole + m.roleMap = make(map[string]*UserRole, len(roles)) + m.limiters = make(map[string]*RateLimiter, len(roles)) + m.roles = make([]roleEntry, 0, len(roles)) + + // Sort roles by name for deterministic iteration order + sorted := make([]RoleInput, len(roles)) + copy(sorted, roles) + sort.Slice(sorted, func(i, j int) bool { return sorted[i].Name < sorted[j].Name }) + + for _, ri := range sorted { + role := &UserRole{ + Name: ri.Name, + DisabledCmds: resolveDisabledCmds(ri.DisabledCommands), + RateLimitCfg: ri.RateLimit, + } + m.roleMap[ri.Name] = role + + entry := roleEntry{roleName: ri.Name} + for _, uid := range ri.UserIDs { + if uid == "*" { + entry.wildcard = true + } else { + if entry.userIDs == nil { + entry.userIDs = make(map[string]bool) + } + entry.userIDs[strings.ToLower(uid)] = true + } + } + m.roles = append(m.roles, entry) + + // Create per-role rate limiter if configured + if ri.RateLimit != nil && ri.RateLimit.MaxMessages > 0 { + m.limiters[ri.Name] = NewRateLimiter(ri.RateLimit.MaxMessages, ri.RateLimit.Window) + } + } +} + +// ResolveRole returns the role for a given user ID. +// Resolution order: explicit match → default role → wildcard → nil. +// Nil-receiver safe. +func (m *UserRoleManager) ResolveRole(userID string) *UserRole { + if m == nil { + return nil + } + m.mu.RLock() + defer m.mu.RUnlock() + + uid := strings.ToLower(userID) + + // 1. Explicit match in non-wildcard roles + for _, entry := range m.roles { + if !entry.wildcard && entry.userIDs[uid] { + return m.roleMap[entry.roleName] + } + } + + // 2. Default role + if m.defaultRole != "" { + if role, ok := m.roleMap[m.defaultRole]; ok { + return role + } + } + + // 3. Wildcard role + for _, entry := range m.roles { + if entry.wildcard { + return m.roleMap[entry.roleName] + } + } + + return nil +} + +// AllowRate checks the per-user rate limit based on the user's role. +// Returns (allowed, handled). handled=false means no role-specific limit +// was found; the caller should fall back to the global limiter. +// Nil-receiver safe. +func (m *UserRoleManager) AllowRate(userID string) (allowed, handled bool) { + if m == nil { + return true, false + } + role := m.ResolveRole(userID) + if role == nil || role.RateLimitCfg == nil { + return true, false + } + m.mu.RLock() + rl := m.limiters[role.Name] + m.mu.RUnlock() + if rl == nil { + return true, false + } + return rl.Allow(userID), true +} + +// Snapshot returns a serializable representation of the current role configuration. +func (m *UserRoleManager) Snapshot() map[string]any { + if m == nil { + return map[string]any{"configured": false} + } + m.mu.RLock() + defer m.mu.RUnlock() + + roles := make(map[string]any, len(m.roles)) + for _, entry := range m.roles { + role := m.roleMap[entry.roleName] + + userIDs := make([]string, 0) + if entry.wildcard { + userIDs = append(userIDs, "*") + } + for id := range entry.userIDs { + userIDs = append(userIDs, id) + } + sort.Strings(userIDs) + + disabledCmds := make([]string, 0, len(role.DisabledCmds)) + for cmd := range role.DisabledCmds { + disabledCmds = append(disabledCmds, cmd) + } + sort.Strings(disabledCmds) + + roleData := map[string]any{ + "user_ids": userIDs, + "disabled_commands": disabledCmds, + } + if role.RateLimitCfg != nil { + roleData["rate_limit"] = map[string]any{ + "max_messages": role.RateLimitCfg.MaxMessages, + "window_secs": int(role.RateLimitCfg.Window / time.Second), + } + } + roles[entry.roleName] = roleData + } + + return map[string]any{ + "configured": true, + "default_role": m.defaultRole, + "roles": roles, + } +} + +// Stop terminates all per-role rate limiter goroutines. Nil-receiver safe. +func (m *UserRoleManager) Stop() { + if m == nil { + return + } + m.mu.Lock() + defer m.mu.Unlock() + for _, rl := range m.limiters { + rl.Stop() + } +} diff --git a/core/user_roles_test.go b/core/user_roles_test.go new file mode 100644 index 0000000..8d62141 --- /dev/null +++ b/core/user_roles_test.go @@ -0,0 +1,217 @@ +package core + +import ( + "sync" + "testing" + "time" +) + +func testRoles() []RoleInput { + return []RoleInput{ + { + Name: "admin", + UserIDs: []string{"admin1", "admin2"}, + DisabledCommands: []string{}, + RateLimit: &RateLimitCfg{MaxMessages: 50, Window: time.Minute}, + }, + { + Name: "member", + UserIDs: []string{"*"}, + DisabledCommands: []string{"*"}, + RateLimit: &RateLimitCfg{MaxMessages: 3, Window: time.Minute}, + }, + } +} + +func TestUserRoleManager_ResolveRole_ExactMatch(t *testing.T) { + m := NewUserRoleManager() + m.Configure("member", testRoles()) + + role := m.ResolveRole("admin1") + if role == nil || role.Name != "admin" { + t.Errorf("expected admin role, got %+v", role) + } + role = m.ResolveRole("admin2") + if role == nil || role.Name != "admin" { + t.Errorf("expected admin role for admin2, got %+v", role) + } +} + +func TestUserRoleManager_ResolveRole_CaseInsensitive(t *testing.T) { + m := NewUserRoleManager() + m.Configure("member", testRoles()) + + role := m.ResolveRole("ADMIN1") + if role == nil || role.Name != "admin" { + t.Errorf("expected case-insensitive match to admin, got %+v", role) + } +} + +func TestUserRoleManager_ResolveRole_WildcardFallback(t *testing.T) { + m := NewUserRoleManager() + m.Configure("member", testRoles()) + + role := m.ResolveRole("unknown_user") + if role == nil || role.Name != "member" { + t.Errorf("expected member (wildcard) role, got %+v", role) + } +} + +func TestUserRoleManager_ResolveRole_DefaultRole(t *testing.T) { + // No wildcard role; use defaultRole + m := NewUserRoleManager() + m.Configure("viewer", []RoleInput{ + {Name: "admin", UserIDs: []string{"admin1"}, DisabledCommands: []string{}}, + {Name: "viewer", UserIDs: []string{"viewer1"}, DisabledCommands: []string{"shell"}}, + }) + + role := m.ResolveRole("unknown_user") + if role == nil || role.Name != "viewer" { + t.Errorf("expected default role 'viewer', got %+v", role) + } +} + +func TestUserRoleManager_ResolveRole_NoMatch(t *testing.T) { + m := NewUserRoleManager() + m.Configure("", []RoleInput{ + {Name: "admin", UserIDs: []string{"admin1"}, DisabledCommands: []string{}}, + }) + + role := m.ResolveRole("unknown_user") + if role != nil { + t.Errorf("expected nil for unmatched user, got %+v", role) + } +} + +func TestUserRoleManager_DisabledCmdsWildcard(t *testing.T) { + m := NewUserRoleManager() + m.Configure("member", testRoles()) + + role := m.ResolveRole("regular_user") + if role == nil { + t.Fatal("expected member role") + } + // Member has disabled_commands = ["*"], should disable all builtins + for _, bc := range builtinCommands { + if !role.DisabledCmds[bc.id] { + t.Errorf("member role should disable %q", bc.id) + } + } +} + +func TestUserRoleManager_DisabledCmdsEmpty(t *testing.T) { + m := NewUserRoleManager() + m.Configure("member", testRoles()) + + role := m.ResolveRole("admin1") + if role == nil { + t.Fatal("expected admin role") + } + if len(role.DisabledCmds) != 0 { + t.Errorf("admin role should have no disabled commands, got %d", len(role.DisabledCmds)) + } +} + +func TestUserRoleManager_AllowRate_RoleSpecific(t *testing.T) { + m := NewUserRoleManager() + m.Configure("member", testRoles()) + + // Member has max_messages=3 + for i := 0; i < 3; i++ { + allowed, handled := m.AllowRate("user1") + if !allowed || !handled { + t.Errorf("request %d should be allowed, got allowed=%v handled=%v", i+1, allowed, handled) + } + } + // 4th should be blocked + allowed, handled := m.AllowRate("user1") + if allowed || !handled { + t.Errorf("4th request should be blocked, got allowed=%v handled=%v", allowed, handled) + } + + // Admin has max_messages=50, should still be allowed + allowed, handled = m.AllowRate("admin1") + if !allowed || !handled { + t.Errorf("admin should be allowed, got allowed=%v handled=%v", allowed, handled) + } +} + +func TestUserRoleManager_AllowRate_NoRoleLimit(t *testing.T) { + m := NewUserRoleManager() + m.Configure("viewer", []RoleInput{ + {Name: "viewer", UserIDs: []string{"*"}, DisabledCommands: []string{}}, + // No RateLimit on viewer role + }) + + allowed, handled := m.AllowRate("anyone") + if !allowed || handled { + t.Errorf("expected allowed=true handled=false for role without rate_limit, got %v %v", allowed, handled) + } +} + +func TestUserRoleManager_AllowRate_Concurrent(t *testing.T) { + m := NewUserRoleManager() + m.Configure("member", testRoles()) + + var wg sync.WaitGroup + for i := 0; i < 50; i++ { + wg.Add(1) + go func() { + defer wg.Done() + m.AllowRate("concurrent_user") + }() + } + wg.Wait() +} + +func TestUserRoleManager_NilReceiver(t *testing.T) { + var m *UserRoleManager + + if role := m.ResolveRole("anyone"); role != nil { + t.Error("nil manager should return nil role") + } + allowed, handled := m.AllowRate("anyone") + if !allowed || handled { + t.Error("nil manager should return allowed=true handled=false") + } + m.Stop() // should not panic +} + +func TestUserRoleManager_Stop(t *testing.T) { + m := NewUserRoleManager() + m.Configure("member", testRoles()) + + m.Stop() + m.Stop() // idempotent + + // AllowRate should still work (just no cleanup goroutine) + allowed, handled := m.AllowRate("admin1") + if !allowed || !handled { + t.Errorf("should still work after Stop, got allowed=%v handled=%v", allowed, handled) + } +} + +func TestUserRoleManager_Snapshot(t *testing.T) { + m := NewUserRoleManager() + m.Configure("member", testRoles()) + + snap := m.Snapshot() + if snap["configured"] != true { + t.Error("expected configured=true") + } + if snap["default_role"] != "member" { + t.Errorf("expected default_role=member, got %v", snap["default_role"]) + } + roles, ok := snap["roles"].(map[string]any) + if !ok || len(roles) != 2 { + t.Errorf("expected 2 roles, got %v", snap["roles"]) + } +} + +func TestUserRoleManager_Snapshot_Nil(t *testing.T) { + var m *UserRoleManager + snap := m.Snapshot() + if snap["configured"] != false { + t.Error("nil manager snapshot should have configured=false") + } +} From 62fbe44fc0940f8010c05aed5c59d9a718f2c087 Mon Sep 17 00:00:00 2001 From: sean2077 <37324769+sean2077@users.noreply.github.com> Date: Mon, 16 Mar 2026 09:02:45 +0800 Subject: [PATCH 2/3] =?UTF-8?q?fix:=20address=20multi-user=20review=20find?= =?UTF-8?q?ings=20=E2=80=94=20race=20conditions,=20validation,=20and=20edg?= =?UTF-8?q?e=20cases?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 | 95 +++++++++++++++-------------------------- core/engine.go | 42 ++++++++++++++---- core/engine_test.go | 10 +++-- core/management.go | 10 ++++- core/user_roles.go | 37 ++++++++++++++++ core/user_roles_test.go | 56 ++++++++++++++++++++++++ 6 files changed, 177 insertions(+), 73 deletions(-) diff --git a/cmd/cc-connect/main.go b/cmd/cc-connect/main.go index f138d8f..64d01d4 100644 --- a/cmd/cc-connect/main.go +++ b/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, diff --git a/core/engine.go b/core/engine.go index 9db0470..a4e6a72 100644 --- a/core/engine.go +++ b/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 } diff --git a/core/engine_test.go b/core/engine_test.go index 41b71f0..a887051 100644 --- a/core/engine_test.go +++ b/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) } } diff --git a/core/management.go b/core/management.go index b91dff4..eff6c40 100644 --- a/core/management.go +++ b/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) diff --git a/core/user_roles.go b/core/user_roles.go index 91b6770..385c109 100644 --- a/core/user_roles.go +++ b/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 { diff --git a/core/user_roles_test.go b/core/user_roles_test.go index 8d62141..1c36fd3 100644 --- a/core/user_roles_test.go +++ b/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) + } +} From 5be00c702f0f3823a80111561aabbb404e3b75be Mon Sep 17 00:00:00 2001 From: sean2077 <37324769+sean2077@users.noreply.github.com> Date: Mon, 16 Mar 2026 09:17:47 +0800 Subject: [PATCH 3/3] fix: case-insensitive duplicate detection, custom command ACL, and audit logging MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Normalize user IDs to lowercase in ValidateRoleInputs and validateUsersConfig to catch case-insensitive duplicates (e.g. "User1" in admin + "user1" in member) - Apply disabled_commands ACL check to custom commands — previously only builtin commands were checked, so explicitly disabling a custom command name had no effect - Add audit: log entries for custom command and skill execution - Add 2 new tests for case-insensitive validation and custom command ACL --- config/config.go | 5 +++-- core/engine.go | 13 +++++++++++++ core/engine_test.go | 30 ++++++++++++++++++++++++++++++ core/user_roles.go | 5 +++-- core/user_roles_test.go | 10 ++++++++++ 5 files changed, 59 insertions(+), 4 deletions(-) diff --git a/config/config.go b/config/config.go index c6ce582..0529e04 100644 --- a/config/config.go +++ b/config/config.go @@ -292,10 +292,11 @@ func validateUsersConfig(prefix string, u *UsersConfig) error { wildcardCount++ continue } - if prev, dup := seenUserIDs[uid]; dup { + lower := strings.ToLower(uid) + if prev, dup := seenUserIDs[lower]; dup { return fmt.Errorf("config: %s.users: user %q appears in both role %q and %q", prefix, uid, prev, roleName) } - seenUserIDs[uid] = roleName + seenUserIDs[lower] = roleName } } if wildcardCount > 1 { diff --git a/core/engine.go b/core/engine.go index a4e6a72..0e7fad8 100644 --- a/core/engine.go +++ b/core/engine.go @@ -2138,10 +2138,23 @@ func (e *Engine) handleCommand(p Platform, msg *Message, raw string) bool { return true default: if custom, ok := e.commands.Resolve(cmd); ok { + if disabledCmds[strings.ToLower(custom.Name)] { + slog.Info("audit: command_blocked", + "user_id", msg.UserID, "platform", msg.Platform, + "project", e.name, "command", custom.Name, "reason", "disabled") + e.reply(p, msg.ReplyCtx, fmt.Sprintf(e.i18n.T(MsgCommandDisabled), "/"+custom.Name)) + return true + } + slog.Info("audit: command_executed", + "user_id", msg.UserID, "platform", msg.Platform, + "project", e.name, "command", custom.Name, "type", "custom") e.executeCustomCommand(p, msg, custom, args) return true } if skill := e.skills.Resolve(cmd); skill != nil { + slog.Info("audit: command_executed", + "user_id", msg.UserID, "platform", msg.Platform, + "project", e.name, "command", skill.Name, "type", "skill") e.executeSkill(p, msg, skill, args) return true } diff --git a/core/engine_test.go b/core/engine_test.go index a887051..b620e44 100644 --- a/core/engine_test.go +++ b/core/engine_test.go @@ -649,6 +649,36 @@ func TestEngine_RoleBasedACL_NoUsersConfig_Legacy(t *testing.T) { } } +func TestEngine_CustomCommand_DisabledByRole(t *testing.T) { + e := newTestEngine() + e.commands.Add("deploy", "deploy command", "deploy it", "", "", "test") + + urm := NewUserRoleManager() + urm.Configure("member", []RoleInput{ + {Name: "admin", UserIDs: []string{"admin1"}, DisabledCommands: []string{}}, + {Name: "member", UserIDs: []string{"*"}, DisabledCommands: []string{"deploy"}}, + }) + e.SetUserRoles(urm) + + // Member should be blocked from custom command + p := &stubPlatformEngine{n: "test"} + msg := &Message{SessionKey: "test:u1", UserID: "user1", ReplyCtx: "ctx"} + e.handleCommand(p, msg, "/deploy") + + if len(p.sent) != 1 || (!strings.Contains(p.sent[0], "disabled") && !strings.Contains(p.sent[0], "禁用")) { + t.Errorf("custom command should be blocked for member, got: %v", p.sent) + } + + // Admin should be allowed + p2 := &stubPlatformEngine{n: "test"} + msg2 := &Message{SessionKey: "test:a1", UserID: "admin1", ReplyCtx: "ctx"} + e.handleCommand(p2, msg2, "/deploy") + + if len(p2.sent) > 0 && (strings.Contains(p2.sent[0], "disabled") || strings.Contains(p2.sent[0], "禁用")) { + t.Errorf("custom command should be allowed for admin, got: %v", p2.sent) + } +} + // --- role-based rate limit tests --- func TestEngine_RateLimit_RoleSpecific(t *testing.T) { diff --git a/core/user_roles.go b/core/user_roles.go index 385c109..774ed65 100644 --- a/core/user_roles.go +++ b/core/user_roles.go @@ -218,10 +218,11 @@ func ValidateRoleInputs(defaultRole string, roles []RoleInput) error { wildcardCount++ continue } - if prev, dup := seenUserIDs[uid]; dup { + lower := strings.ToLower(uid) + if prev, dup := seenUserIDs[lower]; dup { return fmt.Errorf("user %q appears in both role %q and %q", uid, prev, ri.Name) } - seenUserIDs[uid] = ri.Name + seenUserIDs[lower] = ri.Name } } if wildcardCount > 1 { diff --git a/core/user_roles_test.go b/core/user_roles_test.go index 1c36fd3..b904f2e 100644 --- a/core/user_roles_test.go +++ b/core/user_roles_test.go @@ -265,6 +265,16 @@ func TestValidateRoleInputs_EmptyUserIDs(t *testing.T) { } } +func TestValidateRoleInputs_CaseInsensitiveDuplicate(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 case-insensitive duplicate user IDs") + } +} + func TestValidateRoleInputs_Valid(t *testing.T) { err := ValidateRoleInputs("member", testRoles()) if err != nil {