diff --git a/plugins/rbac-backend/src/service/policies-rest-api.test.ts b/plugins/rbac-backend/src/service/policies-rest-api.test.ts index 443ada9111..62e12a1de8 100644 --- a/plugins/rbac-backend/src/service/policies-rest-api.test.ts +++ b/plugins/rbac-backend/src/service/policies-rest-api.test.ts @@ -1240,7 +1240,7 @@ describe('REST policies api', () => { effect: 'allow', }, ], - newPolicy: [{ permission: 'policy-entity', policy: 'write' }], + newPolicy: [{ permission: 'policy-entity', policy: 'create' }], }); expect(result.statusCode).toEqual(400); @@ -1261,7 +1261,7 @@ describe('REST policies api', () => { effect: 'unknown', }, ], - newPolicy: [{ permission: 'policy-entity', policy: 'write' }], + newPolicy: [{ permission: 'policy-entity', policy: 'create' }], }); expect(result.statusCode).toEqual(400); @@ -1285,7 +1285,7 @@ describe('REST policies api', () => { newPolicy: [ { permission: 'policy-entity', - policy: 'write', + policy: 'create', effect: 'allow', }, ], @@ -1344,7 +1344,7 @@ describe('REST policies api', () => { newPolicy: [ { permission: 'policy-entity', - policy: 'write', + policy: 'create', effect: 'allow', }, ], @@ -1353,7 +1353,7 @@ describe('REST policies api', () => { expect(result.statusCode).toEqual(409); expect(result.body.error).toEqual({ name: 'ConflictError', - message: `Policy '[user:default/permission_admin, policy-entity, write, allow]' has been already stored`, + message: `Policy '[user:default/permission_admin, policy-entity, create, allow]' has been already stored`, }); }); @@ -1465,7 +1465,7 @@ describe('REST policies api', () => { mockEnforcer.hasPolicy = jest .fn() .mockImplementation(async (...param: string[]): Promise => { - if (param[2] === 'write') { + if (param[2] === 'create') { return false; } return true; @@ -1489,7 +1489,7 @@ describe('REST policies api', () => { newPolicy: [ { permission: 'policy-entity', - policy: 'write', + policy: 'create', effect: 'allow', }, ], @@ -1506,7 +1506,7 @@ describe('REST policies api', () => { mockEnforcer.hasPolicy = jest .fn() .mockImplementation(async (...param: string[]): Promise => { - if (param[2] === 'write') { + if (param[2] === 'create') { return false; } return true; @@ -1532,7 +1532,7 @@ describe('REST policies api', () => { newPolicy: [ { permission: 'policy-entity', - policy: 'write', + policy: 'create', effect: 'allow', }, ], @@ -1549,7 +1549,7 @@ describe('REST policies api', () => { mockEnforcer.hasPolicy = jest .fn() .mockImplementation(async (...param: string[]): Promise => { - if (param[2] === 'write') { + if (param[2] === 'create') { return false; } return true; @@ -1569,7 +1569,7 @@ describe('REST policies api', () => { newPolicy: [ { permission: 'policy-entity', - policy: 'write', + policy: 'create', effect: 'allow', }, ], @@ -1582,7 +1582,7 @@ describe('REST policies api', () => { mockEnforcer.hasPolicy = jest .fn() .mockImplementation(async (...param: string[]): Promise => { - if (param[2] === 'write') { + if (param[2] === 'create') { return false; } return true; @@ -1606,7 +1606,7 @@ describe('REST policies api', () => { newPolicy: [ { permission: 'policy-entity', - policy: 'write', + policy: 'create', effect: 'allow', }, { @@ -1628,7 +1628,7 @@ describe('REST policies api', () => { mockEnforcer.hasPolicy = jest .fn() .mockImplementation(async (...param: string[]): Promise => { - if (param[2] === 'write') { + if (param[2] === 'update') { return false; } return true; @@ -1652,12 +1652,12 @@ describe('REST policies api', () => { newPolicy: [ { permission: 'policy-entity', - policy: 'write', + policy: 'update', effect: 'allow', }, { permission: 'policy-entity', - policy: 'write', + policy: 'update', effect: 'allow', }, ], @@ -1666,7 +1666,7 @@ describe('REST policies api', () => { expect(result.statusCode).toBe(409); expect(result.body.error).toEqual({ name: 'ConflictError', - message: `Duplicate polices found; user:default/permission_admin, policy-entity, write, allow is a duplicate`, + message: `Duplicate polices found; user:default/permission_admin, policy-entity, update, allow is a duplicate`, }); }); @@ -3649,7 +3649,7 @@ describe('REST policies api', () => { mockEnforcer.hasPolicy = jest .fn() .mockImplementation(async (...param: string[]): Promise => { - if (param[2] === 'write') { + if (param[2] === 'create') { return false; } return true; diff --git a/plugins/rbac-backend/src/validation/policies-validation.test.ts b/plugins/rbac-backend/src/validation/policies-validation.test.ts index 6cc668a39d..1f8f6cfb4b 100644 --- a/plugins/rbac-backend/src/validation/policies-validation.test.ts +++ b/plugins/rbac-backend/src/validation/policies-validation.test.ts @@ -68,6 +68,20 @@ describe('rest data validation', () => { expect(err?.message).toEqual(`'policy' field must not be empty`); }); + it('should return an error when policy has an invalid value', () => { + const policy: RoleBasedPolicy = { + entityReference: 'user:default/guest', + permission: 'catalog-entity', + policy: 'invalid-policy', + effect: 'allow', + }; + const err = validatePolicy(policy); + expect(err).toBeTruthy(); + expect(err?.message).toEqual( + `'policy' has invalid value: 'invalid-policy'. It should be one of: create, read, update, delete, use`, + ); + }); + it('should return an error when effect is empty', () => { const policy: RoleBasedPolicy = { entityReference: 'user:default/guest', diff --git a/plugins/rbac-backend/src/validation/policies-validation.ts b/plugins/rbac-backend/src/validation/policies-validation.ts index b755f74b78..fac5b67499 100644 --- a/plugins/rbac-backend/src/validation/policies-validation.ts +++ b/plugins/rbac-backend/src/validation/policies-validation.ts @@ -5,6 +5,8 @@ import { AuthorizeResult } from '@backstage/plugin-permission-common'; import { Enforcer } from 'casbin'; import { + isValidPermissionAction, + PermissionActionValues, Role, RoleBasedPolicy, Source, @@ -58,6 +60,10 @@ export function validatePolicy(policy: RoleBasedPolicy): Error | undefined { if (!policy.policy) { return new Error(`'policy' field must not be empty`); + } else if (!isValidPermissionAction(policy.policy)) { + return new Error( + `'policy' has invalid value: '${policy.policy}'. It should be one of: ${PermissionActionValues.join(', ')}`, + ); } if (!policy.effect) { diff --git a/plugins/rbac-common/src/types.ts b/plugins/rbac-common/src/types.ts index 51c096d58c..952fa34043 100644 --- a/plugins/rbac-common/src/types.ts +++ b/plugins/rbac-common/src/types.ts @@ -73,11 +73,24 @@ export type NonEmptyArray = [T, ...T[]]; // Permission framework attributes action has values: 'create' | 'read' | 'update' | 'delete' | undefined. // But we are introducing an action named "use" when action does not exist('undefined') to avoid // a more complicated model with multiple policy and request shapes. -export type PermissionAction = 'create' | 'read' | 'update' | 'delete' | 'use'; +export const PermissionActionValues = [ + 'create', + 'read', + 'update', + 'delete', + 'use', +] as const; +export type PermissionAction = (typeof PermissionActionValues)[number]; export const toPermissionAction = ( attr: PermissionAttributes, ): PermissionAction => attr.action ?? 'use'; +export function isValidPermissionAction( + action: string, +): action is PermissionAction { + return (PermissionActionValues as readonly string[]).includes(action); +} + export type PermissionInfo = { name: string; action: PermissionAction;