fix(auth): sign out only after definitive session expiry - #414
fix(auth): sign out only after definitive session expiry#414Alae-Tamouh wants to merge 1 commit into
Conversation
| // token surfaces as a 401 too. Only sign out when a forced refresh shows | ||
| // the session itself is unusable. (403 is authorization — admin gates — | ||
| // and never means expiry.) | ||
| if (response.status === 401 && !isSessionExpiryNotified()) { |
There was a problem hiding this comment.
Could we ensure simultaneous 401 responses share a single Cognito refresh request? Currently, each request may independently call resolveSession({ forceRefresh: true }), creating redundant refresh calls.
This currently creates redundant Cognito calls and could become problematic if refresh-token rotation is enabled later.
There was a problem hiding this comment.
It seems this concern is already addressed by the current amplify implementation used: https://github.com/aws-amplify/amplify-js/blob/3c7a96f9aa3d19c4f84dfee4a88a458e5438cbb5/packages/auth/src/providers/cognito/utils/refreshAuthTokens.ts#L74-L75
If it's the case, no need to reimplement it.
| } | ||
| }, | ||
|
|
||
| async getSession(): Promise<AuthSession | null> { |
There was a problem hiding this comment.
When the Cognito session has definitively expired, WebSocket and Yjs connections receive null from getSession() and keep retrying instead of logging the user out. The logout only happens after a REST request detects the expiry. Should realtime session checks also notify AuthContext when expiry is definitive?
There was a problem hiding this comment.
They should, I'll address it shortly
| expect(await screen.findByText('idle')).toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it('starts a new session epoch after successful SSO completion', async () => { |
There was a problem hiding this comment.
Could we add a test that starts authenticated, calls notifySessionExpired(), and verifies that the user is cleared and authService.logout() is called once? The bridge and API are tested separately, but the actual UI authentication transition is not.
There was a problem hiding this comment.
Agreed, Ill add it
jeromevdl
left a comment
There was a problem hiding this comment.
Great finding @Alae-Tamouh. I never faced this so thank you for your PR.
A few comments before approving.
Issue #, if available: None
Description of changes:
Problem
When Cognito can no longer refresh a session, API calls fail with 401 but AuthContext retains the previous user. The UI remains authenticated and continues issuing failing requests.
Logging out on every API 401 is unsafe because provider endpoints can also return GitHub, GitLab, Bitbucket, or Jira 401 responses while the Cognito session remains valid.
Reproducer
Solution
Classify session resolution as valid, definitively expired, or transiently unavailable. On an API 401, force a Cognito refresh before notifying AuthContext. A small epoch-aware bridge coalesces expiry notifications and prevents late requests from an old session from signing out a newly authenticated local or SSO session.
Impact
Expired users return cleanly to login. Network failures and provider-specific 401 responses no longer cause false logout, and stale requests cannot expire a replacement session.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.