Mirrors/unleash.unleash
1
0
Fork 0
mirror of https://github.com/Unleash/unleash.git synced 2024-12-22 19:07:54 +01:00
Code Issues Projects Releases Wiki Activity

Fix/project role permission grant (#8084)

Browse Source 
## 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 <fredrikstrandoseberg@Fredrik-sin-MacBook-Pro.local>
This commit is contained in:
Fredrik Strand Oseberg 2024-09-10 20:35:45 +02:00 committed by GitHub
parent 1532f481d5
commit e1b7cfd8dd
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
7 changed files with 247 additions and 43 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
.gitignore vendored
View File

@ -3,6 +3,7 @@ lerna-debug
npm-debug
.DS_Store
/dist
.vscode
# Logs
logs

121
src/lib/features/project/can-grant-project-role.test.ts Normal file
View File

@ -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();
});
});

21
src/lib/features/project/can-grant-project-role.ts Normal file
View File

@ -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;
});
});
};

88
src/lib/features/project/project-service.e2e.test.ts
View File

@ -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(

30
src/lib/features/project/project-service.ts
View File

@ -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(

22
src/lib/routes/admin-api/user/user.ts
View File

@ -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<void> {
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,

7
src/lib/types/experimental.ts
View File

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