Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIX][ENTERPRISE] OAuth "Merge Roles" removes roles from users #23588

Merged
merged 10 commits into from
Nov 22, 2021
4 changes: 2 additions & 2 deletions app/custom-oauth/server/custom_oauth_server.js
Original file line number Diff line number Diff line change
Expand Up @@ -334,6 +334,8 @@ export class CustomOAuth {
return;
}

callbacks.run('afterProcessOAuthUser', { serviceName, serviceData, user });

// User already created or merged and has identical name as before
if (user.services && user.services[serviceName] && user.services[serviceName].id === serviceData.id && user.name === serviceData.name) {
return;
Expand All @@ -343,8 +345,6 @@ export class CustomOAuth {
throw new Meteor.Error('CustomOAuth', `User with username ${ user.username } already exists`);
}

callbacks.run('afterProcessOAuthUser', { serviceName, serviceData, user });

const serviceIdKey = `services.${ serviceName }.id`;
const update = {
$set: {
Expand Down
12 changes: 12 additions & 0 deletions app/lib/server/functions/addOAuthService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,18 @@ export function addOAuthService(name: string, values: { [k: string]: string | bo
enterprise: true,
invalidValue: false,
modules: ['oauth-enterprise'] });
settingsRegistry.add(`Accounts_OAuth_Custom-${ name }-roles_to_sync` , values.rolesToSync || '', { type: 'string',
group: 'OAuth',
section: `Custom OAuth: ${ name }`,
i18nLabel: 'Accounts_OAuth_Custom_Roles_To_Sync',
i18nDescription: 'Accounts_OAuth_Custom_Roles_To_Sync_Description',
enterprise: true,
enableQuery: {
_id: `Accounts_OAuth_Custom-${ name }-merge_roles`,
value: true,
},
invalidValue: '',
modules: ['oauth-enterprise'] });
settingsRegistry.add(`Accounts_OAuth_Custom-${ name }-merge_users`, values.mergeUsers || false, { type: 'boolean', group: 'OAuth', section: `Custom OAuth: ${ name }`, i18nLabel: 'Accounts_OAuth_Custom_Merge_Users', persistent: true });
settingsRegistry.add(`Accounts_OAuth_Custom-${ name }-show_button` , values.showButton || true , { type: 'boolean', group: 'OAuth', section: `Custom OAuth: ${ name }`, i18nLabel: 'Accounts_OAuth_Custom_Show_Button_On_Login_Page', persistent: true });
settingsRegistry.add(`Accounts_OAuth_Custom-${ name }-groups_channel_map` , values.channelsMap || '{\n\t"rocket-admin": "admin",\n\t"tech-support": "support"\n}' , { type: 'code' , multiline: true, code: 'application/json', group: 'OAuth', section: `Custom OAuth: ${ name }`, i18nLabel: 'Accounts_OAuth_Custom_Channel_Map', persistent: true });
Expand Down
1 change: 1 addition & 0 deletions app/lib/server/methods/removeOAuthService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ Meteor.methods({
Settings.removeById(`Accounts_OAuth_Custom-${ name }-avatar_field`),
Settings.removeById(`Accounts_OAuth_Custom-${ name }-roles_claim`),
Settings.removeById(`Accounts_OAuth_Custom-${ name }-merge_roles`),
Settings.removeById(`Accounts_OAuth_Custom-${ name }-roles_to_sync`),
Settings.removeById(`Accounts_OAuth_Custom-${ name }-merge_users`),
Settings.removeById(`Accounts_OAuth_Custom-${ name }-show_button`),
Settings.removeById(`Accounts_OAuth_Custom-${ name }-groups_claim`),
Expand Down
3 changes: 3 additions & 0 deletions app/lib/server/startup/oAuthServicesUpdate.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ function _OAuthServicesUpdate() {
data.mergeUsers = settings.get(`${ key }-merge_users`);
data.mapChannels = settings.get(`${ key }-map_channels`);
data.mergeRoles = settings.get(`${ key }-merge_roles`);
data.rolesToSync = settings.get(`${ key }-roles_to_sync`);
data.showButton = settings.get(`${ key }-show_button`);

new CustomOAuth(serviceName.toLowerCase(), {
Expand All @@ -78,6 +79,7 @@ function _OAuthServicesUpdate() {
channelsAdmin: data.channelsAdmin,
mergeUsers: data.mergeUsers,
mergeRoles: data.mergeRoles,
rolesToSync: data.rolesToSync,
accessTokenParam: data.accessTokenParam,
showButton: data.showButton,
});
Expand Down Expand Up @@ -184,6 +186,7 @@ function customOAuthServicesInit() {
mergeUsers: process.env[`${ serviceKey }_merge_users`] === 'true',
mapChannels: process.env[`${ serviceKey }_map_channels`],
mergeRoles: process.env[`${ serviceKey }_merge_roles`] === 'true',
rolesToSync: process.env[`${ serviceKey }_roles_to_sync`],
showButton: process.env[`${ serviceKey }_show_button`] === 'true',
avatarField: process.env[`${ serviceKey }_avatar_field`],
};
Expand Down
2 changes: 1 addition & 1 deletion app/statistics/server/lib/getServicesStatistics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ function getCustomOAuthServices(): Record<string, {
const name = key.replace('Accounts_OAuth_Custom-', '');
return [name, {
enabled: Boolean(value),
mergeRoles: Boolean(settings.get(`Accounts_OAuth_Custom-${ name }-merge_roles`)),
mergeRoles: settings.get<boolean>(`Accounts_OAuth_Custom-${ name }-merge_roles`),
users: Users.countActiveUsersByService(name),
}];
}));
Expand Down
4 changes: 3 additions & 1 deletion ee/server/configuration/oauth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ interface IOAuthUserIdentity {
interface IOAuthSettings {
mapChannels: string;
mergeRoles: string;
rolesToSync: string;
rolesClaim: string;
groupsClaim: string;
channelsAdmin: string;
Expand All @@ -33,6 +34,7 @@ function getOAuthSettings(serviceName: string): IOAuthSettings {
return {
mapChannels: settings.get(`Accounts_OAuth_Custom-${ serviceName }-map_channels`) as string,
mergeRoles: settings.get(`Accounts_OAuth_Custom-${ serviceName }-merge_roles`) as string,
rolesToSync: settings.get(`Accounts_OAuth_Custom-${ serviceName }-roles_to_sync`) as string,
rolesClaim: settings.get(`Accounts_OAuth_Custom-${ serviceName }-roles_claim`) as string,
groupsClaim: settings.get(`Accounts_OAuth_Custom-${ serviceName }-groups_claim`) as string,
channelsAdmin: settings.get(`Accounts_OAuth_Custom-${ serviceName }-channels_admin`) as string,
Expand Down Expand Up @@ -61,7 +63,7 @@ onLicense('oauth-enterprise', () => {
}

if (settings.mergeRoles) {
OAuthEEManager.updateRolesFromSSO(auth.user, auth.serviceData, settings.rolesClaim);
OAuthEEManager.updateRolesFromSSO(auth.user, auth.serviceData, settings.rolesClaim, settings.rolesToSync.split(',').map((role) => role.trim()));
}
});

Expand Down
18 changes: 7 additions & 11 deletions ee/server/lib/oauth/Manager.ts
Original file line number Diff line number Diff line change
Expand Up @@ -35,27 +35,23 @@ export class OAuthEEManager {
}
}

static updateRolesFromSSO(user: Record<string, any>, identity: Record<string, any>, roleClaimName: string): void {
static updateRolesFromSSO(user: Record<string, any>, identity: Record<string, any>, roleClaimName: string, rolesToSync: string[]): void {
if (user && identity && roleClaimName) {
const rolesFromSSO = this.mapRolesFromSSO(identity, roleClaimName);

if (!Array.isArray(user.roles)) {
user.roles = [];
}

const toRemove = user.roles.filter((val: any) => !rolesFromSSO.includes(val));
const toRemove = user.roles.filter((val: any) => !rolesFromSSO.includes(val) && rolesToSync.includes(val));

// loop through roles that user has that sso doesnt have and remove each one
toRemove.forEach(function(role: any) {
removeUserFromRoles(user._id, role);
});
// remove all roles that the user has, but sso doesnt
removeUserFromRoles(user._id, toRemove);

const toAdd = rolesFromSSO.filter((val: any) => !user.roles.includes(val));
const toAdd = rolesFromSSO.filter((val: any) => !user.roles.includes(val) && (!rolesToSync.length || rolesToSync.includes(val)));

// loop through sso roles and add the new ones
toAdd.forEach(function(role: any) {
addUserRoles(user._id, role);
});
// add all roles that sso has, but the user doesnt
addUserRoles(user._id, toAdd);
}
}

Expand Down
2 changes: 2 additions & 0 deletions packages/rocketchat-i18n/i18n/en.i18n.json
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@
"Accounts_OAuth_Custom_Merge_Users": "Merge users",
"Accounts_OAuth_Custom_Name_Field": "Name field",
"Accounts_OAuth_Custom_Roles_Claim": "Roles/Groups field name",
"Accounts_OAuth_Custom_Roles_To_Sync": "Roles to Sync",
"Accounts_OAuth_Custom_Roles_To_Sync_Description": "OAuth Roles to sync on user login and creation (comma-separated).",
"Accounts_OAuth_Custom_Scope": "Scope",
"Accounts_OAuth_Custom_Secret": "Secret",
"Accounts_OAuth_Custom_Show_Button_On_Login_Page": "Show Button on Login Page",
Expand Down
1 change: 1 addition & 0 deletions server/startup/migrations/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,4 +70,5 @@ import './v243';
import './v244';
import './v245';
import './v246';
import './v247';
import './xrun';
27 changes: 27 additions & 0 deletions server/startup/migrations/v247.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import { settings, settingsRegistry } from '../../../app/settings/server';
import { addMigration } from '../../lib/migrations';

addMigration({
version: 247,
up() {
const customOauthServices = settings.getByRegexp(/Accounts_OAuth_Custom-[^-]+$/mi);
const serviceNames = customOauthServices.map(([key]) => key.replace('Accounts_OAuth_Custom-', ''));

serviceNames.forEach((serviceName) => {
settingsRegistry.add(`Accounts_OAuth_Custom-${ serviceName }-roles_to_sync`, '', {
type: 'string',
group: 'OAuth',
section: `Custom OAuth: ${ serviceName }`,
i18nLabel: 'Accounts_OAuth_Custom_Roles_To_Sync',
i18nDescription: 'Accounts_OAuth_Custom_Roles_To_Sync_Description',
enterprise: true,
enableQuery: {
_id: `Accounts_OAuth_Custom-${ serviceName }-merge_roles`,
value: true,
},
invalidValue: '',
modules: ['oauth-enterprise'],
});
});
},
});