From f787afed7ed98bf779a0ffacbaa82a0bcaa17e1f Mon Sep 17 00:00:00 2001 From: Ambroise C Date: Wed, 1 Mar 2023 09:49:07 +0100 Subject: [PATCH] SONAR-18372 Display specific error message when failing password change Also remove some code smells --- server/sonar-web/src/main/js/api/users.ts | 13 ++++-- .../js/apps/users/components/PasswordForm.tsx | 25 +++++------ .../__tests__/PasswordForm-test.tsx | 41 +++++++++++++------ .../components/common/ResetPasswordForm.tsx | 22 +++++----- .../__tests__/ResetPasswordForm-test.tsx | 30 ++++++++++++++ .../ResetPasswordForm-test.tsx.snap | 18 ++++++++ server/sonar-web/src/main/js/types/users.ts | 5 +++ .../resources/org/sonar/l10n/core.properties | 2 + 8 files changed, 116 insertions(+), 40 deletions(-) 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 { 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(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 { 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 { }; 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 {

{header}

- {error && {error}} + {errorTranslationKey && {translate(errorTranslationKey)}} 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 = {}) { 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 { 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 { {success && {translate('my_profile.password.changed')}} {errors && - errors.map((e, i) => ( - /* eslint-disable-next-line react/no-array-index-key */ - + errors.map((e) => ( + {e} ))} 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`] = `
`; + +exports[`should not change password if new password is same as old 1`] = ` + + user.new_password_same_as_old + +`; + +exports[`should not change password if old password is incorrect 1`] = ` + + user.old_password_incorrect + +`; 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) -- 2.39.5