chore: improve access service (#4689)

## 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:

https://github.com/Unleash/unleash/blob/78273e4ff30d7012aa91f6549115587dc31a6e0b/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.

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);
+    },
+);

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<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);

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]',
         );
     }
 });

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

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',
             },
         ],

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(

src/lib/types/stores/access-store.ts

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