Repository navigation
Hold the login-page redirect until permissions load (#3540) - #3541
Conversation
A post-login guard re-parse that fires between the token write and the /my-permissions response tests canAdminSection against empty permissions: an admin's stashed /admin-prefixed target takes the non-admin fallback to /workspaces, and once the fetch completes nothing re-attempts the stash — the user is parked (the fmtk flows 'Handle' wait_for timeout, ~every 2nd-3rd nightly). AuthService now tracks permissionsLoaded across the session cycle (set false on token save/clear, true when the fetch settles on any outcome but a 401), and guardLoggedInPublicRoute holds (returns null) for a logged-in user on a public route while it is false — the fetch's completion notify re-parses the same location and the gate then decides with live data. open_admin_users retries its navigation once as harness-side belt and suspenders.
github-actions
Bot
added
the
backport/2.0
Review follow-up: the reachable flake path is a mid-saveToken navigation to /admin/users (the e2e login wait matches the typed email, so the harness navigates before /my-permissions answers) — a non-public route, so the guardLoggedInPublicRoute hold never fires and guardAdminRoute bounced the empty-permission admin to /workspaces. guardAdminRoute now takes permissionsLoaded and holds the same way; the fetch's completion notify re-parses the held location and keeps an admin there (or bounces a settled non-admin). Also pins the integration path: app_redirect_test now wires the real flag into its router and drives the deferred-permission login both ways (admin stays, non-admin bounces on settle), plus a persisted-boot-token lifecycle test and doc precision on the hold's scope and the flag's meaning.
… changelog headline
Fresh-eyes reviewFull review (REQUEST CHANGES → addressed in 4c424a3): Blocking issues1. The hold does not cover Important issues2. The integration behavior is not pinned anywhere. Nits / questions
Verified non-issues: no strand in the normal lifecycle (all VerdictREQUEST CHANGES — the hold fixes the wrong guard: Delta review of the fix commit (APPROVE): Review: PR #3541 (delta 5bd0e31..4c424a3)Prior findings verificationBlocking 1 — VERIFIED FIXED. Mutation check: I reverted the guard body to the pre-fix logic (keeping the parameter) — both new widget tests fail with Important 2 — VERIFIED FIXED. app_redirect_test.dart:125 wires the real Nits 3–4 — fixed (doc rewordings verified in the tree). All three touched test files pass (180 tests), plus oidc_complete_page_test (the other Blocking issuesNone. Important issuesNone. Nits / questions
Regression bleedNone found. The VerdictAPPROVE — the blocking and important findings are genuinely fixed with load-bearing tests (mutation-verified), and only doc-accuracy nits remain. |
3D dependency graph (self-contained HTML)The page is fully self-contained: save it from either link and it opens offline. GitHub serves the raw gist as plain text, so the source link downloads; the render link views. |
The dispatched flows run exposed the second-order gap: the admin gate's hold can mount AdminUsersPage while /my-permissions is in flight, and the page re-resolves its permission-gated tabs only in build() — with no dependency on AuthService, the settle notification never rebuilt it, so it stayed on 'No admin sections available' forever (the events test's 'Handle' timeout, 65s through both the wait and the harness retry). The page now watches AuthService, so the fetch's completion notify rebuilds it with live permissions. Also updates the prior review's doc nits (hold-duration honesty — the fetch has no client-side timeout; test-router fidelity with production's canAdminSection wiring; changelog headline).
e2e validation (fmtk flows, dispatched on this branch)
* run predates 9812570 by one docs-only commit; 8 failures were the #3469 wedge family (one isolate-wedge marker) — this branch does not touch those paths. The first dispatched run caught a real second-order bug the review had flagged as a worst case: with the hold, Across the three runs the |
|
Created backport PR for
Please cherry-pick the changes locally and resolve any conflicts. git fetch origin backport-3541-to-stable/2.0
git worktree add --checkout .worktree/backport-3541-to-stable/2.0 backport-3541-to-stable/2.0
cd .worktree/backport-3541-to-stable/2.0
git reset --hard HEAD^
git cherry-pick -x 633a8be638978ea343df173ec1da93e8981d1033 |
…issions load (#3545) * frontend: hold the login-page redirect until permissions load (#3540) A post-login guard re-parse that fires between the token write and the /my-permissions response tests canAdminSection against empty permissions: an admin's stashed /admin-prefixed target takes the non-admin fallback to /workspaces, and once the fetch completes nothing re-attempts the stash — the user is parked (the fmtk flows 'Handle' wait_for timeout, ~every 2nd-3rd nightly). AuthService now tracks permissionsLoaded across the session cycle (set false on token save/clear, true when the fetch settles on any outcome but a 401), and guardLoggedInPublicRoute holds (returns null) for a logged-in user on a public route while it is false — the fetch's completion notify re-parses the same location and the gate then decides with live data. open_admin_users retries its navigation once as harness-side belt and suspenders. * frontend: guardAdminRoute holds while permissions load too (#3540) Review follow-up: the reachable flake path is a mid-saveToken navigation to /admin/users (the e2e login wait matches the typed email, so the harness navigates before /my-permissions answers) — a non-public route, so the guardLoggedInPublicRoute hold never fires and guardAdminRoute bounced the empty-permission admin to /workspaces. guardAdminRoute now takes permissionsLoaded and holds the same way; the fetch's completion notify re-parses the held location and keeps an admin there (or bounces a settled non-admin). Also pins the integration path: app_redirect_test now wires the real flag into its router and drives the deferred-permission login both ways (admin stays, non-admin bounces on settle), plus a persisted-boot-token lifecycle test and doc precision on the hold's scope and the flag's meaning. * docs(fmtk #3540): review nits — hold-duration honesty, test fidelity, changelog headline * frontend: admin page rebuilds when the permission fetch settles (#3540) The dispatched flows run exposed the second-order gap: the admin gate's hold can mount AdminUsersPage while /my-permissions is in flight, and the page re-resolves its permission-gated tabs only in build() — with no dependency on AuthService, the settle notification never rebuilt it, so it stayed on 'No admin sections available' forever (the events test's 'Handle' timeout, 65s through both the wait and the harness retry). The page now watches AuthService, so the fetch's completion notify rebuilds it with live permissions. Also updates the prior review's doc nits (hold-duration honesty — the fetch has no client-side timeout; test-router fidelity with production's canAdminSection wiring; changelog headline). (cherry picked from commit 633a8be)
…issions load (#3545) (#3545) * frontend: hold the login-page redirect until permissions load (#3540) A post-login guard re-parse that fires between the token write and the /my-permissions response tests canAdminSection against empty permissions: an admin's stashed /admin-prefixed target takes the non-admin fallback to /workspaces, and once the fetch completes nothing re-attempts the stash — the user is parked (the fmtk flows 'Handle' wait_for timeout, ~every 2nd-3rd nightly). AuthService now tracks permissionsLoaded across the session cycle (set false on token save/clear, true when the fetch settles on any outcome but a 401), and guardLoggedInPublicRoute holds (returns null) for a logged-in user on a public route while it is false — the fetch's completion notify re-parses the same location and the gate then decides with live data. open_admin_users retries its navigation once as harness-side belt and suspenders. * frontend: guardAdminRoute holds while permissions load too (#3540) Review follow-up: the reachable flake path is a mid-saveToken navigation to /admin/users (the e2e login wait matches the typed email, so the harness navigates before /my-permissions answers) — a non-public route, so the guardLoggedInPublicRoute hold never fires and guardAdminRoute bounced the empty-permission admin to /workspaces. guardAdminRoute now takes permissionsLoaded and holds the same way; the fetch's completion notify re-parses the held location and keeps an admin there (or bounces a settled non-admin). Also pins the integration path: app_redirect_test now wires the real flag into its router and drives the deferred-permission login both ways (admin stays, non-admin bounces on settle), plus a persisted-boot-token lifecycle test and doc precision on the hold's scope and the flag's meaning. * docs(fmtk #3540): review nits — hold-duration honesty, test fidelity, changelog headline * frontend: admin page rebuilds when the permission fetch settles (#3540) The dispatched flows run exposed the second-order gap: the admin gate's hold can mount AdminUsersPage while /my-permissions is in flight, and the page re-resolves its permission-gated tabs only in build() — with no dependency on AuthService, the settle notification never rebuilt it, so it stayed on 'No admin sections available' forever (the events test's 'Handle' timeout, 65s through both the wait and the harness retry). The page now watches AuthService, so the fetch's completion notify rebuilds it with live permissions. Also updates the prior review's doc nits (hold-duration honesty — the fetch has no client-side timeout; test-router fidelity with production's canAdminSection wiring; changelog headline). (cherry picked from commit 633a8be)
1 participant
Summary
The flows-suite
'Handle'wait_for flake (#3540): a logged-in admin intermittently lands on/workspacesinstead of the admin page after login, and nothing retries the navigation. Run 37013393514's backend log pins the mechanism — the workspaces page was fetching at 13:40:30 before/my-permissionsreturned at 13:40:31, so the post-login guard re-parse ran while_permissionswas still empty.guardLoggedInPublicRoutetested the stashed/admin-prefixed target againstcanAccessAdmin == falseand took the#2670non-admin fallback to/workspaces; when permissions land,_fetchPermissionsnotifies nobody, the committed location stays/workspaces, and the stash is never re-attempted. The next test's identical navigation lands fine once the state is settled — which is why a different admin test fails each night.Fix, two layers:
AuthServicetrackspermissionsLoadedacross the session cycle — false when a token is saved or the session cleared, true once the permission fetch settles (a 200, a non-401 refusal, or a network error; a 401 clears the session instead).guardLoggedInPublicRouteholds (returns null) for a logged-in user on a public route while it is false: the fetch's completion fires the notify that re-parses the same location, and the gate then decides with live data. A hold is safe — the login page is a legitimate resting surface for the fraction of a second the fetch takes, and no other guard redirects a logged-in user away from it (no loop: the flag strictly transitions once per login).open_admin_usersre-runs its hash navigation once when the'Handle'wait expires, absorbing any residual navigation race.Closes #3540.
Testing
evaluateGuards.AuthServicelifecycle tests: login settles the flag, a failed fetch still settles it (no eternal hold), logout clears it, a 401 clears the session rather than settling.