Merge pull request #52727 from zirgulis/fix-reauthentication

neil-marcellini · web-flow · commit 8d69d60c1983 · 2024-11-22T09:46:14.000-05:00
[NoQA] Fix reauthentication
diff --git a/src/libs/Authentication.ts b/src/libs/Authentication.ts
@@ -62,55 +62,44 @@ function reauthenticate(command = ''): Promise<void> {
         partnerPassword: CONFIG.EXPENSIFY.PARTNER_PASSWORD,
         partnerUserID: credentials?.autoGeneratedLogin,
         partnerUserSecret: credentials?.autoGeneratedPassword,
-    })
-        .then((response) => {
-            if (response.jsonCode === CONST.JSON_CODE.UNABLE_TO_RETRY) {
-                // If authentication fails, then the network can be unpaused
-                NetworkStore.setIsAuthenticating(false);
+    }).then((response) => {
+        if (response.jsonCode === CONST.JSON_CODE.UNABLE_TO_RETRY) {
+            // When a fetch() fails due to a network issue and an error is thrown we won't log the user out. Most likely they
+            // have a spotty connection and will need to retry reauthenticate when they come back online. Error so it can be handled by the retry mechanism.
+            throw new Error('Unable to retry Authenticate request');
+        }
 
-                // When a fetch() fails due to a network issue and an error is thrown we won't log the user out. Most likely they
-                // have a spotty connection and will need to try to reauthenticate when they come back online. We will error so it
-                // can be handled by callers of reauthenticate().
-                throw new Error('Unable to retry Authenticate request');
-            }
-
-            // If authentication fails and we are online then log the user out
-            if (response.jsonCode !== 200) {
-                const errorMessage = ErrorUtils.getAuthenticateErrorMessage(response);
-                NetworkStore.setIsAuthenticating(false);
-                Log.hmmm('Redirecting to Sign In because we failed to reauthenticate', {
-                    command,
-                    error: errorMessage,
-                });
-                redirectToSignIn(errorMessage);
-                return;
-            }
+        // If authentication fails and we are online then log the user out
+        if (response.jsonCode !== 200) {
+            const errorMessage = ErrorUtils.getAuthenticateErrorMessage(response);
+            NetworkStore.setIsAuthenticating(false);
+            Log.hmmm('Redirecting to Sign In because we failed to reauthenticate', {
+                command,
+                error: errorMessage,
+            });
+            redirectToSignIn(errorMessage);
+            return;
+        }
 
-            // If we reauthenticated due to an expired delegate token, restore the delegate's original account.
-            // This is because the credentials used to reauthenticate were for the delegate's original account, and not for the account they were connected as.
-            if (Delegate.isConnectedAsDelegate()) {
-                Log.info('Reauthenticated while connected as a delegate. Restoring original account.');
-                Delegate.restoreDelegateSession(response);
-                return;
-            }
+        // If we reauthenticated due to an expired delegate token, restore the delegate's original account.
+        // This is because the credentials used to reauthenticate were for the delegate's original account, and not for the account they were connected as.
+        if (Delegate.isConnectedAsDelegate()) {
+            Log.info('Reauthenticated while connected as a delegate. Restoring original account.');
+            Delegate.restoreDelegateSession(response);
+            return;
+        }
 
-            // Update authToken in Onyx and in our local variables so that API requests will use the new authToken
-            updateSessionAuthTokens(response.authToken, response.encryptedAuthToken);
+        // Update authToken in Onyx and in our local variables so that API requests will use the new authToken
+        updateSessionAuthTokens(response.authToken, response.encryptedAuthToken);
 
-            // Note: It is important to manually set the authToken that is in the store here since any requests that are hooked into
-            // reauthenticate .then() will immediate post and use the local authToken. Onyx updates subscribers lately so it is not
-            // enough to do the updateSessionAuthTokens() call above.
-            NetworkStore.setAuthToken(response.authToken ?? null);
+        // Note: It is important to manually set the authToken that is in the store here since any requests that are hooked into
+        // reauthenticate .then() will immediate post and use the local authToken. Onyx updates subscribers lately so it is not
+        // enough to do the updateSessionAuthTokens() call above.
+        NetworkStore.setAuthToken(response.authToken ?? null);
 
-            // The authentication process is finished so the network can be unpaused to continue processing requests
-            NetworkStore.setIsAuthenticating(false);
-        })
-        .catch((error) => {
-            // In case the authenticate call throws error, we need to sign user out as most likely they are missing credentials
-            NetworkStore.setIsAuthenticating(false);
-            Log.hmmm('Redirecting to Sign In because we failed to reauthenticate', {error});
-            redirectToSignIn('passwordForm.error.fallback');
-        });
+        // The authentication process is finished so the network can be unpaused to continue processing requests
+        NetworkStore.setIsAuthenticating(false);
+    });
 }
 
 export {reauthenticate, Authenticate};
