diff --git a/src/lib/db/access-store.ts b/src/lib/db/access-store.ts index 205f0b8764..de92160d66 100644 --- a/src/lib/db/access-store.ts +++ b/src/lib/db/access-store.ts @@ -296,6 +296,14 @@ export class AccessStore implements IAccessStore { return rows.map((r) => r.user_id); } + async getGroupIdsForRole(roleId: number): Promise { + const rows = await this.db + .select(['group_id']) + .from(T.GROUP_ROLE) + .where('role_id', roleId); + return rows.map((r) => r.group_id); + } + async addUserToRole( userId: number, roleId: number, diff --git a/src/lib/features/access/createAccessService.ts b/src/lib/features/access/createAccessService.ts index a3c9fca204..0dfd615873 100644 --- a/src/lib/features/access/createAccessService.ts +++ b/src/lib/features/access/createAccessService.ts @@ -30,7 +30,7 @@ export const createAccessService = ( ); return new AccessService( - { accessStore, accountStore, roleStore, environmentStore }, + { accessStore, accountStore, roleStore, environmentStore, groupStore }, { getLogger, flagResolver }, groupService, ); @@ -52,7 +52,7 @@ export const createFakeAccessService = ( ); return new AccessService( - { accessStore, accountStore, roleStore, environmentStore }, + { accessStore, accountStore, roleStore, environmentStore, groupStore }, { getLogger, flagResolver }, groupService, ); diff --git a/src/lib/features/feature-toggle/createFeatureToggleService.ts b/src/lib/features/feature-toggle/createFeatureToggleService.ts index e7af06cd4c..f00bd234bb 100644 --- a/src/lib/features/feature-toggle/createFeatureToggleService.ts +++ b/src/lib/features/feature-toggle/createFeatureToggleService.ts @@ -86,7 +86,7 @@ export const createFeatureToggleService = ( { getLogger }, ); const accessService = new AccessService( - { accessStore, accountStore, roleStore, environmentStore }, + { accessStore, accountStore, roleStore, environmentStore, groupStore }, { getLogger, flagResolver }, groupService, ); @@ -136,7 +136,7 @@ export const createFakeFeatureToggleService = ( { getLogger }, ); const accessService = new AccessService( - { accessStore, accountStore, roleStore, environmentStore }, + { accessStore, accountStore, roleStore, environmentStore, groupStore }, { getLogger, flagResolver }, groupService, ); diff --git a/src/lib/services/access-service.test.ts b/src/lib/services/access-service.test.ts index f78f141a58..b146f2ffe0 100644 --- a/src/lib/services/access-service.test.ts +++ b/src/lib/services/access-service.test.ts @@ -1,9 +1,18 @@ import NameExistsError from '../error/name-exists-error'; import getLogger from '../../test/fixtures/no-logger'; import { createFakeAccessService } from '../features/access/createAccessService'; -import { IRoleValidation } from './access-service'; +import { AccessService, IRoleValidation } from './access-service'; import { createTestConfig } from '../../test/config/test-config'; import { CUSTOM_ROOT_ROLE_TYPE } from '../util/constants'; +import FakeGroupStore from '../../test/fixtures/fake-group-store'; +import { FakeAccountStore } from '../../test/fixtures/fake-account-store'; +import FakeRoleStore from '../../test/fixtures/fake-role-store'; +import FakeEnvironmentStore from '../../test/fixtures/fake-environment-store'; +import AccessStoreMock from '../../test/fixtures/fake-access-store'; +import { GroupService } from '../services/group-service'; +import FakeEventStore from '../../test/fixtures/fake-event-store'; +import { IRole } from 'lib/types/stores/access-store'; +import { IGroup } from 'lib/types'; function getSetup(customRootRoles: boolean = false) { const config = createTestConfig({ @@ -169,3 +178,58 @@ test('user with custom root role should get a user root role', async () => { expect(roles).toHaveLength(1); expect(roles[0].name).toBe('custom-root-role'); }); + +test('throws error when trying to delete a project role in use by group', async () => { + const groupIdResultOverride = async (): Promise => { + return [1]; + }; + const config = createTestConfig({ + getLogger, + experimental: { + flags: { + customRootRoles: false, + }, + }, + }); + + const eventStore = new FakeEventStore(); + const groupStore = new FakeGroupStore(); + groupStore.getAllWithId = async (): Promise => { + return [{ name: 'group' }]; + }; + const accountStore = new FakeAccountStore(); + const roleStore = new FakeRoleStore(); + const environmentStore = new FakeEnvironmentStore(); + const accessStore = new AccessStoreMock(); + accessStore.getGroupIdsForRole = groupIdResultOverride; + accessStore.getUserIdsForRole = async (): Promise => { + return []; + }; + accessStore.get = async (): Promise => { + return { id: 1, type: 'custom', name: 'project role' }; + }; + const groupService = new GroupService( + { groupStore, eventStore, accountStore }, + { getLogger }, + ); + + const accessService = new AccessService( + { + accessStore, + accountStore, + roleStore, + environmentStore, + groupStore, + }, + config, + groupService, + ); + + try { + await accessService.deleteRole(1); + } catch (e) { + expect(e.toString()).toBe( + 'RoleInUseError: Role is in use by users(0) or groups(1). You cannot delete a role that is in use without first removing the role from the users and groups.', + ); + } +}); diff --git a/src/lib/services/access-service.ts b/src/lib/services/access-service.ts index bd2841de4f..4da1582515 100644 --- a/src/lib/services/access-service.ts +++ b/src/lib/services/access-service.ts @@ -10,7 +10,7 @@ import { IUserRole, } from '../types/stores/access-store'; import { Logger } from '../logger'; -import { IAccountStore, IUnleashStores } from '../types/stores'; +import { IAccountStore, IGroupStore, IUnleashStores } from '../types/stores'; import { IAvailablePermissions, ICustomRole, @@ -34,7 +34,7 @@ import { 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 { IGroup, IGroupModelWithProjectRole } from '../types/group'; import { GroupService } from './group-service'; import { IFlagResolver, IUnleashConfig } from 'lib/types'; @@ -80,6 +80,8 @@ export class AccessService { private groupService: GroupService; + private groupStore: IGroupStore; + private environmentStore: IEnvironmentStore; private logger: Logger; @@ -92,9 +94,14 @@ export class AccessService { accountStore, roleStore, environmentStore, + groupStore, }: Pick< IUnleashStores, - 'accessStore' | 'accountStore' | 'roleStore' | 'environmentStore' + | 'accessStore' + | 'accountStore' + | 'roleStore' + | 'environmentStore' + | 'groupStore' >, { getLogger, @@ -109,6 +116,7 @@ export class AccessService { this.environmentStore = environmentStore; this.logger = getLogger('/services/access-service.ts'); this.flagResolver = flagResolver; + this.groupStore = groupStore; } /** @@ -397,6 +405,14 @@ export class AccessService { return []; } + async getGroupsForRole(roleId: number): Promise { + const groupdIdList = await this.store.getGroupIdsForRole(roleId); + if (groupdIdList.length > 0) { + return this.groupStore.getAllWithId(groupdIdList); + } + return []; + } + async getProjectUsersForRole( roleId: number, projectId?: string, @@ -578,10 +594,11 @@ export class AccessService { await this.validateRoleIsNotBuiltIn(id); const roleUsers = await this.getUsersForRole(id); + const roleGroups = await this.getGroupsForRole(id); - if (roleUsers.length > 0) { + if (roleUsers.length > 0 || roleGroups.length > 0) { throw new RoleInUseError( - 'Role is in use by more than one user. You cannot delete a role that is in use without first removing the role from the users.', + `Role is in use by users(${roleUsers.length}) or groups(${roleGroups.length}). You cannot delete a role that is in use without first removing the role from the users and groups.`, ); } diff --git a/src/lib/types/stores/access-store.ts b/src/lib/types/stores/access-store.ts index 14dd24affb..57cd219397 100644 --- a/src/lib/types/stores/access-store.ts +++ b/src/lib/types/stores/access-store.ts @@ -68,6 +68,8 @@ export interface IAccessStore extends Store { getUserIdsForRole(roleId: number, projectId?: string): Promise; + getGroupIdsForRole(roleId: number, projectId?: string): Promise; + wipePermissionsFromRole(role_id: number): Promise; addEnvironmentPermissionsToRole( diff --git a/src/test/e2e/services/access-service.e2e.test.ts b/src/test/e2e/services/access-service.e2e.test.ts index ceb7f59422..07c8547fa2 100644 --- a/src/test/e2e/services/access-service.e2e.test.ts +++ b/src/test/e2e/services/access-service.e2e.test.ts @@ -735,7 +735,7 @@ test('Should be denied access to delete a role that is in use', async () => { await accessService.deleteRole(customRole.id); } catch (e) { expect(e.toString()).toBe( - 'RoleInUseError: Role is in use by more than one user. You cannot delete a role that is in use without first removing the role from the users.', + 'RoleInUseError: Role is in use by users(1) or groups(0). You cannot delete a role that is in use without first removing the role from the users and groups.', ); } }); diff --git a/src/test/fixtures/access-service-mock.ts b/src/test/fixtures/access-service-mock.ts index 2aaa9e6c12..f692c4eec1 100644 --- a/src/test/fixtures/access-service-mock.ts +++ b/src/test/fixtures/access-service-mock.ts @@ -19,6 +19,7 @@ class AccessServiceMock extends AccessService { accountStore: undefined, roleStore: undefined, environmentStore: undefined, + groupStore: undefined, }, { getLogger: noLoggerProvider, flagResolver: undefined }, undefined, diff --git a/src/test/fixtures/fake-access-store.ts b/src/test/fixtures/fake-access-store.ts index c754246280..b01eac2d85 100644 --- a/src/test/fixtures/fake-access-store.ts +++ b/src/test/fixtures/fake-access-store.ts @@ -146,6 +146,10 @@ class AccessStoreMock implements IAccessStore { throw new Error('Method not implemented.'); } + getGroupIdsForRole(roleId: number, projectId?: string): Promise { + throw new Error('Method not implemented.'); + } + addUserToRole(userId: number, roleId: number): Promise { this.userToRoleMap.set(userId, roleId); return Promise.resolve(undefined);