feat(rbac): improve conditional policy validation

janus-idp · May 16, 2024 · 4c89103 · 4c89103
1 parent 5e60875
commit 4c89103
Show file tree

Hide file tree

Showing 9 changed files with 368 additions and 57 deletions.
diff --git a/plugins/rbac-backend/docs/apis.md b/plugins/rbac-backend/docs/apis.md
@@ -705,6 +705,34 @@ Based on the above schema:
 }
 ```
 
+**NOTE**: We do not support the ability to run conditions in parallel during creation. An example can be found below, notice that `anyOf` and `not` are on the same level. Consider making separate condition requests, or nest your conditions based on the available criteria.
+
+```json
+{
+  "anyOf": [
+    {
+      "rule": "IS_ENTITY_OWNER",
+      "resourceType": "catalog-entity",
+      "params": {
+        "claims": ["group:default/team-a"]
+      }
+    },
+    {
+      "rule": "IS_ENTITY_KIND",
+      "resourceType": "catalog-entity",
+      "params": {
+        "kinds": ["Group"]
+      }
+    }
+  ],
+  "not": {
+    "rule": "IS_ENTITY_KIND",
+    "resourceType": "catalog-entity",
+    "params": { "kinds": ["Api"] }
+  }
+}
+```
+
 To utilize this condition to the RBAC REST api you need to wrap it with more info:
 
 ```json

diff --git a/plugins/rbac-backend/src/file-permissions/csv-file-watcher.ts b/plugins/rbac-backend/src/file-permissions/csv-file-watcher.ts
@@ -19,7 +19,7 @@ import {
   checkForDuplicatePolicies,
   validateGroupingPolicy,
   validatePolicy,
-} from '../service/policies-validation';
+} from '../validation/policies-validation';
 
 export const CSV_PERMISSION_POLICY_FILE_AUTHOR = 'csv permission policy file';
 

diff --git a/plugins/rbac-backend/src/service/permission-policy.ts b/plugins/rbac-backend/src/service/permission-policy.ts
@@ -29,8 +29,8 @@ import {
 } from '../database/role-metadata';
 import { CSVFileWatcher } from '../file-permissions/csv-file-watcher';
 import { metadataStringToPolicy, removeTheDifference } from '../helper';
+import { validateEntityReference } from '../validation/policies-validation';
 import { EnforcerDelegate } from './enforcer-delegate';
-import { validateEntityReference } from './policies-validation';
 
 export const ADMIN_ROLE_NAME = 'role:default/rbac_admin';
 export const ADMIN_ROLE_AUTHOR = 'application configuration';

diff --git a/plugins/rbac-backend/src/service/policies-rest-api.test.ts b/plugins/rbac-backend/src/service/policies-rest-api.test.ts
@@ -148,7 +148,7 @@ const conditionalStorage = {
 };
 
 const validateRoleConditionMock = jest.fn().mockImplementation();
