Dashboard: window auth loop counter and rename to dashboard-auth-loop stat

Author: p-jackson

client/dashboard/app/auth/index.tsx

@@ -24,25 +24,53 @@ const BOOTSTRAP_ERROR_MESSAGE = 'Failed to bootstrap user object';
 
 const AUTH_BOUNCE_COUNT_KEY = 'wpcom_auth_bounce_count';
 
-function trackAuthBounce() {
+const AUTH_LOOP_WINDOW_MS = 10 * 1000;
+
+interface AuthBounceRecord {
+	count: number;
+	at: number;
+}
+
+// bumpStat when we have a login redirect loop.
+// We track the time of the last bounce, and if it was within a window we count
+// it towards our loop count. This is more reliable than clearing the counter on
+// successful auth, because how do we know when it is safe to clear the count?
+// It could be that immediately after successful auth, the very next API returns
+// 401 and causes a bounce, yet we would have already cleared the count.
+function trackAuthBounceLoop() {
 	try {
-		const count = Number( window.sessionStorage.getItem( AUTH_BOUNCE_COUNT_KEY ) ) + 1;
-		window.sessionStorage.setItem( AUTH_BOUNCE_COUNT_KEY, String( count ) );
+		const now = Date.now();
+		const storedRecord: unknown = JSON.parse(
+			window.sessionStorage.getItem( AUTH_BOUNCE_COUNT_KEY ) ?? 'null'
+		);
+		const previousRecord = isAuthBounceRecord( storedRecord ) ? storedRecord : null;
+		const withinWindow = previousRecord !== null && now - previousRecord.at < AUTH_LOOP_WINDOW_MS;
+		const count = withinWindow ? previousRecord.count + 1 : 1;
+
+		window.sessionStorage.setItem(
+			AUTH_BOUNCE_COUNT_KEY,
+			JSON.stringify( { count, at: now } satisfies AuthBounceRecord )
+		);
 
 		if ( count >= 2 ) {
-			bumpStat( 'dashboard-auth', 'loop' );
+			bumpStat( 'dashboard-auth-loop', String( count ) );
 		}
 	} catch {
-		// sessionStorage can be unavailable in private contexts.
+		// sessionStorage can be unavailable in private contexts or JSON.parse may fail.
 	}
 }
 
-function clearAuthBounceCount() {
-	try {
-		window.sessionStorage.removeItem( AUTH_BOUNCE_COUNT_KEY );
-	} catch {
-		// sessionStorage can be unavailable in private contexts.
-	}
+// Checks that what is stored in sessionStorage matches the AuthBounceRecord
+// shape we expect.
+function isAuthBounceRecord( value: unknown ): value is AuthBounceRecord {
+	return (
+		typeof value === 'object' &&
+		value !== null &&
+		'count' in value &&
+		typeof value.count === 'number' &&
+		'at' in value &&
+		typeof value.at === 'number'
+	);
 }
 
 function getOAuthAuthorizeUrl( {
@@ -161,7 +189,7 @@ export function AuthProvider( { children }: { children: React.ReactNode } ) {
 			authErrorHandled.current = true;
 
 			bumpStat( 'dashboard-auth', `bounce:${ reason }` );
-			trackAuthBounce();
+			trackAuthBounceLoop();
 
 			if ( config.isEnabled( 'oauth' ) ) {
 				const state = crypto.randomUUID();
@@ -227,7 +255,6 @@ export function AuthProvider( { children }: { children: React.ReactNode } ) {
 			if ( ! successStatBumped.current ) {
 				successStatBumped.current = true;
 				bumpStat( 'dashboard-auth', shouldUseBootstrap() ? 'success:bootstrap' : 'success:fetch' );
-				clearAuthBounceCount();
 			}
 		}
 	}, [ user ] );

client/dashboard/app/auth/test/index.test.tsx

@@ -109,29 +109,36 @@ describe( '<AuthProvider> stats', () => {
 		await waitFor( () =>
 			expect( mockedBumpStat ).toHaveBeenCalledWith( 'dashboard-auth', 'bounce:bootstrap' )
 		);
-		expect( mockedBumpStat ).not.toHaveBeenCalledWith( 'dashboard-auth', 'loop' );
+		expect( mockedBumpStat ).not.toHaveBeenCalledWith( 'dashboard-auth-loop', expect.anything() );
 	} );
 
-	test( 'bumps a loop stat when a bounce repeats without a successful auth in between', async () => {
+	test( 'bumps a loop stat with the bounce count when a bounce repeats within the loop window', async () => {
 		config.enable( 'wpcom-user-bootstrap' );
-		window.sessionStorage.setItem( 'wpcom_auth_bounce_count', '1' );
+		window.sessionStorage.setItem(
+			'wpcom_auth_bounce_count',
+			JSON.stringify( { count: 1, at: Date.now() } )
+		);
 
 		renderAuth();
 
-		await waitFor( () => expect( mockedBumpStat ).toHaveBeenCalledWith( 'dashboard-auth', 'loop' ) );
+		await waitFor( () =>
+			expect( mockedBumpStat ).toHaveBeenCalledWith( 'dashboard-auth-loop', '2' )
+		);
 	} );
 
-	test( 'resets the bounce count when auth succeeds', async () => {
+	test( 'does not bump a loop stat when the previous bounce is outside the loop window', async () => {
 		config.enable( 'wpcom-user-bootstrap' );
-		window.currentUser = testUser;
-		window.sessionStorage.setItem( 'wpcom_auth_bounce_count', '1' );
+		window.sessionStorage.setItem(
+			'wpcom_auth_bounce_count',
+			JSON.stringify( { count: 5, at: Date.now() - 60 * 1000 } )
+		);
 
 		renderAuth();
 
-		expect( await screen.findByText( 'signed in' ) ).toBeVisible();
 		await waitFor( () =>
-			expect( window.sessionStorage.getItem( 'wpcom_auth_bounce_count' ) ).toBeNull()
+			expect( mockedBumpStat ).toHaveBeenCalledWith( 'dashboard-auth', 'bounce:bootstrap' )
 		);
+		expect( mockedBumpStat ).not.toHaveBeenCalledWith( 'dashboard-auth-loop', expect.anything() );
 	} );
 
 	test( 'bumps a bounce stat when the session expires mid-app', async () => {