Chore: break LDAP manager into smaller pieces to improve unit tests

apps/meteor/.mocharc.js

@@ -24,6 +24,7 @@ module.exports = {
 	...base, // see https://github.com/mochajs/mocha/issues/3916
 	exit: true,
 	spec: [
+		'ee/server/lib/ldap/*.spec.ts',
 		'ee/tests/**/*.tests.ts',
 		'ee/tests/**/*.spec.ts',
 		'tests/unit/app/**/*.spec.ts',

apps/meteor/app/importer/server/classes/ImportDataConverter.ts

@@ -217,7 +217,10 @@ export class ImportDataConverter {
 					continue;
 				}
 
-				updateData.$set[keyPath] = source[key];
+				updateData.$set = {
+					...updateData.$set,
+					...{ [keyPath]: source[key] },
+				};
 			}
 		};
 
@@ -237,16 +240,16 @@ export class ImportDataConverter {
 		}
 
 		// #ToDo: #TODO: Move this to the model class
-		const updateData: Record<string, any> = {
-			$set: {
+		const updateData: Record<string, any> = Object.assign(Object.create(null), {
+			$set: Object.assign(Object.create(null), {
 				...(userData.roles && { roles: userData.roles }),
 				...(userData.type && { type: userData.type }),
 				...(userData.statusText && { statusText: userData.statusText }),
 				...(userData.bio && { bio: userData.bio }),
 				...(userData.services?.ldap && { ldap: true }),
 				...(userData.avatarUrl && { _pendingAvatarUrl: userData.avatarUrl }),
-			},
-		};
+			}),
+		});
 
 		this.addCustomFields(updateData, userData);
 		this.addUserServices(updateData, userData);

apps/meteor/app/utils/client/index.ts

@@ -9,6 +9,5 @@ export { getUserNotificationPreference } from '../lib/getUserNotificationPrefere
 export { getAvatarColor } from '../lib/getAvatarColor';
 export { getURL } from '../lib/getURL';
 export { placeholders } from '../lib/placeholders';
-export { templateVarHandler } from '../lib/templateVarHandler';
 export { APIClient } from './lib/RestApiClient';
 export { secondsToHHMMSS } from '../../../lib/utils/secondsToHHMMSS';

apps/meteor/app/utils/lib/templateVarHandler.js

@@ -1,11 +1,6 @@
-import { Meteor } from 'meteor/meteor';
+import { Logger } from '../../../server/lib/logger/Logger';
 
