Add QA Auth utils - #98433
Conversation
|
Hey, I noticed you changed If you want to automatically generate translations for other locales, an Expensify employee will have to:
Alternatively, if you are an external contributor, you can run the translation script locally with your own OpenAI API key. To learn more, try running: npx ts-node ./scripts/generateTranslations.ts --helpTypically, you'd want to translate only what you changed by running |
JakubKorytko
left a comment
JakubKorytko
left a comment
There was a problem hiding this comment.
overall structure looks solid, four lifecycle and concurrency cases that can lose errors, restore credentials after cleanup, or invalidate a session from another tab
| if (!storage) { | ||
| return null; | ||
| } | ||
| const raw = storage.getItem(CONST.SESSION_STORAGE_KEYS.QA_AUTH_REDIRECT_FLOW); |
There was a problem hiding this comment.
shouldn't getItem and removeItem be inside a try/catch too? Accessing window.sessionStorage can succeed while these methods still throw SecurityError. That would crash app boot even though this function promises to return null for unreadable storage
| // Fire and forget; the catch only prevents an unhandled rejection. Callers joining via | ||
| // getPendingCloudflareAuthCompletion() still see the failure on their own handler. | ||
| completeCloudflareAuthRedirect({code, codeVerifier: flow.codeVerifier}).catch((error: unknown) => { | ||
| lastErrorMessage = error instanceof Error ? error.message : String(error); |
There was a problem hiding this comment.
could a rejected exchange update an observable outcome? Only the message changes, so lastOutcome remains exchanging. Once the pending promise is cleared, the next probe sees neither a completion nor a session and starts another redirect instead of surfacing this failure
| // Onyx.clear wipes the key but its callback is async, so drop the cache synchronously. Cache only: | ||
| // clearing the in-flight refs below wouldn't cancel the work, it would just let a second flight overlap. | ||
| registerSessionCleanupCallback(() => { | ||
| sessionCache = null; |
There was a problem hiding this comment.
could cleanup invalidate all in-flight auth work with a generation token? Clearing the cache does not stop pending PKCE generation, exchangeCode, or refreshTokens. If one resolves after sign-out, it can navigate the tab or write the old account's QA session back into Onyx
| if (staleAccessToken && current.accessToken !== staleAccessToken) { | ||
| return Promise.resolve('skipped-newer-token'); | ||
| } | ||
| refreshPromise = refreshTokens(current.refreshToken) |
There was a problem hiding this comment.
could refresh be coordinated across browser tabs? refreshPromise only deduplicates this JS context, while the Onyx session is shared through storage events and Cloudflare rotates refresh tokens. Two tabs can submit the same token; if one succeeds and the other gets invalid_grant, the losing tab can clear the winner's newly persisted session
2 participants
Explanation of Change
This PR adds QA auth utils. We will use them later to build authentication flow for web and mobile on NewDot.
Fixed Issues
$ #91419
PROPOSAL:
Tests
Offline tests
QA Steps
// TODO: These must be filled out, or the issue title must include "[No QA]."
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectionAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))npm run compress-svg)Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari