Repository navigation
fix(security): Roles select-all filter and 3.9 UX polish - #1116
Conversation
Select all and Deselect now act on the filtered category, and the category summary counts only what is on screen. A bUnit test filters to identity/roles and asserts Applications and Users stay unchecked. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
Apply the 3.9-safe Roles UX polish: drop lectures and leftover loader copy, hide zero-state counters, collapse covered-by notes, simplify search and delete confirmation, and keep Select all as a Deselect toggle for the visible set. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Deselect now removes the same selectable set that Select all adds, so a wildcard-covered exact grant is not dropped when the visible category is cleared. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
sfmskywalker
left a comment
There was a problem hiding this comment.
Elsa 3 Code Review: APPROVE + HIGH @ c94032a
Code Review, Round 1/4
Scope: release/3.9.0 @ 2842adf0 ← c94032a2 (3 commits, 8 files, all under src/modules/Elsa.Studio.Security*). Fixes #1114 and #1115. Checked against the 3.9 Roles UX review and the #1114/#1115 issue bodies.
Verdict: No blockers. The Select-all fix is correct and tested. Every polish change maps to an in-scope 3.9 item. None of the 3.10 items (15, 16, 17, 20) are in the PR. Apart from the bulk-action scope, permission semantics are unchanged.
Scope map (#1115)
| # | Item | Status |
|---|---|---|
| 1 | Exact-permissions lecture cut | ✅ |
| 2 | Core-generates-ID helper + read-only ID caption cut | ✅ |
| 3 | Count N role(s), hidden when the filtered list is empty; footer, suffixes and check icon gone (desktop and mobile) |
✅ |
| 4 | Header counter hidden at zero, otherwise N selected |
✅ (see N1) |
| 5 | Advanced lecture cut; empty state No wildcards. |
✅ |
| 6 | Also covered by … once per resource row; covered-only verbs keep the #1075 chip |
✅ |
| 8 | Search and permission filter: no label, placeholder Name, ID, or permission (aria-label kept) |
✅ |
| 9 | Add advanced grant → Add; safe-delete Close → Cancel |
✅ |
| 10 | Edit title is {name}; ID stays under it |
✅ |
| 11 | Single Wildcard chip (Broad access and the GrantType labels removed); 1 resource today |
✅ Advanced grants can only be wildcards (TryValidateAdvancedGrant / IsRecognized), so the single label is accurate |
| 12 | Safe delete: banner gone, 0 · 0 line hidden only when both counts are zero, token sentence shortened, Delete {name} kept |
✅ |
| 13 | ID column and mobile ID hidden | ✅ |
| 14 | Select all ↔ Deselect on the visible set | ✅ |
| 18 | Delete text, Cancel outlined, Save filled primary | ✅ |
| 19 | Filtered-empty card has Clear search | ✅ |
3.10 items are out:
- 15: the tabs are still
Exact permissions/Advanced grants. - 16:
RolePermissionPreviewis untouched. - 17: there is no Remove duplicates.
- 20: in
DeleteRoleDialogonly theSafeConfirmationbranch and itsCloseLabelchanged. The blocked, remediation, conflict, incomplete and last-default states are unchanged, and ConfigurationBlocked/Incomplete still read Close.
Nothing outside the spec was added. The category summary and auto-expand now count only visible resources, which #1114's fix text asks for.
Permission semantics
- The save payload is unchanged. It is still
NormalizePermissions(_direct ∪ _advanced ∪ _unresolved)intoCreateRoleRequest/UpdateRoleRequest. LoadStoredGrantsis unchanged, and so are wildcard parsing, validation and matching (RolePermissionAuthoringis untouched). No API client or contract changes.- Select all adds the same set as before (
SelectableGrants, which leaves out exact grants that a wildcard already covers). It is now limited to the visible resources. That is the #1114 fix. - Deselect (item 14) removes exactly that set. Wildcards, unresolved grants, hidden resources and exact grants covered by a wildcard are all kept.
Revert probes
| Mutation | Result |
|---|---|
| Select all widened back to the whole category | SelectAll_WithAnActiveFilter_GrantsOnlyTheVisibleResources fails |
| Category summary widened back to the whole category | SelectAll_WithAnActiveFilter_GrantsOnlyTheVisibleResources fails (on 3 direct · 3 permissions) |
c94032a2 reverted (Deselect uses all concrete grants again) |
Deselect_PreservesExactGrantsThatAreAlsoCoveredByAWildcard fails |
| Deselect widened to the whole category | No PR test fails; only an ad-hoc probe catches it (see N3) |
Edge cases
I ran these as scratch bUnit tests (not committed). All pass on the head:
- Empty filter: Select all grants all 9 verbs and shows
9 selectedand Deselect; Deselect clears all 9. - Filtered Deselect with hidden grants: stored
applications:viewplusroles:*exact grants. Deselect underidentity/rolessaves["identity/applications:view"]. - Wildcard + filtered Select all:
identity/*:viewstored. Savesidentity/*:view,identity/roles:createandidentity/roles:update.roles:viewis not duplicated, and nothing from Applications or Users is added. - Deselect with a covered exact grant: saves
identity/*:viewandidentity/roles:view. - Everything covered (
identity/*:*): the button reads Select all, the click does nothing, and the save is["identity/*:*"]. - Partially selected visible set: the button reads Select all and completes only the visible resource. A hidden
users:viewis kept.
Builds and tests
Elsa.Studio.Host.ServerRelease (net8/9/10): build succeeded, 0 errors.Elsa.Studio.Host.WasmRelease (net8/9/10): build succeeded, 0 errors.Elsa.Studio.Security.Tests: 132/132 passed (139/139 with the scratch edge-case tests).Elsa.Studio.Administration.Tests, which references the Security module: 63/63 passed.git diff --checkis clean.
Security
There are no new inputs, no markup injection, and no change to authorization. Access.CanDelete / IsReadOnly still gate every action, and the bulk actions return early when IsReadOnly. The safe-delete copy change follows item 12. Greptile's "permanent" concern is withdrawn on the thread.
Maintainability
Good. SelectableGrants is the single source for Select all, Deselect and the fully-selected check, so the toggle cannot drift from what it changes. AlsoCoveredWildcards is small and readable. The tests are focused and use a shared IdentityCatalog() fixture.
Non-blocking
- N1.
N selectedcounts a wildcard twice.SelectedCount = direct + covered + advanced, so a role that stores onlyidentity/roles:*shows 4 selected while no checkbox is checked. Item 4 asks forN selectedbut does not define N. ConsiderDirectCount + _advanced.Count(stored grants) orDirectCount + CoveredCount(effective permissions) in a follow-up. The Greptile P2 thread was resolved without a change. - N2. The Roles browser spec is stale.
tests/browser/RoleManagement/role-management.spec.tsstill targetsFilter permissions,Add advanced grant,Search roles by name, ID, or permission,all loaded|matching searchand0 roles · matching search. It was already stale at the base (Select all exact), and PR CI does not run it. Refresh it in a follow-up before the next real-host acceptance pass. - N3. Test gap. No committed test proves that a filtered Deselect keeps grants on hidden resources (the widen-Deselect mutation passes the suite). A test like the "filtered Deselect with hidden grants" case above would cover it.
- N4. Select all can do nothing. When every visible verb is covered by a wildcard, Select all is shown but does nothing. This was already the case before the PR.
- N5. Dead CSS. The
.role-idrule inRoles.razorno longer has any users.
CI, Greptile and threads on c94032a2
- Build and test, GitGuardian, CLA and Greptile Review are green. CodeRabbit is skipped for this base. Merge state is CLEAN.
- Greptile: 5/5 at
c94032a2(advisory only). - Threads: 4/4 resolved. The P1 Deselect thread was fixed in
c94032a2. The two P2s on list ID and delete copy were withdrawn by Greptile as spec-mandated. The P2 on the selected count was resolved without a change (N1).
Gate: APPROVE + HIGH and green CI on c94032a2. Ready to merge.
Purpose
Fixes the Roles editor Select-all filter bug and applies the 3.9-safe Roles UX polish.
Fixes #1114
Fixes #1115
Scope
Description
Problem
Select all in the Roles create/edit editor ignored the permission filter and granted every permission in the category. The Roles list and editor also shipped leftover loader copy and lecture text that Sipke asked to strip for 3.9.
Solution
Commits on
cursor/roles-select-all-filter-and-ux-72bf(c94032a2):fix(security): select only visible role permissions— Select all / Deselect act only on the resources currently visible under the active filter. The category summary counts only what is on screen.fix(security): trim Roles list and editor copy— 3.9-safe polish from the Roles UX review (items 1–6, 8–14, 18–19).fix(security): keep covered exact grants on Deselect— Greptile P1: Deselect uses the same selectable set as Select all, so a wildcard-covered exact grant is not dropped.Both Server and WASM hosts use the same
Elsa.Studio.Securitymodule (AddSecurityModulein each hostProgram.cs).Server host: Release build succeeded, 0 errors (net8/net9/net10).
WASM host: Release build succeeded, 0 errors (net8/net9/net10).
3.9-safe checklist
N role(s); hide it when empty; drop the footer, suffixes, and check icon. Same on mobile.N selected.No wildcards.Also covered by …once per resource row. Covered-only verbs still show the #1075 chip.Name, ID, or permission.Add advanced grant→Add. Safe-delete Close →Cancel.Create first rolestays.{name}. ID stays under the title.Wildcardchip.1 resource today/N resources today.0 · 0line; token sentence shortened. Delete{name}kept. Other delete states unchanged.Out of scope (3.10): 15 tab renames, 16 permission-preview redesign, 17 Remove duplicates, 20 blocked/remediation delete copy.
Roles copy is hardcoded English in
Elsa.Studio.Security(no.resx/ localizer keys). Localization tests were not affected.Verification
Local:
Elsa.Studio.Host.ServerRelease: Build succeeded, 0 errors.Elsa.Studio.Host.WasmRelease: Build succeeded, 0 errors.dotnet test Elsa.Studio.sln --configuration Release --no-build(pre-review-fix): 1893 passed, 0 failed.SelectAll_WithAnActiveFilter_GrantsOnlyTheVisibleResourcesSelectAll_TogglesToDeselectForTheVisibleSetDeselect_PreservesExactGrantsThatAreAlsoCoveredByAWildcardPermissionCounter_HidesAtZeroAndShowsSelectedCountExactGrantCoveredByAWildcard_ShowsOneNotePerResourceAdvancedGrants_UseASingleWildcardChipAndSingularResourceCopyRender_WhenSearchMatchesNothing_HidesTheCountAndClearSearchRestoresTheListRender_WhenImpactIsSafe_ShowsSafeConfirmationAndDeleteActionReverting the #1114 filter-aware Select all makes
SelectAll_WithAnActiveFilter_GrantsOnlyTheVisibleResourcesfail.Live browser pass was not run here: this environment has no Elsa Core identity backend.
Checklist