|
|
@@ -2,288 +2,441 @@
|
|
|
|
|
|
> 审计范围:`/internal` 下全部非测试生产代码(logic、model、middleware、handler、loaders、server、svc、config、consts、response、util)及入口文件 `perm.go`、gRPC 客户端 `permclient/`。
|
|
|
> 审计时间:2026-04-18
|
|
|
+> 审计重点:逻辑一致性、并发竞态、数据完整性、水平越权、缓存一致性、僵尸代码、N+1、接口契约。
|
|
|
|
|
|
---
|
|
|
|
|
|
## 🚩 核心逻辑漏洞 (High Risk)
|
|
|
|
|
|
----
|
|
|
+### H-1. BindRoles 的 permsLevel 越级校验错误地封死了 ADMIN/DEVELOPER 成员
|
|
|
|
|
|
-### H-1. UpdateUser 存在 Read-Modify-Write 竞态,可静默撤销 tokenVersion 安全递增
|
|
|
+- **位置**:`internal/logic/user/bindRolesLogic.go` 第 60-82 行
|
|
|
+- **描述**:
|
|
|
+ ```go
|
|
|
+ caller := middleware.GetUserDetails(l.ctx)
|
|
|
+ ...
|
|
|
+ for _, r := range roles {
|
|
|
+ ...
|
|
|
+ if caller != nil && !caller.IsSuperAdmin {
|
|
|
+ if caller.MinPermsLevel == 0 || r.PermsLevel < caller.MinPermsLevel {
|
|
|
+ return response.ErrForbidden("不能分配权限级别高于自身的角色")
|
|
|
+ }
|
|
|
+ }
|
|
|
+ }
|
|
|
+ ```
|
|
|
|
|
|
-- **位置**:`internal/logic/user/updateUserLogic.go` 第 47-112 行
|
|
|
-- **描述**:`UpdateUser` 先通过 `FindOne` 读取完整用户行(包含 `tokenVersion`),在内存中修改字段后调用通用 `Update()` 回写**全部字段**。`Update()` 对应的 SQL 形如 `UPDATE sys_user SET ... tokenVersion=?, ... WHERE id=?`,会将内存中的 tokenVersion 值**覆盖**到数据库。
|
|
|
+ 这段 permsLevel 校验对**所有非超管调用者**生效,包括 `ADMIN` / `DEVELOPER` 成员。问题在于:
|
|
|
+ 1. 在 `userDetailsLoader.loadRoles` 中,`MinPermsLevel` **默认为 `math.MaxInt64`**(见 `internal/loaders/userDetailsLoader.go` 第 227 行),仅当用户存在启用角色时才会被覆盖。
|
|
|
+ 2. 产品自动创建的 `admin_{code}` 管理员、以及大部分 `ADMIN`/`DEVELOPER` 成员通过 `sys_product_member.memberType` 获得权限,**不关联任何 `sys_user_role` 角色**,因此他们的 `MinPermsLevel` 永远是 `math.MaxInt64`。
|
|
|
+ 3. 代码中的 sentinel 判断 `caller.MinPermsLevel == 0` 永远不会命中(MinPermsLevel 只会是 `math.MaxInt64` 或 `[1,999]`)。
|
|
|
+ 4. 因此 `r.PermsLevel (1-999) < math.MaxInt64` 必然为 `true`,**任何 permsLevel 的角色都会被判为"权限级别高于自身"而拒绝绑定**。
|
|
|
|
|
|
- 然而 `UpdatePassword` 和 `UpdateStatus` 使用的是原子 SQL:
|
|
|
- ```sql
|
|
|
- UPDATE sys_user SET tokenVersion = tokenVersion + 1 WHERE id = ?
|
|
|
- ```
|
|
|
- 如果在 `UpdateUser` 的 `FindOne` 和 `Update` 之间,另一个请求执行了 `UpdatePassword` 或 `UpdateStatus`,原子递增的 tokenVersion 会被 `UpdateUser` 的陈旧值**静默覆盖回去**。
|
|
|
+ 对比 `checkPermLevel`(access.go 第 143-151 行)的设计:只有在 `callerPri == targetPri`(都是 MEMBER)时才会进入 permsLevel 比较,而 ADMIN/DEVELOPER 因 MemberType 优先级更高就已经放行。`bindRolesLogic` 的这段校验缺失了这一前置判定。
|
|
|
|
|
|
- **影响**:
|
|
|
- - 场景复现:用户 A 修改密码(tokenVersion 5→6,所有旧 session 应失效)。几乎同时,管理员修改该用户昵称(读到 tokenVersion=5,写回 tokenVersion=5)。结果数据库 tokenVersion 被还原为 5,用户旧 session(tokenVersion=5)仍然有效。
|
|
|
- - **密码修改后的安全失效机制被绕过**,攻击者持有旧 token 仍可继续访问系统。
|
|
|
+ - 在真实产线,`ADMIN` 成员(包括系统自动创建的 `admin_{code}`)**无法给任何用户绑定任何角色**,管理员最核心的运营能力被封死。
|
|
|
+ - `DEVELOPER` 成员同样被封死。
|
|
|
+ - 该 bug 在测试中未暴露,是因为 `bindRolesLogic_test.go` 第 314 行人为构造了 `MinPermsLevel: 50` 的 ADMIN 上下文(见 TC-0208),并不反映 loader 真实产出的 `math.MaxInt64`。
|
|
|
|
|
|
-- **修复方案**:`UpdateUser` 应改为**部分字段更新**,不触碰 `tokenVersion`、`password` 等安全敏感字段。如果需要修改 status 并递增 tokenVersion,应使用与 `UpdateStatus` 相同的原子 SQL,或在事务中使用 `SELECT ... FOR UPDATE` 锁定行。
|
|
|
+- **修复方案**:与 `checkPermLevel` 对齐,只对同为 `MEMBER` 类型的调用者做 permsLevel 比较;对 ADMIN/DEVELOPER 直接放行:
|
|
|
|
|
|
```go
|
|
|
- // 方案一:拆分为部分更新 SQL
|
|
|
- func (m *customSysUserModel) UpdateProfile(ctx context.Context, id int64, fields map[string]interface{}) error {
|
|
|
- // 动态构建 SET 子句,仅更新传入字段,不覆盖 tokenVersion/password
|
|
|
+ if caller != nil && !caller.IsSuperAdmin &&
|
|
|
+ caller.MemberType != consts.MemberTypeAdmin &&
|
|
|
+ caller.MemberType != consts.MemberTypeDeveloper {
|
|
|
+ // 只有 MEMBER 类型调用者才需要 permsLevel 越级校验
|
|
|
+ if caller.MinPermsLevel < math.MaxInt64 && r.PermsLevel < caller.MinPermsLevel {
|
|
|
+ return response.ErrForbidden("不能分配权限级别高于自身的角色")
|
|
|
+ }
|
|
|
}
|
|
|
+ ```
|
|
|
|
|
|
- // 方案二:如果更新 status 需要递增 tokenVersion,单独调用 UpdateStatus
|
|
|
- if req.Status != 0 && user.Status != req.Status {
|
|
|
- if err := l.svcCtx.SysUserModel.UpdateStatus(l.ctx, req.Id, req.Status); err != nil {
|
|
|
- return err
|
|
|
+ 同时,`caller.MinPermsLevel == 0` 这段无效判断应改为 `caller.MinPermsLevel == math.MaxInt64`(或按上面的写法从条件里移除)。
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### H-2. gRPC `GetUserPerms` 未校验用户状态,冻结用户仍被下发全量权限
|
|
|
+
|
|
|
+- **位置**:`internal/server/permserver.go` 第 191-216 行
|
|
|
+- **描述**:
|
|
|
+ ```go
|
|
|
+ func (s *PermServer) GetUserPerms(ctx context.Context, req *pb.GetUserPermsReq) (*pb.GetUserPermsResp, error) {
|
|
|
+ // 产品签名校验 ...
|
|
|
+ ud := s.svcCtx.UserDetailsLoader.Load(ctx, req.UserId, req.ProductCode)
|
|
|
+ if ud.Username == "" {
|
|
|
+ return nil, status.Error(codes.NotFound, "用户不存在")
|
|
|
}
|
|
|
+ return &pb.GetUserPermsResp{
|
|
|
+ MemberType: ud.MemberType,
|
|
|
+ Perms: ud.Perms,
|
|
|
+ }, nil
|
|
|
}
|
|
|
```
|
|
|
|
|
|
----
|
|
|
+ 对比同一文件的 `VerifyToken`(第 157-189 行),VerifyToken 在返回前校验了 `ud.Status == StatusEnabled` 和 `MemberType != ""`。但 `GetUserPerms` 完全没有这两层过滤,仅判断了用户是否存在。
|
|
|
|
|
|
-### H-2. AdminLogin 缺少按用户名维度的频率限制,暴力破解风险高于产品端
|
|
|
+ 结果:当管理后台调用 `UpdateUserStatus` 将用户冻结后:
|
|
|
+ - `sys_user.status = 2 (Disabled)`,`tokenVersion` +1
|
|
|
+ - `userDetailsLoader.Clean` 清理缓存
|
|
|
+ - 下一次 Load 从 DB 读出 `ud.Status = 2`,但 `ud.Perms` 依然根据 `loadPerms` 逻辑完整计算
|
|
|
+ - 产品服务器通过 gRPC `GetUserPerms` 查询,**仍然会拿到完整的权限列表**
|
|
|
|
|
|
-- **位置**:`internal/logic/pub/adminLoginLogic.go`、`internal/handler/routes.go` 第 174-188 行
|
|
|
-- **描述**:产品端登录 `ValidateProductLogin` 在 `loginService.go` 中使用了 `UsernameLoginLimit`(每用户名 300 秒 10 次),但管理后台登录 `AdminLogin` 只经过 IP 维度的 `LoginRateLimit`(每 IP 60 秒 20 次),没有按用户名限频。
|
|
|
-- **影响**:攻击者通过代理池轮换 IP,可对超级管理员账号进行无限次暴力破解,而 IP 维度的限流完全无法防御此场景。考虑到管理后台登录还需要提供 `managementKey`,实际风险有所降低,但密钥和密码的双重暴力破解仍然可行。
|
|
|
-- **修复方案**:在 `AdminLogin` 中复用或新增按用户名维度的频率限制器:
|
|
|
+- **影响**:
|
|
|
+ - 被冻结的用户在接入方(产品服务)一侧仍然具备全部权限。虽然浏览器侧因 tokenVersion 变化 access token 已失效,但如果接入方自己缓存了 userId → perms 的映射并据此鉴权,用户可以继续获得访问。
|
|
|
+ - 同样地,`sys_product_member.status = Disabled` 的用户,如果其部门类型为 `DEV`,`loadPerms` 依然会返回全量权限(详见 H-3)。
|
|
|
+ - 形成"本系统侧已冻结,但对外仍判定有权限"的一致性漏洞。
|
|
|
+
|
|
|
+- **修复方案**:对齐 `VerifyToken` 的校验逻辑:
|
|
|
|
|
|
```go
|
|
|
- func (l *AdminLoginLogic) AdminLogin(req *types.AdminLoginReq) (*types.LoginResp, error) {
|
|
|
- if l.svcCtx.UsernameLoginLimit != nil {
|
|
|
- code, _ := l.svcCtx.UsernameLoginLimit.Take(req.Username)
|
|
|
- if code == limit.OverQuota {
|
|
|
- return nil, response.ErrTooManyRequests("该账号登录尝试过于频繁,请5分钟后再试")
|
|
|
- }
|
|
|
- }
|
|
|
- // ... 原有逻辑
|
|
|
+ ud := s.svcCtx.UserDetailsLoader.Load(ctx, req.UserId, req.ProductCode)
|
|
|
+ if ud.Username == "" {
|
|
|
+ return nil, status.Error(codes.NotFound, "用户不存在")
|
|
|
+ }
|
|
|
+ if ud.Status != consts.StatusEnabled {
|
|
|
+ return nil, status.Error(codes.PermissionDenied, "用户已被冻结")
|
|
|
+ }
|
|
|
+ if !ud.IsSuperAdmin && ud.MemberType == "" {
|
|
|
+ // 产品成员已被禁用或移除
|
|
|
+ return nil, status.Error(codes.PermissionDenied, "用户已不是该产品的成员")
|
|
|
}
|
|
|
```
|
|
|
|
|
|
---
|
|
|
|
|
|
-### H-3. RefreshToken 与 Login 共享同一速率限制桶,正常刷新可导致登录被锁
|
|
|
+### H-3. DEV 部门用户可绕过"产品成员禁用"继续获得全量权限
|
|
|
|
|
|
-- **位置**:`internal/handler/routes.go` 第 174-201 行
|
|
|
-- **描述**:`refreshToken` 路由与 `login`、`adminLogin` 共用 `LoginRateLimit` 中间件(基于 IP,60 秒 20 次)。RefreshToken 是 access token 过期后的常规续签操作,调用频率远高于登录。
|
|
|
+- **位置**:`internal/loaders/userDetailsLoader.go` 第 347-363 行(`loadPerms`)、`internal/middleware/jwtauthMiddleware.go` 第 79-86 行
|
|
|
+- **描述**:`loadPerms` 在判断"自动获得全量权限"时使用的是 **OR** 逻辑,其中部门类型判定独立于产品成员状态:
|
|
|
|
|
|
-- **影响**:
|
|
|
- - 如果前端在 access token 过期时自动调用 refreshToken,一个 IP 下有多个用户时(如办公网络 NAT 出口),刷新请求很快耗尽配额,导致同 IP 下所有用户无法登录。
|
|
|
- - 反过来,攻击者可以通过大量发送 refreshToken 请求来消耗某个 IP 的登录配额,造成该 IP 下的所有用户无法登录(拒绝服务)。
|
|
|
+ ```go
|
|
|
+ if ud.IsSuperAdmin ||
|
|
|
+ ud.MemberType == consts.MemberTypeAdmin ||
|
|
|
+ ud.MemberType == consts.MemberTypeDeveloper ||
|
|
|
+ (ud.DeptType == consts.DeptTypeDev && ud.DeptStatus == consts.StatusEnabled) {
|
|
|
+ codes, err := l.models.SysPermModel.FindAllCodesByProductCode(ctx, ud.ProductCode)
|
|
|
+ ud.Perms = codes
|
|
|
+ return
|
|
|
+ }
|
|
|
+ ```
|
|
|
+
|
|
|
+ 而 `loadMembership` 针对被禁用的成员只做了一件事:跳过 `ud.MemberType` 赋值(即 `MemberType = ""`),**并不回退/阻断后续的部门判定**。
|
|
|
+
|
|
|
+ 同时,`jwtauthMiddleware.Handle` 仅校验了 `ud.Username`、`ud.Status`、`claims.TokenVersion`,没有校验产品成员是否被禁用(不像 `RefreshToken` 会阻断 `ud.MemberType == ""` 的场景)。
|
|
|
+
|
|
|
+ 复现链路:
|
|
|
+ 1. 用户 U 属于研发部门(`deptType=DEV`,`deptStatus=Enabled`),在产品 P 作为 `MEMBER` 登录获得 accessToken
|
|
|
+ 2. 管理员通过 `UpdateMember` 将 U 在 P 的成员资格 `Status` 改为 `Disabled`
|
|
|
+ 3. 缓存被 `Del`,下次请求 loader 从 DB 重算
|
|
|
+ 4. `loadMembership` 因 `member.Status != Enabled` 直接 `return`,`MemberType = ""`
|
|
|
+ 5. `loadPerms` 匹配第 4 个条件 `DeptType == DEV && DeptStatus == Enabled`,**依然返回全量权限**
|
|
|
+ 6. 中间件不校验 MemberType,放行请求 → 用户携带冻结后的 membership 继续访问
|
|
|
|
|
|
-- **修复方案**:为 `refreshToken` 使用独立的速率限制器,或直接移除其 IP 限流(因为 refreshToken 自身已有 JWT 签名验证和 tokenVersion 校验保护):
|
|
|
+- **影响**:产品管理员通过"禁用产品成员"无法真正撤销 DEV 部门用户对该产品的访问;必须同时修改用户部门,才能剥夺其权限。与接口语义不一致,是真实的水平越权旁路。
|
|
|
+
|
|
|
+- **修复方案**:`loadPerms` 的 DEV 部门短路应**叠加一个"用户是该产品成员且成员状态启用"的前置条件**;或在 loader 中记录 `memberStatus` 字段,并在 DEV 部门判定里做交集:
|
|
|
|
|
|
```go
|
|
|
- // servicecontext.go 中新增
|
|
|
- refreshRlMiddleware := middleware.NewRateLimitMiddleware(
|
|
|
- rds, 60, 60, c.CacheRedis.KeyPrefix+":rl:refresh", c.BehindProxy,
|
|
|
- )
|
|
|
+ // 方案:DEV 部门自动权限需要"产品成员存在且启用"作为前置
|
|
|
+ deptFullPerms := ud.DeptType == consts.DeptTypeDev &&
|
|
|
+ ud.DeptStatus == consts.StatusEnabled &&
|
|
|
+ ud.MemberType != "" // 等价于"成员存在且启用",因为禁用成员会把 MemberType 清空
|
|
|
+
|
|
|
+ if ud.IsSuperAdmin ||
|
|
|
+ ud.MemberType == consts.MemberTypeAdmin ||
|
|
|
+ ud.MemberType == consts.MemberTypeDeveloper ||
|
|
|
+ deptFullPerms {
|
|
|
+ ...
|
|
|
+ }
|
|
|
```
|
|
|
|
|
|
+ 同时建议在 `jwtauthMiddleware` 或在业务层对 `ud.MemberType == "" && !IsSuperAdmin && productCode != ""` 场景也返回 401/403(与 `refreshTokenLogic` 的第 53 行保持一致)。
|
|
|
+
|
|
|
---
|
|
|
|
|
|
-### H-4. UpdateProduct 状态校验静默忽略无效值
|
|
|
+## ⚠️ 健壮性与性能建议 (Medium)
|
|
|
|
|
|
-- **位置**:`internal/logic/product/updateProductLogic.go` 第 49-51 行
|
|
|
-- **描述**:
|
|
|
+### M-1. 自动生成的产品管理员密码熵过低(仅 32 bits)
|
|
|
+
|
|
|
+- **位置**:`internal/logic/product/createProductLogic.go` 第 80、157-163 行
|
|
|
+- **描述**:`adminPassword` 通过 `generateRandomHex(8)` 生成,而该函数的实现是先 `rand.Read(b)` 读取 8 字节(16 hex 字符),再 `[:length]` 截断取前 8 个字符。因此 adminPassword 实际有效熵只有 **4 字节 = 32 bits**(约 42 亿种可能)。同理 `generateRandomHex(32)` 的 appKey 是 16 字节 = 128 bits(OK),`generateRandomHex(64)` 的 appSecret 是 32 字节 = 256 bits(OK)。
|
|
|
+
|
|
|
+ 虽然 `mustChangePassword=Yes` 会强制首次登录改密,但在管理员拿到 `CreateProductResp` 到其首次登录的时间窗口内,该密码仍可被暴力破解(尤其管理后台目前已经按用户名限流 5min/10 次,基本可挡住穷举,但设计上不应依赖外部限流来保护一个 32 bit 的秘密)。
|
|
|
+
|
|
|
+- **建议**:
|
|
|
```go
|
|
|
- if req.Status == consts.StatusEnabled || req.Status == consts.StatusDisabled {
|
|
|
- product.Status = req.Status
|
|
|
+ func generateRandomHex(length int) (string, error) {
|
|
|
+ byteLen := (length + 1) / 2
|
|
|
+ b := make([]byte, byteLen)
|
|
|
+ if _, err := rand.Read(b); err != nil {
|
|
|
+ return "", err
|
|
|
+ }
|
|
|
+ return hex.EncodeToString(b)[:length], nil
|
|
|
}
|
|
|
```
|
|
|
- 当传入 `status=3` 或 `status=-1` 等无效值时,代码**静默忽略**,不更新也不报错。而 `updateRoleLogic`、`updateUserLogic`、`updateDeptLogic`、`updateMemberLogic` 对相同场景都会返回 400 错误。
|
|
|
+ 这样 `generateRandomHex(8)` 就真正提供 8 个 hex 字符 = 4 字节实熵,再考虑到一次性临时密码,可以直接把 adminPassword 生成改为 `generateRandomHex(16)`(16 hex 字符 ≈ 64 bits)。
|
|
|
|
|
|
-- **影响**:接口行为不一致。调用方传入非法状态值后收到成功响应,误以为状态已修改,实际上并未生效。在前端管理界面可能导致状态显示与实际不符。
|
|
|
+---
|
|
|
|
|
|
-- **修复方案**:与其他更新接口保持一致,显式校验并报错:
|
|
|
+### M-2. BindRoles 与 BindRolePerms 的 DELETE 走循环逐条执行
|
|
|
|
|
|
+- **位置**:
|
|
|
+ - `internal/logic/user/bindRolesLogic.go` 第 117-122 行
|
|
|
+ - `internal/logic/role/bindRolePermsLogic.go` 第 105-110 行
|
|
|
+- **描述**:
|
|
|
+ ```go
|
|
|
+ for _, roleId := range toRemove {
|
|
|
+ query := fmt.Sprintf("DELETE FROM %s WHERE `userId` = ? AND `roleId` = ?", l.svcCtx.SysUserRoleModel.TableName())
|
|
|
+ if _, err := session.ExecCtx(ctx, query, req.UserId, roleId); err != nil {
|
|
|
+ return err
|
|
|
+ }
|
|
|
+ }
|
|
|
+ ```
|
|
|
+ 当 `toRemove` 有 N 项时,会在同一事务内执行 N 次独立 DELETE,每次都要经过 session 往返。虽然单用户的角色数一般不多(< 20),但这个实现在事务持锁窗口上是 N 倍于批量 DELETE 的。
|
|
|
+- **建议**:合并为一次 `DELETE ... WHERE userId=? AND roleId IN (?,?,?)`:
|
|
|
```go
|
|
|
- if req.Status != 0 {
|
|
|
- if req.Status != consts.StatusEnabled && req.Status != consts.StatusDisabled {
|
|
|
- return response.ErrBadRequest("状态值无效,仅支持 1(启用) 和 2(禁用)")
|
|
|
+ if len(toRemove) > 0 {
|
|
|
+ placeholders := strings.Repeat("?,", len(toRemove))
|
|
|
+ placeholders = placeholders[:len(placeholders)-1]
|
|
|
+ args := make([]interface{}, 0, len(toRemove)+1)
|
|
|
+ args = append(args, req.UserId)
|
|
|
+ for _, id := range toRemove {
|
|
|
+ args = append(args, id)
|
|
|
+ }
|
|
|
+ query := fmt.Sprintf("DELETE FROM %s WHERE `userId`=? AND `roleId` IN (%s)",
|
|
|
+ l.svcCtx.SysUserRoleModel.TableName(), placeholders)
|
|
|
+ if _, err := session.ExecCtx(ctx, query, args...); err != nil {
|
|
|
+ return err
|
|
|
}
|
|
|
- product.Status = req.Status
|
|
|
}
|
|
|
```
|
|
|
|
|
|
---
|
|
|
|
|
|
-## ⚠️ 健壮性与性能建议 (Medium/Low)
|
|
|
+### M-3. UserDetailLogic 在"超管 + 产品上下文"场景下返回跨产品的 roleIds
|
|
|
+
|
|
|
+- **位置**:`internal/logic/user/userDetailLogic.go` 第 50-56 行
|
|
|
+- **描述**:
|
|
|
+ ```go
|
|
|
+ productCode := middleware.GetProductCode(l.ctx)
|
|
|
+ var roleIds []int64
|
|
|
+ if productCode != "" && !caller.IsSuperAdmin {
|
|
|
+ roleIds, _ = l.svcCtx.SysUserRoleModel.FindRoleIdsByUserIdForProduct(...)
|
|
|
+ } else {
|
|
|
+ roleIds, _ = l.svcCtx.SysUserRoleModel.FindRoleIdsByUserId(...)
|
|
|
+ }
|
|
|
+ ```
|
|
|
+ 条件只看"非超管 + 有 productCode"。当超管自己带着某产品的 productCode 查看某用户时,会走 `else` 分支,返回用户**在所有产品下**的角色 ID 列表。前端如果基于这个列表渲染"当前产品的已绑定角色",展示会错乱(例如超管在产品 A 下看用户 U 的详情,却看到 U 在产品 B 的角色)。
|
|
|
+- **建议**:超管的判定与产品过滤解耦——**只要有 productCode,就按产品过滤**:
|
|
|
+ ```go
|
|
|
+ if productCode != "" {
|
|
|
+ roleIds, err = l.svcCtx.SysUserRoleModel.FindRoleIdsByUserIdForProduct(l.ctx, user.Id, productCode)
|
|
|
+ } else {
|
|
|
+ roleIds, err = l.svcCtx.SysUserRoleModel.FindRoleIdsByUserId(l.ctx, user.Id)
|
|
|
+ }
|
|
|
+ ```
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M-1. UpdateUserStatus 对同一用户产生 3 次数据库读取
|
|
|
+### M-4. `FindRoleIdsByUserIdForProduct` 未过滤角色状态,返回含已禁用角色
|
|
|
|
|
|
-- **位置**:`internal/logic/user/updateUserStatusLogic.go`
|
|
|
-- **描述**:请求处理链路中对同一 userId 执行了 3 次 `FindOne`:
|
|
|
- 1. 第 40 行 `SysUserModel.FindOne` —— 检查用户是否存在、是否超管
|
|
|
- 2. 第 56 行 `CheckManageAccess` → `checkDeptHierarchy` 内部又调用 `SysUserModel.FindOne`(第 115 行 `access.go`)
|
|
|
- 3. 第 62 行 `SysUserModel.UpdateStatus` 内部再次调用 `FindOne`(用于构建缓存 key)
|
|
|
+- **位置**:`internal/model/userrole/sysUserRoleModel.go` 第 43-50 行
|
|
|
+- **描述**:
|
|
|
+ ```sql
|
|
|
+ SELECT ur.roleId FROM sys_user_role ur
|
|
|
+ INNER JOIN sys_role r ON ur.roleId = r.id
|
|
|
+ WHERE ur.userId = ? AND r.productCode = ?
|
|
|
+ ```
|
|
|
+ 只按 `productCode` 过滤,没有 `AND r.status = 1`。而 `loadRoles` / `FindMinPermsLevelByUserIdAndProductCode` 都需要基于"启用角色"做判定。
|
|
|
|
|
|
- 虽然 go-zero 的 model 层有 cache,第 2-3 次读取会命中缓存,但仍有序列化/反序列化开销和额外的网络往返(如果 Redis 部署在远端)。
|
|
|
+ 后续在 `userDetailsLoader.loadRoles`(第 314-344 行)通过内存过滤 `r.Status == StatusEnabled` 做了二次兜底,因此用于**权限计算**的路径是正确的。但:
|
|
|
+ 1. `UserDetail` 接口直接把这批 roleIds 返回给前端(含禁用角色)。
|
|
|
+ 2. `bindRolesLogic` 的 "existingRoleIds diff 逻辑"(第 85-110 行)会把已禁用的旧关联当作"存在",只有当请求里明确包含了该禁用角色时才会保留,否则会被 `toRemove` 删除 —— 表现为"重新绑定时,用户原本禁用的旧角色会被清掉"。这个行为从业务语义上是可接受的(禁用角色本就不应再绑定),但与 SQL 字面不一致,容易误解。
|
|
|
|
|
|
-- **建议**:将首次查询的结果传递到后续函数中复用,或让 `UpdateStatus` 接收 username 参数以跳过内部 FindOne。
|
|
|
+- **建议**:要么在 SQL 里显式 `AND r.status = 1`,要么在命名上明确包含禁用(如 `FindAllRoleIdsByUserIdForProduct`)。推荐前者:
|
|
|
+
|
|
|
+ ```sql
|
|
|
+ WHERE ur.userId = ? AND r.productCode = ? AND r.status = 1
|
|
|
+ ```
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M-2. gRPC 客户端使用已弃用的 `grpc.WithInsecure()`
|
|
|
+### M-5. UpdateDept 对"deptType 字段无变化"时也会级联清空子部门用户缓存
|
|
|
|
|
|
-- **位置**:`permclient/permclient.go` 第 17 行
|
|
|
-- **描述**:`grpc.WithInsecure()` 自 gRPC v1.53 起已被弃用,推荐使用 `grpc.WithTransportCredentials(insecure.NewCredentials())`。
|
|
|
-- **建议**:
|
|
|
+- **位置**:`internal/logic/dept/updateDeptLogic.go` 第 77-89 行
|
|
|
+- **描述**:条件是 `if req.DeptType == DeptTypeNormal || req.DeptType == DeptTypeDev`,只要请求带了合法的 deptType,就会执行 `FindByPathPrefix` 然后挨个 `Clean` 子部门用户缓存;但并没有比较 `req.DeptType` 与当前 `dept.DeptType` 是否真的不同。
|
|
|
+
|
|
|
+ 另一层问题:如源代码注释所说,`loadPerms` 只看用户**自身**部门的 deptType/status,子部门用户并不受父部门 deptType 变化影响。所以从权限计算的正确性来看,**根本不需要**级联清理子部门用户缓存;只清理当前部门直属用户已经足够。
|
|
|
+
|
|
|
+- **建议**:去掉级联清理,或收窄到"deptType 真的发生变化时仅清理自身部门用户"。代码可简化为:
|
|
|
|
|
|
```go
|
|
|
- import "google.golang.org/grpc/credentials/insecure"
|
|
|
+ userIds, _ := l.svcCtx.SysUserModel.FindIdsByDeptId(l.ctx, req.Id)
|
|
|
+ for _, uid := range userIds {
|
|
|
+ l.svcCtx.UserDetailsLoader.Clean(l.ctx, uid)
|
|
|
+ }
|
|
|
+ ```
|
|
|
|
|
|
- conn, err := grpc.NewClient(target, grpc.WithTransportCredentials(insecure.NewCredentials()))
|
|
|
+ 子部门级联逻辑可以删除。
|
|
|
+
|
|
|
+---
|
|
|
+
|
|
|
+### M-6. 僵尸字段:JWT `Claims` 中的 `Perms` 从未被产线代码写入
|
|
|
+
|
|
|
+- **位置**:`internal/middleware/jwtauthMiddleware.go` 第 23-32 行
|
|
|
+- **描述**:`middleware.Claims` 结构体保留了 `Perms []string` 字段:
|
|
|
+ ```go
|
|
|
+ type Claims struct {
|
|
|
+ ...
|
|
|
+ Perms []string `json:"perms,omitempty"`
|
|
|
+ jwt.RegisteredClaims
|
|
|
+ }
|
|
|
```
|
|
|
+ 但 `GenerateAccessToken`(`internal/logic/auth/jwt.go` 第 23-39 行)**没有给 `Perms` 赋值**,产线签出的 JWT 永远不带该字段。仅测试 (`jwt_test.go`)、README 出现过引用。属于设计未落地或已重构遗留的字段。
|
|
|
+- **建议**:确认是否还需要(例如未来把 perms 塞进 token 做无状态鉴权);不再需要则移除 `Perms` 字段,避免维护歧义。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M-3. 配置文件包含明文敏感信息并提交到仓库
|
|
|
+### M-7. `X-Real-IP` 信任策略过于简单,且不支持标准 `X-Forwarded-For`
|
|
|
|
|
|
-- **位置**:`etc/perm-api-dev.yaml`、`etc/perm-api-prod.yaml`、`etc/perm-api-test.yaml`、`etc/perm-api-xiaom.yaml`
|
|
|
-- **描述**:数据库密码、Redis 密码、JWT 签名密钥、ManagementKey 等敏感信息以明文形式存储在 YAML 配置文件中,且已提交到 Git 仓库。
|
|
|
-- **影响**:如果仓库泄露或被分享,所有凭据将暴露。即使后续更改密码,Git 历史中仍保留旧值。
|
|
|
-- **建议**:使用环境变量注入或密钥管理服务(如 Vault)管理敏感配置。至少在 `.gitignore` 中排除含真实密钥的配置文件,仅保留模板文件。
|
|
|
+- **位置**:`internal/middleware/ratelimitMiddleware.go` 第 41-52 行
|
|
|
+- **描述**:
|
|
|
+ ```go
|
|
|
+ if behindProxy {
|
|
|
+ if ip := r.Header.Get("X-Real-IP"); ip != "" {
|
|
|
+ return ip
|
|
|
+ }
|
|
|
+ }
|
|
|
+ ```
|
|
|
+ 问题:
|
|
|
+ 1. 仅支持 `X-Real-IP`,未兼容更通用的 `X-Forwarded-For`(多数 K8s Ingress、ELB 默认设置的是 XFF)。
|
|
|
+ 2. 只要 `behindProxy=true`,任何 `X-Real-IP` 都无条件信任;如果反向代理没有正确覆盖客户端传入的头,攻击者可以伪造 IP 规避限流。
|
|
|
+ 3. XFF 是多段逗号分隔,需要按最右可信节点反向取值;本实现一旦支持 XFF 必须小心这一点。
|
|
|
+- **建议**:
|
|
|
+ - 同时兼容 `X-Forwarded-For`,取其中未被你控制的最右侧一段作为客户端 IP。
|
|
|
+ - 把"可信代理 CIDR 列表"配置化:只有来自可信代理网段的请求才信任 header,其他直接使用 `RemoteAddr`。
|
|
|
+ - 如果暂时不打算加复杂度,至少把 XFF 支持补上,且确保部署文档强调反向代理必须覆盖这些 header。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M-4. SyncPerms 接口未限制权限数组大小
|
|
|
+### M-8. DeptTree 对所有登录用户开放,暴露完整组织架构
|
|
|
+
|
|
|
+- **位置**:`internal/logic/dept/deptTreeLogic.go`、`internal/handler/routes.go` 第 42-69 行
|
|
|
+- **描述**:路由仅通过 `JwtAuth` 中间件保护,`DeptTreeLogic` 自身完全不做权限过滤,**任何通过 JWT 的用户(包括产品端 MEMBER)**都能拉到全公司组织架构。
|
|
|
+- **影响**:组织架构通常包含内部部门命名、层级关系、部门类型(NORMAL/DEV 可暗示岗位属性),属于内部敏感信息,不应暴露给产品端普通成员。
|
|
|
|
|
|
-- **位置**:`internal/logic/pub/syncPermsService.go` 第 42 行
|
|
|
-- **描述**:`ExecuteSyncPerms` 对传入的 `perms` 数组长度没有上限检查。产品客户端可以一次性发送极大的权限列表,导致:
|
|
|
- - 数据库事务中执行大量 INSERT/UPDATE
|
|
|
- - 内存中构建大量对象
|
|
|
- - 事务持锁时间过长
|
|
|
-- **建议**:增加上限检查,如 `if len(perms) > 5000 { return error }`。
|
|
|
+- **建议**:根据业务定位决定:
|
|
|
+ - 严格方案:仅超管可拉全量树;普通成员只能拿自身部门及下级子部门(`strings.HasPrefix(d.Path, caller.DeptPath)`)。
|
|
|
+ - 宽松方案:超管和任意 ADMIN/DEVELOPER 可见;MEMBER 只见自身部门节点。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M-5. 僵尸代码:`GetUserPerms` 函数及其参数
|
|
|
+### M-9. ProductList 对所有登录用户返回全量产品列表
|
|
|
|
|
|
-- **位置**:`internal/logic/auth/perms.go`
|
|
|
-- **描述**:`GetUserPerms` 函数接收 `deptId` 和 `isSuperAdmin` 两个参数,但函数体内**完全未使用**这两个参数,仅转发给 `UserDetailsLoader.Load()`。且该函数在所有生产代码中**从未被调用**,仅在测试文件中使用。
|
|
|
-- **建议**:如果此函数预留用于未来 gRPC 接口,应更新签名移除未使用参数;否则应删除。
|
|
|
+- **位置**:`internal/logic/product/productListLogic.go`
|
|
|
+- **描述**:任何 JWT 通过的用户都能列出系统全部产品(产品编码、名称、状态、创建时间),非超管会隐藏 `AppKey`,但产品列表本身仍然对全员可见。其他产品的普通成员也能"看见"存在哪些其他产品。
|
|
|
+- **影响**:产品信息属于租户元数据,跨产品可见会让攻击者进行产品横向探测(例如发现有"内部管理后台"、"支付中心"等高价值目标并针对性登录)。
|
|
|
+- **建议**:
|
|
|
+ - 仅超管可列全部;其他成员只能看到自己是成员的产品(通过 `FindListByUserId` 过滤)。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M-6. 僵尸代码:多个 Middleware Context Helper 函数
|
|
|
+### M-10. 缓存失效与 DB 事务不原子:Clean 失败静默吞错
|
|
|
|
|
|
-- **位置**:`internal/middleware/jwtauthMiddleware.go` 第 110-136 行
|
|
|
-- **描述**:以下函数在生产代码中**从未被调用**(仅在 testutil 中使用):
|
|
|
- - `GetUsername(ctx)` —— 生产代码中直接使用 `GetUserDetails(ctx).Username`
|
|
|
- - `GetMemberType(ctx)` —— 同上
|
|
|
- - `IsSuperAdmin(ctx)` —— 生产代码中直接用 `caller.IsSuperAdmin`
|
|
|
-- **建议**:如果这些函数不打算作为公开 API 暴露给外部包使用,应考虑移除以减少维护负担。
|
|
|
+- **位置**:`UpdateUser` (`updateUserLogic.go:132`)、`UpdateUserStatus` (`updateUserStatusLogic.go:66`)、`ChangePassword` (`changePasswordLogic.go:59`)、`BindRoles` (`bindRolesLogic.go:141`)、`SetUserPerms` (`setUserPermsLogic.go:115`)、`UpdateMember` (`updateMemberLogic.go:62`)、`RemoveMember` (`removeMemberLogic.go:51`)、`DeleteRole` (`deleteRoleLogic.go:53`)、`SyncPerms` (`syncPermsService.go:115`)
|
|
|
+- **描述**:所有写操作都按"先 DB 事务提交,再调用 `UserDetailsLoader.Clean/Del/BatchDel`"的顺序执行;而 `Clean/Del/BatchDel` 内部的 Redis 错误只写日志,不返回。这意味着:
|
|
|
+ - Redis 瞬时不可用或网络抖动时,DB 已经提交但缓存未失效。
|
|
|
+ - 在 `defaultCacheTTL = 300s`(5 分钟)之内,其他请求命中旧缓存,包括 `tokenVersion` / `MemberType` / `Perms` 等关键字段。
|
|
|
+ - 对于密码修改、冻结账号这类安全动作,最大 5 分钟的旧 token 继续可用就是一段不可忽视的风险窗口。
|
|
|
+- **建议**:
|
|
|
+ - 对"安全关键操作"(密码修改、冻结、tokenVersion 变化)收紧:在 `Clean` 失败时返回 5xx,或降低这类 key 的 TTL(如 30s)。
|
|
|
+ - 引入"延迟双删"或消息队列补偿:DB 提交后投递一条缓存失效消息,由消费端重试直至成功。
|
|
|
+ - 至少在 loader 层增加"失败时重试一次或返回错误"的能力,让上层可以决定是否对外暴露失败。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M-7. 僵尸代码:多个 Model 层函数仅被测试代码调用
|
|
|
+### M-11. DeleteDept 的前置校验与实际删除之间存在 TOCTOU 竞态
|
|
|
|
|
|
-- **位置**:各 model 包的自定义 Model 文件
|
|
|
-- **描述**:以下接口方法在生产代码中从未被调用,仅存在于测试或 mock 中:
|
|
|
+- **位置**:`internal/logic/dept/deleteDeptLogic.go`
|
|
|
+- **描述**:流程依次执行:
|
|
|
+ 1. `FindByParentId(id)` 校验无子部门
|
|
|
+ 2. `FindIdsByDeptId(id)` 校验无关联用户
|
|
|
+ 3. `Delete(id)`
|
|
|
|
|
|
- | 函数 | 所在包 | 说明 |
|
|
|
- |------|--------|------|
|
|
|
- | `DeleteByUserId` | userrole, userperm | 生产代码全部使用带 `ForProduct` 后缀的变体 |
|
|
|
- | `DeleteByUserIdForProduct`(非 Tx 版本) | userrole, userperm | 生产代码全部使用 `Tx` 事务版本 |
|
|
|
- | `DeleteByRoleId`(非 Tx 版本) | roleperm | 生产代码使用 `DeleteByRoleIdTx` |
|
|
|
- | `DisableNotInCodes`(非 Tx 版本) | perm | 生产代码使用 `DisableNotInCodesWithTx` |
|
|
|
- | `FindAllByProductCode` | perm | 生产代码使用 `FindAllCodesByProductCode` |
|
|
|
- | `FindListByDeptIds` | user | 从未在任何 logic 中被调用 |
|
|
|
- | `FindByUserId` | userperm, userrole, productmember | 仅测试中使用 |
|
|
|
- | `FindPermIdsByUserIdAndEffect`(不含 ForProduct 后缀) | userperm | 生产代码使用 `ForProduct` 版本 |
|
|
|
- | `DeleteByUserIdTx`(不含 ForProduct) | userrole, userperm | 生产代码使用 `ForProductTx` 版本 |
|
|
|
+ 步骤 2 与步骤 3 之间,另一个管理员可能:
|
|
|
+ - 创建以该部门为父的子部门(`CreateDept`)
|
|
|
+ - 把某用户迁入该部门(`UpdateUser.DeptId`)
|
|
|
|
|
|
-- **建议**:这些函数可能是为了方便测试编写的辅助方法。如果确认不会对外暴露,可以标注注释说明用途;如果完全冗余,建议移除以保持接口精简。
|
|
|
+ 由于 `sys_dept` 没有对被删除部门的外键约束,删除会成功,留下"孤儿"子部门或"指向已删除部门的用户"。虽然都是超管才能执行、并发概率极低,但依然是逻辑一致性缺口。
|
|
|
+- **建议**:
|
|
|
+ - 方案一:把三步放进一个事务,并对 `sys_dept` 行加 `SELECT ... FOR UPDATE`。
|
|
|
+ - 方案二:采用"逻辑删除"代替物理删除。
|
|
|
+ - 方案三:在 `CreateDept` / `UpdateUser` 的写入端校验目标部门 `status=Enabled` 且未被删除。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M-8. CreateProduct 缺少产品编码格式校验
|
|
|
+### M-12. AddMember / BindRoles 的权限校验依赖 caller 的 ProductCode(JWT),但可操作的是 req.ProductCode
|
|
|
+
|
|
|
+- **位置**:`internal/logic/member/addMemberLogic.go`、`internal/logic/user/bindRolesLogic.go`
|
|
|
+- **描述**:当前通过 `RequireProductAdminFor(req.ProductCode)` 校验调用者是 "req.ProductCode 这个产品的管理员",这一步是正确的。但 `CheckMemberTypeAssignment(assignedType)` 内部是拿 `caller.MemberType`(= caller 自己 JWT 所在产品的 MemberType)和 `assignedType` 比较的;当 caller 切换到别的产品操作(通过传 req.ProductCode 与 caller.ProductCode 不同),这个比较实际是"A 产品的我的级别 vs B 产品要分配的级别",语义错配。
|
|
|
+
|
|
|
+ 目前由于 `RequireProductAdminFor` 要求 `caller.MemberType == ADMIN && caller.ProductCode == req.ProductCode`,非超管的跨产品路径已经被卡住;只有超管能跨产品,而超管在 `CheckMemberTypeAssignment` 里直接 return nil。所以**当前没有实际越权风险**。
|
|
|
+
|
|
|
+- **建议**:这属于"防御性冗余"——未来如果放宽 `RequireProductAdminFor` 的跨产品规则(例如允许全局 ADMIN),`CheckMemberTypeAssignment` 的语义就会失效。建议把 `CheckMemberTypeAssignment` 显式接收 `productCode`,内部按目标产品重新读 caller 在该产品的 MemberType:
|
|
|
|
|
|
-- **位置**:`internal/logic/product/createProductLogic.go` 第 44-55 行
|
|
|
-- **描述**:`CreateUser` 对 `username` 有严格的正则校验(`^[a-zA-Z0-9_]{2,64}$`),但 `CreateProduct` 对 `code` 仅校验了长度上限(64 字符),未校验格式。产品编码被广泛用作数据库 WHERE 条件和 Redis 缓存 key 的一部分。
|
|
|
-- **影响**:如果传入包含特殊字符(如空格、中文、`/`、`:`)的产品编码,虽然不会导致 SQL 注入(参数化查询),但可能导致:
|
|
|
- - Redis key 格式混乱
|
|
|
- - 自动生成的管理员用户名 `admin_{code}` 不合法
|
|
|
- - 前端 URL 编码问题
|
|
|
-- **建议**:增加正则校验:
|
|
|
```go
|
|
|
- var productCodeRegexp = regexp.MustCompile(`^[a-zA-Z][a-zA-Z0-9_-]{1,63}$`)
|
|
|
- if !productCodeRegexp.MatchString(req.Code) {
|
|
|
- return nil, response.ErrBadRequest("产品编码只能包含字母、数字、下划线和中划线,须以字母开头")
|
|
|
- }
|
|
|
+ func CheckMemberTypeAssignmentFor(ctx, svcCtx, productCode, assignedType) error
|
|
|
```
|
|
|
|
|
|
---
|
|
|
|
|
|
-### M-9. UpdateDept 修改 status 时未清理子部门用户的缓存
|
|
|
+## 📝 低风险 / 遗留问题 (Low)
|
|
|
|
|
|
-- **位置**:`internal/logic/dept/updateDeptLogic.go` 第 73-92 行
|
|
|
-- **描述**:当修改部门 `status` 为禁用时,仅清理了该部门直属用户的缓存。子部门用户的缓存清理逻辑被包裹在 `if req.DeptType == "NORMAL" || req.DeptType == "DEV"` 条件中,仅当请求中显式传入了 `DeptType` 字段才会执行。
|
|
|
-- **影响**:如果禁用一个 DEV 类型部门时只传入 `status=2` 而不传入 `deptType`,该部门下所有**直属**用户的缓存会被清理,但子部门的用户缓存不会。由于 `loadPerms` 只检查用户**自身**部门的 DeptType 和 Status,子部门用户不受父部门状态影响,所以当前行为在逻辑上是**正确的**。但代码意图不够清晰,建议增加注释说明为什么子部门不需要级联清理。
|
|
|
+### L-1. 敏感配置明文提交到仓库(遗留)
|
|
|
+
|
|
|
+- **位置**:`etc/perm-api-dev.yaml`、`etc/perm-api-prod.yaml`、`etc/perm-api-test.yaml`、`etc/perm-api-xiaom.yaml`
|
|
|
+- **描述**:MySQL/Redis 密码、AccessSecret、RefreshSecret、ManagementKey 等均以明文形式存在,且已提交至 Git。即使后续轮换,历史提交仍留痕。
|
|
|
+- **建议**:改用环境变量或密钥管理服务注入,`etc/*.yaml` 只保留模板。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### L-1. UserList 在超管模式下无 productCode 时查询全量用户
|
|
|
+### L-2. 登录限流桶的维度混用
|
|
|
|
|
|
-- **位置**:`internal/logic/user/userListLogic.go` 第 56-62 行
|
|
|
-- **描述**:当超管不传 `productCode` 时,执行 `FindListByPage` 查询**全量用户**。在用户量较大(如数千用户)时,虽然有分页,但 `COUNT(*)` 查询可能较慢(全表扫描,无 WHERE 条件)。
|
|
|
-- **建议**:如果用户表数据量增长到万级以上,考虑为 `COUNT(*)` 增加缓存或近似计数。当前业务规模下此为低优先级。
|
|
|
+- **位置**:`internal/svc/servicecontext.go` 第 31 行、`internal/handler/routes.go` 第 143-160 行
|
|
|
+- **描述**:`LoginRateLimit`(IP 60s 20 次)**同时**挂给了 `/auth/login` 和 `/auth/adminLogin`。AdminLogin 现在有了 `UsernameLoginLimit`(5 分钟 10 次)作为第二道防线,这是合适的。但两个路由共享同一个 IP 桶,意味着:
|
|
|
+ - 办公网 NAT 出口下 20 人在 1 分钟内都在尝试登录时,其他登录被误判。
|
|
|
+ - 对产品端登录的合法压力会挤占管理后台登录的配额,反之亦然。
|
|
|
+- **建议**:拆成两个独立桶(`rl:login:product`、`rl:login:admin`),并可适度提高 product 端桶的配额。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### L-2. DeptTree 一次性加载所有部门到内存构建树
|
|
|
+### L-3. `UpdateDept` 整行回写,未使用乐观锁
|
|
|
|
|
|
-- **位置**:`internal/logic/dept/deptTreeLogic.go` 第 28 行
|
|
|
-- **描述**:`FindAll` 一次性查询所有部门记录,在内存中构建树结构。对于权限系统的典型规模(几十到几百个部门),这是完全合理的做法。
|
|
|
-- **建议**:无需优化。仅在此记录,如果未来部门数量异常增长,可考虑前端懒加载。
|
|
|
+- **位置**:`internal/logic/dept/updateDeptLogic.go` 第 42-65 行
|
|
|
+- **描述**:仍是 Read-Modify-Write 模式,没有 `updateTime` 条件乐观锁(不像 `UpdateUser` 已用 `WHERE updateTime=?`)。部门操作是超管串行动作,并发概率极低,风险有限。
|
|
|
+- **建议**:若希望统一范式,与 `UpdateProfile` 一样接上 `WHERE updateTime = ?` 的乐观锁,失败返回 `ErrConflict`。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### L-3. BindRoles 和 BindRolePerms 采用全量删除后重新插入
|
|
|
+### L-4. UpdateRole 允许 ADMIN 任意调整 permsLevel(设计层面)
|
|
|
|
|
|
-- **位置**:`internal/logic/user/bindRolesLogic.go`、`internal/logic/role/bindRolePermsLogic.go`
|
|
|
-- **描述**:绑定角色/权限时,事务中先 `DELETE` 该用户(或角色)在当前产品下的所有关联记录,再 `BatchInsert` 新记录。这种"先删后插"方式简洁可靠,适合关联数量不大(通常每用户 < 20 个角色,每角色 < 200 个权限)的场景。
|
|
|
-- **建议**:当前实现合理。如果未来出现单角色绑定数百个权限的场景,可优化为差异更新(仅插入新增、删除移除)以减少写入量。
|
|
|
+- **位置**:`internal/logic/role/updateRoleLogic.go`
|
|
|
+- **描述**:产品 ADMIN 可以把一个角色的 permsLevel 从 500 改成 1,并绑定给普通 MEMBER,以此让该 MEMBER 的 `MinPermsLevel = 1`,进而绕过 `checkPermLevel` 的等级约束。由于 ADMIN 本身已经是高级别角色并拥有该产品全部权限,实际并未提升其权力范围;但"下放"级别的能力会让普通 MEMBER 能管理更多同级用户。
|
|
|
+- **建议**:如希望 permsLevel 成为"不可越级下放"的强约束,可要求修改 permsLevel 到低于某个阈值时必须是超管。当前风险较小。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### L-4. createDeptLogic 在事务中执行两步操作以回填 Path
|
|
|
+### L-5. singleflight 在 `Load` 失败路径仍返回带零值的 UserDetails
|
|
|
|
|
|
-- **位置**:`internal/logic/dept/createDeptLogic.go` 第 68-93 行
|
|
|
-- **描述**:创建部门时需要先 INSERT 获得自增 ID,再用 ID 构建 `path` 字段并 UPDATE。代码使用了事务保证两步操作的原子性,实现正确。
|
|
|
-- **建议**:当前实现合理。如果追求极致优化,可考虑使用 UUID 或预分配 ID 来避免两步操作,但收益极小。
|
|
|
+- **位置**:`internal/loaders/userDetailsLoader.go` 第 106-134 行
|
|
|
+- **描述**:当 `loadFromDB` 的 `ok == false`(例如用户不存在)时,仍然把 `ud` 返回给 `sf.Do` 调用方;随后 `Load` 的 caller 通过 `ud.Username == ""` 判断。目前中间件/登录路径都有后续的 `Username == ""` 检查,因此不会被滥用。建议在 Load 内部直接 `return nil`(不缓存、不复用),让上游直接看到 nil 语义,减少误用风险。
|
|
|
|
|
|
---
|
|
|
|
|
|
-### L-5. permclient.GetUserPerms 缺少 AppKey/AppSecret 参数传递
|
|
|
+### L-6. FindAllCodesByProductCode 仅按 status 过滤,依赖 UNIQUE (productCode, code) 去重
|
|
|
|
|
|
-- **位置**:`permclient/permclient.go` 第 58-63 行
|
|
|
-- **描述**:
|
|
|
- ```go
|
|
|
- func (c *PermClient) GetUserPerms(ctx context.Context, userId int64, productCode string) (*pb.GetUserPermsResp, error) {
|
|
|
- return c.cli.GetUserPerms(ctx, &pb.GetUserPermsReq{
|
|
|
- UserId: userId,
|
|
|
- ProductCode: productCode,
|
|
|
- })
|
|
|
- }
|
|
|
- ```
|
|
|
- gRPC 服务端 `GetUserPerms` 需要 `AppKey` 和 `AppSecret` 进行产品身份验证,但客户端封装函数未传递这两个参数,**请求必然失败**(返回 `Unauthenticated`)。
|
|
|
-- **影响**:任何通过 `permclient.GetUserPerms` 发起的调用都无法通过服务端的认证校验。
|
|
|
-- **修复方案**:
|
|
|
- ```go
|
|
|
- func (c *PermClient) GetUserPerms(ctx context.Context, appKey, appSecret string, userId int64, productCode string) (*pb.GetUserPermsResp, error) {
|
|
|
- return c.cli.GetUserPerms(ctx, &pb.GetUserPermsReq{
|
|
|
- AppKey: appKey,
|
|
|
- AppSecret: appSecret,
|
|
|
- UserId: userId,
|
|
|
- ProductCode: productCode,
|
|
|
- })
|
|
|
- }
|
|
|
- ```
|
|
|
+- **位置**:`internal/model/perm/sysPermModel.go` 第 53-60 行
|
|
|
+- **描述**:`SELECT code FROM sys_perm WHERE productCode=? AND status=1`。依赖 `uk_product_code(productCode, code)` 来保证单产品下 code 唯一,这是 schema 约束层面保证的。若将来有场景允许软删除(status=2)后重新创建同 code,需要注意历史禁用记录的去重。当前无风险,仅作提醒。
|
|
|
|
|
|
---
|
|
|
|
|
|
@@ -291,12 +444,27 @@
|
|
|
|
|
|
| 维度 | 评估 |
|
|
|
|------|------|
|
|
|
-| **逻辑一致性** | 整体良好。UpdateProduct 状态校验是唯一的行为不一致点。 |
|
|
|
-| **并发与竞态** | 发现 1 个严重的 Read-Modify-Write 竞态条件(H-1),涉及安全关键的 tokenVersion 字段。 |
|
|
|
-| **资源管理** | 良好。所有数据库操作通过 go-zero 连接池管理,事务使用正确,无泄漏风险。 |
|
|
|
-| **数据完整性** | 关键写操作(BindRoles、BindPerms、RemoveMember、DeleteRole、CreateProduct、SyncPerms)均在事务中执行,原子性有保障。 |
|
|
|
-| **安全漏洞** | SQL 注入风险:无(全部参数化查询)。水平越权:已有完善的 CheckManageAccess 层级校验。发现管理后台暴力破解风险(H-2)和限流竞争问题(H-3)。 |
|
|
|
-| **边界处理** | 对 nil、空值、可选字段的处理较为完善。`UserDetails` 的零值初始化合理。 |
|
|
|
-| **数据库性能** | 存在可优化的冗余读取(M-1),但整体无 N+1 查询问题。列表接口批量查询+map 组装的模式正确。 |
|
|
|
-| **僵尸代码** | 发现 1 个僵尸函数(M-5)、3 个未使用的 middleware helper(M-6)、10+ 个仅测试调用的 model 方法(M-7)。 |
|
|
|
-| **接口契约** | gRPC 客户端 `GetUserPerms` 缺少必要参数(L-5),会导致调用方无法正常使用。 |
|
|
|
+| **逻辑一致性** | 发现两个严重逻辑 Bug:BindRoles 对 ADMIN/DEVELOPER 的 permsLevel 校验误伤(H-1),以及 DEV 部门绕过产品成员禁用(H-3)。 |
|
|
|
+| **并发与竞态** | 关键写操作已使用乐观锁/事务,`UpdateProfile` 已引入 `WHERE updateTime=?`;只剩 `DeleteDept` 的 TOCTOU(M-11)和 `UpdateDept` 的 RMW(L-3)。 |
|
|
|
+| **资源管理** | DB/Redis 连接统一由 go-zero 池管理,未见泄漏;事务 `TransactCtx` 用法正确。 |
|
|
|
+| **数据完整性** | 核心写入(创建产品、绑定角色权限、删除角色、同步权限、移除成员)都已放入事务。缓存失效与 DB 提交非原子(M-10),Redis 故障时存在 ≤5 min 的陈旧窗口。 |
|
|
|
+| **安全漏洞** | gRPC `GetUserPerms` 未校验用户状态(H-2),存在"本系统已冻结,对外仍有权限"的一致性漏洞;DEV 部门旁路(H-3)是真实的水平越权路径;组织架构/产品列表过度暴露(M-8、M-9);adminPassword 熵过低(M-1)。 |
|
|
|
+| **边界处理** | 对 `nil`、空串、可选字段(指针)普遍处理得当;`UserDetails` 零值语义合理。 |
|
|
|
+| **数据库性能** | BindRoles / BindRolePerms 的逐条 DELETE(M-2)可批量化;其他列表接口均采用"批量查询 + map 组装"的正确模式,无 N+1。 |
|
|
|
+| **僵尸代码** | `Claims.Perms` 字段未被产线使用(M-6);上一版审计报告中的几个 helper 与 model 函数已清理。 |
|
|
|
+| **接口契约与对象完整性** | `UserDetailLogic` 返回字段与产品上下文语义不一致(M-3);`FindRoleIdsByUserIdForProduct` 未过滤角色状态(M-4);`UpdateDept` 的缓存级联过宽(M-5)。 |
|
|
|
+
|
|
|
+### 修复优先级建议
|
|
|
+
|
|
|
+1. **立即修复(P0)**:
|
|
|
+ - H-1 BindRoles 对 ADMIN 的误拦截 —— 直接影响产品管理员最核心的运营能力。
|
|
|
+ - H-2 gRPC `GetUserPerms` 未校验冻结用户 —— 跨系统一致性漏洞,可能被接入方当作"真相源"从而放行冻结用户。
|
|
|
+ - H-3 DEV 部门绕过产品成员禁用 —— 真实的越权路径。
|
|
|
+
|
|
|
+2. **短期修复(P1)**:
|
|
|
+ - M-1 弱密码生成函数
|
|
|
+ - M-2 批量 DELETE
|
|
|
+ - M-3/M-4 roleIds 返回语义
|
|
|
+ - M-10 缓存失效的原子性补偿
|
|
|
+
|
|
|
+3. **中期修复(P2)**:其他 M 级条目与 L 级遗留项。
|