From 8e0c46428a60bdd3bf1c67853e297cdd37d719a2 Mon Sep 17 00:00:00 2001 From: Dominika Zemanovicova Date: Thu, 18 Jul 2024 16:27:47 +0200 Subject: [PATCH] fix(rbac): add additional validation for permission policies --- .../src/service/policies-rest-api.test.ts | 36 +++++++++---------- .../validation/policies-validation.test.ts | 14 ++++++++ .../src/validation/policies-validation.ts | 12 +++++++ 3 files changed, 44 insertions(+), 18 deletions(-) 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..4bd5e34b99 100644 --- a/plugins/rbac-backend/src/validation/policies-validation.ts +++ b/plugins/rbac-backend/src/validation/policies-validation.ts @@ -5,6 +5,7 @@ import { AuthorizeResult } from '@backstage/plugin-permission-common'; import { Enforcer } from 'casbin'; import { + PermissionAction, Role, RoleBasedPolicy, Source, @@ -58,6 +59,13 @@ export function validatePolicy(policy: RoleBasedPolicy): Error | undefined { if (!policy.policy) { return new Error(`'policy' field must not be empty`); + } else if (!isValidPermissionAction(policy.policy)) { + const validOptions = ['create', 'read', 'update', 'delete', 'use'].join( + ', ', + ); + return new Error( + `'policy' has invalid value: '${policy.policy}'. It should be one of: ${validOptions}`, + ); } if (!policy.effect) { @@ -96,6 +104,10 @@ export function validateRole(role: Role): Error | undefined { return undefined; } +function isValidPermissionAction(action: string): action is PermissionAction { + return ['create', 'read', 'update', 'delete', 'use'].includes(action); +} + function isValidEffectValue(effect: string): boolean { return ( effect === AuthorizeResult.ALLOW.toLocaleLowerCase() ||