NP-51314 Fix redirect on public pages when token expires#8568
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
iBuzza
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/utils/urlPaths.ts`:
- Around line 103-110: isPublicPage is using broad startsWith/endsWith checks
that mark protected routes (e.g. UrlPathTemplate.RegistrationNew, /projects/new,
/projects/:id/edit) as public; update the logic in isPublicPage to use stricter
matching: for UrlPathTemplate.ResearchProfileRoot and
UrlPathTemplate.ProjectsRoot only treat the exact root or exactly one-segment
public views (e.g. /projects or /projects/:identifier) as public (reject any
further segments like "new" or "edit"), and replace the
registrationLandingPageParts startsWith/endsWith check with an explicit check
for the allowed registration landing path(s) rather than a prefix/suffix
exclusion; locate isPublicPage and modify the predicate to use exact equality or
a one-segment regex/pattern for those symbols
(UrlPathTemplate.ResearchProfileRoot, UrlPathTemplate.ProjectsRoot,
registrationLandingPageParts) to prevent protected subroutes from being
classified public.
🪄 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: a4f361c9-e05b-4162-bfe0-dec43298cdb3
📒 Files selected for processing (6)
src/api/authApi.tssrc/api/hooks/useIsAuthenticated.tssrc/components/registration-landing-page/RegistrationLandingPage.tsxsrc/layout/Logout.tsxsrc/utils/constants.tssrc/utils/urlPaths.ts
iBuzza
requested review from
Eiriknil,
denektenina and
joachimjorgensen
as code owners
| // Don't redirect to SignedOut when viewing public content (e.g. a registration landing page); | ||
| // the request simply proceeds unauthenticated instead of kicking the user off a public page. |
There was a problem hiding this comment.
Ikke så nyttig kommentar i mine øyne
| // Don't redirect to SignedOut when viewing public content (e.g. a registration landing page); | |
| // the request simply proceeds unauthenticated instead of kicking the user off a public page. |
There was a problem hiding this comment.
Jeg liker kommentaren, den sier noe om at det løser et faktisk problem vi har hatt, heller enn at det er "overbeskyttende" som det kan være lett å tenke
| // Keep the simulated-expired state consistent across full-page reloads (e.g. after a redirect to | ||
| // SignedOut): without this the Redux `user` would be repopulated from the still-valid real token, | ||
| // causing SignedOutPage to bounce back to Root in a redirect loop. |
There was a problem hiding this comment.
Ikke så nyttig kommentar i mine øyne. Blir fort stale denne om man endrer på Redux user etc.
| // Keep the simulated-expired state consistent across full-page reloads (e.g. after a redirect to | |
| // SignedOut): without this the Redux `user` would be repopulated from the still-valid real token, | |
| // causing SignedOutPage to bounce back to Root in a redirect loop. |
There was a problem hiding this comment.
Jeg syns denne også er nyttig. Det fungerer som dokumentasjon for featuren som simulateExpiredToken faktisk er. Mulig den (også?) skulle vært dokumentert på en egen side da.
| // Gate the curator branch on a live token, not just the (possibly stale) Redux user, so that an | ||
| // expired token doesn't trigger a failing authenticated tickets request on this public page. | ||
| const isAuthenticatedQuery = useIsAuthenticated(); | ||
| const isTicketCurator = hasTicketCuratorRole(user) && isAuthenticatedQuery.data === true; |
There was a problem hiding this comment.
Burde dette vært bakt inn i hjelpefunksjonen? Er det noen ganger man ønsker å vite om en bruker er kurator, men ikke verifisere at bruker er autentisert?
There was a problem hiding this comment.
Jeg tenker de er greie å ha separate, som her. Jeg har ikke satt meg veldig inn i use casene, men jeg tror ikke de alltid trenger å sjekkes sammen, siden mange sider er for autentiserte brukere i sin helhet.
| Beta = 'beta', | ||
| EnvironmentBanner = 'environmentBanner', | ||
| RedirectPath = 'redirectPath', | ||
| /** Dev-only: when set to 'true', the app behaves as if the session token has expired. Compiled out of production builds. */ |
There was a problem hiding this comment.
| /** Dev-only: when set to 'true', the app behaves as if the session token has expired. Compiled out of production builds. */ |
denektenina
left a comment
denektenina
left a comment
There was a problem hiding this comment.
Approvet men les over spesielt kommentaren min om mulig misforståelse av hva useIsAuthenticated gjør.
| export const isPublicPage = (path: string) => { | ||
| const pathname = path.split('?')[0]; | ||
| if (protectedPageTemplates.some((template) => matchPath(template, pathname))) { | ||
| return false; | ||
| } | ||
| return publicPageTemplates.some((template) => matchPath(template, pathname)); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Kunne vært lurt med tester på isPublicPage siden det kan være vanskelig å se av koden hvordan den forholder seg til ulike routes, og dessuten for å sikre at vi ikke endrer funksjonalitet ved en feil hvis den endres senere.
| /** | ||
| * Reactively reports whether the current session has a valid (non-expired) token. | ||
| * Unlike the Redux `user`, which is populated once at startup and goes stale when the token | ||
| * expires, this reflects the live token state and can be used to gate authenticated requests. | ||
| */ | ||
| export const useIsAuthenticated = () => |
There was a problem hiding this comment.
Jeg er litt bekymret for at navn og kommentar gjør det lett å misforstå hva denne hooken gjør. Hvis man ser noe slikt i koden:
const isAuthenticatedQuery = useIsAuthenticated(); vil man kanskje tro at man gjør en "live" sjekk på nåværende status på tokenet, men av det jeg har lest virker det som den bare gjør dette kallet ved mounting (første render) og når siden får fokus igjen.
For denne bugfixen det er bra nok funksjonalitet, men kanskje kommentaren burde endres til noe ala:
"Checks whether the current session has a valid (non-expired) token. Updates at mount-time and on window focus. Can be used to gate authenticated requests."
eller
"Gate the curator branch on the token state at mount time, not just the (possibly stale) Redux user — this prevents a failing authenticated request when navigating to a public page with an expired token."
eller noe annet bedre som du kommer på.
| // Don't redirect to SignedOut when viewing public content (e.g. a registration landing page); | ||
| // the request simply proceeds unauthenticated instead of kicking the user off a public page. |
There was a problem hiding this comment.
Jeg liker kommentaren, den sier noe om at det løser et faktisk problem vi har hatt, heller enn at det er "overbeskyttende" som det kan være lett å tenke
| // Keep the simulated-expired state consistent across full-page reloads (e.g. after a redirect to | ||
| // SignedOut): without this the Redux `user` would be repopulated from the still-valid real token, | ||
| // causing SignedOutPage to bounce back to Root in a redirect loop. |
There was a problem hiding this comment.
Jeg syns denne også er nyttig. Det fungerer som dokumentasjon for featuren som simulateExpiredToken faktisk er. Mulig den (også?) skulle vært dokumentert på en egen side da.
| // Gate the curator branch on a live token, not just the (possibly stale) Redux user, so that an | ||
| // expired token doesn't trigger a failing authenticated tickets request on this public page. | ||
| const isAuthenticatedQuery = useIsAuthenticated(); | ||
| const isTicketCurator = hasTicketCuratorRole(user) && isAuthenticatedQuery.data === true; |
There was a problem hiding this comment.
Jeg tenker de er greie å ha separate, som her. Jeg har ikke satt meg veldig inn i use casene, men jeg tror ikke de alltid trenger å sjekkes sammen, siden mange sider er for autentiserte brukere i sin helhet.
iBuzza
dismissed stale reviews from denektenina and coderabbitai[bot]
via
f88f389
3 participants
Description
Link to Jira issue: NP-51314
When a user's session token expired while they were viewing a public page (e.g. a published registration landing page, search, a research profile, a project), the app would redirect them to the
SignedOutpage. This was disruptive because those pages don't require authentication — the user was kicked off content they were allowed to see.The root cause was twofold:
getAccessTokenunconditionally redirected toSignedOutwhenever no valid token was present, even on public pages.That triggered an authenticated tickets request that failed.
After this change:
getAccessTokenskips theSignedOutredirect when the current page is public, returningnullso the request proceeds unauthenticated instead. On protected pages the redirect still happens as before.isPublicPage) was extracted fromLogout.tsxintourlPaths.ts(deduped) and rewritten to match against actual route templates viamatchPath. This correctly keeps protected routes like/registration(new),/projects/new, and/projects/:id/editnon-public — broad prefix/suffix matching had wrongly classified them as public — and also handles paths with query strings (e.g. /search?query=...).RegistrationLandingPagenow gates the curator branch on a live token via the newuseIsAuthenticatedhook instead of the possibly-stale Reduxuser, so an expired token no longer fires a failing authenticated tickets request on this public page.A dev-only
simulateExpiredTokenswitch was added to reproduce an expired session in the GUI without touchingCognito. It is compiled out of production builds viaimport.meta.env.DEV.How to test
Affected part of the application: a published registration landing page, e.g. /registration/ (and other public pages: /search, /research-profile/, /projects/).
This requires the dev-only token-expiry simulation, so run the app locally (npm run dev / npm start).
Reproduce the bug (without this branch) / verify the fix (with it):
localStorage.setItem('simulateExpiredToken', 'true');
- Expected (this branch): you stay on the public page and can read its content. No redirect to SignedOut, and no failing authenticated tickets request in the
network tab.
- Expected: you are redirected to SignedOut (with a redirect path so re-login returns you there).
localStorage.removeItem('simulateExpiredToken');
- Expected: authenticated behaviour returns to normal (curator actions/tickets load again on the landing page).
Checklist
Ensure that the changes are aligned with the expectations of PO/designer before marking the PR as ready for review.
Also ensure that the following criterias are met:
Note: some of these criterias may not always be relevant, and can simply be marked as completed.