Regression: LDAP User Data Sync not always working (#23321)

* Regression: LDAP User Data not updating on login

* Close modal on background sync

* Replaced warning with an error

* Fixed email sync

* Return promise

* Review Items

app/importer/server/classes/ImportDataConverter.ts

@@ -9,9 +9,9 @@ import { IImportChannel } from '../../../../definition/IImportChannel';
 import { IConversionCallbacks } from '../definitions/IConversionCallbacks';
 import { IImportUserRecord, IImportChannelRecord, IImportMessageRecord } from '../../../../definition/IImportRecord';
 import { Users, Rooms, Subscriptions, ImportData } from '../../../models/server';
-import { generateUsernameSuggestion, insertMessage } from '../../../lib/server';
+import { generateUsernameSuggestion, insertMessage, saveUserIdentity, addUserToDefaultChannels } from '../../../lib/server';
 import { setUserActiveStatus } from '../../../lib/server/functions/setUserActiveStatus';
-import { IUser } from '../../../../definition/IUser';
+import { IUser, IUserEmail } from '../../../../definition/IUser';
 import type { Logger } from '../../../../server/lib/logger/Logger';
 
 type IRoom = Record<string, any>;
@@ -148,6 +148,26 @@ export class ImportDataConverter {
 		}
 	}
 
