Merge pull request #10347 from growilabs/fix/login-error-display-issue-9799

fix: Ensure login errors display regardless of password login configuration

apps/app/src/client/components/LoginForm/LoginForm.spec.tsx

@@ -0,0 +1,287 @@
+import React from 'react';
+
+import {
+  render, screen, fireEvent, waitFor,
+} from '@testing-library/react';
+import {
+  describe, it, expect, vi, beforeEach,
+} from 'vitest';
+
+import { apiv3Post } from '~/client/util/apiv3-client';
+import type { IExternalAuthProviderType } from '~/interfaces/external-auth-provider';
+
+import { LoginForm } from './LoginForm';
+
+vi.mock('next-i18next', () => ({
+  useTranslation: () => ({
+    t: (key: string) => key,
+  }),
+}));
+
+vi.mock('next/router', () => ({
+  useRouter: () => ({
+    push: vi.fn(),
+  }),
+}));
+
+vi.mock('~/client/util/t-with-opt', () => ({
+  useTWithOpt: () => (key: string) => key,
+}));
+
+vi.mock('~/client/util/apiv3-client', () => ({
+  apiv3Post: vi.fn(),
+}));
+
+vi.mock('./ExternalAuthButton', () => ({
+  ExternalAuthButton: ({ authType }: { authType: string }) => (
+    <button type="button" data-testid={`external-auth-${authType}`}>
+      External Auth {authType}
+    </button>
+  ),
+}));
+
+vi.mock('../CompleteUserRegistration', () => ({
+  CompleteUserRegistration: () => <div>Complete Registration</div>,
+}));
+
+const defaultProps = {
+  isEmailAuthenticationEnabled: false,
+  registrationMode: 'Open' as const,
+  registrationWhitelist: [],
+  isPasswordResetEnabled: true,
+  isLocalStrategySetup: true,
+  isLdapStrategySetup: false,
+  isLdapSetupFailed: false,
+  minPasswordLength: 8,
+  isMailerSetup: true,
+};
+
+const mockApiv3Post = vi.mocked(apiv3Post);
+
+describe('LoginForm - Error Display', () => {
+  beforeEach(() => {
+    vi.clearAllMocks();
+  });
+
+  describe('when password login is enabled', () => {
+    it('should display login form', () => {
+      const props = {
+        ...defaultProps,
+        isLocalStrategySetup: true,
+      };
+
+      render(<LoginForm {...props} />);
+
+      expect(screen.getByTestId('login-form')).toBeInTheDocument();
+    });
+
+    it('should display external account login errors', () => {
+      const externalAccountLoginError = {
+        message: 'jwks must be a JSON Web Key Set formatted object',
+        name: 'ExternalAccountLoginError',
+      };
+
+      const props = {
+        ...defaultProps,
+        isLocalStrategySetup: true,
+        externalAccountLoginError,
+      };
+
+      render(<LoginForm {...props} />);
+
+      expect(screen.getByText('jwks must be a JSON Web Key Set formatted object')).toBeInTheDocument();
+    });
+  });
+
+  describe('when password login is disabled', () => {
+    it('should still display external account login errors', () => {
+      const externalAccountLoginError = {
+        message: 'jwks must be a JSON Web Key Set formatted object',
+        name: 'ExternalAccountLoginError',
+      };
+
+      const props = {
+        ...defaultProps,
+        isLocalStrategySetup: false,
+        isLdapStrategySetup: false,
+        enabledExternalAuthType: ['oidc'] satisfies IExternalAuthProviderType[],
+        externalAccountLoginError,
+      };
+
+      render(<LoginForm {...props} />);
+
+      expect(screen.getByText('jwks must be a JSON Web Key Set formatted object')).toBeInTheDocument();
+    });
+
+    it('should not render local/LDAP form but should still show errors', () => {
+      const externalAccountLoginError = {
+        message: 'OIDC authentication failed',
+        name: 'ExternalAccountLoginError',
+      };
+
+      const props = {
+        ...defaultProps,
+        isLocalStrategySetup: false,
+        isLdapStrategySetup: false,
+        enabledExternalAuthType: ['oidc'] satisfies IExternalAuthProviderType[],
+        externalAccountLoginError,
+      };
+
+      render(<LoginForm {...props} />);
+
+      expect(screen.queryByTestId('tiUsernameForLogin')).not.toBeInTheDocument();
+      expect(screen.queryByTestId('tiPasswordForLogin')).not.toBeInTheDocument();
+      expect(screen.getByText('OIDC authentication failed')).toBeInTheDocument();
+    });
+  });
+
+  describe('error display priority and login error handling', () => {
+    it('should show external errors when no login errors exist', () => {
+      const externalAccountLoginError = {
+        message: 'External error message',
+        name: 'ExternalAccountLoginError',
+      };
+
+      const props = {
+        ...defaultProps,
+        isLocalStrategySetup: true,
+        externalAccountLoginError,
+      };
+
+      render(<LoginForm {...props} />);
+
+      expect(screen.getByText('External error message')).toBeInTheDocument();
+    });
+
+    it('should prioritize login errors over external account login errors after failed login', async() => {
+      const externalAccountLoginError = {
+        message: 'External error message',
+        name: 'ExternalAccountLoginError',
+      };
+
+      // Mock API call to return error
+      mockApiv3Post.mockRejectedValueOnce([
+        {
+          message: 'Invalid username or password',
+          code: 'LOGIN_FAILED',
+          args: {},
+        },
+      ]);
+
+      const props = {
+        ...defaultProps,
+        isLocalStrategySetup: true,
+        externalAccountLoginError,
+      };
+
+      render(<LoginForm {...props} />);
+
+      // Initially, external error should be visible
+      expect(screen.getByText('External error message')).toBeInTheDocument();
+
+      // Fill in login form and submit
+      const usernameInput = screen.getByTestId('tiUsernameForLogin');
+      const passwordInput = screen.getByTestId('tiPasswordForLogin');
+      const submitButton = screen.getByTestId('btnSubmitForLogin');
+
+      fireEvent.change(usernameInput, { target: { value: 'testuser' } });
+      fireEvent.change(passwordInput, { target: { value: 'wrongpassword' } });
+      fireEvent.click(submitButton);
+
+      // Wait for login error to appear and external error to be replaced
+      await waitFor(() => {
+        expect(screen.getByText('Invalid username or password')).toBeInTheDocument();
+      });
+
+      // External error should no longer be visible when login error exists
+      expect(screen.queryByText('External error message')).not.toBeInTheDocument();
+    });
+
+    it('should display dangerouslySetInnerHTML errors for PROVIDER_DUPLICATED_USERNAME_EXCEPTION', async() => {
+      // Mock API call to return PROVIDER_DUPLICATED_USERNAME_EXCEPTION error
+      mockApiv3Post.mockRejectedValueOnce([
+        {
+          message: 'This username is already taken by <a href="/login">another provider</a>',
+          code: 'provider-duplicated-username-exception',
+          args: {},
+        },
+      ]);
+
+      const props = {
+        ...defaultProps,
+        isLocalStrategySetup: true,
+      };
+
+      render(<LoginForm {...props} />);
+
+      // Fill in login form and submit
+      const usernameInput = screen.getByTestId('tiUsernameForLogin');
+      const passwordInput = screen.getByTestId('tiPasswordForLogin');
+      const submitButton = screen.getByTestId('btnSubmitForLogin');
+
+      fireEvent.change(usernameInput, { target: { value: 'testuser' } });
+      fireEvent.change(passwordInput, { target: { value: 'password' } });
+      fireEvent.click(submitButton);
+
+      // Wait for the dangerouslySetInnerHTML error to appear
+      await waitFor(() => {
+        // Check that the error with HTML content is rendered
+        expect(screen.getByText(/This username is already taken by/)).toBeInTheDocument();
+      });
+    });
+
+    it('should handle multiple login errors correctly', async() => {
+      // Mock API call to return multiple errors
+      mockApiv3Post.mockRejectedValueOnce([
+        {
+          message: 'Username is required',
+          code: 'VALIDATION_ERROR',
+          args: {},
+        },
+        {
+          message: 'Password is too short',
+          code: 'VALIDATION_ERROR',
+          args: {},
+        },
+      ]);
+
+      const props = {
+        ...defaultProps,
+        isLocalStrategySetup: true,
+      };
+
+      render(<LoginForm {...props} />);
+
+      // Submit form without filling inputs
+      const submitButton = screen.getByTestId('btnSubmitForLogin');
+      fireEvent.click(submitButton);
+
+      // Wait for multiple errors to appear
+      await waitFor(() => {
+        expect(screen.getByText('Username is required')).toBeInTheDocument();
+        expect(screen.getByText('Password is too short')).toBeInTheDocument();
+      });
+    });
+  });
+
+  describe('error display when both login methods are disabled', () => {
+    it('should still display external errors when no login methods are available', () => {
+      const externalAccountLoginError = {
+        message: 'Authentication service unavailable',
+        name: 'ExternalAccountLoginError',
+      };
+
+      const props = {
+        ...defaultProps,
+        isLocalStrategySetup: false,
+        isLdapStrategySetup: false,
+        enabledExternalAuthType: undefined,
+        externalAccountLoginError,
+      };
+
+      render(<LoginForm {...props} />);
+
+      expect(screen.getByText('Authentication service unavailable')).toBeInTheDocument();
+    });
+  });
+});

