From b66a019bc36b3bcec393b2a8db35d0e2684210b5 Mon Sep 17 00:00:00 2001 From: Chirag Madlani <12962843+chirag-madlani@users.noreply.github.com> Date: Sun, 23 Feb 2025 19:20:21 +0530 Subject: [PATCH] fix(ui): forever loading for oidc auth refresh failure (#19854) * fix(ui): forever loading for oidc auth refresh failure * fix redirect login issue with iframe added tests for iframe vs popup login added tests for clearing localState for refresh api failures --- .../MsalAuthenticator.test.tsx | 176 ++++++++++-- .../AppAuthenticators/MsalAuthenticator.tsx | 13 +- .../OidcAuthenticator.test.tsx | 251 ++++++++++++++++++ .../AppAuthenticators/OidcAuthenticator.tsx | 3 + 4 files changed, 412 insertions(+), 31 deletions(-) create mode 100644 openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OidcAuthenticator.test.tsx diff --git a/openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/MsalAuthenticator.test.tsx b/openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/MsalAuthenticator.test.tsx index ac2036d510e..8ec4da1fa0d 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/MsalAuthenticator.test.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/MsalAuthenticator.test.tsx @@ -10,42 +10,160 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - -import { render, screen } from '@testing-library/react'; +import { InteractionStatus } from '@azure/msal-browser'; +import { useMsal } from '@azure/msal-react'; +import { act, render, screen } from '@testing-library/react'; import React from 'react'; -import { MemoryRouter } from 'react-router-dom'; +import { msalLoginRequest } from '../../../utils/AuthProvider.util'; +import { AuthenticatorRef } from '../AuthProviders/AuthProvider.interface'; import MsalAuthenticator from './MsalAuthenticator'; -jest.mock('../../../hooks/useApplicationStore', () => { - return { - useApplicationStore: jest.fn(() => ({ - authConfig: {}, - getOidcToken: jest.fn(), - setOidcToken: jest.fn(), - })), - }; -}); +// Mock MSAL hooks and utilities +jest.mock('@azure/msal-react', () => ({ + useMsal: jest.fn(), + useAccount: jest.fn(), +})); + +jest.mock('../../../utils/AuthProvider.util', () => ({ + msalLoginRequest: { + scopes: ['test.scope'], + }, + parseMSALResponse: jest.fn().mockImplementation((response) => ({ + id_token: 'mock-id-token', + ...response, + })), +})); + +const mockInstance = { + loginPopup: jest.fn(), + loginRedirect: jest.fn(), + handleRedirectPromise: jest.fn(), + ssoSilent: jest.fn(), +}; + +const mockProps = { + children:
Test Children
, + onLoginSuccess: jest.fn(), + onLoginFailure: jest.fn(), + onLogoutSuccess: jest.fn(), +}; + +describe('MsalAuthenticator', () => { + let authenticatorRef: AuthenticatorRef | null = null; + + beforeEach(() => { + jest.clearAllMocks(); + // Default mock implementation for useMsal + (useMsal as jest.Mock).mockReturnValue({ + instance: mockInstance, + accounts: [{ username: 'test@example.com' }], + inProgress: InteractionStatus.None, + }); + }); + + it('should handle login in iframe using popup', async () => { + // Mock window.self !== window.top for iframe detection + Object.defineProperty(window, 'self', { + value: { location: {} }, + writable: true, + }); + Object.defineProperty(window, 'top', { + value: { location: {} }, + writable: true, + }); + + mockInstance.loginPopup.mockResolvedValueOnce({ + account: { username: 'test@example.com' }, + }); -describe('Test MsalAuthenticator component', () => { - it('Checks if the MsalAuthenticator renders', async () => { - const onLogoutMock = jest.fn(); - const onLoginMock = jest.fn(); - const onFailedLoginMock = jest.fn(); - const authenticatorRef = null; render( -

- , - { - wrapper: MemoryRouter, - } + {...mockProps} + ref={(ref) => (authenticatorRef = ref)} + /> ); - const children = screen.getByTestId('children'); - expect(children).toBeInTheDocument(); + await act(async () => { + authenticatorRef?.invokeLogin(); + }); + + expect(mockInstance.loginPopup).toHaveBeenCalledWith(msalLoginRequest); + expect(mockProps.onLoginSuccess).toHaveBeenCalled(); + }); + + it('should handle login in normal window using redirect', async () => { + // Mock window.self === window.top for normal window detection + Object.defineProperty(window, 'self', { + value: window, + writable: true, + }); + Object.defineProperty(window, 'top', { + value: window, + writable: true, + }); + + render( + (authenticatorRef = ref)} + /> + ); + + await act(async () => { + authenticatorRef?.invokeLogin(); + }); + + expect(mockInstance.loginRedirect).toHaveBeenCalledWith(msalLoginRequest); + }); + + it('should handle logout', async () => { + render( + (authenticatorRef = ref)} + /> + ); + + await act(async () => { + authenticatorRef?.invokeLogout(); + }); + + expect(mockProps.onLogoutSuccess).toHaveBeenCalled(); + }); + + it('should handle renewIdToken successfully', async () => { + mockInstance.ssoSilent.mockResolvedValueOnce({ + account: { username: 'test@example.com' }, + idToken: 'new-token', + }); + + render( + (authenticatorRef = ref)} + /> + ); + + const result = await authenticatorRef?.renewIdToken(); + + expect(mockInstance.ssoSilent).toHaveBeenCalled(); + expect(result).toBe('mock-id-token'); + }); + + it('should show loader when interaction is in progress', () => { + (useMsal as jest.Mock).mockReturnValue({ + instance: mockInstance, + accounts: [{ username: 'test@example.com' }], + inProgress: InteractionStatus.Login, + }); + + render( + (authenticatorRef = ref)} + /> + ); + + expect(screen.getByTestId('loader')).toBeInTheDocument(); }); }); diff --git a/openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/MsalAuthenticator.tsx b/openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/MsalAuthenticator.tsx index 470af128a46..e3e766e0793 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/MsalAuthenticator.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/MsalAuthenticator.tsx @@ -63,8 +63,17 @@ const MsalAuthenticator = forwardRef( const login = async () => { try { - // Use login with redirect to avoid popup issue with maximized browser window - await instance.loginRedirect(); + const isInIframe = window.self !== window.top; + + if (isInIframe) { + // Use popup login when in iframe to avoid redirect issues + const response = await instance.loginPopup(msalLoginRequest); + + onLoginSuccess(parseMSALResponse(response)); + } else { + // Use login with redirect for normal window context + await instance.loginRedirect(msalLoginRequest); + } } catch (error) { onLoginFailure(error as AxiosError); } diff --git a/openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OidcAuthenticator.test.tsx b/openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OidcAuthenticator.test.tsx new file mode 100644 index 00000000000..67d0360e799 --- /dev/null +++ b/openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OidcAuthenticator.test.tsx @@ -0,0 +1,251 @@ +/* + * Copyright 2025 Collate. + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +import { act, render, screen } from '@testing-library/react'; +import { User } from 'oidc-client'; +import React from 'react'; +import { Callback } from 'react-oidc'; +import { MemoryRouter } from 'react-router-dom'; +import { ROUTES } from '../../../constants/constants'; +import { useApplicationStore } from '../../../hooks/useApplicationStore'; +import TokenService from '../../../utils/Auth/TokenService/TokenServiceUtil'; +import { setOidcToken } from '../../../utils/LocalStorageUtils'; +import { + AuthenticatorRef, + OidcUser, +} from '../AuthProviders/AuthProvider.interface'; +import OidcAuthenticator from './OidcAuthenticator'; + +const mockOnLoginSuccess = jest.fn(); +const mockOnLoginFailure = jest.fn(); +const mockOnLogoutSuccess = jest.fn(); + +// Mock dependencies +jest.mock('../../../utils/LocalStorageUtils'); +jest.mock('../../../utils/Auth/TokenService/TokenServiceUtil'); +jest.mock('../../../hooks/useApplicationStore'); +jest.mock('react-oidc', () => ({ + ...jest.requireActual('react-oidc'), + Callback: jest.fn().mockImplementation(() => { + return

Callback
; + }), +})); + +const mockHistoryPush = jest.fn(); +jest.mock('react-router-dom', () => ({ + ...jest.requireActual('react-router-dom'), + useHistory: () => ({ + push: mockHistoryPush, + }), +})); + +describe('OidcAuthenticator - Silent SignIn Tests', () => { + const mockUpdateAxiosInterceptors = jest.fn(); + const mockTokenServiceInstance = { + clearRefreshInProgress: jest.fn(), + }; + + beforeEach(() => { + jest.clearAllMocks(); + (TokenService.getInstance as jest.Mock).mockReturnValue( + mockTokenServiceInstance + ); + (useApplicationStore as unknown as jest.Mock).mockReturnValue({ + updateAxiosInterceptors: mockUpdateAxiosInterceptors, + }); + }); + + it('should handle silent sign-in success', async () => { + const mockUser = { + id_token: 'mock-id-token', + } as User; + + render( + +
Child Component
} + userConfig={{}} + onLoginFailure={mockOnLoginFailure} + onLoginSuccess={mockOnLoginSuccess} + onLogoutSuccess={mockOnLogoutSuccess}> +
Protected Content
+ + + ); + + expect(Callback).toHaveBeenCalled(); + + // Get the Callback component's onSuccess prop and call it + const callbackProps = (Callback as jest.Mock).mock.calls[0][0]; + await act(async () => { + callbackProps.onSuccess(mockUser); + }); + + // Verify the success flow + expect(setOidcToken).toHaveBeenCalledWith('mock-id-token'); + expect(mockUpdateAxiosInterceptors).toHaveBeenCalled(); + expect(mockTokenServiceInstance.clearRefreshInProgress).toHaveBeenCalled(); + }); + + it('should handle silent sign-in failure', async () => { + const mockError = new Error('Silent sign-in failed'); + + render( + +
Child Component
} + userConfig={{}} + onLoginFailure={mockOnLoginFailure} + onLoginSuccess={mockOnLoginSuccess} + onLogoutSuccess={mockOnLogoutSuccess}> +
Protected Content
+ + + ); + + // Get the Callback component's onError prop and call it + const callbackProps = (Callback as jest.Mock).mock.calls[0][0]; + await act(async () => { + callbackProps.onError(mockError); + }); + + // Verify the failure flow + expect(mockTokenServiceInstance.clearRefreshInProgress).toHaveBeenCalled(); + expect(mockOnLogoutSuccess).toHaveBeenCalled(); + expect(mockHistoryPush).toHaveBeenCalledWith(ROUTES.SIGNIN); + }); +}); + +describe('OidcAuthenticator - Login Tests', () => { + const mockUpdateAxiosInterceptors = jest.fn(); + const mockSetIsSigningUp = jest.fn(); + const mockOnLoginSuccess = jest.fn(); + const mockOnLoginFailure = jest.fn(); + + beforeEach(() => { + jest.clearAllMocks(); + (useApplicationStore as unknown as jest.Mock).mockReturnValue({ + updateAxiosInterceptors: mockUpdateAxiosInterceptors, + setIsSigningUp: mockSetIsSigningUp, + }); + }); + + it('should handle login success', async () => { + const mockUser = { + id_token: 'mock-id-token', + profile: { + email: 'test@test.com', + name: 'Test User', + }, + } as OidcUser; + + render( + +
Child Component
} + userConfig={{}} + onLoginFailure={mockOnLoginFailure} + onLoginSuccess={mockOnLoginSuccess} + onLogoutSuccess={mockOnLogoutSuccess}> +
Protected Content
+ + + ); + + // Get the Callback component's onSuccess prop and call it + const callbackProps = (Callback as jest.Mock).mock.calls[0][0]; + await act(async () => { + callbackProps.onSuccess(mockUser); + }); + + // Verify the success flow + expect(setOidcToken).toHaveBeenCalledWith('mock-id-token'); + expect(mockOnLoginSuccess).toHaveBeenCalledWith(mockUser); + }); + + it('should handle login failure', async () => { + const mockError = new Error('Login failed'); + + render( + +
Child Component
} + userConfig={{}} + onLoginFailure={mockOnLoginFailure} + onLoginSuccess={mockOnLoginSuccess} + onLogoutSuccess={mockOnLogoutSuccess}> +
Protected Content
+ + + ); + + // Get the Callback component's onError prop and call it + const callbackProps = (Callback as jest.Mock).mock.calls[0][0]; + await act(async () => { + callbackProps.onError(mockError); + }); + + // Verify the failure flow + expect(mockOnLoginFailure).toHaveBeenCalled(); + }); + + it('should handle login initiation', async () => { + const ref = React.createRef(); + + render( + +
Child Component
} + ref={ref} + userConfig={{}} + onLoginFailure={mockOnLoginFailure} + onLoginSuccess={mockOnLoginSuccess} + onLogoutSuccess={mockOnLogoutSuccess}> +
Protected Content
+ + + ); + + // Trigger login + await act(async () => { + ref.current?.invokeLogin(); + }); + + // Verify login initialization + expect(mockSetIsSigningUp).toHaveBeenCalledWith(true); + }); + + it('should render SignInPage when not authenticated and not signing up', () => { + (useApplicationStore as unknown as jest.Mock).mockReturnValue({ + isAuthenticated: false, + isSigningUp: false, + currentUser: {}, + newUser: {}, + }); + + render( + +
Child Component
} + userConfig={{}} + onLoginFailure={mockOnLoginFailure} + onLoginSuccess={mockOnLoginSuccess} + onLogoutSuccess={mockOnLogoutSuccess}> +
Protected Content
+ + + ); + + // Verify SignInPage is rendered + expect(screen.getByTestId('signin-page')).toBeInTheDocument(); + }); +}); diff --git a/openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OidcAuthenticator.tsx b/openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OidcAuthenticator.tsx index 74eb1f3597a..90b4ceb6e82 100644 --- a/openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OidcAuthenticator.tsx +++ b/openmetadata-ui/src/main/resources/ui/src/components/Auth/AppAuthenticators/OidcAuthenticator.tsx @@ -115,6 +115,9 @@ const OidcAuthenticator = forwardRef( // eslint-disable-next-line no-console console.error(error); + // Clear the refresh token in progress flag + // Since refresh token request completes with a callback + TokenService.getInstance().clearRefreshInProgress(); onLogoutSuccess(); history.push(ROUTES.SIGNIN); };