+	addUserEmails(updateData: Record<string, any>, userData: IImportUser, existingEmails: Array<IUserEmail>): void {
+		if (!userData.emails?.length) {
+			return;
+		}
+
+		const verifyEmails = Boolean(this.options.flagEmailsAsVerified);
+		const newEmailList: Array<IUserEmail> = [];
+
+		for (const email of userData.emails) {
+			const verified = verifyEmails || existingEmails.find((ee) => ee.address === email)?.verified || false;
+
+			newEmailList.push({
+				address: email,
+				verified,
+			});
+		}
+
+		updateData.$set.emails = newEmailList;
+	}
+
 	addUserServices(updateData: Record<string, any>, userData: IImportUser): void {
 		if (!userData.services) {
 			return;
@@ -170,14 +190,6 @@ export class ImportDataConverter {
 		}
 	}
 
-	flagEmailsAsVerified(updateData: Record<string, any>, userData: IImportUser): void {
-		if (!this.options.flagEmailsAsVerified || !userData.emails.length) {
-			return;
-		}
-
-		updateData.$set['emails.$[].verified'] = true;
-	}
-
 	addCustomFields(updateData: Record<string, any>, userData: IImportUser): void {
 		if (!userData.customFields) {
 			return;
@@ -202,7 +214,11 @@ export class ImportDataConverter {
 		subset(userData.customFields, 'customFields');
 	}
 
-	updateUserId(_id: string, userData: IImportUser): void {
+	updateUser(existingUser: IUser, userData: IImportUser): void {
+		const { _id } = existingUser;
+
+		userData._id = _id;
+
 		// #ToDo: #TODO: Move this to the model class
 		const updateData: Record<string, any> = {
 			$set: {
@@ -210,35 +226,28 @@ export class ImportDataConverter {
 				type: userData.type || 'user',
 				...userData.statusText && { statusText: userData.statusText },
 				...userData.bio && { bio: userData.bio },
-				...userData.name && { name: userData.name },
 				...userData.services?.ldap && { ldap: true },
+				...userData.avatarUrl && { _pendingAvatarUrl: userData.avatarUrl },
 			},
 		};
 
 		this.addCustomFields(updateData, userData);
 		this.addUserServices(updateData, userData);
 		this.addUserImportId(updateData, userData);
-		this.flagEmailsAsVerified(updateData, userData);
+		this.addUserEmails(updateData, userData, existingUser.emails || []);
 		Users.update({ _id }, updateData);
-	}
 
-	updateUser(existingUser: IUser, userData: IImportUser): void {
-		userData._id = existingUser._id;
+		if (userData.utcOffset) {
+			Users.setUtcOffset(_id, userData.utcOffset);
+		}
 
-		this.updateUserId(userData._id, userData);
+		if (userData.name || userData.username) {
+			saveUserIdentity({ _id, name: userData.name, username: userData.username });
+		}
 
 		if (userData.importIds.length) {
 			this.addUserToCache(userData.importIds[0], existingUser._id, existingUser.username);
 		}
-
-		if (userData.avatarUrl) {
-			try {
-				Users.update({ _id: existingUser._id }, { $set: { _pendingAvatarUrl: userData.avatarUrl } });
-			} catch (error) {
-				this._logger.warn(`Failed to set ${ existingUser._id }'s avatar from url ${ userData.avatarUrl }`);
-				this._logger.error(error);
-			}
-		}
 	}
 
 	insertUser(userData: IImportUser): IUser {
@@ -253,35 +262,10 @@ export class ImportDataConverter {
 			joinDefaultChannelsSilenced: true,
 		});
 
-		userData._id = userId;
 		const user = Users.findOneById(userId, {});
+		this.updateUser(user, userData);
 
-		if (user && userData.importIds.length) {
-			this.addUserToCache(userData.importIds[0], user._id, userData.username);
-		}
-
-		Meteor.runAsUser(userId, () => {
-			Meteor.call('setUsername', userData.username, { joinDefaultChannelsSilenced: true });
-			if (userData.name) {
-				Users.setName(userId, userData.name);
-			}
-
-			this.updateUserId(userId, userData);
-
-			if (userData.utcOffset) {
-				Users.setUtcOffset(userId, userData.utcOffset);
-			}
-
-			if (userData.avatarUrl) {
-				try {
-					Users.update({ _id: userId }, { $set: { _pendingAvatarUrl: userData.avatarUrl } });
-				} catch (error) {
-					this._logger.warn(`Failed to set ${ userId }'s avatar from url ${ userData.avatarUrl }`);
-					this._logger.error(error);
-				}
-			}
-		});
-
+		addUserToDefaultChannels(user, true);
 		return user;
 	}
 

client/views/admin/settings/groups/LDAPGroupPage.tsx

@@ -50,6 +50,8 @@ function LDAPGroupPage({ _id, ...group }: ISetting): JSX.Element {
 		try {
 			await testConnection(undefined);
 			const confirmSync = async (): Promise<void> => {
+				closeModal();
+
 				try {
 					const { message } = await syncNow(undefined);
 					dispatchToastMessage({ type: 'success', message: t(message) });

server/lib/ldap/Connection.ts

@@ -173,7 +173,7 @@ export class LDAPConnection {
 			}
 			searchOptions.filter = new this.ldapjs.filters.OrFilter({ filters });
 		} else {
-			searchLogger.warn('Unique Identifier Field is not configured.');
+			throw new Error('Unique Identifier Field is not configured.');
 		}
 
 		searchLogger.info({ msg: 'Searching by id', id });
@@ -394,7 +394,7 @@ export class LDAPConnection {
 	}
 
 	public async bindDN(dn: string, password: string): Promise<void> {
-		await new Promise<void>((resolve, reject) => {
+		return new Promise<void>((resolve, reject) => {
 			try {
 				this.client.bind(dn, password, (error) => {
 					if (error) {

server/lib/ldap/Manager.ts

@@ -205,19 +205,18 @@ export class LDAPManager {
 	private static async addLdapUser(ldapUser: ILDAPEntry, username: string | undefined, password: string | undefined, ldap: LDAPConnection): Promise<LDAPLoginResult> {
 		const user = await this.syncUserForLogin(ldapUser, undefined, username);
 
-		this.onLogin(ldapUser, user, password, ldap, true);
-		if (user) {
-			return {
-				userId: user._id,
-			};
-		}
-	}
-
-	private static onLogin(ldapUser: ILDAPEntry, user: IUser | undefined, password: string | undefined, ldap: LDAPConnection, isNewUser: boolean): void {
 		if (!user) {
 			return;
 		}
 
+		this.onLogin(ldapUser, user, password, ldap, true);
+
+		return {
+			userId: user._id,
+		};
+	}
+
+	private static onLogin(ldapUser: ILDAPEntry, user: IUser, password: string | undefined, ldap: LDAPConnection, isNewUser: boolean): void {
 		logger.debug('running onLDAPLogin');
 		if (settings.get<boolean>('LDAP_Login_Fallback') && typeof password === 'string' && password.trim() !== '') {
 			Accounts.setPassword(user._id, password, { logout: false });
@@ -233,7 +232,10 @@ export class LDAPManager {
 			throw new Meteor.Error('LDAP-login-error', `LDAP Authentication succeeded, but there's already an existing user with provided username [${ user.username }] in Mongo.`);
 		}
 
-		const syncData = settings.get<boolean>('LDAP_Update_Data_On_Login') ?? true;
+		// If we're merging an ldap user with a local user, then we need to sync the data even if 'update data on login' is off.
+		const forceUserSync = !user.ldap;
+
+		const syncData = forceUserSync || (settings.get<boolean>('LDAP_Update_Data_On_Login') ?? true);
 		logger.debug({ msg: 'Logging user in', syncData });
 		const updatedUser = (syncData && await this.syncUserForLogin(ldapUser, user)) || user;