chore: Improve access service iter 2 (#4779)

## About the changes
In #4689 I forgot to add backward
compatibility for a public method that was being used in Enterprise.

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,25 @@ export class AccessStore implements IAccessStore {
 
     async addPermissionsToRole(
         role_id: number,
-        permissions: string[],
+        permissions: PermissionRef[] | string[],
         environment?: string,
     ): Promise<void> {
-        const rows = await this.db
-            .select('id as permissionId')
-            .from<number>(T.PERMISSIONS)
-            .whereIn('permission', permissions);
+        const permissionsAsRefs = (permissions ?? []).map((p) => {
+            if (typeof p === 'string') {
+                return { name: p };
+            } else {
+                return p;
+            }
+        });
+        // no need to pass down the environment in this particular case because it'll be overriden
+        const permissionsWithIds = await this.resolvePermissions(
+            permissionsAsRefs,
+        );
 
-        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[] | string[],
         environment?: string,
     ): Promise<void>;