diff --git a/src/libs/Middleware/Reauthentication.ts b/src/libs/Middleware/Reauthentication.ts
@@ -1,33 +1,61 @@
+import redirectToSignIn from '@libs/actions/SignInRedirect';
 import * as Authentication from '@libs/Authentication';
 import Log from '@libs/Log';
 import * as MainQueue from '@libs/Network/MainQueue';
 import * as NetworkStore from '@libs/Network/NetworkStore';
+import type {RequestError} from '@libs/Network/SequentialQueue';
 import NetworkConnection from '@libs/NetworkConnection';
 import * as Request from '@libs/Request';
+import RequestThrottle from '@libs/RequestThrottle';
 import CONST from '@src/CONST';
 import type Middleware from './types';
 
 // We store a reference to the active authentication request so that we are only ever making one request to authenticate at a time.
 let isAuthenticating: Promise<void> | null = null;
 
+const reauthThrottle = new RequestThrottle('Re-authentication');
+
 function reauthenticate(commandName?: string): Promise<void> {
     if (isAuthenticating) {
         return isAuthenticating;
     }
 
-    isAuthenticating = Authentication.reauthenticate(commandName)
+    isAuthenticating = retryReauthenticate(commandName)
         .then((response) => {
-            isAuthenticating = null;
             return response;
         })
         .catch((error) => {
-            isAuthenticating = null;
             throw error;
+        })
+        .finally(() => {
+            isAuthenticating = null;
         });
 
     return isAuthenticating;
 }
 
+function retryReauthenticate(commandName?: string): Promise<void> {
+    return Authentication.reauthenticate(commandName).catch((error: RequestError) => {
+        return reauthThrottle
+            .sleep(error, 'Authenticate')
+            .then(() => retryReauthenticate(commandName))
+            .catch(() => {
+                NetworkStore.setIsAuthenticating(false);
+                Log.hmmm('Redirecting to Sign In because we failed to reauthenticate after multiple attempts', {error});
+                redirectToSignIn('passwordForm.error.fallback');
+            });
+    });
+}
+
+// Used in tests to reset the reauthentication state
+function resetReauthentication(): void {
+    // Resets the authentication state flag to allow new reauthentication flows to start fresh
+    isAuthenticating = null;
+
+    // Clears any pending reauth timeouts set by reauthThrottle.sleep()
+    reauthThrottle.clear();
+}
+
 const Reauthentication: Middleware = (response, request, isFromSequentialQueue) =>
     response
         .then((data) => {
@@ -118,3 +146,4 @@ const Reauthentication: Middleware = (response, request, isFromSequentialQueue)
         });
 
 export default Reauthentication;
+export {reauthenticate, resetReauthentication, reauthThrottle};
diff --git a/src/libs/Network/SequentialQueue.ts b/src/libs/Network/SequentialQueue.ts
@@ -2,7 +2,7 @@ import Onyx from 'react-native-onyx';
 import * as ActiveClientManager from '@libs/ActiveClientManager';
 import Log from '@libs/Log';
 import * as Request from '@libs/Request';
