Mirrors/unleash.unleash
1
0
Fork 0
mirror of https://github.com/Unleash/unleash.git synced 2024-12-22 19:07:54 +01:00
Code Issues Projects Releases Wiki Activity

chore: improve access service (#4689)

Browse Source 
## About the changes
This enables us to use names instead of permission ids across all our
APIs at the computational cost of searching for the ids in the DB but
improving the API user experience

## Open topics
We're using methods that are test-only and circumvent our business
logic. This makes our test to rely on assumptions that are not always
true because these assumptions are not validated frequently.

i.e. We are expecting that after removing a permission it's no longer
there, but to test this, the permission has to be there before:

78273e4ff3/src/test/e2e/services/access-service.e2e.test.ts (L367-L375)

But it seems that's not the case.

We'll look into improving this later.
This commit is contained in:
Gastón Fournier 2023-09-19 11:36:29 +02:00 committed by GitHub
parent 2843388673
commit 2186e2b568
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 270 additions and 125 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

110
src/lib/db/access-store.test.ts Normal file
View File

@ -0,0 +1,110 @@
import dbInit from '../../test/e2e/helpers/database-init';
import getLogger from '../../test/fixtures/no-logger';
import { PermissionRef } from 'lib/services/access-service';
import { AccessStore } from './access-store';
let db;
beforeAll(async () => {
db = await dbInit('access_store_serial', getLogger);
});
afterAll(async () => {
if (db) {
await db.destroy();
}
});
// Helper function to make the test cases more readable
const args = (permissions: PermissionRef[], expectations?: PermissionRef[]) => {
if (expectations) {
return [permissions, expectations];
} else {
return [permissions];
}
};
test('resolvePermissions returns empty list if undefined', async () => {
const access = db.stores.accessStore as AccessStore;
const result = await access.resolvePermissions(
undefined as unknown as PermissionRef[],
);
expect(result).toStrictEqual([]);
});
test('resolvePermissions returns empty list if empty list', async () => {
const access = db.stores.accessStore as AccessStore;
const result = await access.resolvePermissions([] as PermissionRef[]);
expect(result).toStrictEqual([]);
});
test.each([
args([{ id: 1 }]),
args([{ id: 4, environment: 'development' }]),
args([{ id: 4, name: 'should keep the id' }]),
args([
{ id: 1, environment: 'development' },
{ id: 2, name: 'ignore this name' },
]),
])(
'resolvePermissions with permission ids (%o) returns the list unmodified',
async (permissions) => {
const access = db.stores.accessStore as AccessStore;
const result = await access.resolvePermissions(permissions);
expect(result).toStrictEqual(permissions);
},
);
test.each([
args(
[{ name: 'CREATE_CONTEXT_FIELD' }],
[{ id: 18, name: 'CREATE_CONTEXT_FIELD' }],
),
args(
[{ name: 'CREATE_FEATURE', environment: 'development' }],
[{ id: 2, name: 'CREATE_FEATURE', environment: 'development' }],
),
args(
[
{ name: 'CREATE_CONTEXT_FIELD' },
{ name: 'CREATE_FEATURE', environment: 'development' },
],
[
{ id: 18, name: 'CREATE_CONTEXT_FIELD' },
{ id: 2, name: 'CREATE_FEATURE', environment: 'development' },
],
),
])(
'resolvePermissions with permission names (%o) will inject the ids',
async (permissions, expected) => {
const access = db.stores.accessStore as AccessStore;
const result = await access.resolvePermissions(permissions);
expect(result).toStrictEqual(expected);
},
);
test.each([
args(
[
{ name: 'CREATE_CONTEXT_FIELD' },
{ id: 3 },
{ name: 'CREATE_FEATURE', environment: 'development' },
{ id: 15, environment: 'development' },
{ name: 'UPDATE_FEATURE', environment: 'development' },
],
[
{ id: 18, name: 'CREATE_CONTEXT_FIELD' },
{ id: 3 },
{ id: 2, name: 'CREATE_FEATURE', environment: 'development' },
{ id: 15, environment: 'development' },
{ id: 7, name: 'UPDATE_FEATURE', environment: 'development' },
],
),
])(
'resolvePermissions mixed ids and names (%o) will inject the ids where they are missing',
async (permissions, expected) => {
const access = db.stores.accessStore as AccessStore;
const result = await access.resolvePermissions(permissions);
expect(result).toStrictEqual(expected);
},
);

99
src/lib/db/access-store.ts
View File

@ -20,7 +20,11 @@ import {
ROOT_PERMISSION_TYPE,
} from '../util/constants';
import { Db } from './db';
import { IdPermissionRef } from 'lib/services/access-service';
import {
IdPermissionRef,
NamePermissionRef,
PermissionRef,
} from 'lib/services/access-service';
const T = {
ROLE_USER: 'role_user',
@ -46,6 +50,12 @@ interface IPermissionRow {
role_id: number;
}
type ResolvedPermission = {
id: number;
name?: string;
environment?: string;
};
export class AccessStore implements IAccessStore {
private logger: Logger;
@ -63,6 +73,75 @@ export class AccessStore implements IAccessStore {
});
}
private permissionHasId = (permission: PermissionRef): boolean => {
return (permission as IdPermissionRef).id !== undefined;
};
private permissionNamesToIds = async (
permissions: NamePermissionRef[],
): Promise<ResolvedPermission[]> => {
const permissionNames = (permissions ?? [])
.filter((p) => p.name !== undefined)
.map((p) => p.name);
if (permissionNames.length === 0) {
return [];
}
const stopTimer = this.timer('permissionNamesToIds');
const rows = await this.db
.select('id', 'permission')
.from(T.PERMISSIONS)
.whereIn('permission', permissionNames);
const rowByPermissionName = rows.reduce((acc, row) => {
acc[row.permission] = row;
return acc;
}, {} as Map<string, IPermissionRow>);
const permissionsWithIds = permissions.map((permission) => ({
id: rowByPermissionName[permission.name].id,
...permission,
}));
stopTimer();
return permissionsWithIds;
};
resolvePermissions = async (
permissions: PermissionRef[],
): Promise<ResolvedPermission[]> => {
if (permissions === undefined || permissions.length === 0) {
return [];
}
// permissions without ids (just names)
const permissionsWithoutIds = permissions.filter(
(p) => !this.permissionHasId(p),
) as NamePermissionRef[];
const idPermissionsFromNamed = await this.permissionNamesToIds(
permissionsWithoutIds,
);
if (permissionsWithoutIds.length === permissions.length) {
// all named permissions without ids
return idPermissionsFromNamed;
} else if (permissionsWithoutIds.length === 0) {
// all permissions have ids
return permissions as ResolvedPermission[];
}
// some permissions have ids, some don't (should not happen!)
return permissions.map((permission) => {
if (this.permissionHasId(permission)) {
return permission as ResolvedPermission;
} else {
return idPermissionsFromNamed.find(
(p) => p.name === (permission as NamePermissionRef).name,
)!;
}
});
};
async delete(key: number): Promise<void> {
await this.db(T.ROLES).where({ id: key }).del();
}
@ -222,9 +301,11 @@ export class AccessStore implements IAccessStore {
async addEnvironmentPermissionsToRole(
role_id: number,
permissions: IdPermissionRef[],
permissions: PermissionRef[],
): Promise<void> {
const rows = permissions.map((permission) => {
const resolvedPermission = await this.resolvePermissions(permissions);
const rows = resolvedPermission.map((permission) => {
return {
role_id,
permission_id: permission.id,
@ -700,18 +781,16 @@ export class AccessStore implements IAccessStore {
async addPermissionsToRole(
role_id: number,
permissions: string[],
permissions: PermissionRef[],
environment?: string,
): Promise<void> {
const rows = await this.db
.select('id as permissionId')
.from<number>(T.PERMISSIONS)
.whereIn('permission', permissions);
// no need to pass down the environment in this particular case because it'll be overriden
const permissionsWithIds = await this.resolvePermissions(permissions);
const newRoles = rows.map((row) => ({
const newRoles = permissionsWithIds.map((p) => ({
role_id,
environment,
permission_id: row.permissionId,
permission_id: p.id,
}));
return this.db.batchInsert(T.ROLE_PERMISSION, newRoles);

2
src/lib/schema/role-schema.test.ts
View File

@ -38,7 +38,7 @@ test('role schema rejects a role with a broken permission list', async () => {
await roleSchema.validateAsync(role);
} catch (error) {
expect(error.details[0].message).toBe(
'"permissions[0].id" is required',
'"permissions[0]" must contain at least one of [id, name]',
);
}
});

4
src/lib/schema/role-schema.ts
View File

@ -3,9 +3,11 @@ import joi from 'joi';
export const permissionRoleSchema = joi
.object()
.keys({
id: joi.number().required(),
id: joi.number(),
name: joi.string(),
environment: joi.string().optional().allow('').allow(null).default(''),
})
.or('id', 'name')
.options({ stripUnknown: true, allowUnknown: false, abortEarly: false });
export const roleSchema = joi

1
src/lib/services/access-service.test.ts
View File

@ -154,6 +154,7 @@ test('should be able to validate and cleanup with additional properties', async
permissions: [
{
id: 1,
name: 'name',
environment: 'development',
},
],

10
src/lib/services/access-service.ts
View File

@ -70,7 +70,7 @@ export interface IRoleValidation {
permissions?: PermissionRef[];
}
interface IRoleUpdate {
export interface IRoleUpdate {
id: number;
name: string;
description: string;
@ -406,7 +406,7 @@ export class AccessService {
}
return this.store.addPermissionsToRole(
roleId,
[permission],
[{ name: permission }],
environment,
);
}
@ -630,11 +630,13 @@ export class AccessService {
const newRole = await this.roleStore.create(baseRole);
if (rolePermissions) {
if (roleType === CUSTOM_ROOT_ROLE_TYPE) {
// this branch uses named permissions
await this.store.addPermissionsToRole(
newRole.id,
rolePermissions.map((p: NamePermissionRef) => p.name),
rolePermissions,
);
} else {
// this branch uses id permissions
await this.store.addEnvironmentPermissionsToRole(
newRole.id,
rolePermissions,
@ -673,7 +675,7 @@ export class AccessService {
if (roleType === CUSTOM_ROOT_ROLE_TYPE) {
await this.store.addPermissionsToRole(
newRole.id,
rolePermissions.map((p: NamePermissionRef) => p.name),
rolePermissions,
);
} else {
await this.store.addEnvironmentPermissionsToRole(

2
src/lib/types/stores/access-store.ts
View File

@ -164,7 +164,7 @@ export interface IAccessStore extends Store<IRole, number> {
addPermissionsToRole(
role_id: number,
permissions: string[],
permissions: PermissionRef[],
environment?: string,
): Promise<void>;

31
src/test/e2e/api/admin/api-token.auth.e2e.test.ts
View File

@ -12,6 +12,7 @@ import {
UPDATE_CLIENT_API_TOKEN,
} from '../../../../lib/types';
import { addDays } from 'date-fns';
import { AccessService, UserService } from 'lib/services';
let stores;
let db;
@ -175,33 +176,32 @@ test('Token-admin should be allowed to create token', async () => {
test('A role with only CREATE_PROJECT_API_TOKEN can create project tokens', async () => {
expect.assertions(0);
const preHook = (app, config, { userService, accessService }) => {
const preHook = (
app,
config,
{
userService,
accessService,
}: { userService: UserService; accessService: AccessService },
) => {
app.use('/api/admin/', async (req, res, next) => {
const role = await accessService.getRootRole(RoleName.VIEWER);
const role = (await accessService.getRootRole(RoleName.VIEWER))!;
const user = await userService.createUser({
email: 'powerpuffgirls_viewer@example.com',
rootRole: role.id,
});
req.user = user;
const createClientApiTokenRole = await accessService.createRole({
name: 'project_client_token_creator',
description: 'Can create client tokens',
permissions: [],
permissions: [{ name: CREATE_PROJECT_API_TOKEN }],
type: 'root-custom',
});
await accessService.addPermissionToRole(
role.id,
CREATE_PROJECT_API_TOKEN,
);
await accessService.addUserToRole(
user.id,
createClientApiTokenRole.id,
'default',
);
req.user = await userService.createUser({
email: 'someguyinplaces@example.com',
rootRole: role.id,
});
req.user = user;
next();
});
};
@ -225,12 +225,12 @@ describe('Fine grained API token permissions', () => {
test('should be allowed to create client tokens', async () => {
const preHook = (app, config, { userService, accessService }) => {
app.use('/api/admin/', async (req, res, next) => {
const role = await accessService.getRootRole(
const builtInRole = await accessService.getRootRole(
RoleName.VIEWER,
);
const user = await userService.createUser({
email: 'mylittlepony_viewer@example.com',
rootRole: role.id,
rootRole: builtInRole.id,
});
req.user = user;
const createClientApiTokenRole =
@ -240,8 +240,9 @@ describe('Fine grained API token permissions', () => {
permissions: [],
type: 'root-custom',
});
// not sure if we should add the permission to the builtin role or to the newly created role
await accessService.addPermissionToRole(
role.id,
builtInRole.id,
CREATE_CLIENT_API_TOKEN,
);
await accessService.addUserToRole(

116
src/test/e2e/services/access-service.e2e.test.ts
View File

@ -1,14 +1,15 @@
import dbInit, { ITestDb } from '../helpers/database-init';
import getLogger from '../../fixtures/no-logger';
// eslint-disable-next-line import/no-unresolved
import { AccessService } from '../../../lib/services/access-service';
import {
AccessService,
PermissionRef,
} from '../../../lib/services/access-service';
import * as permissions from '../../../lib/types/permissions';
import { RoleName } from '../../../lib/types/model';
import {
ICreateGroupUserModel,
IPermission,
IUnleashStores,
IUserAccessOverview,
} from '../../../lib/types';
@ -70,7 +71,7 @@ const createGroup = async ({
};
let roleIndex = 0;
const createRole = async (rolePermissions: IPermission[]) => {
const createRole = async (rolePermissions: PermissionRef[]) => {
return accessService.createRole({
name: `Role ${roleIndex}`,
description: `Role ${roleIndex++} description`,
@ -269,6 +270,25 @@ beforeAll(async () => {
);
editorUser = await createUser(editorRole.id);
const testAdmin = await createUser(adminRole.id);
await projectService.createProject(
{
id: 'some-project',
name: 'Some project',
description: 'Used in the test',
},
testAdmin,
);
await projectService.createProject(
{
id: 'unusedprojectname',
name: 'Another project not used',
description: 'Also used in the test',
},
testAdmin,
);
});
afterAll(async () => {
@ -372,6 +392,12 @@ test('should remove CREATE_FEATURE on default environment', async () => {
'*',
);
// TODO: to validate the remove works, we should make sure that we had permission before removing it
// this currently does not work, just keeping the comment here for future reference
// expect(
// await accessService.hasPermission(user, CREATE_FEATURE, 'some-project'),
// ).toBe(true);
await accessService.removePermissionFromRole(
editRole.id,
permissions.CREATE_FEATURE,
@ -621,7 +647,7 @@ test('should support permission with "ALL" environment requirement', async () =>
const { CREATE_FEATURE_STRATEGY } = permissions;
await accessStore.addPermissionsToRole(
customRole.id,
[CREATE_FEATURE_STRATEGY],
[{ name: CREATE_FEATURE_STRATEGY }],
'production',
);
await accessStore.addUserToRole(user.id, customRole.id, ALL_PROJECTS);
@ -728,14 +754,10 @@ test('Should be denied access to delete a role that is in use', async () => {
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
{
id: 8,
name: 'DELETE_FEATURE',
displayName: 'Delete Feature Toggles',
type: 'project',
},
]);
@ -971,8 +993,6 @@ test('Should allow user to take multiple group roles and have expected permissio
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
]);
@ -980,8 +1000,6 @@ test('Should allow user to take multiple group roles and have expected permissio
{
id: 8,
name: 'DELETE_FEATURE',
displayName: 'Delete Feature Toggles',
type: 'project',
},
]);
@ -1153,28 +1171,20 @@ test('if user has two roles user has union of permissions from the two roles', a
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
{
id: 8,
name: 'DELETE_FEATURE',
displayName: 'Delete Feature Toggles',
type: 'project',
},
]);
const secondRole = await createRole([
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
{
id: 13,
name: 'UPDATE_PROJECT',
displayName: 'Can update projects',
type: 'project',
},
]);
@ -1202,28 +1212,20 @@ test('calling set for user overwrites existing roles', async () => {
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
{
id: 8,
name: 'DELETE_FEATURE',
displayName: 'Delete Feature Toggles',
type: 'project',
},
]);
const secondRole = await createRole([
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
{
id: 13,
name: 'UPDATE_PROJECT',
displayName: 'Can update projects',
type: 'project',
},
]);
@ -1273,28 +1275,20 @@ test('if group has two roles user has union of permissions from the two roles',
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
{
id: 8,
name: 'DELETE_FEATURE',
displayName: 'Delete Feature Toggles',
type: 'project',
},
]);
const secondRole = await createRole([
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
{
id: 13,
name: 'UPDATE_PROJECT',
displayName: 'Can update projects',
type: 'project',
},
]);
@ -1328,28 +1322,20 @@ test('calling set for group overwrites existing roles', async () => {
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
{
id: 8,
name: 'DELETE_FEATURE',
displayName: 'Delete Feature Toggles',
type: 'project',
},
]);
const secondRole = await createRole([
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
{
id: 13,
name: 'UPDATE_PROJECT',
displayName: 'Can update projects',
type: 'project',
},
]);
@ -1405,8 +1391,6 @@ test('group with root role can be assigned a project specific role', async () =>
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
]);
@ -1474,8 +1458,6 @@ test('calling set roles for user with empty role array removes all roles', async
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
]);
@ -1506,14 +1488,10 @@ test('calling set roles for user with empty role array should not remove root ro
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
{
id: 8,
name: 'DELETE_FEATURE',
displayName: 'Delete Feature Toggles',
type: 'project',
},
]);
@ -1545,14 +1523,10 @@ test('remove user access should remove all project roles', async () => {
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
{
id: 8,
name: 'DELETE_FEATURE',
displayName: 'Delete Feature Toggles',
type: 'project',
},
]);
@ -1560,8 +1534,6 @@ test('remove user access should remove all project roles', async () => {
{
id: 13,
name: 'UPDATE_PROJECT',
displayName: 'Can update projects',
type: 'project',
},
]);
@ -1593,14 +1565,10 @@ test('remove user access should remove all project roles, while leaving root rol
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
{
id: 8,
name: 'DELETE_FEATURE',
displayName: 'Delete Feature Toggles',
type: 'project',
},
]);
@ -1608,8 +1576,6 @@ test('remove user access should remove all project roles, while leaving root rol
{
id: 13,
name: 'UPDATE_PROJECT',
displayName: 'Can update projects',
type: 'project',
},
]);
@ -1670,8 +1636,6 @@ test('calling set roles for group with empty role array removes all roles', asyn
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
]);
@ -1714,14 +1678,10 @@ test('calling set roles for group with empty role array should not remove root r
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
{
id: 8,
name: 'DELETE_FEATURE',
displayName: 'Delete Feature Toggles',
type: 'project',
},
]);
@ -1764,14 +1724,10 @@ test('remove group access should remove all project roles', async () => {
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
{
id: 8,
name: 'DELETE_FEATURE',
displayName: 'Delete Feature Toggles',
type: 'project',
},
]);
@ -1779,8 +1735,6 @@ test('remove group access should remove all project roles', async () => {
{
id: 13,
name: 'UPDATE_PROJECT',
displayName: 'Can update projects',
type: 'project',
},
]);
@ -1817,14 +1771,10 @@ test('remove group access should remove all project roles, while leaving root ro
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
{
id: 8,
name: 'DELETE_FEATURE',
displayName: 'Delete Feature Toggles',
type: 'project',
},
]);
@ -1832,8 +1782,6 @@ test('remove group access should remove all project roles, while leaving root ro
{
id: 13,
name: 'UPDATE_PROJECT',
displayName: 'Can update projects',
type: 'project',
},
]);
@ -1914,8 +1862,6 @@ test('access overview should have group access for groups that they are in', asy
{
id: 13,
name: 'UPDATE_PROJECT',
displayName: 'Can update projects',
type: 'project',
},
]);

4
src/test/fixtures/fake-access-store.ts vendored
View File

@ -180,8 +180,8 @@ class AccessStoreMock implements IAccessStore {
addPermissionsToRole(
role_id: number,
permissions: string[],
projectId?: string,
permissions: PermissionRef[],
environment?: string,
): Promise<void> {
// do nothing for now
return Promise.resolve(undefined);

16
src/test/fixtures/no-logger.ts vendored
View File

@ -1,23 +1,27 @@
/* eslint-disable no-console */
import { Logger } from '../../lib/logger';
let muteError = false;
let verbose = false;
function noLoggerProvider(): Logger {
// do something with the name
return {
debug: () => {},
info: () => {},
warn: () => {},
debug: verbose ? console.log : () => {},
info: verbose ? console.log : () => {},
warn: verbose ? console.warn : () => {},
error: muteError ? () => {} : console.error,
fatal: console.error,
};
}
noLoggerProvider.setMuteError = (mute) => {
noLoggerProvider.setMuteError = (mute: boolean) => {
muteError = mute;
};
// use for debugging only, try not to commit tests with verbose set to true
noLoggerProvider.setVerbose = (beVerbose: boolean) => {
verbose = beVerbose;
};
module.exports = noLoggerProvider;
export default noLoggerProvider;