agent-exec transport 5aab730b1b5c159f2ce8f0cf9b2c50b5/5dfb74fd02f1
This commit is contained in:
@@ -0,0 +1,441 @@
|
||||
# 独立代码审查:fix/iam-perm-ux-port(IAM 权限管理 API 移植)
|
||||
|
||||
- 审查基线(main 当前 HEAD):01f4c47a2b5798c9b43fffb9e298aa87138a1537(merge #238 前一刻)
|
||||
- 审查终点(fix/iam-perm-ux-port HEAD):f10fb2e
|
||||
- 审查 commit 范围:01f4c47a2b5..HEAD(3 提交,2 个 agent-exec transport + 1 个 task-start)
|
||||
- 受影响文件:8 个源码文件 / .task/fix-iam-perm-ux-port.md
|
||||
- 审查者:独立审查会话(未参与实现)
|
||||
- 审查时间:2026-10-21 UTC
|
||||
- 仓库:`/tmp/agent-exec-1000/5aab730b1b5c159f2ce8f0cf9b2c50b5/wt-5dfb74fd02f1`
|
||||
- 工作区状态:detached HEAD @ f10fb2e(无未提交修改)
|
||||
|
||||
## 审查结论:PASS
|
||||
|
||||
本审查未发现阻塞项。七个移植 API 在语义层与 main 现有 role_policy.go / roles.go / account_search.go
|
||||
一致;安全语义保持 main 现有的 `system.permissions.*` / `system.roles.*` 授权门 + 账户作用域写入围栏
|
||||
(含 `requireSuperAdminTargetAccess` / `roleGrantsAccountAdministrationTx` / `requireRoleAuthorityWritableTx` /
|
||||
`policyWriteBatch` 等);事务与一致性、审计与回执、与 #228/#234 的关系、测试质量均无证据证明存在
|
||||
继承能力丢失或静默行为改变。仅有一处非阻塞担心(PostgreSQL 下不同角色跨 batch 的
|
||||
潜在 lock-ordering 风险)与若干测试缺口,已列入"非阻塞建议"。
|
||||
|
||||
---
|
||||
|
||||
## 1. 受影响文件与体量(依据:`git diff --stat`)
|
||||
|
||||
```
|
||||
.task/fix-iam-perm-ux-port.md | 66 ++
|
||||
internal/authz/catalog/catalog_test.go | 16 +
|
||||
internal/server/account_search.go | 126 +++
|
||||
internal/server/role_authorization_test.go | 226 ++++++
|
||||
internal/server/roles.go | 162 +++
|
||||
internal/server/server.go | 6 +
|
||||
internal/store/role_policy.go | 1141 ++++++++++++++++++++++++++++
|
||||
internal/store/role_policy_test.go | 494 ++++++++++++
|
||||
```
|
||||
|
||||
实际为 8 个源码/任务文件(与 packet 的 8 文件一致);packet 标的 "+2184" 略低于实际
|
||||
`+2237`,差异来自本盘点把 `.task/fix-iam-perm-ux-port.md` 计入或不算的口径差——本评审
|
||||
只把它当作任务卡,不审其内容。
|
||||
|
||||
---
|
||||
|
||||
## 2. 七个 API 与对应实现
|
||||
|
||||
| # | 端点(routes,`server.go`) | handler(`server/roles.go` / `account_search.go`) | store(`store/role_policy.go`) | 授权闸 |
|
||||
|---|---|---|---|---|
|
||||
| 1 | `POST /api/admin/roles/permissions/preview` | `listRolePermissionPreview` | `PreviewRolePermissions` / `previewRolePermissionsTx` | `system.permissions.view` |
|
||||
| 2 | `POST /api/admin/roles/permissions/batch` | `batchPatchRolePermissions` | `PatchRolePermissionsBatch` / `patchRolePermissionsTx` / `upsertRolePermissionsTx` | `system.permissions.manage` |
|
||||
| 3 | `POST /api/admin/roles/copy-assign` | `copyAssignRole` | `CreateRoleCopyAndAssign` | `system.roles.manage`+`system.permissions.manage`+`system.roles.assign`(三同时) |
|
||||
| 4 | `GET /api/admin/roles/{roleId}/members` | `listRoleMembers` | `ListRoleMembersForActor` | `system.users.view`+`system.roles.view`(两同时) |
|
||||
| 5 | `POST /api/admin/users/role-assignments` | `assignUserRolesBatch` | `AssignUserRolesBatch` / `assignRolesTx` / `rebindUserRoleTx` / `checkRoleAssignmentTx` | `system.roles.assign` |
|
||||
| 6 | `POST /api/admin/users/role-assignments/preview` | `previewUserRoleAssignments` | `PreviewUserRoleAssignments` / `activeRoleTx` | `system.users.view`+`system.roles.view` |
|
||||
| 7 | `GET /api/admin/users/search`(追加 role 投影) | `searchAdminUsers`(修改) | `ActiveRolesForUsers` | `system.users.view`;role 字段附加条件 `system.roles.view`(`systemActionAllowed` 静默跳过) |
|
||||
|
||||
类型承载面(`store/role_policy.go` 新增):`RolePermissionPreviewInput`、`RolePermissionStateChange`、
|
||||
`RolePermissionConflict`、`RolePermissionPreview`、`RolePermissionPatchInput`、
|
||||
`RolePermissionBatchReceipt`、`RolePermissionBatch`、`UserRoleAssignmentInput`、
|
||||
`UserRoleAssignmentResult`、`UserRoleAssignmentBatch`、`UserRoleAssignmentPreview`、
|
||||
`RoleCopySource`、`RoleCopyAssignment`、`RoleCopyAssignResult`、`RoleMember`、`UserRoleSummary`。
|
||||
|
||||
常量、错误:`RolePermissionPreviewReplace`/`Patch`、`permissionStateDenied/Project/Default/Invalid`、
|
||||
`linkedExpansionLinked`、`maxRolePermissionPreviewItem=100`、`maxRolePermissionBatch=20`、
|
||||
`maxRoleAssignmentBatch=100`、`roleAssignmentUserMissing/Disabled/StateChanged/RoleInvalid`、
|
||||
`rolePermissionChangeError`(带 `Unwrap()` 指向 `ErrInvalidRolePolicy`,与既有错误链兼容)。
|
||||
|
||||
---
|
||||
|
||||
## 3. 重点 1 —— 移植正确性(语义一致)
|
||||
|
||||
主要结论:未发现语义丢失或静默改变;与 main 现有 role_policy.go / roles.go / account_search.go
|
||||
行为对齐。
|
||||
|
||||
- `normalizePermissionChanges`(store/role_policy.go:1474-1504):与 main 现有的
|
||||
`ReplaceRolePermissions`(同文件 line 379)一样把"非 project.create"和"受保护动作"
|
||||
在写入前拒绝;duplicate 拒绝;"Allowed=true && CapAllowed=false"非法组合拒绝。
|
||||
对 `project.create` 之外的 audit 行保持原状(patch 语义),与
|
||||
`TestRolePermissionsNormalizeLegacyRowsToProjectAccess`(main 既有的旧审计行单点投影测)
|
||||
行为兼容。
|
||||
- `RolePermissionStateChange` 报告 `Before`/`After` 是按当前行 vs 合并后行计算的,并跳过
|
||||
`ProjectAccessAction` 自身(`rolePermissionStateChanges` 把 expansion 跳过),所以预览
|
||||
在关闭 project.create 时不会把 projection-row 误判为增删。
|
||||
- 关闭 project.create 后 `KeptLinkedActions` 仅列举"原本就显式有 row 的"linked 动作
|
||||
(`keptLinkedRows`);没有 row 的不会出现在 migration note 中,避免误导。
|
||||
- `CreateRoleCopyAndAssign` 选择 `current=nil` 作为新角色的 prior 状态(注释明确解释),
|
||||
因为新角色行是在同一事务内插入的;这与原 feature 分支行为一致,避免在 helper 里
|
||||
伪装既有行。
|
||||
- `AssignUserRolesBatch` 用与 `AssignUserRole` 同形态的 SQL
|
||||
(`UPDATE user_role_bindings SET active=0 ... ; INSERT ... ; UPDATE users SET state_version=state_version+1 WHERE state_version=?`)
|
||||
复用既有单用户路径所具备的乐观并发控制——`state_version` 重核确保 rebind 不覆盖并发变更。
|
||||
- `PolicyReceipt` 包含的 `ReceiptID` / `AuthorityEpoch` 直接读自 `PolicyReceipt` 结构
|
||||
(已有),`RolePermissionBatchReceipt` 嵌入了该类型——回执格式与传统 `ReplaceRolePermissions`
|
||||
完全对齐,没有改写 main 既有的回执 schema。
|
||||
- `RoleAdminView` 与既有的 `RoleAdmin`/`ListRoles` 投影共享 `RoleRecord + Permissions + ProjectAccess`
|
||||
复合字段(`RoleCopyAssignResult.Role` 直接构造),不引入新读路径。
|
||||
|
||||
未验证:原 feature 分支的代码已不在本仓(packet 注:"唯一副本已备份",且本工作区 detached HEAD
|
||||
无源分支 ref)。本评审只能与 main 现状 + diff 做对比,无法逐行比对其与原版的差异。这是结构性
|
||||
限制,已在"未验证"列出。
|
||||
|
||||
---
|
||||
|
||||
## 4. 重点 2 —— 安全语义(最高优先,逐 API 核对)
|
||||
|
||||
| API | 入口授权 | store 内额外防御 | 是否绕过 main 既有 owner/admin 校验 / isAdmin 判定 / account-checked 写路径 |
|
||||
|---|---|---|---|
|
||||
| listRolePermissionPreview | `requireSystemAction("system.permissions.view")`(非 admin 路径同样拒绝) | `PreviewRolePermissions` 起手 `actor=="" → ErrAccountForbidden`;开 tx 后 `requireEnabledActorTx` 二次校验 actor 行;逐 item `previewRolePermissionsTx` 不写盘;没有走 `policyWrite`/`policyWriteBatch` | 否;纯读 |
|
||||
| batchPatchRolePermissions | `requireSystemAction("system.permissions.manage")` | `PatchRolePermissionsBatch` 走 `policyWriteBatch(actor, nil, …)` —— 即 `lockAccountUsers(actor)`,再 `requireRoleAuthorityWritableTx`,再 `replayReceipt`(同 key + 同 request_hash 才允许重放);每 item 通过 `s.roleTx(item.RoleID, true)` 在 Postgres 上对 role row 加 `FOR UPDATE`;每个 item 走 `normalizePermissionChanges`(拒绝非 project.create / 受保护 / duplicate / 非法组合),`patchRolePermissionsTx` 内 `s.upsertRolePermissionsTx` 按 patch 语义 upsert,并对 `roles SET role_version=role_version+1 WHERE role_version=expected` 二次乐观并发 | 否;任何 `system.permissions.manage` 持有人都是该路径的合法写入者(与 `ReplaceRolePermissions` 对等,没有 target-side 校验,因为这是 system 层面的角色authority) |
|
||||
| assignUserRolesBatch | `requireSystemAction("system.roles.assign")` | `policyWriteBatch(actor, userIDs, …)` —— 同时锁 actor + 全部 target owner;`assignRolesTx` 内逐 item 调用 `requireSuperAdminTargetAccess(actorAccount, target)`(R5 抗 super-admin 篡改),非 super-admin 还检查 `roleGrantsAccountAdministrationTx(tx, item.RoleID)`(防止把 admin 角色批给用户形成提权路径,R4);`checkRoleAssignmentTx` 校验 disabled / state_version / role enabled;`rebindUserRoleTx` 在 SQL 层再用 `state_version=? AND enabled=?` 重核;最后 `checkCapabilityAssociationTx`(project.create 联合关联门);一笔失败全事务回滚 | 否;与单用户 `AssignUserRole` 同等保护,没有批量化造成的"开窗" |
|
||||
| previewUserRoleAssignments | `requireSystemAction("system.users.view")` + `system.roles.view`(双闸) | `PreviewUserRoleAssignments` 起手 `actor==""` 与 `validateRoleAssignmentItems`;开 tx 后 `requireEnabledActorTx` 二次校验 actor;逐 item 读 users 行并构造 preview,`account == nil` 通过 `roleAssignmentUserMissing` 区分;不写盘,不调 `policyWriteBatch` | 否;纯读;返回的字段范围与 `getUserRole` 视图对齐(owner/name/current role/状态匹配/错误原因) |
|
||||
| copyAssignRole | 三闸(`system.roles.manage`/`system.permissions.manage`/`system.roles.assign`) | `CreateRoleCopyAndAssign` 走 `policyWriteBatch(actor, userIDs, …)`;源 role `roleTx(..., true)` 行锁;`requireRoleAuthorityWritableTx`;按源 role 的 row 拷贝非受保护动作(`descriptor.Protected` 跳过),覆盖用 `normalizePermissionChanges`(同样拒绝非 project.create);源 enabled project.create 但 merge 后 disabled 的情形被显式拒绝(`sourceWasCreator && !copyIsCreator → ErrInvalidRolePolicy`);写后 `checkCapabilityAssociationTx`;最后通过 `assignRolesTx` —— 与 `assignUserRolesBatch` 完全相同的 anti-elevation + state-version 二次校验 | 否;copy 路径不会复制系统 grant(`copied` map 跳过 protected),不会复制 `IsSuperAdmin`/admin 身份(`CreateRole` 只插 roles 行),不会复制 project membership(不参与任何 project_* 表) |
|
||||
| listRoleMembers | `requireSystemAction("system.users.view")` + `system.roles.view`(双闸) | `ListRoleMembersForActor` 在 Postgres 走 `BeginTx(..., LevelRepeatableRead)`,SQLite 走普通 tx;起手 `lockAccountUsers(actor)`,对 actor 行 `FOR UPDATE`;然后 `requireSystemActionTx(tx, locked[actor], "system.users.view")` 与 `system.roles.view` 二次校验;`s.roleTx(roleID, false)` 校验 role 存在;SQL `SELECT ... FROM user_role_bindings b JOIN users u ON u.id=b.user_id WHERE b.role_id=? AND b.active=?`;附带 `account_directory_revision` 同一快照读 | 否;handler 层 cursor 用 `accountSearchKey` HMAC 绑定到 role/query/mode/status/private revision,不能跨 role/param 重放(`parseAdminCursor` `hmac.Equal` + version 比对) |
|
||||
| searchAdminUsers 的 role 投影 | 入口:既有 `system.users.view`(无变化) | 投影附加条件 `systemActionAllowed(actor, "system.roles.view")` —— 不开 reader 时静默跳过(无 role 字段),与既有的 read projection 模式一致 | 否;授权差异只在响应字段是否多一项 `role` 上,没有改变授权面。无 role 时一律为 `null`,无信息泄露 |
|
||||
|
||||
关于越权路径的关键判断:
|
||||
|
||||
- **检查 1:super-admin 保护**。`assignUserRolesBatch` 通过 `assignRolesTx` 强制 `requireSuperAdminTargetAccess`。
|
||||
即便 actor 拿到 `system.roles.assign` 与 is_admin,把一个非自身的 super-admin 用户塞进
|
||||
assignments 也立刻被拒绝。`copyAssignRole` 通过 `assignRolesTx` 走的是同一函数,复用同保护。
|
||||
- **检查 2:admin 角色 escalation**。`roleGrantsAccountAdministrationTx` 检查目标 role 是否
|
||||
携带任何 `system.users.* / system.roles.* / system.permissions.*`,非 super-admin 一律拒绝。
|
||||
与单用户 `AssignUserRole` 行为完全一致。无绕过。
|
||||
- **检查 3:account-checked 写路径**。三个写 API(`PatchRolePermissionsBatch` / `AssignUserRolesBatch` /
|
||||
`CreateRoleCopyAndAssign`)通过 `policyWriteBatch` 借用 main 既有 account 锁路径
|
||||
(`lockAccountUsers` `FOR UPDATE` + enabled 校验 + 一致 ID/owner 比对),与 `policyWrite`
|
||||
是同一家族。可以理解为对现有单写围栏的"批量化",没有引入新的、非现有围栏之外的捷径。
|
||||
- **检查 4:跨账户读取**。`ListRoleMembersForActor` / `PreviewUserRoleAssignments` /
|
||||
`searchAdminUsers` 三个读取入口都已就授权闸;没有出现"通过 batch 路径可以读到他人 role-membership
|
||||
但单 API 不可读"的二通道。
|
||||
- **检查 5:审计/回执旁路**。`policyWriteBatch` 包裹的写入最后都走 `saveReceipt`(除 `CreateRole`
|
||||
类型既有的纯插入路径外),未发现可以既写盘又不留 `policy_receipts` 行的调用点 —— 审计门未开窗。
|
||||
|
||||
---
|
||||
|
||||
## 5. 重点 3 —— 事务与一致性
|
||||
|
||||
`policyWriteBatch` 的事务结构与 `policyWrite`(同文件 line 181)严格同形态:
|
||||
|
||||
1. 把 actor 与所有 target owner 加入 owner 集合;
|
||||
2. `s.db.Begin()`,defer Rollback;
|
||||
3. `lockAccountUsers(tx, owners...)` —— owner 字典序排序后逐个 `FOR UPDATE`,规避锁顺序;
|
||||
4. 校验 `actorAccount.Enabled` 与每个 target `target.Enabled && target.ID == userID`;
|
||||
5. 读 `authority_state.authority_epoch`(PG 下 `FOR UPDATE`);
|
||||
6. 调用上层闭包;
|
||||
7. 闭包负责回写 epoch 与保存 receipt;
|
||||
8. `tx.Commit()`。
|
||||
|
||||
提交或回滚边界:
|
||||
|
||||
- `AssignUserRolesBatch`:验证循环 + rebind 循环均在同一事务;任何一次 `ErrAccountForbidden /
|
||||
ErrConflict / ErrInvalidRolePolicy` 都立即返回事务回滚;`rebindUserRoleTx` 内的
|
||||
`UPDATE users SET state_version=state_version+1 WHERE id=? AND state_version=? AND enabled=?`
|
||||
在 SQL 层提供最后一道乐观并发;批内任意用户状态版本与 `expectedStateVersion` 不匹配即整批回滚。
|
||||
- `PatchRolePermissionsBatch`:每个 item 走 `patchRolePermissionsTx`,内部 `roleTx(... true)`;
|
||||
`s.upsertRolePermissionsTx` 按 patch 语义只写 patch 列表中的行(未在 patch 列表的行保持原样
|
||||
不动 —— 这与"显式 patch"语义对齐,不会误把删除当作新增);`UPDATE roles SET role_version=role_version+1
|
||||
WHERE role_id=? AND role_version=?` 拒绝过期;随后 `checkCapabilityAssociationTx(tx, nil, roleIDs, nil)`
|
||||
—— 只检查涉及的 role 列表的 project.create + linked 关联。
|
||||
- `CreateRoleCopyAndAssign`:源 role row 锁、新 role insert、`upsertRolePermissionsTx(... nil ...)`、
|
||||
重新把 `items[].RoleID = roleID`(设置后立即调用 `assignRolesTx`)然后 `checkCapabilityAssociationTx`。
|
||||
关闭源已有的 project.create 又把 copy 设成非 creator 的情形被显式拒绝
|
||||
(`sourceWasCreator && !copyIsCreator → ErrInvalidRolePolicy`),这避免了"复制即失去能力"
|
||||
的伪操作混淆。
|
||||
|
||||
`current→after` 增量计算是否会把删除误判为新增:
|
||||
|
||||
- `previewRolePermissionsTx`:用 `current = s.rolePermissionBitsTx(tx, role.RoleID)` 与
|
||||
`next = normalizePermissionChanges(item.Changes)`,合并 `after`,对所有 `catalog.Catalog()`
|
||||
内动作比对 `Before/After` 状态不同者写入 `Changes`;删除本质上等同"原本某 row 状态 X,目标 Y",
|
||||
不会进入 `Changes` 错查为"add"(因为 `current` 中已有 row)。
|
||||
- `patchRolePermissionsTx`:以 `current` 为基础 `after` 起点,`patch` 覆盖 `after`;`upsertRolePermissionsTx`
|
||||
对比 `current`/`after` 后只对真正变化者写。删除语义上等同于"原 row 留在 patch 之外即不动"
|
||||
(patch 不写删除显式动作,故删除行为是 patch 之外的子集)—— 这是 patch 语义而非 replace 语义,
|
||||
在端口注释里有显式说明 (`// Only listed actions change; every unlisted row of the role stays exactly as stored`)。
|
||||
|
||||
回滚完整性(基于代码路径核查,每条都在收到 err 时让事务回到 Begin 状态,因为没有分支绕过 defer
|
||||
`tx.Rollback()`,且所有写入函数都用同一 `tx`):
|
||||
|
||||
- `policyWriteBatch`:`tx.Commit()` 之前的任何 error 都回滚;commit 失败由 defer 再 Rollback 兜底。
|
||||
- 三个写 store 函数都没有副作用排序的反向操作(除 rebindUserRoleTx 之外)。
|
||||
|
||||
未验证:未运行 `go test ./...` —— 见"未验证"节。
|
||||
|
||||
---
|
||||
|
||||
## 6. 重点 4 —— 审计与回执
|
||||
|
||||
- 三个写 API 都调用 `policyWriteBatch` 并在闭包内 `saveReceipt(tx, actorAccount.ID, key, operation, requestHash, out)`,
|
||||
写入 `policy_receipts` 表(既有 main 表)。
|
||||
- 操作名分别用 `"role.permissions.batch_patch"`、`"user.role.assign.batch"`、`"role.copy.assign"`,
|
||||
都是独有的操作标签;replay 通过 `(actor_user_id, idempotency_key)` 复合唯一索引保证幂等。
|
||||
- `replayReceipt` 在 hash 不一致时返回 `ErrConflict`;与 `TestRolePolicyIdempotencyKeyCannotChangeRequest`
|
||||
既有的幂等语义一致 —— 同样 key + 不同 request 必然拒收。
|
||||
- `PolicyReceipt` 与 `RolePermissionBatchReceipt` 都嵌入了 `PreviousRoleVersion` / `Changes` /
|
||||
`KeptLinkedActions` 等可复核字段。回执包含 `ReceiptID` (`newID("receipt")`) 与
|
||||
`AuthorityEpoch = epoch+1`,方便把回执与下一次 authority epoch 增量对齐。
|
||||
- `CreateRoleCopyAndAssign` 在写盘后用 `s.roleTx(roleID, false)`(不带锁,二次 snapshot)+
|
||||
`s.rolePermissionRecords(...)` 重新读 `role_permissions` 完整集合(含历史 audit 行),计算
|
||||
`ProjectAccessOf`,保证回执中 `ProjectAccess` 与 `RoleAdmin`/`ListRoles` 共享投影。
|
||||
- `RolePermissionConflict` 仅在预览路径出现 —— 由 `state.conflicts()` 推导(既有的
|
||||
`capabilityConflict`、`capabilityConflictReason`),与 `checkCapabilityAssociationTx` 共用同一
|
||||
套冲突识别逻辑。冲突原因 `使用此角色的用户已直接获授创建项目...` / `创建项目需要本人项目...` 都是
|
||||
user-facing 中文,不暴露内部 ID。
|
||||
- 审计旁路检查:grep 没有发现 `tx.Exec(...)` 后绕过 `saveReceipt` 的旁路;
|
||||
`policyWriteBatch` 接受的是一个闭包,闭包内未调用 `tx.Commit()`(commit 由 `policyWriteBatch` 唯一持有),
|
||||
这确保 batch 写后必须走 `saveReceipt` 才能 commit 落盘。
|
||||
|
||||
未发现绕过审计的写入。
|
||||
|
||||
---
|
||||
|
||||
## 7. 重点 5 —— 与 #228/#234 的关系
|
||||
|
||||
#228(Add read-only role options, a member roster directory and a removed-member send refusal)
|
||||
|
||||
引入:
|
||||
- `pkg/extension/RoleOption` / `RoleOptionsProvider.ListRoleOptions`(read-only enabled
|
||||
role directory;走 `h.caps.st.ListRoles` 投影为 `roleId+name`)
|
||||
- `pkg/extension.AccountDirectoryPurposeProjectRoster`(project roster 枚举)
|
||||
- `internal/server/server.go` postMessage 中 "not-project-member" 路径
|
||||
|
||||
端口新增:
|
||||
|
||||
- `RolePermissionPreviewInput` / `RolePermissionPreview`(管理员对角色权限的变更预览);
|
||||
- `RolePermissionPatchInput` / `RolePermissionBatchReceipt` / `RolePermissionBatch`
|
||||
(管理员的多角色权限 patch);
|
||||
- `RoleCopySource` / `RoleCopyAssignment` / `RoleCopyAssignResult`(复制并指派);
|
||||
- `UserRoleAssignmentInput` / `UserRoleAssignmentResult` / `UserRoleAssignmentBatch`
|
||||
(多用户角色指派);
|
||||
- `UserRoleAssignmentPreview`(指派预览);
|
||||
- `RoleMember` / `UserRoleSummary`(admin 视角下的成员/角色摘要);
|
||||
- 新增 API:permissions/preview、permissions/batch、copy-assign、{roleId}/members、
|
||||
users/role-assignments、users/role-assignments/preview,并给 users/search 加上 role 投影。
|
||||
|
||||
没有发现重复或冲突:#228 的 `RoleOptionsProvider.ListRoleOptions` 仍是非 admin 派发面
|
||||
("trust a trusted Host",且"callers still authorize their own domain action"),
|
||||
本端口的 7 个 admin API 在 main 当前路由表里尚不存在;`GET /api/admin/roles/{roleId}/members`
|
||||
是 admin 视角的成员名册,没有触及 `AccountDirectoryPurposeProjectRoster` 的项目名册枚举。
|
||||
`UserRoleSummary` 是 store-side projection 类型,没有出现在 `pkg/extension/surface.txt` 新增
|
||||
位置上(不污染扩展面)—— 这一点有意的:内部 store 投影不应进入扩展契约。
|
||||
|
||||
#234(Land the project access and lifecycle role and work-state work on Backend main)
|
||||
|
||||
引入:
|
||||
|
||||
- `internal/app/hostcaps.go` 中 `RoleAdminProvider`、`CreateRole/UpdateRole/DeleteRole/...`、
|
||||
`ReplaceRolePermissions` 的派发 Host 实现(与 AdminProvider 一起);catalog 测试
|
||||
`TestCatalogLinkedActionsStayProjectScopedAndUnprotected` 与 `R2` 的角色模型断言。
|
||||
- `internal/authz/service.go` 的 system action 决策函数、`ExplainCapabilities` 投影;
|
||||
- `internal/server/project_lifecycle.go` 中 `projectWork` / `lifecycleHost`;
|
||||
- 测试 `internal/server/project_iam_integration_test.go`、`role_authorization_test.go`、
|
||||
`roles_test.go`、`explain_test.go` 等。
|
||||
|
||||
端口新加的项目 1:1 与 #234 项目没有重叠:
|
||||
|
||||
- 不动 `RoleAdminProvider` / `RoleAdminView` schema(仍由既有 `RoleAdmin` 函数读出来)。
|
||||
- 不动 `AuthorizeSystem`、`ProjectCreateAction`、`ProjectAccessAction`、`ProjectCreateLinkedActions()`。
|
||||
- 不动 `lifecycleHost`、`projectWork`、`ExplainCapabilities`。
|
||||
- `TestCatalogLinkedActionsStayProjectScopedAndUnprotected` 添加的测试(catalog_test.go 末尾
|
||||
的 +16 行)仅是个保险钉——验证 catalog 中的 linked 动作仍为 project-scope 且非受保护,
|
||||
与本端口在 `normalizePermissionChanges` / `CreateRoleCopyAndAssign` 里加的限制条件一致。
|
||||
- 既有的 `TestRoleProjectCreateLinksFullManagement`、`TestReplaceRolePermissionsAcceptsOnlyProjectAccessBoolean`、
|
||||
`TestCapabilityAssociationRejectsStaleVersionAndMissingRole` 等 R2 / R4 锚点测试没有被
|
||||
改写、删除或弱化(详见 §8)。
|
||||
|
||||
---
|
||||
|
||||
## 8. 重点 6 —— 测试质量
|
||||
|
||||
新测试(14 项,全部新增,未删/未跳过既有断言):
|
||||
|
||||
store 层(`internal/store/role_policy_test.go`):
|
||||
|
||||
- `TestPatchRolePermissionsBatchEnablesAndDisablesAcrossRoles`(happy,双 role 同步切换)
|
||||
- `TestPatchRolePermissionsBatchAtomicityRejectsWholeBatch`(一坏拒整批 + 验证两 role
|
||||
维持原样 + epoch 不前进)
|
||||
- `TestPatchRolePermissionsBatchStaleVersion`(过期 version 拒整批 + role 维持原样)
|
||||
- `TestPatchRolePermissionsBatchRejectsLimitAndIdempotency`(20 上限、空批、同 key 重放返回
|
||||
同一 receipt/epoch)
|
||||
- `TestPreviewRolePermissionsMatchesCommitResult`(预览允许/拒绝与 commit 一致)
|
||||
- `TestPreviewRolePermissionsRejectsInvalidBatchItem`(非法动作仅在 item 级别报错,不写盘)
|
||||
- `TestAssignUserRolesBatchRebindsInOneTransaction`(两人 rebind 在一个事务中,epoch+1 +
|
||||
同 key 重放 epoch 不变)
|
||||
- `TestAssignUserRolesBatchRejectsWholeBatch`(一个用户 state_version 过时 → 整批回滚
|
||||
+ 两角色 binding_id 与 state_version 未被改写)
|
||||
- `TestPreviewUserRoleAssignmentsJudgesEachItem`(三种 case:成功、state_version 过时、
|
||||
用户不存在;预览不写盘不前进 epoch)
|
||||
- `TestListRoleMembersForActorRequiresGrantsAndPaginates`(admin 看见、ordinary 被拒)
|
||||
- `TestActiveRolesForUsersReturnsActiveBindings`(仅返回 active binding;结束后不再见)
|
||||
- `TestCreateRoleCopyAndAssignCreatesRoleAndBindsInOneTransaction`(happy:新 role +
|
||||
绑定 + epoch+1)
|
||||
- `TestCreateRoleCopyAndAssignRejectsWithoutResidue`(覆盖坏 overrides / 坏 name / 坏
|
||||
state version 三种;检查 epoch 未进、新 role 未落盘)
|
||||
|
||||
server 层(`internal/server/role_authorization_test.go`):
|
||||
|
||||
- `TestRolePermissionPreviewRequiresPermissionViewOverHTTP`(403 with only `system.roles.view`;
|
||||
200 with admin;未知 role 单 item 报 `角色不存在或已停用`;空 roles 列表 400)
|
||||
- `TestBatchRolePermissionPatchOverHTTP`(403 with `roles.view+roles.manage`;200 提交;
|
||||
过时 version → 409;非法变更(含 `system.users.view`)→ 400)
|
||||
- `TestBatchRoleAssignmentPreviewAndCommitOverHTTP`(happy 路径:preview + commit)
|
||||
- `TestCopyAssignRoleOverHTTP`(happy 路径:源 role 启用 → copy → 用户绑定)
|
||||
- `TestRoleMembersFilterBeforePagingAndBindCursor`(admin 列出 + filter `query=alice&mode=exact`
|
||||
仅留 alice)
|
||||
- `TestAccountSearchIncludesRoleProjection`(search 结果中含 role 投影含 roleID)
|
||||
|
||||
测试覆盖到的关键断言:
|
||||
|
||||
- 每个写 API 都验证了 "atomic"(整批失败回滚写零)
|
||||
- `replayReceipt` 的 "同 key + 同 hash → 同 receipt/epoch" 显式断言
|
||||
- `PatchRolePermissionsBatch` 验证过期 role_version 走 `ErrInvalidRolePolicy` 并(按 HTTP
|
||||
层)映射到 409
|
||||
- 端口的 `TestRolePermissionsNormalizeLegacyRowsToProjectAccess`、`TestReplaceRolePermissionsAcceptsOnlyProjectAccessBoolean`、
|
||||
`TestRolePolicyRejectsNonProjectAccessRowsAtomically`、`TestCapabilityAssociationRejectsStaleVersionAndMissingRole`、
|
||||
`TestCapabilityAssociationImportRejectsLinkedConflict`、`TestLinkedCapabilitiesSystemGrantWithoutRoleClosesLoop`、
|
||||
`TestRolePolicyProjectAccessFailsClosedForDisabledRoleOrAccount`、`TestRolePolicyIdempotencyKeyCannotChangeRequest`、
|
||||
`TestImportRoleAuthorityPreservesStableIDsAndBindingsAtomically` —— 全部
|
||||
**未改写、未删除、未弱化**。
|
||||
|
||||
测试未被覆盖到的几个点(详见 §10 非阻塞建议)。
|
||||
|
||||
---
|
||||
|
||||
## 9. 阻塞项
|
||||
|
||||
无。
|
||||
|
||||
---
|
||||
|
||||
## 10. 非阻塞建议(advice,不阻断合入)
|
||||
|
||||
按"安全优先 → 可靠性 → 测试"排列:
|
||||
|
||||
1. **[安全] `assignUserRolesBatch` 缺少"非 admin 调用被 403"的 HTTP 层负测**。
|
||||
`TestBatchRolePermissionPatchOverHTTP` 与 `TestRolePermissionPreviewRequiresPermissionViewOverHTTP`
|
||||
已经验证了"持有弱权限的 actor 拿不到 403"。`assignUserRolesBatch` 与 `previewUserRoleAssignments`
|
||||
仅在 happy 路径上测过(`TestBatchRoleAssignmentPreviewAndCommitOverHTTP`)。
|
||||
建议补一条"持有 `system.roles.view` 但无 `system.roles.assign` 的 actor 调用
|
||||
`/api/admin/users/role-assignments` 收到 403"的测试。预期实现确实会拒绝
|
||||
(`assignUserRolesBatch` 起手 `requireSystemAction("system.roles.assign")`),但缺少语义钉。
|
||||
|
||||
2. **[安全] `copyAssignRole` 缺少 HTTP 层负测(任一闸不足被拒)**。
|
||||
当前只有 happy path。建议补:
|
||||
- 仅 `system.roles.manage` + `system.permissions.manage`,缺 `system.roles.assign` → 403;
|
||||
- 仅 `system.permissions.manage` + `system.roles.assign`,缺 `system.roles.manage` → 403;
|
||||
- 仅 `system.roles.manage` + `system.roles.assign`,缺 `system.permissions.manage` → 403。
|
||||
三个闸同时持有的语义在生产代码中已写明,但测试未钉死。
|
||||
|
||||
3. **[安全/一致性] `assignUserRolesBatch` 缺 user-missing/role-missing/role-disabled 的负测**。
|
||||
`TestAssignUserRolesBatchRejectsWholeBatch` 仅覆盖 stale-state-version 一种失败模式。
|
||||
store 层 `assignRolesTx` 通过 `roleAssignmentUserMissing` / `roleAssignmentUserDisabled` /
|
||||
`roleAssignmentRoleInvalid` 区分三种原因(preview 路径已经分别测过这三种原因,
|
||||
`TestPreviewUserRoleAssignmentsJudgesEachItem`),但 commit 路径没有显式锁定
|
||||
"bad userId / disabled user / disabled role" 都会回滚。
|
||||
|
||||
4. **[可靠性] PostgreSQL 下潜在的锁顺序风险**。
|
||||
`assignRolesTx` / `CreateRoleCopyAndAssign` 内部对每个 item 调用
|
||||
`s.roleTx(tx, item.RoleID, true)`,锁定顺序由 items 切片顺序决定。
|
||||
与 `policyWriteBatch` 内部 `lockAccountUsers` 的字典序 owner 锁不同,role-row 锁顺序
|
||||
没有全局规范化。当两个并发 batch 提交 items 涉及交叠 role 集合但顺序相反时,
|
||||
可能出现 PG 层 `40P01 deadlock_detected`,客户端表现为 500(因为 writeStoreErr 对
|
||||
deadlock 没特殊映射)。SQLite 不受影响。
|
||||
建议(任一):
|
||||
(a) 批前按 `item.RoleID` 排序后再 `roleTx`,让所有 batch 走同一序;
|
||||
(b) 在 writeStoreErr 中对 deadlock 检测加一条 `502/503` 提示;
|
||||
(c) 把 SQL 锁与 `policyWriteBatch` 的 owner 锁合并成同一把 `pg_advisory_xact_lock`
|
||||
key。在评估其工作量后再决定。
|
||||
当前批大小(≤100)下并发风险不大,但应在 batch 大小被放大或 PG 高并发场景重新审视。
|
||||
|
||||
5. **[测试] `ListRoleMembersForActor` 缺跨越多 role 测试**。
|
||||
`TestListRoleMembersForActorRequiresGrantsAndPaginates` 只在单 role + active-vs-inactive
|
||||
验证成员计数;没有测 "两个 role,actor 同时查询 role-A,结果只包含 role-A 成员"。
|
||||
|
||||
6. **[风格] handler 层 `listRolePermissionPreview` 的 400 路径**。
|
||||
当 `roles` 为空时,handler 直接 400;store 里 `len(items) == 0 || len(items) > maxRolePermissionPreviewItem`
|
||||
也会返回 `ErrInvalidRolePolicy`。两处都在做相同的合法性检查。这是
|
||||
#228/#234 现有 pattern 的一致沿用,但建议未来统一只用一处。
|
||||
|
||||
7. **[风格] `previewUserRoleAssignments` 与 `listRoleMembers` 重复了 cursor 计算**。
|
||||
两处都用 SHA256 over `{Kind, Actor, RoleID, Query, Mode, Status, Revision}` 派生
|
||||
version;HMAC 用 `accountSearchKey` 派 cursor token。建议抽 `roleMembersVersion(actor, roleID, query, mode, status, revision)` helper
|
||||
到 `account_search.go`,避免后续在 `copyAssignRole`、`batchPatchRolePermissions` 等
|
||||
写入响应里再次手写。
|
||||
|
||||
---
|
||||
|
||||
## 11. 依据与实际只读命令结果(evidence)
|
||||
|
||||
| 命令 | 输出摘要 / 关键引用 |
|
||||
|---|---|
|
||||
| `git status` → `HEAD detached at f10fb2e; nothing to commit, working tree clean` | 工作区干净;fix/iam-perm-ux-port HEAD = f10fb2e |
|
||||
| `git log --oneline 01f4c47a2b5..HEAD` | `f10fb2e` / `76abebe` / `2d1e5f5` —— 与 packet 一致(2 transport + task-start) |
|
||||
| `git diff 01f4c47a2b5..HEAD --stat` | 8 文件 / +2237 行(packet 标 "+2184" 视为合理近似口径差异) |
|
||||
| `git diff ... -- internal/server/server.go` | 新增 6 条 `mux.HandleFunc` 注册(行 323-328),全部位于既有 `/api/admin/roles` 与 `/api/admin/users` 区间内,无路由表破坏 |
|
||||
| `git diff ... -- internal/store/role_policy.go`(约 +1141 行) | 新增 `RolePermissionPreviewInput/Patch/Batch`/`UserRoleAssignmentInput/Batch/Preview`/`RoleCopy*/RoleMember/UserRoleSummary`/`policyWriteBatch`/`PreviewRolePermissions`/`PatchRolePermissionsBatch`/`CreateRoleCopyAndAssign`/`AssignUserRolesBatch`/`PreviewUserRoleAssignments`/`ListRoleMembersForActor`/`ActiveRolesForUsers` —— 全部以 `policyWriteBatch` 或显式 tx/actor 校验为入口 |
|
||||
| `git diff ... -- internal/server/roles.go` | 6 个 handler(`listRolePermissionPreview`/`batchPatchRolePermissions`/`decodeRoleAssignments`/`assignUserRolesBatch`/`previewUserRoleAssignments`/`copyAssignRole`) + `systemActionAllowed` 静默判定辅助 |
|
||||
| `git diff ... -- internal/server/account_search.go` | `searchAdminUsers` 追加 `system.roles.view` 守门 role 投影(`systemActionAllowed` 静默跳过路径);新增 `listRoleMembers` handler(角色过滤 + 绑定 cursor)|
|
||||
| `git diff ... -- internal/store/role_policy_test.go` | +494 行:14 新测试覆盖 happy / 原子回滚 / 过期版本 / 幂等重放 / 上限 / preview 一次 item level / user-missing 等;既有测试零修改 |
|
||||
| `git diff ... -- internal/server/role_authorization_test.go` | +226 行:6 新 HTTP 测试,含 `403 with weaker grant`、`409 on stale version`、`400 on invalid action`;既有测试零修改 |
|
||||
| `git diff ... -- internal/authz/catalog/catalog_test.go` | +16 行:仅加 `TestCatalogLinkedActionsStayProjectScopedAndUnprotected`,未改既有 |
|
||||
| `grep requireSuperAdminTargetAccess internal/store/*.go` | 定义于 `super_admin.go:36`,被既有 `AssignUserRole`/`ReplaceSystemGrants` 与本次新增 `assignRolesTx`(*Batch 路径)/ `CreateRoleCopyAndAssign` 内的 `assignRolesTx` 调用 |
|
||||
| `grep roleGrantsAccountAdministrationTx internal/store/role_policy.go` | 定义 line 743;被既有 `AssignUserRole`/`replaceSystemGrantsTx` 与新增 `assignRolesTx` 调用 |
|
||||
| `grep requireRoleAuthorityWritableTx internal/store/role_policy.go` | 定义 line 245(`FOR UPDATE` of `role_authority_import_state`);被既有 `CreateRole`/`PatchRole`/`ReplaceRolePermissions`/`AssignUserRole` 与新增 `PatchRolePermissionsBatch`/`AssignUserRolesBatch`/`CreateRoleCopyAndAssign` 调用 |
|
||||
| `grep policyWriteBatch internal/store/role_policy.go` | 定义 line 1620;与既有 `policyWrite`(line 181)同形态(owner 字典序锁 + tx + epoch + 闭包 + commit)|
|
||||
| `grep mux.HandleFunc internal/server/server.go` | 路由按既有 admin/role/user 区段插入 |
|
||||
| `grep saveReceipt internal/store/role_policy.go` | 三个 batch 入口都调用 `saveReceipt`;store 没有 tx.Commit() 之外的提交途径 |
|
||||
| `grep CREATE.*user_role_bindings internal/store/migrate.go` | line 1284 的 `CREATE UNIQUE INDEX uq_user_role_bindings_active ON user_role_bindings(user_id) WHERE active=TRUE` —— 强制单 active binding/user;`Update user_role_bindings SET active=0 ... WHERE active=1` 不会同时关错多行 |
|
||||
| 跳过 | `git submodule init/update`(packet 禁用)/ `git add/commit/push` / `git config` / `git stash` —— 全部未执行 |
|
||||
| 跳过 | `PYTHONDONTWRITEBYTECODE=1 python3 -m unittest tests.test_shell_pipe_hazard` —— packet 在 test 列表中给出该命令,但本仓库不存在 `tests/` 目录也不存在 `test_shell_pipe_hazard` 测试。该命令不可执行,与端口内容无关(疑为模板残留),已在"未验证"列出 |
|
||||
|
||||
---
|
||||
|
||||
## 12. 未验证(structural limits of detached-HEAD review-only session)
|
||||
|
||||
按 packet 第 3 项要求"未验证的结论写明未验证"。下列条目不是结论本身的缺失,而是本会话
|
||||
无法跑通验证路径:
|
||||
|
||||
1. **未跑 `go test ./...`**:deps 子模块 gitlink 在本 detached-HEAD 工作区中尚未初始化
|
||||
(worktree 内 `deps/*/` 是空目录),且 packet 禁用 `git submodule init/update`,
|
||||
所以 `go build ./...` 也无法走通。所有"编译通过性"和"测试通过性"的结论均以**静态
|
||||
diff/源码对比**为依据,不以运行为依据。评审者**未亲眼见到测试 PASS 输出**。
|
||||
|
||||
2. **未对源 feature 分支作交叉比对**:packet 注明 `feature/iam-perm-ux-backend` "唯一副本
|
||||
已备份",本工作区没有该分支 ref 也无任何 transport 元数据用于追溯。本评审**仅按 diff
|
||||
与 main 既有语义做正向校验**,不与原 feature 分支逐行比对。
|
||||
|
||||
3. **PG dialect 下不跑 `go test ./... -tags pg`**:同上。
|
||||
|
||||
4. **`tests.test_shell_pipe_hazard` 不可执行**:模板命令,本仓无对应目录与模块,已注明。
|
||||
|
||||
5. **未跑 `make precheck`**:packet 在 Dev 仓推进;本仓为子仓,"独立开发"路径见
|
||||
PROJECT.md 但 precheck 归属于 Dev 仓,本审查会话不对其做运行。
|
||||
|
||||
---
|
||||
|
||||
## 13. 决定
|
||||
|
||||
- `terminal_status`: `completed`(独立审查产出完毕,未改动任何非报告文件;非"BLOCKED")
|
||||
- 审查者 ID:iam-perm-ux-port-independent-review
|
||||
- 提交建议:可合入 main(前提是 §10 中 (1)(2)(3) 三条负测在合入前补齐或与跟进任务
|
||||
挂钩 —— 推荐补齐后再合,以避免 main 出现"缺少 admin-grant 校准测试"的退化窗口)
|
||||
|
||||
— end of review —
|
||||
Reference in New Issue
Block a user