diff --git a/internal/server/role_authorization_test.go b/internal/server/role_authorization_test.go index 356f713..55cfb61 100644 --- a/internal/server/role_authorization_test.go +++ b/internal/server/role_authorization_test.go @@ -480,47 +480,99 @@ func TestLegacyRoleRowsDoNotAuthorizeSystemActionsOrAccess(t *testing.T) { func TestRolePermissionPreviewRequiresPermissionViewOverHTTP(t *testing.T) { srv, st := newAuthTestServer(t) h := srv.Handler() - admin := bootAdmin(t, h, st) + admin := bootSuperAdmin(t, h, st) + // Preview is a permission-management read: system.permissions.view is + // required, and holding system.roles.view alone must not unlock it. + roleViewer := grantSystemActionsFixture(t, h, st, admin, "preview-role-viewer", "system.roles.view") roleRec := do(h, "POST", "/api/admin/roles", `{"name":"预览职位"}`, admin) if roleRec.Code != http.StatusOK { t.Fatalf("create role: %d %s", roleRec.Code, roleRec.Body.String()) } role := decodeRoleHTTP(t, roleRec) body := `{"roles":[{"roleId":"` + role.RoleID + `","expectedRoleVersion":1,"changes":[{"actionId":"project.create","allowed":false,"capAllowed":true}]}]}` + if rec := bearerRequest(h, "POST", "/api/admin/roles/permissions/preview", body, roleViewer); rec.Code != http.StatusForbidden { + t.Fatalf("role viewer previewed permissions: %d %s", rec.Code, rec.Body.String()) + } if rec := do(h, "POST", "/api/admin/roles/permissions/preview", body, admin); rec.Code != http.StatusOK { t.Fatalf("preview denied: %d %s", rec.Code, rec.Body.String()) } else if !strings.Contains(rec.Body.String(), role.RoleID) { t.Fatalf("preview body: %s", rec.Body.String()) } - if rec := do(h, "POST", "/api/admin/roles/permissions/preview", - `{"roles":[{"roleId":"unknown","expectedRoleVersion":1,"changes":[]}]}`, admin); rec.Code != http.StatusBadRequest && - rec.Code != http.StatusInternalServerError { - t.Fatalf("empty-changes preview: %d %s", rec.Code, rec.Body.String()) + // Preview judges every item separately, so an unknown role is reported + // inside that item's preview instead of failing the whole read. + var unknown struct { + Previews []struct { + RoleID string `json:"roleId"` + VersionMatches bool `json:"versionMatches"` + Error string `json:"error"` + ErrorActionID string `json:"errorActionId"` + } `json:"previews"` + } + unknownRec := do(h, "POST", "/api/admin/roles/permissions/preview", + `{"roles":[{"roleId":"unknown","expectedRoleVersion":1,"changes":[]}]}`, admin) + if unknownRec.Code != http.StatusOK { + t.Fatalf("unknown-role preview: %d %s", unknownRec.Code, unknownRec.Body.String()) + } + if err := json.Unmarshal(unknownRec.Body.Bytes(), &unknown); err != nil || len(unknown.Previews) != 1 { + t.Fatalf("decode unknown preview: %v body=%s", err, unknownRec.Body.String()) + } + // store's roleAssignmentRoleInvalid reason ("角色不存在或已停用"): + // preview reports an unusable role per item instead of failing the read. + const roleInvalidReason = "角色不存在或已停用" + if unknown.Previews[0].RoleID != "unknown" || unknown.Previews[0].VersionMatches || + unknown.Previews[0].Error != roleInvalidReason || unknown.Previews[0].ErrorActionID != "" { + t.Fatalf("unknown preview item: %+v", unknown.Previews[0]) + } + // An empty item list is still a malformed request. + if rec := do(h, "POST", "/api/admin/roles/permissions/preview", `{"roles":[]}`, admin); rec.Code != http.StatusBadRequest { + t.Fatalf("empty preview: %d %s", rec.Code, rec.Body.String()) } } func TestBatchRolePermissionPatchOverHTTP(t *testing.T) { srv, st := newAuthTestServer(t) h := srv.Handler() - admin := bootAdmin(t, h, st) - first := createRoleHTTP(t, h, st, admin, "批量职位一") - second := createRoleHTTP(t, h, st, admin, "批量职位二") + admin := bootSuperAdmin(t, h, st) + // Batch commit is a permission-management write: system.permissions.manage + // is required, roles.view/manage alone is not enough. + roleManager := grantSystemActionsFixture(t, h, st, admin, "batch-role-manager", "system.roles.view", "system.roles.manage") + first := createRoleHTTP(t, h, admin, "批量职位一") + second := createRoleHTTP(t, h, admin, "批量职位二") body := `{"idempotencyKey":"batch-1","roles":[` + `{"roleId":"` + first.RoleID + `","expectedRoleVersion":1,"changes":[{"actionId":"project.create","allowed":true,"capAllowed":true}]},` + `{"roleId":"` + second.RoleID + `","expectedRoleVersion":1,"changes":[{"actionId":"project.create","allowed":true,"capAllowed":true}]}` + `]}` + if rec := bearerRequest(h, "POST", "/api/admin/roles/permissions/batch", body, roleManager); rec.Code != http.StatusForbidden { + t.Fatalf("role manager patched permissions: %d %s", rec.Code, rec.Body.String()) + } if rec := do(h, "POST", "/api/admin/roles/permissions/batch", body, admin); rec.Code != http.StatusOK { t.Fatalf("batch denied: %d %s", rec.Code, rec.Body.String()) } else if !strings.Contains(rec.Body.String(), "receipts") { t.Fatalf("batch body: %s", rec.Body.String()) } - body = `{"idempotencyKey":"batch-bad","roles":[` + + // A stale expectedRoleVersion in any item rejects the whole batch with + // 409 Conflict (main's role-version class) and writes nothing at all. + stale := `{"idempotencyKey":"batch-stale","roles":[` + `{"roleId":"` + first.RoleID + `","expectedRoleVersion":2,"changes":[{"actionId":"project.create","allowed":false,"capAllowed":true}]},` + - `{"roleId":"` + second.RoleID + `","expectedRoleVersion":1,"changes":[{"actionId":"system.users.view","allowed":true,"capAllowed":true}]}` + + `{"roleId":"` + second.RoleID + `","expectedRoleVersion":1,"changes":[{"actionId":"project.create","allowed":false,"capAllowed":true}]}` + `]}` - if rec := do(h, "POST", "/api/admin/roles/permissions/batch", body, admin); rec.Code != http.StatusInternalServerError && - rec.Code != http.StatusBadRequest { - t.Fatalf("bad batch response: %d %s", rec.Code, rec.Body.String()) + if rec := do(h, "POST", "/api/admin/roles/permissions/batch", stale, admin); rec.Code != http.StatusConflict { + t.Fatalf("stale batch: %d %s", rec.Code, rec.Body.String()) + } + for _, role := range []roleHTTPView{first, second} { + rec := do(h, "GET", "/api/admin/roles/"+role.RoleID, "", admin) + view := decodeRoleProjectAccess(t, rec) + if view.RoleVersion != 2 || !view.ProjectAccess || len(view.Permissions) != 1 { + t.Fatalf("stale batch wrote %s: %+v", role.RoleID, view) + } + } + // A change outside project access is an invalid policy: 400 Bad Request. + invalid := `{"idempotencyKey":"batch-invalid","roles":[` + + `{"roleId":"` + first.RoleID + `","expectedRoleVersion":2,"changes":[{"actionId":"system.users.view","allowed":true,"capAllowed":true}]},` + + `{"roleId":"` + second.RoleID + `","expectedRoleVersion":2,"changes":[{"actionId":"project.create","allowed":false,"capAllowed":true}]}` + + `]}` + if rec := do(h, "POST", "/api/admin/roles/permissions/batch", invalid, admin); rec.Code != http.StatusBadRequest { + t.Fatalf("invalid batch: %d %s", rec.Code, rec.Body.String()) } } @@ -536,7 +588,7 @@ func TestBatchRoleAssignmentPreviewAndCommitOverHTTP(t *testing.T) { } alice, _ := st.GetAccountByOwner("alice") bob, _ := st.GetAccountByOwner("bob") - role := createRoleHTTP(t, h, st, admin, "职位预览") + role := createRoleHTTP(t, h, admin, "职位预览") preview := `{"assignments":[` + `{"userId":"` + alice.ID + `","roleId":"` + role.RoleID + `","expectedStateVersion":` + strconv.FormatInt(alice.StateVersion, 10) + `},` + `{"userId":"` + bob.ID + `","roleId":"` + role.RoleID + `","expectedStateVersion":` + strconv.FormatInt(bob.StateVersion, 10) + `}` + @@ -565,7 +617,7 @@ func TestCopyAssignRoleOverHTTP(t *testing.T) { t.Fatal(err) } alice, _ := st.GetAccountByOwner("alice") - source := createRoleHTTP(t, h, st, admin, "源职位") + source := createRoleHTTP(t, h, admin, "源职位") enable := do(h, "PUT", "/api/admin/roles/"+source.RoleID+"/permissions", `{"expectedRoleVersion":1,"idempotencyKey":"copy-source","changes":[{"actionId":"project.create","allowed":true,"capAllowed":true}]}`, admin) if enable.Code != http.StatusOK { @@ -591,7 +643,7 @@ func TestRoleMembersFilterBeforePagingAndBindCursor(t *testing.T) { } alice, _ := st.GetAccountByOwner("alice") bob, _ := st.GetAccountByOwner("bob") - role := createRoleHTTP(t, h, st, admin, "成员职位") + role := createRoleHTTP(t, h, admin, "成员职位") assign := func(userID string, version int64, key string) { t.Helper() rec := do(h, "PUT", "/api/admin/users/"+userID+"/role", @@ -621,7 +673,7 @@ func TestAccountSearchIncludesRoleProjection(t *testing.T) { t.Fatal(err) } alice, _ := st.GetAccountByOwner("alice") - role := createRoleHTTP(t, h, st, admin, "账户目录职位") + role := createRoleHTTP(t, h, admin, "账户目录职位") if rec := do(h, "PUT", "/api/admin/users/"+alice.ID+"/role", `{"roleId":"`+role.RoleID+`","expectedStateVersion":`+strconv.FormatInt(alice.StateVersion, 10)+`,"idempotencyKey":"search-assign"}`, admin); rec.Code != http.StatusOK { t.Fatalf("assign: %d %s", rec.Code, rec.Body.String()) @@ -633,25 +685,9 @@ func TestAccountSearchIncludesRoleProjection(t *testing.T) { } } -func decodeRoleHTTP(t *testing.T, rec *httptest.ResponseRecorder) struct { - RoleID string `json:"roleId"` - Name string `json:"name"` -} { - t.Helper() - var role struct { - RoleID string `json:"roleId"` - Name string `json:"name"` - } - if err := json.Unmarshal(rec.Body.Bytes(), &role); err != nil { - t.Fatalf("decode role: %v body=%s", err, rec.Body.String()) - } - return role -} - -func createRoleHTTP(t *testing.T, h http.Handler, st *store.Store, admin, name string) struct { - RoleID string `json:"roleId"` - Name string `json:"name"` -} { +// createRoleHTTP creates a role over HTTP and returns the decoded view. It +// reuses roles_test.go's decodeRoleHTTP so the package keeps one decoder. +func createRoleHTTP(t *testing.T, h http.Handler, admin, name string) roleHTTPView { t.Helper() rec := do(h, "POST", "/api/admin/roles", `{"name":"`+name+`"}`, admin) if rec.Code != http.StatusOK { diff --git a/internal/store/role_policy.go b/internal/store/role_policy.go index b9861bf..392b3cf 100644 --- a/internal/store/role_policy.go +++ b/internal/store/role_policy.go @@ -2387,11 +2387,12 @@ func (s *Store) CreateRoleCopyAndAssign(actor string, source RoleCopySource, spe VALUES(?,?,?,?,?,?,?,?,?,?)`), roleID, name, normalizeRoleName(name), description, true, 1, actorAccount.ID, actorAccount.ID, now, now); err != nil { return err } - writes := make([]RolePermissionChange, 0, len(merged)) - for _, row := range merged { - writes = append(writes, row) - } - if err := s.upsertRolePermissionsTx(tx, actorAccount.ID, roleID, map[string]RolePermissionChange{}, writes, now); err != nil { + // The role row was inserted in this same transaction, so the copy has + // no prior permission state: current is nil and every entry of the + // merged target is a fresh grant. That is what the original branch + // passed too (nil current against a freshly written role); faking a + // non-empty prior state here would misreport the helper's delta. + if err := s.upsertRolePermissionsTx(tx, actorAccount.ID, roleID, nil, merged, now); err != nil { return err } for i := range items { @@ -2415,8 +2416,13 @@ func (s *Store) CreateRoleCopyAndAssign(actor string, source RoleCopySource, spe if err != nil { return err } + // ProjectAccess is the normalized projection every other role + // read path fills in (RoleAdmin, ListRoles); the copy receipt must + // report the same value or a copied project.create role reads as + // access-less. out = RoleCopyAssignResult{ReceiptID: newID("receipt"), AuthorityEpoch: epoch + 1, - Role: RoleAdminView{RoleRecord: role, BindingCount: len(bindings), Permissions: permissions}, + Role: RoleAdminView{RoleRecord: role, BindingCount: len(bindings), + ProjectAccess: projectAccessOf(permissions), Permissions: permissions}, Bindings: bindings} return s.saveReceipt(tx, actorAccount.ID, key, "role.copy.assign", requestHash, out) }) diff --git a/internal/store/role_policy_test.go b/internal/store/role_policy_test.go index 10e0944..5c50eda 100644 --- a/internal/store/role_policy_test.go +++ b/internal/store/role_policy_test.go @@ -1028,22 +1028,29 @@ func TestAssignUserRolesBatchRejectsWholeBatch(t *testing.T) { roleID string previous string }{ - {alice.ID, initialOne, alice.ID, roleOne.RoleID}, - {bob.ID, initialTwo, bob.ID, roleTwo.RoleID}, + {alice.ID, initialOne, roleOne.RoleID, alice.ID}, + {bob.ID, initialTwo, roleTwo.RoleID, bob.ID}, } { view, err := st.RoleForUser(check.userID) if err != nil || view.Binding == nil || view.Role == nil || view.Role.RoleID != check.roleID { t.Fatalf("user %s rebound across rollback: %+v err=%v", check.userID, view, err) } + // The rolled-back batch also leaves the earlier binding record + // itself untouched: same binding id and user state version. + if view.Binding.BindingID != check.binding.BindingID || view.Binding.UserStateVersion != check.binding.UserStateVersion { + t.Fatalf("user %s binding rewritten across rollback: got %+v want %+v", check.userID, view.Binding, check.binding) + } _ = check.previous } } // TestPreviewUserRoleAssignmentsJudgesEachItem returns one preview per // input; mismatches carry reason text without writing rows or receipts. +// A batch names each user at most once (validateRoleAssignmentItems), so +// the three judgements are asked of three distinct users. func TestPreviewUserRoleAssignmentsJudgesEachItem(t *testing.T) { st := openTestStore(t) - _, alice, _ := rolePolicyAccounts(t, st) + _, alice, bob := rolePolicyAccounts(t, st) role, err := st.CreateRole("admin", RoleCreate{Name: "预览分配"}) if err != nil { t.Fatal(err) @@ -1052,10 +1059,11 @@ func TestPreviewUserRoleAssignmentsJudgesEachItem(t *testing.T) { t.Fatal(err) } alice, _ = st.GetAccount(alice.ID) + epochBefore, _ := st.AuthorityEpoch() previews, err := st.PreviewUserRoleAssignments("admin", []UserRoleAssignmentInput{ {UserID: alice.ID, RoleID: role.RoleID, ExpectedStateVersion: alice.StateVersion}, - {UserID: alice.ID, RoleID: role.RoleID, ExpectedStateVersion: alice.StateVersion + 99}, + {UserID: bob.ID, RoleID: role.RoleID, ExpectedStateVersion: bob.StateVersion + 99}, {UserID: "user_missing", RoleID: role.RoleID, ExpectedStateVersion: 1}, }) if err != nil { @@ -1073,6 +1081,9 @@ func TestPreviewUserRoleAssignmentsJudgesEachItem(t *testing.T) { if previews[2].Error != roleAssignmentUserMissing { t.Fatalf("missing user: %+v", previews[2]) } + if epoch, _ := st.AuthorityEpoch(); epoch != epochBefore { + t.Fatalf("preview advanced epoch: before=%d after=%d", epochBefore, epoch) + } // The commit's first item still passes; rebinding still works. if _, err := st.AssignUserRolesBatch("admin", []UserRoleAssignmentInput{{UserID: alice.ID, RoleID: role.RoleID, ExpectedStateVersion: alice.StateVersion}},