Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/project role permission grant (#8084) #8132

Merged
merged 1 commit into from
Sep 11, 2024
Merged

Fix/project role permission grant (#8084) #8132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ lerna-debug
npm-debug
.DS_Store
/dist
.vscode

# Logs
logs
Expand Down
121 changes: 121 additions & 0 deletions src/lib/features/project/can-grant-project-role.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 changes: 21 additions & 0 deletions src/lib/features/project/can-grant-project-role.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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 changes: 54 additions & 34 deletions src/lib/features/project/project-service.e2e.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -772,72 +772,92 @@ 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',
description: '',
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: '[email protected]',
});
const projectAuditUser = extractAuditInfoFromUser(projectUser);
const secondUser = await stores.userStore.insert({
name: 'Some other user',
email: '[email protected]',
});
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: '[email protected]',
});
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(
Expand Down
30 changes: 27 additions & 3 deletions src/lib/features/project/project-service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand Down
22 changes: 17 additions & 5 deletions src/lib/routes/admin-api/user/user.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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;
Expand All @@ -42,6 +46,8 @@ class UserController extends Controller {

private projectService: ProjectService;

private flagResolver: IFlagResolver;

constructor(
config: IUnleashConfig,
{
Expand All @@ -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',
Expand Down Expand Up @@ -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,
Expand Down
Loading
Loading