From: Ambroise C <ambroise.christea@sonarsource.com>
Date: Wed, 1 Mar 2023 08:49:07 +0000 (+0100)
Subject: SONAR-18372 Display specific error message when failing password change
X-Git-Tag: 10.0.0.68432~183
X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=f787afed7ed98bf779a0ffacbaa82a0bcaa17e1f;p=sonarqube.git

SONAR-18372 Display specific error message when failing password change

Also remove some code smells
---

diff --git a/server/sonar-web/src/main/js/api/users.ts b/server/sonar-web/src/main/js/api/users.ts
index c4584533e77..ca921bd026c 100644
--- a/server/sonar-web/src/main/js/api/users.ts
+++ b/server/sonar-web/src/main/js/api/users.ts
@@ -18,9 +18,9 @@
  * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
 import { throwGlobalError } from '../helpers/error';
-import { getJSON, post, postJSON } from '../helpers/request';
+import { getJSON, HttpStatus, parseJSON, post, postJSON } from '../helpers/request';
 import { IdentityProvider, Paging } from '../types/types';
-import { CurrentUser, HomePage, NoticeType, User } from '../types/users';
+import { ChangePasswordResults, CurrentUser, HomePage, NoticeType, User } from '../types/users';
 
 export function getCurrentUser(): Promise<CurrentUser> {
   return getJSON('/api/users/current', undefined, true);
@@ -35,7 +35,14 @@ export function changePassword(data: {
   password: string;
   previousPassword?: string;
 }) {
-  return post('/api/users/change_password', data).catch(throwGlobalError);
+  return post('/api/users/change_password', data).catch(async (response) => {
+    if (response.status === HttpStatus.BadRequest) {
+      const { result } = await parseJSON(response);
+      return Promise.reject<ChangePasswordResults>(result);
+    }
+
+    return throwGlobalError(response);
+  });
 }
 
 export interface UserGroup {
diff --git a/server/sonar-web/src/main/js/apps/users/components/PasswordForm.tsx b/server/sonar-web/src/main/js/apps/users/components/PasswordForm.tsx
index 9cc62cb1d36..a82a236f8b5 100644
--- a/server/sonar-web/src/main/js/apps/users/components/PasswordForm.tsx
+++ b/server/sonar-web/src/main/js/apps/users/components/PasswordForm.tsx
@@ -24,11 +24,9 @@ import Modal from '../../../components/controls/Modal';
 import { Alert } from '../../../components/ui/Alert';
 import MandatoryFieldMarker from '../../../components/ui/MandatoryFieldMarker';
 import MandatoryFieldsExplanation from '../../../components/ui/MandatoryFieldsExplanation';
-import { throwGlobalError } from '../../../helpers/error';
 import { addGlobalSuccessMessage } from '../../../helpers/globalMessages';
 import { translate } from '../../../helpers/l10n';
-import { parseError } from '../../../helpers/request';
-import { User } from '../../../types/users';
+import { ChangePasswordResults, User } from '../../../types/users';
 
 interface Props {
   isCurrentUser: boolean;
@@ -38,7 +36,7 @@ interface Props {
 
 interface State {
   confirmPassword: string;
-  error?: string;
+  errorTranslationKey?: string;
   newPassword: string;
   oldPassword: string;
   submitting: boolean;
@@ -61,14 +59,13 @@ export default class PasswordForm extends React.PureComponent<Props, State> {
     this.mounted = false;
   }
 
-  handleError = (response: Response) => {
-    if (!this.mounted || response.status !== 400) {
-      return throwGlobalError(response);
-    } else {
-      return parseError(response).then(
-        (errorMsg) => this.setState({ error: errorMsg, submitting: false }),
-        throwGlobalError
-      );
+  handleError = (result: ChangePasswordResults) => {
+    if (this.mounted) {
+      if (result === ChangePasswordResults.OldPasswordIncorrect) {
+        this.setState({ errorTranslationKey: 'user.old_password_incorrect', submitting: false });
+      } else if (result === ChangePasswordResults.NewPasswordSameAsOld) {
+        this.setState({ errorTranslationKey: 'user.new_password_same_as_old', submitting: false });
+      }
     }
   };
 
@@ -100,7 +97,7 @@ export default class PasswordForm extends React.PureComponent<Props, State> {
   };
 
   render() {
-    const { error, submitting, newPassword, confirmPassword } = this.state;
+    const { errorTranslationKey, submitting, newPassword, confirmPassword } = this.state;
 
     const header = translate('my_profile.password.title');
     return (
@@ -110,7 +107,7 @@ export default class PasswordForm extends React.PureComponent<Props, State> {
             <h2>{header}</h2>
           </header>
           <div className="modal-body">
-            {error && <Alert variant="error">{error}</Alert>}
+            {errorTranslationKey && <Alert variant="error">{translate(errorTranslationKey)}</Alert>}
 
             <MandatoryFieldsExplanation className="modal-field" />
 
diff --git a/server/sonar-web/src/main/js/apps/users/components/__tests__/PasswordForm-test.tsx b/server/sonar-web/src/main/js/apps/users/components/__tests__/PasswordForm-test.tsx
index a1f1c68fa9e..6b81c916a5e 100644
--- a/server/sonar-web/src/main/js/apps/users/components/__tests__/PasswordForm-test.tsx
+++ b/server/sonar-web/src/main/js/apps/users/components/__tests__/PasswordForm-test.tsx
@@ -21,6 +21,8 @@ import { shallow } from 'enzyme';
 import * as React from 'react';
 import { changePassword } from '../../../../api/users';
 import { mockUser } from '../../../../helpers/testMocks';
+import { mockEvent, waitAndUpdate } from '../../../../helpers/testUtils';
+import { ChangePasswordResults } from '../../../../types/users';
 import PasswordForm from '../PasswordForm';
 
 const password = 'new password asdf';
@@ -38,38 +40,51 @@ it('should handle password change', async () => {
   const wrapper = shallowRender({ onClose });
 
   wrapper.setState({ newPassword: password, confirmPassword: password });
-  wrapper.instance().handleChangePassword({ preventDefault: jest.fn() } as any);
+  wrapper.instance().handleChangePassword(mockEvent({ preventDefault: jest.fn() }));
 
-  await new Promise(setImmediate);
+  await waitAndUpdate(wrapper);
 
   expect(onClose).toHaveBeenCalled();
 });
 
-it('should handle password change error', async () => {
+it('should handle password change error when new password is same as old', async () => {
   const wrapper = shallowRender();
 
-  (changePassword as jest.Mock).mockRejectedValue(new Response(undefined, { status: 400 }));
+  jest.mocked(changePassword).mockRejectedValue(ChangePasswordResults.NewPasswordSameAsOld);
+  wrapper.setState({ newPassword: password, confirmPassword: password });
+  wrapper.instance().mounted = true;
+  wrapper.instance().handleChangePassword(mockEvent({ preventDefault: jest.fn() }));
+
+  await waitAndUpdate(wrapper);
+
+  expect(wrapper.state().errorTranslationKey).toBe('user.new_password_same_as_old');
+});
+
+it('should handle password change error when old password is incorrect', async () => {
+  const wrapper = shallowRender();
+
+  jest.mocked(changePassword).mockRejectedValue(ChangePasswordResults.OldPasswordIncorrect);
 
   wrapper.setState({ newPassword: password, confirmPassword: password });
   wrapper.instance().mounted = true;
-  wrapper.instance().handleChangePassword({ preventDefault: jest.fn() } as any);
+  wrapper.instance().handleChangePassword(mockEvent({ preventDefault: jest.fn() }));
 
-  await new Promise(setImmediate);
+  await waitAndUpdate(wrapper);
 
-  expect(wrapper.state('error')).toBe('default_error_message');
+  expect(wrapper.state().errorTranslationKey).toBe('user.old_password_incorrect');
 });
 
 it('should handle form changes', () => {
   const wrapper = shallowRender();
 
-  wrapper.instance().handleConfirmPasswordChange({ currentTarget: { value: 'pwd' } } as any);
-  expect(wrapper.state('confirmPassword')).toBe('pwd');
+  wrapper.instance().handleConfirmPasswordChange(mockEvent({ currentTarget: { value: 'pwd' } }));
+  expect(wrapper.state().confirmPassword).toBe('pwd');
 
-  wrapper.instance().handleNewPasswordChange({ currentTarget: { value: 'pwd' } } as any);
-  expect(wrapper.state('newPassword')).toBe('pwd');
+  wrapper.instance().handleNewPasswordChange(mockEvent({ currentTarget: { value: 'pwd' } }));
+  expect(wrapper.state().newPassword).toBe('pwd');
 
-  wrapper.instance().handleOldPasswordChange({ currentTarget: { value: 'pwd' } } as any);
-  expect(wrapper.state('oldPassword')).toBe('pwd');
+  wrapper.instance().handleOldPasswordChange(mockEvent({ currentTarget: { value: 'pwd' } }));
+  expect(wrapper.state().oldPassword).toBe('pwd');
 });
 
 function shallowRender(props: Partial<PasswordForm['props']> = {}) {
diff --git a/server/sonar-web/src/main/js/components/common/ResetPasswordForm.tsx b/server/sonar-web/src/main/js/components/common/ResetPasswordForm.tsx
index 129275f9f66..b092ab0545e 100644
--- a/server/sonar-web/src/main/js/components/common/ResetPasswordForm.tsx
+++ b/server/sonar-web/src/main/js/components/common/ResetPasswordForm.tsx
@@ -24,7 +24,7 @@ import { Alert } from '../../components/ui/Alert';
 import MandatoryFieldMarker from '../../components/ui/MandatoryFieldMarker';
 import MandatoryFieldsExplanation from '../../components/ui/MandatoryFieldsExplanation';
 import { translate } from '../../helpers/l10n';
-import { LoggedInUser } from '../../types/users';
+import { ChangePasswordResults, LoggedInUser } from '../../types/users';
 
 interface Props {
   className?: string;
@@ -76,12 +76,15 @@ export default class ResetPasswordForm extends React.Component<Props, State> {
       this.password.focus();
       this.setErrors([translate('user.password_doesnt_match_confirmation')]);
     } else {
-      changePassword({ login: user.login, password, previousPassword }).then(
-        this.handleSuccessfulChange,
-        () => {
-          // error already reported.
-        }
-      );
+      changePassword({ login: user.login, password, previousPassword })
+        .then(this.handleSuccessfulChange)
+        .catch((result: ChangePasswordResults) => {
+          if (result === ChangePasswordResults.OldPasswordIncorrect) {
+            this.setErrors([translate('user.old_password_incorrect')]);
+          } else if (result === ChangePasswordResults.NewPasswordSameAsOld) {
+            this.setErrors([translate('user.new_password_same_as_old')]);
+          }
+        });
     }
   };
 
@@ -94,9 +97,8 @@ export default class ResetPasswordForm extends React.Component<Props, State> {
         {success && <Alert variant="success">{translate('my_profile.password.changed')}</Alert>}
 
         {errors &&
-          errors.map((e, i) => (
-            /* eslint-disable-next-line react/no-array-index-key */
-            <Alert key={i} variant="error">
+          errors.map((e) => (
+            <Alert key={e} variant="error">
               {e}
             </Alert>
           ))}
diff --git a/server/sonar-web/src/main/js/components/common/__tests__/ResetPasswordForm-test.tsx b/server/sonar-web/src/main/js/components/common/__tests__/ResetPasswordForm-test.tsx
index fedd4850bde..8e86f5a1d4a 100644
--- a/server/sonar-web/src/main/js/components/common/__tests__/ResetPasswordForm-test.tsx
+++ b/server/sonar-web/src/main/js/components/common/__tests__/ResetPasswordForm-test.tsx
@@ -22,12 +22,18 @@ import * as React from 'react';
 import { changePassword } from '../../../api/users';
 import { mockLoggedInUser } from '../../../helpers/testMocks';
 import { mockEvent, waitAndUpdate } from '../../../helpers/testUtils';
+import { ChangePasswordResults } from '../../../types/users';
+import { Alert } from '../../ui/Alert';
 import ResetPasswordForm from '../ResetPasswordForm';
 
 jest.mock('../../../api/users', () => ({
   changePassword: jest.fn().mockResolvedValue({}),
 }));
 
+beforeEach(() => {
+  jest.clearAllMocks();
+});
+
 it('should trigger on password change prop', () => {
   const onPasswordChange = jest.fn();
   const wrapper = shallowRender({ onPasswordChange });
@@ -50,6 +56,30 @@ it('should not trigger password change', () => {
   expect(wrapper.state().errors).toBeDefined();
 });
 
+it('should not change password if new password is same as old', async () => {
+  jest.mocked(changePassword).mockRejectedValueOnce(ChangePasswordResults.NewPasswordSameAsOld);
+
+  const wrapper = shallowRender();
+  wrapper.instance().oldPassword = { value: 'testold' } as HTMLInputElement;
+  wrapper.instance().password = { value: 'test', focus: () => {} } as HTMLInputElement;
+  wrapper.instance().passwordConfirmation = { value: 'test' } as HTMLInputElement;
+  wrapper.instance().handleChangePassword(mockEvent());
+  await waitAndUpdate(wrapper);
+  expect(wrapper.find(Alert)).toMatchSnapshot();
+});
+
+it('should not change password if old password is incorrect', async () => {
+  jest.mocked(changePassword).mockRejectedValueOnce(ChangePasswordResults.OldPasswordIncorrect);
+
+  const wrapper = shallowRender();
+  wrapper.instance().oldPassword = { value: 'testold' } as HTMLInputElement;
+  wrapper.instance().password = { value: 'test', focus: () => {} } as HTMLInputElement;
+  wrapper.instance().passwordConfirmation = { value: 'test' } as HTMLInputElement;
+  wrapper.instance().handleChangePassword(mockEvent());
+  await waitAndUpdate(wrapper);
+  expect(wrapper.find(Alert)).toMatchSnapshot();
+});
+
 it('should trigger password change', async () => {
   const user = mockLoggedInUser();
   const wrapper = shallowRender({ user });
diff --git a/server/sonar-web/src/main/js/components/common/__tests__/__snapshots__/ResetPasswordForm-test.tsx.snap b/server/sonar-web/src/main/js/components/common/__tests__/__snapshots__/ResetPasswordForm-test.tsx.snap
index 3f32833747d..120f08f447f 100644
--- a/server/sonar-web/src/main/js/components/common/__tests__/__snapshots__/ResetPasswordForm-test.tsx.snap
+++ b/server/sonar-web/src/main/js/components/common/__tests__/__snapshots__/ResetPasswordForm-test.tsx.snap
@@ -69,3 +69,21 @@ exports[`renders correctly 1`] = `
   </div>
 </form>
 `;
+
+exports[`should not change password if new password is same as old 1`] = `
+<Alert
+  key="user.new_password_same_as_old"
+  variant="error"
+>
+  user.new_password_same_as_old
+</Alert>
+`;
+
+exports[`should not change password if old password is incorrect 1`] = `
+<Alert
+  key="user.old_password_incorrect"
+  variant="error"
+>
+  user.old_password_incorrect
+</Alert>
+`;
diff --git a/server/sonar-web/src/main/js/types/users.ts b/server/sonar-web/src/main/js/types/users.ts
index f4e69a48f20..eb94140e2b4 100644
--- a/server/sonar-web/src/main/js/types/users.ts
+++ b/server/sonar-web/src/main/js/types/users.ts
@@ -89,6 +89,11 @@ export interface UserBase {
   name?: string;
 }
 
+export const enum ChangePasswordResults {
+  OldPasswordIncorrect = 'old_password_incorrect',
+  NewPasswordSameAsOld = 'new_password_same_as_old',
+}
+
 export function isUserActive(user: UserBase): user is UserActive {
   return user.active !== false && Boolean(user.name);
 }
diff --git a/sonar-core/src/main/resources/org/sonar/l10n/core.properties b/sonar-core/src/main/resources/org/sonar/l10n/core.properties
index 62bb421d3a6..4e085db061a 100644
--- a/sonar-core/src/main/resources/org/sonar/l10n/core.properties
+++ b/sonar-core/src/main/resources/org/sonar/l10n/core.properties
@@ -2119,6 +2119,8 @@ alert.dismiss=Dismiss this message
 #
 #------------------------------------------------------------------------------
 user.password_doesnt_match_confirmation=Password doesn't match confirmation.
+user.old_password_incorrect=Old password is incorrect
+user.new_password_same_as_old=New password must be different from old password
 user.login_or_email_used_as_scm_account=Login and email are automatically considered as SCM accounts
 user.x_deleted={0} (deleted)