-import * as RequestThrottle from '@libs/RequestThrottle';
+import RequestThrottle from '@libs/RequestThrottle';
 import * as PersistedRequests from '@userActions/PersistedRequests';
 import * as QueuedOnyxUpdates from '@userActions/QueuedOnyxUpdates';
 import CONST from '@src/CONST';
@@ -28,6 +28,7 @@ resolveIsReadyPromise?.();
 let isSequentialQueueRunning = false;
 let currentRequestPromise: Promise<void> | null = null;
 let isQueuePaused = false;
+const sequentialQueueRequestThrottle = new RequestThrottle('SequentialQueue');
 
 /**
  * Puts the queue into a paused state so that no requests will be processed
@@ -99,7 +100,7 @@ function process(): Promise<void> {
 
             Log.info('[SequentialQueue] Removing persisted request because it was processed successfully.', false, {request: requestToProcess});
             PersistedRequests.endRequestAndRemoveFromQueue(requestToProcess);
-            RequestThrottle.clear();
+            sequentialQueueRequestThrottle.clear();
             return process();
         })
         .catch((error: RequestError) => {
@@ -108,17 +109,18 @@ function process(): Promise<void> {
             if (error.name === CONST.ERROR.REQUEST_CANCELLED || error.message === CONST.ERROR.DUPLICATE_RECORD) {
                 Log.info("[SequentialQueue] Removing persisted request because it failed and doesn't need to be retried.", false, {error, request: requestToProcess});
                 PersistedRequests.endRequestAndRemoveFromQueue(requestToProcess);
-                RequestThrottle.clear();
+                sequentialQueueRequestThrottle.clear();
                 return process();
             }
             PersistedRequests.rollbackOngoingRequest();
-            return RequestThrottle.sleep(error, requestToProcess.command)
+            return sequentialQueueRequestThrottle
+                .sleep(error, requestToProcess.command)
                 .then(process)
                 .catch(() => {
                     Onyx.update(requestToProcess.failureData ?? []);
                     Log.info('[SequentialQueue] Removing persisted request because it failed too many times.', false, {error, request: requestToProcess});
                     PersistedRequests.endRequestAndRemoveFromQueue(requestToProcess);
-                    RequestThrottle.clear();
+                    sequentialQueueRequestThrottle.clear();
                     return process();
                 });
         });
@@ -271,5 +273,19 @@ function waitForIdle(): Promise<unknown> {
     return isReadyPromise;
 }
 
-export {flush, getCurrentRequest, isRunning, isPaused, push, waitForIdle, pause, unpause, process};
+/**
+ * Clear any pending requests during test runs
+ * This is to prevent previous requests interfering with other tests
+ */
+function resetQueue(): void {
+    isSequentialQueueRunning = false;
+    currentRequestPromise = null;
+    isQueuePaused = false;
+    isReadyPromise = new Promise((resolve) => {
+        resolveIsReadyPromise = resolve;
+    });
+    resolveIsReadyPromise?.();
+}
+
+export {flush, getCurrentRequest, isRunning, isPaused, push, waitForIdle, pause, unpause, process, resetQueue, sequentialQueueRequestThrottle};
 export type {RequestError};
diff --git a/src/libs/Network/index.ts b/src/libs/Network/index.ts
@@ -6,14 +6,28 @@ import pkg from '../../../package.json';
 import * as MainQueue from './MainQueue';
 import * as SequentialQueue from './SequentialQueue';
 
+// React Native uses a number for the timer id, but Web/NodeJS uses a Timeout object
+let processQueueInterval: NodeJS.Timeout | number;
+
 // We must wait until the ActiveClientManager is ready so that we ensure only the "leader" tab processes any persisted requests
 ActiveClientManager.isReady().then(() => {
     SequentialQueue.flush();
 
     // Start main queue and process once every n ms delay
-    setInterval(MainQueue.process, CONST.NETWORK.PROCESS_REQUEST_DELAY_MS);
+    processQueueInterval = setInterval(MainQueue.process, CONST.NETWORK.PROCESS_REQUEST_DELAY_MS);
 });
 
