Mirrors/unleash.unleash
1
0
Fork 0
mirror of https://github.com/Unleash/unleash.git synced 2025-01-20 00:08:02 +01:00
Code Issues Projects Releases Wiki Activity

Revert "chore: improve access service" (#4773)

Browse Source 
Reverts Unleash/unleash#4689 temporarily to figure out what's the
problem with the failing test
This commit is contained in:
Gastón Fournier 2023-09-19 12:03:16 +02:00 committed by GitHub
parent 2186e2b568
commit 12d9297f68
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 125 additions and 270 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -1,110 +0,0 @@
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,11 +20,7 @@ import {
ROOT_PERMISSION_TYPE,
} from '../util/constants';
import { Db } from './db';
import {
IdPermissionRef,
NamePermissionRef,
PermissionRef,
} from 'lib/services/access-service';
import { IdPermissionRef } from 'lib/services/access-service';
const T = {
ROLE_USER: 'role_user',
@ -50,12 +46,6 @@ interface IPermissionRow {
role_id: number;
}
type ResolvedPermission = {
id: number;
name?: string;
environment?: string;
};
export class AccessStore implements IAccessStore {
private logger: Logger;
@ -73,75 +63,6 @@ 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();
}
@ -301,11 +222,9 @@ export class AccessStore implements IAccessStore {
async addEnvironmentPermissionsToRole(
role_id: number,
permissions: PermissionRef[],
permissions: IdPermissionRef[],
): Promise<void> {
const resolvedPermission = await this.resolvePermissions(permissions);
const rows = resolvedPermission.map((permission) => {
const rows = permissions.map((permission) => {
return {
role_id,
permission_id: permission.id,
@ -781,16 +700,18 @@ export class AccessStore implements IAccessStore {
async addPermissionsToRole(
role_id: number,
permissions: PermissionRef[],
permissions: string[],
environment?: string,
): Promise<void> {
// no need to pass down the environment in this particular case because it'll be overriden
const permissionsWithIds = await this.resolvePermissions(permissions);
const rows = await this.db
.select('id as permissionId')
.from<number>(T.PERMISSIONS)
.whereIn('permission', permissions);
const newRoles = permissionsWithIds.map((p) => ({
const newRoles = rows.map((row) => ({
role_id,
environment,
permission_id: p.id,
permission_id: row.permissionId,
}));
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]" must contain at least one of [id, name]',
'"permissions[0].id" is required',
);
}
});

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

@ -3,11 +3,9 @@ import joi from 'joi';
export const permissionRoleSchema = joi
.object()
.keys({
id: joi.number(),
name: joi.string(),
id: joi.number().required(),
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,7 +154,6 @@ 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[];
}
export interface IRoleUpdate {
interface IRoleUpdate {
id: number;
name: string;
description: string;
@ -406,7 +406,7 @@ export class AccessService {
}
return this.store.addPermissionsToRole(
roleId,
[{ name: permission }],
[permission],
environment,
);
}
@ -630,13 +630,11 @@ 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,
rolePermissions.map((p: NamePermissionRef) => p.name),
);
} else {
// this branch uses id permissions
await this.store.addEnvironmentPermissionsToRole(
newRole.id,
rolePermissions,
@ -675,7 +673,7 @@ export class AccessService {
if (roleType === CUSTOM_ROOT_ROLE_TYPE) {
await this.store.addPermissionsToRole(
newRole.id,
rolePermissions,
rolePermissions.map((p: NamePermissionRef) => p.name),
);
} 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: PermissionRef[],
permissions: string[],
environment?: string,
): Promise<void>;

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

@ -12,7 +12,6 @@ import {
UPDATE_CLIENT_API_TOKEN,
} from '../../../../lib/types';
import { addDays } from 'date-fns';
import { AccessService, UserService } from 'lib/services';
let stores;
let db;
@ -176,32 +175,33 @@ 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,
}: { userService: UserService; accessService: AccessService },
) => {
const preHook = (app, config, { userService, 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: [{ name: CREATE_PROJECT_API_TOKEN }],
permissions: [],
type: 'root-custom',
});
await accessService.addPermissionToRole(
role.id,
CREATE_PROJECT_API_TOKEN,
);
await accessService.addUserToRole(
user.id,
createClientApiTokenRole.id,
'default',
);
req.user = user;
req.user = await userService.createUser({
email: 'someguyinplaces@example.com',
rootRole: role.id,
});
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 builtInRole = await accessService.getRootRole(
const role = await accessService.getRootRole(
RoleName.VIEWER,
);
const user = await userService.createUser({
email: 'mylittlepony_viewer@example.com',
rootRole: builtInRole.id,
rootRole: role.id,
});
req.user = user;
const createClientApiTokenRole =
@ -240,9 +240,8 @@ 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(
builtInRole.id,
role.id,
CREATE_CLIENT_API_TOKEN,
);
await accessService.addUserToRole(

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

@ -1,15 +1,14 @@
import dbInit, { ITestDb } from '../helpers/database-init';
import getLogger from '../../fixtures/no-logger';
import {
AccessService,
PermissionRef,
} from '../../../lib/services/access-service';
// eslint-disable-next-line import/no-unresolved
import { AccessService } 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';
@ -71,7 +70,7 @@ const createGroup = async ({
};
let roleIndex = 0;
const createRole = async (rolePermissions: PermissionRef[]) => {
const createRole = async (rolePermissions: IPermission[]) => {
return accessService.createRole({
name: `Role ${roleIndex}`,
description: `Role ${roleIndex++} description`,
@ -270,25 +269,6 @@ 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 () => {
@ -392,12 +372,6 @@ 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,
@ -647,7 +621,7 @@ test('should support permission with "ALL" environment requirement', async () =>
const { CREATE_FEATURE_STRATEGY } = permissions;
await accessStore.addPermissionsToRole(
customRole.id,
[{ name: CREATE_FEATURE_STRATEGY }],
[CREATE_FEATURE_STRATEGY],
'production',
);
await accessStore.addUserToRole(user.id, customRole.id, ALL_PROJECTS);
@ -754,10 +728,14 @@ 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',
},
]);
@ -993,6 +971,8 @@ test('Should allow user to take multiple group roles and have expected permissio
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
]);
@ -1000,6 +980,8 @@ test('Should allow user to take multiple group roles and have expected permissio
{
id: 8,
name: 'DELETE_FEATURE',
displayName: 'Delete Feature Toggles',
type: 'project',
},
]);
@ -1171,20 +1153,28 @@ 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',
},
]);
@ -1212,20 +1202,28 @@ 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',
},
]);
@ -1275,20 +1273,28 @@ 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',
},
]);
@ -1322,20 +1328,28 @@ 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',
},
]);
@ -1391,6 +1405,8 @@ test('group with root role can be assigned a project specific role', async () =>
{
id: 2,
name: 'CREATE_FEATURE',
displayName: 'Create Feature Toggles',
type: 'project',
},
]);
@ -1458,6 +1474,8 @@ 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',
},
]);
@ -1488,10 +1506,14 @@ 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',
},
]);
@ -1523,10 +1545,14 @@ 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',
},
]);
@ -1534,6 +1560,8 @@ test('remove user access should remove all project roles', async () => {
{
id: 13,
name: 'UPDATE_PROJECT',
displayName: 'Can update projects',
type: 'project',
},
]);
@ -1565,10 +1593,14 @@ 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',
},
]);
@ -1576,6 +1608,8 @@ test('remove user access should remove all project roles, while leaving root rol
{
id: 13,
name: 'UPDATE_PROJECT',
displayName: 'Can update projects',
type: 'project',
},
]);
@ -1636,6 +1670,8 @@ 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',
},
]);
@ -1678,10 +1714,14 @@ 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',
},
]);
@ -1724,10 +1764,14 @@ 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',
},
]);
@ -1735,6 +1779,8 @@ test('remove group access should remove all project roles', async () => {
{
id: 13,
name: 'UPDATE_PROJECT',
displayName: 'Can update projects',
type: 'project',
},
]);
@ -1771,10 +1817,14 @@ 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',
},
]);
@ -1782,6 +1832,8 @@ test('remove group access should remove all project roles, while leaving root ro
{
id: 13,
name: 'UPDATE_PROJECT',
displayName: 'Can update projects',
type: 'project',
},
]);
@ -1862,6 +1914,8 @@ 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: PermissionRef[],
environment?: string,
permissions: string[],
projectId?: string,
): Promise<void> {
// do nothing for now
return Promise.resolve(undefined);

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

@ -1,27 +1,23 @@
/* 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: verbose ? console.log : () => {},
info: verbose ? console.log : () => {},
warn: verbose ? console.warn : () => {},
debug: () => {},
info: () => {},
warn: () => {},
error: muteError ? () => {} : console.error,
fatal: console.error,
};
}
noLoggerProvider.setMuteError = (mute: boolean) => {
noLoggerProvider.setMuteError = (mute) => {
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;