-jest.mock('./condition-validation', () => {
+jest.mock('../validation/condition-validation', () => {
   return {
     validateRoleCondition: jest
       .fn()

diff --git a/plugins/rbac-backend/src/service/policies-rest-api.ts b/plugins/rbac-backend/src/service/policies-rest-api.ts
@@ -52,14 +52,14 @@ import {
   RoleMetadataStorage,
 } from '../database/role-metadata';
 import { deepSortedEqual, isPermissionAction, policyToString } from '../helper';
-import { validateRoleCondition } from './condition-validation';
-import { EnforcerDelegate } from './enforcer-delegate';
-import { PluginPermissionMetadataCollector } from './plugin-endpoints';
+import { validateRoleCondition } from '../validation/condition-validation';
 import {
   validateEntityReference,
   validatePolicy,
   validateRole,
-} from './policies-validation';
+} from '../validation/policies-validation';
+import { EnforcerDelegate } from './enforcer-delegate';
+import { PluginPermissionMetadataCollector } from './plugin-endpoints';
 
 export class PoliciesServer {
   constructor(

diff --git a/.../src/service/condition.validation.test.ts → ...c/validation/condition-validation.test.ts b/.../src/service/condition.validation.test.ts → ...c/validation/condition-validation.test.ts
@@ -351,6 +351,7 @@ describe('condition-validation', () => {
       } catch (err) {
         unexpectedErr = err;
       }
+
       expect(unexpectedErr).toBeUndefined();
     });
   });
@@ -724,4 +725,203 @@ describe('condition-validation', () => {
       expect(unexpectedErr).toBeUndefined();
     });
   });
+
+  describe('complex conditions', () => {
+    it('should fail validation of role-condition.conditions in parallel with condition rule', () => {
+      const condition: RoleConditionalPolicyDecision<PermissionAction> = {
+        id: 1,
+        pluginId: 'catalog',
+        resourceType: 'catalog-entity',
+        roleEntityRef: 'role:default/test',
+        result: AuthorizeResult.CONDITIONAL,
+        permissionMapping: ['read'],
+        conditions: {
+          allOf: [
+            {
+              rule: 'IS_ENTITY_OWNER',
+              resourceType: 'catalog-entity',
+              params: {
+                claims: ['user:default/logarifm', 'group:default/team-a'],
+              },
+            },
+            {
+              rule: 'IS_ENTITY_KIND',
+              resourceType: 'catalog-entity',
+              params: { kinds: ['Group'] },
+            },
+          ],
+          rule: 'IS_ENTITY_OWNER',
+          resourceType: 'catalog-entity',
+          params: {
+            claims: ['user:default/logarifm', 'group:default/team-a'],
+          },
+        },
+      };
+      expect(() => validateRoleCondition(condition)).toThrow(
+        `RBAC plugin does not support parallel conditions alongside rules, consider reworking request to include nested condition criteria. Conditional criteria causing the error allOf, 'rule: IS_ENTITY_OWNER'.`,
+      );
+    });
+
+    it('should fail validation of role-condition.conditions criteria (allOf, not) in parallel', () => {
+      const condition: RoleConditionalPolicyDecision<PermissionAction> = {
+        id: 1,
+        pluginId: 'catalog',
+        resourceType: 'catalog-entity',
+        roleEntityRef: 'role:default/test',
+        result: AuthorizeResult.CONDITIONAL,
+        permissionMapping: ['read'],
+        conditions: {
+          allOf: [
+            {
+              rule: 'IS_ENTITY_OWNER',
+              resourceType: 'catalog-entity',
+              params: {
+                claims: ['user:default/logarifm', 'group:default/team-a'],
+              },
+            },
+            {
+              rule: 'IS_ENTITY_KIND',
+              resourceType: 'catalog-entity',
+              params: { kinds: ['Group'] },
+            },
+          ],
+          not: {
+            rule: 'IS_ENTITY_OWNER',
+            resourceType: 'catalog-entity',
+            params: {
+              claims: ['user:default/logarifm', 'group:default/team-a'],
+            },
+          },
+        },
+      };
+      expect(() => validateRoleCondition(condition)).toThrow(
+        `RBAC plugin does not support parallel conditions, consider reworking request to include nested condition criteria. Conditional criteria causing the error allOf,not.`,
+      );
+    });
+
+    it('should fail validation of role-condition.conditions criteria (allOf, anyOf) in parallel', () => {
+      const condition: RoleConditionalPolicyDecision<PermissionAction> = {
+        id: 1,
+        pluginId: 'catalog',
+        resourceType: 'catalog-entity',
+        roleEntityRef: 'role:default/test',
+        result: AuthorizeResult.CONDITIONAL,
+        permissionMapping: ['read'],
+        conditions: {
+          allOf: [
+            {
+              rule: 'IS_ENTITY_OWNER',
+              resourceType: 'catalog-entity',
+              params: {
+                claims: ['user:default/logarifm', 'group:default/team-a'],
+              },
+            },
+            {
+              rule: 'IS_ENTITY_KIND',
+              resourceType: 'catalog-entity',
+              params: { kinds: ['Group'] },
+            },
+          ],
+          anyOf: [
+            {
+              rule: 'IS_ENTITY_OWNER',
+              resourceType: 'catalog-entity',
+              params: {
+                claims: ['user:default/logarifm', 'group:default/team-a'],
+              },
+            },
+            {
+              rule: 'IS_ENTITY_KIND',
+              resourceType: 'catalog-entity',
+              params: { kinds: ['Group'] },
+            },
+          ],
+        },
+      };
+      expect(() => validateRoleCondition(condition)).toThrow(
+        `RBAC plugin does not support parallel conditions, consider reworking request to include nested condition criteria. Conditional criteria causing the error allOf,anyOf.`,
+      );
+    });
+
+    it('should fail validation of role-condition.conditions criteria (not, anyOf) in parallel', () => {
+      const condition: RoleConditionalPolicyDecision<PermissionAction> = {
+        id: 1,
+        pluginId: 'catalog',
+        resourceType: 'catalog-entity',
+        roleEntityRef: 'role:default/test',
+        result: AuthorizeResult.CONDITIONAL,
+        permissionMapping: ['read'],
+        conditions: {
+          not: {
+            rule: 'IS_ENTITY_OWNER',
+            resourceType: 'catalog-entity',
+            params: {
+              claims: ['user:default/logarifm', 'group:default/team-a'],
+            },
+          },
+          anyOf: [
+            {
+              rule: 'IS_ENTITY_OWNER',
+              resourceType: 'catalog-entity',
+              params: {
+                claims: ['user:default/logarifm', 'group:default/team-a'],
+              },
+            },
+            {
+              rule: 'IS_ENTITY_KIND',
+              resourceType: 'catalog-entity',
+              params: { kinds: ['Group'] },
+            },
+          ],
+        },
+      };
+      expect(() => validateRoleCondition(condition)).toThrow(
+        `RBAC plugin does not support parallel conditions, consider reworking request to include nested condition criteria. Conditional criteria causing the error anyOf,not.`,
+      );
+    });
+
+    it('should validate role-condition.conditions that are nested', () => {
+      const condition: RoleConditionalPolicyDecision<PermissionAction> = {
+        id: 1,
+        pluginId: 'catalog',
+        resourceType: 'catalog-entity',
+        roleEntityRef: 'role:default/test',
+        result: AuthorizeResult.CONDITIONAL,
+        permissionMapping: ['read'],
+        conditions: {
+          anyOf: [
+            {
+              not: {
+                rule: 'IS_ENTITY_OWNER',
+                resourceType: 'catalog-entity',
+                params: {
+                  claims: ['user:default/logarifm', 'group:default/team-a'],
+                },
+              },
+            },
+            {
+              rule: 'IS_ENTITY_OWNER',
+              resourceType: 'catalog-entity',
+              params: {
+                claims: ['user:default/logarifm', 'group:default/team-a'],
+              },
+            },
+            {
+              rule: 'IS_ENTITY_KIND',
+              resourceType: 'catalog-entity',
+              params: { kinds: ['Group'] },
+            },
+          ],
+        },
+      };
+
+      let unexpectedErr;
+      try {
+        validateRoleCondition(condition);
+      } catch (err) {
+        unexpectedErr = err;
+      }
+      expect(unexpectedErr).toBeUndefined();
+    });
+  });
 });
