diff --git a/.changeset/six-squids-pretend.md b/.changeset/six-squids-pretend.md new file mode 100644 index 00000000000..f150528ffb5 --- /dev/null +++ b/.changeset/six-squids-pretend.md @@ -0,0 +1,5 @@ +--- +'@rocket.chat/meteor': major +--- + +Fixes role assignment precedence in SAML provisioning, ensuring that SAML-specific default roles take priority over global registration roles, and preventing role merging when both are configured. diff --git a/apps/meteor/app/authentication/server/startup/index.js b/apps/meteor/app/authentication/server/startup/index.js index aa6418592fa..12825cde0f6 100644 --- a/apps/meteor/app/authentication/server/startup/index.js +++ b/apps/meteor/app/authentication/server/startup/index.js @@ -294,7 +294,7 @@ Accounts.insertUserDoc = async function (options, user) { delete user.globalRoles; - if (user.services && !user.services.password) { + if (user.services && !user.services.password && !options.skipAuthServiceDefaultRoles) { const defaultAuthServiceRoles = parseCSV(settings.get('Accounts_Registration_AuthenticationServices_Default_Roles') || ''); if (defaultAuthServiceRoles.length > 0) { diff --git a/apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts b/apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts index eb37c9c6fde..c81230feeb3 100644 --- a/apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts +++ b/apps/meteor/app/meteor-accounts-saml/server/lib/SAML.ts @@ -30,14 +30,19 @@ const showErrorMessage = function (res: ServerResponse, err: string): void { }; const convertRoleNamesToIds = async (roleNamesOrIds: string[]): Promise => { - const roles = (await Roles.findInIdsOrNames(roleNamesOrIds).toArray()).map((role) => role._id); + const normalizedRoleNamesOrIds = roleNamesOrIds.map((role) => role.trim()).filter((role) => role.length > 0); + if (!normalizedRoleNamesOrIds.length) { + throw new Error(`No valid role names or ids provided for conversion: ${roleNamesOrIds.join(', ')}`); + } - if (roles.length !== roleNamesOrIds.length) { - SystemLogger.warn(`Failed to convert some role names to ids: ${roleNamesOrIds.join(', ')}`); + const roles = (await Roles.findInIdsOrNames(normalizedRoleNamesOrIds).toArray()).map((role) => role._id); + + if (roles.length !== normalizedRoleNamesOrIds.length) { + SystemLogger.warn(`Failed to convert some role names to ids: ${normalizedRoleNamesOrIds.join(', ')}`); } if (!roles.length) { - throw new Error(`We should have at least one existing role to create the user: ${roleNamesOrIds.join(', ')}`); + throw new Error(`We should have at least one existing role to create the user: ${normalizedRoleNamesOrIds.join(', ')}`); } return roles; @@ -93,14 +98,8 @@ export class SAML { } public static async insertOrUpdateSAMLUser(userObject: ISAMLUser): Promise<{ userId: string; token: string }> { - const { - generateUsername, - immutableProperty, - nameOverwrite, - mailOverwrite, - channelsAttributeUpdate, - defaultUserRole = 'user', - } = SAMLUtils.globalSettings; + const { generateUsername, immutableProperty, nameOverwrite, mailOverwrite, channelsAttributeUpdate, defaultUserRole } = + SAMLUtils.globalSettings; let customIdentifierMatch = false; let customIdentifierAttributeName: string | null = null; @@ -142,9 +141,14 @@ export class SAML { const active = !settings.get('Accounts_ManuallyApproveNewUsers'); if (!user) { - // If we received any role from the mapping, use them - otherwise use the default role for creation. - const roleNamesOrIds = userObject.roles?.length ? userObject.roles : ensureArray(defaultUserRole.split(',')); - const roles = await convertRoleNamesToIds(roleNamesOrIds); + let roleNamesOrIds: string[] = []; + if (userObject.roles && userObject.roles.length > 0) { + roleNamesOrIds = userObject.roles; + } else if (defaultUserRole) { + roleNamesOrIds = ensureArray(defaultUserRole.split(',')); + } + + const roles = roleNamesOrIds.length > 0 ? await convertRoleNamesToIds(roleNamesOrIds) : []; const newUser: Record = { name: fullName, @@ -178,7 +182,11 @@ export class SAML { } } - const userId = await Accounts.insertUserDoc({}, newUser); + // only set skipAuthServiceDefaultRoles if SAML is providing its own roles + // otherwise, leave it as false to fallback to generic auth service default roles + // from Accounts_Registration_AuthenticationServices_Default_Roles + const skipAuthServiceDefaultRoles = roleNamesOrIds.length > 0; + const userId = await Accounts.insertUserDoc({ skipAuthServiceDefaultRoles, skipNewUserRolesSetting: true }, newUser); user = await Users.findOneById(userId); if (user && userObject.channels && channelsAttributeUpdate !== true) { diff --git a/apps/meteor/app/meteor-accounts-saml/server/lib/Utils.ts b/apps/meteor/app/meteor-accounts-saml/server/lib/Utils.ts index f3b875f41a0..8ddb44d45c8 100644 --- a/apps/meteor/app/meteor-accounts-saml/server/lib/Utils.ts +++ b/apps/meteor/app/meteor-accounts-saml/server/lib/Utils.ts @@ -73,6 +73,7 @@ export class SAMLUtils { globalSettings.mailOverwrite = Boolean(samlConfigs.mailOverwrite); globalSettings.channelsAttributeUpdate = Boolean(samlConfigs.channelsAttributeUpdate); globalSettings.includePrivateChannelsInUpdate = Boolean(samlConfigs.includePrivateChannelsInUpdate); + globalSettings.defaultUserRole = samlConfigs.defaultUserRole; if (samlConfigs.immutableProperty && typeof samlConfigs.immutableProperty === 'string') { globalSettings.immutableProperty = samlConfigs.immutableProperty; @@ -82,10 +83,6 @@ export class SAMLUtils { globalSettings.usernameNormalize = samlConfigs.usernameNormalize; } - if (samlConfigs.defaultUserRole && typeof samlConfigs.defaultUserRole === 'string') { - globalSettings.defaultUserRole = samlConfigs.defaultUserRole; - } - if (samlConfigs.userDataFieldMap && typeof samlConfigs.userDataFieldMap === 'string') { globalSettings.userDataFieldMap = samlConfigs.userDataFieldMap; }