From e1b7cfd8dd7b5497675aba4ab121150c656946f7 Mon Sep 17 00:00:00 2001 From: Fredrik Strand Oseberg Date: Tue, 10 Sep 2024 20:35:45 +0200 Subject: [PATCH] Fix/project role permission grant (#8084) ## Background In #6380 we fixed a privilege escalation bug that allowed members of a project that had permission to add users to the project with roles that had a higher permission set than themselves. The PR linked essentially constricts you only be able to assign users to roles that you possess yourself if you are not an Admin or Project owner. This fix broke expectations for another customer who needed to have a project owner without the DELETE_PROJECT permission. The fix above made it so that their custom project owner role only was able to assign users to the project with the role that they posessed. ## Fix Instead of looking directly at which role the role granter has, this PR addresses the issue by making the assessment based on the permission sets of the user and the roles to be granted. If the granter has all the permissions of the role being granted, the granter is permitted to assign the role. ## Other considerations The endpoint to get roles was changed in this PR. It previously only retrieved the roles that the user had in the project. This no-longer makes sense because the user should be able to see other project roles than the one they themselves hold when assigning users to the project. The drawback of returning all project roles is that there may be a project role in the list that the user does not have access to assign, because they do not hold all the permissions required of the role. This was discussed internally and we decided that it's an acceptable trade-off for now because the complexities of returning a role list based on comparing permissions set is not trivial. We would have to retrieve each project role with permissions from the database, and run the same in-memory check against the users permission to determine which roles to return from this endpoint. Instead we opted for returning all project roles and display an error if you try to assign a role that you do not have access to. ## Follow up When this is merged, there's no longer need for the frontend logic that filters out roles in the role assignment form. I deliberately left this out of the scope for this PR because I couldn't wrap my head around everything that was going on there and I thought it was better to pair on this with @chriswk or @nunogois in order to make sure we get this right as the logic for this filtering seemed quite complex and was touching multiple different components. --------- Co-authored-by: Fredrik Strand Oseberg --- .gitignore | 1 + .../project/can-grant-project-role.test.ts | 121 ++++++++++++++++++ .../project/can-grant-project-role.ts | 21 +++ .../project/project-service.e2e.test.ts | 88 ++++++++----- src/lib/features/project/project-service.ts | 30 ++++- src/lib/routes/admin-api/user/user.ts | 22 +++- src/lib/types/experimental.ts | 7 +- 7 files changed, 247 insertions(+), 43 deletions(-) create mode 100644 src/lib/features/project/can-grant-project-role.test.ts create mode 100644 src/lib/features/project/can-grant-project-role.ts diff --git a/.gitignore b/.gitignore index 9f826b0cf8..842fdcf856 100644 --- a/.gitignore +++ b/.gitignore @@ -3,6 +3,7 @@ lerna-debug npm-debug .DS_Store /dist +.vscode # Logs logs diff --git a/src/lib/features/project/can-grant-project-role.test.ts b/src/lib/features/project/can-grant-project-role.test.ts new file mode 100644 index 0000000000..67afa4908a --- /dev/null +++ b/src/lib/features/project/can-grant-project-role.test.ts @@ -0,0 +1,121 @@ +import { canGrantProjectRole } from './can-grant-project-role'; + +describe('canGrantProjectRole', () => { + test('should return true if the granter has all the permissions the receiver needs', () => { + const granterPermissions = [ + { + project: 'test', + environment: undefined, + permission: 'CREATE_FEATURE', + }, + { + project: 'test', + environment: 'production', + permission: 'UPDATE_FEATURE_ENVIRONMENT', + }, + { + project: 'test', + environment: 'production', + permission: 'APPROVE_CHANGE_REQUEST', + }, + ]; + + const receiverPermissions = [ + { + id: 28, + name: 'UPDATE_FEATURE_ENVIRONMENT', + environment: 'production', + displayName: 'Enable/disable flags', + type: 'environment', + }, + { + id: 29, + name: 'APPROVE_CHANGE_REQUEST', + environment: 'production', + displayName: 'Enable/disable flags', + type: 'environment', + }, + ]; + + canGrantProjectRole(granterPermissions, receiverPermissions); + }); + + test('should return false if the granter and receiver permissions have different environments', () => { + const granterPermissions = [ + { + project: 'test', + environment: 'production', + permission: 'UPDATE_FEATURE_ENVIRONMENT', + }, + { + project: 'test', + environment: 'production', + permission: 'APPROVE_CHANGE_REQUEST', + }, + ]; + + const receiverPermissions = [ + { + id: 28, + name: 'UPDATE_FEATURE_ENVIRONMENT', + environment: 'development', + displayName: 'Enable/disable flags', + type: 'environment', + }, + { + id: 29, + name: 'APPROVE_CHANGE_REQUEST', + environment: 'development', + displayName: 'Enable/disable flags', + type: 'environment', + }, + ]; + + expect( + canGrantProjectRole(granterPermissions, receiverPermissions), + ).toBeFalsy(); + }); + + test('should return false if the granter does not have all receiver permissions', () => { + const granterPermissions = [ + { + project: 'test', + environment: 'production', + permission: 'UPDATE_FEATURE_ENVIRONMENT', + }, + { + project: 'test', + environment: 'production', + permission: 'APPROVE_CHANGE_REQUEST', + }, + ]; + + const receiverPermissions = [ + { + id: 28, + name: 'UPDATE_FEATURE_ENVIRONMENT', + environment: 'production', + displayName: 'Enable/disable flags', + type: 'environment', + }, + { + id: 29, + name: 'APPROVE_CHANGE_REQUEST', + environment: 'production', + displayName: 'Enable/disable flags', + type: 'environment', + }, + { + id: 26, + name: 'UPDATE_FEATURE_STRATEGY', + environment: 'production', + displayName: 'Update activation strategies', + type: 'environment', + }, + ]; + + expect( + canGrantProjectRole(granterPermissions, receiverPermissions), + ).toBeFalsy(); + }); +}); diff --git a/src/lib/features/project/can-grant-project-role.ts b/src/lib/features/project/can-grant-project-role.ts new file mode 100644 index 0000000000..faaf8f2dba --- /dev/null +++ b/src/lib/features/project/can-grant-project-role.ts @@ -0,0 +1,21 @@ +import type { IPermission } from '../../types'; +import type { IUserPermission } from '../../types/stores/access-store'; + +export const canGrantProjectRole = ( + granterPermissions: IUserPermission[], + receiverPermissions: IPermission[], +) => { + return receiverPermissions.every((receiverPermission) => { + return granterPermissions.some((granterPermission) => { + if (granterPermission.environment) { + return ( + granterPermission.permission === receiverPermission.name && + granterPermission.environment === + receiverPermission.environment + ); + } + + return granterPermission.permission === receiverPermission.name; + }); + }); +}; diff --git a/src/lib/features/project/project-service.e2e.test.ts b/src/lib/features/project/project-service.e2e.test.ts index 06733b1896..eeebf1f558 100644 --- a/src/lib/features/project/project-service.e2e.test.ts +++ b/src/lib/features/project/project-service.e2e.test.ts @@ -793,7 +793,8 @@ describe('Managing Project access', () => { ), ); }); - test('Users can not assign roles they do not have to a user through explicit roles endpoint', async () => { + + test('Users can not assign roles where they do not hold the same permissions', async () => { const project = { id: 'user_fail_assign_to_user', name: 'user_fail_assign_to_user', @@ -801,64 +802,83 @@ describe('Managing Project access', () => { mode: 'open' as const, defaultStickiness: 'clientId', }; + + const auditUser = extractAuditInfoFromUser(user); await projectService.createProject(project, user, auditUser); const projectUser = await stores.userStore.insert({ name: 'Some project user', email: 'fail_assign_role_to_user@example.com', }); - const projectAuditUser = extractAuditInfoFromUser(projectUser); const secondUser = await stores.userStore.insert({ name: 'Some other user', email: 'otheruser_no_roles@example.com', }); - const customRole = await stores.roleStore.create({ - name: 'role_that_noone_has', - roleType: 'custom', - description: - 'Used to prove that you can not assign a role you do not have via setRolesForUser', - }); + + const customRoleUserAccess = await accessService.createRole( + { + name: 'Project-permissions-lead', + description: 'Role', + permissions: [ + { + name: 'PROJECT_USER_ACCESS_WRITE', + }, + ], + createdByUserId: SYSTEM_USER_ID, + }, + SYSTEM_USER_AUDIT, + ); + + const customRoleUpdateEnvironments = await accessService.createRole( + { + name: 'Project Lead', + description: 'Role', + permissions: [ + { + name: 'UPDATE_FEATURE_ENVIRONMENT', + environment: 'production', + }, + { + name: 'CREATE_FEATURE_STRATEGY', + environment: 'production', + }, + ], + createdByUserId: SYSTEM_USER_ID, + }, + SYSTEM_USER_AUDIT, + ); + + await projectService.setRolesForUser( + project.id, + projectUser.id, + [customRoleUserAccess.id], + auditUser, + ); + + const auditProjectUser = extractAuditInfoFromUser(projectUser); + await expect( projectService.setRolesForUser( project.id, secondUser.id, - [customRole.id], - projectAuditUser, + [customRoleUpdateEnvironments.id], + auditProjectUser, ), ).rejects.toThrow( new InvalidOperationError( 'User tried to assign a role they did not have access to', ), ); - }); - test('Users can not assign roles they do not have to a group through explicit roles endpoint', async () => { - const project = { - id: 'user_fail_assign_to_group', - name: 'user_fail_assign_to_group', - description: '', - mode: 'open' as const, - defaultStickiness: 'clientId', - }; - await projectService.createProject(project, user, auditUser); - const projectUser = await stores.userStore.insert({ - name: 'Some project user', - email: 'fail_assign_role_to_group@example.com', - }); - const projectAuditUser = extractAuditInfoFromUser(projectUser); + const group = await stores.groupStore.create({ name: 'Some group_awaiting_role', }); - const customRole = await stores.roleStore.create({ - name: 'role_that_noone_has_fail_assign_group', - roleType: 'custom', - description: - 'Used to prove that you can not assign a role you do not have via setRolesForGroup', - }); - return expect( + + await expect( projectService.setRolesForGroup( project.id, group.id, - [customRole.id], - projectAuditUser, + [customRoleUpdateEnvironments.id], + auditProjectUser, ), ).rejects.toThrow( new InvalidOperationError( diff --git a/src/lib/features/project/project-service.ts b/src/lib/features/project/project-service.ts index 6e210ada46..4128074635 100644 --- a/src/lib/features/project/project-service.ts +++ b/src/lib/features/project/project-service.ts @@ -89,6 +89,7 @@ import { throwExceedsLimitError } from '../../error/exceeds-limit-error'; import type EventEmitter from 'events'; import type { ApiTokenService } from '../../services/api-token-service'; import type { TransitionalProjectData } from './project-read-model-type'; +import { canGrantProjectRole } from './can-grant-project-role'; type Days = number; type Count = number; @@ -940,15 +941,38 @@ export default class ProjectService { userAddingAccess.id, projectId, ); + if ( this.isAdmin(userAddingAccess.id, userRoles) || this.isProjectOwner(userRoles, projectId) ) { return true; } - return rolesBeingAdded.every((roleId) => - userRoles.some((userRole) => userRole.id === roleId), - ); + + // Users may have access to multiple projects, so we need to filter out the permissions based on this project. + // Since the project roles are just collections of permissions that are not tied to a project in the database + // not filtering here might lead to false positives as they may have the permission in another project. + if (this.flagResolver.isEnabled('projectRoleAssignment')) { + const filteredUserPermissions = userPermissions.filter( + (permission) => permission.project === projectId, + ); + + const rolesToBeAssignedData = await Promise.all( + rolesBeingAdded.map((role) => this.accessService.getRole(role)), + ); + const rolesToBeAssignedPermissions = rolesToBeAssignedData.flatMap( + (role) => role.permissions, + ); + + return canGrantProjectRole( + filteredUserPermissions, + rolesToBeAssignedPermissions, + ); + } else { + return rolesBeingAdded.every((roleId) => + userRoles.some((userRole) => userRole.id === roleId), + ); + } } async addAccess( diff --git a/src/lib/routes/admin-api/user/user.ts b/src/lib/routes/admin-api/user/user.ts index 8fec2cff13..f6c2d1b6d8 100644 --- a/src/lib/routes/admin-api/user/user.ts +++ b/src/lib/routes/admin-api/user/user.ts @@ -13,7 +13,10 @@ import { createRequestSchema } from '../../../openapi/util/create-request-schema import { createResponseSchema } from '../../../openapi/util/create-response-schema'; import { meSchema, type MeSchema } from '../../../openapi/spec/me-schema'; import { serializeDates } from '../../../types/serialize-dates'; -import type { IUserPermission } from '../../../types/stores/access-store'; +import type { + IRole, + IUserPermission, +} from '../../../types/stores/access-store'; import type { PasswordSchema } from '../../../openapi/spec/password-schema'; import { emptyResponse, @@ -28,6 +31,7 @@ import { rolesSchema, type RolesSchema, } from '../../../openapi/spec/roles-schema'; +import type { IFlagResolver } from '../../../types'; class UserController extends Controller { private accessService: AccessService; @@ -42,6 +46,8 @@ class UserController extends Controller { private projectService: ProjectService; + private flagResolver: IFlagResolver; + constructor( config: IUnleashConfig, { @@ -68,6 +74,7 @@ class UserController extends Controller { this.userSplashService = userSplashService; this.openApiService = openApiService; this.projectService = projectService; + this.flagResolver = config.flagResolver; this.route({ method: 'get', @@ -174,10 +181,15 @@ class UserController extends Controller { ): Promise { const { projectId } = req.query; if (projectId) { - const roles = await this.accessService.getAllProjectRolesForUser( - req.user.id, - projectId, - ); + let roles: IRole[]; + if (this.flagResolver.isEnabled('projectRoleAssignment')) { + roles = await this.accessService.getProjectRoles(); + } else { + roles = await this.accessService.getAllProjectRolesForUser( + req.user.id, + projectId, + ); + } this.openApiService.respondWithValidation( 200, res, diff --git a/src/lib/types/experimental.ts b/src/lib/types/experimental.ts index 83b42a03df..09e56e2bc3 100644 --- a/src/lib/types/experimental.ts +++ b/src/lib/types/experimental.ts @@ -62,7 +62,8 @@ export type IFlagKey = | 'useProjectReadModel' | 'addonUsageMetrics' | 'onboardingMetrics' - | 'onboardingUI'; + | 'onboardingUI' + | 'projectRoleAssignment'; export type IFlags = Partial<{ [key in IFlagKey]: boolean | Variant }>; @@ -307,6 +308,10 @@ const flags: IFlags = { process.env.UNLEASH_EXPERIMENTAL_ONBOARDING_UI, false, ), + projectRoleAssignment: parseEnvVarBoolean( + process.env.UNLEASH_EXPERIMENTAL_PROJECT_ROLE_ASSIGNMENT, + false, + ), }; export const defaultExperimentalOptions: IExperimentalOptions = {