Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/ONYXKEYS.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ const ONYXKEYS = {
/** Boolean flag set whenever we are searching for reports in the server */
RAM_ONLY_IS_SEARCHING_FOR_REPORTS: 'isSearchingForReports',

/** Boolean flag indicating a SignInWithShortLivedAuthToken request is in flight. RAM-only so an interrupted request never persists a stuck `true` to IndexedDB and blocks future reauth attempts. */
RAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKEN: 'isAuthenticatingWithShortLivedToken',

/** Note: These are Persisted Requests - not all requests in the main queue as the key name might lead one to believe */
PERSISTED_REQUESTS: 'networkRequestQueue',
PERSISTED_ONGOING_REQUESTS: 'networkOngoingRequestQueue',
Expand Down Expand Up @@ -1557,6 +1560,7 @@ type OnyxValuesMapping = {
[ONYXKEYS.ONBOARDING_ADMINS_CHAT_REPORT_ID]: string;
[ONYXKEYS.ONBOARDING_LAST_VISITED_PATH]: string;
[ONYXKEYS.RAM_ONLY_IS_SEARCHING_FOR_REPORTS]: boolean;
[ONYXKEYS.RAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKEN]: boolean;
[ONYXKEYS.LAST_VISITED_PATH]: string | undefined;
[ONYXKEYS.REPORT_LAST_VISIT_TIMES]: OnyxTypes.ReportLastVisitTimes;
[ONYXKEYS.RECENTLY_USED_REPORT_FIELDS]: OnyxTypes.RecentlyUsedReportFields;
Expand Down
12 changes: 10 additions & 2 deletions src/libs/AppState/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import type {ExtraLoadingContext, GlobalStateSnapshot, NavigationStateInfo, Netw

let currentSession: OnyxEntry<Session>;
let currentNetwork: OnyxEntry<Network>;
let currentIsAuthenticatingWithShortLivedToken = false;

// We have opted for connectWithoutView here as this is strictly non-UI and only for logging.
Onyx.connectWithoutView({
Expand All @@ -29,6 +30,14 @@ Onyx.connectWithoutView({
},
});

// We have opted for connectWithoutView here as this is strictly non-UI and only for logging.
Onyx.connectWithoutView({
key: ONYXKEYS.RAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKEN,
callback: (value) => {
currentIsAuthenticatingWithShortLivedToken = !!value;
},
});

/**
* Captures current navigation state.
*/
Expand All @@ -54,12 +63,11 @@ function captureNavigationState(): NavigationStateInfo {
function captureSessionState(): SessionStateInfo {
// Check multiple authentication states to get complete picture
const isSessionLoading = !!currentSession?.loading;
const isAuthenticatingWithShortLivedToken = !!currentSession?.isAuthenticatingWithShortLivedToken;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we update the app version and the user has isAuthenticatingWithShortLivedToken in the session and not in the ram only key? should we temporarily coalesce both values here to cover this case and then later remove the key from the session?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think coalescing them would actually re-trap the stuck users :)

the whole point of moving the flag to RAM-only is that we stop reading session.isAuthenticatingWithShortLivedToken. that persisted field IS the bug. currently-stuck users have it persisted as true in IndexedDB and the legacy code reads it back on every reload, blocking every reauth attempt. by ignoring it in the new code, they get unblocked on the next reload because the RAM-only key starts undefined.

if we OR both values here, a stuck user's session.isAuthenticatingWithShortLivedToken=true would still evaluate to true and reauth would keep aborting, which is exactly what we are trying to escape from.

the second unit test (reauthenticate proceeds even when a legacy session.isAuthenticatingWithShortLivedToken=true is persisted) covers this app-upgrade scenario: legacy stuck session field + undefined RAM-only key -> reauth proceeds normally.

also fwiw, this AppState file is just the diagnostic log captured when ActivityIndicator hangs. it isnt the actual reauth abort path, that lives in Reauthentication.ts. the log reflecting the new RAM-only state is correct (it shows what reauth actually reads, not the dead legacy value).

happy to add a follow-up cleanup migration to delete the leftover session.isAuthenticatingWithShortLivedToken from IndexedDB if you want it gone, but the fix here doesnt depend on it. WDYT?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah I think we can leave it

const isAuthenticatingFromNetworkStore = isAuthenticatingNetworkStore();

return {
isSessionLoading,
isAuthenticatingWithShortLivedToken,
isAuthenticatingWithShortLivedToken: currentIsAuthenticatingWithShortLivedToken,
isAuthenticatingFromNetworkStore,
};
}
Expand Down
9 changes: 8 additions & 1 deletion src/libs/Reauthentication.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,6 @@ let isSupportAuthTokenUsed = false;
Onyx.connectWithoutView({
key: ONYXKEYS.SESSION,
callback: (value) => {
isAuthenticatingWithShortLivedToken = !!value?.isAuthenticatingWithShortLivedToken;
isSupportAuthTokenUsed = !!value?.isSupportAuthTokenUsed;

Sentry.setUser({
Expand All @@ -46,6 +45,14 @@ Onyx.connectWithoutView({
},
});

// Kept on a RAM-only key so an interrupted SignIn cannot persist a stuck `true` to IndexedDB and block all future reauth attempts.
Onyx.connectWithoutView({
key: ONYXKEYS.RAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKEN,
callback: (value) => {
isAuthenticatingWithShortLivedToken = !!value;
},
});

let account: OnyxEntry<Account>;
// Authentication lib is not connected to any changes on the UI
// So it is okay to use connectWithoutView here.
Expand Down
19 changes: 15 additions & 4 deletions src/libs/actions/Session/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,9 @@ function setSupportAuthToken(supportAuthToken: string, email: string, accountID:
}

function getShortLivedLoginParams(isSupportAuthTokenUsed = false, isSAML = false) {
const optimisticData: Array<OnyxUpdate<typeof ONYXKEYS.ACCOUNT | typeof ONYXKEYS.SESSION | typeof ONYXKEYS.HYBRID_APP>> = [
const optimisticData: Array<
OnyxUpdate<typeof ONYXKEYS.ACCOUNT | typeof ONYXKEYS.SESSION | typeof ONYXKEYS.HYBRID_APP | typeof ONYXKEYS.RAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKEN>
> = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.ACCOUNT,
Expand All @@ -171,14 +173,19 @@ function getShortLivedLoginParams(isSupportAuthTokenUsed = false, isSAML = false
value: {
signedInWithShortLivedAuthToken: true,
signedInWithSAML: isSAML,
isAuthenticatingWithShortLivedToken: true,
isSupportAuthTokenUsed,
},
},
// Kept on a RAM-only key so an interrupted SignIn cannot persist a stuck `true` to IndexedDB and block all future reauth attempts.
{
onyxMethod: Onyx.METHOD.SET,
key: ONYXKEYS.RAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKEN,
value: true,
},
];

// Subsequently, we revert it back to the default value of 'signedInWithShortLivedAuthToken' in 'finallyData' to ensure the user is logged out on refresh
const finallyData: Array<OnyxUpdate<typeof ONYXKEYS.ACCOUNT | typeof ONYXKEYS.SESSION>> = [
const finallyData: Array<OnyxUpdate<typeof ONYXKEYS.ACCOUNT | typeof ONYXKEYS.SESSION | typeof ONYXKEYS.RAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKEN>> = [
{
onyxMethod: Onyx.METHOD.MERGE,
key: ONYXKEYS.ACCOUNT,
Expand All @@ -193,9 +200,13 @@ function getShortLivedLoginParams(isSupportAuthTokenUsed = false, isSAML = false
signedInWithShortLivedAuthToken: null,
signedInWithSAML: isSAML,
isSupportAuthTokenUsed: null,
isAuthenticatingWithShortLivedToken: false,
},
},
{
onyxMethod: Onyx.METHOD.SET,
key: ONYXKEYS.RAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKEN,
value: false,
},
];

const failureData: Array<OnyxUpdate<typeof ONYXKEYS.HYBRID_APP>> = [];
Expand Down
1 change: 1 addition & 0 deletions src/setup/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ export default function () {
ONYXKEYS.RAM_ONLY_UPDATE_AVAILABLE,
ONYXKEYS.RAM_ONLY_UPDATE_REQUIRED,
ONYXKEYS.RAM_ONLY_IS_SEARCHING_FOR_REPORTS,
ONYXKEYS.RAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKEN,
ONYXKEYS.RAM_ONLY_WALLET_ONFIDO,
ONYXKEYS.COLLECTION.RAM_ONLY_REPORT_LOADING_STATE,
ONYXKEYS.RAM_ONLY_PLAID_LINK_TOKEN,
Expand Down
3 changes: 0 additions & 3 deletions src/types/onyx/Session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,6 @@ type Session = {
/** User signed in with short lived token */
signedInWithShortLivedAuthToken?: boolean;

/** Indicates whether the user is re-authenticating with shortLivedToken */
isAuthenticatingWithShortLivedToken?: boolean;

/** User signed in with SAML */
signedInWithSAML?: boolean;

Expand Down
37 changes: 37 additions & 0 deletions tests/actions/SessionTest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,43 @@ describe('Session', () => {
redirectToSignInSpy.mockRestore();
});

test('reauthenticate aborts when RAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKEN is true', async () => {
// Given a SignIn with short lived token is currently in flight
await Onyx.set(ONYXKEYS.RAM_ONLY_IS_AUTHENTICATING_WITH_SHORT_LIVED_TOKEN, true);
await waitForBatchedUpdates();

const redirectToSignInSpy = jest.spyOn(SignInRedirect, 'default').mockImplementation(() => Promise.resolve());

// When reauthenticate is called
const result = await reauthenticate('TestCommand');
await waitForBatchedUpdates();

// Then it aborts cleanly without redirecting to sign in
expect(result).toBe(false);
expect(redirectToSignInSpy).not.toHaveBeenCalled();

redirectToSignInSpy.mockRestore();
});

test('reauthenticate proceeds even when a legacy session.isAuthenticatingWithShortLivedToken=true is persisted (recovers stuck users)', async () => {
// Given a session in Onyx that still carries the legacy stuck flag from before the RAM-only migration.
// The Session type no longer declares the field, so cast to write the legacy shape.
await Onyx.merge(ONYXKEYS.SESSION, {isAuthenticatingWithShortLivedToken: true} as unknown as Session);
await waitForBatchedUpdates();

const redirectToSignInSpy = jest.spyOn(SignInRedirect, 'default').mockImplementation(() => Promise.resolve());

// When reauthenticate is called with no credentials stored
const result = await reauthenticate('TestCommand');
await waitForBatchedUpdates();

// Then the legacy persisted flag does NOT block reauth. Reauth proceeds, finds no credentials, and redirects to sign in.
expect(result).toBe(false);
expect(redirectToSignInSpy).toHaveBeenCalledWith('No credentials available');

redirectToSignInSpy.mockRestore();
});

test('Authenticate is called with saved credentials when a session expires', async () => {
// Given a test user and set of authToken with subscriptions to session and credentials
const TEST_USER_LOGIN = 'test@testguy.com';
Expand Down
Loading