Improve role administration and permission-aware Studio UX - #999
Improve role administration and permission-aware Studio UX#999sfmskywalker wants to merge 38 commits into
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
Authorization-disabled hosts can fail to render, and menu filtering can irreversibly mutate provider-owned trees.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 55/56 changed files
- Comments generated: 2
- Review effort level: Balanced
Greptile SummarySummary
Merge SafetyNot safe to merge. The outstanding user-detail navigation issue must be fixed so tenant-hosted Studio deployments retain their configured application base path. Confidence Score: 4/5Not safe to merge because one outstanding blocking navigation defect remains. The two resolved permission-loading and disposal findings are fully addressed in the current code. The unresolved user-list finding remains outstanding: Reviews (7): Last reviewed commit: "Simplify generated role ID guidance" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟡 Changes recommended
The unresolved NavMenu disposal race can propagate an ObjectDisposedException during authentication notifications.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 60/61 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Two moderate permission-safety and browser-coverage issues remain unresolved.
Review details
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
src/framework/Elsa.Studio.Core/Services/CurrentUserPermissionService.cs:53
- Permission claims are returned before verifying that the principal is authenticated. An unauthenticated
ClaimsIdentitycan still contain claims, so such a principal would passHasAsyncand see protected navigation despite the service's fail-closed behavior. CheckIsAuthenticatedbefore accepting claims or consulting the effective source.
tests/browser/RoleManagement/role-management.spec.ts:440 - These negative checks do not match the menu links they are intended to verify. The menu providers render relative values such as
href="workflows/definitions", while every selector here requires a suffix beginning with/; CSS attribute selectors inspect the raw attribute, so the assertions can pass even when unauthorized links remain. Use suffixes without the leading slash so both relative and absolute href values are covered.
- Files reviewed: 60/61 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Addressed the two suppressed findings from Copilot review 5136190703 in commit 0fe8bf0: CurrentUserPermissionService now rejects unauthenticated principals before accepting permission claims or effective grants, with regression coverage for forged claims; the restricted-navigation browser assertions now cover relative, root-relative, and absolute href values. Validation: 49 Core tests pass, RoleManagement TypeScript typecheck passes, and git diff --check passes. |
|
@copilot review |
Reviewed the current branch and the latest workflow results. No additional actionable changes are required; the previously identified findings are already addressed, and the latest Copilot review and CodeQL runs completed successfully. |
|
|
||
| protected static string Scope(string? tenantId) => string.IsNullOrWhiteSpace(tenantId) ? "Host" : tenantId; | ||
| protected static string UserUrl(string id) => $"security/users/{Uri.EscapeDataString(id)}"; | ||
| protected static string UserUrl(string id) => $"/security/users/{Uri.EscapeDataString(id)}"; |
There was a problem hiding this comment.
If HostedWasm is served under a tenant-prefixed application base such as /tenant/, this root-relative URL bypasses that base. User links and row clicks then navigate to /security/users/{id} rather than /tenant/security/users/{id}, causing the user detail page to open at the wrong route or return a 404. Return a base-relative URL such as security/users/{Uri.EscapeDataString(id)}.
Artifacts
- The authored Playwright script reads the exact pre-change and current UserUrl expressions, serves each with the HostedWasm tenant base, and exercises the user link and row navigation; it demonstrates the root-relative regression.
- The command output records the working directory, exit code 0, exact source expressions, and Chromium-resolved link and row paths; it confirms the current revision drops the tenant prefix.
- Chromium records the pre-change base-relative user target resolving under `/tenant/` for both the generated link and row navigation; it establishes the correct prior behavior.
Poster for pre-change tenant-base navigation
- The poster frame shows the pre-change user URL expression and tenant application base used in the Chromium recording; it establishes the expected tenant-preserving behavior.
- Chromium records the current root-relative user target resolving at `/security/users/...` after the generated-link and row-navigation actions; it demonstrates the tenant prefix is discarded.
Poster for current root-relative user navigation
- The poster frame shows the current leading-slash UserUrl expression against the tenant application base; it highlights the introduced base-discarding behavior.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/modules/Elsa.Studio.Security/Components/UserListSurface.razor.cs
Line: 109
Comment:
**Preserve tenant base path**
If HostedWasm is served under a tenant-prefixed application base such as `/tenant/`, this root-relative URL bypasses that base. User links and row clicks then navigate to `/security/users/{id}` rather than `/tenant/security/users/{id}`, causing the user detail page to open at the wrong route or return a 404. Return a base-relative URL such as `security/users/{Uri.EscapeDataString(id)}`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Summary
Validation
dotnet build Elsa.Studio.sln(passes; 0 errors, 6 warnings)dotnet test Elsa.Studio.sln --no-build --logger "console;verbosity=minimal"(passes)npm run typecheckintests/browser/RoleManagement(passes)Notes
main