New: [AEA-6292] - Authentication redirect & blocking render improvements#1865
Open
connoravo-nhs wants to merge 87 commits intomainfrom
Open
New: [AEA-6292] - Authentication redirect & blocking render improvements#1865connoravo-nhs wants to merge 87 commits intomainfrom
connoravo-nhs wants to merge 87 commits intomainfrom
Conversation
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
…en blocking conditions Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
… concurrent on primary session on select role page Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
…on, as blocked rn Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
…-tracker-ui into AEA-6292
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
…s have a test Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
connoravo-nhs
commented
Mar 5, 2026
| tokenMapping.cis2IdToken !== undefined && | ||
| tokenMapping.cis2ExpiresIn !== undefined && | ||
| tokenMapping.lastActivityTime !== undefined && | ||
| tokenMapping.lastActivityTime > Date.now() - fifteenMinutes |
Contributor
Author
There was a problem hiding this comment.
A strange case where a token wasn't fully "cleaned" from the database, it existed as a user entry but no further information. This was presenting as a concurrent session to users who weren't logged in elsewhere.
connoravo-nhs
commented
Mar 5, 2026
| function checkIfValidTokenMapping(tokenMapping: TokenMappingItem | undefined): boolean { | ||
| const fifteenMinutes = 15 * 60 * 1000 | ||
|
|
||
| return tokenMapping !== undefined && |
Contributor
Author
There was a problem hiding this comment.
A strange case where a token wasn't fully "cleaned" from the database, it existed as a user entry but no further information. This was presenting as a concurrent session to users who weren't logged in elsewhere.
connoravo-nhs
commented
Mar 5, 2026
| return ( | ||
| <AccessContext.Provider value={{}}> | ||
| {children} | ||
| {shouldBlockChildren() ? <Layout><LoadingPage /></Layout> : children} |
Contributor
Author
There was a problem hiding this comment.
Children shown should always satisfy shouldBlockChildren within the Access Provider. Having it seperately could lead to potential race conditions.
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Manipulate lock rather than idempotent logout Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
…l/eps-prescription-tracker-ui into aea-5884-comments-on-modal
…cause multi-tab issues Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
…-parser Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors auth/logout flow and render-blocking logic to reduce unwanted redirects and effect execution during auth transitions, introduces cross-tab logout coordination, and tightens Cognito token-mapping validation to mitigate eventual-consistency edge cases.
Changes:
- Centralises sign-out handling via
handleSignoutEvent+ adds a localStorage “logout marker” and per-tab IDs to coordinate cross-tab logout behavior. - Reworks AccessProvider render-blocking/redirect logic and migrates session-timeout modal state into AuthProvider-backed storage.
- Updates Cognito token/tokenMock lambdas to treat only “complete” token mappings as valid for concurrent-session decisions.
Reviewed changes
Copilot reviewed 54 out of 54 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/cpt-ui/src/styles/EpsModal.scss | Modal sizing adjustments for desktop/mobile. |
| packages/cpt-ui/src/pages/SessionSelection.tsx | Switches logout handling to shared signout helper. |
| packages/cpt-ui/src/pages/PrescriptionListPage.tsx | Uses shared signout helper for restart-login / cancellation cases. |
| packages/cpt-ui/src/pages/PrescriptionDetailsPage.tsx | Uses shared signout helper for 401 restart-login. |
| packages/cpt-ui/src/pages/LogoutPage.tsx | Updates logout flow + render conditions during signout. |
| packages/cpt-ui/src/pages/LoginPage.tsx | Extracts sign-in logic into helper and uses shared signout helper. |
| packages/cpt-ui/src/pages/LoadingPage.tsx | Adds test id for loading header. |
| packages/cpt-ui/src/pages/BasicDetailsSearchResultsPage.tsx | Uses shared signout helper for 401 restart-login. |
| packages/cpt-ui/src/hooks/useSessionTimeout.ts | Moves timeout actions to auth-backed state and shared signout helper. |
| packages/cpt-ui/src/helpers/tabHelpers.ts | Adds per-tab ID and open-tabs tracking helpers. |
| packages/cpt-ui/src/helpers/logout.tsx | Adds logout marker logic + unified signout entrypoint. |
| packages/cpt-ui/src/helpers/loginFunctions.tsx | Adds handleSignIn helper; reuses signOut on failure. |
| packages/cpt-ui/src/helpers/getSearchParams.tsx | Adds OAuth error query param parsing. |
| packages/cpt-ui/src/helpers/axios.tsx | Throws explicit error when Cognito token missing. |
| packages/cpt-ui/src/context/AuthProvider.tsx | Stores session-timeout modal state + logout modal type in localStorage-backed auth context. |
| packages/cpt-ui/src/context/AccessProvider.tsx | Reworks redirect + render-blocking logic; session timeout modal driven via auth context state. |
| packages/cpt-ui/src/constants/ui-strings/YourSelectedRoleStrings.ts | Text casing tweak. |
| packages/cpt-ui/src/constants/ui-strings/SessionTimeoutModalStrings.ts | Updates timeout modal copy. |
| packages/cpt-ui/src/constants/environment.ts | Adds logout marker + tab tracking storage keys. |
| packages/cpt-ui/src/components/SessionTimeoutModal.tsx | Adds countdown + aria-live announcements and uses auth-backed state. |
| packages/cpt-ui/src/components/RBACBanner.tsx | Prevents banner rendering when not signed in / missing user details. |
| packages/cpt-ui/src/components/EpsRoleSelectionPage.tsx | Uses shared signout helper on auth failures. |
| packages/cpt-ui/src/components/EpsModal.tsx | Adds aria-describedby plumbing. |
| packages/cpt-ui/src/components/EpsLogoutModal.tsx | Adds disabled state to confirm button. |
| packages/cpt-ui/src/components/EpsHeader.tsx | Uses auth-backed “logout modal type” and shared signout helper; disables action buttons during logout. |
| packages/cpt-ui/src/App.tsx | Drives session timeout modal from auth context + new timeout hook. |
| packages/cpt-ui/jest.setup.ts | Extends environment mock for new constants/paths. |
| packages/cpt-ui/tests/useSessionTimeout.test.tsx | Updates hook tests for new auth-backed flow. |
| packages/cpt-ui/tests/tabHelpers.test.ts | Adds tests for new tab helpers. |
| packages/cpt-ui/tests/mocks/AuthStateMock.tsx | Extends auth mock for new auth context fields. |
| packages/cpt-ui/tests/logout.test.tsx | Adds tests for new logout helpers. |
| packages/cpt-ui/tests/axios.test.tsx | Updates expected error message on missing token. |
| packages/cpt-ui/tests/SessionTimeoutModal.test.tsx | Adds tests for updated timeout modal behavior/ARIA. |
| packages/cpt-ui/tests/SearchPrescriptionPagePaths.test.tsx | Updates AccessContext usage after AccessProvider refactor. |
| packages/cpt-ui/tests/SearchPrescriptionPageKeyboard.test.tsx | Updates AccessContext usage after AccessProvider refactor. |
| packages/cpt-ui/tests/SearchPrescriptionPageInputDetection.test.tsx | Updates AccessContext usage after AccessProvider refactor. |
| packages/cpt-ui/tests/SearchPrescriptionPageCoverage.test.tsx | Updates AccessContext usage after AccessProvider refactor. |
| packages/cpt-ui/tests/RBACBanner.test.tsx | Updates mocks for new RBACBanner signed-in guard. |
| packages/cpt-ui/tests/LogoutPage.test.tsx | Updates mocks for PUBLIC_PATHS and async user-info update. |
| packages/cpt-ui/tests/EpsRoleSelectionPage.test.tsx | Updates expectations to use handleSignoutEvent. |
| packages/cpt-ui/tests/EpsLogoutModal.test.tsx | Adds tests for new disabled confirm button behavior. |
| packages/cpt-ui/tests/EpsHeader.test.tsx | Updates provider setup for AccessContext changes. |
| packages/cpt-ui/tests/CookiePolicyPage.test.tsx | Removes now-unneeded environment mock. |
| packages/cpt-ui/tests/AuthProvider.test.tsx | Updates signOut error-handling expectations. |
| packages/cpt-ui/tests/App.test.tsx | Updates mocked contexts for App timeout modal changes. |
| packages/cpt-ui/tests/AccessProvider.test.tsx | Updates tests for new redirect/block/session-timeout behavior. |
| packages/cpt-ui/tests/AccessProvider.shouldBlockChildren.test.tsx | Adds targeted tests for render-blocking logic. |
| packages/cpt-ui/tests/AccessProvider.ensureRoleSelected.test.tsx | Adds targeted tests for redirect logic. |
| packages/common/commonTypes/src/sessionTimeoutModalState.ts | Adds shared type for session-timeout modal state. |
| packages/common/commonTypes/src/index.ts | Re-exports new shared modal type. |
| packages/cognito/tests/test_token.mock.test.ts | Updates mock token mapping shape to include “complete” token data. |
| packages/cognito/tests/test_token.cis2.test.ts | Updates CIS2 token mapping mocks for “complete” token data and boundary conditions. |
| packages/cognito/src/tokenMock.ts | Validates token mapping completeness before treating session as active. |
| packages/cognito/src/token.ts | Validates token mapping completeness before treating session as active. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Comment on lines
228
to
231
| } | ||
|
|
||
| logger.debug("No conditions met - not checking user info") | ||
| return |
Comment on lines
+132
to
+145
| if (isOpen && timeLeft > 0) { | ||
| // Only start if not already running or if starting fresh | ||
| if (!countdownTimerRef.current) { | ||
| let secondsLeft = Math.floor(timeLeft / 1000) | ||
|
|
||
| // Set initial time | ||
| auth.setSessionTimeoutModalInfo(prev => ({...prev, timeLeft: secondsLeft})) | ||
|
|
||
| // Start countdown that decrements every second | ||
| countdownTimerRef.current = setInterval(() => { | ||
| secondsLeft -= 1 | ||
|
|
||
| auth.setSessionTimeoutModalInfo(prev => ({...prev, timeLeft: secondsLeft})) | ||
| // Auto-logout when countdown reaches 0 |
Comment on lines
+71
to
+78
| // Initialize aria-live region when modal first opens | ||
| useEffect(() => { | ||
| const shouldInitialize = isOpen && timeLeft > 0 && liveRegionRef.current | ||
| if (shouldInitialize) { | ||
| const minutes = Math.floor(timeLeft / 60) | ||
| const seconds = timeLeft % 60 | ||
| const announcement = formatTimeAnnouncement(minutes, seconds) | ||
| updateLiveRegion(liveRegionRef, announcement) |
Comment on lines
163
to
167
| <EpsModal | ||
| isOpen={isOpen} | ||
| ariaLabelledBy="session-timeout-title" | ||
| ariaDescribedBy="session-timeout-title" | ||
| onClose={onStayLoggedIn} |
Comment on lines
+22
to
+75
| const handleStayLoggedIn = useCallback(async () => { | ||
| // Prevent multiple simultaneous extension attempts or cross-calls | ||
| if (actionLockRef.current !== undefined) { | ||
| logger.info("Session action already in progress, ignoring duplicate request") | ||
| return | ||
| } | ||
| }, []) | ||
|
|
||
| const handleTimeoutLogout = useCallback(async () => { | ||
| logger.warn("Session expired - automatically logging out user") | ||
| clearCountdownTimer() | ||
| await props.onTimeout() | ||
| }, [props.onTimeout, clearCountdownTimer]) | ||
|
|
||
| // Effect to start/stop countdown based on modal visibility | ||
| useEffect(() => { | ||
| if (props.showModal && props.timeLeft > 0) { | ||
| // Only start if not already running or if starting fresh | ||
| if (!countdownTimerRef.current) { | ||
| let secondsLeft = Math.floor(props.timeLeft / 1000) | ||
|
|
||
| // Set initial time | ||
| setSessionState(prev => ({ | ||
| ...prev, | ||
| timeLeft: secondsLeft | ||
| })) | ||
|
|
||
| // Start countdown that decrements every second | ||
| countdownTimerRef.current = setInterval(() => { | ||
| secondsLeft -= 1 | ||
| try { | ||
| actionLockRef.current = "extending" | ||
| logger.info("User chose to extend session") | ||
| auth.setSessionTimeoutModalInfo(prev => ({...prev, action: "extending", buttonDisabled: true})) | ||
|
|
||
| setSessionState(prev => ({ | ||
| ...prev, | ||
| timeLeft: secondsLeft | ||
| })) | ||
| // Call the selectedRole API with current role to refresh session | ||
| if (auth.selectedRole) { | ||
| await updateRemoteSelectedRole(auth.selectedRole) | ||
| logger.info("Session extended successfully") | ||
|
|
||
| // Auto-logout when countdown reaches 0 | ||
| if (secondsLeft <= 0) { | ||
| clearInterval(countdownTimerRef.current!) | ||
| countdownTimerRef.current = null | ||
| handleTimeoutLogout() | ||
| } | ||
| }, 1000) as unknown as number | ||
| // Hide modal and refresh user info | ||
| auth.setLogoutModalType(undefined) | ||
| auth.setSessionTimeoutModalInfo( | ||
| prev => ({...prev, showModal: false, timeLeft: 0, buttonDisabled: false, action: undefined})) | ||
| actionLockRef.current = undefined | ||
| await auth.updateTrackerUserInfo() | ||
| } else { | ||
| logger.error("No selected role available to extend session") | ||
| auth.setSessionTimeoutModalInfo(prev => ({...prev, action: "loggingOut", buttonDisabled: true})) | ||
| await handleLogOut() | ||
| } | ||
| } else { | ||
| // Clear timer when modal is hidden | ||
| clearCountdownTimer() | ||
| } catch (error) { | ||
| logger.error("Error extending session:", error) | ||
| auth.setSessionTimeoutModalInfo(prev => ({...prev, action: "loggingOut", buttonDisabled: true})) | ||
| await handleLogOut() | ||
| } | ||
| }, [auth]) | ||
|
|
||
| // Cleanup on unmount | ||
| return clearCountdownTimer | ||
| }, [props.showModal]) // Only depend on showModal, not timeLeft | ||
|
|
||
| const handleExtendSession = useCallback(async () => { | ||
| setSessionState(prev => ({...prev, isExtending: true})) | ||
| try { | ||
| await props.onStayLoggedIn() | ||
| setSessionState(prev => ({...prev, isExtending: false})) | ||
| } catch { | ||
| setSessionState(prev => ({...prev, isExtending: false})) | ||
| await props.onLogOut() | ||
| const handleLogOut = useCallback(async () => { | ||
| // Prevent multiple simultaneous logout attempts or cross-calls | ||
| if (actionLockRef.current !== undefined) { | ||
| logger.info("Session action already in progress, ignoring duplicate request") | ||
| return | ||
| } | ||
| }, [props]) | ||
| actionLockRef.current = "loggingOut" | ||
| logger.info("User chose to log out from session timeout modal") | ||
| auth.setSessionTimeoutModalInfo(prev => ({...prev, action: "loggingOut", buttonDisabled: true})) | ||
| await handleSignoutEvent(auth, navigate, "Timeout") | ||
| auth.setLogoutModalType(undefined) | ||
| }, [auth]) | ||
|
|
||
| const handleTimeout = useCallback(async () => { | ||
| logger.warn("Session automatically timed out") | ||
| clearCountdownTimer() | ||
| auth.updateInvalidSessionCause("Timeout") | ||
| await handleSignoutEvent(auth, navigate, "Timeout") | ||
| }, [auth]) |
Comment on lines
+123
to
+125
| if (isRecentMarker(existingMarker)) { | ||
| logger.info("Existing market is recent", existingMarker) | ||
| return existingMarker |
| cis2IdToken: idToken, | ||
| cis2ExpiresIn: decodedIdToken.exp.toString(), | ||
| selectedRoleId: decodedIdToken.selected_roleid, | ||
| currentlySelectedRole: decodedIdToken.selected_roleid, |
| sessionId: "session-id", | ||
| cis2AccessToken: "foo", | ||
| cis2IdToken: "bar", | ||
| cis2ExpiresIn: Date.now() - - (10 * 60 * 1000) |
Comment on lines
18
to
20
| if (auth.isSignedIn || auth.isSigningIn) { | ||
| signOut(auth, AUTH_CONFIG.REDIRECT_SIGN_OUT) | ||
| handleSignoutEvent(auth, navigate, "LogoutPage", AUTH_CONFIG.REDIRECT_SIGN_OUT) | ||
| } else if (auth.isSigningOut) { |
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
… back on logout page Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
Signed-off-by: Connor Avery <214469360+connoravo-nhs@users.noreply.github.com>
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Details
Aims to resolve the bugs identified in:
Changes made:
The ensureRoleSelected function has been somewhat re-written to ensure hierarchical order is correct & that if statements are "looser" fitting to negative scenarios. Public paths shouldn't be affected by redirections apart from root and login.
The shouldBlockChildren function has also had additional conditions added, that will ensure a render block occurs, so that on-page useEffects don't run.
The shouldBlockChildren now dictates within AccessContext.Provider return, whether the children or loading page should be shown. This is believed to resolve an issue where re-rendering wasn't occurring in the event of a state change if a user was on the loading page in an auth transition state.
Token lambdas now check for a 'complete' token information in the DynamoDb table, to prevent the cause of eventual consistency of a session logout, where a dynamoDb row remains with skeleton information and no actual credentials.