From 8db13d8e86b01aa57be4eafbcdf1b3c588d694a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bar=C4=B1=C5=9F=20Soner=20U=C5=9Fakl=C4=B1?= Date: Wed, 30 Aug 2023 19:29:46 -0400 Subject: [PATCH] refactor: how admins change emails (#11973) * refactor: how admins change emails ability for admins to change emails from acp ability for admins to change passwords from acp only users themselves can use /user//edit/email group actions in manage users dropdown admins can use the same modal from profile page instead of interstitial to update email add missing checks to addEmail, if email take throw error add targetUid to email change event * test: bunch of baloney * test: remove old test --- public/language/en-GB/admin/manage/users.json | 17 ++++- public/language/en-GB/user.json | 1 + public/openapi/write/users/uid/emails.yaml | 4 +- public/src/admin/manage/users.js | 69 ++++++++++++++++- public/src/admin/modules/change-email.js | 41 ++++++++++ public/src/client/account/edit.js | 16 +++- public/src/client/account/profile.js | 5 +- src/api/users.js | 11 ++- src/controllers/accounts/edit.js | 11 +-- src/user/email.js | 7 +- src/user/interstitials.js | 25 ++----- src/views/admin/manage/users.tpl | 74 +++++++++++-------- test/api.js | 5 ++ test/controllers.js | 16 ---- 14 files changed, 217 insertions(+), 85 deletions(-) create mode 100644 public/src/admin/modules/change-email.js diff --git a/public/language/en-GB/admin/manage/users.json b/public/language/en-GB/admin/manage/users.json index 1fef96f56d..08e5701f40 100644 --- a/public/language/en-GB/admin/manage/users.json +++ b/public/language/en-GB/admin/manage/users.json @@ -4,16 +4,21 @@ "edit": "Actions", "make-admin": "Make Admin", "remove-admin": "Remove Admin", + "change-email": "Change Email", + "new-email": "New Email", "validate-email": "Validate Email", "send-validation-email": "Send Validation Email", + "change-password": "Change Password", "password-reset-email": "Send Password Reset Email", "force-password-reset": "Force Password Reset & Log User Out", - "ban": "Ban User(s)", + "ban": "Ban", + "ban-users": "Ban User(s)", "temp-ban": "Ban User(s) Temporarily", "unban": "Unban User(s)", "reset-lockout": "Reset Lockout", "reset-flags": "Reset Flags", - "delete": "Delete User(s)", + "delete": "Delete", + "delete-users": "Delete User(s)", "delete-content": "Delete User(s) Content", "purge": "Delete User(s) and Content", "download-csv": "Download CSV", @@ -81,6 +86,7 @@ "alerts.button-ban-x": "Ban %1 user(s)", "alerts.unban-success": "User(s) unbanned!", "alerts.lockout-reset-success": "Lockout(s) reset!", + "alerts.password-change-success": "Password(s) changed!", "alerts.flag-reset-success": "Flags(s) reset!", "alerts.no-remove-yourself-admin": "You can't remove yourself as Administrator!", "alerts.make-admin-success": "User is now administrator.", @@ -106,6 +112,7 @@ "alerts.create": "Create User", "alerts.button-create": "Create", "alerts.button-cancel": "Cancel", + "alerts.button-change": "Change", "alerts.error-passwords-different": "Passwords must match!", "alerts.error-x": "Error

%1

", "alerts.create-success": "User created!", @@ -113,6 +120,10 @@ "alerts.prompt-email": "Emails: ", "alerts.email-sent-to": "An invitation email has been sent to %1", "alerts.x-users-found": "%1 user(s) found, (%2 seconds)", + "alerts.select-a-single-user-to-change-email": "Select a single user to change email", "export-users-started": "Exporting users as csv, this might take a while. You will receive a notification when it is complete.", - "export-users-completed": "Users exported as csv, click here to download." + "export-users-completed": "Users exported as csv, click here to download.", + "email": "Email", + "password": "Password", + "manage": "Manage" } \ No newline at end of file diff --git a/public/language/en-GB/user.json b/public/language/en-GB/user.json index 69bb631da7..e56cdfd4c1 100644 --- a/public/language/en-GB/user.json +++ b/public/language/en-GB/user.json @@ -64,6 +64,7 @@ "change_picture": "Change Picture", "change_username": "Change Username", "change_email": "Change Email", + "email-updated": "Email Updated", "email_same_as_password": "Please enter your current password to continue – you've entered your new email again", "edit": "Edit", "edit-profile": "Edit Profile", diff --git a/public/openapi/write/users/uid/emails.yaml b/public/openapi/write/users/uid/emails.yaml index c6b67acf9e..3c046a0dee 100644 --- a/public/openapi/write/users/uid/emails.yaml +++ b/public/openapi/write/users/uid/emails.yaml @@ -81,4 +81,6 @@ post: type: array items: type: string - description: An email address \ No newline at end of file + description: An email address + '400': + description: error occured (aka email taken) diff --git a/public/src/admin/manage/users.js b/public/src/admin/manage/users.js index 613389d9f9..d2ac9d20c1 100644 --- a/public/src/admin/manage/users.js +++ b/public/src/admin/manage/users.js @@ -1,8 +1,8 @@ 'use strict'; define('admin/manage/users', [ - 'translator', 'benchpress', 'autocomplete', 'api', 'slugify', 'bootbox', 'alerts', 'accounts/invite', 'helpers', -], function (translator, Benchpress, autocomplete, api, slugify, bootbox, alerts, AccountInvite, helpers) { + 'translator', 'benchpress', 'autocomplete', 'api', 'slugify', 'bootbox', 'alerts', 'accounts/invite', 'helpers', 'admin/modules/change-email', +], function (translator, Benchpress, autocomplete, api, slugify, bootbox, alerts, AccountInvite, helpers, changeEmail) { const Users = {}; Users.init = function () { @@ -273,6 +273,26 @@ define('admin/manage/users', [ socket.emit('admin.user.resetLockouts', uids, done('[[admin/manage/users:alerts.lockout-reset-success]]')); }); + $('.change-email').on('click', function () { + const uids = getSelectedUids(); + if (uids.length !== 1) { + return alerts.error('[[admin/manage/users:alerts.select-a-single-user-to-change-email]]'); + } + changeEmail.init({ + uid: uids[0], + onSuccess: function (newEmail) { + update('.notvalidated', false); + update('.pending', false); + update('.expired', false); + update('.validated', false); + update('.validated-by-admin', !!newEmail); + update('.no-email', !newEmail); + $('.users-table [component="user/select/single"]:checked').parents('.user-row').find('.validated-by-admin .email').text(newEmail); + // $('.users-table [component="user/select/single"]:checked').parents('.user-row').find('.no-email'). + }, + }); + }); + $('.validate-email').on('click', function () { const uids = getSelectedUids(); if (!uids.length) { @@ -311,6 +331,51 @@ define('admin/manage/users', [ }); }); + $('.change-password').on('click', async function () { + const uids = getSelectedUids(); + if (!uids.length) { + return; + } + async function changePassword(modal) { + const newPassword = modal.find('#newPassword').val(); + const confirmPassword = modal.find('#confirmPassword').val(); + if (newPassword !== confirmPassword) { + throw new Error('[[[user:change_password_error_match]]'); + } + await Promise.all(uids.map(uid => api.put('/users/' + uid + '/password', { + currentPassword: '', + newPassword: newPassword, + }))); + } + + const modal = bootbox.dialog({ + message: `
+ + + + +
`, + title: '[[admin/manage/users:change-password]]', + onEscape: true, + buttons: { + cancel: { + label: '[[admin/manage/users:alerts.button-cancel]]', + className: 'btn-link', + }, + change: { + label: '[[admin/manage/users:alerts.button-change]]', + className: 'btn-primary', + callback: function () { + changePassword(modal).then(() => { + modal.modal('hide'); + }).catch(alerts.error); + return false; + }, + }, + }, + }); + }); + $('.password-reset-email').on('click', function () { const uids = getSelectedUids(); if (!uids.length) { diff --git a/public/src/admin/modules/change-email.js b/public/src/admin/modules/change-email.js new file mode 100644 index 0000000000..f237792854 --- /dev/null +++ b/public/src/admin/modules/change-email.js @@ -0,0 +1,41 @@ +'use strict'; + +define('admin/modules/change-email', [ + 'api', 'bootbox', 'alerts', +], function (api, bootbox, alerts) { + const ChangeEmail = {}; + + ChangeEmail.init = function (params) { + const modal = bootbox.dialog({ + message: ` + + + `, + title: '[[admin/manage/users:change-email]]', + onEscape: true, + buttons: { + cancel: { + label: '[[admin/manage/users:alerts.button-cancel]]', + className: 'btn-link', + }, + change: { + label: '[[admin/manage/users:alerts.button-change]]', + className: 'btn-primary', + callback: function () { + const newEmail = modal.find('#newEmail').val(); + api.post('/users/' + params.uid + '/emails', { + skipConfirmation: true, + email: newEmail, + }).then(() => { + modal.modal('hide'); + params.onSuccess(newEmail); + }).catch(alerts.error); + return false; + }, + }, + }, + }); + }; + + return ChangeEmail; +}); diff --git a/public/src/client/account/edit.js b/public/src/client/account/edit.js index 929767bb4a..587b4055ab 100644 --- a/public/src/client/account/edit.js +++ b/public/src/client/account/edit.js @@ -8,7 +8,8 @@ define('forum/account/edit', [ 'hooks', 'bootbox', 'alerts', -], function (header, picture, translator, api, hooks, bootbox, alerts) { + 'admin/modules/change-email', +], function (header, picture, translator, api, hooks, bootbox, alerts, changeEmail) { const AccountEdit = {}; AccountEdit.init = function () { @@ -25,6 +26,19 @@ define('forum/account/edit', [ updateSignature(); updateAboutMe(); handleGroupSort(); + + if (!ajaxify.data.isSelf && app.user.isAdmin) { + $(`a[href="${config.relative_path}/user/${ajaxify.data.userslug}/edit/email"]`).on('click', () => { + changeEmail.init({ + uid: ajaxify.data.uid, + email: ajaxify.data.email, + onSuccess: function () { + alerts.success('[[user:email-updated]]'); + }, + }); + return false; + }); + } }; function updateProfile() { diff --git a/public/src/client/account/profile.js b/public/src/client/account/profile.js index 03a91066e1..dc2885e80e 100644 --- a/public/src/client/account/profile.js +++ b/public/src/client/account/profile.js @@ -15,7 +15,10 @@ define('forum/account/profile', [ processPage(); if (parseInt(ajaxify.data.emailChanged, 10) === 1) { - bootbox.alert('[[user:emailUpdate.change-instructions]]'); + bootbox.alert({ + message: '[[user:emailUpdate.change-instructions]]', + closeButton: false, + }); } socket.removeListener('event:user_status_change', onUserStatusChange); diff --git a/src/api/users.js b/src/api/users.js index 84fe4b2b3b..d4456128d1 100644 --- a/src/api/users.js +++ b/src/api/users.js @@ -417,8 +417,15 @@ usersAPI.addEmail = async (caller, { email, skipConfirmation, uid }) => { skipConfirmation = canManageUsers && skipConfirmation; if (skipConfirmation) { - await user.setUserField(uid, 'email', email); - await user.email.confirmByUid(uid); + if (!email.length) { + await user.email.remove(uid); + } else { + if (!await user.email.available(email)) { + throw new Error('[[error:email-taken]]'); + } + await user.setUserField(uid, 'email', email); + await user.email.confirmByUid(uid); + } } else { await usersAPI.update(caller, { uid, email }); } diff --git a/src/controllers/accounts/edit.js b/src/controllers/accounts/edit.js index 8560f043e8..f3b8ce58ec 100644 --- a/src/controllers/accounts/edit.js +++ b/src/controllers/accounts/edit.js @@ -90,16 +90,7 @@ editController.username = async function (req, res, next) { editController.email = async function (req, res, next) { const targetUid = await user.getUidByUserslug(req.params.userslug); - if (!targetUid) { - return next(); - } - - const [isAdminOrGlobalMod, canEdit] = await Promise.all([ - user.isAdminOrGlobalMod(req.uid), - privileges.users.canEdit(req.uid, targetUid), - ]); - - if (!isAdminOrGlobalMod && !canEdit) { + if (!targetUid || req.uid !== parseInt(targetUid, 10)) { return next(); } diff --git a/src/user/email.js b/src/user/email.js index c5d025cb15..9a18a62e34 100644 --- a/src/user/email.js +++ b/src/user/email.js @@ -40,7 +40,12 @@ UserEmail.remove = async function (uid, sessionId) { db.sortedSetRemove('email:sorted', `${email.toLowerCase()}:${uid}`), user.email.expireValidation(uid), sessionId ? user.auth.revokeAllSessions(uid, sessionId) : Promise.resolve(), - events.log({ type: 'email-change', email, newEmail: '' }), + events.log({ + targetUid: uid, + type: 'email-change', + email, + newEmail: '', + }), ]); }; diff --git a/src/user/interstitials.js b/src/user/interstitials.js index fbdb63f9ab..c16cb48ff0 100644 --- a/src/user/interstitials.js +++ b/src/user/interstitials.js @@ -28,8 +28,7 @@ Interstitials.email = async (data) => { return data; } - const [canManageUsers, hasPassword, hasPending] = await Promise.all([ - privileges.admin.can('admin:users', data.req.uid), + const [hasPassword, hasPending] = await Promise.all([ user.hasPassword(data.userData.uid), user.email.isValidationPending(data.userData.uid), ]); @@ -44,12 +43,7 @@ Interstitials.email = async (data) => { data: { email, requireEmailAddress: meta.config.requireEmailAddress, - issuePasswordChallenge: - hasPassword && - ( - (canManageUsers && data.userData.uid === data.req.uid) || // admin changing own email - (!canManageUsers && !!data.userData.uid) // non-admins changing own email - ), + issuePasswordChallenge: hasPassword, hasPending, }, callback: async (userData, formData) => { @@ -73,7 +67,7 @@ Interstitials.email = async (data) => { }), ]); - if (!canManageUsers && !isPasswordCorrect) { + if (!isPasswordCorrect) { await sleep(2000); } @@ -92,14 +86,7 @@ Interstitials.email = async (data) => { } // Admins editing will auto-confirm, unless editing their own email - if (canManageUsers && userData.uid !== data.req.uid) { - if (!await user.email.available(formData.email)) { - throw new Error('[[error:email-taken]]'); - } - await user.email.remove(userData.uid); - await user.setUserField(userData.uid, 'email', formData.email); - await user.email.confirmByUid(userData.uid); - } else if (canEdit) { + if (canEdit) { if (hasPassword && !isPasswordCorrect) { throw new Error('[[error:invalid-password]]'); } @@ -120,8 +107,8 @@ Interstitials.email = async (data) => { throw new Error('[[error:invalid-email]]'); } - if (current.length && (!hasPassword || (hasPassword && isPasswordCorrect) || canManageUsers)) { - // User or admin explicitly clearing their email + if (current.length && (!hasPassword || (hasPassword && isPasswordCorrect))) { + // User explicitly clearing their email await user.email.remove(userData.uid, isSelf ? data.req.session.id : null); } } diff --git a/src/views/admin/manage/users.tpl b/src/views/admin/manage/users.tpl index 23127a9876..5c76ac94cc 100644 --- a/src/views/admin/manage/users.tpl +++ b/src/views/admin/manage/users.tpl @@ -39,22 +39,40 @@
-
@@ -105,36 +123,34 @@ {users.username} - +
- {{{ if (!./email && !./emailToConfirm) }}} - [[admin/manage/users:users.no-email]] - {{{ else }}} + [[admin/manage/users:users.no-email]] + - {{{ if ./email }}}{./email}{{{ end }}} + - + - {./emailToConfirm} + - + - {./emailToConfirm} + - + - {./emailToConfirm} + - {{{ end }}}
diff --git a/test/api.js b/test/api.js index 6723cd3f5e..7abfec005e 100644 --- a/test/api.js +++ b/test/api.js @@ -547,6 +547,11 @@ describe('API', async () => { return; } + if (response.statusCode === 400 && context[method].responses['400']) { + // TODO: check 400 schema to response.body? + return; + } + const http200 = context[method].responses['200']; if (!http200) { return; diff --git a/test/controllers.js b/test/controllers.js index 70e3b8a80a..87e7a460d7 100644 --- a/test/controllers.js +++ b/test/controllers.js @@ -455,22 +455,6 @@ describe('Controllers', () => { assert.strictEqual(result.req.session.emailChanged, 1); }); - it('should set email if admin is changing it', async () => { - const uid = await user.create({ username: 'interstiuser3' }); - const result = await user.interstitials.email({ - userData: { uid: uid, updateEmail: true }, - req: { uid: adminUid }, - interstitials: [], - }); - - await result.interstitials[0].callback({ uid: uid }, { - email: 'interstiuser3@nodebb.org', - }); - const userData = await user.getUserData(uid); - assert.strictEqual(userData.email, 'interstiuser3@nodebb.org'); - assert.strictEqual(userData['email:confirmed'], 1); - }); - it('should throw error if user tries to edit other users email', async () => { const uid = await user.create({ username: 'interstiuser4' }); try {