fix(rbac): add additional validation for permission policies (#1908)

* fix(rbac): add additional validation for permission policies

* fix(rbac): remove duplication permission action values (#1939)

Signed-off-by: Oleksandr Andriienko <[email protected]>

---------

Signed-off-by: Oleksandr Andriienko <[email protected]>
Co-authored-by: Oleksandr Andriienko <[email protected]>

Author: dzemanov

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<boolean> => {
-          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<boolean> => {
-          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<boolean> => {
-          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<boolean> => {
-          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<boolean> => {
-          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<boolean> => {
-          if (param[2] === 'write') {
+          if (param[2] === 'create') {
             return false;
           }
           return true;

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',

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) {

plugins/rbac-common/src/types.ts

@@ -73,11 +73,24 @@ export type NonEmptyArray<T> = [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;