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 = {