diff --git a/app/api/server/v1/roles.js b/app/api/server/v1/roles.js index c17e43107c6..92895cb9f6a 100644 --- a/app/api/server/v1/roles.js +++ b/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, }); } diff --git a/app/models/server/models/Roles.js b/app/models/server/models/Roles.js index 531ab9462e2..d305c071d10 100644 --- a/app/models/server/models/Roles.js +++ b/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; } diff --git a/app/models/server/raw/Roles.js b/app/models/server/raw/Roles.js index de771bcad9b..7e06551fde5 100644 --- a/app/models/server/raw/Roles.js +++ b/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]; diff --git a/tests/end-to-end/api/13-roles.js b/tests/end-to-end/api/13-roles.js index f1ca6ea0591..162152a0721 100644 --- a/tests/end-to-end/api/13-roles.js +++ b/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)