Repository navigation
fix(studio): forward-port Roles select-all filter fix and 3.9 polish (studio#1116) - #8592
Conversation
Port elsa-studio #1116 into the consolidated Studio tree: Select all and Deselect act only on resources visible under the active filter, Deselect keeps wildcard-covered exact grants, and the 3.9-safe Roles list/editor copy is trimmed. F4 keep-alive tabs, in-place advanced grant editing, and permission-refresh boundaries are unchanged. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
📒 Files selected for processing (8)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comments Outside DiffThese findings could not be posted inline.
|
sfmskywalker
left a comment
sfmskywalker
left a comment
There was a problem hiding this comment.
Elsa 3 Code Review: APPROVE + HIGH @ 4083e9e
Code Review, Round 1/4
Scope: main @ b104fd2d (F4: #8586 + #8587 merged) ← 4083e9e7. One commit, 8 files under src/studio/modules/Elsa.Studio.Security*. Forward-port of studio#1116, whose merge 0ad18748 has the same tree as the approved head c94032a2.
Verdict: No blockers. The port matches #1116 line for line. All 3.9 tests are ported. No 3.10 item is included. The F4 behaviour is unchanged. Permission semantics change only in how far Select all/Deselect reach.
Fidelity to #1116
- I compared the changed lines of
2842adf0..c94032a2(studio) withb104fd2d..4083e9e7(core) file by file, after mapping the paths. All 5 production files are identical:RoleEditorSurface.razor,RoleAdvancedGrantCard.razor,DeleteRoleDialog.razor(.cs)andRoles.razor. So areRolesPageTests.csandDeleteRoleDialogTests.cs. RoleEditorSurfaceTests.cshas one extra line:Assert.DoesNotContain("Direct grant", …)in the edit-render test. Studio already had this assertion and core's base did not, so the port brings the two files back in line.- After the port, the only differences between the studio and core files are the ones that already existed. Core keeps its extra
@using Elsa.Studio.Security.Modelsand its extraworkflows/definitions:updateassertion. - The test method sets in all three test files are identical to studio's head. All 7 new #1116 tests are present, and so are the updated assertions for items 1–6, 8–14, 18 and 19.
- 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, incomplete and last-default copy is unchanged.
- 15: the tabs are still
F4 behaviour
KeepPanelsAliveis still on the permissionMudTabs(RoleEditorSurface.razor:121).- In-place advanced-grant editing: the card's edit branch,
BeginEditAdvancedGrantandApplyAdvancedGrantEditAsync, are untouched. The existing in-place edit tests pass. - Access boundary (B2):
RoleAdministrationAccessBoundaryis not touched. During a re-check it still keeps the content mounted, passes downCanCreate/CanUpdate/CanDelete = false, and queues changes that arrive mid-check.- The new bulk action fits this model. The Select all/Deselect button only renders when
!IsReadOnly, andToggleCategory,SelectCategoryandDeselectCategoryall return early whenIsReadOnly. - I added a scratch probe that re-renders the editor with the re-check access and then with the restored access. While access is being re-checked, Select all/Deselect and Save are hidden. Once access is restored, Deselect is back. The filter and the selection are kept through both steps.
RoleAdministrationAccessBoundaryTests(includingRender_WhenChangedArrivesDuringACheck_KeepsEditsAndHonoursTheQueuedRefresh) pass.
- The new bulk action fits this model. The Select all/Deselect button only renders when
Permission semantics
- The save payload is unchanged. It is still
NormalizePermissions(_direct ∪ _advanced ∪ _unresolved)intoCreateRoleRequest/UpdateRoleRequest. LoadStoredGrants,RolePermissionAuthoringand wildcard validation and matching are all unchanged. There are no API or contract changes.- Select all adds the same uncovered exact grants as before, but only for visible resources. Deselect removes exactly that set. Wildcards, unresolved grants, hidden resources and covered exact grants are 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 |
| Deselect back to all concrete grants (the Deselect wildcard fix reverted) | Deselect_PreservesExactGrantsThatAreAlsoCoveredByAWildcard fails |
| Deselect widened to the whole category | No ported test fails; only a scratch probe catches it (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 a hidden grant: saves
["identity/applications:view"]. identity/*:view+ filtered Select all: saves the wildcard plusroles:createandroles:updateonly.- Deselect with a covered exact grant: saves
identity/*:viewandidentity/roles:view. - Everything covered by
identity/*:*: Select all does nothing, and the save is unchanged. - Partly selected visible set: Select all completes only the visible resource.
- Access re-check: see F4 behaviour above.
Builds and tests
Elsa.Studio.Host.ServerRelease (net8/9/10): build succeeded, 0 warnings, 0 errors.Elsa.Studio.Host.WasmRelease (net8/9/10): build succeeded, 0 warnings, 0 errors.Elsa.Studio.Security.Tests: 154/154 passed.Elsa.Studio.Administration.Tests: 65/65 passed.- Solution grouping:
scripts/solutionunit tests passed (8/8), andsolution_groups.py --checkreports "Solution grouping and 10 filters are up to date." TheSolution groupsworkflow did not run in CI, because no.sln,.slnfor.csprojchanged.
Security and maintainability
There are no new inputs, no markup injection and no change to authorization. Maintainability is the same as #1116. SelectableGrants is the single source for Select all, Deselect and the fully-selected check, and the port is a clean copy of that design.
Non-blocking (carried over from #1116; all still apply)
- N1.
N selectedmixes units.SelectedCount = direct + covered + advanced, soidentity/roles:*alone shows 4 selected with no box checked. Greptile raised this again here (thread atRoleEditorSurface.razor:325, open). - N2. The Roles browser spec is stale.
test/studio/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. CI does not run it. - N3. Test gap. No test proves that a filtered Deselect keeps grants on hidden resources (the widen-Deselect mutation passes the suite).
- N4. Select all can do nothing. When every visible verb is covered by a wildcard, Select all is shown but does nothing. Greptile raised this here (thread at
RoleEditorSurface.razor:524, open). - N5. Dead CSS. The
.role-idrule inRoles.razor(line 83) no longer has any users.
These are best fixed in studio first and then ported, so the two copies stay identical.
CI, Greptile and threads on 4083e9e7
- Green:
pr / ubuntu-latest(Compile + Test), CodeQL and all its Analyze jobs,select-tests, GitGuardian and CLA.run-affected-testswas skipped. CodeRabbit was skipped because the PR is a draft. - Greptile: 4/5 at
4083e9e7(advisory only). - Threads: 2, both open and both from Greptile. They match N1 and N4 and are non-blocking.
Gate: APPROVE + HIGH and green CI on 4083e9e7. Before merging, the PR still needs to come out of draft, and the two open Greptile threads need a reply or resolution, since GitHub currently reports the merge state as BLOCKED.
| @foreach (var covering in alsoCovered) | ||
| { | ||
| <MudTooltip Text="The wildcard already grants this; the exact grant is redundant."> | ||
| <span class="role-grant-state">Also covered by @covering</span> | ||
| </MudTooltip> |
There was a problem hiding this comment.
When different wildcards cover exact grants for different verbs of the same resource, the editor now groups their “Also covered by” notes under the resource instead of beside the relevant checkboxes. An administrator must work out which note describes each exact grant from the wildcard text, making the permissions display misleading.
Artifacts
Authored bUnit test for two wildcard-covered verbs
- This executable test renders the role with both exact grants and wildcards and checks which verb option contains each note; it passes before the PR and fails after it.
Chromium capture script for the rendered permission row
- This script opens each actual bUnit-rendered row in Playwright Chromium and records its visible text, video, and poster; the recordings compare the same scenario.
bUnit result at the previous review SHA
- The recorded dotnet test command passes and prints both notes inside their respective verb options; verb association is visible before the PR.
▶ Previous rendered editor row with notes beside each verb
- Chromium displays the bUnit-rendered row from the previous review SHA, where each wildcard note accompanies its applicable verb.
Poster of previous row with verb-associated notes
- The captured Chromium frame shows the previous render's wildcard notes within the verb options; their associations remain visible.
Chromium capture output for the previous row
- The recorded capture command reports wildcard notes in both verb options and exits successfully; it confirms what the previous video displays.
bUnit result at the current PR HEAD
- The same recorded dotnet test command fails and prints two resource-level notes but verb options containing only update and view; the associations are lost.
▶ Current rendered editor row with notes detached from verbs
- Chromium displays the bUnit-rendered PR row with both wildcard notes beside the resource and none inside its verb options; the changed placement is visible.
Poster of current row with resource-level notes
- The captured Chromium frame shows both notes grouped under the resource rather than beside update and view; the verb context is absent.
Chromium capture output for the current row
- The recorded capture command reports both notes in the resource row and verb options containing only their labels; it confirms the current video's visible result.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/studio/modules/Elsa.Studio.Security/Components/RoleEditorSurface.razor
Line: 174-178
Comment:
**Wildcard notes lose verb context**
When different wildcards cover exact grants for different verbs of the same resource, the editor now groups their “Also covered by” notes under the resource instead of beside the relevant checkboxes. An administrator must work out which note describes each exact grant from the wildcard text, making the permissions display misleading.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.2 participants
Purpose
Forward-port elsa-studio #1116 into elsa-core
main, where Studio now lives undersrc/studioafter the consolidation.Refs elsa-studio #1114
Refs elsa-studio #1115
Refs elsa-studio #1116
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 still shipped leftover loader copy and lecture text that 3.9 trimmed in studio.
Solution
Port of studio
0ad1874859fdd5beabfdac980ab13ee7f9b372af(#1116) intosrc/studio/modules/Elsa.Studio.Securityand its tests:F4 work already on
main(#8586,#8587) is preserved:KeepPanelsAliveon the permission tabs, in-place advanced-grant editing, and the permission-refresh signal on administration access boundaries.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 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.
Verification
Server host:
dotnet build src/studio/hosts/Elsa.Studio.Host.Server/Elsa.Studio.Host.Server.csproj --configuration Release— Build succeeded, 0 warnings, 0 errors (net8/net9/net10).WASM host:
dotnet build src/studio/hosts/Elsa.Studio.Host.Wasm/Elsa.Studio.Host.Wasm.csproj --configuration Release— Build succeeded, 0 warnings, 0 errors (net8/net9/net10).Security tests:
dotnet test src/studio/modules/Elsa.Studio.Security.Tests/Elsa.Studio.Security.Tests.csproj --configuration Release— 154 passed, 0 failed (net10.0), including:SelectAll_WithAnActiveFilter_GrantsOnlyTheVisibleResourcesSelectAll_TogglesToDeselectForTheVisibleSetDeselect_PreservesExactGrantsThatAreAlsoCoveredByAWildcardPermissionCounter_HidesAtZeroAndShowsSelectedCountExactGrantCoveredByAWildcard_ShowsOneNotePerResourceAdvancedGrants_UseASingleWildcardChipAndSingularResourceCopyRender_WhenSearchMatchesNothing_HidesTheCountAndClearSearchRestoresTheListRender_WhenImpactIsSafe_ShowsSafeConfirmationAndDeleteActionAdministration tests:
dotnet test src/studio/modules/Elsa.Studio.Administration.Tests/Elsa.Studio.Administration.Tests.csproj --configuration Release— 65 passed, 0 failed (net10.0).Reverting the filter-aware Select all (passing the unfiltered category into
ToggleCategory) makesSelectAll_WithAnActiveFilter_GrantsOnlyTheVisibleResourcesfail (Assert.Falseexpected false, actual true on a hidden resource after Select all). The revert was not committed.Live browser pass was not run here: this environment has no Elsa Core identity backend.
Checklist