fix: NodeBB#11259, clean old emails when updating via admin (NodeBB#1…

…1260)

when admin is changing users emails check if its avaiable and remove old email of user first
upgrade script to cleanup email:uid, email:sorted, will remove entries if user doesn't exist or doesn't have email or if entry in user hash doesn't match entry in email:uid
fix missing ! in email interstitial
fix missing await in canSendValidation,
fix broken tests
dont pass sessionId to email.remove if admin is changing/removing email

src/upgrades/2.8.7/fix-email-sorted-sets.js

@@ -0,0 +1,46 @@
+'use strict';
+
+
+const db = require('../../database');
+const batch = require('../../batch');
+
+
+module.exports = {
+	name: 'Fix user email sorted sets',
+	timestamp: Date.UTC(2023, 1, 4),
+	method: async function () {
+		const { progress } = this;
+		const bulkRemove = [];
+		await batch.processSortedSet('email:uid', async (data) => {
+			progress.incr(data.length);
+			const usersData = await db.getObjects(data.map(d => `user:${d.score}`));
+			data.forEach((emailData, index) => {
+				const { score: uid, value: email } = emailData;
+				const userData = usersData[index];
+				// user no longer exists or doesn't have email set in user hash
+				// remove the email/uid pair from email:uid, email:sorted
+				if (!userData || !userData.email) {
+					bulkRemove.push(['email:uid', email]);
+					bulkRemove.push(['email:sorted', `${email.toLowerCase()}:${uid}`]);
+					return;
+				}
+
+				// user has email but doesn't match whats stored in user hash, gh#11259
+				if (userData.email && userData.email.toLowerCase() !== email.toLowerCase()) {
+					bulkRemove.push(['email:uid', email]);
+					bulkRemove.push(['email:sorted', `${email.toLowerCase()}:${uid}`]);
+				}
+			});
+		}, {
+			batch: 500,
+			withScores: true,
+			progress: progress,
+		});
+
+		await batch.processArray(bulkRemove, async (bulk) => {
+			await db.sortedSetRemoveBulk(bulk);
+		}, {
+			batch: 500,
+		});
+	},
+};

src/user/email.js

@@ -39,7 +39,7 @@ UserEmail.remove = async function (uid, sessionId) {
 		db.sortedSetRemove('email:uid', email.toLowerCase()),
 		db.sortedSetRemove('email:sorted', `${email.toLowerCase()}:${uid}`),
 		user.email.expireValidation(uid),
-		user.auth.revokeAllSessions(uid, sessionId),
+		sessionId ? user.auth.revokeAllSessions(uid, sessionId) : Promise.resolve(),
 		events.log({ type: 'email-change', email, newEmail: '' }),
 	]);
 };
@@ -69,7 +69,7 @@ UserEmail.expireValidation = async (uid) => {
 };
 
 UserEmail.canSendValidation = async (uid, email) => {
-	const pending = UserEmail.isValidationPending(uid, email);
+	const pending = await UserEmail.isValidationPending(uid, email);
 	if (!pending) {
 		return true;
 	}
@@ -196,6 +196,20 @@ UserEmail.confirmByUid = async function (uid) {
 		throw new Error('[[error:invalid-email]]');
 	}
 
+	// If another uid has the same email throw error
+	const oldUid = await db.sortedSetScore('email:uid', currentEmail.toLowerCase());
+	if (oldUid && oldUid !== parseInt(uid, 10)) {
+		throw new Error('[[error:email-taken]]');
+	}
+
+	const confirmedEmails = await db.getSortedSetRangeByScore(`email:uid`, 0, -1, uid, uid);
+	if (confirmedEmails.length) {
+		// remove old email of user by uid
+		await db.sortedSetsRemoveRangeByScore([`email:uid`], uid, uid);
+		await db.sortedSetRemoveBulk(
+			confirmedEmails.map(email => [`email:sorted`, `${email.toLowerCase()}:${uid}`])
+		);
+	}
 	await Promise.all([
 		db.sortedSetAddBulk([
 			['email:uid', uid, currentEmail.toLowerCase()],

src/user/interstitials.js

@@ -42,6 +42,7 @@ Interstitials.email = async (data) => {
 		callback: async (userData, formData) => {
 			// Validate and send email confirmation
 			if (userData.uid) {
+				const isSelf = parseInt(userData.uid, 10) === parseInt(data.req.uid, 10);
 				const [isPasswordCorrect, canEdit, { email: current, 'email:confirmed': confirmed }, { allowed, error }] = await Promise.all([
 					user.isPasswordCorrect(userData.uid, formData.password, data.req.ip),
 					privileges.users.canEdit(data.req.uid, userData.uid),
@@ -68,13 +69,17 @@ Interstitials.email = async (data) => {
 					if (formData.email === current) {
 						if (confirmed) {
 							throw new Error('[[error:email-nochange]]');
-						} else if (await user.email.canSendValidation(userData.uid, current)) {
+						} else if (!await user.email.canSendValidation(userData.uid, current)) {
 							throw new Error(`[[error:confirm-email-already-sent, ${meta.config.emailConfirmInterval}]]`);
 						}
 					}
 
 					// Admins editing will auto-confirm, unless editing their own email
 					if (isAdminOrGlobalMod && 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) {
@@ -99,8 +104,8 @@ Interstitials.email = async (data) => {
 					}
 
 					if (current.length && (!hasPassword || (hasPassword && isPasswordCorrect) || isAdminOrGlobalMod)) {
-						// User explicitly clearing their email
-						await user.email.remove(userData.uid, data.req.session.id);
+						// User or admin explicitly clearing their email
+						await user.email.remove(userData.uid, isSelf ? data.req.session.id : null);
 					}
 				}
 			} else {

test/user/emails.js

@@ -120,7 +120,7 @@ describe('email confirmation (library methods)', () => {
 			await user.email.sendValidationEmail(uid, {
 				email,
 			});
-			const ok = await user.email.canSendValidation(uid, 'test@example.com');
+			const ok = await user.email.canSendValidation(uid, email);
 
 			assert.strictEqual(ok, false);
 		});
@@ -131,7 +131,7 @@ describe('email confirmation (library methods)', () => {
 				email,
 			});
 			await db.pexpire(`confirm:byUid:${uid}`, 1000);
-			const ok = await user.email.canSendValidation(uid, 'test@example.com');
+			const ok = await user.email.canSendValidation(uid, email);
 
 			assert(ok);
 		});