From 088d6da3c1691a94672530f16821853a968fa749 Mon Sep 17 00:00:00 2001 From: Ed1s0nZ Date: Sat, 10 Oct 2026 15:42:42 +0800 Subject: [PATCH] fix(rbac): revoke only affected role sessions --- CHANGELOG.md | 2 + docs/en-US/rbac.md | 4 +- docs/fixes/P-026-role-session-revocation.md | 40 +++++ docs/zh-CN/rbac.md | 4 +- internal/handler/rbac.go | 40 ++++- internal/handler/rbac_role_sessions_test.go | 147 +++++++++++++++++++ internal/security/auth_manager.go | 51 ++++++- internal/security/auth_role_sessions_test.go | 62 ++++++++ 8 files changed, 338 insertions(+), 12 deletions(-) create mode 100644 docs/fixes/P-026-role-session-revocation.md create mode 100644 internal/handler/rbac_role_sessions_test.go create mode 100644 internal/security/auth_role_sessions_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 0df9f8a9..b00937e0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ ### Fixed +- 修复平台角色更新/删除使全体用户掉线的问题：仅撤销受影响角色成员的会话；名称、描述及等价权限集合更新保持会话有效。 + - 修复俄语等长文案导致的仪表盘状态卡片、风险徽标与队列标签溢出；侧栏及风险名称完整换行，布局随卡片可用宽度适配。 - 自更新恢复在改写文件前严格验证恢复记录、State、提交和二进制配对；拒绝缺失、重复或矛盾证据，保留可恢复现场。 diff --git a/docs/en-US/rbac.md b/docs/en-US/rbac.md index 67d1e717..4ca375fe 100644 --- a/docs/en-US/rbac.md +++ b/docs/en-US/rbac.md @@ -204,7 +204,7 @@ System roles are immutable; create a custom role instead of modifying them. ### When changes take effect - Updating a user, password, enabled state, or role membership revokes that user's sessions; they must sign in again. -- Updating or deleting a custom role revokes all sessions; all users must sign in again. +- Changing a custom role's permissions or scope, or deleting it, revokes only its members' sessions; those users must sign in again. Unrelated users remain signed in. Name, description, and permission-order changes do not revoke sessions, and deleting a role with no members leaves existing sessions valid. - Robots resolve the bound user/service account on every message, so disablement and role changes affect the next message. - Background batch jobs resolve a Principal from the task owner rather than trusting frontend state. @@ -365,7 +365,7 @@ Inspect the scope for that specific permission, not only the overall display sco ### Role changed but the user sees old access -Role changes revoke sessions. Sign in again. Robots resolve again on the next message. +Role permission or scope changes revoke that role's members' sessions. Affected users must sign in again. Robots resolve again on the next message. ### The built-in `admin` password is lost diff --git a/docs/fixes/P-026-role-session-revocation.md b/docs/fixes/P-026-role-session-revocation.md new file mode 100644 index 00000000..dbb8bc90 --- /dev/null +++ b/docs/fixes/P-026-role-session-revocation.md @@ -0,0 +1,40 @@ +# P-026：角色维护的会话失效范围 + +## 工作流与维护检查 + +- 请求：修复角色更新/删除使无关用户掉线的问题。阶段：缺陷反馈与修复（P10）。 +- 上游依据：用户提供的 A/B 结果、已核对的调用链及明确修复授权；无需新产品、UI 或 API 决策。允许实施。 +- 范围：handler 角色更新/删除、AuthManager 精确撤销、回归测试；不更改数据库结构、API 或部署。 +- 验收：删除零成员角色保持所有原有 token 有效；变更权限/scope 或删除有成员角色只撤销成员；描述/名称及等价权限集合变更不撤销；失败操作不撤销。 +- 维护检查：rbac.go 429 行，auth_manager.go 266 行；数据库文件 1453 行，本次只读取，不添加逻辑。选择 narrow_fix，无需先做结构重构。已有 handler 测试缺少携带真实会话的角色维护回归。 +- 假设：现有 Session.Roles 是登录时的角色 ID 快照，适合作为精确失效依据。作用范围仍为当前 AuthManager 实例。 + +## 实现设计与验证计划 + +在 AuthManager 增加按角色 ID 撤销方法，直接匹配会话快照的 Roles，覆盖同成员多个 token 和多个角色。角色删除后仍可匹配旧会话，不依赖已删除的数据库关联。 + +更新前读取旧权限集合，更新成功后比较规范化权限集合和数据库实际保存的 scope；权限顺序、重复、空白不构成实质变化。错误路径保留会话。串行化同 handler 的角色更新/删除，避免旧状态比较被并发角色维护打乱。 + +登录记录会话撤销版本；发布会话时检查版本，若期间发生角色撤销，则重新读取授权，避免撤销之后再写入旧快照。版本变化只要求并发中的登录重新解析，现有无关会话保持有效。 + +验证：真实登录 token 的 handler HTTP 回归、security 单元测试、相关包测试与 race 检查、git diff --check。部署及线上复验不在本次范围。 + +## 实现记录 + +- `internal/handler/rbac.go`：更新角色前读取权限集合，成功后按权限/scope 变化精确撤销；角色删除改为按角色撤销；用互斥锁串行化角色更新与删除。 +- `internal/security/auth_manager.go`：增加 `RevokeRoleSessions`，匹配登录快照而非删除后的数据库关系；登录发布通过版本检查防止撤销后重新写入旧授权。用户授权撤销和全局撤销也推进版本，维持相同安全边界。 +- 新增两个回归文件；中英文 RBAC 管理指南及 Unreleased changelog 已同步更新。 +- 分支：`codex/fix-rbac-role-session-revocation`。修复已完成，进入检视与 main 合并流程；尚未部署。检视及合并结果另见本次检视记录。 + +## 验证记录（2026-10-10，Asia/Shanghai） + +- 新增 HTTP 回归 12 个场景全部通过，含真实 admin、无关 viewer、多角色成员和同成员多个 token；权限收回或角色删除后重新登录不会保留 `chat:write`，scope 收窄后重新登录得到新 scope。 +- `go test ./internal/security ./internal/handler`：两个包全部通过。 +- `go vet ./internal/security ./internal/handler`：通过。 +- `git diff --check`：通过。 +- 原实现对照：使用临时 Go overlay 将两个生产文件映射到基线 HEAD，保持工作树不变，运行同一 HTTP 回归。8 个成功维护场景检出管理员旧 token 的 `401`（预期 `200`），证明测试可识别原缺陷；对照测试退出码 1 为预期结果。 +- `go test -race ./internal/security ./internal/handler -run 'TestRBACRoleMaintenanceSessionIsolation|TestRevokeRoleSessions|TestPublishSession|TestAuthManagerAuthenticatesCreatedRBACUser' -count=1`：通过，无 race 报告。 + +## 边界 + +作用范围仍为当前服务实例。已经通过鉴权并持有 Principal 的在途请求/后台任务不由会话撤销中断；此次修复保证后续 token 鉴权及新登录快照的行为。上线需按现有发布流程部署，生产环境尚未复验。回滚可恢复本次涉及文件的基线实现，但会恢复全员掉线缺陷。 diff --git a/docs/zh-CN/rbac.md b/docs/zh-CN/rbac.md index 685485e7..d6e90a18 100644 --- a/docs/zh-CN/rbac.md +++ b/docs/zh-CN/rbac.md @@ -210,7 +210,7 @@ project:write → own ### 7.3 权限变更何时生效 - 更新用户、密码、启用状态或角色后，该用户现有会话会被撤销，需要重新登录。 -- 更新或删除自定义角色后，平台会撤销全部现有会话，所有用户需重新登录。 +- 修改自定义角色的权限或 Scope、或删除自定义角色后，仅该角色成员的现有会话会被撤销，需要重新登录；无关用户保持在线。仅修改名称、描述或调整权限顺序不会撤销会话；删除零成员角色也不影响现有会话。 - 机器人每条消息实时解析绑定用户/服务账号权限；用户禁用或角色调整会立即影响下一条消息。 - 后台批量任务会根据任务 owner 重新解析 Principal，不应依赖创建任务时的前端状态。 @@ -375,7 +375,7 @@ curl -X POST http://localhost:8080/api/rbac/resource-assignments \ ### 角色改了但用户仍是旧权限 -角色变更会撤销会话。让用户重新登录；机器人下一条消息会重新解析权限。 +角色权限或 Scope 变更会撤销该角色成员的会话。让受影响用户重新登录；机器人下一条消息会重新解析权限。 ### 忘记了内置 `admin` 密码 diff --git a/internal/handler/rbac.go b/internal/handler/rbac.go index f4784e5e..120e7198 100644 --- a/internal/handler/rbac.go +++ b/internal/handler/rbac.go @@ -5,6 +5,7 @@ import ( "net/http" "strconv" "strings" + "sync" "cyberstrike-ai/internal/audit" "cyberstrike-ai/internal/authctx" @@ -20,6 +21,7 @@ type RBACHandler struct { logger *zap.Logger audit *audit.Service auth *security.AuthManager + roleMu sync.Mutex // Serialize role snapshots, mutations, and revocation. } func NewRBACHandler(db *database.DB, logger *zap.Logger) *RBACHandler { @@ -140,6 +142,8 @@ func (h *RBACHandler) CreateRole(c *gin.Context) { } func (h *RBACHandler) UpdateRole(c *gin.Context) { + h.roleMu.Lock() + defer h.roleMu.Unlock() id := strings.TrimSpace(c.Param("id")) existing, err := h.db.GetRBACRoleByID(id) if err != nil { @@ -159,6 +163,11 @@ func (h *RBACHandler) UpdateRole(c *gin.Context) { c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) return } + previousPermissions, err := h.db.ListRBACRolePermissionKeys(id) + if err != nil { + c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + return + } role, err := h.db.UpsertRBACRole(id, req.Name, req.Description, req.Scope, req.Permissions) if err != nil { c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) @@ -170,13 +179,15 @@ func (h *RBACHandler) UpdateRole(c *gin.Context) { if h.audit != nil { h.audit.RecordOK(c, "rbac", "update_role", "更新平台角色", "role", id, nil) } - if h.auth != nil { - h.auth.RevokeAllSessions() + if h.auth != nil && (existing.Scope != role.Scope || !sameRolePermissions(previousPermissions, req.Permissions)) { + h.auth.RevokeRoleSessions(id) } c.JSON(http.StatusOK, gin.H{"role": role}) } func (h *RBACHandler) DeleteRole(c *gin.Context) { + h.roleMu.Lock() + defer h.roleMu.Unlock() id := strings.TrimSpace(c.Param("id")) if err := h.db.DeleteRBACRole(id); err != nil { c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) @@ -186,11 +197,34 @@ func (h *RBACHandler) DeleteRole(c *gin.Context) { h.audit.RecordOK(c, "rbac", "delete_role", "删除平台角色", "role", id, nil) } if h.auth != nil { - h.auth.RevokeAllSessions() + h.auth.RevokeRoleSessions(id) } c.JSON(http.StatusOK, gin.H{"success": true}) } +// Permission order, duplicate keys, and whitespace do not change authorization. +func sameRolePermissions(a, b []string) bool { + normalize := func(keys []string) map[string]bool { + set := make(map[string]bool, len(keys)) + for _, key := range keys { + if key = strings.TrimSpace(key); key != "" { + set[key] = true + } + } + return set + } + left, right := normalize(a), normalize(b) + if len(left) != len(right) { + return false + } + for key := range left { + if !right[key] { + return false + } + } + return true +} + func (h *RBACHandler) ListUsers(c *gin.Context) { users, err := h.db.ListRBACUsers() if err != nil { diff --git a/internal/handler/rbac_role_sessions_test.go b/internal/handler/rbac_role_sessions_test.go new file mode 100644 index 00000000..532efa9c --- /dev/null +++ b/internal/handler/rbac_role_sessions_test.go @@ -0,0 +1,147 @@ +package handler + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "path/filepath" + "testing" + + "cyberstrike-ai/internal/database" + "cyberstrike-ai/internal/security" + + "github.com/gin-gonic/gin" + "go.uber.org/zap" +) + +func TestRBACRoleMaintenanceSessionIsolation(t *testing.T) { + gin.SetMode(gin.TestMode) + for _, tc := range []struct { + name string + method string + member bool + scope string + permissions []string + target string + status int + revoke bool + }{ + {name: "delete empty role", method: http.MethodDelete, status: 200}, + {name: "delete member role", method: http.MethodDelete, member: true, status: 200, revoke: true}, + {name: "metadata only", method: http.MethodPut, member: true, scope: "all", permissions: []string{"auth:self", "chat:write"}, status: 200}, + {name: "equivalent permission set", method: http.MethodPut, member: true, scope: " all ", permissions: []string{" chat:write ", "auth:self", "chat:write", " "}, status: 200}, + {name: "permission reduction", method: http.MethodPut, member: true, scope: "all", permissions: []string{"auth:self"}, status: 200, revoke: true}, + {name: "permission expansion", method: http.MethodPut, member: true, scope: "all", permissions: []string{"auth:self", "chat:write", "chat:read"}, status: 200, revoke: true}, + {name: "scope reduction", method: http.MethodPut, member: true, scope: "own", permissions: []string{"auth:self", "chat:write"}, status: 200, revoke: true}, + {name: "empty role permission change", method: http.MethodPut, scope: "own", permissions: []string{"auth:self"}, status: 200}, + {name: "invalid permission", method: http.MethodPut, member: true, scope: "all", permissions: []string{"invalid:permission"}, status: 400}, + {name: "missing role update", method: http.MethodPut, member: true, target: "missing-role", status: 404}, + {name: "system role update", method: http.MethodPut, member: true, target: database.RBACSystemRoleViewer, status: 400}, + {name: "system role deletion", method: http.MethodDelete, member: true, target: database.RBACSystemRoleViewer, status: 400}, + } { + t.Run(tc.name, func(t *testing.T) { + db, err := database.NewDB(filepath.Join(t.TempDir(), "roles.db"), zap.NewNop()) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = db.Close() }) + auth := security.NewAuthManager(12) + adminPassword, err := auth.AttachRBACStore(db) + if err != nil { + t.Fatal(err) + } + role, err := db.UpsertRBACRole("", "probe", "before", database.RBACScopeAll, []string{"auth:self", "chat:write"}) + if err != nil { + t.Fatal(err) + } + hash, err := security.HashPassword("member-password") + if err != nil { + t.Fatal(err) + } + roles := []string{database.RBACSystemRoleViewer} + if tc.member { + roles = append(roles, role.ID) + } + if _, err := db.CreateRBACUser("member", "Member", hash, true, roles); err != nil { + t.Fatal(err) + } + if _, err := db.CreateRBACUser("unrelated", "Unrelated", hash, true, []string{database.RBACSystemRoleViewer}); err != nil { + t.Fatal(err) + } + login := func(username, password string) string { + t.Helper() + token, _, err := auth.Authenticate(username, password) + if err != nil { + t.Fatal(err) + } + return token + } + adminToken := login("admin", adminPassword) + memberTokens := []string{login("member", "member-password"), login("member", "member-password")} + unrelatedToken := login("unrelated", "member-password") + h := NewRBACHandler(db, zap.NewNop()) + h.SetAuthManager(auth) + router := gin.New() + router.Use(security.AuthMiddleware(auth), security.RBACMiddleware(db)) + router.GET("/api/rbac/me", h.Me) + router.PUT("/api/rbac/roles/:id", h.UpdateRole) + router.DELETE("/api/rbac/roles/:id", h.DeleteRole) + request := func(method, path, token string, body []byte) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(method, path, bytes.NewReader(body)) + req.Header.Set("Authorization", "Bearer "+token) + req.Header.Set("Content-Type", "application/json") + w := httptest.NewRecorder() + router.ServeHTTP(w, req) + return w + } + checkMe := func(token string, want int) { + t.Helper() + w := request(http.MethodGet, "/api/rbac/me", token, nil) + if w.Code != want { + t.Fatalf("me status = %d, want %d: %s", w.Code, want, w.Body.String()) + } + } + for _, token := range append([]string{adminToken, unrelatedToken}, memberTokens...) { + checkMe(token, http.StatusOK) + } + body, err := json.Marshal(map[string]interface{}{ + "name": "probe renamed", "description": "after", "scope": tc.scope, "permissions": tc.permissions, + }) + if err != nil { + t.Fatal(err) + } + target := role.ID + if tc.target != "" { + target = tc.target + } + w := request(tc.method, "/api/rbac/roles/"+target, adminToken, body) + if w.Code != tc.status { + t.Fatalf("maintenance status = %d, want %d: %s", w.Code, tc.status, w.Body.String()) + } + checkMe(adminToken, http.StatusOK) + checkMe(unrelatedToken, http.StatusOK) + want := http.StatusOK + if tc.revoke { + want = http.StatusUnauthorized + } + for _, token := range memberTokens { + checkMe(token, want) + } + if tc.revoke { + newToken := login("member", "member-password") + checkMe(newToken, http.StatusOK) + session, _ := auth.ValidateToken(newToken) + if tc.method == http.MethodDelete || tc.name == "permission reduction" { + if session.Permissions["chat:write"] { + t.Fatal("new login retained revoked chat:write permission") + } + } + if tc.name == "scope reduction" && session.ScopeFor("chat:write") != database.RBACScopeOwn { + t.Fatal("new login retained old chat:write scope") + } + } + }) + } +} diff --git a/internal/security/auth_manager.go b/internal/security/auth_manager.go index 74e3b79a..48f25f77 100644 --- a/internal/security/auth_manager.go +++ b/internal/security/auth_manager.go @@ -37,6 +37,9 @@ type AuthManager struct { mu sync.RWMutex sessions map[string]Session + // Changes when cached authorization is revoked, including while a login + // is resolving access outside mu. + accessVersion uint64 } // NewAuthManager creates a new AuthManager instance. @@ -87,14 +90,29 @@ func (a *AuthManager) AttachRBACStore(db *database.DB) (generatedAdminPassword s // Authenticate validates the password and creates a new session. func (a *AuthManager) Authenticate(username, password string) (string, time.Time, error) { - session, err := a.authenticateSession(username, password) - if err != nil { - return "", time.Time{}, err + for { + a.mu.RLock() + version := a.accessVersion + a.mu.RUnlock() + session, err := a.authenticateSession(username, password) + if err != nil { + return "", time.Time{}, err + } + if a.publishSession(session, version) { + return session.Token, session.ExpiresAt, nil + } } +} + +// publishSession rejects a snapshot resolved across an authorization revocation. +func (a *AuthManager) publishSession(session Session, version uint64) bool { a.mu.Lock() + defer a.mu.Unlock() + if a.accessVersion != version { + return false + } a.sessions[session.Token] = session - a.mu.Unlock() - return session.Token, session.ExpiresAt, nil + return true } func (a *AuthManager) authenticateSession(username, password string) (Session, error) { @@ -212,6 +230,7 @@ func (a *AuthManager) UpdateUserPassword(userID, password string) error { return err } a.mu.Lock() + a.accessVersion++ for token, session := range a.sessions { if session.UserID == userID { delete(a.sessions, token) @@ -238,6 +257,7 @@ func (a *AuthManager) RevokeUserSessions(userID string) { return } a.mu.Lock() + a.accessVersion++ for token, session := range a.sessions { if session.UserID == userID { delete(a.sessions, token) @@ -246,8 +266,29 @@ func (a *AuthManager) RevokeUserSessions(userID string) { a.mu.Unlock() } +// RevokeRoleSessions invalidates only snapshots containing the changed role. +// Matching the session snapshot also works after the role has been deleted. +func (a *AuthManager) RevokeRoleSessions(roleID string) { + roleID = strings.TrimSpace(roleID) + if roleID == "" { + return + } + a.mu.Lock() + defer a.mu.Unlock() + a.accessVersion++ + for token, session := range a.sessions { + for _, id := range session.Roles { + if id == roleID { + delete(a.sessions, token) + break + } + } + } +} + func (a *AuthManager) RevokeAllSessions() { a.mu.Lock() + a.accessVersion++ a.sessions = make(map[string]Session) a.mu.Unlock() } diff --git a/internal/security/auth_role_sessions_test.go b/internal/security/auth_role_sessions_test.go new file mode 100644 index 00000000..3e20fb33 --- /dev/null +++ b/internal/security/auth_role_sessions_test.go @@ -0,0 +1,62 @@ +package security + +import ( + "testing" + "time" +) + +func TestRevokeRoleSessionsMatchesSnapshotAndPreservesOthers(t *testing.T) { + a := NewAuthManager(12) + for _, session := range []Session{ + {Token: "member-first", UserID: "member", Roles: []string{"viewer", "changed"}}, + {Token: "member-second", UserID: "member", Roles: []string{"changed"}}, + {Token: "other", UserID: "other", Roles: []string{"viewer"}}, + {Token: "admin", UserID: "admin", Roles: []string{"admin"}}, + } { + session.ExpiresAt = time.Now().Add(time.Hour) + a.sessions[session.Token] = session + } + a.RevokeRoleSessions(" ") + if len(a.sessions) != 4 { + t.Fatal("blank role revoked sessions") + } + a.RevokeRoleSessions(" changed ") + for _, token := range []string{"member-first", "member-second"} { + if _, ok := a.ValidateToken(token); ok { + t.Fatalf("affected session %s remains valid", token) + } + } + for _, token := range []string{"other", "admin"} { + if _, ok := a.ValidateToken(token); !ok { + t.Fatalf("unrelated session %s revoked", token) + } + } +} + +func TestPublishSessionRejectsSnapshotResolvedAcrossRevocation(t *testing.T) { + for _, kind := range []string{"role", "user", "all"} { + t.Run(kind, func(t *testing.T) { + a := NewAuthManager(12) + version := a.accessVersion + // A login has resolved its snapshot but has not yet published it. + snapshot := Session{Token: "pending", UserID: "member", Roles: []string{"changed"}, ExpiresAt: time.Now().Add(time.Hour)} + switch kind { + case "role": + a.RevokeRoleSessions("changed") + case "user": + a.RevokeUserSessions("member") + case "all": + a.RevokeAllSessions() + } + if a.publishSession(snapshot, version) { + t.Fatal("stale authorization snapshot published after revocation") + } + if _, ok := a.ValidateToken(snapshot.Token); ok { + t.Fatal("rejected snapshot is visible to authentication") + } + if !a.publishSession(snapshot, a.accessVersion) { + t.Fatal("freshly resolved snapshot rejected") + } + }) + } +}