From d9e3ff9cd439ba200a766fd0422f03fbaa7a497f Mon Sep 17 00:00:00 2001 From: Mateusz Kwasniewski Date: Wed, 15 Mar 2023 14:44:08 +0100 Subject: [PATCH] refactor: move project membership check from access to project (#3322) --- src/lib/services/access-service.ts | 28 +------------------ src/lib/services/project-service.ts | 28 +++++++++++++++++++ .../e2e/services/access-service.e2e.test.ts | 21 +------------- .../e2e/services/project-service.e2e.test.ts | 28 +++++++++++++++++++ 4 files changed, 58 insertions(+), 47 deletions(-) diff --git a/src/lib/services/access-service.ts b/src/lib/services/access-service.ts index d4f5dc815f..0a9da5c356 100644 --- a/src/lib/services/access-service.ts +++ b/src/lib/services/access-service.ts @@ -25,13 +25,12 @@ import NameExistsError from '../error/name-exists-error'; import { IEnvironmentStore } from 'lib/types/stores/environment-store'; import RoleInUseError from '../error/role-in-use-error'; import { roleSchema } from '../schema/role-schema'; -import { CUSTOM_ROLE_TYPE, ALL_PROJECTS, ALL_ENVS } from '../util/constants'; +import { ALL_ENVS, ALL_PROJECTS, CUSTOM_ROLE_TYPE } from '../util/constants'; import { DEFAULT_PROJECT } from '../types/project'; import InvalidOperationError from '../error/invalid-operation-error'; import BadDataError from '../error/bad-data-error'; import { IGroupModelWithProjectRole } from '../types/group'; import { GroupService } from './group-service'; -import { uniqueByKey } from '../util/unique'; const { ADMIN } = permissions; @@ -410,31 +409,6 @@ export class AccessService { return [roles, users.flat(), groups]; } - async getProjectMembers( - projectId: string, - ): Promise>> { - const [, users, groups] = await this.getProjectRoleAccess(projectId); - const actualUsers = users.map((user) => ({ - id: user.id, - email: user.email, - username: user.username, - })); - const actualGroupUsers = groups - .flatMap((group) => group.users) - .map((user) => user.user) - .map((user) => ({ - id: user.id, - email: user.email, - username: user.username, - })); - return uniqueByKey([...actualUsers, ...actualGroupUsers], 'id'); - } - - async isProjectMember(userId: number, projectId: string): Promise { - const users = await this.getProjectMembers(projectId); - return Boolean(users.find((user) => user.id === userId)); - } - async createDefaultProjectRoles( owner: IUser, projectId: string, diff --git a/src/lib/services/project-service.ts b/src/lib/services/project-service.ts index a6a1aeeaf9..7596d4be22 100644 --- a/src/lib/services/project-service.ts +++ b/src/lib/services/project-service.ts @@ -50,6 +50,7 @@ import { IGroupModelWithProjectRole, IGroupRole } from 'lib/types/group'; import { FavoritesService } from './favorites-service'; import { TimeToProduction } from '../read-models/time-to-production/time-to-production'; import { IProjectStatsStore } from 'lib/types/stores/project-stats-store-type'; +import { uniqueByKey } from '../util/unique'; const getCreatedBy = (user: IUser) => user.email || user.username || 'unknown'; @@ -623,6 +624,33 @@ export default class ProjectService { return this.store.getMembersCountByProject(projectId); } + async getProjectUsers( + projectId: string, + ): Promise>> { + const [, users, groups] = await this.accessService.getProjectRoleAccess( + projectId, + ); + const actualUsers = users.map((user) => ({ + id: user.id, + email: user.email, + username: user.username, + })); + const actualGroupUsers = groups + .flatMap((group) => group.users) + .map((user) => user.user) + .map((user) => ({ + id: user.id, + email: user.email, + username: user.username, + })); + return uniqueByKey([...actualUsers, ...actualGroupUsers], 'id'); + } + + async isProjectUser(userId: number, projectId: string): Promise { + const users = await this.getProjectUsers(projectId); + return Boolean(users.find((user) => user.id === userId)); + } + async getProjectsByUser(userId: number): Promise { return this.store.getProjectsByUser(userId); } diff --git a/src/test/e2e/services/access-service.e2e.test.ts b/src/test/e2e/services/access-service.e2e.test.ts index 88beb8c868..85e1184df0 100644 --- a/src/test/e2e/services/access-service.e2e.test.ts +++ b/src/test/e2e/services/access-service.e2e.test.ts @@ -6,7 +6,7 @@ import { AccessService } from '../../../lib/services/access-service'; import * as permissions from '../../../lib/types/permissions'; import { RoleName } from '../../../lib/types/model'; -import { IUnleashStores, IUser } from '../../../lib/types'; +import { IUnleashStores } from '../../../lib/types'; import FeatureToggleService from '../../../lib/services/feature-toggle-service'; import ProjectService from '../../../lib/services/project-service'; import { createTestConfig } from '../../config/test-config'; @@ -43,16 +43,6 @@ const createUserViewerAccess = async (name, email) => { return user; }; -const isProjectMember = async ( - user: Pick, - projectName: string, - condition: boolean, -) => { - expect(await accessService.isProjectMember(user.id, projectName)).toBe( - condition, - ); -}; - const hasCommonProjectAccess = async (user, projectName, condition) => { const defaultEnv = 'default'; const developmentEnv = 'development'; @@ -415,13 +405,6 @@ test('should grant user access to project', async () => { // // Should be able to update feature toggles inside the project await hasCommonProjectAccess(sUser, project, true); - await isProjectMember(sUser, project, true); - await isProjectMember(user, project, true); - // should list project members - expect(await accessService.getProjectMembers(project)).toStrictEqual([ - { email: user.email, id: user.id, username: user.username }, - { email: sUser.email, id: sUser.id, username: sUser.username }, - ]); // Should not be able to admin the project itself. expect( @@ -912,7 +895,6 @@ test('Should be allowed move feature toggle to project when given access through const projectRole = await accessService.getRoleByName(RoleName.MEMBER); await hasCommonProjectAccess(viewerUser, project.id, false); - await isProjectMember(viewerUser, project.id, false); await accessService.addGroupToRole( groupWithProjectAccess.id!, @@ -922,7 +904,6 @@ test('Should be allowed move feature toggle to project when given access through ); await hasCommonProjectAccess(viewerUser, project.id, true); - await isProjectMember(viewerUser, project.id, true); }); test('Should not lose user role access when given permissions from a group', async () => { diff --git a/src/test/e2e/services/project-service.e2e.test.ts b/src/test/e2e/services/project-service.e2e.test.ts index 94dbbed19e..eb5cb04d0a 100644 --- a/src/test/e2e/services/project-service.e2e.test.ts +++ b/src/test/e2e/services/project-service.e2e.test.ts @@ -26,6 +26,16 @@ let featureToggleService: FeatureToggleService; let favoritesService: FavoritesService; let user; +const isProjectUser = async ( + userId: number, + projectName: string, + condition: boolean, +) => { + expect(await projectService.isProjectUser(userId, projectName)).toBe( + condition, + ); +}; + beforeAll(async () => { db = await dbInit('project_service_serial', getLogger); stores = db.stores; @@ -243,6 +253,8 @@ test('should get list of users with access to project', async () => { expect(users[0].name).toBe(user.name); expect(users[0].roleId).toBe(owner.id); expect(member).toBeTruthy(); + + await isProjectUser(users[0].id, project.id, true); }); test('should add a member user to the project', async () => { @@ -285,6 +297,19 @@ test('should add a member user to the project', async () => { expect(memberUsers[0].name).toBe(projectMember1.name); expect(memberUsers[1].id).toBe(projectMember2.id); expect(memberUsers[1].name).toBe(projectMember2.name); + expect(await projectService.getProjectUsers(project.id)).toStrictEqual([ + { email: user.email, id: user.id, username: user.username }, + { + email: projectMember1.email, + id: projectMember1.id, + username: projectMember1.username, + }, + { + email: projectMember2.email, + id: projectMember2.id, + username: projectMember2.username, + }, + ]); }); test('should add admin users to the project', async () => { @@ -328,6 +353,9 @@ test('should add admin users to the project', async () => { expect(adminUsers[1].name).toBe(projectAdmin1.name); expect(adminUsers[2].id).toBe(projectAdmin2.id); expect(adminUsers[2].name).toBe(projectAdmin2.name); + await isProjectUser(adminUsers[0].id, project.id, true); + await isProjectUser(adminUsers[1].id, project.id, true); + await isProjectUser(adminUsers[2].id, project.id, true); }); test('add user should fail if user already have access', async () => {