|
|
@@ -1,510 +1,149 @@
|
|
|
-# 深度审计报告 · Round 17
|
|
|
+# 审计报告(第 18 轮)
|
|
|
|
|
|
-> 基线:R16 审计后的代码库快照。R16 提出的 1 条 High(H-R16-1,`RemoveMember` 未在 tx 内吊销会话)、1 条 Medium(M-R16-1,`UserList`/`UserDetail` 未对普通 MEMBER 脱敏 PII)与 2 条 Low(L-R16-1/2)均已在代码中落地:`removeMemberLogic.go` 在 tx 体里显式 `IncrementTokenVersionWithTx` 并在 post-commit 清 sysUser 低层缓存;`userListLogic.go` / `userDetailLogic.go` 新增 `maskUserPII` 对 MEMBER 遮蔽 email/phone/remark;`updateUserLogic.go` 去掉了 ADMIN 旁路 DeptPath 前缀校验,并补上"DEV → NORMAL 跨域调动"时的 tokenVersion 递增;`updateDeptLogic.go` 在"DEV→NORMAL / Enabled→Disabled"这类权限收窄迁移时对整棵子树的 user 做 `BatchIncrementTokenVersionWithTx`。R15/R14/R13/R12/R11/R10 的既往修复经复检全部稳定。
|
|
|
+> 审计范围:`internal/` 下的全部生产代码(排除 `_test.go`、`mocks/`、CLI 生成的 `*_gen.go` 中模型原型)。
|
|
|
+> 重点关注:多接口间的逻辑耦合、并发/事务一致性、缓存与 DB 的双写对齐、权限边界、接口契约完整性。
|
|
|
>
|
|
|
-> 本轮聚焦四块 R16 未覆盖的面:
|
|
|
->
|
|
|
-> 1. **跨对象事务闭环的外部边**:`UpdateDept` / `DeleteDept` 的 sys_dept X 锁只能串行化"改部门 / 删部门"路径;但 `CreateUser` / `CreateProductLogic` 的 auto-provision 用户行**完全不走事务、不对 sys_dept 取锁**,留下"DeleteDept 已完成 → 新 user 仍以该 deptId 落盘"的 orphan-row 窗口;同类结构性问题还影响 `UpdateUser` 的 `*req.DeptId == user.DeptId` 分支与 `AddMember` 对目标 `sys_user.Status` 的读后写。
|
|
|
-> 2. **缓存失效与事务提交的时序错位**:go-zero `sqlc.CachedConn.ExecCtx` 的"exec → DelCache"顺序对普通 `sql.DB` 是正确的,但在 `session.ExecCtx` 语义下,DelCache 发生在外层 tx **commit 之前**;`DeleteDept` / `DeleteRole` / `UpdateDept` 等走 `*WithTx` 的路径会在这个窗口内被并发 `FindOne` 把**旧行**回填进缓存,随后 commit,留下最长 5min 的"幽灵快照"——尤其对 `sys_dept` 这种 `UserDetailsLoader.loadDept` 直接消费行内容的对象具有权限语义。
|
|
|
-> 3. **"全权用户"对 SyncPerms 新增 perm 的感知延迟**:`SyncPermsService` 的 L-R11-4 优化(pure-add 不清 `UserDetailsLoader.CleanByProduct`)基于"loadPerms 对任何 user 的结果都不会因为新增 perm 变化"的假设,但 `loadPerms` 对 SuperAdmin / ADMIN / DEVELOPER / DEV-dept 启用成员走的是 `FindAllCodesByProductCode`——新增 perm **会**立刻改变这条分支的返回集,缓存没清等价于最长 5min TTL 的权限延迟。
|
|
|
-> 4. **权限级别纵向防护的对称性**:`UpdateRoleLogic` 有 L-R12-3 `!caller.IsSuperAdmin && req.PermsLevel < role.PermsLevel → 403` 防提权;但 `CreateRoleLogic` 不做任何 caller permsLevel 与 `req.PermsLevel` 的比较,让 product ADMIN 直接新建一个 permsLevel=1 的角色后 BindRoles 给下属,绕过 `GuardRoleLevelAssignable` 对"同级"的拦截,横向等价于"给 DEVELOPER 签发一张 ADMIN 入场券"。
|
|
|
+> R10~R17 已发现的核心漏洞(包括本轮编号为 H-R17-1/2/3、M-R17-1/2、L-R17-1~6)经核查均已在代码中落地修复并附注释(`createUserLogic.go` / `deleteDeptLogic.go` / `updateDeptLogic.go` / `createRoleLogic.go` / `deleteRoleLogic.go` / `syncPermsService.go` / `updateUserStatusLogic.go` / `changePasswordLogic.go` / `createDeptLogic.go` / `deptTreeLogic.go` 等)。本轮仅列出**新增发现**。
|
|
|
|
|
|
---
|
|
|
|
|
|
## 🚩 核心逻辑漏洞 (High Risk)
|
|
|
|
|
|
-### H-R17-1 · `CreateUser` / `CreateProductLogic` 对目标 `sys_dept` 无事务锁,与 `DeleteDept` 存在 TOCTOU 竞态(orphan user + 幽灵部门缓存 权限升级)
|
|
|
-
|
|
|
-**位置**
|
|
|
-
|
|
|
-- `internal/logic/user/createUserLogic.go:86-140`(`FindOne(deptId)` 是 cached 读,随后 `Insert(sys_user)` 不在任何事务中,`sys_dept` 无 S 锁)
|
|
|
-- `internal/logic/product/createProductLogic.go`(为新产品自动注入 `admin_<productCode>` 行时同样走非 tx `SysUserModel.Insert`)
|
|
|
-- 对比参照:`internal/logic/dept/deleteDeptLogic.go:44-68`(已经用 `SELECT id FROM sys_dept WHERE id=? FOR UPDATE` + `SELECT id FROM sys_user WHERE deptId=? FOR SHARE` 做"删除 → 检查既有用户"的串行化)
|
|
|
-
|
|
|
-**描述**
|
|
|
-
|
|
|
-`DeleteDept` 已经通过 X 锁 + `FOR SHARE` 锁把"既有用户占着这个部门就不让删"编织进事务:
|
|
|
-
|
|
|
-```44:68:internal/logic/dept/deleteDeptLogic.go
|
|
|
-var deptId int64
|
|
|
-lockQuery := fmt.Sprintf("SELECT `id` FROM %s WHERE `id` = ? FOR UPDATE", l.svcCtx.SysDeptModel.TableName())
|
|
|
-if err := session.QueryRowCtx(ctx, &deptId, lockQuery, req.Id); err != nil {
|
|
|
- return response.ErrNotFound("部门不存在")
|
|
|
-}
|
|
|
-// ... 略 ...
|
|
|
-var userIds []int64
|
|
|
-userQuery := fmt.Sprintf("SELECT `id` FROM %s WHERE `deptId` = ? FOR SHARE", l.svcCtx.SysUserModel.TableName())
|
|
|
-if err := session.QueryRowsCtx(ctx, &userIds, userQuery, req.Id); err != nil {
|
|
|
- return err
|
|
|
-}
|
|
|
-if len(userIds) > 0 {
|
|
|
- return response.ErrBadRequest("该部门下仍有关联用户,无法删除")
|
|
|
-}
|
|
|
-return l.svcCtx.SysDeptModel.DeleteWithTx(ctx, session, req.Id)
|
|
|
-```
|
|
|
-
|
|
|
-但这个串行化**只对"既有用户 vs 删除"有效**:`CreateUser` 这条"未来用户 vs 删除"链路完全游离事务外——
|
|
|
-
|
|
|
-```86-140:internal/logic/user/createUserLogic.go
|
|
|
-if req.DeptId > 0 {
|
|
|
- newDept, derr := l.svcCtx.SysDeptModel.FindOne(l.ctx, req.DeptId) // 命中 sqlc 低层缓存 / stale 读
|
|
|
- if derr != nil {
|
|
|
- return nil, response.ErrBadRequest("部门不存在")
|
|
|
- }
|
|
|
- if newDept.Status != consts.StatusEnabled {
|
|
|
- return nil, response.ErrBadRequest("目标部门已停用")
|
|
|
- }
|
|
|
- // DeptPath 校验 ...
|
|
|
-}
|
|
|
-
|
|
|
-hashedPwd, err := bcrypt.GenerateFromPassword([]byte(req.Password), bcrypt.DefaultCost)
|
|
|
-// ... 纯非事务 Insert,无任何 sys_dept 锁参与 ...
|
|
|
-result, err := l.svcCtx.SysUserModel.Insert(l.ctx, &userModel.SysUser{
|
|
|
- DeptId: req.DeptId,
|
|
|
- // ...
|
|
|
-})
|
|
|
-```
|
|
|
-
|
|
|
-竞态时序(T1 = SuperAdmin 删部门,T2 = ADMIN 创建用户):
|
|
|
-
|
|
|
-1. T2 执行 `FindOne(X)`,命中 sqlc 低层缓存或 DB 非锁读,拿到 `dept X{status=Enabled}`;DeptPath / DeptType / Status 所有校验通过。
|
|
|
-2. T1 开 tx → `SELECT ... FOR UPDATE` 锁住 `sys_dept[X]`。
|
|
|
-3. T1 `SELECT id FROM sys_user WHERE deptId=X FOR SHARE`:T2 **尚未插入**(Insert 发生在 bcrypt 之后,bcrypt default cost 约 60–100ms),返回空集。
|
|
|
-4. T1 `DELETE FROM sys_dept WHERE id=X`,commit,释放 X 锁。go-zero 的 sqlc `DelCache(cacheSysDeptIdPrefix+X)` 在 commit 之前已经执行过(见 H-R17-2)。
|
|
|
-5. T2 `bcrypt.GenerateFromPassword` 完成,`Insert sys_user` 落库,`deptId=X`。该 INSERT **不触及 sys_dept**(表级无外键、无触发器),DB 不会阻止。
|
|
|
-6. 产生 orphan 用户:`sys_user.deptId=X`,`sys_dept.id=X` 不存在。
|
|
|
-
|
|
|
-**紧接着的二次放大**:T2 触发缓存层的"幽灵部门"后果——
|
|
|
-
|
|
|
-- **良性分支**:`UserDetailsLoader.loadDept` 下次 miss 时 `SysDeptModel.FindOne(X) → ErrNotFound`,`loadOk=false` → `ErrLoaderDegraded` → jwtauthMiddleware 503,该用户**永远无法登录**直到运维手动 data fix。这是高可用事故,但不是权限事故。
|
|
|
-- **恶性分支**:如果 sys_dept 低层缓存里还残留着 X 的旧行(H-R17-2 描述的"commit 后幽灵快照",或者任何并发 `FindOne` 在 T1 的 `DelCache` 与 commit 之间把旧行回填),`loadDept` 成功返回旧 `{Path: "/1/5/", DeptType: DEV, Status: Enabled}`。用户携带这条"已死部门"登录成功,并且:
|
|
|
- - `DeptPath = "/1/5/"` 继续被 `checkDeptHierarchy` / `CheckAddMemberAccess` 当作真实部门链判据;
|
|
|
- - 若残留快照是 DEV 部门,任意 `loadPerms` 的产品将对该用户走"DEV 部门全权"分支(`FindAllCodesByProductCode`),**凭空获得任何产品的全部权限**,且完全不在组织架构视图中(`DeptTree` 查询时已无该 dept 行)。
|
|
|
-
|
|
|
-`CreateProductLogic` 同构受害:auto-provision 的 initial admin 行同样走 `SysUserModel.Insert` 而不是 `InsertWithTx`,即使调用方已经是 SuperAdmin 也一样会踩这个坑(SuperAdmin 删部门 + 另一个 SuperAdmin 同时创产品,两个 P0 管理员并发一点不罕见)。
|
|
|
-
|
|
|
-**影响**
|
|
|
-
|
|
|
-- 业务数据完整性破口:`sys_user.deptId` 丧失"必然引用已存在 sys_dept"的不变式,下游所有基于 deptPath 前缀匹配的鉴权、以及 DeptTree/UserList 的展示聚合都会在事实层面出现不一致;
|
|
|
-- 当缓存幽灵(H-R17-2)叠加上时,orphan 用户可以**无感继承一个已被超管删除的 DEV 部门的全部跨产品权限 ≤5min**——即便被删的 DEV 部门本意是"这批研发被整体调离";
|
|
|
-- 运维侧不容易察觉:orphan 行本身登录 503 看上去是"个别用户异常",与稳定态下的"DB 抖动"几乎无法区分,审计事件不会标记 deptId 指向已死部门;
|
|
|
-- 与 R16 的 L-R16-1(ADMIN 挪人出 DeptPath 外)互补:那一条堵"挪出",这一条堵"新建时就漂在外面"。
|
|
|
-
|
|
|
-**修复方案**
|
|
|
-
|
|
|
-把 `CreateUser` / `CreateProductLogic` 的用户建行动作移进同一个事务,并在事务内用 `FindOneForShareTx` 对目标 `sys_dept` 取 S 锁:
|
|
|
-
|
|
|
-```go
|
|
|
-// CreateUser 改写示意
|
|
|
-var id int64
|
|
|
-err = l.svcCtx.SysUserModel.TransactCtx(l.ctx, func(ctx context.Context, session sqlx.Session) error {
|
|
|
- if req.DeptId > 0 {
|
|
|
- newDept, derr := l.svcCtx.SysDeptModel.FindOneForShareTx(ctx, session, req.DeptId)
|
|
|
- if derr != nil {
|
|
|
- if errors.Is(derr, sqlx.ErrNotFound) {
|
|
|
- return response.ErrBadRequest("部门不存在或已删除")
|
|
|
- }
|
|
|
- return derr
|
|
|
- }
|
|
|
- if newDept.Status != consts.StatusEnabled {
|
|
|
- return response.ErrBadRequest("目标部门已停用")
|
|
|
- }
|
|
|
- if newDept.DeptType == consts.DeptTypeDev && !caller.IsSuperAdmin {
|
|
|
- return response.ErrForbidden("仅超级管理员可将用户调入研发部门")
|
|
|
- }
|
|
|
- if !caller.IsSuperAdmin && !strings.HasPrefix(newDept.Path, caller.DeptPath) {
|
|
|
- return response.ErrForbidden("无权在非自己管辖的部门下创建用户")
|
|
|
- }
|
|
|
- }
|
|
|
- // bcrypt 的慢计算放到事务外(见下一行),事务内只做锁 + Insert
|
|
|
- result, ierr := l.svcCtx.SysUserModel.InsertWithTx(ctx, session, &userModel.SysUser{ /* 带 hashedPwd */ })
|
|
|
- if ierr != nil {
|
|
|
- return ierr
|
|
|
- }
|
|
|
- id, _ = result.LastInsertId()
|
|
|
- return nil
|
|
|
-})
|
|
|
-```
|
|
|
-
|
|
|
-两点注意:
|
|
|
-
|
|
|
-1. `bcrypt.GenerateFromPassword` 必须在**事务外**完成,否则 100ms 的 bcrypt 会把 sys_dept S 锁持有时长拉高,阻塞 `DeleteDept` / `UpdateDept` 的 X 锁竞争。bcrypt 的产物 `hashedPwd` 在事务外算好,tx 内只做锁 + INSERT。
|
|
|
-2. `CreateProductLogic` 的 auto-provision 用户 INSERT 目前跟产品创建是同一 tx 的兄弟事务 / post-commit 步骤(`compensateCreatedRows` 的兜底对象),改造时可以把初始 admin 行拿到产品 tx 里一起锁 `sys_dept[0]`(如果默认 deptId=0 代表"无部门"则不取锁即可;如果是具体 deptId 就必须取 S 锁),避免 compensate 路径复杂化。
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### H-R17-2 · `DeleteDept` / `DeleteRole` / `UpdateDept` 等 `*WithTx` 路径的 sqlc `ExecCtx` "exec 先行、DelCache 后行"语义在事务内退化为**提交前清缓存**,引入幽灵快照窗口
|
|
|
-
|
|
|
-**位置**
|
|
|
-
|
|
|
-- go-zero `core/stores/sqlc/cachedsql.go::CachedConn.ExecCtx`:`exec(ctx, cc.db) → DelCache(keys...)`,`cc.db` 在 tx 内是 `session`,exec 仅"把语句排进事务日志",DB 可见性要等外层 `TransactCtx` 的 commit;DelCache 却立刻对 Redis 下手。
|
|
|
-- `internal/model/dept/sysDeptModel_gen.go:88-95` `DeleteWithTx` 调用路径
|
|
|
-- `internal/model/dept/sysDeptModel_gen.go:122-134` `BatchDeleteWithTx` 同构
|
|
|
-- `internal/model/role/sysRoleModel_gen.go` `DeleteWithTx` / `UpdateWithTx`(role 侧也踩同一模式)
|
|
|
-- 消费方:`internal/logic/dept/deleteDeptLogic.go:68`、`internal/logic/role/deleteRoleLogic.go`、`internal/logic/dept/updateDeptLogic.go`
|
|
|
-
|
|
|
-**描述**
|
|
|
-
|
|
|
-go-zero `sqlc.CachedConn.ExecCtx` 伪代码:
|
|
|
-
|
|
|
-```go
|
|
|
-func (cc CachedConn) ExecCtx(ctx, exec, keys...) {
|
|
|
- res, err := exec(ctx, cc.db) // 事务内这里的"ctx + db"实际是 session.ExecCtx
|
|
|
- if err != nil { return nil, err }
|
|
|
- if err := cc.DelCacheCtx(ctx, keys...); err != nil { return nil, err }
|
|
|
- return res, nil
|
|
|
-}
|
|
|
-```
|
|
|
-
|
|
|
-当 `DeleteWithTx(ctx, session, id)` 被 `TransactCtx` 包住时:
|
|
|
-
|
|
|
-1. `session.ExecCtx(DELETE FROM sys_dept WHERE id=?, id)`:SQL 进入事务日志,**其他事务看不到**该行已被删除(RR 隔离级别)。
|
|
|
-2. `DelCacheCtx(cacheSysDeptIdPrefix+id)`:Redis `DEL` 立刻生效,缓存从此刻起为 miss。
|
|
|
-3. 后续 tx 内可能还有多条 SQL 执行……最终外层 `TransactCtx` 走到 commit。
|
|
|
-
|
|
|
-在 (2) 与 commit 之间的窗口里:
|
|
|
-
|
|
|
-- 任意并发事务 T3 调用 `SysDeptModel.FindOne(id)` → 缓存 miss → `SELECT * FROM sys_dept WHERE id=?` → **读到旧行**(RR 下 T3 看不到 T1 未提交的 DELETE)→ 写回 Redis 覆盖 `DelCache` 的空槽。
|
|
|
-- T1 commit 之后,`sys_dept[id]` 真正消失,但 Redis 里仍有 T3 刚写回的旧行,生存期至少到本 key 的 TTL(默认 `sqlc.WithExpiry` 通常是小时级)。
|
|
|
-
|
|
|
-**影响**
|
|
|
-
|
|
|
-1. **`DeleteDept` 路径**:`sys_dept` 在删除后仍残留 cached 旧行,长达 TTL。任何下游 `UserDetailsLoader.loadDept` 都会拿到这条旧行:
|
|
|
- - 组合 H-R17-1 的 orphan 用户:新用户凭"幽灵部门"拿到完整 DeptPath,等价于**未被剔除**的部门子树继续承担权限载体。
|
|
|
- - 即使没有新 orphan 用户,已被调离的遗留用户(管理员把 deptId 改到别的 dept 之后删掉了原 dept)如果刚好在窗口内触发 UD cache miss,还是会按"原部门 DeptType=DEV"拿到全权,直到 TTL 到期。
|
|
|
-2. **`DeleteRole` 路径**:`sys_role[id]` cached 幽灵角色的消费面较窄——`ResolveOwnRoleOr404` 拿到后会走 `RequireProductAdminFor` 或后续 UPDATE/DELETE 的 CAS,commit 后真正撞不到行即回 `ErrUpdateConflict` → 409。不逃逸权限,但让删除后的后续操作出现"魂在但肉体已毁"的 409,增加排障负担。
|
|
|
-3. **`UpdateDept` 路径**:`UpdateDept` 当前在 tx 内通过 `UpdateWithTx`(同样是 `ExecCtx` + DelCache)改字段;commit 前的 DelCache → 并发 FindOne 回填旧值 → commit 后幽灵快照。虽然 `updateDeptLogic.go` 在 post-commit 又调了一次 `BatchIncrementTokenVersionWithTx` + `UserDetailsLoader.CleanByUserIds`,但**没有针对 `sys_dept[id]` 低层缓存本身再做一次 post-commit 失效**,一旦前面的窗口发生,该 dept 的其他非当前用户路径(如 DeptTree、CreateUser、AddMember 的校验读)都会在 TTL 窗口内看到"DEV→NORMAL 之前"的 DeptType,等价于 L-R16-2 想堵的"权限收窄不立刻生效"。
|
|
|
-
|
|
|
-**修复方案**
|
|
|
-
|
|
|
-这是 go-zero 在事务语义下公认的坑,本项目已经在 `UpdateProductLogic` / `UpdateUserLogic` / `RemoveMemberLogic` 等路径上用过正确姿势——**tx 成功 commit 之后再补一次 post-commit DelCache**,不要把幂等性押在 sqlc 的内嵌 `ExecCtx` 钩子上。具体:
|
|
|
-
|
|
|
-1. 给 `sys_dept` / `sys_role` 模型暴露 `InvalidateDeptCache(ctx, id)` / `InvalidateRoleCache(ctx, id)`(参考既有 `InvalidateProductCache` 的签名:id-based 三键一并清);
|
|
|
-2. `DeleteDept` / `UpdateDept` / `DeleteRole` / `UpdateRole` 在 `TransactCtx` 返回 nil 后,走 `loaders.DetachCacheCleanCtx` 再调一次该失效方法;
|
|
|
-3. 对 `UpdateDept` 当前"dept.Status / dept.DeptType 权限收窄"的支路同样补一次 post-commit `InvalidateDeptCache`,把 L-R16-2 的修复真正合拢——否则"user tokenVersion 已 bump 但 dept 缓存里还是旧 DeptType"意味着下次 UD 重建时仍会按旧 DeptType 走"DEV 全权"分支,现在的 BatchIncrementTokenVersionWithTx 相当于白做。
|
|
|
-
|
|
|
-长期方案:自研 `DeleteWithTx` / `UpdateWithTx` 包装层,把 `DelCache` 从 `ExecCtx` 里摘出来,只在事务成功 commit 后执行(`TransactCtx` 里注册 `OnCommit` 钩子)。这样所有 `*WithTx` 路径一次性脱离本陷阱,避免逐个调用点打补丁。
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### H-R17-3 · `CreateRoleLogic` 缺失 caller permsLevel 与 `req.PermsLevel` 的纵向对称校验,product ADMIN 可借"建新角色 + BindRoles"实现下属纵向提权
|
|
|
-
|
|
|
-**位置**
|
|
|
-
|
|
|
-- `internal/logic/role/createRoleLogic.go:33-76`(`PermsLevel` 只做 1–999 的合法性检查,不比 caller 的 assignable level)
|
|
|
-- 对比参照:`internal/logic/role/updateRoleLogic.go` L-R12-3 分支(`!caller.IsSuperAdmin && req.PermsLevel < role.PermsLevel` 明确 403)
|
|
|
-
|
|
|
-**描述**
|
|
|
-
|
|
|
-项目"纵向权限防护"的核心不变式在 `auth/access.go::GuardRoleLevelAssignable` / `CheckRoleLevelAgainst`:
|
|
|
-
|
|
|
-```237:240:internal/logic/auth/access.go
|
|
|
-if rolePermsLevel <= snap.Level {
|
|
|
- return response.ErrForbidden("不能分配权限级别高于自身的角色(含同级)")
|
|
|
-}
|
|
|
-```
|
|
|
-
|
|
|
-`UpdateRoleLogic` 在 R12 那轮也补齐了"非超管不能把既有角色往上调档"的防护。但 `CreateRoleLogic` 的 `PermsLevel` 校验只做了字面校验:
|
|
|
-
|
|
|
-```53-55:internal/logic/role/createRoleLogic.go
|
|
|
-if req.PermsLevel < 1 || req.PermsLevel > 999 {
|
|
|
- return nil, response.ErrBadRequest("权限级别必须在 1-999 之间")
|
|
|
-}
|
|
|
-```
|
|
|
-
|
|
|
-product ADMIN 可以走如下两步实现"把下属拉到与自己相同乃至更高的 assignable level":
|
|
|
-
|
|
|
-1. `CreateRole(ProductCode=P, PermsLevel=1)` → 新建 `R_super`,通过,因为 `RequireProductAdminFor` 放行 product ADMIN,`PermsLevel=1` 只看字面范围;
|
|
|
-2. `BindRoles(userId=D, roleIds=[R_super])` → `GuardRoleLevelAssignable` 判断 caller 的 assignable snapshot,`HasFullProductPerms(product ADMIN)` 返回 `true` → 直接放行(见 `access.go::CheckRoleLevelAgainst`);
|
|
|
-3. 下属 D 从此持有 `sys_user_role{roleId=R_super}`,下一次 UD 重建后 `ud.MinPermsLevel = 1`,在 `LoadCallerAssignableLevel` / `BindRoles` / `SetUserPerms` 中可把产品内任何 >1 的角色分配给他人——横向等价于"DEVELOPER 被签了 ADMIN 入场券"。
|
|
|
-
|
|
|
-这条链并不是在分配既有角色的环节被 `GuardRoleLevelAssignable` 拦截(ADMIN 确实全权),而是**在创建角色时先把弹药造好**:新角色的 PermsLevel 不受 caller 自身 assignable level 的约束。`UpdateRoleLogic` 的 R12-3 防护没有覆盖"先建后提升"的构造式等价路径。
|
|
|
-
|
|
|
-**影响**
|
|
|
-
|
|
|
-- 横向提权(escalate-by-delegate):product ADMIN 把一个 DEVELOPER / MEMBER 提到与自己同级甚至更高 assignable level,之后该下属的操作就会被审计记录为下属本人所为,ADMIN 可以"借刀杀人",审计链的"谁最终动的手"追溯性被破坏;
|
|
|
-- 横向绕 `CheckMemberTypeAssignment`:那个函数只管 memberType 的同级拦截,不涉及 role.permsLevel,故此条攻击路径独立成立;
|
|
|
-- 与既有 L-R13-2(`SetUserPerms` 拒绝对 ADMIN/DEVELOPER/SuperAdmin 下 DENY)不冲突:DENY 只是 perm 粒度兜底,role 粒度下 R_super 自带全部 perm 视角(配合 L-R11-4 的 loadPerms 全权分支逻辑),DENY 拦不住。
|
|
|
-
|
|
|
-**修复方案**
|
|
|
-
|
|
|
-CreateRole 加入与 UpdateRole 对称的 `GuardRoleLevelAssignable`/`CheckRoleLevelAgainst` 校验:
|
|
|
-
|
|
|
-```go
|
|
|
-caller := middleware.GetUserDetails(l.ctx)
|
|
|
-if caller == nil {
|
|
|
- return nil, response.ErrUnauthorized("未登录")
|
|
|
-}
|
|
|
-if !caller.IsSuperAdmin {
|
|
|
- snap, err := authHelper.LoadCallerAssignableLevel(l.ctx, l.svcCtx, caller)
|
|
|
- if err != nil {
|
|
|
- return nil, err
|
|
|
- }
|
|
|
- // 新建角色的权限等级必须严格低于 caller 的可分配等级;同级也拒(与 CheckRoleLevelAgainst 一致)
|
|
|
- if err := authHelper.CheckRoleLevelAgainst(snap, req.PermsLevel); err != nil {
|
|
|
- return nil, err
|
|
|
- }
|
|
|
-}
|
|
|
-```
|
|
|
-
|
|
|
-特别注意:product ADMIN / DEVELOPER 的 `HasFullProductPerms=true`,`CheckRoleLevelAgainst` 对 HasFullPerms 会直接放行——这正是本 issue 的根因。修复时**不能**简单复用 `CheckRoleLevelAgainst`,而要把"创建角色"定义为:
|
|
|
-
|
|
|
-- SuperAdmin:任意 PermsLevel 通过;
|
|
|
-- product ADMIN:`req.PermsLevel` 必须 **>= 2**(给自己留出唯一的"顶格"语义,ADMIN 自身的 assignable 等价于 sentinel 0),且必须 **>= caller 的最小 permsLevel + 1**(避免 ADMIN 以 permsLevel=1 的角色强制覆盖自身 assignable baseline);
|
|
|
-- DEVELOPER 及以下:走 `CheckRoleLevelAgainst(snap, req.PermsLevel)` 标准路径(snap.Level 由下属自己的角色决定,拒同级)。
|
|
|
-
|
|
|
-推荐把这条语义沉淀到一个新 helper `GuardCreateRolePermsLevel(ctx, svcCtx, caller, reqLevel) error`,与 `GuardRoleLevelAssignable` 并排放在 `access.go` 里,后续 UpdateRoleLogic 的 R12-3 支也可以复用。
|
|
|
+### H-R18-1 · `UpdateRoleLogic` 重命名角色时未失效旧名字索引缓存,残留"幽灵角色快照"
|
|
|
+- **描述**:
|
|
|
+ `internal/logic/role/updateRoleLogic.go` 通过 `SysRoleModel.UpdateWithOptLock(role, prevUpdateTime)` 落库,model 层(`sysRoleModel.go:101-116`)传给 `m.ExecCtx` 的失效键只有:
|
|
|
+ ```
|
|
|
+ sysRoleIdKey = cacheSysRoleIdPrefix + data.Id
|
|
|
+ sysRoleProductCodeNameKey = cacheSysRoleProductCodeNamePrefix + data.ProductCode + ":" + data.Name // ← 新 name
|
|
|
+ ```
|
|
|
+ 当本次 UPDATE 把 `name` 从 `X` 改成 `Y` 时,`cacheSysRoleProductCodeNamePrefix:<code>:X`(旧 name 键)**没有人清**,Redis 里仍然保留着指向原主键的索引值。
|
|
|
+- **影响**:
|
|
|
+ 1. `FindOneByProductCodeName(code, X)` 在 TTL(默认 7 天,走 sqlc 默认)内会继续命中旧索引 → 拿到"名为 X、实际已改为 Y"的脏行。
|
|
|
+ 2. 结合同样 R17 已修过但调用路径仍在的 `CreateRole`(依赖 DB `UNIQUE(productCode,name)` 而非预检):当运营先 "UpdateRole X→Y",紧接着 "CreateRole 名字=X" 时,DB 的 UNIQUE 允许新建一条合法的 X,但 Redis 索引仍指向**原来的主键**,导致 `FindOneByProductCodeName(code, X)` 直至下一次 Insert/Update 触发 DelCache 才会自愈——两条同名 `X` 的"旧主键映射 → 新 name=Y 的行"状态会持续长达 TTL。
|
|
|
+ 3. 与 R17 对 `DeleteRole` 补的 `InvalidateRoleCache` 对称缺口:rename 路径唯独漏掉这一对"旧 name 键"的显式失效。
|
|
|
+- **修复方案**:
|
|
|
+ 在 `UpdateRole` 里先快照 `prevProductCode/prevName`,`UpdateWithOptLock` 完成后显式调用 `InvalidateRoleCache(ctx, role.Id, prevProductCode, prevName)` 和 `InvalidateRoleCache(ctx, role.Id, role.ProductCode, role.Name)`——与 R17 `DeleteRoleLogic` 的失效顺序保持一致,避免"幽灵 role 快照"。若不想改 model 语义,也可直接在 `updateRoleLogic.go` 里手动 `l.svcCtx.Redis.DelCtx(ctx, "cache:sysRole:productCode:name:"+prevCode+":"+prevName)`:
|
|
|
+ ```go
|
|
|
+ prevProductCode := role.ProductCode
|
|
|
+ prevName := role.Name
|
|
|
+ // ... 赋值 req.Name 到 role.Name 之后
|
|
|
+ if err := l.svcCtx.SysRoleModel.UpdateWithOptLock(l.ctx, role, prevUpdateTime); err != nil { ... }
|
|
|
+ cleanCtx, cancel := loaders.DetachCacheCleanCtx(l.ctx)
|
|
|
+ defer cancel()
|
|
|
+ if prevName != role.Name {
|
|
|
+ l.svcCtx.SysRoleModel.InvalidateRoleCache(cleanCtx, role.Id, prevProductCode, prevName)
|
|
|
+ }
|
|
|
+ l.svcCtx.SysRoleModel.InvalidateRoleCache(cleanCtx, role.Id, role.ProductCode, role.Name)
|
|
|
+ ```
|
|
|
+
|
|
|
+### H-R18-2 · `UpdateDeptLogic` 冻结 NORMAL 部门仅递增 tokenVersion,未实际收窄权限,与注释声明不一致
|
|
|
+- **描述**:
|
|
|
+ `internal/logic/dept/updateDeptLogic.go:99-107` 的 `normalDeptFrozen` 分支判定为 "NORMAL 部门 Enabled→Disabled",注释声明为"**冻结本部门所有活动**,一并吊销"。实际行为:
|
|
|
+ 1. 事务内对受影响 userIds 走 `BatchIncrementTokenVersionWithTx` — 仅让旧 JWT 失效。
|
|
|
+ 2. `userDetailsLoader.go:loadPerms` 的授权计算里**只有 DEV 部门**分支依赖 `DeptStatus == Enabled`(第 543 行);对 NORMAL 成员,`DeptStatus` 从未进入权限计算。
|
|
|
+ 3. `checkDeptHierarchy`(`access.go:377-422`)也不读 `targetDept.Status`。
|
|
|
+ 4. 用户 `sys_user.Status` 保持不变。
|
|
|
+
|
|
|
+ 净效果:被"冻结"的 NORMAL 部门成员只是被强制退出一次登录;立即重新登录即可继续使用所有原有权限。审计日志里会留下 "affectedUsers=N revokedSessions=N",值班看上去像是"已吊销",但业务权限实际上**没有任何变化**。
|
|
|
+- **影响**:
|
|
|
+ 1. 对运营侧形成"已冻结假象":操作者执行"冻结某部门"后相信该部门无法再访问系统,但成员立刻重登就恢复;在出现"部门整体涉嫌违规 → 临时冻结调查"这类场景下放出越权窗口。
|
|
|
+ 2. 审计日志里的 `audit="UpdateDept"` 带 `revokedSessions` 字段容易被上级审阅/合规稽核误判为"已生效的访问控制措施"。
|
|
|
+ 3. 与 `updateUserStatusLogic` / `updateMemberLogic` / `updateProductLogic` 同族 "收窄" 路径的业务语义不对称——后三者都是"DB 字段落地 + loadPerms 分支响应"的完整闭环。
|
|
|
+- **修复方案**(择一,建议 A):
|
|
|
+ - **A. 让 `loadPerms` 对 NORMAL 成员也读 DeptStatus**:在 `userDetailsLoader.go:524-625` `loadPerms` 的普通成员分支最前面加 `if ud.DeptStatus != consts.StatusEnabled { ud.Perms = nil; return nil }`;同时 `middleware/jwtauthMiddleware.go` 的校验链里补 "`ud.DeptStatus != Enabled` → 401/403"。
|
|
|
+ - **B. 放弃冻结 NORMAL 部门的语义**:把注释里的"一并吊销"改掉,并在 `UpdateDeptLogic` 的 `normalDeptFrozen` 分支只失效缓存、不再 `BatchIncrementTokenVersionWithTx`,避免运营误解。
|
|
|
+ - A 与当前 `UpdateProduct` 禁用产品时的全员收窄口径完全对称,优先推荐。
|
|
|
|
|
|
---
|
|
|
|
|
|
## ⚠️ 健壮性与性能建议 (Medium/Low)
|
|
|
|
|
|
-### M-R17-1 · `SyncPermsService` 的 pure-add 分支对"全权用户"有最长 5min 的权限可见延迟
|
|
|
-
|
|
|
-**位置**
|
|
|
-
|
|
|
-- `internal/logic/pub/syncPermsService.go:163-180`(`if updated > 0 || disabled > 0` 才清 `CleanByProduct`)
|
|
|
-- `internal/loaders/userDetailsLoader.go:539-554`(SuperAdmin / ADMIN / DEVELOPER / DEV-dept 成员走 `FindAllCodesByProductCode`)
|
|
|
-
|
|
|
-**描述**
|
|
|
-
|
|
|
-L-R11-4 的注释写得很清楚:
|
|
|
-
|
|
|
-> 纯新增(added>0 && updated==0 && disabled==0)时不需要清 CleanByProduct。新增的 perm 在本次 SyncPerms 之前**不可能**已经被绑定到任何 role……loadPerms 对当前全体 user 的计算结果与上次结果完全一致。
|
|
|
-
|
|
|
-但"loadPerms 对当前全体 user 的计算结果完全一致"这一条只对**普通 MEMBER**成立——他们走 `FindPermIdsByRoleIds` + `FindPermIdsByUserIdAndEffectForProduct` 的路径,新 perm 没被任何 role/allow/deny 引用,确实不影响。**全权用户走的是 `FindAllCodesByProductCode(productCode)` 单条分支**,这条查询返回的是该产品下所有 status=Enabled 的 perm 全集,新增任意 perm 都会让集合变大。
|
|
|
-
|
|
|
-具体时序:
|
|
|
-
|
|
|
-1. 产品 P 下已有 perm `A`/`B`,SuperAdmin `S` 登录,`ud.Perms = [A, B]`,写入 Redis(TTL=5min)。
|
|
|
-2. P 的业务服务发布 `v2`,新增 perm `C`,`SyncPerms` 走 pure-add 分支,`updated=disabled=0` → **跳过** `CleanByProduct`。
|
|
|
-3. `S` 在接下来 5 分钟内的每次 `/auth/userinfo` / login / refreshToken / 下游业务拉取 UD 都会命中 Redis 旧快照,`ud.Perms` 仍然是 `[A, B]`。
|
|
|
-4. 下游业务按 `Perms` 列表鉴权 `/v2/C`,S 被 403,"超管登录就是拉不到新接口"在发版当天很容易引起误判。
|
|
|
-
|
|
|
-**影响**
|
|
|
-
|
|
|
-- 发版 SLA:承诺下游"SyncPerms 成功即可使用新权限"的口径在这四类用户上被静默违反;
|
|
|
-- 审计链扭曲:`GetUserPerms` 的 perms 列表对全权用户最长延迟 5min,运营查验"这个超管到底能不能访问 C"时,看到的快照和真实授权的上游 DB 视图不一致;
|
|
|
-- 并不逃逸"最终一致",属 Medium 的感知可见性问题。
|
|
|
-
|
|
|
-**修复方案**
|
|
|
-
|
|
|
-两个可选路径:
|
|
|
-
|
|
|
-1. 精准失效(推荐):`SyncPerms` 在 `added > 0` 且当前产品存在全权用户(SuperAdmin + 本产品 ADMIN/DEVELOPER + DEV-dept 启用成员)时,只清这些用户的 UD 缓存,避免 CleanByProduct 的"全产品成员被穿回 DB"开销。实现上:
|
|
|
- - `added > 0` → 事务内 `SELECT userId FROM sys_product_member WHERE productCode=? AND status=Enabled AND memberType IN ('ADMIN','DEVELOPER')` + 对 `sys_user` 全表 `WHERE isSuperAdmin=1` + 对 DEV 部门用户的查询(或直接在 tx 外跑,不影响锁序);
|
|
|
- - 批量 `UserDetailsLoader.CleanByUserIds`。
|
|
|
-2. 简单保守:把 `if updated > 0 || disabled > 0` 扩成 `if added > 0 || updated > 0 || disabled > 0`,承担 `CleanByProduct` 的穿透开销作为代价。由于 SyncPerms 是 CI/CD 高频事件,这条偏"CleanByProduct 所有成员过一遍 Redis DEL",必要时再回退到方案 1。
|
|
|
-
|
|
|
-注释里的 `loadPerms 对当前全体 user 的计算结果完全一致` 必须同步改写——当前版本已经是错的,留着会把后人引向错误假设。
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### M-R17-2 · `UpdateUserLogic` 在"DEV→NORMAL 调动"+"status 冻结"并发发生时对 `tokenVersion` 做双倍递增,虽然功能无害,但注释与 sysUser 低层缓存 invalidation 次数不对称
|
|
|
-
|
|
|
-**位置**
|
|
|
-
|
|
|
-- `internal/logic/user/updateUserLogic.go`(R16-2 引入的 `devAccessRevoked` 分支会 `IncrementTokenVersionWithTx`)
|
|
|
-- `internal/model/user/sysUserModel.go::UpdatePassword`/`UpdateStatus`/`UpdateProfile` 各自会做 `tokenVersion+1`
|
|
|
-
|
|
|
-**描述**
|
|
|
-
|
|
|
-R16 给 `UpdateUser` 加上了"DEV → NORMAL 跨域调动时 tokenVersion+1";如果同一请求还命中 `status Enabled→Disabled`,`UpdateProfile` 内部本就会 `tokenVersion+1`(或 R16 的代码路径再走一次显式 `IncrementTokenVersionWithTx`),结果是事务内对同一 userId 做了 2 次自增。功能上毫无副作用(新 token 不会分叉,Redis 缓存还是会 post-commit Clean),但:
|
|
|
-
|
|
|
-- R16 的注释只解释了"DEV 调动"这一半为什么要 +1,没有声明"如果还叠加了 status change,递增会叠加";
|
|
|
-- 未来运维如果用 `tokenVersion` 做"有效会话批次号"的信号量分析,会看到"单次 UpdateUser 却 +2"的异常样本,产生噪声;
|
|
|
-- 双递增让 `UpdateProfile` 的 CAS `WHERE updateTime=?` 在第二次 SQL 之前要依赖第一次 SQL 的结果(事务内一致),调试复杂。
|
|
|
-
|
|
|
-**修复方案**
|
|
|
-
|
|
|
-两种路径:
|
|
|
-
|
|
|
-1. 改语义:把"是否需要 bump tokenVersion"收归到 `UpdateUser` 这一层决策,内部只调一次 `IncrementTokenVersionWithTx(userId)` 或直接在 `UpdateProfile` 的 SQL 里多带一个 `bumpTokenVersion bool` 参数,tx 内只做 1 次 `tokenVersion+1`。
|
|
|
-2. 不改语义,在注释里明确声明"devAccessRevoked + statusChanged 重叠时会 +2,但对安全语义等价于 +1",并在 `IncrementTokenVersionWithTx` 的文档注释里补上"可安全叠加调用"。
|
|
|
-
|
|
|
-无论走哪条都建议新增一条 TC:单请求同时触发 dev 调出 + 冻结,断言新 tokenVersion = 旧 + N(N 取当前实现值),把行为锁死。
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### L-R17-1 · `CreateProductLogic` 的 initial admin 用户名 `admin_<productCode>` 可被任意 product ADMIN 预抢注,导致 SuperAdmin 上新产品时撞 UNIQUE(username) 回滚
|
|
|
-
|
|
|
-**位置**
|
|
|
-
|
|
|
-- `internal/logic/product/createProductLogic.go`(生成 `adminUsername := fmt.Sprintf("admin_%s", req.Code)` 然后 `SysUserModel.Insert`)
|
|
|
-- `internal/logic/user/createUserLogic.go:54`(username regex `^[a-zA-Z0-9_]{2,64}$` 不保留 `admin_` 前缀)
|
|
|
-
|
|
|
-**描述**
|
|
|
-
|
|
|
-product ADMIN 在自己产品下可用 `CreateUser` 直接创建用户名 `admin_acme`,这个用户跟未来真正要上线的 productCode `acme` 共用一个 UNIQUE(username) 槽位。超管过了几个月要创建产品 `acme`:
|
|
|
-
|
|
|
-1. `CreateProductLogic` 走到 auto-provision step,`username = "admin_acme"` 已存在;
|
|
|
-2. `SysUserModel.Insert` 撞 1062,`util.IsDuplicateEntryErr` 被 `compensateCreatedRows` 捕获,已创建的 sys_product / 可能的 role / perm 全部回滚;
|
|
|
-3. 超管必须让运维人肉 data fix 把抢注的 `admin_acme` 改名,或改用一个非冲突的 productCode。
|
|
|
-
|
|
|
-组合 R16 的 H-R16-1 等一系列权限收敛之后,这条攻击虽然不能**提权**,但可以:
|
|
|
-
|
|
|
-- 针对"已知下一批待上线 productCode"(内部 Roadmap 半公开场景)做批量抢注,让超管短期内根本无法上新产品,等价于一次**业务 DoS**;
|
|
|
-- 如果被抢注的 `admin_<code>` 之后被 product ADMIN 给了自己或同谋,产品上线失败的日志/告警把 SuperAdmin 的注意力引向"产品 code 冲突",抢注者获得时间差;
|
|
|
-- 与"product ADMIN 看不到其他产品列表"(M-2) 并不冲突——抢注者只要知道 code 就行,不需要看列表。
|
|
|
-
|
|
|
-**修复方案**
|
|
|
-
|
|
|
-- 短期:把 auto-provision 的 username 加上不可人工构造的前缀 / 后缀,例如 `svc_admin_<code>_<random_10hex>`,彻底脱离 `^[a-zA-Z0-9_]{2,64}$` 的"纯小写字母数字"可人工推测空间。
|
|
|
-- 中期:在 `CreateUser` 的 username 正则层面引入保留前缀黑名单(`admin_` / `svc_` / `root_` / `sys_` 等),非超管创建以这些前缀开头的 username 直接 400。
|
|
|
-- 长期:引入 username namespace 机制,把"系统账号"和"用户账号"隔离到不同 table / 不同 uniqueness 域(例如用 `sys_service_account` 存 auto-provision 行,与 `sys_user` 分离)。
|
|
|
+### M-R18-1 · `AddMemberLogic` / `SetUserPerms` / `BindRoles` 未复核 `ud.Status`,仅靠中间件兜底
|
|
|
+- 文件:`internal/logic/member/addMemberLogic.go`、`internal/logic/user/bindRolesLogic.go`、`setUserPermsLogic.go`
|
|
|
+- 这些管理接口均先 `middleware.GetUserDetails` 取 caller,随后进入业务流。caller.UD 的 5min 聚合缓存里的 `Status` 字段在"超管刚把 caller 冻结但 Redis Clean 抖动失败"窗口内仍是旧值,jwtauthMiddleware 的 `ud.Status != Enabled` 拦截同样可能被旁路。虽然 R11-R15 已经给高风险写入加了 `loadFreshMinPermsLevel` 强一致复核,但"caller 本身已被冻结"这一维度只靠缓存。
|
|
|
+- **建议**:在 `access.go` 新增 `RequireActiveCaller(ctx, svcCtx)`,在 HTTP 写路径入口一次性走 DB 强一致复核 caller 的 `sys_user.status`(类似 `loadFreshMinPermsLevel` 的缓存旁路)。成本是每次写 +1 次 `FindOne(caller.UserId)`,但写路径 QPS 不高,收益是彻底堵住"刚被冻结还能写"的 TTL 级 TOCTOU。
|
|
|
+
|
|
|
+### M-R18-2 · `DeptTreeLogic` 非超管分支仍全表 `FindAll` 再内存过滤
|
|
|
+- 文件:`internal/logic/dept/deptTreeLogic.go`
|
|
|
+- `checkDeptHierarchy` 对非超管要求 `DeptPath` 前缀匹配——完全可以在 DB 层用 `WHERE path LIKE CONCAT(?, '%')` 走索引,避免把全量 `sys_dept` 拉回进程内存再过滤。
|
|
|
+- **建议**:模型层补 `FindByPathPrefix(ctx, prefix)`(带 `path` 前缀索引命中),在 `DeptTreeLogic` 非超管分支改用它。对 300~500 个部门这种真实规模,内存滤选问题不大;但模型接口暴露 `FindAll` 让其它调用方未来踩到同类坑。
|
|
|
+
|
|
|
+### M-R18-3 · `loadMembership` 使用 `err == productmember.ErrNotFound` 进行裸等值比较
|
|
|
+- 文件:`internal/loaders/userDetailsLoader.go:473`
|
|
|
+- 现状:`productmember.ErrNotFound = sqlx.ErrNotFound`,裸等值成立;但今后任何一层包装(`fmt.Errorf("%w", err)` / 多级 model)都会让这条分支失效——走到 error 分支打日志并最终合并成 `ErrLoaderDegraded`,把"真的不是成员"退化为 503。
|
|
|
+- **建议**:统一改为 `errors.Is(err, productmember.ErrNotFound)`,与文件内其它 error 分支口径对齐(如 `loadUser` 的 `errors.Is(err, sqlx.ErrNotFound)`)。
|
|
|
+
|
|
|
+### M-R18-4 · `CreateUserLogic` 的强口径校验落在 `req.DeptId > 0` 内层,DB 落盘后的 `req.DeptId == 0`(超管)被直接放行
|
|
|
+- 文件:`internal/logic/user/createUserLogic.go:134-161`
|
|
|
+- 审计 H-R17-1 的 S 锁在 `req.DeptId > 0` 分支生效;超管走 `deptId=0` 分支完全跳过 `FindOneForShareTx`,同时 Insert 依然落成功行——这符合"只有超管能创建无部门用户"的业务约束,但会在历史数据里继续沉淀"DeptId=0 的 MEMBER 幽灵账号"。`checkDeptHierarchy` 虽然对这类账号 fail-close,但缺少一条"创建即审计"的告警事件,运维无法识别此类账号何时被创建以及谁创建。
|
|
|
+- **建议**:在超管 `deptId=0` 分支显式打一条 `logx.Infow("create user without dept", logx.Field("audit", "create_user_no_dept"), ...)`,方便事后回捞。
|
|
|
+
|
|
|
+### L-R18-1 · `GuardCreateRolePermsLevel` 的 `snap.HasFullPerms` 分支对 DEVELOPER 是事实上的死代码
|
|
|
+- 文件:`internal/logic/auth/access.go:261-282`
|
|
|
+- `CreateRoleLogic` 的入口已有 `authHelper.RequireProductAdminFor(role.ProductCode)` 把 DEVELOPER / MEMBER 全部挡在门外,因此 `GuardCreateRolePermsLevel` 内部 `HasFullPerms` 分支仅对 "超管"(已提前 return)和 "ADMIN" 两类 caller 生效。函数注释已说明"为未来 DEVELOPER 可建 sub-role 留接口",但需要显式的单元测试在 DEVELOPER 入口放开时捕获行为偏移;目前该分支没有任何调用路径会触发。
|
|
|
+- **建议**:要么删除 DEVELOPER 相关注释只保留"ADMIN"语义,要么增加一条明确的契约测试(入口临时放开 DEVELOPER 后验证 `permsLevel<=1` 被拒),避免注释与调用现实脱节。
|
|
|
+
|
|
|
+### L-R18-2 · `UpdateProduct(Disabled→Enabled)` 没有对应的回灌 tokenVersion 语义补偿
|
|
|
+- 文件:`internal/logic/product/updateProductLogic.go`
|
|
|
+- 产品 Enabled→Disabled 会 `BatchIncrementTokenVersionWithTx` + 全员缓存 Clean;反向的 Disabled→Enabled 只清缓存、不动 tokenVersion。理论上符合"升权不踢下线"的原则,但在"误禁用 → 立即启用"的运维回滚场景下,被踢下线的用户必须全部重登——这是业务可接受的代价,需要在接口文档 / 审计日志里显式声明,避免运维对"为什么已经恢复了还不能用旧 token"产生误解。
|
|
|
+- **建议**:在 `updateProductLogic.go` 的禁用分支审计日志里加字段 `audit_hint="sessions_revoked_irreversibly"`,或在 OpenAPI 文档里显式标注。
|
|
|
+
|
|
|
+### L-R18-3 · `loadPerms` 对"最终 perm 集合为空"的普通成员返回 `nil` 而非 `[]`
|
|
|
+- 文件:`internal/loaders/userDetailsLoader.go:603-623`
|
|
|
+- Go `encoding/json` 对 `[]string(nil)` 会输出 `null`,前端 / gRPC 客户端若按"数组"做判断会触发不必要的 defensive check;对比"有 perm 但全 deny 掉的用户"输出 `[]`,两种"空"表达不一致。
|
|
|
+- **建议**:`loadPerms` 出口处 `if ud.Perms == nil { ud.Perms = []string{} }`,保证响应体里 `perms` 恒为数组;对 JWT 签发路径同样友好。
|
|
|
+
|
|
|
+### L-R18-4 · `MemberListLogic` 的 `map[int64]struct{Username, Nickname string}` 匿名类型降低可读性
|
|
|
+- 文件:`internal/logic/member/memberListLogic.go:55-58`
|
|
|
+- 匿名结构体作为 map value 在 Go 里每次构造都推导类型,IDE 跳转 / pprof 分析不友好。改为具名类型(或直接用 `*userModel.SysUser`)能让 N>N 条成员列表的调试 / 性能归因更方便。
|
|
|
+- **建议**:本地定义 `type nickname struct { Username, Nickname string }`,或复用现有 `sysUserModel.UserWithMemberType` 风格的小结构体。
|
|
|
+
|
|
|
+### L-R18-5 · `CreateDeptLogic` 对 `req.Sort` 没有上限检查
|
|
|
+- 文件:`internal/logic/dept/createDeptLogic.go`
|
|
|
+- `Sort` 直接透传到 `sys_dept.sort int`。超管可以随意写 `math.MaxInt64`,虽然不会崩 DB,但会把部门树的排序稳定性依赖转嫁到业务前端;同类字段在 `SysRole` / `SysProduct` 也都没有保护。
|
|
|
+- **建议**:统一在 model 层(或前端)给 `sort` 加范围校验(例如 `[-100000, 100000]`),与 `permsLevel 1-999` 的口径一致;或者在接口契约里显式声明"sort 仅在同级部门间相对有效"。
|
|
|
+
|
|
|
+### L-R18-6 · `CreateProductLogic.compensateCreatedRows` 使用独立 ctx,但先后顺序"member → user → product"未对 FK 依赖建模
|
|
|
+- 文件:`internal/logic/product/createProductLogic.go:343-396`
|
|
|
+- 当前无外键约束,删除顺序无所谓;但如果未来对 `sys_product_member` / `sys_user_role` / `sys_user_perm` 加上 `ON DELETE RESTRICT`,`compensateCreatedRows` 的顺序需要与 FK 对齐。
|
|
|
+- **建议**:在函数顶部加一行注释说明"删除顺序假设无 FK 约束;如果加 FK,顺序必须调整"——降低未来 schema 演进时的踩坑概率。
|
|
|
+
|
|
|
+### L-R18-7 · `UpdateUserLogic` 的 `devAccessRevoked` 判定对 `oldDept.Status != Enabled` 也触发收窄路径
|
|
|
+- 文件:`internal/logic/user/updateUserLogic.go:184-193`
|
|
|
+- 判定 `devAccessRevoked=true` 的三元条件之一是 `newDept.Status != consts.StatusEnabled`;按 `updateUserLogic` 自身的前置校验(line 136-138)新部门必须 Enabled,这个分支实际上是"目标 dept 启用→未启用"的兜底,与事务外 FindOne 已做的校验重复。
|
|
|
+- **建议**:在注释里把这条兜底的"双重冗余"显式标注,或者直接删掉 `newDept.Status != consts.StatusEnabled` 这一支(因为事务外非空分支必然 Enabled)。
|
|
|
+
|
|
|
+### L-R18-8 · `userDetailsLoader.BatchDel` 中 `batchUnregister` 的错误被 `logCacheInvalidationErr` 吞掉,但不上报熔断
|
|
|
+- 文件:`internal/loaders/userDetailsLoader.go:294-314`
|
|
|
+- 大批量缓存失效失败是"Redis 抖动"的强信号,目前只有日志、没有 metric。生产环境如果 Redis 长时间不可用,UD 聚合缓存和 sys_user 低层缓存同时保持陈旧,最长 5min 期间的任何授权判定都不得退化到 DB。
|
|
|
+- **建议**:在 `logCacheInvalidationErr` 的非 ctx-canceled 分支增一次 `prometheus.CounterVec.Inc`(或 go-zero 的 metric),告警阈值 5xx/min → on-call,让运维第一时间感知"缓存失效链路挂了"。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### L-R17-2 · `CreateUser` / `CreateProductLogic` 未显式设置 `Avatar`,导致 `sql.NullString` 以 `{Valid:true, String:""}` 默认落库,对消费方而言歧义
|
|
|
-
|
|
|
-**位置**
|
|
|
-
|
|
|
-- `internal/logic/user/createUserLogic.go:120-134`(Insert 字段集未包含 Avatar)
|
|
|
-- `internal/logic/product/createProductLogic.go`(initial admin 构造 `SysUser{…}` 同样未设 Avatar)
|
|
|
-- `internal/model/user/sysUserModel_gen.go::SysUser.Avatar`(类型为 `sql.NullString`)
|
|
|
-- `internal/loaders/userDetailsLoader.go::loadSysUser`(`ud.Avatar = u.Avatar.String`)
|
|
|
-
|
|
|
-**描述**
|
|
|
-
|
|
|
-Go 结构体字面量初始化时未设置的 `sql.NullString` 字段默认 `{Valid:false, String:""}`。但 `SysUserModel.Insert` 走的是 sqlx 的结构体到列映射,Valid=false 的 NullString 写入 DB 时会落 `NULL`,此时读取出来后 `u.Avatar.String=""` & `u.Avatar.Valid=false`;`ud.Avatar = u.Avatar.String` 会把 `""` 拷到 UD.Avatar。
|
|
|
-
|
|
|
-看起来不会出问题——但**默认行为依赖"Go 零值 + sql.NullString 语义链"**,一旦未来有人把 `SysUser.Avatar` 改成纯 `string`(去掉 NullString),现在的 Insert 字段集不变就会落一个 `''`;而此前已有用户的 Avatar 可能 `NULL`(Valid=false),DB 层 inplace migrate 时会出现部分行 `NULL`、部分行 `''` 并存的脏数据。前端把"未上传头像"判断为 `avatar == null` 还是 `avatar == ''`?两种写法都合理,但两种数据并存会让"默认头像"只对一半用户生效。
|
|
|
-
|
|
|
-**修复方案**
|
|
|
-
|
|
|
-在 CreateUser / CreateProductLogic 的 Insert 字面量里显式写:
|
|
|
-
|
|
|
-```go
|
|
|
-Avatar: sql.NullString{Valid: false},
|
|
|
-```
|
|
|
-
|
|
|
-或者把 Avatar 字段从 NullString 降级为普通 string + 业务层在 loadSysUser 后显式处理"空串 = 未上传"语义。更 aggressive 一点,直接在 SysUser 的 Insert helper 里对所有 Nullable 字段走显式默认值构造器,避免散落各处的"忘记赋值→依赖零值"。
|
|
|
+## 二次核验:R17 已修项留存状态(抽查)
|
|
|
+
|
|
|
+| 编号 | 预期修复位点 | 实际状态 |
|
|
|
+| :--- | :--- | :--- |
|
|
|
+| H-R17-1 | `createUserLogic.go` / `createProductLogic.go` 事务内 `FindOneForShareTx(sys_dept)` | ✅ 已落地,注释齐备 |
|
|
|
+| H-R17-2 | `deleteDeptLogic.go` / `updateDeptLogic.go` / `deleteRoleLogic.go` post-commit `InvalidateDeptCache` / `InvalidateRoleCache` | ✅ 已落地(但 `updateRoleLogic` rename 路径漏 old-name 键——见 H-R18-1) |
|
|
|
+| H-R17-3 | `createRoleLogic.go` 非超管 `permsLevel>=2` | ✅ 通过 `GuardCreateRolePermsLevel` 落地 |
|
|
|
+| M-R17-1 | `syncPermsService.go` 全量触发 `CleanByProduct` | ✅ 已落地 |
|
|
|
+| M-R17-2 | `updateUserLogic.go` tokenVersion 双递增的语义澄清 | ✅ 注释齐备 |
|
|
|
+| L-R17-1 | `createUserLogic.go` 保留前缀 `admin_` / `svc_` / `root_` / `sys_` | ✅ 已落地 |
|
|
|
+| L-R17-2 | `sql.NullString{Valid: false}` 显式落 NULL | ✅ 已落地 |
|
|
|
+| L-R17-3 | `deptTreeLogic.go` 空 DeptPath / 空 tree 审计日志 | ✅ 已落地 |
|
|
|
+| L-R17-4 | `changePasswordLogic.go` 同密码短路先于 bcrypt | ✅ 已落地 |
|
|
|
+| L-R17-5 | `createDeptLogic.go` Insert→UpdatePathWithTx 两步 | ✅ 已落地 |
|
|
|
+| L-R17-6 | `updateUserStatusLogic.go` 对解冻也 tokenVersion+1 的注释 | ✅ 已落地 |
|
|
|
|
|
|
---
|
|
|
|
|
|
-### L-R17-3 · `DeptTreeLogic` 对非超管只返回 DeptPath 子树,但非成员子树被整体丢弃时没有审计日志标记,排障时无法区分"用户无权"和"DB 抖动返空"
|
|
|
-
|
|
|
-**位置**
|
|
|
-
|
|
|
-- `internal/logic/dept/deptTreeLogic.go:52-63`(非超管分支过滤非子树部门时直接 continue,不 log)
|
|
|
+## 小结
|
|
|
|
|
|
-**描述**
|
|
|
+本轮新增发现 **2 项高风险**(H-R18-1 缓存键残留、H-R18-2 NORMAL 部门冻结语义偏差),**4 项中风险**与 **8 项低风险**。
|
|
|
|
|
|
-非超管(含产品 ADMIN / DEVELOPER / MEMBER)调用 `DeptTree`,`DeptPath` 前缀不匹配的部门都被静默过滤。当 `caller.DeptPath == ""`(数据链路异常、user 刚被超管改到部门外等)时,直接返回空数组,UI 显示"无部门",看起来跟"DB 抖动返空"无法区分。L-R15-2 已经把 fullAccess 收敛给 SuperAdmin,但**对过滤掉多少 dept**没有可观测性。
|
|
|
-
|
|
|
-**修复方案**
|
|
|
-
|
|
|
-- 对 `len(list) > 0 && len(filtered) == 0` 且 `caller.DeptPath != ""` 的边界打一条 INFO:caller 有 DeptPath 但过滤后为空(可能是 DeptPath 指向已删除部门 = H-R17-1 的前奏迹象);
|
|
|
-- 对 `caller.DeptPath == ""` 直接 log WARN,提示运维可能是 L-R16-1 / H-R17-1 类的孤儿账号。
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### L-R17-4 · `changePasswordLogic.go::ChangePassword` 的"旧密码等于新密码"校验顺序让 bcrypt compare 先于字符串等值比较,浪费 ~60ms CPU
|
|
|
-
|
|
|
-**位置**
|
|
|
-
|
|
|
-- `internal/logic/auth/changePasswordLogic.go:60-67`
|
|
|
-
|
|
|
-**描述**
|
|
|
-
|
|
|
-```60-67:internal/logic/auth/changePasswordLogic.go
|
|
|
-if err := bcrypt.CompareHashAndPassword([]byte(user.Password), []byte(req.OldPassword)); err != nil {
|
|
|
- logx.WithContext(l.ctx).Infof("change-password old-password mismatch userId=%d", userId)
|
|
|
- return response.ErrBadRequest("原密码错误")
|
|
|
-}
|
|
|
-
|
|
|
-if req.OldPassword == req.NewPassword {
|
|
|
- return response.ErrBadRequest("新密码不能与原密码相同")
|
|
|
-}
|
|
|
-```
|
|
|
-
|
|
|
-校验顺序:
|
|
|
-
|
|
|
-1. bcrypt 比对 old password(耗时 ~60ms,CPU 重);
|
|
|
-2. 字符串比较 old == new(纳秒级);
|
|
|
-3. bcrypt 生成新 hash(另一次 ~60ms)。
|
|
|
-
|
|
|
-把 (2) 提前到 (1) 之前:
|
|
|
-
|
|
|
-- 对合法请求无影响((2) 返 false 时进入 (1) 正常流程);
|
|
|
-- 对"同密码"攻击请求可以提前 return,少做一次 bcrypt compare(60ms CPU)。
|
|
|
-
|
|
|
-**修复方案**
|
|
|
-
|
|
|
-直接交换顺序,`req.OldPassword == req.NewPassword` 放到 bcrypt compare 之前:
|
|
|
-
|
|
|
-```go
|
|
|
-if req.OldPassword == req.NewPassword {
|
|
|
- return response.ErrBadRequest("新密码不能与原密码相同")
|
|
|
-}
|
|
|
-if err := bcrypt.CompareHashAndPassword([]byte(user.Password), []byte(req.OldPassword)); err != nil {
|
|
|
- // ...
|
|
|
-}
|
|
|
-```
|
|
|
-
|
|
|
-关注点:这会让"攻击者用正确用户名 + 猜对 oldPwd 等于 newPwd"的响应时间比"猜错 oldPwd"快约 60ms,理论上构成 timing 信号。但这条信号对攻击者几乎无用——要 trigger 这条分支攻击者需要**已经知道 oldPwd**,一旦攻击者已知 oldPwd,ChangePassword 本身就是 game over。所以 timing 差异可以接受。
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### L-R17-5 · `createDeptLogic.go` 的"Insert → FindOneWithTx → Update"三步写回 Path,可以合并为一次 `UPDATE … SET path = CONCAT(?, id, '/') WHERE id=?` 省一次 DB roundtrip
|
|
|
-
|
|
|
-**位置**
|
|
|
-
|
|
|
-- `internal/logic/dept/createDeptLogic.go:86-109`
|
|
|
-
|
|
|
-**描述**
|
|
|
-
|
|
|
-```86-109:internal/logic/dept/createDeptLogic.go
|
|
|
-result, err := l.svcCtx.SysDeptModel.InsertWithTx(ctx, session, &deptModel.SysDept{ /* Path: parentPath 是占位 */ })
|
|
|
-// ...
|
|
|
-deptId, _ = result.LastInsertId()
|
|
|
-d, err := l.svcCtx.SysDeptModel.FindOneWithTx(ctx, session, deptId)
|
|
|
-if err != nil {
|
|
|
- return err
|
|
|
-}
|
|
|
-d.Path = fmt.Sprintf("%s%d/", parentPath, deptId)
|
|
|
-return l.svcCtx.SysDeptModel.UpdateWithTx(ctx, session, d)
|
|
|
-```
|
|
|
-
|
|
|
-三步模式:Insert → 再 FindOne → 再 Update,`FindOneWithTx` 这一步纯粹为了拿到其余字段喂给 `UpdateWithTx`(`UpdateWithTx` 要整行对象)。其实 `InsertWithTx` 已返回 LastInsertId,parentPath 本身已知,可以直接一条 UPDATE 搞定,或者 `UpdateWithTx` 之外再暴露一个 `UpdatePathOnlyWithTx` 模式。
|
|
|
-
|
|
|
-**修复方案**
|
|
|
-
|
|
|
-建议在 `sysDeptModel` 加一个:
|
|
|
-
|
|
|
-```go
|
|
|
-func (m *customSysDeptModel) UpdatePathWithTx(ctx context.Context, session sqlx.Session, id int64, path string, updateTime int64) error {
|
|
|
- // UPDATE sys_dept SET `path`=?, `updateTime`=? WHERE id=?
|
|
|
- // 同时 DelCache(cacheSysDeptIdPrefix+id)
|
|
|
-}
|
|
|
-```
|
|
|
-
|
|
|
-CreateDept 改调 `UpdatePathWithTx(ctx, session, deptId, fmt.Sprintf("%s%d/", parentPath, deptId), now)`,省掉 FindOneWithTx 的一次往返;tx 持锁时间缩短,降低 sys_dept X 锁的尾部延迟。
|
|
|
-
|
|
|
----
|
|
|
-
|
|
|
-### L-R17-6 · `UpdateUserStatusLogic` 对"解冻(Disabled → Enabled)"同样执行 `tokenVersion+1` 与 `UserDetailsLoader.Clean`,注释只覆盖冻结场景
|
|
|
-
|
|
|
-**位置**
|
|
|
-
|
|
|
-- `internal/logic/user/updateUserStatusLogic.go:59-71`(`UpdateStatus` 内部 `tokenVersion+1`)
|
|
|
-- `internal/model/user/sysUserModel.go::UpdateStatus`
|
|
|
-
|
|
|
-**描述**
|
|
|
-
|
|
|
-`UpdateStatus` 的 SQL 写死 `SET tokenVersion = tokenVersion + 1`,无论 target.Status 是从 Disabled → Enabled 还是 Enabled → Disabled。对 Enabled→Disabled 侧的 audit 注释写得很清楚(冻结后的旧 token 必须立即失效);对 Disabled→Enabled 侧无注释,实际上也会 +1(虽然用户本来就因为 Disabled 无法登录,再 +1 等价于空动作,无害)。
|
|
|
-
|
|
|
-**修复方案**
|
|
|
-
|
|
|
-- 不改代码,仅补注释:`UpdateStatus` 无条件递增 tokenVersion 是刻意设计,让"冻结-解冻-冻结"这条路径里 Redis 残留缓存的 tokenVersion 比对始终相对于"最新值"计算,即使解冻间隙 DB 抖动,也不会让一个"冻结前未过期的 access token"在解冻后复活。这条语义在 `UpdateUserStatusLogic` 和 `sysUserModel.UpdateStatus` 的函数头注释里都要显式写出来,避免后续 R18 读代码时误以为"解冻无需 bump"做成条件判断反而引入回归。
|
|
|
-
|
|
|
----
|
|
|
+其中 **H-R18-1** 必须优先修复——它是 R17 对 `DeleteRole` 补失效的"对偶对称缺口",实现成本极低(只要在 `updateRoleLogic.go` post-commit 追加一次 `InvalidateRoleCache(old)`)。
|
|
|
|
|
|
-## 汇总
|
|
|
+**H-R18-2** 是业务与实现的语义偏差,修复方案需要产品 / 合规侧先定义清楚"冻结部门"的业务意图(强吊销 vs 会话软吊销),再决定 A(收紧 `loadPerms`)还是 B(澄清注释 + 调整审计字段)。
|
|
|
|
|
|
-- 本轮新发现 3 条 High(`H-R17-1`、`H-R17-2`、`H-R17-3`),均为**结构性**问题,无法通过单点补丁彻底解决,需要:
|
|
|
- - `H-R17-1` 把 `CreateUser`/`CreateProductLogic` 的 user 落库动作迁入事务并加 `sys_dept` S 锁(bcrypt 必须留在事务外);
|
|
|
- - `H-R17-2` 增加 post-commit 低层缓存失效兜底,覆盖 `sys_dept` / `sys_role` 的 `*WithTx` 路径;长期建议通过自定义事务钩子把 go-zero sqlc 的"exec → DelCache"模式从事务语义里摘出去;
|
|
|
- - `H-R17-3` 在 `CreateRoleLogic` 镜像 `UpdateRoleLogic` 的 L-R12-3 防护,并抽象 `GuardCreateRolePermsLevel`。
|
|
|
-- Medium 2 条、Low 6 条,以感知可见性 / 注释对齐 / 性能微调为主。
|
|
|
-- R16 及更早各轮修复经本轮复检全部稳定,不存在回归。
|
|
|
+其它 Medium / Low 项均可作为常态化重构批次推进,不会立刻造成事故。
|