apps/app/src/client/components/LoginForm/LoginForm.tsx

@@ -154,9 +154,9 @@ export const LoginForm = (props: LoginFormProps): JSX.Element => {
     return (
       <ul className="alert alert-danger">
         {errors.map((err, index) => (
-          <li className={index > 0 ? 'mt-1' : ''}>
+          <small className={index > 0 ? 'mt-1' : ''}>
             {tWithOpt(err.message, err.args)}
-          </li>
+          </small>
         ))}
       </ul>
     );
@@ -165,17 +165,6 @@ export const LoginForm = (props: LoginFormProps): JSX.Element => {
   const renderLocalOrLdapLoginForm = useCallback(() => {
     const { isLdapStrategySetup } = props;
 
-    // separate login errors into two arrays based on error code
-    const [loginErrorListForDangerouslySetInnerHTML, loginErrorList] = separateErrorsBasedOnErrorCode(loginErrors);
-    // Generate login error elements using dangerouslySetInnerHTML
-    const loginErrorElementWithDangerouslySetInnerHTML = generateDangerouslySetErrors(loginErrorListForDangerouslySetInnerHTML);
-    // Generate login error elements using <ul>, <li>
-
-    const loginErrorElement = (loginErrorList ?? []).length > 0
-      // prioritize loginErrorList because the list should contains new error
-      ? generateSafelySetErrors(loginErrorList)
-      : generateSafelySetErrors(props.externalAccountLoginError != null ? [props.externalAccountLoginError] : []);
-
     return (
       <>
         {/* !! - DO NOT DELETE HIDDEN ELEMENT - !! -- 7.12 ryoji-s */}
@@ -191,8 +180,6 @@ export const LoginForm = (props: LoginFormProps): JSX.Element => {
             <span dangerouslySetInnerHTML={{ __html: t('login.set_env_var_for_logs') }}></span>
           </div>
         )}
-        {loginErrorElementWithDangerouslySetInnerHTML}
-        {loginErrorElement}
 
         <form role="form" onSubmit={handleLoginWithLocalSubmit} id="login-form">
           <div className="input-group">
@@ -253,8 +240,7 @@ export const LoginForm = (props: LoginFormProps): JSX.Element => {
       </>
     );
   }, [
-    props, separateErrorsBasedOnErrorCode, loginErrors, generateDangerouslySetErrors, generateSafelySetErrors,
-    isLdapSetupFailed, t, handleLoginWithLocalSubmit, isLoading,
+    props, isLdapSetupFailed, t, handleLoginWithLocalSubmit, isLoading,
   ]);
 
 
@@ -510,6 +496,25 @@ export const LoginForm = (props: LoginFormProps): JSX.Element => {
           <div className="col-12 px-md-4 pb-5">
             <ReactCardFlip isFlipped={isRegistering} flipDirection="horizontal" cardZIndex="3">
               <div className="front">
+                {/* Error display section - always shown regardless of login method configuration */}
+                {(() => {
+                  // separate login errors into two arrays based on error code
+                  const [loginErrorListForDangerouslySetInnerHTML, loginErrorList] = separateErrorsBasedOnErrorCode(loginErrors);
+                  // Generate login error elements using dangerouslySetInnerHTML
+                  const loginErrorElementWithDangerouslySetInnerHTML = generateDangerouslySetErrors(loginErrorListForDangerouslySetInnerHTML);
+                  // Generate login error elements - prioritize loginErrorList, fallback to externalAccountLoginError
+                  const loginErrorElement = (loginErrorList ?? []).length > 0
+                    ? generateSafelySetErrors(loginErrorList)
+                    : generateSafelySetErrors(props.externalAccountLoginError != null ? [props.externalAccountLoginError] : []);
+
+                  return (
+                    <>
+                      {loginErrorElementWithDangerouslySetInnerHTML}
+                      {loginErrorElement}
+                    </>
+                  );
+                })()}
+
                 {isLocalOrLdapStrategiesEnabled && renderLocalOrLdapLoginForm()}
                 {isLocalOrLdapStrategiesEnabled && isSomeExternalAuthEnabled && (
                   <div className="text-center text-line d-flex align-items-center mb-3">