Chore: Improvements on role syncing (ldap, oauth and saml)

app/authorization/server/functions/addUserRoles.ts

@@ -2,36 +2,32 @@ import { Meteor } from 'meteor/meteor';
 import _ from 'underscore';
 
 import { getRoles } from './getRoles';
-import { Users } from '../../../models/server';
 import { IRole, IUser } from '../../../../definition/IUser';
-import { Roles } from '../../../models/server/raw';
+import { Users, Roles } from '../../../models/server/raw';
 
-export const addUserRoles = (userId: IUser['_id'], roleNames: IRole['name'][], scope?: string): boolean => {
+export const addUserRolesAsync = async (userId: IUser['_id'], roleNames: IRole['name'][], scope?: string): Promise<boolean> => {
 	if (!userId || !roleNames) {
 		return false;
 	}
 
-	const user = Users.db.findOneById(userId);
+	const user = await Users.findOneById(userId);
 	if (!user) {
 		throw new Meteor.Error('error-invalid-user', 'Invalid user', {
 			function: 'RocketChat.authz.addUserRoles',
 		});
 	}
 
-	if (!Array.isArray(roleNames)) {
-		// TODO: remove this check
-		roleNames = [roleNames];
-	}
-
 	const existingRoleNames = _.pluck(getRoles(), '_id');
 	const invalidRoleNames = _.difference(roleNames, existingRoleNames);
 
 	if (!_.isEmpty(invalidRoleNames)) {
-		for (const role of invalidRoleNames) {
-			Promise.await(Roles.createOrUpdate(role));
+		for await (const role of invalidRoleNames) {
+			await Roles.createOrUpdate(role);
 		}
 	}
 
-	Promise.await(Roles.addUserRoles(userId, roleNames, scope));
-	return true;
+	return Roles.addUserRoles(userId, roleNames, scope);
 };
+
+export const addUserRoles = (userId: IUser['_id'], roleNames: IRole['name'][], scope?: string): boolean =>
+	Promise.await(addUserRolesAsync(userId, roleNames, scope));

app/authorization/server/functions/removeUserFromRoles.ts

@@ -0,0 +1,41 @@
+import { Meteor } from 'meteor/meteor';
+import _ from 'underscore';
+
+import { getRoles } from './getRoles';
+import { IRole, IUser } from '../../../../definition/IUser';
+import { Users, Roles } from '../../../models/server/raw';
+import { ensureArray } from '../../../../lib/utils/arrayUtils';
+
+export const removeUserFromRolesAsync = async (
+	userId: IUser['_id'],
+	roleNames: IRole['name'] | Array<IRole['name']>,
+	scope?: string,
+): Promise<boolean> => {
+	if (!userId || !roleNames) {
+		return false;
+	}
+
+	const user = await Users.findOneById(userId);
+
+	if (!user) {
+		throw new Meteor.Error('error-invalid-user', 'Invalid user', {
+			function: 'RocketChat.authz.removeUserFromRoles',
+		});
+	}
+
+	roleNames = ensureArray(roleNames);
+
+	const existingRoleNames = _.pluck(getRoles(), '_id');
+	const invalidRoleNames = _.difference(roleNames, existingRoleNames);
+
+	if (!_.isEmpty(invalidRoleNames)) {
+		throw new Meteor.Error('error-invalid-role', 'Invalid role', {
+			function: 'RocketChat.authz.removeUserFromRoles',
+		});
+	}
+
+	return Roles.removeUserRoles(userId, roleNames, scope);
+};
+
+export const removeUserFromRoles = (userId: IUser['_id'], roleNames: IRole['name'] | Array<IRole['name']>, scope?: string): boolean =>
+	Promise.await(removeUserFromRolesAsync(userId, roleNames, scope));

app/authorization/server/index.js

@@ -1,11 +1,11 @@
-import { addUserRoles } from './functions/addUserRoles';
+import { addUserRolesAsync, addUserRoles } from './functions/addUserRoles';
 import { canAccessRoom, canAccessRoomId, roomAccessAttributes, roomAccessValidators } from './functions/canAccessRoom';
 import { canSendMessage, validateRoomMessagePermissions } from './functions/canSendMessage';
 import { getRoles } from './functions/getRoles';
 import { getUsersInRole } from './functions/getUsersInRole';
 import { hasAllPermission, hasAtLeastOnePermission, hasPermission } from './functions/hasPermission';
 import { hasRole, subscriptionHasRole } from './functions/hasRole';
-import { removeUserFromRoles } from './functions/removeUserFromRoles';
+import { removeUserFromRoles, removeUserFromRolesAsync } from './functions/removeUserFromRoles';
 import { AuthorizationUtils } from '../lib/AuthorizationUtils';
 import './methods/addPermissionToRole';
 import './methods/addUserToRole';
@@ -21,9 +21,11 @@ export {
 	hasRole,
 	subscriptionHasRole,
 	removeUserFromRoles,
+	removeUserFromRolesAsync,
 	canSendMessage,
 	validateRoomMessagePermissions,
 	roomAccessValidators,
+	addUserRolesAsync,
 	addUserRoles,
 	canAccessRoom,
 	canAccessRoomId,

app/meteor-accounts-saml/server/lib/SAML.ts

@@ -19,6 +19,7 @@ import { ISAMLAction } from '../definition/ISAMLAction';
 import { ISAMLUser } from '../definition/ISAMLUser';
 import { SAMLUtils } from './Utils';
 import { SystemLogger } from '../../../../server/lib/logger/system';
+import { ensureArray } from '../../../../lib/utils/arrayUtils';
 
 const showErrorMessage = function (res: ServerResponse, err: string): void {
 	res.writeHead(200, {
@@ -127,7 +128,7 @@ export class SAML {
 
 		if (!user) {
 			// If we received any role from the mapping, use them - otherwise use the default role for creation.
-			const roles = userObject.roles?.length ? userObject.roles : SAMLUtils.ensureArray<string>(defaultUserRole.split(','));
+			const roles = userObject.roles?.length ? userObject.roles : ensureArray<string>(defaultUserRole.split(','));
 
 			const newUser: Record<string, any> = {
 				name: userObject.fullName,

app/meteor-accounts-saml/server/lib/Utils.ts

@@ -9,6 +9,7 @@ import { ISAMLGlobalSettings } from '../definition/ISAMLGlobalSettings';
 import { IUserDataMap, IAttributeMapping } from '../definition/IAttributeMapping';
 import { StatusCode } from './constants';
 import { Logger } from '../../../../server/lib/logger/Logger';
+import { ensureArray } from '../../../../lib/utils/arrayUtils';
 
 let providerList: Array<IServiceProviderOptions> = [];
 let debug = false;
@@ -327,7 +328,7 @@ export class SAMLUtils {
 		const values: Record<string, string> = {
 			regex: '',
 		};
-		const fieldNames = this.ensureArray<string>(mapping.fieldName);
+		const fieldNames = ensureArray<string>(mapping.fieldName);
 
 		let mainValue;
 		for (const fieldName of fieldNames) {
@@ -406,11 +407,6 @@ export class SAMLUtils {
 		return name;
 	}
 
-	public static ensureArray<T>(param: T | Array<T>): Array<T> {
-		const emptyArray: Array<T> = [];
-		return emptyArray.concat(param);
-	}
-
 	public static mapProfileToUserObject(profile: Record<string, any>): ISAMLUser {
 		const userDataMap = this.getUserDataMapping();
 		SAMLUtils.log('parsed userDataMap', userDataMap);
@@ -448,7 +444,7 @@ export class SAMLUtils {
 				idpSession: profile.sessionIndex,
 				nameID: profile.nameID,
 			},
-			emailList: this.ensureArray<string>(email),
+			emailList: ensureArray<string>(email),
 			fullName: name || profile.displayName || profile.username,
 			eppn: profile.eppn,
 			attributeList,

ee/server/configuration/saml.ts

@@ -4,6 +4,7 @@ import { SAMLUtils } from '../../../app/meteor-accounts-saml/server/lib/Utils';
 import { settings } from '../../../app/settings/server';
 import { addSettings } from '../settings/saml';
 import { Users } from '../../../app/models/server';
+import { ensureArray } from '../../../lib/utils/arrayUtils';
 
 onLicense('saml-enterprise', () => {
 	SAMLUtils.events.on('mapUser', ({ profile, userObject }: { profile: Record<string, any>; userObject: ISAMLUser }) => {
@@ -20,7 +21,7 @@ onLicense('saml-enterprise', () => {
 				value = value.split(',');
 			}
 
-			userObject.roles = SAMLUtils.ensureArray<string>(value);
+			userObject.roles = ensureArray<string>(value);
 		}
 	});
 

ee/server/lib/ldap/Manager.ts

@@ -15,8 +15,8 @@ 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 { api } from '../../../../server/sdk/api';
 import { addUserToRoom, removeUserFromRoom, createRoom } from '../../../../app/lib/server/functions';
+import { syncUserRoles } from '../syncUserRoles';
 import { Team } from '../../../../server/sdk';
 
 export class LDAPEEManager extends LDAPManager {
@@ -178,36 +178,20 @@ export class LDAPEEManager extends LDAPManager {
 		}
 	}
 
-	private static broadcastRoleChange(type: string, _id: string, uid: string, username: string): void {
-		// #ToDo: would be better to broadcast this only once for all users and roles, or at least once by user.
-		if (!settings.get('UI_DisplayRoles')) {
-			return;
-		}
-
-		api.broadcast('user.roleUpdate', {
-			type,
-			_id,
-			u: {
-				_id: uid,
-				username,
-			},
-		});
-	}
-
 	private static async syncUserRoles(ldap: LDAPConnection, user: IUser, dn: string): Promise<void> {
 		const { username } = user;
 		if (!username) {
 			logger.debug('User has no username');
 			return;
 		}
 
-		const syncUserRoles = settings.get<boolean>('LDAP_Sync_User_Data_Roles') ?? false;
+		const shouldSyncUserRoles = settings.get<boolean>('LDAP_Sync_User_Data_Roles') ?? false;
 		const syncUserRolesAutoRemove = settings.get<boolean>('LDAP_Sync_User_Data_Roles_AutoRemove') ?? false;
 		const syncUserRolesFieldMap = (settings.get<string>('LDAP_Sync_User_Data_RolesMap') ?? '').trim();
 		const syncUserRolesFilter = (settings.get<string>('LDAP_Sync_User_Data_Roles_Filter') ?? '').trim();
 		const syncUserRolesBaseDN = (settings.get<string>('LDAP_Sync_User_Data_Roles_BaseDN') ?? '').trim();
 
-		if (!syncUserRoles || !syncUserRolesFieldMap) {
+		if (!shouldSyncUserRoles || !syncUserRolesFieldMap) {
 			logger.debug('not syncing user roles');
 			return;
 		}
@@ -232,37 +216,28 @@ export class LDAPEEManager extends LDAPManager {
 		}
 
 		const ldapFields = Object.keys(fieldMap);
+		const roleList: Array<IRole['name']> = [];
+		const allowedRoles: Array<IRole['name']> = [];
+
 		for await (const ldapField of ldapFields) {
 			if (!fieldMap[ldapField]) {
 				continue;
 			}
 
 			const userField = fieldMap[ldapField];
-
 			const [roleName] = userField.split(/\.(.+)/);
-			if (!_.find<IRole>(roles, (el) => el.name === roleName)) {
-				logger.debug(`User Role doesn't exist: ${roleName}`);
-				continue;
-			}
-
-			logger.debug(`User role exists for mapping ${ldapField} -> ${roleName}`);
+			allowedRoles.push(roleName);
 
 			if (await this.isUserInGroup(ldap, syncUserRolesBaseDN, syncUserRolesFilter, { dn, username }, ldapField)) {
-				if (await Roles.addUserRoles(user._id, roleName)) {
-					this.broadcastRoleChange('added', roleName, user._id, username);
-				}
-				logger.debug(`Synced user group ${roleName} from LDAP for ${user.username}`);
-				continue;
-			}
-
-			if (!syncUserRolesAutoRemove) {
+				roleList.push(roleName);
 				continue;
 			}
-
-			if (await Roles.removeUserRoles(user._id, roleName)) {
-				this.broadcastRoleChange('removed', roleName, user._id, username);
-			}
 		}
+
+		await syncUserRoles(user._id, roleList, {
+			allowedRoles,
+			skipRemovingRoles: !syncUserRolesAutoRemove,
+		});
 	}
 
 	private static createRoomForSync(channel: string): IRoom | undefined {

ee/server/lib/oauth/Manager.ts

@@ -1,8 +1,8 @@
-import { addUserRoles, removeUserFromRoles } from '../../../../app/authorization/server';
 import { Rooms } from '../../../../app/models/server';
 import { addUserToRoom, createRoom } from '../../../../app/lib/server/functions';
 import { Logger } from '../../../../app/logger/server';
 import { Roles } from '../../../../app/models/server/raw';
+import { syncUserRoles } from '../syncUserRoles';
 
 export const logger = new Logger('OAuth');
 
@@ -49,15 +49,11 @@ export class OAuthEEManager {
 				user.roles = [];
 			}
 
-			const toRemove = user.roles.filter((val: any) => !rolesFromSSO.includes(val) && rolesToSync.includes(val));
-
-			// remove all roles that the user has, but sso doesnt
-			removeUserFromRoles(user._id, toRemove);
-
-			const toAdd = rolesFromSSO.filter((val: any) => !user.roles.includes(val) && (!rolesToSync.length || rolesToSync.includes(val)));
-
-			// add all roles that sso has, but the user doesnt
-			addUserRoles(user._id, toAdd);
+			Promise.await(
+				syncUserRoles(user._id, rolesFromSSO, {
+					allowedRoles: rolesToSync,
+				}),
+			);
 		}
 	}