From 4e0ba5a09e105c5f463f26a75e5d709cf6a56983 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Wed, 13 Sep 2023 17:04:26 +0200 Subject: [PATCH 01/13] test: why does it fail? --- src/test/e2e/services/access-service.e2e.test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/test/e2e/services/access-service.e2e.test.ts b/src/test/e2e/services/access-service.e2e.test.ts index a959b82d73c0..64677498e25b 100644 --- a/src/test/e2e/services/access-service.e2e.test.ts +++ b/src/test/e2e/services/access-service.e2e.test.ts @@ -363,6 +363,9 @@ test('should remove CREATE_FEATURE on default environment', async () => { permissions.CREATE_FEATURE, '*', ); + expect( + await accessService.hasPermission(user, CREATE_FEATURE, 'some-project'), + ).toBe(true); await accessService.removePermissionFromRole( editRole.id, From 45c285ea7ad66e3bbf0b3ac1aa2087f48bd370ef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Wed, 13 Sep 2023 17:31:15 +0200 Subject: [PATCH 02/13] Fix test --- src/test/e2e/services/access-service.e2e.test.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/test/e2e/services/access-service.e2e.test.ts b/src/test/e2e/services/access-service.e2e.test.ts index 64677498e25b..0a59f5add72a 100644 --- a/src/test/e2e/services/access-service.e2e.test.ts +++ b/src/test/e2e/services/access-service.e2e.test.ts @@ -364,7 +364,7 @@ test('should remove CREATE_FEATURE on default environment', async () => { '*', ); expect( - await accessService.hasPermission(user, CREATE_FEATURE, 'some-project'), + await accessService.hasPermission(user, CREATE_FEATURE, 'default'), ).toBe(true); await accessService.removePermissionFromRole( @@ -373,6 +373,11 @@ test('should remove CREATE_FEATURE on default environment', async () => { '*', ); + expect( + await accessService.hasPermission(user, CREATE_FEATURE, 'default'), + ).toBe(false); + + // if project does not exist we also return false expect( await accessService.hasPermission(user, CREATE_FEATURE, 'some-project'), ).toBe(false); From f3aa63621a169bd49f101f40a98bf4d9a703a5dc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Fri, 15 Sep 2023 11:37:34 +0200 Subject: [PATCH 03/13] Add support for named permissions --- src/lib/db/access-store.ts | 90 +++++++++++-- src/lib/schema/role-schema.test.ts | 2 +- src/lib/schema/role-schema.ts | 4 +- src/lib/services/access-service.test.ts | 1 + src/lib/services/access-service.ts | 33 ++++- src/lib/types/stores/access-store.ts | 2 +- .../e2e/api/admin/api-token.auth.e2e.test.ts | 36 ++--- .../e2e/services/access-service.e2e.test.ts | 127 +++++------------- src/test/fixtures/fake-access-store.ts | 2 +- src/test/fixtures/no-logger.ts | 16 ++- 10 files changed, 177 insertions(+), 136 deletions(-) diff --git a/src/lib/db/access-store.ts b/src/lib/db/access-store.ts index 7008cc8caadc..6e07db8ff12d 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; @@ -53,6 +63,66 @@ export class AccessStore implements IAccessStore { private db: Db; + // for internal usage only + + private isIdPermissionRef = (permission: PermissionRef): boolean => { + return (permission as IdPermissionRef).id !== undefined; + }; + + private permissionNamesToIds = async ( + permissions: NamePermissionRef[], + ): Promise => { + if (permissions === undefined || permissions.length === 0) { + return []; + } + const permissionNames = permissions + .filter((p) => p.name !== undefined) + .map((p) => p.name); + + const rows = await this.db + .select('id as permissionId', 'permission') + .from(T.PERMISSIONS) + .whereIn('permission', permissionNames); + + return rows.map((row) => ({ + id: row.permissionId as number, + name: row.permission as string, + environment: permissions.find( + (p) => p.name === (row.permission as string), + )?.environment, + })); + }; + + private resolvePermissions = async ( + permissions: PermissionRef[], + ): Promise => { + // permissions without ids (just names) + const permissionsWithoutIds = permissions.filter( + (p) => !this.isIdPermissionRef(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.isIdPermissionRef(permission)) { + return permission as ResolvedPermission; + } else { + return idPermissionsFromNamed.find( + (p) => p.name === (permission as NamePermissionRef).name, + )!; + } + }); + }; + constructor(db: Db, eventBus: EventEmitter, getLogger: Function) { this.db = db; this.logger = getLogger('access-store.ts'); @@ -222,9 +292,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 +772,16 @@ export class AccessStore implements IAccessStore { async addPermissionsToRole( role_id: number, - permissions: string[], + permissions: Omit[], 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 + 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 05fb65b6d030..7d2ed31935b0 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 67fc1d5ee7d4..4946efc90631 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 6a4b72997ee0..688ce802799f 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 b7ab159e1c91..69291e4fc0cd 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; @@ -162,7 +162,14 @@ export class AccessService { try { const userP = await this.getPermissionsForUser(user); - return userP + this.logger.debug( + 'User permissions', + userP, + // userP.filter( + // (p) => p.permission === permissions.CREATE_FEATURE, + // ), + ); + const found = userP .filter( (p) => !p.project || @@ -175,11 +182,13 @@ export class AccessService { p.environment === environment || p.environment === ALL_ENVS, ) - .some( + .find( (p) => permissionsArray.includes(p.permission) || p.permission === ADMIN, ); + this.logger.debug('Permission', found); + return found !== undefined; } catch (e) { this.logger.error( `Error checking ${permissionLogInfo}, userId=${user.id} projectId=${projectId}`, @@ -197,6 +206,7 @@ export class AccessService { permission: p, })); } + this.logger.debug('Fetching permissions for user', user.id); return this.store.getPermissionsForUser(user.id); } @@ -237,6 +247,7 @@ export class AccessService { roleId: number, projectId: string, ): Promise { + this.logger.debug(`Adding user=${userId} to role=${roleId}`); return this.store.addUserToRole(userId, roleId, projectId); } @@ -404,9 +415,13 @@ export class AccessService { `ProjectId cannot be empty for permission=${permission}`, ); } + // return this.store.addPermissionsToRole(roleId, [ + // { name: permission, environment }, + // ]); + // console.log('addPermissionToRole', roleId, permission, environment); return this.store.addPermissionsToRole( roleId, - [permission], + [{ name: permission }], environment, ); } @@ -629,12 +644,18 @@ export class AccessService { const rolePermissions = role.permissions; const newRole = await this.roleStore.create(baseRole); if (rolePermissions) { + this.logger.debug( + `Adding permissions to ${roleType} ${newRole.id}`, + 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 +694,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 875e4519a985..966d60d6983e 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 fed77e8b5208..17047cbd5c27 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,11 +12,13 @@ import { UPDATE_CLIENT_API_TOKEN, } from '../../../../lib/types'; import { addDays } from 'date-fns'; +import { AccessService, UserService } from 'lib/services'; let stores; let db; beforeAll(async () => { + //getLogger.setVerbose(true); db = await dbInit('token_api_auth_serial', getLogger); stores = db.stores; }); @@ -175,33 +177,36 @@ 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, - }); + console.log( + 'Created role', + await accessService.getRole(createClientApiTokenRole.id), + ); + req.user = user; next(); }); }; @@ -225,12 +230,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 +245,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 0a59f5add72a..58f4e0b17f13 100644 --- a/src/test/e2e/services/access-service.e2e.test.ts +++ b/src/test/e2e/services/access-service.e2e.test.ts @@ -1,16 +1,14 @@ 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, -} from '../../../lib/types'; +import { ICreateGroupUserModel, IUnleashStores } from '../../../lib/types'; import FeatureToggleService from '../../../lib/services/feature-toggle-service'; import ProjectService from '../../../lib/services/project-service'; import { createTestConfig } from '../../config/test-config'; @@ -68,7 +66,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`, @@ -261,6 +259,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 () => { @@ -363,9 +380,12 @@ test('should remove CREATE_FEATURE on default environment', async () => { permissions.CREATE_FEATURE, '*', ); - expect( - await accessService.hasPermission(user, CREATE_FEATURE, 'default'), - ).toBe(true); + + // 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, @@ -373,11 +393,6 @@ test('should remove CREATE_FEATURE on default environment', async () => { '*', ); - expect( - await accessService.hasPermission(user, CREATE_FEATURE, 'default'), - ).toBe(false); - - // if project does not exist we also return false expect( await accessService.hasPermission(user, CREATE_FEATURE, 'some-project'), ).toBe(false); @@ -621,7 +636,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 +743,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 +982,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 +989,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 +1160,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 +1201,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 +1264,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 +1311,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 +1380,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 +1447,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 +1477,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 +1512,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 +1523,6 @@ test('remove user access should remove all project roles', async () => { { id: 13, name: 'UPDATE_PROJECT', - displayName: 'Can update projects', - type: 'project', }, ]); @@ -1593,14 +1554,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 +1565,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 +1625,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 +1667,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 +1713,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 +1724,6 @@ test('remove group access should remove all project roles', async () => { { id: 13, name: 'UPDATE_PROJECT', - displayName: 'Can update projects', - type: 'project', }, ]); @@ -1817,14 +1760,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 +1771,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', }, ]); diff --git a/src/test/fixtures/fake-access-store.ts b/src/test/fixtures/fake-access-store.ts index bc5242a57f69..e2341809efdf 100644 --- a/src/test/fixtures/fake-access-store.ts +++ b/src/test/fixtures/fake-access-store.ts @@ -181,7 +181,7 @@ class AccessStoreMock implements IAccessStore { addPermissionsToRole( role_id: number, permissions: string[], - projectId?: string, + 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 35c5df361e92..30f222ae9ac7 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; From 16c6aac5d5ef6c94c08d0fa1803afb464981533c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Fri, 15 Sep 2023 11:56:40 +0200 Subject: [PATCH 04/13] Remove some test logs --- src/lib/services/access-service.ts | 19 ++----------------- .../e2e/api/admin/api-token.auth.e2e.test.ts | 5 ----- 2 files changed, 2 insertions(+), 22 deletions(-) diff --git a/src/lib/services/access-service.ts b/src/lib/services/access-service.ts index ab9738e18f72..44992a051ec3 100644 --- a/src/lib/services/access-service.ts +++ b/src/lib/services/access-service.ts @@ -162,14 +162,7 @@ export class AccessService { try { const userP = await this.getPermissionsForUser(user); - this.logger.debug( - 'User permissions', - userP, - // userP.filter( - // (p) => p.permission === permissions.CREATE_FEATURE, - // ), - ); - const found = userP + return userP .filter( (p) => !p.project || @@ -182,13 +175,11 @@ export class AccessService { p.environment === environment || p.environment === ALL_ENVS, ) - .find( + .some( (p) => permissionsArray.includes(p.permission) || p.permission === ADMIN, ); - this.logger.debug('Permission', found); - return found !== undefined; } catch (e) { this.logger.error( `Error checking ${permissionLogInfo}, userId=${user.id} projectId=${projectId}`, @@ -206,7 +197,6 @@ export class AccessService { permission: p, })); } - this.logger.debug('Fetching permissions for user', user.id); return this.store.getPermissionsForUser(user.id); } @@ -247,7 +237,6 @@ export class AccessService { roleId: number, projectId: string, ): Promise { - this.logger.debug(`Adding user=${userId} to role=${roleId}`); return this.store.addUserToRole(userId, roleId, projectId); } @@ -644,10 +633,6 @@ export class AccessService { const rolePermissions = role.permissions; const newRole = await this.roleStore.create(baseRole); if (rolePermissions) { - this.logger.debug( - `Adding permissions to ${roleType} ${newRole.id}`, - rolePermissions, - ); if (roleType === CUSTOM_ROOT_ROLE_TYPE) { // this branch uses named permissions await this.store.addPermissionsToRole( 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 17047cbd5c27..97b711971959 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 @@ -18,7 +18,6 @@ let stores; let db; beforeAll(async () => { - //getLogger.setVerbose(true); db = await dbInit('token_api_auth_serial', getLogger); stores = db.stores; }); @@ -202,10 +201,6 @@ test('A role with only CREATE_PROJECT_API_TOKEN can create project tokens', asyn createClientApiTokenRole.id, 'default', ); - console.log( - 'Created role', - await accessService.getRole(createClientApiTokenRole.id), - ); req.user = user; next(); }); From dd54d0578960d452f14369cfd34ab674591e9b9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Fri, 15 Sep 2023 12:03:24 +0200 Subject: [PATCH 05/13] Fix incompatibility with fake store --- src/test/fixtures/fake-access-store.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/fixtures/fake-access-store.ts b/src/test/fixtures/fake-access-store.ts index 84ca2d888bd0..3daa2a66467f 100644 --- a/src/test/fixtures/fake-access-store.ts +++ b/src/test/fixtures/fake-access-store.ts @@ -180,7 +180,7 @@ class AccessStoreMock implements IAccessStore { addPermissionsToRole( role_id: number, - permissions: string[], + permissions: PermissionRef[], environment?: string, ): Promise { // do nothing for now From c7b12a7c2e4f62fd18c9280a34ed603b21fa2f88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Fri, 15 Sep 2023 12:11:07 +0200 Subject: [PATCH 06/13] Fix compilation issue --- src/lib/db/access-store.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/lib/db/access-store.ts b/src/lib/db/access-store.ts index 007586e440fb..bea64de9f204 100644 --- a/src/lib/db/access-store.ts +++ b/src/lib/db/access-store.ts @@ -772,10 +772,10 @@ export class AccessStore implements IAccessStore { async addPermissionsToRole( role_id: number, - permissions: Omit[], + permissions: PermissionRef[], environment?: string, ): Promise { - // no need to pass down the environment in this particular case + // no need to pass down the environment in this particular case because it'll be overriden const permissionsWithIds = await this.resolvePermissions(permissions); const newRoles = permissionsWithIds.map((p) => ({ From 45dacbc2b92a61ea4bbbc06eadbe3c46bbbc2d5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Fri, 15 Sep 2023 12:18:55 +0200 Subject: [PATCH 07/13] Adapt to new interface --- src/test/e2e/services/access-service.e2e.test.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/test/e2e/services/access-service.e2e.test.ts b/src/test/e2e/services/access-service.e2e.test.ts index 194395a0f37a..0656ef420b23 100644 --- a/src/test/e2e/services/access-service.e2e.test.ts +++ b/src/test/e2e/services/access-service.e2e.test.ts @@ -1855,8 +1855,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', }, ]); From 7a61c4ab411d924629a48968332ee08fd10d3052 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Fri, 15 Sep 2023 16:23:28 +0200 Subject: [PATCH 08/13] Add tests to resolve permissions --- src/lib/db/access-store.test.ts | 112 ++++++++++++++++++++++++++++++++ src/lib/db/access-store.ts | 21 +++--- 2 files changed, 125 insertions(+), 8 deletions(-) create mode 100644 src/lib/db/access-store.test.ts diff --git a/src/lib/db/access-store.test.ts b/src/lib/db/access-store.test.ts new file mode 100644 index 000000000000..0bccc675397e --- /dev/null +++ b/src/lib/db/access-store.test.ts @@ -0,0 +1,112 @@ +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('feature_strategy_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( + undefined as unknown 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 f470743f4852..66f6ecb6dcac 100644 --- a/src/lib/db/access-store.ts +++ b/src/lib/db/access-store.ts @@ -80,22 +80,27 @@ export class AccessStore implements IAccessStore { .map((p) => p.name); const rows = await this.db - .select('id as permissionId', 'permission') + .select('id', 'permission') .from(T.PERMISSIONS) .whereIn('permission', permissionNames); - return rows.map((row) => ({ - id: row.permissionId as number, - name: row.permission as string, - environment: permissions.find( - (p) => p.name === (row.permission as string), - )?.environment, + const rowByPermissionName = rows.reduce((acc, row) => { + acc[row.permission] = row; + return acc; + }, {} as Map); + + return permissions.map((permission) => ({ + id: rowByPermissionName[permission.name].id, + ...permission, })); }; - private resolvePermissions = async ( + resolvePermissions = async ( permissions: PermissionRef[], ): Promise => { + if (permissions === undefined || permissions.length === 0) { + return []; + } // permissions without ids (just names) const permissionsWithoutIds = permissions.filter( (p) => !this.isIdPermissionRef(p), From 67fccedfa47bb22afc6496c6dde73179b7ccc9e7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Fri, 15 Sep 2023 19:47:16 +0200 Subject: [PATCH 09/13] Remove unnecessary comment --- src/lib/services/access-service.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/lib/services/access-service.ts b/src/lib/services/access-service.ts index 44992a051ec3..46a32cd59d8f 100644 --- a/src/lib/services/access-service.ts +++ b/src/lib/services/access-service.ts @@ -404,10 +404,6 @@ export class AccessService { `ProjectId cannot be empty for permission=${permission}`, ); } - // return this.store.addPermissionsToRole(roleId, [ - // { name: permission, environment }, - // ]); - // console.log('addPermissionToRole', roleId, permission, environment); return this.store.addPermissionsToRole( roleId, [{ name: permission }], From ca01f42ab3d2ddc4b7f7937fc3a0e220d968aa5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Tue, 19 Sep 2023 10:08:38 +0200 Subject: [PATCH 10/13] Update src/lib/db/access-store.test.ts --- src/lib/db/access-store.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/db/access-store.test.ts b/src/lib/db/access-store.test.ts index 0bccc675397e..e7c302bc2f6e 100644 --- a/src/lib/db/access-store.test.ts +++ b/src/lib/db/access-store.test.ts @@ -35,7 +35,7 @@ test('resolvePermissions returns empty list if undefined', async () => { test('resolvePermissions returns empty list if empty list', async () => { const access = db.stores.accessStore as AccessStore; const result = await access.resolvePermissions( - undefined as unknown as PermissionRef[], + [] as PermissionRef[], ); expect(result).toStrictEqual([]); }); From 7b6596e3a1cd0d599b3ad2d17955a6bfbf507be9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Tue, 19 Sep 2023 10:08:45 +0200 Subject: [PATCH 11/13] Update src/lib/db/access-store.test.ts --- src/lib/db/access-store.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/lib/db/access-store.test.ts b/src/lib/db/access-store.test.ts index e7c302bc2f6e..be2567e06fc9 100644 --- a/src/lib/db/access-store.test.ts +++ b/src/lib/db/access-store.test.ts @@ -6,7 +6,7 @@ import { AccessStore } from './access-store'; let db; beforeAll(async () => { - db = await dbInit('feature_strategy_store_serial', getLogger); + db = await dbInit('access_store_serial', getLogger); }); afterAll(async () => { From 84a17f93aff99167c4d972d789665b32b93ec1c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Tue, 19 Sep 2023 10:23:07 +0200 Subject: [PATCH 12/13] lint --- src/lib/db/access-store.test.ts | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/lib/db/access-store.test.ts b/src/lib/db/access-store.test.ts index be2567e06fc9..18b1228fe9a3 100644 --- a/src/lib/db/access-store.test.ts +++ b/src/lib/db/access-store.test.ts @@ -34,9 +34,7 @@ test('resolvePermissions returns empty list if undefined', async () => { test('resolvePermissions returns empty list if empty list', async () => { const access = db.stores.accessStore as AccessStore; - const result = await access.resolvePermissions( - [] as PermissionRef[], - ); + const result = await access.resolvePermissions([] as PermissionRef[]); expect(result).toStrictEqual([]); }); From d5c34022f614c212228d8362b2eaa4f3d90335fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Tue, 19 Sep 2023 10:34:11 +0200 Subject: [PATCH 13/13] Add timer to transform permission names to ids --- src/lib/db/access-store.ts | 42 +++++++++++++++++++++----------------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/src/lib/db/access-store.ts b/src/lib/db/access-store.ts index 66f6ecb6dcac..072eee2673cb 100644 --- a/src/lib/db/access-store.ts +++ b/src/lib/db/access-store.ts @@ -63,22 +63,33 @@ export class AccessStore implements IAccessStore { private db: Db; - // for internal usage only + constructor(db: Db, eventBus: EventEmitter, getLogger: Function) { + this.db = db; + this.logger = getLogger('access-store.ts'); + this.timer = (action: string) => + metricsHelper.wrapTimer(eventBus, DB_TIME, { + store: 'access-store', + action, + }); + } - private isIdPermissionRef = (permission: PermissionRef): boolean => { + private permissionHasId = (permission: PermissionRef): boolean => { return (permission as IdPermissionRef).id !== undefined; }; private permissionNamesToIds = async ( permissions: NamePermissionRef[], ): Promise => { - if (permissions === undefined || permissions.length === 0) { - return []; - } - const permissionNames = permissions + 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) @@ -89,10 +100,13 @@ export class AccessStore implements IAccessStore { return acc; }, {} as Map); - return permissions.map((permission) => ({ + const permissionsWithIds = permissions.map((permission) => ({ id: rowByPermissionName[permission.name].id, ...permission, })); + + stopTimer(); + return permissionsWithIds; }; resolvePermissions = async ( @@ -103,7 +117,7 @@ export class AccessStore implements IAccessStore { } // permissions without ids (just names) const permissionsWithoutIds = permissions.filter( - (p) => !this.isIdPermissionRef(p), + (p) => !this.permissionHasId(p), ) as NamePermissionRef[]; const idPermissionsFromNamed = await this.permissionNamesToIds( permissionsWithoutIds, @@ -118,7 +132,7 @@ export class AccessStore implements IAccessStore { } // some permissions have ids, some don't (should not happen!) return permissions.map((permission) => { - if (this.isIdPermissionRef(permission)) { + if (this.permissionHasId(permission)) { return permission as ResolvedPermission; } else { return idPermissionsFromNamed.find( @@ -128,16 +142,6 @@ export class AccessStore implements IAccessStore { }); }; - constructor(db: Db, eventBus: EventEmitter, getLogger: Function) { - this.db = db; - this.logger = getLogger('access-store.ts'); - this.timer = (action: string) => - metricsHelper.wrapTimer(eventBus, DB_TIME, { - store: 'access-store', - action, - }); - } - async delete(key: number): Promise { await this.db(T.ROLES).where({ id: key }).del(); }