diff --git a/src/lib/db/access-store.test.ts b/src/lib/db/access-store.test.ts new file mode 100644 index 0000000000..18b1228fe9 --- /dev/null +++ b/src/lib/db/access-store.test.ts @@ -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); + }, +); diff --git a/src/lib/db/access-store.ts b/src/lib/db/access-store.ts index 1c8eb619c5..072eee2673 100644 --- a/src/lib/db/access-store.ts +++ b/src/lib/db/access-store.ts @@ -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 => { + 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); + + const permissionsWithIds = permissions.map((permission) => ({ + id: rowByPermissionName[permission.name].id, + ...permission, + })); + + stopTimer(); + return permissionsWithIds; + }; + + resolvePermissions = async ( + permissions: PermissionRef[], + ): Promise => { + 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 { 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 { - 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 { - const rows = await this.db - .select('id as permissionId') - .from(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); diff --git a/src/lib/schema/role-schema.test.ts b/src/lib/schema/role-schema.test.ts index 05fb65b6d0..7d2ed31935 100644 --- a/src/lib/schema/role-schema.test.ts +++ b/src/lib/schema/role-schema.test.ts @@ -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]', ); } }); diff --git a/src/lib/schema/role-schema.ts b/src/lib/schema/role-schema.ts index 67fc1d5ee7..4946efc906 100644 --- a/src/lib/schema/role-schema.ts +++ b/src/lib/schema/role-schema.ts @@ -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 diff --git a/src/lib/services/access-service.test.ts b/src/lib/services/access-service.test.ts index 6a4b72997e..688ce80279 100644 --- a/src/lib/services/access-service.test.ts +++ b/src/lib/services/access-service.test.ts @@ -154,6 +154,7 @@ test('should be able to validate and cleanup with additional properties', async permissions: [ { id: 1, + name: 'name', environment: 'development', }, ], diff --git a/src/lib/services/access-service.ts b/src/lib/services/access-service.ts index 2286aa41e6..46a32cd59d 100644 --- a/src/lib/services/access-service.ts +++ b/src/lib/services/access-service.ts @@ -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( diff --git a/src/lib/types/stores/access-store.ts b/src/lib/types/stores/access-store.ts index bd1f0c9126..b3f5194c3f 100644 --- a/src/lib/types/stores/access-store.ts +++ b/src/lib/types/stores/access-store.ts @@ -164,7 +164,7 @@ export interface IAccessStore extends Store { addPermissionsToRole( role_id: number, - permissions: string[], + permissions: PermissionRef[], environment?: string, ): Promise; diff --git a/src/test/e2e/api/admin/api-token.auth.e2e.test.ts b/src/test/e2e/api/admin/api-token.auth.e2e.test.ts index fed77e8b52..97b7119719 100644 --- a/src/test/e2e/api/admin/api-token.auth.e2e.test.ts +++ b/src/test/e2e/api/admin/api-token.auth.e2e.test.ts @@ -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( diff --git a/src/test/e2e/services/access-service.e2e.test.ts b/src/test/e2e/services/access-service.e2e.test.ts index 93beeac2b0..40119816a5 100644 --- a/src/test/e2e/services/access-service.e2e.test.ts +++ b/src/test/e2e/services/access-service.e2e.test.ts @@ -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', }, ]); diff --git a/src/test/fixtures/fake-access-store.ts b/src/test/fixtures/fake-access-store.ts index 19ef6619df..3daa2a6646 100644 --- a/src/test/fixtures/fake-access-store.ts +++ b/src/test/fixtures/fake-access-store.ts @@ -180,8 +180,8 @@ class AccessStoreMock implements IAccessStore { addPermissionsToRole( role_id: number, - permissions: string[], - projectId?: string, + permissions: PermissionRef[], + environment?: string, ): Promise { // do nothing for now return Promise.resolve(undefined); diff --git a/src/test/fixtures/no-logger.ts b/src/test/fixtures/no-logger.ts index 35c5df361e..30f222ae9a 100644 --- a/src/test/fixtures/no-logger.ts +++ b/src/test/fixtures/no-logger.ts @@ -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;