feat: Adding Project access requires same role (#6270)

In order to prevent users from being able to assign roles/permissions they don't have, this PR adds a check that the user performing the action either is Admin, Project owner or has the same role they are trying to grant/add. This addAccess method is only used from Enterprise, so there will be a separate PR there, updating how we return the roles list for a user, so that our frontend can only present the roles a user is actually allowed to grant. This adds the validation to the backend to ensure that even if the frontend thinks we're allowed to add any role to any user here, the backend can be smart enough to stop it. We should still update frontend as well, so that it doesn't look like we can add roles we won't be allowed to.
Unleash · Feb 20, 2024 · e9d9db1 · e9d9db1
1 parent 4857a73
commit e9d9db1
Show file tree

Hide file tree

Showing 20 changed files with 652 additions and 144 deletions.
diff --git a/frontend/src/component/executiveDashboard/ExecutiveDashboard.tsx b/frontend/src/component/executiveDashboard/ExecutiveDashboard.tsx
@@ -1,9 +1,7 @@
-import { ComponentProps, useEffect, useMemo, useState, VFC } from 'react';
+import { useMemo, useState, VFC } from 'react';
 import {
-    Autocomplete,
     Box,
     styled,
-    TextField,
     Typography,
     useMediaQuery,
     useTheme,

diff --git a/frontend/src/component/project/ProjectAccess/ProjectAccessAssign/ProjectAccessAssign.tsx b/frontend/src/component/project/ProjectAccess/ProjectAccessAssign/ProjectAccessAssign.tsx
@@ -36,6 +36,7 @@ import {
 import { caseInsensitiveSearch } from 'utils/search';
 import { IServiceAccount } from 'interfaces/service-account';
 import { MultipleRoleSelect } from 'component/common/MultipleRoleSelect/MultipleRoleSelect';
+import { IUserProjectRole } from '../../../../interfaces/userProjectRoles';
 
 const StyledForm = styled('form')(() => ({
     display: 'flex',
@@ -95,6 +96,7 @@ interface IProjectAccessAssignProps {
     serviceAccounts: IServiceAccount[];
     groups: IGroup[];
     roles: IRole[];
+    userRoles: IUserProjectRole[];
 }
 
 export const ProjectAccessAssign = ({
@@ -104,6 +106,7 @@ export const ProjectAccessAssign = ({
     serviceAccounts,
     groups,
     roles,
+    userRoles,
 }: IProjectAccessAssignProps) => {
     const { uiConfig } = useUiConfig();
     const { flags } = uiConfig;
@@ -318,7 +321,13 @@ export const ProjectAccessAssign = ({
     };
 
     const isValid = selectedOptions.length > 0 && selectedRoles.length > 0;
-
+    const filteredRoles = userRoles.some(
+        (userrole) => userrole.name === 'Admin' || userrole.name === 'Owner',
+    )
+        ? roles
+        : roles.filter((role) =>
+              userRoles.some((userrole) => role.id === userrole.id),
+          );
     return (
         <SidebarModal
             open
@@ -441,7 +450,7 @@ export const ProjectAccessAssign = ({
                         <StyledAutocompleteWrapper>
                             <MultipleRoleSelect
                                 data-testid={PA_ROLE_ID}
-                                roles={roles}
+                                roles={filteredRoles}
                                 value={selectedRoles}
                                 setValue={setRoles}
                             />

diff --git a/frontend/src/component/project/ProjectAccess/ProjectAccessCreate/ProjectAccessCreate.tsx b/frontend/src/component/project/ProjectAccess/ProjectAccessCreate/ProjectAccessCreate.tsx
@@ -2,10 +2,12 @@ import { ProjectAccessAssign } from '../ProjectAccessAssign/ProjectAccessAssign'
 import { useRequiredPathParam } from 'hooks/useRequiredPathParam';
 import useProjectAccess from 'hooks/api/getters/useProjectAccess/useProjectAccess';
 import { useAccess } from 'hooks/api/getters/useAccess/useAccess';
+import { useUserProjectRoles } from '../../../../hooks/api/getters/useUserProjectRoles/useUserProjectRoles';
 
 export const ProjectAccessCreate = () => {
     const projectId = useRequiredPathParam('projectId');
 
+    const { roles: userRoles } = useUserProjectRoles(projectId);
     const { access } = useProjectAccess(projectId);
     const { users, serviceAccounts, groups } = useAccess();
 
@@ -20,6 +22,7 @@ export const ProjectAccessCreate = () => {
             serviceAccounts={serviceAccounts}
             groups={groups}
             roles={access.roles}
+            userRoles={userRoles}
         />
     );
 };
diff --git a/...end/src/component/project/ProjectAccess/ProjectAccessEditGroup/ProjectAccessEditGroup.tsx b/...end/src/component/project/ProjectAccess/ProjectAccessEditGroup/ProjectAccessEditGroup.tsx
@@ -4,10 +4,12 @@ import useProjectAccess, {
     ENTITY_TYPE,
 } from 'hooks/api/getters/useProjectAccess/useProjectAccess';
 import { useAccess } from 'hooks/api/getters/useAccess/useAccess';
+import { useUserProjectRoles } from '../../../../hooks/api/getters/useUserProjectRoles/useUserProjectRoles';
 
 export const ProjectAccessEditGroup = () => {
     const projectId = useRequiredPathParam('projectId');
     const groupId = useRequiredPathParam('groupId');
+    const { roles: userRoles } = useUserProjectRoles(projectId);
 
     const { access } = useProjectAccess(projectId);
     const { users, serviceAccounts, groups } = useAccess();
@@ -29,6 +31,7 @@ export const ProjectAccessEditGroup = () => {
             serviceAccounts={serviceAccounts}
             groups={groups}
             roles={access.roles}
+            userRoles={userRoles}
         />
     );
 };
diff --git a/frontend/src/component/project/ProjectAccess/ProjectAccessEditUser/ProjectAccessEditUser.tsx b/frontend/src/component/project/ProjectAccess/ProjectAccessEditUser/ProjectAccessEditUser.tsx
@@ -4,11 +4,12 @@ import useProjectAccess, {
     ENTITY_TYPE,
 } from 'hooks/api/getters/useProjectAccess/useProjectAccess';
 import { useAccess } from 'hooks/api/getters/useAccess/useAccess';
+import { useUserProjectRoles } from '../../../../hooks/api/getters/useUserProjectRoles/useUserProjectRoles';
 
 export const ProjectAccessEditUser = () => {
     const projectId = useRequiredPathParam('projectId');
     const userId = useRequiredPathParam('userId');
-
+    const { roles: userRoles } = useUserProjectRoles(projectId);
     const { access } = useProjectAccess(projectId);
     const { users, serviceAccounts, groups } = useAccess();
 
@@ -29,6 +30,7 @@ export const ProjectAccessEditUser = () => {
             serviceAccounts={serviceAccounts}
             groups={groups}
             roles={access.roles}
+            userRoles={userRoles}
         />
     );
 };
diff --git a/frontend/src/hooks/api/getters/useUserProjectRoles/getUserProjectRolesFetcher.ts b/frontend/src/hooks/api/getters/useUserProjectRoles/getUserProjectRolesFetcher.ts
@@ -0,0 +1,20 @@
+import { formatApiPath } from '../../../../utils/formatPath';
+import handleErrorResponses from '../httpErrorResponseHandler';
+
+export const getUserProjectRolesFetcher = (id: string) => {
+    const fetcher = () => {
+        const path = formatApiPath(`api/admin/user/roles?projectId=${id}`);
+        return fetch(path, {
+            method: 'GET',
+        })
+            .then(handleErrorResponses('User Project roles'))
+            .then((res) => res.json());
+    };
+
+    const KEY = `api/admin/projects/${id}/roles`;
+
+    return {
+        fetcher,
+        KEY,
+    };
+};
diff --git a/frontend/src/hooks/api/getters/useUserProjectRoles/useUserProjectRoles.ts b/frontend/src/hooks/api/getters/useUserProjectRoles/useUserProjectRoles.ts
@@ -0,0 +1,26 @@
+import { getUserProjectRolesFetcher } from './getUserProjectRolesFetcher';
+import useSWR, { SWRConfiguration } from 'swr';
+import { useCallback } from 'react';
+import { IUserProjectRoles } from '../../../../interfaces/userProjectRoles';
+
+export const useUserProjectRoles = (
+    projectId: string,
+    options: SWRConfiguration = {},
+) => {
+    const { KEY, fetcher } = getUserProjectRolesFetcher(projectId);
+    const { data, error, mutate } = useSWR<IUserProjectRoles>(
+        KEY,
+        fetcher,
+        options,
+    );
+
+    const refetch = useCallback(() => {
+        mutate();
+    }, [mutate]);
+    return {
+        roles: data?.roles || [],
+        loading: !error && !data,
+        error,
+        refetch,
+    };
+};
diff --git a/frontend/src/interfaces/userProjectRoles.ts b/frontend/src/interfaces/userProjectRoles.ts
@@ -0,0 +1,12 @@
+export interface IUserProjectRole {
+    id: number;
+    name: string;
+    type: string;
+    project?: string;
+    description?: string;
+}
+
+export interface IUserProjectRoles {
+    version: number;
+    roles: IUserProjectRole[];
+}
diff --git a/src/lib/db/access-store.ts b/src/lib/db/access-store.ts
@@ -403,6 +403,42 @@ export class AccessStore implements IAccessStore {
             .where('ru.user_id', '=', userId);
     }
 
+    async getAllProjectRolesForUser(
+        userId: number,
+        project: string,
+    ): Promise<IRoleWithProject[]> {
+        const stopTimer = this.timer('get_all_project_roles_for_user');
+        const roles = await this.db
+            .select(['id', 'name', 'type', 'project', 'description'])
+            .from<IRole[]>(T.ROLES)
+            .innerJoin(`${T.ROLE_USER} as ru`, 'ru.role_id', 'id')
+            .where('ru.user_id', '=', userId)
+            .andWhere((builder) => {
+                builder
+                    .where('ru.project', '=', project)
+                    .orWhere('type', '=', 'root');
+            })
+            .union([
+                this.db
+                    .select(['id', 'name', 'type', 'project', 'description'])
+                    .from<IRole[]>(T.ROLES)
+                    .innerJoin(`${T.GROUP_ROLE} as gr`, 'gr.role_id', 'id')
+                    .innerJoin(
+                        `${T.GROUP_USER} as gu`,
+                        'gu.group_id',
+                        'gr.group_id',
+                    )
+                    .where('gu.user_id', '=', userId)
+                    .andWhere((builder) => {
+                        builder
+                            .where('gr.project', '=', project)
+                            .orWhere('type', '=', 'root');
+                    }),
+            ]);
+        stopTimer();
+        return roles;
+    }
+
     async getRootRoleForUser(userId: number): Promise<IRole | undefined> {
         return this.db
             .select(['id', 'name', 'type', 'description'])

diff --git a/src/lib/features/metrics/instance/metrics.test.ts b/src/lib/features/metrics/instance/metrics.test.ts
@@ -271,7 +271,6 @@ test('should return 204 if metrics are disabled by feature flag', async () => {
 
 describe('bulk metrics', () => {
     test('filters out metrics for environments we do not have access for. No auth setup so we can only access default env', async () => {
-        const timer = new Date().valueOf();
         await request
             .post('/api/client/metrics/bulk')
             .send({
@@ -298,29 +297,17 @@ describe('bulk metrics', () => {
                 ],
             })
             .expect(202);
-        console.log(
-            `Posting happened ${new Date().valueOf() - timer} ms after`,
-        );
         await services.clientMetricsServiceV2.bulkAdd(); // Force bulk collection.
-        console.log(
-            `Bulk add happened ${new Date().valueOf() - timer} ms after`,
-        );
         const developmentReport =
             await services.clientMetricsServiceV2.getClientMetricsForToggle(
                 'test_feature_two',
                 1,
             );
-        console.log(
-            `Getting for toggle two ${new Date().valueOf() - timer} ms after`,
-        );
         const defaultReport =
             await services.clientMetricsServiceV2.getClientMetricsForToggle(
                 'test_feature_one',
                 1,
             );
-        console.log(
-            `Getting for toggle one ${new Date().valueOf() - timer} ms after`,
-        );
         expect(developmentReport).toHaveLength(0);
         expect(defaultReport).toHaveLength(1);
         expect(defaultReport[0].yes).toBe(1000);

diff --git a/src/lib/features/project/project-controller.ts b/src/lib/features/project/project-controller.ts
@@ -16,16 +16,16 @@ import ProjectService from './project-service';
 import VariantsController from '../../routes/admin-api/project/variants';
 import {
     createResponseSchema,
-    ProjectDoraMetricsSchema,
-    projectDoraMetricsSchema,
     DeprecatedProjectOverviewSchema,
     deprecatedProjectOverviewSchema,
-    projectsSchema,
-    ProjectsSchema,
+    ProjectDoraMetricsSchema,
+    projectDoraMetricsSchema,
     projectOverviewSchema,
+    ProjectsSchema,
+    projectsSchema,
 } from '../../openapi';
 import { getStandardResponses } from '../../openapi/util/standard-responses';
-import { OpenApiService, SettingService } from '../../services';
+import { AccessService, OpenApiService, SettingService } from '../../services';
 import { IAuthRequest } from '../../routes/unleash-types';
 import { ProjectApiTokenController } from '../../routes/admin-api/project/api-token';
 import ProjectArchiveController from '../../routes/admin-api/project/project-archive';
@@ -45,6 +45,8 @@ export default class ProjectController extends Controller {
 
     private settingService: SettingService;
 
+    private accessService: AccessService;
+
     private openApiService: OpenApiService;
 
     private flagResolver: IFlagResolver;
@@ -54,6 +56,7 @@ export default class ProjectController extends Controller {
         this.projectService = services.projectService;
         this.openApiService = services.openApiService;
         this.settingService = services.settingService;
+        this.accessService = services.accessService;
         this.flagResolver = config.flagResolver;
 
         this.route({
@@ -62,7 +65,7 @@ export default class ProjectController extends Controller {
             handler: this.getProjects,
             permission: NONE,
             middleware: [
-                services.openApiService.validPath({
+                this.openApiService.validPath({
                     tags: ['Projects'],
                     operationId: 'getProjects',
                     summary: 'Get a list of all projects.',
@@ -82,7 +85,7 @@ export default class ProjectController extends Controller {
             handler: this.getDeprecatedProjectOverview,
             permission: NONE,
             middleware: [
-                services.openApiService.validPath({
+                this.openApiService.validPath({
                     tags: ['Projects'],
                     operationId: 'getDeprecatedProjectOverview',
                     summary: 'Get an overview of a project. (deprecated)',
@@ -105,7 +108,7 @@ export default class ProjectController extends Controller {
             handler: this.getProjectOverview,
             permission: NONE,
             middleware: [
-                services.openApiService.validPath({
+                this.openApiService.validPath({
                     tags: ['Projects'],
                     operationId: 'getProjectOverview',
                     summary: 'Get an overview of a project.',
@@ -125,7 +128,7 @@ export default class ProjectController extends Controller {
             handler: this.getProjectDora,
             permission: NONE,
             middleware: [
-                services.openApiService.validPath({
+                this.openApiService.validPath({
                     tags: ['Projects'],
                     operationId: 'getProjectDora',
                     summary: 'Get an overview project dora metrics.',
@@ -145,7 +148,7 @@ export default class ProjectController extends Controller {
             handler: this.getProjectApplications,
             permission: NONE,
             middleware: [
-                services.openApiService.validPath({
+                this.openApiService.validPath({
                     tags: ['Unstable'],
                     operationId: 'getProjectApplications',
                     summary: 'Get a list of all applications for a project.',