-let logger;
-
-if (Meteor.isServer) {
-	const { Logger } = require('../../../server/lib/logger/Logger');
-	logger = new Logger('TemplateVarHandler');
-}
+const logger = new Logger('TemplateVarHandler');
 
 export const templateVarHandler = function (variable, object) {
 	const templateRegex = /#{([\w\-]+)}/gi;

apps/meteor/ee/server/lib/ldap/Manager.ts

@@ -1,4 +1,3 @@
-import _ from 'underscore';
 import type ldapjs from 'ldapjs';
 import type { ILDAPEntry, IUser, IRoom, ICreatedRoom, IRole, IImportUser } from '@rocket.chat/core-typings';
 import { Users as UsersRaw, Roles, Subscriptions as SubscriptionsRaw } from '@rocket.chat/models';
@@ -10,11 +9,11 @@ import { LDAPDataConverter } from '../../../../server/lib/ldap/DataConverter';
 import { LDAPConnection } from '../../../../server/lib/ldap/Connection';
 import { LDAPManager } from '../../../../server/lib/ldap/Manager';
 import { logger, searchLogger, mapLogger } from '../../../../server/lib/ldap/Logger';
-import { templateVarHandler } from '../../../../app/utils/lib/templateVarHandler';
 import { addUserToRoom, removeUserFromRoom, createRoom } from '../../../../app/lib/server/functions';
 import { syncUserRoles } from '../syncUserRoles';
 import { Team } from '../../../../server/sdk';
 import { ensureArray } from '../../../../lib/utils/arrayUtils';
+import { copyCustomFieldsLDAP } from './copyCustomFieldsLDAP';
 
 export class LDAPEEManager extends LDAPManager {
 	public static async sync(): Promise<void> {
@@ -506,76 +505,16 @@ export class LDAPEEManager extends LDAPManager {
 	}
 
 	public static copyCustomFields(ldapUser: ILDAPEntry, userData: IImportUser): void {
-		if (!settings.get<boolean>('LDAP_Sync_Custom_Fields')) {
-			return;
-		}
-
-		const customFieldsSettings = settings.get<string>('Accounts_CustomFields');
-		const customFieldsMap = settings.get<string>('LDAP_CustomFieldMap');
-
-		if (!customFieldsMap || !customFieldsSettings) {
-			if (customFieldsMap) {
-				logger.debug('Skipping LDAP custom fields because there are no custom fields configured.');
-			}
-			return;
-		}
-
-		let map: Record<string, string>;
-		try {
-			map = JSON.parse(customFieldsMap) as Record<string, string>;
-		} catch (error) {
-			logger.error('Failed to parse LDAP Custom Fields mapping');
-			logger.error(error);
-			return;
-		}
-
-		let customFields: Record<string, any>;
-		try {
-			customFields = JSON.parse(customFieldsSettings) as Record<string, any>;
-		} catch (error) {
-			logger.error('Failed to parse Custom Fields');
-			logger.error(error);
-			return;
-		}
-
-		_.map(map, (userField, ldapField) => {
-			if (!this.getCustomField(customFields, userField)) {
-				logger.debug(`User attribute does not exist: ${userField}`);
-				return;
-			}
-
-			if (!userData.customFields) {
-				userData.customFields = {};
-			}
-
-			const value = templateVarHandler(ldapField, ldapUser);
-
-			if (value) {
-				let ref: Record<string, any> = userData.customFields;
-				const attributeNames = userField.split('.');
-				let previousKey: string | undefined;
-
-				for (const key of attributeNames) {
-					if (previousKey) {
-						if (ref[previousKey] === undefined) {
-							ref[previousKey] = {};
-						} else if (typeof ref[previousKey] !== 'object') {
-							logger.error(`Failed to assign custom field: ${userField}`);
-							return;
-						}
-
-						ref = ref[previousKey];
-					}
-
-					previousKey = key;
-				}
-
-				if (previousKey) {
-					ref[previousKey] = value;
-					logger.debug(`user.customFields.${userField} changed to: ${value}`);
-				}
-			}
-		});
+		return copyCustomFieldsLDAP(
+			{
+				ldapUser,
+				userData,
+				customFieldsSettings: settings.get<string>('Accounts_CustomFields'),
+				customFieldsMap: settings.get<string>('LDAP_CustomFieldMap'),
+				syncCustomFields: settings.get<boolean>('LDAP_Sync_Custom_Fields'),
+			},
+			logger,
+		);
 	}
 
 	private static async importNewUsers(ldap: LDAPConnection, converter: LDAPDataConverter): Promise<void> {
@@ -660,12 +599,4 @@ export class LDAPEEManager extends LDAPManager {
 			}
 		}
 	}
-
-	private static getCustomField(customFields: Record<string, any>, property: string): any {
-		try {
-			return _.reduce(property.split('.'), (acc, el) => acc[el], customFields);
-		} catch {
-			// ignore errors
-		}
-	}
 }

apps/meteor/ee/server/lib/ldap/copyCustomFieldsLDAP.spec.ts

@@ -0,0 +1,209 @@
+import type { IImportUser, ILDAPEntry } from '@rocket.chat/core-typings';
+import { expect, spy } from 'chai';
+
+import { copyCustomFieldsLDAP } from './copyCustomFieldsLDAP';
+import type { Logger } from '../../../../app/logger/server';
+
+describe('LDAP copyCustomFieldsLDAP', () => {
+	it('should copy custom fields from ldapUser to rcUser', () => {
+		const ldapUser = {
+			mail: 'test@test.com',
+			givenName: 'Test',
+		} as unknown as ILDAPEntry;
+
+		const userData = {
+			name: 'Test',
+			username: 'test',
+		} as unknown as IImportUser;
+
+		copyCustomFieldsLDAP(
+			{
+				ldapUser,
+				userData,
+				syncCustomFields: true,
+				customFieldsSettings: JSON.stringify({
+					mappedGivenName: { type: 'text', required: false },
+				}),
+				customFieldsMap: JSON.stringify({
+					givenName: 'mappedGivenName',
+				}),
+			},
+			{
+				debug: () => undefined,
+				error: () => undefined,
+			} as unknown as Logger,
+		);
+
+		expect(userData).to.have.property('customFields');
+		expect(userData.customFields).to.be.eql({ mappedGivenName: 'Test' });
+	});
+
+	it('should copy custom fields from ldapUser to rcUser already having other custom fields', () => {
+		const ldapUser = {
+			mail: 'test@test.com',
+			givenName: 'Test',
+		} as unknown as ILDAPEntry;
+
+		const userData = {
+			name: 'Test',
+			username: 'test',
+			customFields: {
+				custom: 'Test',
+			},
+		} as unknown as IImportUser;
+
+		copyCustomFieldsLDAP(
+			{
+				ldapUser,
+				userData,
+				syncCustomFields: true,
+				customFieldsSettings: JSON.stringify({
+					mappedGivenName: { type: 'text', required: false },
+				}),
+				customFieldsMap: JSON.stringify({
+					givenName: 'mappedGivenName',
+				}),
+			},
+			{
+				debug: () => undefined,
+				error: () => undefined,
+			} as unknown as Logger,
+		);
+
+		expect(userData).to.have.property('customFields');
+		expect(userData.customFields).to.be.eql({ custom: 'Test', mappedGivenName: 'Test' });
+	});
+
+	it('should not copy custom fields from ldapUser to rcUser if syncCustomFields is false', () => {
+		const ldapUser = {
+			mail: 'test@test.com',
+			givenName: 'Test',
+		} as unknown as ILDAPEntry;
+
+		const userData = {
+			name: 'Test',
+			username: 'test',
+		} as unknown as IImportUser;
+
+		copyCustomFieldsLDAP(
+			{
+				ldapUser,
+				userData,
+				syncCustomFields: false,
+				customFieldsSettings: JSON.stringify({
+					mappedGivenName: { type: 'text', required: false },
+				}),
+				customFieldsMap: JSON.stringify({
+					givenName: 'mappedGivenName',
+				}),
+			},
+			{
+				debug: () => undefined,
+				error: () => undefined,
+			} as unknown as Logger,
+		);
+
+		expect(userData).to.not.have.property('customFields');
+	});
+
+	it('should call logger.error if customFieldsSettings is not a valid JSON', () => {
+		const debug = spy();
+		const error = spy();
+		const ldapUser = {
+			mail: 'test@test.com',
+			givenName: 'Test',
+		} as unknown as ILDAPEntry;
+
+		const userData = {
+			name: 'Test',
+			username: 'test',
+		} as unknown as IImportUser;
+
+		copyCustomFieldsLDAP(
+			{
+				ldapUser,
+				userData,
+				syncCustomFields: true,
+				customFieldsSettings: `${JSON.stringify({
+					mappedGivenName: { type: 'text', required: false },
+				})}}`,
+				customFieldsMap: JSON.stringify({
+					givenName: 'mappedGivenName',
+				}),
+			},
+			{
+				debug,
+				error,
+			} as unknown as Logger,
+		);
+		expect(error).to.have.been.called.exactly(1);
+	});
+	it('should call logger.error if customFieldsMap is not a valid JSON', () => {
+		const debug = spy();
+		const error = spy();
+		const ldapUser = {
+			mail: 'test@test.com',
+			givenName: 'Test',
+		} as unknown as ILDAPEntry;
+
+		const userData = {
+			name: 'Test',
+			username: 'test',
+		} as unknown as IImportUser;
+
+		copyCustomFieldsLDAP(
+			{
+				ldapUser,
+				userData,
+				syncCustomFields: true,
+				customFieldsSettings: JSON.stringify({
+					mappedGivenName: { type: 'text', required: false },
+				}),
+				customFieldsMap: `${JSON.stringify({
+					givenName: 'mappedGivenName',
+				})}}`,
+			},
+			{
+				debug,
+				error,
+			} as unknown as Logger,
+		);
+		expect(error).to.have.been.called.exactly(1);
+	});
+
+	it('should call logger.debug if some custom fields are not mapped but still mapping the other fields', () => {
+		const debug = spy();
+		const error = spy();
+		const ldapUser = {
+			mail: 'test@test.com',
+			givenName: 'Test',
+		} as unknown as ILDAPEntry;
+
+		const userData = {
+			name: 'Test',
+			username: 'test',
+		} as unknown as IImportUser;
+
+		copyCustomFieldsLDAP(
+			{
+				ldapUser,
+				userData,
+				syncCustomFields: true,
+				customFieldsSettings: JSON.stringify({
+					mappedGivenName: { type: 'text', required: false },
+				}),
+				customFieldsMap: JSON.stringify({
+					givenName: 'mappedGivenName',
+					test: 'test',
+				}),
+			},
+			{
+				debug,
+				error,
+			} as unknown as Logger,
+		);
+		expect(debug).to.have.been.called.exactly(1);
+		expect(userData).to.have.property('customFields');
+		expect(userData.customFields).to.be.eql({ mappedGivenName: 'Test' });
+	});
+});