Skip to content

Commit

Permalink
fix: fetching user root roles include custom ones (#4068)
Browse files Browse the repository at this point in the history 
## 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>
  • Loading branch information
@gastonfournier @nunogois
gastonfournier and nunogois committed Jun 22, 2023
1 parent 40a4451 commit 4cedb00
Show file tree
Hide file tree
Showing 6 changed files with 90 additions and 49 deletions.
2 changes: 1 addition & 1 deletion src/lib/features/access/createAccessService.ts
View file
Expand Up @@ -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 },
Expand Down
65 changes: 41 additions & 24 deletions src/lib/services/access-service.test.ts
View file
@@ -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}`,
Expand All @@ -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',
Expand All @@ -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',
};
Expand All @@ -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',
Expand All @@ -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',
Expand All @@ -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',
Expand All @@ -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',
Expand All @@ -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');
});
12 changes: 6 additions & 6 deletions src/lib/services/access-service.ts
View file
Expand Up @@ -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';
Expand All @@ -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';
Expand Down Expand Up @@ -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,
Expand All @@ -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(
Expand Down
8 changes: 4 additions & 4 deletions src/lib/types/model.ts
View file
Expand Up @@ -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 {
Expand Down
35 changes: 26 additions & 9 deletions src/test/fixtures/fake-access-store.ts
View file
Expand Up @@ -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[],
Expand Down Expand Up @@ -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.');
}
Expand Down Expand Up @@ -123,24 +129,35 @@ 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[]> {
throw new Error('Method not implemented.');
}

addUserToRole(userId: number, roleId: number): Promise<void> {
throw new Error('Method not implemented.');
this.userToRoleMap.set(userId, roleId);
return Promise.resolve(undefined);
}

addPermissionsToRole(
role_id: number,
permissions: string[],
projectId?: string,
): Promise<void> {
throw new Error('Method not implemented.');
// do nothing for now
return Promise.resolve(undefined);
}

removePermissionFromRole(
Expand Down
17 changes: 12 additions & 5 deletions src/test/fixtures/fake-role-store.ts
View file
Expand Up @@ -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[]> {
Expand All @@ -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);
Expand All @@ -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[]> {
Expand All @@ -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> {
Expand Down

1 comment on commit 4cedb00

@vercel
Copy link

@vercel vercel bot commented on 4cedb00 Jun 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Successfully deployed to the following URLs:

unleash-monorepo-frontend – ./frontend

unleash-monorepo-frontend.vercel.app
unleash-monorepo-frontend-git-main-unleash-team.vercel.app
unleash-monorepo-frontend-unleash-team.vercel.app

Please sign in to comment.