Skip to content

Commit e731798

Browse files
dzemanovdzemanov
committed
fix(rbac): add additional validation for permission policies
1 parent 1f6b6c3 commit e731798

File tree

3 files changed

+44
-18
lines changed

3 files changed

+44
-18
lines changed

‎plugins/rbac-backend/src/service/policies-rest-api.test.ts

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -1240,7 +1240,7 @@ describe('REST policies api', () => {
12401240
effect: 'allow',
12411241
},
12421242
],
1243-
newPolicy: [{ permission: 'policy-entity', policy: 'write' }],
1243+
newPolicy: [{ permission: 'policy-entity', policy: 'create' }],
12441244
});
12451245

12461246
expect(result.statusCode).toEqual(400);
@@ -1261,7 +1261,7 @@ describe('REST policies api', () => {
12611261
effect: 'unknown',
12621262
},
12631263
],
1264-
newPolicy: [{ permission: 'policy-entity', policy: 'write' }],
1264+
newPolicy: [{ permission: 'policy-entity', policy: 'create' }],
12651265
});
12661266

12671267
expect(result.statusCode).toEqual(400);
@@ -1285,7 +1285,7 @@ describe('REST policies api', () => {
12851285
newPolicy: [
12861286
{
12871287
permission: 'policy-entity',
1288-
policy: 'write',
1288+
policy: 'create',
12891289
effect: 'allow',
12901290
},
12911291
],
@@ -1344,7 +1344,7 @@ describe('REST policies api', () => {
13441344
newPolicy: [
13451345
{
13461346
permission: 'policy-entity',
1347-
policy: 'write',
1347+
policy: 'create',
13481348
effect: 'allow',
13491349
},
13501350
],
@@ -1353,7 +1353,7 @@ describe('REST policies api', () => {
13531353
expect(result.statusCode).toEqual(409);
13541354
expect(result.body.error).toEqual({
13551355
name: 'ConflictError',
1356-
message: `Policy '[user:default/permission_admin, policy-entity, write, allow]' has been already stored`,
1356+
message: `Policy '[user:default/permission_admin, policy-entity, create, allow]' has been already stored`,
13571357
});
13581358
});
13591359

@@ -1465,7 +1465,7 @@ describe('REST policies api', () => {
14651465
mockEnforcer.hasPolicy = jest
14661466
.fn()
14671467
.mockImplementation(async (...param: string[]): Promise<boolean> => {
1468-
if (param[2] === 'write') {
1468+
if (param[2] === 'create') {
14691469
return false;
14701470
}
14711471
return true;
@@ -1489,7 +1489,7 @@ describe('REST policies api', () => {
14891489
newPolicy: [
14901490
{
14911491
permission: 'policy-entity',
1492-
policy: 'write',
1492+
policy: 'create',
14931493
effect: 'allow',
14941494
},
14951495
],
@@ -1506,7 +1506,7 @@ describe('REST policies api', () => {
15061506
mockEnforcer.hasPolicy = jest
15071507
.fn()
15081508
.mockImplementation(async (...param: string[]): Promise<boolean> => {
1509-
if (param[2] === 'write') {
1509+
if (param[2] === 'create') {
15101510
return false;
15111511
}
15121512
return true;
@@ -1532,7 +1532,7 @@ describe('REST policies api', () => {
15321532
newPolicy: [
15331533
{
15341534
permission: 'policy-entity',
1535-
policy: 'write',
1535+
policy: 'create',
15361536
effect: 'allow',
15371537
},
15381538
],
@@ -1549,7 +1549,7 @@ describe('REST policies api', () => {
15491549
mockEnforcer.hasPolicy = jest
15501550
.fn()
15511551
.mockImplementation(async (...param: string[]): Promise<boolean> => {
1552-
if (param[2] === 'write') {
1552+
if (param[2] === 'create') {
15531553
return false;
15541554
}
15551555
return true;
@@ -1569,7 +1569,7 @@ describe('REST policies api', () => {
15691569
newPolicy: [
15701570
{
15711571
permission: 'policy-entity',
1572-
policy: 'write',
1572+
policy: 'create',
15731573
effect: 'allow',
15741574
},
15751575
],
@@ -1582,7 +1582,7 @@ describe('REST policies api', () => {
15821582
mockEnforcer.hasPolicy = jest
15831583
.fn()
15841584
.mockImplementation(async (...param: string[]): Promise<boolean> => {
1585-
if (param[2] === 'write') {
1585+
if (param[2] === 'create') {
15861586
return false;
15871587
}
15881588
return true;
@@ -1606,7 +1606,7 @@ describe('REST policies api', () => {
16061606
newPolicy: [
16071607
{
16081608
permission: 'policy-entity',
1609-
policy: 'write',
1609+
policy: 'create',
16101610
effect: 'allow',
16111611
},
16121612
{
@@ -1628,7 +1628,7 @@ describe('REST policies api', () => {
16281628
mockEnforcer.hasPolicy = jest
16291629
.fn()
16301630
.mockImplementation(async (...param: string[]): Promise<boolean> => {
1631-
if (param[2] === 'write') {
1631+
if (param[2] === 'update') {
16321632
return false;
16331633
}
16341634
return true;
@@ -1652,12 +1652,12 @@ describe('REST policies api', () => {
16521652
newPolicy: [
16531653
{
16541654
permission: 'policy-entity',
1655-
policy: 'write',
1655+
policy: 'update',
16561656
effect: 'allow',
16571657
},
16581658
{
16591659
permission: 'policy-entity',
1660-
policy: 'write',
1660+
policy: 'update',
16611661
effect: 'allow',
16621662
},
16631663
],
@@ -1666,7 +1666,7 @@ describe('REST policies api', () => {
16661666
expect(result.statusCode).toBe(409);
16671667
expect(result.body.error).toEqual({
16681668
name: 'ConflictError',
1669-
message: `Duplicate polices found; user:default/permission_admin, policy-entity, write, allow is a duplicate`,
1669+
message: `Duplicate polices found; user:default/permission_admin, policy-entity, update, allow is a duplicate`,
16701670
});
16711671
});
16721672

@@ -3649,7 +3649,7 @@ describe('REST policies api', () => {
36493649
mockEnforcer.hasPolicy = jest
36503650
.fn()
36513651
.mockImplementation(async (...param: string[]): Promise<boolean> => {
3652-
if (param[2] === 'write') {
3652+
if (param[2] === 'create') {
36533653
return false;
36543654
}
36553655
return true;

‎plugins/rbac-backend/src/validation/policies-validation.test.ts

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,20 @@ describe('rest data validation', () => {
6868
expect(err?.message).toEqual(`'policy' field must not be empty`);
6969
});
7070

71+
it('should return an error when policy has an invalid value', () => {
72+
const policy: RoleBasedPolicy = {
73+
entityReference: 'user:default/guest',
74+
permission: 'catalog-entity',
75+
policy: 'invalid-policy',
76+
effect: 'allow',
77+
};
78+
const err = validatePolicy(policy);
79+
expect(err).toBeTruthy();
80+
expect(err?.message).toEqual(
81+
`'policy' has invalid value: 'invalid-policy'. It should be one of: create, read, update, delete, use`,
82+
);
83+
});
84+
7185
it('should return an error when effect is empty', () => {
7286
const policy: RoleBasedPolicy = {
7387
entityReference: 'user:default/guest',

‎plugins/rbac-backend/src/validation/policies-validation.ts

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import { AuthorizeResult } from '@backstage/plugin-permission-common';
55
import { Enforcer } from 'casbin';
66

77
import {
8+
PermissionAction,
89
Role,
910
RoleBasedPolicy,
1011
Source,
@@ -58,6 +59,13 @@ export function validatePolicy(policy: RoleBasedPolicy): Error | undefined {
5859

5960
if (!policy.policy) {
6061
return new Error(`'policy' field must not be empty`);
62+
} else if (!isValidPermissionAction(policy.policy)) {
63+
const validOptions = ['create', 'read', 'update', 'delete', 'use'].join(
64+
', ',
65+
);
66+
return new Error(
67+
`'policy' has invalid value: '${policy.policy}'. It should be one of: ${validOptions}`,
68+
);
6169
}
6270

6371
if (!policy.effect) {
@@ -96,6 +104,10 @@ export function validateRole(role: Role): Error | undefined {
96104
return undefined;
97105
}
98106

107+
function isValidPermissionAction(action: string): action is PermissionAction {
108+
return ['create', 'read', 'update', 'delete', 'use'].includes(action);
109+
}
110+
99111
function isValidEffectValue(effect: string): boolean {
100112
return (
101113
effect === AuthorizeResult.ALLOW.toLocaleLowerCase() ||

0 commit comments

Comments
 (0)