Skip to content

Commit

Permalink
fix(rbac): add additional validation for permission policies
Browse files Browse the repository at this point in the history
  • Loading branch information
@dzemanov
dzemanov committed Jul 31, 2024
1 parent eed06dc commit 8e0c464
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 18 deletions.
36 changes: 18 additions & 18 deletions plugins/rbac-backend/src/service/policies-rest-api.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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);
Expand All @@ -1285,7 +1285,7 @@ describe('REST policies api', () => {
newPolicy: [
{
permission: 'policy-entity',
policy: 'write',
policy: 'create',
effect: 'allow',
},
],
Expand Down Expand Up @@ -1344,7 +1344,7 @@ describe('REST policies api', () => {
newPolicy: [
{
permission: 'policy-entity',
policy: 'write',
policy: 'create',
effect: 'allow',
},
],
Expand All @@ -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`,
});
});

Expand Down Expand Up @@ -1465,7 +1465,7 @@ describe('REST policies api', () => {
mockEnforcer.hasPolicy = jest
.fn()
.mockImplementation(async (...param: string[]): Promise<boolean> => {
if (param[2] === 'write') {
if (param[2] === 'create') {
return false;
}
return true;
Expand All @@ -1489,7 +1489,7 @@ describe('REST policies api', () => {
newPolicy: [
{
permission: 'policy-entity',
policy: 'write',
policy: 'create',
effect: 'allow',
},
],
Expand All @@ -1506,7 +1506,7 @@ describe('REST policies api', () => {
mockEnforcer.hasPolicy = jest
.fn()
.mockImplementation(async (...param: string[]): Promise<boolean> => {
if (param[2] === 'write') {
if (param[2] === 'create') {
return false;
}
return true;
Expand All @@ -1532,7 +1532,7 @@ describe('REST policies api', () => {
newPolicy: [
{
permission: 'policy-entity',
policy: 'write',
policy: 'create',
effect: 'allow',
},
],
Expand All @@ -1549,7 +1549,7 @@ describe('REST policies api', () => {
mockEnforcer.hasPolicy = jest
.fn()
.mockImplementation(async (...param: string[]): Promise<boolean> => {
if (param[2] === 'write') {
if (param[2] === 'create') {
return false;
}
return true;
Expand All @@ -1569,7 +1569,7 @@ describe('REST policies api', () => {
newPolicy: [
{
permission: 'policy-entity',
policy: 'write',
policy: 'create',
effect: 'allow',
},
],
Expand All @@ -1582,7 +1582,7 @@ describe('REST policies api', () => {
mockEnforcer.hasPolicy = jest
.fn()
.mockImplementation(async (...param: string[]): Promise<boolean> => {
if (param[2] === 'write') {
if (param[2] === 'create') {
return false;
}
return true;
Expand All @@ -1606,7 +1606,7 @@ describe('REST policies api', () => {
newPolicy: [
{
permission: 'policy-entity',
policy: 'write',
policy: 'create',
effect: 'allow',
},
{
Expand All @@ -1628,7 +1628,7 @@ describe('REST policies api', () => {
mockEnforcer.hasPolicy = jest
.fn()
.mockImplementation(async (...param: string[]): Promise<boolean> => {
if (param[2] === 'write') {
if (param[2] === 'update') {
return false;
}
return true;
Expand All @@ -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',
},
],
Expand All @@ -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`,
});
});

Expand Down Expand Up @@ -3649,7 +3649,7 @@ describe('REST policies api', () => {
mockEnforcer.hasPolicy = jest
.fn()
.mockImplementation(async (...param: string[]): Promise<boolean> => {
if (param[2] === 'write') {
if (param[2] === 'create') {
return false;
}
return true;
Expand Down
14 changes: 14 additions & 0 deletions plugins/rbac-backend/src/validation/policies-validation.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Expand Down
12 changes: 12 additions & 0 deletions plugins/rbac-backend/src/validation/policies-validation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { AuthorizeResult } from '@backstage/plugin-permission-common';
import { Enforcer } from 'casbin';

import {
PermissionAction,
Role,
RoleBasedPolicy,
Source,
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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() ||
Expand Down

0 comments on commit 8e0c464

Please sign in to comment.