From 4736084e00e3756b0c606475f30331d9d6977427 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nuno=20G=C3=B3is?= Date: Tue, 18 Jun 2024 17:06:34 +0100 Subject: [PATCH] fix: check for permission in group access assignment (#7408) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix project role assignment for users with `ADMIN` permission, even if they don't have the Admin root role. This happens when e.g. users inherit the `ADMIN` permission from a group root role, but are not Admins themselves. --------- Co-authored-by: Gastón Fournier --- .../ProjectAccessAssign.tsx | 11 ++-- .../project/project-service.e2e.test.ts | 53 +++++++++++++++++-- src/lib/features/project/project-service.ts | 18 ++++--- 3 files changed, 68 insertions(+), 14 deletions(-) diff --git a/frontend/src/component/project/ProjectAccess/ProjectAccessAssign/ProjectAccessAssign.tsx b/frontend/src/component/project/ProjectAccess/ProjectAccessAssign/ProjectAccessAssign.tsx index ce7f408d3956..2fb5d03b14bb 100644 --- a/frontend/src/component/project/ProjectAccess/ProjectAccessAssign/ProjectAccessAssign.tsx +++ b/frontend/src/component/project/ProjectAccess/ProjectAccessAssign/ProjectAccessAssign.tsx @@ -38,6 +38,8 @@ import { caseInsensitiveSearch } from 'utils/search'; import type { IServiceAccount } from 'interfaces/service-account'; import { MultipleRoleSelect } from 'component/common/MultipleRoleSelect/MultipleRoleSelect'; import type { IUserProjectRole } from '../../../../interfaces/userProjectRoles'; +import { useCheckProjectPermissions } from 'hooks/useHasAccess'; +import { ADMIN } from 'component/providers/AccessProvider/permissions'; const StyledForm = styled('form')(() => ({ display: 'flex', @@ -119,6 +121,8 @@ export const ProjectAccessAssign = ({ useProjectApi(); const edit = Boolean(selected); + const checkPermissions = useCheckProjectPermissions(projectId); + const { setToastData, setToastApiError } = useToast(); const navigate = useNavigate(); @@ -323,11 +327,10 @@ export const ProjectAccessAssign = ({ const isValid = selectedOptions.length > 0 && selectedRoles.length > 0; const displayAllRoles = + checkPermissions(ADMIN) || userRoles.length === 0 || - userRoles.some( - (userRole) => - userRole.name === 'Admin' || userRole.name === 'Owner', - ); + userRoles.some((userRole) => userRole.name === 'Owner'); + let filteredRoles: IRole[]; if (displayAllRoles) { filteredRoles = roles; diff --git a/src/lib/features/project/project-service.e2e.test.ts b/src/lib/features/project/project-service.e2e.test.ts index 88b4ed461b0c..b9e6c704ff35 100644 --- a/src/lib/features/project/project-service.e2e.test.ts +++ b/src/lib/features/project/project-service.e2e.test.ts @@ -440,6 +440,51 @@ describe('Managing Project access', () => { ), ).resolves.not.toThrow(); }); + + test('Admin group members should be allowed to add any project role', async () => { + const viewerUser = await stores.userStore.insert({ + name: 'Some project admin', + email: 'some_project_admin@example.com', + }); + await accessService.setUserRootRole(viewerUser.id, RoleName.VIEWER); + + const adminRole = await stores.roleStore.getRoleByName(RoleName.ADMIN); + const adminGroup = await stores.groupStore.create({ + name: 'admin_group', + rootRole: adminRole.id, + }); + await stores.groupStore.addUsersToGroup( + adminGroup.id, + [{ user: { id: viewerUser.id } }], + opsUser.username!, + ); + + const project = { + id: 'some-project', + name: 'sp', + description: '', + mode: 'open' as const, + defaultStickiness: 'clientId', + }; + await projectService.createProject(project, user, auditUser); + const customRole = await stores.roleStore.create({ + name: 'my_custom_project_role_admin_user', + roleType: 'custom', + description: + 'Used to prove that you can assign a role when you are admin', + }); + + await expect( + projectService.addAccess( + project.id, + [customRole.id], // roles + [], // groups + [opsUser.id], // users + extractAuditInfoFromUser(viewerUser), + ), + ).resolves.not.toThrow(); + }); + test('Users with project owner should be allowed to add any project role', async () => { const project = { id: 'project-owner', @@ -451,11 +496,11 @@ describe('Managing Project access', () => { await projectService.createProject(project, user, auditUser); const projectAdmin = await stores.userStore.insert({ name: 'Some project admin', - email: 'admin@example.com', + email: 'some_other_project_admin@example.com', }); const projectCustomer = await stores.userStore.insert({ name: 'Some project customer', - email: 'customer@example.com', + email: 'some_project_customer@example.com', }); const ownerRole = await stores.roleStore.getRoleByName(RoleName.OWNER); await accessService.addUserToRole( @@ -464,7 +509,7 @@ describe('Managing Project access', () => { project.id, ); const customRole = await stores.roleStore.create({ - name: 'my_custom_role', + name: 'my_custom_project_role', roleType: 'custom', description: 'Used to prove that you can assign a role the project owner does not have', @@ -477,7 +522,7 @@ describe('Managing Project access', () => { [projectCustomer.id], auditUser, ), - ).resolves; + ).resolves.not.toThrow(); }); test('Users with project role should only be allowed to grant same role to others', async () => { const project = { diff --git a/src/lib/features/project/project-service.ts b/src/lib/features/project/project-service.ts index 0a9ce5721b06..cca524d57144 100644 --- a/src/lib/features/project/project-service.ts +++ b/src/lib/features/project/project-service.ts @@ -52,6 +52,7 @@ import { SYSTEM_USER_ID, type ProjectCreated, type IProjectOwnersReadModel, + ADMIN, } from '../../types'; import type { IProjectAccessModel, @@ -838,16 +839,21 @@ export default class ProjectService { } private async isAllowedToAddAccess( - userAddingAccess: number, + userAddingAccess: IAuditUser, projectId: string, rolesBeingAdded: number[], ): Promise { + const userPermissions = + await this.accessService.getPermissionsForUser(userAddingAccess); + if (userPermissions.some(({ permission }) => permission === ADMIN)) { + return true; + } const userRoles = await this.accessService.getAllProjectRolesForUser( - userAddingAccess, + userAddingAccess.id, projectId, ); if ( - this.isAdmin(userAddingAccess, userRoles) || + this.isAdmin(userAddingAccess.id, userRoles) || this.isProjectOwner(userRoles, projectId) ) { return true; @@ -864,7 +870,7 @@ export default class ProjectService { users: number[], auditUser: IAuditUser, ): Promise { - if (await this.isAllowedToAddAccess(auditUser.id, projectId, roles)) { + if (await this.isAllowedToAddAccess(auditUser, projectId, roles)) { await this.accessService.addAccessToProject( roles, groups, @@ -915,7 +921,7 @@ export default class ProjectService { await this.validateAtLeastOneOwner(projectId, ownerRole); } const isAllowedToAssignRoles = await this.isAllowedToAddAccess( - auditUser.id, + auditUser, projectId, newRoles, ); @@ -966,7 +972,7 @@ export default class ProjectService { await this.validateAtLeastOneOwner(projectId, ownerRole); } const isAllowedToAssignRoles = await this.isAllowedToAddAccess( - auditUser.id, + auditUser, projectId, newRoles, );