fix(mobile): only log out on a genuinely rejected refresh token (port #2812)#2833
Open
Gilbert09 wants to merge 3 commits into
Open
fix(mobile): only log out on a genuinely rejected refresh token (port #2812)#2833Gilbert09 wants to merge 3 commits into
Gilbert09 wants to merge 3 commits into
Conversation
…2812) Mobile had its own OAuth refresh path that threw a generic Error on any non-ok response, so a transient network/server blip at startup deleted the tokens and signed the user out. Classify the refresh failure in `refreshAccessToken` and only sign out when the refresh token is genuinely dead: - 401/403, or 400 with `invalid_grant`/`invalid_token` -> auth_error -> sign out - 5xx -> server_error -> keep session - network failure -> network_error -> keep session - other 400s (e.g. invalid_client) -> unknown_error/config -> keep session `initializeAuth` now keeps the stored session on transient/config failures so the per-request `authedFetch` retry can recover, and only deletes tokens on a genuine auth_error. Ports desktop #2812 to apps/mobile. Generated-By: PostHog Code Task-Id: c274cae6-c3d7-46f4-bfe1-96b3514dbc94
|
React Doctor found no issues in the changed files. 🎉 Reviewed by React Doctor for commit |
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
apps/mobile/src/features/auth/stores/authStore.ts:370-372
The spread `{ ...CLEARED_AUTH_STATE }` in `logout` creates an identical shallow copy with no additions. `set` accepts a partial state directly, so the spread is superfluous here. The other two call sites correctly add `isLoading: false` to the spread, which is why the spread is needed there — but not here.
```suggestion
set(CLEARED_AUTH_STATE);
},
}),
```
Reviews (1): Last reviewed commit: "fix(mobile): only log out on a genuinely..." | Re-trigger Greptile |
… logout `set` accepts the partial state directly, so `set(CLEARED_AUTH_STATE)` is equivalent to spreading it with no additions. The two other call sites keep the spread because they add `isLoading: false`. Generated-By: PostHog Code Task-Id: c274cae6-c3d7-46f4-bfe1-96b3514dbc94
Generated-By: PostHog Code Task-Id: b11bf371-325f-4f8b-904d-0209ab60c582
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
Ports desktop #2812 ("fix(auth): Log out on rejected refresh token instead of stalling startup") to the React Native / Expo app at
apps/mobile.Mobile has its own OAuth refresh path (not the core
OAuthService).refreshAccessTokenthrew a genericErroron any non-ok response, andinitializeAuthdeleted the stored tokens and signed the user out on any thrown error. A transient network or server blip at startup therefore logged the user out unnecessarily — the exact bug #2812 guards against.Changes
lib/oauth.ts—refreshAccessTokennow classifies the failure and throws aTokenRefreshErrorcarrying anerrorCode, mirroring desktop's categories:401/403, or400withinvalid_grant/invalid_token→auth_error(token genuinely dead)5xx→server_error(transient, retryable)network_error400s (e.g.invalid_client/invalid_request, config bugs) →unknown_errorA small helper parses the OAuth error code from the JSON body and tolerates non-JSON bodies.
stores/authStore.ts—initializeAuthnow only deletes tokens / signs out onauth_error. On transient or config failures it keeps the stored session, so the next request's existing per-requestauthedFetchretry path (mobile fix(mobile): auto-refresh on PostHog 401/403 auth failures #2408) can recover. Also deduped the repeated auth-clearingset({...})payload into a sharedCLEARED_AUTH_STATE.The proactive
scheduleTokenRefreshhandler is intentionally left as-is: it already keeps the session on every error (it only logs), so it never had the bug, and logging out from a background timer on a still-valid access token would be a new behavior beyond this port.Tests
lib/oauth.test.ts(new) — covers the classification: 401/403 →auth_error, 400invalid_grant/invalid_token→auth_error, 400invalid_client/invalid_request→unknown_error, 5xx →server_error, thrown fetch →network_error.stores/authStore.test.ts—initializeAuthwith an expired token:auth_error→ tokens deleted + signed out;server_error/network_error/unknown_error→ session kept, no token deletion.pnpm --filter @posthog/mobile lintand the auth/api test suites pass.