From 46d4475b8fd08c86ff3e107b41a3d0ce0826eb60 Mon Sep 17 00:00:00 2001 From: karilint Date: Wed, 1 Apr 2026 15:17:20 +0300 Subject: [PATCH 1/6] Fix admin user management flows --- backend/src/routes/user.ts | 43 +++++++--- .../components/Person/Tabs/AddUserModal.tsx | 4 +- .../Person/Tabs/ChangePasswordForm.test.tsx | 82 +++++++++++++++++++ .../Person/Tabs/ChangePasswordForm.tsx | 49 +++++++---- .../src/components/Person/Tabs/PersonTab.tsx | 11 ++- frontend/src/redux/userReducer.ts | 7 +- 6 files changed, 157 insertions(+), 39 deletions(-) create mode 100644 frontend/src/components/Person/Tabs/ChangePasswordForm.test.tsx diff --git a/backend/src/routes/user.ts b/backend/src/routes/user.ts index 98884c0cf..24a111a32 100644 --- a/backend/src/routes/user.ts +++ b/backend/src/routes/user.ts @@ -106,33 +106,50 @@ router.post('/create', async (req, res) => { router.put('/password', async (req, res) => { if (!req.user) throw new AccessError() - const { userId } = req.user - const { newPassword, oldPassword } = req.body as { newPassword: string; oldPassword: string } + const { userId, role } = req.user + const { newPassword, oldPassword, targetUserId } = req.body as { + newPassword: string + oldPassword?: string + targetUserId?: number + } + + const isAdminChangingAnotherUser = role === Role.Admin && typeof targetUserId === 'number' && targetUserId !== userId + const userIdToUpdate = isAdminChangingAnotherUser ? targetUserId : userId const foundUser = await nowDb.com_users.findFirst({ where: { - user_id: userId, + user_id: userIdToUpdate, }, select: { password: true, newpassword: true }, }) - let passwordMatches: boolean - if (foundUser?.newpassword === null) { - // Compare old password - const hash = md5(oldPassword) - passwordMatches = hash === foundUser.password - } else { - passwordMatches = !!foundUser && !!(await bcrypt.compare(oldPassword, foundUser.newpassword)) + if (!foundUser) return res.status(404).send({ error: 'User not found.' }) + + if (!isAdminChangingAnotherUser) { + let passwordMatches: boolean + if (typeof oldPassword !== 'string' || oldPassword.length === 0) { + return res.status(400).send({ error: 'Old password is required.' }) + } + if (newPassword === oldPassword) { + return res.status(400).send({ error: 'New password must be different from your current password.' }) + } + if (foundUser.newpassword === null) { + // Compare old password + const hash = md5(oldPassword) + passwordMatches = hash === foundUser.password + } else { + passwordMatches = !!(await bcrypt.compare(oldPassword, foundUser.newpassword)) + } + + if (!passwordMatches) return res.status(403).send({ error: 'Old password does not match your current password.' }) } - if (!passwordMatches) return res.status(403).send({ error: 'Old password does not match your current password.' }) - const validationResult = validatePassword(newPassword) if (!validationResult.isValid) return res.status(400).send({ error: validationResult.error }) const passwordHash = await createPasswordHash(newPassword) - await nowDb.com_users.update({ where: { user_id: userId }, data: { newpassword: passwordHash } }) + await nowDb.com_users.update({ where: { user_id: userIdToUpdate }, data: { newpassword: passwordHash } }) return res.status(200).send() }) diff --git a/frontend/src/components/Person/Tabs/AddUserModal.tsx b/frontend/src/components/Person/Tabs/AddUserModal.tsx index 2bce279bc..d3b8f1b68 100644 --- a/frontend/src/components/Person/Tabs/AddUserModal.tsx +++ b/frontend/src/components/Person/Tabs/AddUserModal.tsx @@ -108,11 +108,10 @@ interface UserFieldValues { interface Props { isOpen: boolean onClose: () => void - onSave: () => void personInitials: string } -export const AddUserModal = ({ isOpen, onClose, onSave, personInitials }: Props) => { +export const AddUserModal = ({ isOpen, onClose, personInitials }: Props) => { const [fieldValues, setFieldValues] = useState({ username: '', password: '', @@ -134,7 +133,6 @@ export const AddUserModal = ({ isOpen, onClose, onSave, personInitials }: Props) .then(() => { notify('User saved successfully.') onClose() - onSave() }) .catch(e => { notify((e as { data: { message: string } }).data.message, 'error') diff --git a/frontend/src/components/Person/Tabs/ChangePasswordForm.test.tsx b/frontend/src/components/Person/Tabs/ChangePasswordForm.test.tsx new file mode 100644 index 000000000..bbdc345f6 --- /dev/null +++ b/frontend/src/components/Person/Tabs/ChangePasswordForm.test.tsx @@ -0,0 +1,82 @@ +import { beforeEach, describe, expect, it, jest } from '@jest/globals' +import { fireEvent, render, screen, waitFor } from '@testing-library/react' +import { ChangePasswordForm } from './ChangePasswordForm' + +const mockNotify = jest.fn() +const mockDispatch = jest.fn() +const mockUnwrap = jest.fn() +const mockChangePasswordMutation = jest.fn(() => ({ unwrap: mockUnwrap })) + +jest.mock('@/hooks/notification', () => ({ + useNotify: () => ({ notify: mockNotify }), +})) + +jest.mock('react-redux', () => ({ + useDispatch: () => mockDispatch, +})) + +jest.mock('@/redux/userReducer', () => ({ + removeFirstLogin: () => ({ type: 'user/removeFirstLogin' }), + useChangePasswordMutation: () => [mockChangePasswordMutation], +})) + +jest.mock('@/hooks/usePasswordValidation', () => ({ + usePasswordValidation: () => ({ + passwordRequirements: ['At least 8 characters'], + validatePassword: (password: string) => ({ + isValid: password.length >= 8, + error: password.length >= 8 ? undefined : 'Password must be at least 8 characters long', + }), + }), +})) + +describe('ChangePasswordForm', () => { + beforeEach(() => { + mockNotify.mockReset() + mockDispatch.mockReset() + mockChangePasswordMutation.mockClear() + mockUnwrap.mockReset() + mockUnwrap.mockImplementation(() => Promise.resolve()) + }) + + it('requires the old password for self-service password changes', () => { + render() + + expect(screen.getByTestId('old-password-input')).toBeTruthy() + expect(screen.getByRole('button', { name: 'Change password' })).toHaveProperty('disabled', true) + }) + + it('lets admins set another users password without showing the old password field', async () => { + render() + + expect(screen.queryByTestId('old-password-input')).toBeNull() + + fireEvent.change(screen.getByTestId('new-password-input'), { target: { value: 'validpass' } }) + fireEvent.change(screen.getByTestId('verify-password-input'), { target: { value: 'validpass' } }) + fireEvent.click(screen.getByRole('button', { name: 'Set password' })) + + expect(mockChangePasswordMutation).toHaveBeenCalledWith({ + newPassword: 'validpass', + oldPassword: '', + targetUserId: 42, + }) + expect(mockDispatch).not.toHaveBeenCalled() + await waitFor(() => { + expect(mockNotify).toHaveBeenCalledWith('Password changed successfully for user.') + }) + }) + + it('rejects setting the same password in self-service mode', async () => { + render() + + fireEvent.change(screen.getByTestId('old-password-input'), { target: { value: 'validpass' } }) + fireEvent.change(screen.getByTestId('new-password-input'), { target: { value: 'validpass' } }) + fireEvent.change(screen.getByTestId('verify-password-input'), { target: { value: 'validpass' } }) + fireEvent.click(screen.getByRole('button', { name: 'Change password' })) + + expect(mockChangePasswordMutation).not.toHaveBeenCalled() + await waitFor(() => { + expect(mockNotify).toHaveBeenCalledWith('New password must be different from your current password.', 'error') + }) + }) +}) diff --git a/frontend/src/components/Person/Tabs/ChangePasswordForm.tsx b/frontend/src/components/Person/Tabs/ChangePasswordForm.tsx index 9d03409a7..adcaaf83b 100755 --- a/frontend/src/components/Person/Tabs/ChangePasswordForm.tsx +++ b/frontend/src/components/Person/Tabs/ChangePasswordForm.tsx @@ -7,7 +7,11 @@ import { usePasswordValidation } from '@/hooks/usePasswordValidation' type ChangePasswordError = { status: number; data: { error: string } } -export const ChangePasswordForm = () => { +type ChangePasswordFormProps = { + targetUserId?: number +} + +export const ChangePasswordForm = ({ targetUserId }: ChangePasswordFormProps) => { const [oldPassword, setOldPassword] = useState('') const [newPassword, setNewPassword] = useState('') const [verifyPassword, setVerifyPassword] = useState('') @@ -15,14 +19,15 @@ export const ChangePasswordForm = () => { const { notify } = useNotify() const sx = { display: 'flex', alignItems: 'center', width: '10em', fontWeight: 'bold' } const dispatch = useDispatch() + const isAdminOverride = typeof targetUserId === 'number' const { passwordRequirements, validatePassword } = usePasswordValidation() const isPasswordValid = useMemo(() => validatePassword(newPassword).isValid, [newPassword, validatePassword]) const isFormValid = - oldPassword.length > 0 && newPassword.length > 0 && verifyPassword.length > 0 && isPasswordValid && + (isAdminOverride || oldPassword.length > 0) && newPassword === verifyPassword const changePassword = async () => { @@ -30,19 +35,25 @@ export const ChangePasswordForm = () => { notify('New password was not the same in both fields.', 'error') return } + if (!isAdminOverride && newPassword === oldPassword) { + notify('New password must be different from your current password.', 'error') + return + } const passwordValidationResult = validatePassword(newPassword) if (!passwordValidationResult.isValid) { notify(passwordValidationResult.error ?? 'Password does not meet requirements', 'error') return } - if (newPassword.length === 0 || oldPassword.length === 0) { + if (newPassword.length === 0 || (!isAdminOverride && oldPassword.length === 0)) { notify('Please fill all fields.', 'error') return } try { - await changePasswordMutation({ newPassword, oldPassword }).unwrap() - notify('Password changed successfully.') - dispatch(removeFirstLogin()) + await changePasswordMutation({ newPassword, oldPassword, targetUserId }).unwrap() + notify(isAdminOverride ? 'Password changed successfully for user.' : 'Password changed successfully.') + if (!isAdminOverride) { + dispatch(removeFirstLogin()) + } setOldPassword('') setNewPassword('') setVerifyPassword('') @@ -54,18 +65,20 @@ export const ChangePasswordForm = () => { return ( - - - Old password: + {!isAdminOverride && ( + + + Old password: + + setOldPassword(event.target.value)} + inputProps={{ 'data-testid': 'old-password-input' }} + /> - setOldPassword(event.target.value)} - inputProps={{ 'data-testid': 'old-password-input' }} - /> - + )} New password: @@ -110,7 +123,7 @@ export const ChangePasswordForm = () => { onClick={() => void changePassword()} disabled={!isFormValid} > - Change password + {isAdminOverride ? 'Set password' : 'Change password'} diff --git a/frontend/src/components/Person/Tabs/PersonTab.tsx b/frontend/src/components/Person/Tabs/PersonTab.tsx index 74ecb8016..67c77903b 100755 --- a/frontend/src/components/Person/Tabs/PersonTab.tsx +++ b/frontend/src/components/Person/Tabs/PersonTab.tsx @@ -18,9 +18,9 @@ export const PersonTab = () => { const { notify } = useNotify() const { id: idFromUrl } = useParams() const isNew = idFromUrl === 'new' + const isUserPage = idFromUrl === 'user-page' const isAdmin = currentUser.role == Role.Admin const [isAddUserModalOpen, setIsAddUserModalOpen] = useState(false) - const [disableAddUserButton, setDisableAddUserButton] = useState(false) useEffect(() => { if (!currentUser?.isFirstLogin) return @@ -54,13 +54,12 @@ export const PersonTab = () => { onClose={() => { setIsAddUserModalOpen(false) }} - onSave={() => setDisableAddUserButton(true)} personInitials={data.initials} /> {data.user && } - {isAdmin && !data.user && !disableAddUserButton && mode.option === 'read' && ( + {isAdmin && !data.user && mode.option === 'read' && (