+/**
+ * Clear any existing intervals during test runs
+ * This is to prevent previous intervals interfering with other tests
+ */
+function clearProcessQueueInterval() {
+    if (!processQueueInterval) {
+        return;
+    }
+    clearInterval(processQueueInterval);
+}
+
 /**
  * Perform a queued post request
  */
@@ -55,7 +69,4 @@ function post(command: string, data: Record<string, unknown> = {}, type = CONST.
     });
 }
 
-export {
-    // eslint-disable-next-line import/prefer-default-export
-    post,
-};
+export {post, clearProcessQueueInterval};
diff --git a/src/libs/RequestThrottle.ts b/src/libs/RequestThrottle.ts
@@ -3,41 +3,57 @@ import Log from './Log';
 import type {RequestError} from './Network/SequentialQueue';
 import {generateRandomInt} from './NumberUtils';
 
-let requestWaitTime = 0;
-let requestRetryCount = 0;
+class RequestThrottle {
+    private requestWaitTime = 0;
 
-function clear() {
-    requestWaitTime = 0;
-    requestRetryCount = 0;
-    Log.info(`[RequestThrottle] in clear()`);
-}
+    private requestRetryCount = 0;
+
+    private timeoutID?: NodeJS.Timeout;
+
+    private name: string;
 
-function getRequestWaitTime() {
-    if (requestWaitTime) {
-        requestWaitTime = Math.min(requestWaitTime * 2, CONST.NETWORK.MAX_RETRY_WAIT_TIME_MS);
-    } else {
-        requestWaitTime = generateRandomInt(CONST.NETWORK.MIN_RETRY_WAIT_TIME_MS, CONST.NETWORK.MAX_RANDOM_RETRY_WAIT_TIME_MS);
+    constructor(name: string) {
+        this.name = name;
     }
-    return requestWaitTime;
-}
 
-function getLastRequestWaitTime() {
-    return requestWaitTime;
-}
+    clear() {
+        this.requestWaitTime = 0;
+        this.requestRetryCount = 0;
+        if (this.timeoutID) {
+            Log.info(`[RequestThrottle - ${this.name}] clearing timeoutID: ${String(this.timeoutID)}`);
+            clearTimeout(this.timeoutID);
+            this.timeoutID = undefined;
+        }
+        Log.info(`[RequestThrottle - ${this.name}] cleared`);
+    }
 
-function sleep(error: RequestError, command: string): Promise<void> {
-    requestRetryCount++;
-    return new Promise((resolve, reject) => {
-        if (requestRetryCount <= CONST.NETWORK.MAX_REQUEST_RETRIES) {
-            const currentRequestWaitTime = getRequestWaitTime();
-            Log.info(
-                `[RequestThrottle] Retrying request after error: '${error.name}', '${error.message}', '${error.status}'. Command: ${command}. Retry count:  ${requestRetryCount}. Wait time: ${currentRequestWaitTime}`,
-            );
-            setTimeout(resolve, currentRequestWaitTime);
-            return;
+    getRequestWaitTime() {
+        if (this.requestWaitTime) {
+            this.requestWaitTime = Math.min(this.requestWaitTime * 2, CONST.NETWORK.MAX_RETRY_WAIT_TIME_MS);
+        } else {
+            this.requestWaitTime = generateRandomInt(CONST.NETWORK.MIN_RETRY_WAIT_TIME_MS, CONST.NETWORK.MAX_RANDOM_RETRY_WAIT_TIME_MS);
         }
-        reject();
-    });
+        return this.requestWaitTime;
+    }
+
+    getLastRequestWaitTime() {
+        return this.requestWaitTime;
+    }
+
+    sleep(error: RequestError, command: string): Promise<void> {
+        this.requestRetryCount++;
+        return new Promise((resolve, reject) => {
+            if (this.requestRetryCount <= CONST.NETWORK.MAX_REQUEST_RETRIES) {
+                const currentRequestWaitTime = this.getRequestWaitTime();
+                Log.info(
+                    `[RequestThrottle - ${this.name}] Retrying request after error: '${error.name}', '${error.message}', '${error.status}'. Command: ${command}. Retry count:  ${this.requestRetryCount}. Wait time: ${currentRequestWaitTime}`,
+                );
+                this.timeoutID = setTimeout(resolve, currentRequestWaitTime);
+            } else {
+                reject();
+            }
+        });
+    }
 }
 
-export {clear, getRequestWaitTime, sleep, getLastRequestWaitTime};
+export default RequestThrottle;
diff --git a/tests/actions/ReportTest.ts b/tests/actions/ReportTest.ts
@@ -55,7 +55,7 @@ describe('actions/Report', () => {
         const promise = Onyx.clear().then(jest.useRealTimers);
         if (getIsUsingFakeTimers()) {
             // flushing pending timers
-            // Onyx.clear() promise is resolved in batch which happends after the current microtasks cycle
+            // Onyx.clear() promise is resolved in batch which happens after the current microtasks cycle
             setImmediate(jest.runOnlyPendingTimers);
         }
 
diff --git a/tests/unit/APITest.ts b/tests/unit/APITest.ts
@@ -8,8 +8,8 @@ import HttpUtils from '@src/libs/HttpUtils';
 import * as MainQueue from '@src/libs/Network/MainQueue';
 import * as NetworkStore from '@src/libs/Network/NetworkStore';
 import * as SequentialQueue from '@src/libs/Network/SequentialQueue';
+import {sequentialQueueRequestThrottle} from '@src/libs/Network/SequentialQueue';
 import * as Request from '@src/libs/Request';
-import * as RequestThrottle from '@src/libs/RequestThrottle';
 import ONYXKEYS from '@src/ONYXKEYS';
 import type ReactNativeOnyxMock from '../../__mocks__/react-native-onyx';
 import * as TestHelper from '../utils/TestHelper';
@@ -47,6 +47,7 @@ beforeEach(() => {
     MainQueue.clear();
     HttpUtils.cancelPendingRequests();
     PersistedRequests.clear();
+    sequentialQueueRequestThrottle.clear();
     NetworkStore.checkRequiredData();
 
     // Wait for any Log command to finish and Onyx to fully clear
@@ -242,7 +243,7 @@ describe('APITests', () => {
 
                     // We let the SequentialQueue process again after its wait time
                     return new Promise((resolve) => {
-                        setTimeout(resolve, RequestThrottle.getLastRequestWaitTime());
+                        setTimeout(resolve, sequentialQueueRequestThrottle.getLastRequestWaitTime());
                     });
                 })
                 .then(() => {
@@ -255,7 +256,7 @@ describe('APITests', () => {
 
                     // We let the SequentialQueue process again after its wait time
                     return new Promise((resolve) => {
-                        setTimeout(resolve, RequestThrottle.getLastRequestWaitTime());
+                        setTimeout(resolve, sequentialQueueRequestThrottle.getLastRequestWaitTime());
                     }).then(waitForBatchedUpdates);
                 })
                 .then(() => {
diff --git a/tests/unit/NetworkTest.ts b/tests/unit/NetworkTest.ts

Original file line number	Diff line number	Diff line change
`@@ -55,7 +55,7 @@ describe('actions/Report', () => {`
`55`	`55`	`const promise = Onyx.clear().then(jest.useRealTimers);`
`56`	`56`	`if (getIsUsingFakeTimers()) {`
`57`	`57`	`// flushing pending timers`
`58`		`- // Onyx.clear() promise is resolved in batch which happends after the current microtasks cycle`
	`58`	`+ // Onyx.clear() promise is resolved in batch which happens after the current microtasks cycle`
`59`	`59`	`setImmediate(jest.runOnlyPendingTimers);`
`60`	`60`	`}`
`61`	`61`