diff --git a/server/controllers/user.controller/__tests__/authManagement/updateSettings.test.ts b/server/controllers/user.controller/__tests__/authManagement/updateSettings.test.ts index b14140418a..e7b65e4b64 100644 --- a/server/controllers/user.controller/__tests__/authManagement/updateSettings.test.ts +++ b/server/controllers/user.controller/__tests__/authManagement/updateSettings.test.ts @@ -40,7 +40,6 @@ describe('user.controller > auth management > updateSettings (email, username, p const OLD_PASSWORD = 'oldpassword'; const NEW_PASSWORD = 'newpassword'; - // minimum valid request body const minimumValidRequest: UpdateSettingsRequestBody = { username: OLD_USERNAME, email: OLD_EMAIL @@ -90,15 +89,14 @@ describe('user.controller > auth management > updateSettings (email, username, p beforeEach(async () => { (User.findById as jest.Mock).mockResolvedValue(null); request.user = { id: 'nonexistent-id' }; + request.body = minimumValidRequest; await updateSettings(request, response, next); }); - it('returns 404 and a user-not-found error', async () => { + it('returns 404 and a user-not-found error', () => { expect(response.status).toHaveBeenCalledWith(404); - expect(response.json).toHaveBeenCalledWith({ - error: 'User not found' - }); + expect(response.json).toHaveBeenCalledWith({ error: 'User not found' }); }); it('does not save the user', () => { @@ -114,22 +112,24 @@ describe('user.controller > auth management > updateSettings (email, username, p request.setBody(requestBody); await updateSettings(request, response, next); }); + it('saves the user with the correct details exactly once', () => { expect(saveUser).toHaveBeenCalledWith(response, { ...startingUser }); expect(saveUser).toHaveBeenCalledTimes(1); }); + it('does not send a confirmation email to the user', () => { expect(mailerService.send).not.toHaveBeenCalled(); }); }); - // duplicate username check happens client-side before this request is made describe('when given new username and old email', () => { beforeEach(async () => { requestBody = { ...minimumValidRequest, username: NEW_USERNAME }; request.setBody(requestBody); await updateSettings(request, response, next); }); + it('saves the user with the correct details exactly once', () => { expect(saveUser).toHaveBeenCalledWith(response, { ...startingUser, @@ -137,6 +137,7 @@ describe('user.controller > auth management > updateSettings (email, username, p }); expect(saveUser).toHaveBeenCalledTimes(1); }); + it('does not send a confirmation email to the user', () => { expect(mailerService.send).not.toHaveBeenCalled(); }); @@ -148,6 +149,7 @@ describe('user.controller > auth management > updateSettings (email, username, p request.setBody(requestBody); await updateSettings(request, response, next); }); + it('saves the user with the correct details & verification token once', () => { expect(saveUser).toHaveBeenCalledWith(response, { ...startingUser, @@ -158,6 +160,7 @@ describe('user.controller > auth management > updateSettings (email, username, p }); expect(saveUser).toHaveBeenCalledTimes(1); }); + it('sends a confirmation email to the user', () => { expect(mailerService.send).toHaveBeenCalledWith( expect.objectContaining({ @@ -173,6 +176,7 @@ describe('user.controller > auth management > updateSettings (email, username, p request.setBody(requestBody); await updateSettings(request, response, next); }); + it('saves the user with the correct details once', () => { expect(saveUser).toHaveBeenCalledWith(response, { ...startingUser, @@ -184,6 +188,7 @@ describe('user.controller > auth management > updateSettings (email, username, p }); expect(saveUser).toHaveBeenCalledTimes(1); }); + it('sends a confirmation email to the user', () => { expect(mailerService.send).toHaveBeenCalledWith( expect.objectContaining({ @@ -203,6 +208,7 @@ describe('user.controller > auth management > updateSettings (email, username, p request.setBody(requestBody); await updateSettings(request, response, next); }); + it('saves the user with the correct details once', () => { expect(saveUser).toHaveBeenCalledWith(response, { ...startingUser, @@ -210,6 +216,7 @@ describe('user.controller > auth management > updateSettings (email, username, p }); expect(saveUser).toHaveBeenCalledTimes(1); }); + it('does not send a confirmation email to the user', () => { expect(mailerService.send).not.toHaveBeenCalled(); }); @@ -226,6 +233,7 @@ describe('user.controller > auth management > updateSettings (email, username, p request.setBody(requestBody); await updateSettings(request, response, next); }); + it('saves the user with the correct details once', () => { expect(saveUser).toHaveBeenCalledWith(response, { ...startingUser, @@ -234,12 +242,13 @@ describe('user.controller > auth management > updateSettings (email, username, p }); expect(saveUser).toHaveBeenCalledTimes(1); }); + it('does not send a confirmation email to the user', () => { expect(mailerService.send).not.toHaveBeenCalled(); }); }); - describe.skip('when given old username, new email, and new password with valid current password', () => { + describe('when given old username, new email, and new password with valid current password', () => { beforeEach(async () => { requestBody = { ...minimumValidRequest, @@ -250,6 +259,7 @@ describe('user.controller > auth management > updateSettings (email, username, p request.setBody(requestBody); await updateSettings(request, response, next); }); + it('saves the user with the correct details once', () => { expect(saveUser).toHaveBeenCalledWith(response, { ...startingUser, @@ -261,6 +271,7 @@ describe('user.controller > auth management > updateSettings (email, username, p }); expect(saveUser).toHaveBeenCalledTimes(1); }); + it('sends a confirmation email to the user', () => { expect(mailerService.send).toHaveBeenCalledWith( expect.objectContaining({ @@ -270,7 +281,7 @@ describe('user.controller > auth management > updateSettings (email, username, p }); }); - describe.skip('when given new username, new email, and new password with valid current password', () => { + describe('when given new username, new email, and new password with valid current password', () => { beforeEach(async () => { requestBody = { username: NEW_USERNAME, @@ -281,6 +292,7 @@ describe('user.controller > auth management > updateSettings (email, username, p request.setBody(requestBody); await updateSettings(request, response, next); }); + it('saves the user with the correct details once', () => { expect(saveUser).toHaveBeenCalledWith(response, { ...startingUser, @@ -293,6 +305,7 @@ describe('user.controller > auth management > updateSettings (email, username, p }); expect(saveUser).toHaveBeenCalledTimes(1); }); + it('sends a confirmation email to the user', () => { expect(mailerService.send).toHaveBeenCalledWith( expect.objectContaining({ @@ -304,30 +317,29 @@ describe('user.controller > auth management > updateSettings (email, username, p }); describe('unhappy paths', () => { - // Client-side checks to require username - describe.skip('when missing username', () => { + describe('when missing username', () => { beforeEach(async () => { request.setBody({ email: OLD_EMAIL }); await updateSettings(request, response, next); }); it('returns 401 with an "Missing username" message', () => { - expect(response.status).toHaveBeenCalledWith(400); + expect(response.status).toHaveBeenCalledWith(401); expect(response.json).toHaveBeenCalledWith({ error: 'Username is required.' }); }); - it('does not save the user with the new password', () => { + it('does not save the user', () => { expect(saveUser).not.toHaveBeenCalled(); }); + it('does not send a confirmation email to the user', () => { expect(mailerService.send).not.toHaveBeenCalled(); }); }); - // Client-side checks to require email - describe.skip('when missing email', () => { + describe('when missing email', () => { beforeEach(async () => { request.setBody({ username: OLD_USERNAME }); await updateSettings(request, response, next); @@ -340,63 +352,32 @@ describe('user.controller > auth management > updateSettings (email, username, p }); }); - it('does not save the user with the new password', () => { + it('does not save the user', () => { expect(saveUser).not.toHaveBeenCalled(); }); - it('does not send a confirmation email to the user', () => { - expect(mailerService.send).not.toHaveBeenCalled(); - }); - }); - // Client-side checks to require new password if current password is provided - describe.skip('when given old username, old email, and matching current password and no new password', () => { - beforeEach(async () => { - requestBody = { - ...minimumValidRequest, - currentPassword: OLD_PASSWORD - }; - request.setBody(requestBody); - await updateSettings(request, response, next); - }); - - it('returns 401 with an "New password is required" message', () => { - expect(response.status).toHaveBeenCalledWith(401); - expect(response.json).toHaveBeenCalledWith({ - error: 'New password is required.' - }); - }); - - it('does not save the user with the new password', () => { - expect(saveUser).not.toHaveBeenCalled(); - }); it('does not send a confirmation email to the user', () => { expect(mailerService.send).not.toHaveBeenCalled(); }); }); - describe('when given old username, old email, and non-matching current password and no new password', () => { + describe('when given old username, old email, and matching current password and no new password', () => { beforeEach(async () => { - testUser.comparePassword = jest.fn().mockResolvedValue(false); - requestBody = { ...minimumValidRequest, - currentPassword: 'not the same password', - newPassword: NEW_PASSWORD + currentPassword: OLD_PASSWORD }; request.setBody(requestBody); await updateSettings(request, response, next); }); - it('returns 401 with an "Current password is invalid" message', () => { - expect(response.status).toHaveBeenCalledWith(401); - expect(response.json).toHaveBeenCalledWith({ - error: 'Current password is invalid.' + it('saves the user with the correct details once', () => { + expect(saveUser).toHaveBeenCalledWith(response, { + ...startingUser }); + expect(saveUser).toHaveBeenCalledTimes(1); }); - it('does not save the user with the new password', () => { - expect(saveUser).not.toHaveBeenCalled(); - }); it('does not send a confirmation email to the user', () => { expect(mailerService.send).not.toHaveBeenCalled(); }); @@ -405,10 +386,9 @@ describe('user.controller > auth management > updateSettings (email, username, p describe('when given old username, old email, and non-matching current password and a new password', () => { beforeEach(async () => { testUser.comparePassword = jest.fn().mockResolvedValue(false); - requestBody = { ...minimumValidRequest, - currentPassword: 'not the same password', + currentPassword: 'wrong', newPassword: NEW_PASSWORD }; request.setBody(requestBody); @@ -422,9 +402,10 @@ describe('user.controller > auth management > updateSettings (email, username, p }); }); - it('does not save the user with the new password', () => { + it('does not save the user', () => { expect(saveUser).not.toHaveBeenCalled(); }); + it('does not send a confirmation email to the user', () => { expect(mailerService.send).not.toHaveBeenCalled(); }); @@ -447,9 +428,10 @@ describe('user.controller > auth management > updateSettings (email, username, p }); }); - it('does not save the user with the new password', () => { + it('does not save the user', () => { expect(saveUser).not.toHaveBeenCalled(); }); + it('does not send a confirmation email to the user', () => { expect(mailerService.send).not.toHaveBeenCalled(); }); @@ -459,11 +441,12 @@ describe('user.controller > auth management > updateSettings (email, username, p describe('and when there is any other error', () => { beforeEach(async () => { - User.findById = jest.fn().mockRejectedValue('db error'); + (User.findById as jest.Mock).mockRejectedValue('db error'); requestBody = minimumValidRequest; request.setBody(requestBody); await updateSettings(request, response, next); }); + it('returns a 500 error', () => { expect(response.status).toHaveBeenCalledWith(500); expect(response.json).toHaveBeenCalledWith({ error: 'db error' }); diff --git a/server/controllers/user.controller/authManagement.ts b/server/controllers/user.controller/authManagement.ts index fbb853ac0f..02be42a711 100644 --- a/server/controllers/user.controller/authManagement.ts +++ b/server/controllers/user.controller/authManagement.ts @@ -9,7 +9,8 @@ import { ResetPasswordInitiateRequestBody, ResetOrUpdatePasswordRequestParams, UpdatePasswordRequestBody, - UpdateSettingsRequestBody + UpdateSettingsRequestBody, + RenderedMailerData } from '../../types'; import { mailerService } from '../../utils/mail'; import { renderResetPassword, renderEmailConfirmation } from '../../views/mail'; @@ -149,46 +150,43 @@ export const updateSettings: RequestHandler< PublicUserOrError, UpdateSettingsRequestBody > = async (req, res) => { + if (!req.body.username) { + res.status(401).json({ error: 'Username is required.' }); + return; + } + + if (!req.body.email) { + res.status(401).json({ error: 'Email is required.' }); + return; + } + try { const user = await User.findById(req.user!.id); if (!user) { res.status(404).json({ error: 'User not found' }); return; } + + // Emails to send after saving + const emailsToSend: RenderedMailerData[] = []; + + // Username (always overwrite) user.username = req.body.username; - if (req.body.newPassword) { - if (user.password === undefined) { - user.password = req.body.newPassword; - saveUser(res, user); - } - if (!req.body.currentPassword) { - res.status(401).json({ error: 'Current password is not provided.' }); - return; - } - } - if (req.body.currentPassword) { - const isMatch = await user.comparePassword(req.body.currentPassword); - if (!isMatch) { - res.status(401).json({ error: 'Current password is invalid.' }); - return; - } - user.password = req.body.newPassword!; - await saveUser(res, user); - } else if (user.email !== req.body.email) { + // Email change + if (user.email !== req.body.email) { + user.email = req.body.email; + const EMAIL_VERIFY_TOKEN_EXPIRY_TIME = Date.now() + 3600000 * 24; // 24 hours user.verified = User.EmailConfirmation().Sent; - user.email = req.body.email; - const token = await generateToken(); user.verifiedToken = token; user.verifiedTokenExpires = EMAIL_VERIFY_TOKEN_EXPIRY_TIME; - await saveUser(res, user); - + // generate email for confirming email change: const protocol = process.env.NODE_ENV === 'production' ? 'https' : 'http'; - const mailOptions = renderEmailConfirmation({ + const confirmEmailChange = renderEmailConfirmation({ body: { domain: `${protocol}://${req.headers.host}`, link: `${protocol}://${req.headers.host}/verify?t=${token}` @@ -196,9 +194,32 @@ export const updateSettings: RequestHandler< to: user.email }); - await mailerService.send(mailOptions); - } else { - await saveUser(res, user); + emailsToSend.push(confirmEmailChange); + } + + // Password + if (req.body.newPassword) { + if (user.password === undefined) { + user.password = req.body.newPassword; + } else if (!req.body.currentPassword) { + res.status(401).json({ error: 'Current password is not provided.' }); + return; + } else { + const isMatch = await user.comparePassword(req.body.currentPassword); + if (!isMatch) { + res.status(401).json({ error: 'Current password is invalid.' }); + return; + } + user.password = req.body.newPassword; + } + } + + // Always save for simplicity + await saveUser(res, user); + + // Send any queued emails (e.g., email verification) + if (emailsToSend.length > 0) { + await Promise.all(emailsToSend.map((email) => mailerService.send(email))); } } catch (err) { res.status(500).json({ error: err });