From 4cedb00e04a422e653623763a7e76ce2158db12b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= <gaston@getunleash.io>
Date: Thu, 22 Jun 2023 16:42:01 +0200
Subject: [PATCH] fix: fetching user root roles include custom ones (#4068)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
## About the changes
`getUserRootRoles` should also consider custom root roles
This introduces test cases that unveiled a dependency between stores
(this happens actually at the DB layer having access-service access
tables from two different stores but skipping the store layer).
https://linear.app/unleash/issue/2-1161/a-user-with-custom-root-role-and-permission-to-create-client-api
---------
Co-authored-by: Nuno Góis <github@nunogois.com>
---
.../features/access/createAccessService.ts | 2 +-
src/lib/services/access-service.test.ts | 65 ++++++++++++-------
src/lib/services/access-service.ts | 12 ++--
src/lib/types/model.ts | 8 +--
src/test/fixtures/fake-access-store.ts | 35 +++++++---
src/test/fixtures/fake-role-store.ts | 17 +++--
6 files changed, 90 insertions(+), 49 deletions(-)
diff --git a/src/lib/features/access/createAccessService.ts b/src/lib/features/access/createAccessService.ts
index cd7b2dee3b..a3c9fca204 100644
--- a/src/lib/features/access/createAccessService.ts
+++ b/src/lib/features/access/createAccessService.ts
@@ -45,7 +45,7 @@ export const createFakeAccessService = (
const accountStore = new FakeAccountStore();
const roleStore = new FakeRoleStore();
const environmentStore = new FakeEnvironmentStore();
- const accessStore = new FakeAccessStore();
+ const accessStore = new FakeAccessStore(roleStore);
const groupService = new GroupService(
{ groupStore, eventStore, accountStore },
{ getLogger },
diff --git a/src/lib/services/access-service.test.ts b/src/lib/services/access-service.test.ts
index a9e3385d2c..f78f141a58 100644
--- a/src/lib/services/access-service.test.ts
+++ b/src/lib/services/access-service.test.ts
@@ -1,35 +1,33 @@
import NameExistsError from '../error/name-exists-error';
import getLogger from '../../test/fixtures/no-logger';
-import createStores from '../../test/fixtures/store';
-import { AccessService, IRoleValidation } from './access-service';
-import { GroupService } from './group-service';
+import { createFakeAccessService } from '../features/access/createAccessService';
+import { IRoleValidation } from './access-service';
import { createTestConfig } from '../../test/config/test-config';
+import { CUSTOM_ROOT_ROLE_TYPE } from '../util/constants';
-function getSetup(withNameInUse: boolean) {
- const stores = createStores();
-
+function getSetup(customRootRoles: boolean = false) {
const config = createTestConfig({
getLogger,
+ experimental: {
+ flags: {
+ customRootRoles: customRootRoles,
+ },
+ },
});
- stores.roleStore = {
- ...stores.roleStore,
- async nameInUse(): Promise<boolean> {
- return withNameInUse;
- },
- };
return {
- accessService: new AccessService(stores, config, {} as GroupService),
- stores,
+ accessService: createFakeAccessService(config),
};
}
-test('should fail when name exists', async () => {
- const { accessService } = getSetup(true);
- const existingRole: IRoleValidation = {
+test('should fail when name exists', async () => {
+ const { accessService } = getSetup();
+ const existingRole = await accessService.createRole({
name: 'existing role',
description: 'description',
- };
+ permissions: [],
+ });
+
expect(accessService.validateRole(existingRole)).rejects.toThrow(
new NameExistsError(
`There already exists a role with the name ${existingRole.name}`,
@@ -38,7 +36,7 @@ test('should fail when name exists', async () => {
});
test('should validate a role without permissions', async () => {
- const { accessService } = getSetup(false);
+ const { accessService } = getSetup();
const withoutPermissions: IRoleValidation = {
name: 'name of the role',
@@ -50,7 +48,7 @@ test('should validate a role without permissions', async () => {
});
test('should complete description field when not present', async () => {
- const { accessService } = getSetup(false);
+ const { accessService } = getSetup();
const withoutDescription: IRoleValidation = {
name: 'name of the role',
};
@@ -61,7 +59,7 @@ test('should complete description field when not present', async () => {
});
test('should accept empty permissions', async () => {
- const { accessService } = getSetup(false);
+ const { accessService } = getSetup();
const withEmptyPermissions: IRoleValidation = {
name: 'name of the role',
description: 'description',
@@ -75,7 +73,7 @@ test('should accept empty permissions', async () => {
});
test('should complete environment field of permissions when not present', async () => {
- const { accessService } = getSetup(false);
+ const { accessService } = getSetup();
const withoutEnvironmentInPermissions: IRoleValidation = {
name: 'name of the role',
description: 'description',
@@ -100,7 +98,7 @@ test('should complete environment field of permissions when not present', async
});
test('should return the same object when all fields are valid and present', async () => {
- const { accessService } = getSetup(false);
+ const { accessService } = getSetup();
const roleWithAllFields: IRoleValidation = {
name: 'name of the role',
@@ -125,7 +123,7 @@ test('should return the same object when all fields are valid and present', asyn
});
test('should be able to validate and cleanup with additional properties', async () => {
- const { accessService } = getSetup(false);
+ const { accessService } = getSetup();
const base = {
name: 'name of the role',
description: 'description',
@@ -152,3 +150,22 @@ test('should be able to validate and cleanup with additional properties', async
],
});
});
+
+test('user with custom root role should get a user root role', async () => {
+ const { accessService } = getSetup(true);
+ const customRootRole = await accessService.createRole({
+ name: 'custom-root-role',
+ description: 'test custom root role',
+ type: CUSTOM_ROOT_ROLE_TYPE,
+ permissions: [],
+ });
+ const user = {
+ id: 1,
+ rootRole: customRootRole.id,
+ };
+ await accessService.setUserRootRole(user.id, customRootRole.id);
+
+ const roles = await accessService.getUserRootRoles(user.id);
+ expect(roles).toHaveLength(1);
+ expect(roles[0].name).toBe('custom-root-role');
+});
diff --git a/src/lib/services/access-service.ts b/src/lib/services/access-service.ts
index 8b88c56905..bd2841de4f 100644
--- a/src/lib/services/access-service.ts
+++ b/src/lib/services/access-service.ts
@@ -18,7 +18,6 @@ import {
IRoleData,
IUserWithRole,
RoleName,
- RoleType,
} from '../types/model';
import { IRoleStore } from 'lib/types/stores/role-store';
import NameExistsError from '../error/name-exists-error';
@@ -30,6 +29,7 @@ import {
ALL_PROJECTS,
CUSTOM_ROOT_ROLE_TYPE,
CUSTOM_PROJECT_ROLE_TYPE,
+ ROOT_ROLE_TYPES,
} from '../util/constants';
import { DEFAULT_PROJECT } from '../types/project';
import InvalidOperationError from '../error/invalid-operation-error';
@@ -253,10 +253,10 @@ export class AccessService {
const newRootRole = await this.resolveRootRole(role);
if (newRootRole) {
try {
- await this.store.removeRolesOfTypeForUser(userId, [
- RoleType.ROOT,
- RoleType.ROOT_CUSTOM,
- ]);
+ await this.store.removeRolesOfTypeForUser(
+ userId,
+ ROOT_ROLE_TYPES,
+ );
await this.store.addUserToRole(
userId,
@@ -275,7 +275,7 @@ export class AccessService {
async getUserRootRoles(userId: number): Promise<IRoleWithProject[]> {
const userRoles = await this.store.getRolesForUserId(userId);
- return userRoles.filter((r) => r.type === RoleType.ROOT);
+ return userRoles.filter(({ type }) => ROOT_ROLE_TYPES.includes(type));
}
async removeUserFromRole(
diff --git a/src/lib/types/model.ts b/src/lib/types/model.ts
index be8d8d59f4..f472852d18 100644
--- a/src/lib/types/model.ts
+++ b/src/lib/types/model.ts
@@ -386,11 +386,11 @@ export interface IProject {
defaultStickiness: string;
}
-export interface ICustomRole {
- id: number;
- name: string;
+/**
+ * Extends IRole making description mandatory
+ */
+export interface ICustomRole extends IRole {
description: string;
- type: string;
}
export interface IProjectWithCount extends IProject {
diff --git a/src/test/fixtures/fake-access-store.ts b/src/test/fixtures/fake-access-store.ts
index c4220f00b4..c754246280 100644
--- a/src/test/fixtures/fake-access-store.ts
+++ b/src/test/fixtures/fake-access-store.ts
@@ -8,8 +8,18 @@ import {
IUserRole,
} from '../../lib/types/stores/access-store';
import { IPermission } from 'lib/types/model';
+import { IRoleStore } from 'lib/types';
+import FakeRoleStore from './fake-role-store';
class AccessStoreMock implements IAccessStore {
+ fakeRolesStore: IRoleStore;
+
+ userToRoleMap: Map<number, number> = new Map();
+
+ constructor(roleStore?: IRoleStore) {
+ this.fakeRolesStore = roleStore ?? new FakeRoleStore();
+ }
+
addAccessToProject(
users: IAccessInfo[],
groups: IAccessInfo[],
@@ -88,13 +98,9 @@ class AccessStoreMock implements IAccessStore {
role_id: number,
permissions: IPermission[],
): Promise<void> {
- throw new Error('Method not implemented.');
+ return Promise.resolve(undefined);
}
- userPermissions: IUserPermission[] = [];
-
- roles: IRole[] = [];
-
getAvailablePermissions(): Promise<IPermission[]> {
throw new Error('Method not implemented.');
}
@@ -123,8 +129,17 @@ class AccessStoreMock implements IAccessStore {
throw new Error('Method not implemented.');
}
- getRolesForUserId(userId: number): Promise<IRoleWithProject[]> {
- return Promise.resolve([]);
+ async getRolesForUserId(userId: number): Promise<IRoleWithProject[]> {
+ const roleId = this.userToRoleMap.get(userId);
+ const found =
+ roleId === undefined
+ ? undefined
+ : await this.fakeRolesStore.get(roleId);
+ if (found) {
+ return Promise.resolve([found as IRoleWithProject]);
+ } else {
+ return Promise.resolve([]);
+ }
}
getUserIdsForRole(roleId: number, projectId: string): Promise<number[]> {
@@ -132,7 +147,8 @@ class AccessStoreMock implements IAccessStore {
}
addUserToRole(userId: number, roleId: number): Promise<void> {
- throw new Error('Method not implemented.');
+ this.userToRoleMap.set(userId, roleId);
+ return Promise.resolve(undefined);
}
addPermissionsToRole(
@@ -140,7 +156,8 @@ class AccessStoreMock implements IAccessStore {
permissions: string[],
projectId?: string,
): Promise<void> {
- throw new Error('Method not implemented.');
+ // do nothing for now
+ return Promise.resolve(undefined);
}
removePermissionFromRole(
diff --git a/src/test/fixtures/fake-role-store.ts b/src/test/fixtures/fake-role-store.ts
index 6c7053ef64..b4888c8cd4 100644
--- a/src/test/fixtures/fake-role-store.ts
+++ b/src/test/fixtures/fake-role-store.ts
@@ -19,7 +19,9 @@ export default class FakeRoleStore implements IRoleStore {
}
nameInUse(name: string, existingId?: number): Promise<boolean> {
- throw new Error('Method not implemented.');
+ return Promise.resolve(
+ this.roles.find((r) => r.name === name) !== undefined,
+ );
}
async getAll(): Promise<ICustomRole[]> {
@@ -29,7 +31,7 @@ export default class FakeRoleStore implements IRoleStore {
async create(role: ICustomRoleInsert): Promise<ICustomRole> {
const roleCreated = {
...role,
- type: 'some-type',
+ type: role.roleType,
id: this.roles.length,
};
this.roles.push(roleCreated);
@@ -49,7 +51,7 @@ export default class FakeRoleStore implements IRoleStore {
}
async getRoleByName(name: string): Promise<IRole> {
- return this.roles.find((r) => (r.name = name));
+ return this.roles.find((r) => (r.name = name)) as IRole;
}
getRolesForProject(projectId: string): Promise<IRole[]> {
@@ -72,8 +74,13 @@ export default class FakeRoleStore implements IRoleStore {
throw new Error('Method not implemented.');
}
- get(key: number): Promise<ICustomRole> {
- throw new Error('Method not implemented.');
+ get(id: number): Promise<ICustomRole> {
+ const found = this.roles.find((r) => r.id === id);
+ if (!found) {
+ // this edge case is not properly contemplated in the type definition
+ throw new Error('Not found');
+ }
+ return Promise.resolve(found);
}
exists(key: number): Promise<boolean> {