Repository navigation
Port remaining elsa-studio 3.9 UX PRs into src/studio - #8587
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Bring in #8572 so Studio logout and dashboard permissions resolve against the merged backend. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
Environments and JWT refresh raise a no-dep IPermissionRefreshSignal instead of taking IPermissionSnapshotCache, so Security plus Environments can resolve. Invalidate bumps a generation token, drops the snapshot, then raises Changed so PermissionView and NavMenu re-fetch after sign-in, environment switch, and silent refresh. Sign-out stays on the page when token clear fails. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
sfmskywalker
left a comment
sfmskywalker
left a comment
There was a problem hiding this comment.
Elsa 3 Code Review: REQUEST_CHANGES + HIGH @ fc5f9a5
Code Review, Round 1/4
Scope. I reviewed only this PR's own commits on top of #8586 @ 7b1c75ca: d0cf63c4e, beb0202eb, the two merges of B (875800bb9, a21693540) and fc5f9a55f. That is 109 files, +3,270/−2,415, and 4,125 of those lines are Elsa.sln. I compared each source PR's merged diff on elsa-studio release/3.9.0 line by line against this delta, then checked every touched file against the current release/3.9.0 tip.
This PR contains #8586 @ 7b1c75ca unchanged, so it inherits #8586's open R2 blockers. It needs a main merge or rebase once B's fix lands. B's issues are not repeated here.
Verdict
The port itself is good. The production changes from #1077, #1094, #1096, #1097 and #1098 are close to verbatim. The 401/403 guidance fits main's fail-closed permission model without touching it. The build is clean and every affected suite passes. Four of five revert checks fail as they should.
Three things block it:
- A required CI check fails.
- One source test file from #1094 was silently dropped, so three call sites have no coverage.
- The #1097 user-menu test hunk is missing, although the PR body says it is covered.
Blockers
1. Solution grouping and filters are up to date fails.
Running python3 scripts/solution/solution_groups.py --check on this head reports Out of date: Elsa.sln, Elsa.Studio.slnf. B @ 7b1c75ca passes the same check.
- The new
Elsa.Studio.Localization.Testssits under two stray solution folders,studio→modules, nested undersrc. It should be in the generated Studio grouping. Elsa.Studio.slnfdoesn't list it.
Fix: run python3 scripts/solution/solution_groups.py and commit the result. I tried this on a scratch copy and --check passed afterwards. Separately, most of the 4,125-line Elsa.sln diff is dotnet sln add reordering the whole ProjectConfigurationPlatforms block. I reproduced that from B's Elsa.sln, so it is a tooling side effect, not a content change. It's optional, but adding the one project entry by hand would keep this file's diff to about 20 lines and make later solution diffs reviewable.
2. #1094 RefusedWriteTests.cs was not ported, and the PR body doesn't say so.
The source adds three tests:
- Labels: create refused → snackbar shows
ForbiddenMessage. - Alteration dry-run refused →
Dry-run failed: <guidance>. - Alteration submit refused → guidance in the dialog, with no
HTTP 403.
Without them, the production call-site changes in Labels.razor.cs, AlterationDesignerHost.razor.cs and AlterationSubmitDialog.razor have no tests. I confirmed this with a revert check: I put Labels.razor.cs back to B's version and the Administration suite still passed (55/55).
Porting needs only small adaptations:
labels:viewandlabels:createare valid core names (Elsa.Labels.Permissions.LabelPermissions), andStubPermissionService.Grantsparses them through B's strictPermission.TryParse.- Add the
Elsa.Studio.Alterationsproject reference toElsa.Studio.Administration.Tests.csproj. - Align
NoVariablesInstanceServicewith core'sIWorkflowInstanceService.
If something can't be ported, list it in the source table as adapted or deferred, with the reason.
3. #1097's AppBarUserMenuTests hunk is missing.
The source changes UserMenu_OpensBelowItsButton to use MenuPopoverAssert and links MenuPopoverAssert.cs into Elsa.Studio.ExternalAuthentication.Tests. On this head, AppBarUserMenuTests.cs exists but has no UserMenu_OpensBelowItsButton test at all. That test came from studio #1091, whose production fix, UserMenu.razor TransformOrigin="Origin.TopRight", is already on main. The test never was.
So the user-menu third of the "opens below its button" fix has no regression test, while the language and environment pickers do. The PR body's "AppBarUserMenuTests is now present via #8586" is true of the file but not of this test. Fix: add the test in its #1097 form and add the MenuPopoverAssert.cs <Compile Include … Link> to the ExternalAuthentication test project.
Fidelity, per source PR
| Source | Ported | Adapted (why) | Missing |
|---|---|---|---|
| #1077 Users tenant-scope label | All 5 files, line-for-line | None | None |
| #1089 wait for navigation after async clicks | ConnectionEditorTests, RoleEditorSurfaceTests, RolesPageTests hunks in this PR |
The ElsaIdentitySignOutTests hunk arrives with #8586. The file is identical to release/3.9.0. |
None. Covered. |
| #1090 Enter-to-sign-in | Login tests: Enter in either field, IME composition, missing credentials, Enter while busy, plus LoginChooserTests |
IsSubmitKey, Immediate/OnKeyDown and the BusyState guard are already in #8586's diff, so this PR contributes the tests plus an @using Elsa.Studio.Components that looks redundant, since B's tree compiles without it. Adds AngleSharp package refs because main's test projects don't get it transitively. |
None. The body's table should say the production half comes from #8586. |
| #1094 friendly 401/403 | AuthorizationFailureExtensions, Error.razor/.cs, IdentityApiErrorMapper, UserTaskErrorMapper, the Workflows, Secrets, ExtAuth, Labels, Alterations, AI, OpenTelemetry and PlatformIntegration call sites, and the Core, Admin, ExtAuth, Security, UserTasks and Workflows tests |
MainLayoutErrorBoundaryTests stub implements IsInitialized because main's IFeatureService has that member. The extra using Elsa.Studio.Localization in Labels.razor.cs is needed because the core file doesn't import it already. |
RefusedWriteTests.cs (blocker 2). The shared-helper switch in ConnectionEditorTests, ExternalIdentityLinksTests and RemoteFeatureProviderTests was skipped; each still has a private CreateApiException* although ApiExceptions.cs is already linked into that project (nit). |
| #1096 MudMenu popover waits | PopoverTestExtensions is byte-identical, and the edits to WorkflowActionPermissionTests, WorkflowListPermissionTests and WorkflowEditorLifecycleTests match |
None | None. 5 repeat runs of those 3 classes were 27/27 each time. |
| #1097 pickers open below their buttons | LanguagePicker/EnvironmentPicker TransformOrigin, MenuPopoverAssert, EnvironmentPickerTests, the new Elsa.Studio.Localization.Tests |
Elsa.Studio.sln became Elsa.sln because core's solution is canonical, but it isn't regenerated (blocker 1) |
The AppBarUserMenuTests hunk and the ExtAuth MenuPopoverAssert link (blocker 3) |
| #1098 shared TestLocalizer | src/studio/testing/TestLocalizer.cs is identical to the source, the csproj links are in place, and the local copies are removed (no class TestLocalizer is left outside testing/) |
ActivityExecutionDetailsTests, ActivityExecutionsTabTests and ExecutionDetailsDrawerRenderTests don't exist on main. Dashboard.Tests already uses the shared helper. |
None |
Fit with #8586's permission model
- This PR adds no production permission names. Every grant in its tests (
secrets:view,workflows/*:view,alterations:execute) goes through B's strictPermission.TryParse, which rejects the legacy names, so they are valid{resource}:{verb}names. - It changes only how a backend 401/403 is presented.
IdentityPermissionContextstill turns 401/403 or an unavailable snapshot intoFromGrants([])and never throws intoError.razor, so fail-closed resolution is untouched. - The
Error.razor"Sign in" hand-off appears only for a 401 and only when the user clicks it, as in the source, so a rejected token can't cause a redirect loop.
Verification on fc5f9a5
-
dotnet build Elsa.Studio.slnf -c Releasegave 0 errors.Elsa.Studio.Localization.Testsisn't in the filter (blocker 1), so I built it separately: 0 errors. -
All tests passed (Release,
--no-build):Suite Passed Core 149 Security 119 Administration 62 Authentication.UI 33 Environments 15 Localization 1 StructuredLogs 23 UserTasks 31 Dashboard 107 Workflows 484 ExternalAuthentication 465 -
Revert checks:
Error.razorback to B → 5Errorstests fail ✔IdentityApiErrorMapperback to B → 1 test fails ✔LanguagePickerTransformOriginback toBottomRight→LanguagePickerTestsfails ✔- Enter handler removed from
ElsaIdentityLoginMethod→ 2 Enter tests fail ✔ Labels.razor.csback to B → nothing fails ✘ (blocker 2)
CI, Greptile and threads
- CI: everything is green except
Solution grouping and filters are up to date, which fails (blocker 1).run-affected-testsis skipped (dispatch-only). - Greptile: 0/5 on
fc5f9a55. All 4 findings are in files only #8586 changes:PermissionTrackingComponentBase,DashboardPermissions,ElsaIdentitySignOutServiceand the DashboardIndex.razor.csruntime chip. None is in this PR's own delta. The/identity/logoutfinding should clear now that #8572 has merged. - Threads: 7 unresolved, all on #8586-owned files. This PR's own threads (
ForbiddingBackend×3,ApiExceptions×2) are resolved. - Effect on the gate: because this PR's diff against main includes all of #8586, its Greptile score and threads can only clear once #8586 is fixed and merged and this branch is updated on main.
Maintainability (non-blocking)
Elsa.slnchurn: see blocker 1. A hand-inserted entry keeps the history readable.- Use the shared
ApiExceptions.Createin the three ExtAuth test files, as the source does, rather than keeping three private copies next to the linked helper. - PR body accuracy:
- Small tidy-ups:
- The
TestLocalizerremoval inWorkflowEditorLifecycleTests.csleaves a double blank line. using Elsa.Studio.Testing;was appended out of order in several test files (the source kept them sorted).- The
@using Elsa.Studio.Componentsadded to both login methods looks redundant.
- The
Required for Round 2
- Regenerate the solution grouping so the CI check passes.
- Port
RefusedWriteTests(or document a reasoned adaptation). - Port the #1097
AppBarUserMenuTestshunk with theMenuPopoverAssertlink. - Update the source table.
- After #8586's R2 fixes land, merge B's new head (or main, once B has merged) and re-request Greptile.
Merge gate
This PR merges only on APPROVE + HIGH on its tip, with green CI and Greptile 5/5 on that same tip. Order is A, then B, then C: #8572 (A) is merged (8d88d04f), #8586 (B) must merge next, and this PR (C) only after B. Today the gate is not met: REQUEST_CHANGES, one failing required check, Greptile 0/5 from the inherited #8586 findings, and #8586 still in REQUEST_CHANGES.
Adding the Environments project reference made Microsoft.Extensions and Elsa.Studio.Security extension types collide. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
Regenerate Studio solution grouping so Localization.Tests is in Elsa.Studio.slnf. Port #1094's refused-write tests and #1097's UserMenu_OpensBelowItsButton hunk, switch the ExtAuth suites onto the shared ApiExceptions helper, and tidy import order. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
Register IPermissionRefreshSignal per circuit, publish (generation, snapshot) atomically, reload overtaken GetAsync callers, and re-resolve Users/Roles boundaries plus NavMenu on Changed. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
Bring scoped IPermissionRefreshSignal, atomic cache generation, and the R3 proving tests onto the stacked UX branch. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
Raise IPermissionRefreshSignal only when a JWT refresh fails or the sub/permissions claims change, and cap overtaken GetAsync reloads so a near-expiry token cannot livelock. Keep Users and Roles content mounted while re-checking access, and honour a Changed that arrives mid-check. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
Bring the permission-refresh livelock cap and access-boundary coalesce onto the stacked UX branch. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
Keep C's stacked UX nits to import order and a double blank line. Do not reorder Elsa.sln. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
Keep Users and Roles editors mounted so drafts survive a permission refresh, but pass view-only create/update/delete flags until the new access result arrives. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
Bring in the single B push that disables Users/Roles mutation controls while a permission re-check is in flight and keeps editor drafts mounted. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
CI failed when create:False appeared before the coalesced third GetAsync. WaitForAssertion now requires Calls == 3 and an empty Results queue. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
Bring the boundary-test WaitForAssertion race fix from #8586 so C stays stacked on B. Co-authored-by: Sipke Schoorstra <sipkeschoorstra@outlook.com>
sfmskywalker
left a comment
sfmskywalker
left a comment
There was a problem hiding this comment.
Elsa 3 Code Review: APPROVE + HIGH @ cf32c52
Code Review, Round 3/4
Scope. This is a confirmation round. I checked this PR's own delta on top of #8586 @ c67ff78d, the three #8586 merges since R2, and the R2 nits. #8586's own code is reviewed on #8586, and the findings on it are not repeated as blockers here. This approval covers this PR's own delta only.
Merge confirmation @ cf32c52
cf32c527is a two-parent merge ofbbbd2aefand #8586 @c67ff78d.c67ff78dis one test-only commit on top ofc937a27a: "wait for queued access refresh in boundary tests", touchingSecurityMenuAndBoundaryTests.csonly.git show --remerge-diffis empty, so the merge had no conflicts and no hand edits.git diff bbbd2aef cf32c527is byte-identical togit diff c937a27a c67ff78d. The merge brings in exactlyc67ff78d.- This PR's own delta is unchanged:
git diff c67ff78d cf32c527is byte-identical togit diff c937a27a bbbd2aef. - #8586 @
c67ff78dis an ancestor and contains main (8d88d04f).
Earlier merges since R2 (confirmed)
930e88f0merges #8586 @a244d07einto36eace01.bbbd2aefmerges #8586 @c937a27ainto8eb64348.- Both have an empty
--remerge-diff, so neither needed hand edits. - Each brings in exactly #8586's delta:
36eace01..930e88f0equalse07d2e3a..a244d07e.8eb64348..bbbd2aefequalsa244d07e..c937a27a.
- Outside the 3 files touched by the style commit
8eb64348, this PR's own delta is byte-identical to R2.
Nits
- ExtAuth using order: done (
8eb64348).ConnectionEditorTests,LoginChooserTestsandRemoteFeatureProviderTestsare sorted, with the aliases at the end. ✔ - Double blank line: the one in
ConnectionEditorTestsis removed. The one flagged in R2,WorkflowEditorLifecycleTests.cs:469, is still there. Cosmetic. - #1090 row: improved. It still lists
LoginChooserTestsamong this PR's tests, but those Enter tests arrive with #8586; this PR only changes that file's imports and drops the localTestLocalizer. Cosmetic. - Workflows and Administration test files: about 30 of these from the #1098 port still list
using Elsa.Studio.Testing;last. Cosmetic. Elsa.slnreorder: skipped, as agreed. Content is correct and the grouping check passes.
None of these block. They can be folded into the next #8586 re-merge or left as they are.
Fidelity
Unchanged from R2. #1077, #1089, #1090, #1094 (including RefusedWriteTests), #1096, #1097 (including UserMenu_OpensBelowItsButton) and #1098 are all ported. The R2 revert checks still apply because the code under test is byte-identical.
Verification
-
On
8eb64348(Release): all suites passed.Suite Passed ExternalAuthentication 479 Administration 65 Security 147 Core 150 Environments 21 Localization 1 Authentication.UI 33 Workflows 484 StructuredLogs 23 UserTasks 31 Dashboard 107 -
On
bbbd2aef: Administration (65), Environments (21) and ExternalAuthentication (479) passed. -
Security on
bbbd2aef: failed intermittently, 2 of 51 runs.Render_WhenChangedArrivesDuringACheckasserted before the queued access refresh had finished. -
Security on
cf32c527: stable. A Release build and first run gave 147/147. After that, every run passed:Run type Failures Full suite, consecutive 0 of 30 Full suite, alongside parallel Workflows and ExternalAuthentication runs 0 of 15 Targeted Render_WhenChangedArrivesDuringACheck, both the role and user variants0 of 40 c67ff78dmoves the call-count and "Checking access" assertions insideWaitForAssertionand addsAssert.Empty(service.Results)and the draft-preserved check. That is the right fix for the race.
CI, Greptile and threads @ cf32c52 (checked at 21:44 CEST)
- CI: green.
ubuntu-latestpassed (19m28s).Solution grouping and filters are up to datepassed.CodeQLpassed.- Both
Analyze (csharp)jobs passed. import-rehearsal-testsandpinned-source-contractpassed.run-affected-testsis skipped (dispatch-only).
- Greptile: 3/5 on
cf32c527, reviewed at 21:30 CEST. It reports no new issue for this head. It holds the score only because of the earlier permission-visibility finding in #8586'sUser/RoleAdministrationAccessBoundary.razor. Neither file is in this PR's delta. - Threads: 20 in total, all 20 resolved.
This PR inherits #8586's open findings. It must merge #8586's final head (or main, once #8586 has merged) again before it can merge.
Merge gate
This PR merges only on APPROVE + HIGH on its tip, with green CI and Greptile 5/5 on that same tip. #8572 (A) has merged (8d88d04f). #8586 (B) is open at c67ff78d and must merge next. This PR (C) merges only after it.
State at cf32c527:
- This review: APPROVE + HIGH ✔
- CI: green ✔
- Threads: 0 unresolved ✔
- Greptile: 3/5 ✘, from #8586-owned code.
- #8586: not merged ✘
Gate not met. If #8586 moves again, this PR needs another merge confirmation: a clean merge of exactly #8586's new head, this PR's own delta byte-identical, CI green and Greptile 5/5 on the new tip.
2 participants
Purpose
Ports the remaining merged elsa-studio
release/3.9.0UX PRs into consolidatedsrc/studio.This PR is stacked on #8586 and must merge after it. The branch merges the current #8586 head (
c67ff78d0) so identity gating, the scoped permission-refresh signal, atomic cache generation, the overtaken-reload cap, access-boundary coalesce, view-only mutations during re-check, the mid-check test wait,BusyState, and the shared Studio test helpers stay in sync.Scope
Description
Problem
Several merged elsa-studio 3.9 PRs never landed in core
src/studio: Users still show a tenant-scope column, sign-in forms ignore Enter, 401/403 API failures still surface as rawApiExceptiontext, and test projects each keep their ownTestLocalizer.Solution
Port the remaining 3.9 UX PRs onto the #8586 Studio identity stack.
Source PR status
ElsaIdentitySignOutTestsis present via #8586; that wait is already in the stacked identity sign-out tests.LoginChooserTests). Production (IsSubmitKey, Immediate + OnKeyDown,BusyStateguard) is already on #8586; this PR does not change login production code.AuthorizationFailureExtensions, Error.razor, mappers, call sites, tests, andRefusedWriteTests: Labels create, Alteration dry-run, Alteration submit). The three ExtAuth files now use the sharedApiExceptions.Createhelper.PopoverTestExtensions.OpenMenu) now that #8586 brought inWorkflowActionPermissionTests/WorkflowListPermissionTests/ToolbarMenu_OffersUnpublish.TransformOriginTopRight,MenuPopoverAssert, Environments + Localization tests).UserMenu_OpensBelowItsButton(studio #1091 via #1097) is ported here; productionUserMenu.razorTransformOriginis already on main. ExtAuth tests now linkMenuPopoverAssert.src/studio/testing/TestLocalizer.cs). Workflows.Tests uses the shared helper; localTestTimeFormatterstays inTestDoubles.Design notes
GET /identity/me/permissions. This PR only changes how a 401/403 is shown.BusyState(Func<Task<bool>>) so a second Enter while signing in is ignored.BusyButtonContentwas already on the feat(studio): port 3.9 permission gating onto IdentityPermissionContext #8586 tree; the login methods now use it.WaitForAssertionforCalls == 3.Elsa.Studio.Localization.Testsis in the generated Studio grouping andElsa.Studio.slnf.ConnectionEditorTestsis gone.Elsa.slnwas not reordered.Verification
HEAD:
cf32c527f(on #8586c67ff78d0)Elsa.Studio.Security.Tests(AccessBoundary filter, from B)Checklist
Summary by CodeRabbit