WEB-956: share auth session across tabs and preserve deep links#3598
WEB-956: share auth session across tabs and preserve deep links#3598jonattan-infante wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Note
|
| Layer / File(s) | Summary |
|---|---|
Guard returnUrl preservation and redirect src/app/core/authentication/authentication.guard.ts, src/app/core/authentication/authentication.guard.spec.ts |
AuthenticationGuard.canActivate() now accepts route/state snapshots, allows authenticated access, otherwise logs out and navigates to /login with conditional returnUrl query param for deep links (excluding / and /login). Unit tests verify authenticated access, deep-link preservation, redirect-loop prevention, and complex URL preservation. |
Service cross-tab synchronization and unified storage src/app/core/authentication/authentication.service.ts |
Credential persistence consolidates to localStorage; a BroadcastChannel is added; getSavedCredentials() reads only from localStorage; login() and setCredentials() persist to localStorage; logout() broadcasts {type:'logout'}; setCredentials() broadcasts {type:'login'} only when credentials were newly persisted; the channel listener rehydrates or clears auth/2FA state across tabs. |
Login component returnUrl navigation src/app/login/login.component.ts |
LoginComponent injects ActivatedRoute, reads returnUrl from query params, and navigates to it after successful auth (fallback /), using replaceUrl: true. |
Playwright auth setup storage check playwright/auth.setup.ts |
Playwright setup checks both localStorage and sessionStorage, copies credentials into localStorage only when needed, and errors if credentials are absent from both storages. |
Sequence Diagram(s)
sequenceDiagram
participant User
participant Guard as AuthenticationGuard
participant Service as AuthenticationService
participant Router
User->>Guard: Navigate to protected route (state.url)
Guard->>Service: isAuthenticated()
alt Authenticated
Service-->>Guard: true
Guard-->>User: allow route activation
else Unauthenticated & deep link
Service-->>Guard: false
Guard->>Service: logout()
Guard->>Router: navigate('/login', { queryParams: { returnUrl: state.url }, replaceUrl: true })
Router-->>User: redirect to /login?returnUrl=...
else Unauthenticated & root or '/login'
Service-->>Guard: false
Guard->>Service: logout()
Guard->>Router: navigate('/login', { replaceUrl: true })
Router-->>User: redirect to /login
end
sequenceDiagram
participant Tab1 as Tab 1
participant Channel as BroadcastChannel_mifosXAuth
participant Tab2 as Tab 2
participant Storage as localStorage
Tab1->>Storage: setCredentials(...) -> localStorage
Tab1->>Channel: postMessage({type:'login'})
Channel-->>Tab2: message {type:'login'}
Tab2->>Storage: read credentials from localStorage (restoreSession)
Tab2-->>Tab2: update auth state -> logged in
Tab1->>Channel: postMessage({type:'logout'})
Channel-->>Tab2: message {type:'logout'}
Tab2->>Storage: remove credentials & 2FA token
Tab2-->>Tab2: update auth state -> logged out
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
- openMF/web-app#2650: Touches
authentication.guard.spec.tsand has overlapping test/guard changes. - openMF/web-app#3457: Updates Playwright auth flow and interacts with
localStorage/sessionStoragehandling similar to these changes.
Suggested reviewers
- IOhacker
- gkbishnoi07
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title clearly and specifically summarizes the main changes: sharing auth sessions across tabs and preserving deep links during login, which are the core objectives of this PR. |
| Docstring Coverage | ✅ Passed | No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. |
| Linked Issues check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | ✅ Passed | Check skipped because no linked issues were found for this pull request. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/core/authentication/authentication.service.ts`:
- Around line 481-490: The login broadcast never fires because userLoggedIn$ is
set to true before setCredentials runs, so wasLoggedIn is already true; fix by
recording prior login state and performing credential persistence and broadcast
based on that prior state: in the sign-in flow adjust the order in
setCredentials/sign-in handler (references: userLoggedIn$, setCredentials,
broadcastChannel, credentialsStorageKey, rememberMe) so that you capture const
wasLoggedIn = this.userLoggedIn$.getValue() before setting userLoggedIn$ to true
(or move setCredentials to run before updating userLoggedIn$), write credentials
to storage as existing, and only call broadcastChannel?.postMessage({ type:
'login' }) when !wasLoggedIn.
- Around line 121-129: On the broadcastChannel.onmessage logout branch, also
clear any two-factor state: remove the two-factor token from both localStorage
and sessionStorage (use the two-factor token key variable, e.g.
twoFactorTokenStorageKey or the actual identifier used in this module) and
ensure the two-factor Authorization header is cleared by either calling a
dedicated method on authenticationInterceptor (e.g.
removeTwoFactorAuthorization()) or by extending
authenticationInterceptor.removeAuthorization() to also clear the 2FA header;
keep this change inside the existing logout block so it mirrors logout effects
without re-broadcasting and still calls userLoggedIn$.next(false).
- Around line 61-71: The getSavedCredentials() function still prefers
sessionStorage over the new shared localStorage, which can resurrect stale
per-tab credentials and violate the intended cross-tab behavior; change
getSavedCredentials() to read from the AuthenticationService.private storage
(the storage field set to localStorage) first and fall back to sessionStorage
only if nothing exists in storage, and remove any logic that prioritizes
sessionStorage so that localStorage is the authoritative source for saved
credentials.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ac5bd536-4189-44b8-933d-abe17a37c193
📒 Files selected for processing (4)
src/app/core/authentication/authentication.guard.spec.tssrc/app/core/authentication/authentication.guard.tssrc/app/core/authentication/authentication.service.tssrc/app/login/login.component.ts
jonattan-infante
force-pushed
the
WEB-956/preserve-deep-link
branch
from
d35d11a to
e4e13a5
Compare
|
Thanks @coderabbitai — all three findings were valid. Addressed in the squashed commit (force-push):
I also squashed the two commits into a single commit per project convention. |
|
I'll take another look at the updated branch now. (ᵔ◡ᵔ)🐇✨ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/core/authentication/authentication.guard.ts`:
- Around line 50-55: isMeaningfulTarget currently treats any path that starts
with '/login' as non-meaningful (so '/login-history' is misclassified); change
the check in the isMeaningfulTarget calculation to only exclude the exact
'/login' route by comparing the path portion exactly (e.g. strip query/hash or
use new URL(targetUrl, window.location.origin).pathname === '/login' or a regex
that matches only '^/login$') and then use that boolean in the router.navigate
call (references: targetUrl, isMeaningfulTarget, this.router.navigate).
In `@src/app/core/authentication/authentication.service.ts`:
- Around line 498-510: The broadcast happens immediately after writing
credentials, but the 2FA token (mifosXTwoFactorAuthenticationToken) may not yet
be persisted, so other tabs restoreSession() without 2FA header; ensure the 2FA
state is persisted to the same storage before sending
broadcastChannel.postMessage({ type: 'login' }) — e.g., when setCredentials(...)
is called in authentication.service.ts (and where getSavedCredentials,
credentialsStorageKey, storage and oauthService.setStorage are used), write the
mifosXTwoFactorAuthenticationToken (or include it in the stored credentials
JSON) to localStorage and only then call broadcastChannel.postMessage so other
tabs will see the 2FA header.
- Around line 237-244: The migration currently promotes only the legacy
credentials stored under this.credentialsStorageKey from sessionStorage to
localStorage; update the same migration in AuthenticationService (the block
handling legacySession) to also look for and move the legacy two-factor token
key "mifosXTwoFactorAuthenticationToken" from sessionStorage to localStorage so
both get promoted together, and ensure the returned parsed object (from
JSON.parse(legacySession)) remains unchanged; locate the migration code that
reads sessionStorage.getItem(this.credentialsStorageKey) and add logic to
sessionStorage.getItem("mifosXTwoFactorAuthenticationToken") ->
localStorage.setItem("mifosXTwoFactorAuthenticationToken", token) ->
sessionStorage.removeItem("mifosXTwoFactorAuthenticationToken").
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5fc376f6-0ce5-4f9d-9370-a41c648d14c0
📒 Files selected for processing (4)
src/app/core/authentication/authentication.guard.spec.tssrc/app/core/authentication/authentication.guard.tssrc/app/core/authentication/authentication.service.tssrc/app/login/login.component.ts
jonattan-infante
force-pushed
the
WEB-956/preserve-deep-link
branch
from
e4e13a5 to
54bb983
Compare
|
Thanks again @coderabbitai — second pass also valid. Addressed in the amended commit:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1171 🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 5455
The implementation is clean and complete. Happy to approve once all manual reviewer checks are done. 🐇✨ |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/core/authentication/authentication.service.ts`:
- Around line 122-137: The cross-tab logout handler currently only clears custom
credential keys and 2FA keys but misses clearing tokens used by the oauthService
and also unconditionally flips oauthService storage in setCredentials; update
the logout listener (the block handling event.data?.type === 'logout' inside
listenForCrossTabAuthEvents / where
authenticationInterceptor.removeAuthorization is called) to also clear
oauthService tokens without triggering redirects by invoking
oauthService.revokeTokenAndLogout({ triggerExternalLogout: false }) or, if that
causes redirects, explicitly remove tokens from both localStorage and
sessionStorage keys used by the OAuth library (checking
oauthService.getAccessToken / hasValidAccessToken to confirm); also change
initializeOAuthService and setCredentials so they consistently use the same
storage: preserve the storage selected in initializeOAuthService (do not
unconditionally call oauthService.setupStorage(localStorage) in setCredentials)
or implement a safe migration that moves tokens from sessionStorage to
localStorage when remember-me is enabled and removes tokens from the source
storage after migration so logout/cleanup operations always clear the correct
store (reference initializeOAuthService, setCredentials,
oauthService.hasValidAccessToken, oauthService.logOut).
In `@src/app/login/login.component.ts`:
- Around line 147-150: The code reads returnUrl via
activatedRoute.snapshot.queryParamMap.get('returnUrl') and navigates to it
directly using router.navigateByUrl, which can cause redirect loops or unsafe
navigation; update the login flow to validate/sanitize returnUrl before
navigating: retrieve the raw value with
activatedRoute.snapshot.queryParamMap.get('returnUrl'), ensure it is a safe
relative path (e.g., starts with '/' and does not contain protocol/host or
suspicious segments), reject known auth routes like '/login' or '/logout', and
fall back to '/' when validation fails; use the same variable name returnUrl and
call router.navigateByUrl(returnUrl || '/', { replaceUrl: true }) only after the
check so the navigate call always receives a vetted path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e194101c-0be8-4240-b4da-91a1ec09b34b
📒 Files selected for processing (4)
src/app/core/authentication/authentication.guard.spec.tssrc/app/core/authentication/authentication.guard.tssrc/app/core/authentication/authentication.service.tssrc/app/login/login.component.ts
jonattan-infante
force-pushed
the
WEB-956/preserve-deep-link
branch
from
54bb983 to
c6a651b
Compare
|
Quick follow-up @coderabbitai — I removed the
I'm trading specific correctness here for long-term simplicity. Happy to revisit if a maintainer prefers the migration kept — it's a small block to put back. The 2FA migration suggestion (CodeRabbit #3284700933) is removed for the same reason; it only existed to support the credentials migration. Force-pushed |
|
Tip For best results, initiate chat on the files or code changes.
The two surviving fixes from the second pass (strict 🐇 ✨ ✏️ Learnings added
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/app/login/login.component.ts (1)
147-150:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winVeto stale/trivial
returnUrls before navigating.A hand-crafted or stale
?returnUrl=/loginstill sends a successful sign-in back to the login screen here. Reuse the guard’s exact/loginfilter and fall back to/before callingnavigateByUrl.Suggested patch
- const returnUrl = this.activatedRoute.snapshot.queryParamMap.get('returnUrl'); - // Restore the originally requested deep link if the AuthenticationGuard captured one. - // Falls back to root (default landing) when no returnUrl is present. - this.router.navigateByUrl(returnUrl || '/', { replaceUrl: true }); + const rawReturnUrl = this.activatedRoute.snapshot.queryParamMap.get('returnUrl'); + const isLoginTarget = + rawReturnUrl === '/login' || rawReturnUrl?.startsWith('/login?') || rawReturnUrl?.startsWith('/login#'); + const returnUrl = rawReturnUrl && rawReturnUrl.startsWith('/') && !isLoginTarget ? rawReturnUrl : '/'; + this.router.navigateByUrl(returnUrl, { replaceUrl: true });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/login/login.component.ts` around lines 147 - 150, The login flow currently reads returnUrl from activatedRoute.snapshot.queryParamMap and navigates to it unguarded, which can redirect back to /login; validate and veto trivial or guarded paths before navigating by reusing the AuthenticationGuard's filter logic (e.g., treat '/login' and other non-deep-link paths as invalid), so in the login component after reading returnUrl from activatedRoute.snapshot.queryParamMap.get('returnUrl') check the value against the same check used in AuthenticationGuard (reject '/login' and empty/stale values), then set returnUrl = '/' when invalid and call this.router.navigateByUrl(returnUrl, { replaceUrl: true }).src/app/core/authentication/authentication.service.ts (1)
119-139:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftOAuth/OIDC tabs are still split across two token stores.
This cross-tab flow assumes a shared auth store, but
initializeOAuthService()can still leaveangular-oauth2-oidconsessionStoragewhilesetCredentials()writes the shared session tolocalStorage. In that state, a new tab can seemifosXCredentialsbut still failhasValidAccessToken()/getRefreshToken(), and passive-tab logout can leave the library’s active token store behind.#!/bin/bash set -euo pipefail FILE="src/app/core/authentication/authentication.service.ts" echo '--- Cross-tab auth listener ---' sed -n '119,139p' "$FILE" echo echo '--- OAuth storage initialization / restore path ---' sed -n '145,199p' "$FILE" echo echo '--- Shared credential persistence path ---' sed -n '484,498p' "$FILE" echo echo '--- Relevant OAuthService calls ---' rg -n 'setStorage\(|hasValidAccessToken\(|getRefreshToken\(|logOut\(' "$FILE"Also applies to: 486-498
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/core/authentication/authentication.service.ts` around lines 119 - 139, The cross-tab listener in listenForCrossTabAuthEvents mirrors credentials in localStorage but doesn't synchronize angular-oauth2-oidc's active storage, causing split token state; update the logout/login handling to also clear or reconfigure the OAuthService storage and tokens: on 'logout' call the OAuthService API to remove tokens (e.g., oauthService.logOut() or clear its storage) and ensure oauthService.setStorage(...) is set to the same Storage instance you clear (use the same credentialsStorageKey-backed storage), and on 'login' / restoreSession reinitialize OAuthService storage via initializeOAuthService() or oauthService.setStorage(...) before trying to rehydrate tokens so hasValidAccessToken()/getRefreshToken() see the shared store; ensure setCredentials(...) writes to whichever Storage instance oauthService is configured to use so tabs use a single canonical token store.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/app/core/authentication/authentication.service.ts`:
- Around line 119-139: The cross-tab listener in listenForCrossTabAuthEvents
mirrors credentials in localStorage but doesn't synchronize
angular-oauth2-oidc's active storage, causing split token state; update the
logout/login handling to also clear or reconfigure the OAuthService storage and
tokens: on 'logout' call the OAuthService API to remove tokens (e.g.,
oauthService.logOut() or clear its storage) and ensure
oauthService.setStorage(...) is set to the same Storage instance you clear (use
the same credentialsStorageKey-backed storage), and on 'login' / restoreSession
reinitialize OAuthService storage via initializeOAuthService() or
oauthService.setStorage(...) before trying to rehydrate tokens so
hasValidAccessToken()/getRefreshToken() see the shared store; ensure
setCredentials(...) writes to whichever Storage instance oauthService is
configured to use so tabs use a single canonical token store.
In `@src/app/login/login.component.ts`:
- Around line 147-150: The login flow currently reads returnUrl from
activatedRoute.snapshot.queryParamMap and navigates to it unguarded, which can
redirect back to /login; validate and veto trivial or guarded paths before
navigating by reusing the AuthenticationGuard's filter logic (e.g., treat
'/login' and other non-deep-link paths as invalid), so in the login component
after reading returnUrl from
activatedRoute.snapshot.queryParamMap.get('returnUrl') check the value against
the same check used in AuthenticationGuard (reject '/login' and empty/stale
values), then set returnUrl = '/' when invalid and call
this.router.navigateByUrl(returnUrl, { replaceUrl: true }).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0d45c5e-f77a-4ef7-8570-e4d04f34120a
📒 Files selected for processing (4)
src/app/core/authentication/authentication.guard.spec.tssrc/app/core/authentication/authentication.guard.tssrc/app/core/authentication/authentication.service.tssrc/app/login/login.component.ts
jonattan-infante
force-pushed
the
WEB-956/preserve-deep-link
branch
from
c6a651b to
214d114
Compare
Local CI parity run —
|
| Check | Status | Notes |
|---|---|---|
Single commit (commit-lint.yml) |
✅ | 1 commit (214d114) |
npm run lint (eslint + stylelint + prettier + htmlhint) |
✅ | clean |
headers:check (license-headers.yml) |
✅ | all changed files have MPL-2.0 header |
| Unit tests (jest) | ✅ | 7/7 specs in authentication.guard.spec.ts |
npm run build:prod |
✅ | bundle generated under budget |
Playwright E2E (playwright.yml) |
✅ 25/26 | see below |
About playwright/auth.setup.ts
This file currently reads credentials from sessionStorage and copies them to localStorage — a workaround that exists because of the bug this PR fixes. With AuthenticationService now writing to localStorage directly, the workaround would throw 'mifosXCredentials not found in sessionStorage' and break all authenticated specs.
I adapted it to a backwards-compatible check: prefer localStorage (post-fix), fall back to sessionStorage (pre-fix). No external behaviour change.
About the 1 failing E2E spec
close-client.spec.ts › should keep the client active when cancel or browser back is used from the close flow — fails consistently with element was detached from the DOM, retrying on actionsMenuItem.hover(). The interaction is unrelated to authentication (it's a Material Design 3 menu hover/click on the client view). I suspect it's environment-specific seed data sensitivity rather than a regression from this PR. Happy to dig in further if a maintainer thinks otherwise.
Force-pushed 214d114.
IOhacker
left a comment
IOhacker
left a comment
There was a problem hiding this comment.
Which authentication methods were used for the testing? 2FA? OAUTH? Basic? OIDC?
jonattan-infante
force-pushed
the
WEB-956/preserve-deep-link
branch
from
214d114 to
9f2544c
Compare
|
Both new findings from @coderabbitai are valid. Addressed in 1. Cross-tab OAuth/OIDC logout & storage consistency (Major)
2. Validate
|
| Check | Result |
|---|---|
Single commit (commit-lint.yml) |
✅ |
npm run lint |
✅ |
headers:check |
✅ |
| Unit tests (7/7) | ✅ |
npm run build:prod |
✅ |
| Playwright E2E | ✅ 25/26 (same unrelated close-client flake as before) |
Force-pushed 9f2544c.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 139 🏁 Script executed: Length of output: 34388
1. Cross-tab OAuth/OIDC logout & storage consistency
2. The
Anything that doesn't pass falls back safely to The PR is in great shape. Happy to approve whenever the maintainer team is ready. 🐇✨ [approve] |
Currently AuthenticationService persists the user credentials in
sessionStorage by default, which is scoped to a single browser tab. As
a result, any link to a Mifos URL opened from outside the current tab
— email, chat, bookmark, target="_blank", a new browser window — boots
into an empty storage and the AuthenticationGuard sends the
already-logged-in user back to /login. Compounding this,
AuthenticationGuard does not propagate the requested URL and
LoginComponent unconditionally navigates to "/" after a successful
login, so the originally requested deep link is permanently lost.
This change adopts the pattern used by `@supabase/auth-js`
(GoTrueClient.ts) and Auth0's SPA SDK: localStorage as the single
source of truth for the session, plus a BroadcastChannel for
cross-tab synchronisation. It also makes the guard forward the
target URL through the login flow.
- AuthenticationService.storage is now localStorage unconditionally,
so the session is visible to every tab/window of the same origin.
The `rememberMe` flag is preserved for the backend token-expiration
policy but no longer chooses the Storage instance.
- getSavedCredentials() prefers localStorage and performs a one-time
migration of any leftover sessionStorage credentials so existing
users do not get stranded after the upgrade.
- A BroadcastChannel named `mifosXAuth` is created lazily (with a
feature-detect fallback). Login broadcasts `{ type: 'login' }`,
logout broadcasts `{ type: 'logout' }`; the listener uses
hadPersistedCredentials to fire the login broadcast even though
onLoginSuccess() flips userLoggedIn$ before setCredentials() runs.
- The cross-tab logout handler now also clears the two-factor token
storage and the two-factor authorization header, mirroring what
logout() does on the active tab.
- AuthenticationGuard.canActivate now accepts RouterStateSnapshot and
forwards state.url as a returnUrl query parameter when the target
is a meaningful route. Trivial targets ("/", "/login") are skipped
to avoid noise and redirect loops.
- LoginComponent reads queryParamMap.get('returnUrl') from the
ActivatedRoute when the authentication success alert fires, and
navigates there via navigateByUrl. Falls back to "/" when no
returnUrl is present.
- Adds unit tests for the guard covering: authenticated pass-through,
deep-link capture, "/" and "/login" being skipped to avoid
loops/noise, stale-params handling, and URLs with query params
plus fragments.
Verified end-to-end against an isolated docker-compose stack
(postgres + fineract + web-app):
before: /#/clients -> /#/login -> /#/home (bug)
after: /#/clients -> /#/login?returnUrl=/clients -> /#/clients
cross-tab logout: tab A logs out -> tab B reacts live.
Addresses CodeRabbit review feedback:
- getSavedCredentials() now treats localStorage as authoritative and
migrates legacy sessionStorage credentials once.
- Cross-tab logout handler also clears 2FA state.
- Login broadcast is gated on persisted credentials, not on
userLoggedIn$, so a fresh login is correctly announced to other
tabs even though onLoginSuccess() sets userLoggedIn$ first.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
jonattan-infante
force-pushed
the
WEB-956/preserve-deep-link
branch
from
9f2544c to
89df79c
Compare
|
Hi @IOhacker — I covered Basic end-to-end. The other three modes I couldn't fully validate E2E and I'll explain why below. Setup: Basic: full Playwright suite passes against the upstream stack. OAuth2: asserted that the cross-tab logout properly clears the OAuth library tokens and that the storage choice is consistent. Token issuance and discovery endpoints work end-to-end, but a real login flow hits an upstream Fineract bug ( OIDC: same code path as OAuth2 in the web-app, covered the same way. Same upstream bug gates the live flow. 2FA: verified the cross-tab logout also tears down the 2FA state, and that the 2FA token is persisted before the login broadcast fires so passive tabs rehydrate complete. No live E2E since the upstream image doesn't ship a 2FA tenant. If you have a known-good OAuth/2FA environment to point at, I'll re-run there. |
2 participants
Summary
Fixes WEB-956.
When a logged-in user opens a Mifos URL in a new browser window (a link
from email/Slack, a bookmark,
target="_blank", etc.), the Web Appcurrently forces them to log in again — and then drops them on the home
dashboard instead of the URL they originally requested. The root cause
is twofold: credentials live in
sessionStorage(per-tab), and theauthentication guard / login component discard the target URL.
This PR adopts the pattern used by
@supabase/auth-jsand
@auth0/auth0-spa-js:localStorageas the single source of truth for the session, plus aBroadcastChannelfor cross-tab synchronisation. It also makes theauthentication guard preserve the target URL through the login flow.
Changes
authentication.service.ts— credentials are now persisted inlocalStorageunconditionally, so the authenticated session is visibleto every tab/window of the same origin. The
rememberMeflag ispreserved for the backend token-expiration policy but no longer chooses
the
Storageinstance.authentication.service.ts— aBroadcastChannelnamedmifosXAuthis created lazily (with a feature-detect fallback). Loginbroadcasts
{ type: 'login' }and logout broadcasts{ type: 'logout' }.Other tabs rehydrate or wipe their state in real time, with no reload
needed.
authentication.guard.ts—canActivatenow receivesRouterStateSnapshotand forwardsstate.urlas areturnUrlqueryparameter when redirecting to
/login. Trivial targets (/,/login,/login?...) are skipped to avoid noise and redirect loops.login.component.ts— after a successful authentication, thecomponent reads
returnUrlfrom the activated route'squeryParamMapand navigates there via
navigateByUrl, falling back to/whenabsent.
authentication.guard.spec.ts— adds 6 unit tests: authenticatedpass-through, deep-link capture, loop avoidance for
/and/login,stale-params handling, and URLs with query params + fragments.
Before / after
returnUrlpreserved in/loginURLTest plan
npx jest src/app/core/authentication/authentication.guard.spec.tsnpm run build(
postgres:17+ Fineract +openmf/web-app:mastervs local build)using Playwright. All four scenarios in the table above pass.
/#/clientsin a new window → no re-login./#/clients→ land back on/#/clientsafter login.restoreSessionfor non-Basic auth modes).Remember Mecheckbox still controls thebackend's
rememberflag (it does — only the client-sideStoragechoice was changed).
Notes for reviewers
sessionStorageis still cleared on logout for safety (bothlocalStorageandsessionStorageare wiped — seesetCredentials),so users that had a stale
sessionStoragefrom previous versions arenot affected after upgrading.
BroadcastChannelfeature-detect (typeof BroadcastChannel !== 'undefined')keeps the service usable in environments lacking the API (very old
browsers, some SSR contexts).
by NextAuth/Vercel) because it would require backend changes on
Fineract and is out of scope for this fix.
Summary by CodeRabbit
New Features
Bug Fixes
Tests