fix(auth): improve popup closure detection for third-party login (#2763)

rodrigo-fournier-immutable · web-flow · commit a19e3b48d84f · 2025-12-15T02:07:22.000Z
diff --git a/packages/auth/src/Auth.test.ts b/packages/auth/src/Auth.test.ts
@@ -186,21 +186,11 @@ describe('Auth', () => {
     });
   });
 
-  describe('loginWithPopup - popup closure detection', () => {
+  describe('loginWithPopup', () => {
     let mockUserManager: any;
-    let mockPopupWindow: any;
-    let originalWindowOpen: any;
     let originalCryptoRandomUUID: any;
 
     beforeEach(() => {
-      // Mock window.open to return a controllable popup reference
-      originalWindowOpen = window.open;
-      mockPopupWindow = {
-        closed: false,
-        close: jest.fn(),
-      };
-      window.open = jest.fn().mockReturnValue(mockPopupWindow);
-
       // Mock crypto.randomUUID
       originalCryptoRandomUUID = window.crypto.randomUUID;
       window.crypto.randomUUID = jest.fn().mockReturnValue('test-popup-id');
@@ -212,17 +202,13 @@ describe('Auth', () => {
           extraQueryParams: {},
         },
       };
-
-      jest.useFakeTimers();
     });
 
     afterEach(() => {
-      window.open = originalWindowOpen;
       window.crypto.randomUUID = originalCryptoRandomUUID;
-      jest.useRealTimers();
     });
 
-    it('resolves successfully when authentication completes before popup closes', async () => {
+    it('successfully completes authentication and returns user', async () => {
       const mockOidcUser = {
         id_token: 'token',
         access_token: 'access',
@@ -236,12 +222,7 @@ describe('Auth', () => {
         passport: undefined,
       });
 
-      // Simulate successful authentication that resolves quickly
-      mockUserManager.signinPopup.mockImplementation(() => new Promise((resolve) => {
-        setTimeout(() => {
-          resolve(mockOidcUser);
-        }, 100);
-      }));
+      mockUserManager.signinPopup.mockResolvedValue(mockOidcUser);
 
       const auth = Object.create(Auth.prototype) as Auth;
       (auth as any).userManager = mockUserManager;
@@ -250,60 +231,17 @@ describe('Auth', () => {
       };
       getDetailMock.mockReturnValue('runtime-id-value');
 
-      // Pass directLoginOptions to skip embeddedLoginPrompt
-      const loginPromise = (auth as any).loginWithPopup({
+      const user = await (auth as any).loginWithPopup({
         directLoginMethod: 'google',
         marketingConsentStatus: 'opted_in',
       });
 
-      // Fast-forward time to complete authentication
-      jest.advanceTimersByTime(150);
-
-      const user = await loginPromise;
-
       expect(user).toBeDefined();
       expect(user.profile.sub).toBe('user-123');
-      expect(mockPopupWindow.close).toHaveBeenCalled();
-    });
-
-    it('rejects with error when popup is closed manually before authentication completes', async () => {
-      // Simulate authentication that takes a long time
-      mockUserManager.signinPopup.mockImplementation(() => new Promise((resolve) => {
-        setTimeout(() => {
-          resolve({
-            id_token: 'token',
-            access_token: 'access',
-            refresh_token: 'refresh',
-            expired: false,
-            profile: { sub: 'user-123', email: 'test@example.com', nickname: 'tester' },
-          });
-        }, 5000); // Takes 5 seconds
-      }));
-
-      const auth = Object.create(Auth.prototype) as Auth;
-      (auth as any).userManager = mockUserManager;
-      (auth as any).config = {
-        popupOverlayOptions: { disableHeadlessLoginPromptOverlay: true },
-      };
-      getDetailMock.mockReturnValue('runtime-id-value');
-
-      // Pass directLoginOptions to skip embeddedLoginPrompt
-      const loginPromise = (auth as any).loginWithPopup({
-        directLoginMethod: 'google',
-        marketingConsentStatus: 'opted_in',
-      });
-
-      // Simulate user closing popup after 1 second
-      jest.advanceTimersByTime(1000);
-      mockPopupWindow.closed = true;
-
-      // Advance time to trigger the polling check
-      jest.advanceTimersByTime(500);
-
-      await expect(loginPromise).rejects.toThrow('Popup closed by user');
+      expect(user.profile.email).toBe('test@example.com');
     });
 
-    it('does not reject when popup closes after authentication completes successfully', async () => {
+    it('calls signinPopup with correct configuration', async () => {
       const mockOidcUser = {
         id_token: 'token',
         access_token: 'access',
@@ -317,12 +255,7 @@ describe('Auth', () => {
         passport: undefined,
       });
 
-      // Simulate successful authentication
-      mockUserManager.signinPopup.mockImplementation(() => new Promise((resolve) => {
-        setTimeout(() => {
-          resolve(mockOidcUser);
-        }, 100);
-      }));
+      mockUserManager.signinPopup.mockResolvedValue(mockOidcUser);
 
       const auth = Object.create(Auth.prototype) as Auth;
       (auth as any).userManager = mockUserManager;
@@ -331,41 +264,24 @@ describe('Auth', () => {
       };
       getDetailMock.mockReturnValue('runtime-id-value');
 
-      // Pass directLoginOptions to skip embeddedLoginPrompt
-      const loginPromise = (auth as any).loginWithPopup({
+      await (auth as any).loginWithPopup({
         directLoginMethod: 'google',
         marketingConsentStatus: 'opted_in',
       });
 
-      // Complete authentication
-      jest.advanceTimersByTime(150);
-
-      // Now close the popup AFTER auth completed
-      mockPopupWindow.closed = true;
-
-      // Advance polling interval - should NOT reject
-      jest.advanceTimersByTime(600);
-
-      const user = await loginPromise;
-      expect(user).toBeDefined();
-      expect(user.profile.sub).toBe('user-123');
+      expect(mockUserManager.signinPopup).toHaveBeenCalledWith(
+        expect.objectContaining({
+          popupWindowTarget: 'test-popup-id',
+          popupWindowFeatures: { width: 410, height: 450 },
+          popupAbortOnClose: true,
+          extraQueryParams: expect.any(Object),
+        }),
+      );
     });
 
-    it('stops polling after authentication completes', async () => {
-      const mockOidcUser = {
-        id_token: 'token',
-        access_token: 'access',
-        refresh_token: 'refresh',
-        expired: false,
-        profile: { sub: 'user-123', email: 'test@example.com', nickname: 'tester' },
-      };
-
-      (decodeJwtPayload as jest.Mock).mockReturnValue({
-        username: 'username123',
-        passport: undefined,
-      });
-
-      mockUserManager.signinPopup.mockResolvedValue(mockOidcUser);
+    it('rejects when signinPopup rejects', async () => {
+      const error = new Error('Authentication failed');
+      mockUserManager.signinPopup.mockRejectedValue(error);
 
       const auth = Object.create(Auth.prototype) as Auth;
       (auth as any).userManager = mockUserManager;
@@ -374,26 +290,10 @@ describe('Auth', () => {
       };
       getDetailMock.mockReturnValue('runtime-id-value');
 
-      const setIntervalSpy = jest.spyOn(global, 'setInterval');
-      const clearIntervalSpy = jest.spyOn(global, 'clearInterval');
-
-      // Pass directLoginOptions to skip embeddedLoginPrompt
-      const loginPromise = (auth as any).loginWithPopup({
+      await expect((auth as any).loginWithPopup({
         directLoginMethod: 'google',
         marketingConsentStatus: 'opted_in',
-      });
-
-      // Let authentication complete
-      await Promise.resolve();
-      jest.runAllTimers();
-
-      await loginPromise;
-
-      // Verify that clearInterval was called (timer stopped)
-      expect(clearIntervalSpy).toHaveBeenCalled();
-
-      setIntervalSpy.mockRestore();
-      clearIntervalSpy.mockRestore();
+      })).rejects.toThrow('Authentication failed');
     });
   });
 });
diff --git a/packages/auth/src/Auth.ts b/packages/auth/src/Auth.ts
@@ -41,7 +41,6 @@ import { isAccessTokenExpiredOrExpiring } from './utils/token';
 import LoginPopupOverlay from './overlay/loginPopupOverlay';
 import { LocalForageAsyncStorage } from './storage/LocalForageAsyncStorage';
 
-const LOGIN_POPUP_CLOSED_POLLING_DURATION = 500;
 const formUrlEncodedHeaders = {
   'Content-Type': 'application/x-www-form-urlencoded',
 };
@@ -499,7 +498,7 @@ export class Auth {
       const signinPopup = async () => {
         const extraQueryParams = this.buildExtraQueryParams(directLoginOptionsToUse, imPassportTraceId);
 
-        const userPromise = this.userManager.signinPopup({
+        return this.userManager.signinPopup({
           extraQueryParams,
           popupWindowFeatures: {
             width: 410,
@@ -509,41 +508,6 @@ export class Auth {
           // Enable oidc-client-ts native popup close detection (works for initial screen)
           popupAbortOnClose: true,
         });
-
-        // Additional polling workaround to detect popup closure during navigation
-        // (e.g., when user navigates to third-party login, passwordless, or captcha screens)
-        // This complements oidc-client-ts native detection which only checks once at start
-        const popupRef = window.open('', popupWindowTarget);
-        if (popupRef) {
-          // Flag to track if authentication completed (success or failure)
-          let authenticationCompleted = false;
-
-          // Create a promise that rejects when popup is closed
-          const popupClosedPromise = new Promise<never>((_, reject) => {
-            const timer = setInterval(() => {
-              // Only reject if popup closed AND authentication hasn't completed yet
-              if (popupRef.closed && !authenticationCompleted) {
-                clearInterval(timer);
-                reject(new Error('Popup closed by user'));
-              }
-            }, LOGIN_POPUP_CLOSED_POLLING_DURATION);
-
-            // Clean up timer when the user promise resolves/rejects
-            userPromise.finally(() => {
-              authenticationCompleted = true;
-              clearInterval(timer);
-              // Close popup if still open (e.g., auth completed but popup didn't auto-close)
-              if (!popupRef.closed) {
-                popupRef.close();
-              }
-            });
-          });
-
-          // Race between user authentication and popup being closed
-          return Promise.race([userPromise, popupClosedPromise]);
-        }
-
-        return userPromise;
       };
 
       return new Promise((resolve, reject) => {