diff --git a/...ckend/src/service/condition-validation.ts → ...nd/src/validation/condition-validation.ts b/...ckend/src/service/condition-validation.ts → ...nd/src/validation/condition-validation.ts
@@ -55,19 +55,28 @@ export function validateRoleCondition(
   }
 }
 
+/**
+ * validatePermissionCondition validate conditional permission policies using validateCriteria and validateRule.
+ * @param conditionOrCriteria The Permission Criteria of the conditional permission.
+ * @param jsonPathLocator The location in the JSON of the current check.
+ * @returns undefined.
+ */
 function validatePermissionCondition(
   conditionOrCriteria: PermissionCriteria<
     PermissionCondition<string, PermissionRuleParams>
   >,
   jsonPathLocator: string,
 ) {
+  validateCriteria(conditionOrCriteria, jsonPathLocator);
+
   if ('not' in conditionOrCriteria) {
     validatePermissionCondition(
       conditionOrCriteria.not,
       `${jsonPathLocator}.not`,
     );
     return;
   }
+
   if ('allOf' in conditionOrCriteria) {
     if (
       !Array.isArray(conditionOrCriteria.allOf) ||
@@ -82,6 +91,7 @@ function validatePermissionCondition(
     }
     return;
   }
+
   if ('anyOf' in conditionOrCriteria) {
     if (
       !Array.isArray(conditionOrCriteria.anyOf) ||
@@ -94,8 +104,20 @@ function validatePermissionCondition(
     for (const [index, elem] of conditionOrCriteria.anyOf.entries()) {
       validatePermissionCondition(elem, `${jsonPathLocator}.anyOf[${index}]`);
     }
-    return;
   }
+}
+
+/**
+ * validateRule ensures that there is a rule and resource type associated with each conditional permission.
+ * @param conditionOrCriteria The Permission Criteria of the conditional permission.
+ * @param jsonPathLocator The location in the JSON of the current check.
+ */
+function validateRule(
+  conditionOrCriteria: PermissionCriteria<
+    PermissionCondition<string, PermissionRuleParams>
+  >,
+  jsonPathLocator: string,
+) {
   if (!('resourceType' in conditionOrCriteria)) {
     throw new Error(
       `'resourceType' must be specified in the ${jsonPathLocator}.condition`,
@@ -107,3 +129,44 @@ function validatePermissionCondition(
     );
   }
 }
+
+/**
+ * validateCriteria ensures that there is only one of the following criteria: allOf, anyOf, and not, at any given level.
+ * We want to make sure that there are no parallel conditional criteria for conditional permission policies as this is
+ * not support by the permission framework.
+ *
+ * If more than one criteria are at a given level, we throw an error about the inability to support parallel conditions.
+ * If no criteria are found, we validate the rule.
+ *
+ * @param conditionOrCriteria The Permission Criteria of the conditional permission.
+ * @param jsonPathLocator The location in the JSON of the current check.
+ */
+function validateCriteria(
+  conditionOrCriteria: PermissionCriteria<
+    PermissionCondition<string, PermissionRuleParams>
+  >,
+  jsonPathLocator: string,
+) {
+  const criteriaList = ['allOf', 'anyOf', 'not'];
+  const found: string[] = [];
+
+  for (const crit of criteriaList) {
+    if (crit in conditionOrCriteria) {
+      found.push(crit);
+    }
+  }
+
+  if (found.length > 1) {
+    throw new Error(
+      `RBAC plugin does not support parallel conditions, consider reworking request to include nested condition criteria. Conditional criteria causing the error ${found}.`,
+    );
+  } else if (found.length === 0) {
+    validateRule(conditionOrCriteria, jsonPathLocator);
+  }
+
+  if (found.length === 1 && 'rule' in conditionOrCriteria) {
+    throw new Error(
+      `RBAC plugin does not support parallel conditions alongside rules, consider reworking request to include nested condition criteria. Conditional criteria causing the error ${found}, 'rule: ${conditionOrCriteria.rule}'.`,
+    );
+  }
+}