Regression: roles.removeUserFromRole API not working with scoped roles. (#22799)

app/api/server/v1/roles.js

@@ -3,7 +3,7 @@ import { Match, check } from 'meteor/check';
 
 import { Roles, Users } from '../../../models';
 import { API } from '../api';
-import { getUsersInRole, hasPermission } from '../../../authorization/server';
+import { getUsersInRole, hasPermission, hasRole } from '../../../authorization/server';
 import { settings } from '../../../settings/server/index';
 import { api } from '../../../../server/sdk/api';
 
@@ -202,7 +202,7 @@ API.v1.addRoute('roles.delete', { authRequired: true }, {
 			throw new Meteor.Error('error-role-protected', 'Cannot delete a protected role');
 		}
 
-		const existingUsers = Roles.findUsersInRole(role._id, role.scope);
+		const existingUsers = Roles.findUsersInRole(role.name, role.scope);
 
 		if (existingUsers && existingUsers.count() > 0) {
 			throw new Meteor.Error('error-role-in-use', 'Cannot delete role because it\'s in use');
@@ -219,11 +219,13 @@ API.v1.addRoute('roles.removeUserFromRole', { authRequired: true }, {
 		check(this.bodyParams, {
 			roleName: String,
 			username: String,
+			scope: Match.Maybe(String),
 		});
 
 		const data = {
 			roleName: this.bodyParams.roleName,
 			username: this.bodyParams.username,
+			scope: this.bodyParams.scope,
 		};
 
 		if (!hasPermission(this.userId, 'access-permissions')) {
@@ -242,18 +244,18 @@ API.v1.addRoute('roles.removeUserFromRole', { authRequired: true }, {
 			throw new Meteor.Error('error-invalid-roleId', 'This role does not exist');
 		}
 
-		if (user.roles.indexOf(role.name) === -1) {
+		if (!hasRole(user._id, role.name, data.scope)) {
 			throw new Meteor.Error('error-user-not-in-role', 'User is not in this role');
 		}
 
 		if (role._id === 'admin') {
-			const adminCount = Roles.findUsersInRole('admin', role.scope).count();
+			const adminCount = Roles.findUsersInRole('admin').count();
 			if (adminCount === 1) {
 				throw new Meteor.Error('error-admin-required', 'You need to have at least one admin');
 			}
 		}
 
-		Roles.removeUserRoles(user._id, role.name, role.scope);
+		Roles.removeUserRoles(user._id, role.name, data.scope);
 
 		if (settings.get('UI_DisplayRoles')) {
 			api.broadcast('user.roleUpdate', {
@@ -263,7 +265,7 @@ API.v1.addRoute('roles.removeUserFromRole', { authRequired: true }, {
 					_id: user._id,
 					username: user.username,
 				},
-				scope: role.scope,
+				scope: data.scope,
 			});
 		}
 

app/models/server/models/Roles.js

@@ -10,7 +10,7 @@ export class Roles extends Base {
 	}
 
 	findUsersInRole(name, scope, options) {
-		const role = this.findOne(name);
+		const role = this.findOneByName(name);
 		const roleScope = (role && role.scope) || 'Users';
 		const model = Models[roleScope];
 
@@ -20,7 +20,7 @@ export class Roles extends Base {
 	isUserInRoles(userId, roles, scope) {
 		roles = [].concat(roles);
 		return roles.some((roleName) => {
-			const role = this.findOne(roleName);
+			const role = this.findOneByName(roleName);
 			const roleScope = (role && role.scope) || 'Users';
 			const model = Models[roleScope];
 
@@ -70,7 +70,7 @@ export class Roles extends Base {
 	addUserRoles(userId, roles, scope) {
 		roles = [].concat(roles);
 		for (const roleName of roles) {
-			const role = this.findOne(roleName);
+			const role = this.findOneByName(roleName);
 			const roleScope = (role && role.scope) || 'Users';
 			const model = Models[roleScope];
 
@@ -82,7 +82,7 @@ export class Roles extends Base {
 	removeUserRoles(userId, roles, scope) {
 		roles = [].concat(roles);
 		for (const roleName of roles) {
-			const role = this.findOne(roleName);
+			const role = this.findOneByName(roleName);
 			const roleScope = (role && role.scope) || 'Users';
 			const model = Models[roleScope];
 
@@ -103,6 +103,14 @@ export class Roles extends Base {
 		return this.findOne(query, options);
 	}
 
+	findOneByName(name, options) {
+		const query = {
+			name,
+		};
+
+		return this.findOne(query, options);
+	}
+
 	findByUpdatedDate(updatedAfterDate, options) {
 		const query = {
 			_updatedAt: { $gte: new Date(updatedAfterDate) },
@@ -112,7 +120,7 @@ export class Roles extends Base {
 	}
 
 	canAddUserToRole(uid, roleName, scope) {
-		const role = this.findOne({ _id: roleName }, { fields: { scope: 1 } });
+		const role = this.findOne({ name: roleName }, { fields: { scope: 1 } });
 		if (!role) {
 			return false;
 		}

app/models/server/raw/Roles.js

@@ -16,7 +16,7 @@ export class RolesRaw extends BaseRaw {
 			const roleName = roles[i];
 
 			// eslint-disable-next-line no-await-in-loop
-			const role = await this.findOne({ _id: roleName }, { scope: 1 });
+			const role = await this.findOne({ name: roleName }, { scope: 1 });
 			const roleScope = (role && role.scope) || 'Users';
 			const model = this.models[roleScope];
 

tests/end-to-end/api/13-roles.js

@@ -31,13 +31,14 @@ function createRole(name, scope, description) {
 	});
 }
 
-function addUserToRole(roleName, username) {
+function addUserToRole(roleName, username, scope) {
 	return new Promise((resolve) => {
 		request.post(api('roles.addUserToRole'))
 			.set(credentials)
 			.send({
 				roleName,
 				username,
+				roomId: scope,
 			})
 			.expect('Content-Type', 'application/json')
 			.expect(200)
@@ -337,7 +338,7 @@ describe('[Roles]', function() {
 			roleWithUser = await createRole(`roleWithUser-${ Date.now() }`, 'Users');
 			roleWithoutUser = await createRole(`roleWithoutUser-${ Date.now() }`, 'Users');
 
-			await addUserToRole(roleWithUser._id, login.user);
+			await addUserToRole(roleWithUser.name, login.user);
 		});
 
 		it('should delete a role that it is not being used', (done) => {
@@ -394,7 +395,7 @@ describe('[Roles]', function() {
 			subscriptionsScopedRole = await createRole(`subscriptionsScopedRole-${ Date.now() }`, 'Subscriptions');
 
 			await addUserToRole(usersScopedRole.name, login.user);
-			await addUserToRole(subscriptionsScopedRole.name, login.user);
+			await addUserToRole(subscriptionsScopedRole.name, login.user, 'GENERAL');
 		});
 
 		it('should unassign a role with User scope from an user', (done) => {
@@ -422,6 +423,7 @@ describe('[Roles]', function() {
 				.send({
 					roleName: subscriptionsScopedRole.name,
 					username: login.user,
+					scope: 'GENERAL',
 				})
 				.expect('Content-Type', 'application/json')
 				.expect(200)