From 38d4afb6cb8b813d4a47b56bd6195c8018e6bab1 Mon Sep 17 00:00:00 2001 From: Deepa Rao Date: Wed, 5 Apr 2023 19:01:27 -0700 Subject: [PATCH] Fixes #10924 - Trim the redundant permissions in the permission list (#10928) * Fixes #10924 - Trim the redundant permissions in the permission list * Fixes #10924 - Trim the redundant permissions in the permission list --- .../policyevaluator/PolicyEvaluator.java | 54 +++++- .../permissions/PermissionsResourceTest.java | 39 ++-- .../policyevaluator/PolicyEvaluatorTest.java | 174 ++++++++++++++++++ 3 files changed, 248 insertions(+), 19 deletions(-) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/PolicyEvaluator.java b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/PolicyEvaluator.java index f1bfeb255a7..e2ba4758885 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/PolicyEvaluator.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/PolicyEvaluator.java @@ -128,7 +128,7 @@ public class PolicyEvaluator { rule.evaluatePermission(resourcePermissionMap, policyContext); } } - return new ArrayList<>(resourcePermissionMap.values()); + return PolicyEvaluator.trimResourcePermissions(new ArrayList<>(resourcePermissionMap.values())); } /** Returns a list of operations that a user can perform on all the resources. */ @@ -142,7 +142,7 @@ public class PolicyEvaluator { rule.evaluatePermission(resourcePermissionMap, policyContext); } } - return new ArrayList<>(resourcePermissionMap.values()); + return PolicyEvaluator.trimResourcePermissions(new ArrayList<>(resourcePermissionMap.values())); } /** Returns a list of operations that a user can perform on the given resource/entity type */ @@ -159,7 +159,7 @@ public class PolicyEvaluator { rule.evaluatePermission(resourceType, resourcePermission, policyContext); } } - return resourcePermission; + return PolicyEvaluator.trimResourcePermission(resourcePermission); } public static ResourcePermission getPermission( @@ -176,7 +176,7 @@ public class PolicyEvaluator { rule.evaluatePermission(subjectContext, resourceContext, resourcePermission, policyContext); } } - return resourcePermission; + return PolicyEvaluator.trimResourcePermission(resourcePermission); } /** Get list of resources with all their permissions set to given Access */ @@ -191,7 +191,7 @@ public class PolicyEvaluator { new ResourcePermission().withResource(rd.getName()).withPermissions(permissions); resourcePermissions.add(resourcePermission); } - return resourcePermissions; + return PolicyEvaluator.trimResourcePermissions(resourcePermissions); } /** Get list of resources with all their permissions set to given Access */ @@ -201,7 +201,8 @@ public class PolicyEvaluator { for (MetadataOperation operation : rd.getOperations()) { permissions.add(new Permission().withOperation(operation).withAccess(access)); } - return new ResourcePermission().withResource(rd.getName()).withPermissions(permissions); + return PolicyEvaluator.trimResourcePermission( + new ResourcePermission().withResource(rd.getName()).withPermissions(permissions)); } /** @@ -215,4 +216,45 @@ public class PolicyEvaluator { resourcePermissions.forEach(rp -> resourcePermissionMap.put(rp.getResource(), rp)); return resourcePermissionMap; } + + /** Removes the redundant permissions from the list. */ + public static List trimPermissions(List permissions) { + boolean viewAllPermission = false; + boolean editAllPermission = false; + for (Permission p : permissions) { + if ((p.getOperation().equals(MetadataOperation.VIEW_ALL) + && (p.getAccess().equals(Access.ALLOW) || p.getAccess().equals(Access.DENY)))) { + viewAllPermission = true; + } + if (p.getOperation().equals(MetadataOperation.EDIT_ALL) + && (p.getAccess().equals(Access.ALLOW) || p.getAccess().equals(Access.DENY))) { + editAllPermission = true; + } + } + Iterator permissionIterator = permissions.listIterator(); + while (permissionIterator.hasNext()) { + Permission permission = permissionIterator.next(); + if (viewAllPermission + && permission.getOperation() != MetadataOperation.VIEW_ALL + && permission.getOperation().value().startsWith("View")) { + permissionIterator.remove(); + } else if (editAllPermission + && permission.getOperation() != MetadataOperation.EDIT_ALL + && permission.getOperation().value().startsWith("Edit")) { + permissionIterator.remove(); + } + } + return permissions; + } + + public static ResourcePermission trimResourcePermission(ResourcePermission resourcePermission) { + return resourcePermission.withPermissions(trimPermissions(resourcePermission.getPermissions())); + } + + public static List trimResourcePermissions(List resourcePermissions) { + for (ResourcePermission resourcePermission : resourcePermissions) { + trimResourcePermission(resourcePermission); + } + return resourcePermissions; + } } diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/resources/permissions/PermissionsResourceTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/resources/permissions/PermissionsResourceTest.java index 2bdb1d3eef9..765847c81de 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/resources/permissions/PermissionsResourceTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/resources/permissions/PermissionsResourceTest.java @@ -150,7 +150,8 @@ class PermissionsResourceTest extends OpenMetadataApplicationTest { void get_admin_permissions_for_role() throws HttpResponseException { // Ensure an admin has all the permissions List actualPermissions = getPermissions(ADMIN_AUTH_HEADERS); - assertEquals(PolicyEvaluator.getResourcePermissions(ALLOW), actualPermissions); + assertEquals( + PolicyEvaluator.trimResourcePermissions(PolicyEvaluator.getResourcePermissions(ALLOW)), actualPermissions); } @Test @@ -168,23 +169,27 @@ class PermissionsResourceTest extends OpenMetadataApplicationTest { ORG_NO_OWNER_RULE_OPERATIONS, CONDITIONAL_ALLOW, null, ORGANIZATION_POLICY_NAME, ORG_NO_OWNER_RULE); permissionsBuilder.setPermission( ORG_IS_OWNER_RULE_OPERATIONS, CONDITIONAL_ALLOW, null, ORGANIZATION_POLICY_NAME, ORG_IS_OWNER_RULE); - assertResourcePermissions(permissionsBuilder.getResourcePermissions(), actualPermissions); + assertResourcePermissions( + PolicyEvaluator.trimResourcePermissions(permissionsBuilder.getResourcePermissions()), actualPermissions); // Validate permissions for all resources for data consumer as admin actualPermissions = getPermissions(DATA_CONSUMER_USER_NAME, ADMIN_AUTH_HEADERS); - assertResourcePermissions(permissionsBuilder.getResourcePermissions(), actualPermissions); + assertResourcePermissions( + PolicyEvaluator.trimResourcePermissions(permissionsBuilder.getResourcePermissions()), actualPermissions); // Validate permission as logged-in user for each resource one at a time ResourcePermission actualPermission; for (ResourceDescriptor rd : ResourceRegistry.listResourceDescriptors()) { actualPermission = getPermission(rd.getName(), null, authHeaders); - assertResourcePermission(permissionsBuilder.getPermission(rd.getName()), actualPermission); + assertResourcePermission( + PolicyEvaluator.trimResourcePermission(permissionsBuilder.getPermission(rd.getName())), actualPermission); } // Validate permission of data consumer as admin user for each resource one at a time for (ResourceDescriptor rd : ResourceRegistry.listResourceDescriptors()) { actualPermission = getPermission(rd.getName(), DATA_CONSUMER_USER_NAME, authHeaders); - assertResourcePermission(permissionsBuilder.getPermission(rd.getName()), actualPermission); + assertResourcePermission( + PolicyEvaluator.trimResourcePermission(permissionsBuilder.getPermission(rd.getName())), actualPermission); } } @@ -202,7 +207,8 @@ class PermissionsResourceTest extends OpenMetadataApplicationTest { permissionsBuilder.setPermission( ORG_IS_OWNER_RULE_OPERATIONS, CONDITIONAL_ALLOW, null, ORGANIZATION_POLICY_NAME, ORG_IS_OWNER_RULE); - assertResourcePermissions(permissionsBuilder.getResourcePermissions(), actualPermissions); + assertResourcePermissions( + PolicyEvaluator.trimResourcePermissions(permissionsBuilder.getResourcePermissions()), actualPermissions); } @Test @@ -214,14 +220,16 @@ class PermissionsResourceTest extends OpenMetadataApplicationTest { permissionsBuilder.setPermission( DATA_CONSUMER_ALLOWED, ALLOW, null, DATA_CONSUMER_POLICY_NAME, DATA_CONSUMER_RULES.get(0)); - assertResourcePermissions(permissionsBuilder.getResourcePermissions(), actual); + assertResourcePermissions( + PolicyEvaluator.trimResourcePermissions(permissionsBuilder.getResourcePermissions()), actual); // Get permissions for DATA_CONSUMER and DATA_STEWARD policies together and assert it is correct policies.add(DATA_STEWARD_POLICY.getId()); actual = getPermissionsForPolicies(policies, ADMIN_AUTH_HEADERS); permissionsBuilder.setPermission( DATA_STEWARD_ALLOWED, ALLOW, null, DATA_STEWARD_POLICY_NAME, DATA_STEWARD_RULES.get(0)); - assertResourcePermissions(permissionsBuilder.getResourcePermissions(), actual); + assertResourcePermissions( + PolicyEvaluator.trimResourcePermissions(permissionsBuilder.getResourcePermissions()), actual); } @Test @@ -247,7 +255,8 @@ class PermissionsResourceTest extends OpenMetadataApplicationTest { // Note that conditional list is empty. All the required context to resolve is met when requesting permission of // a specific resource (both subject and resource context). Only Deny, Allow, NotAllow permissions are expected. - assertResourcePermission(permissionsBuilder.getPermission(Entity.TABLE), actualPermission); + assertResourcePermission( + PolicyEvaluator.trimResourcePermission(permissionsBuilder.getPermission(Entity.TABLE)), actualPermission); } @Test @@ -274,15 +283,18 @@ class PermissionsResourceTest extends OpenMetadataApplicationTest { permissionsBuilder.setPermission( ORG_IS_OWNER_RULE_OPERATIONS, ALLOW, null, ORGANIZATION_POLICY_NAME, ORG_IS_OWNER_RULE); ResourcePermission actualPermission = getPermission(Entity.TABLE, table.getId(), null, authHeaders); - assertResourcePermission(permissionsBuilder.getPermission(Entity.TABLE), actualPermission); + assertResourcePermission( + PolicyEvaluator.trimResourcePermission(permissionsBuilder.getPermission(Entity.TABLE)), actualPermission); // get permissions by resource entity name actualPermission = getPermissionByName(Entity.TABLE, table.getFullyQualifiedName(), null, authHeaders); - assertResourcePermission(permissionsBuilder.getPermission(Entity.TABLE), actualPermission); + assertResourcePermission( + PolicyEvaluator.trimResourcePermission(permissionsBuilder.getPermission(Entity.TABLE)), actualPermission); // Admin getting permissions for a specific resource on for Data consumer actualPermission = getPermission(Entity.TABLE, table.getId(), DATA_CONSUMER_USER_NAME, ADMIN_AUTH_HEADERS); - assertResourcePermission(permissionsBuilder.getPermission(Entity.TABLE), actualPermission); + assertResourcePermission( + PolicyEvaluator.trimResourcePermission(permissionsBuilder.getPermission(Entity.TABLE)), actualPermission); PolicyResourceTest policyResourceTest = new PolicyResourceTest(); Policy orgPolicy = policyResourceTest.getEntityByName(ORGANIZATION_POLICY_NAME, "", ADMIN_AUTH_HEADERS); @@ -306,7 +318,8 @@ class PermissionsResourceTest extends OpenMetadataApplicationTest { permissionsBuilder.setPermission( allowedOperations, ALLOW, null, ORGANIZATION_POLICY_NAME, orgPolicy.getRules().get(1)); actualPermission = getPermissionByName(Entity.TABLE, table.getFullyQualifiedName(), null, authHeaders); - assertResourcePermission(permissionsBuilder.getPermission(Entity.TABLE), actualPermission); + assertResourcePermission( + PolicyEvaluator.trimResourcePermission(permissionsBuilder.getPermission(Entity.TABLE)), actualPermission); // Finally, try to patch the field that can't be edited and expect permission denied String field = ResourceRegistry.getField(editOperation); diff --git a/openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/PolicyEvaluatorTest.java b/openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/PolicyEvaluatorTest.java index 2afa990a7bd..6ca035a9cbb 100644 --- a/openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/PolicyEvaluatorTest.java +++ b/openmetadata-service/src/test/java/org/openmetadata/service/security/policyevaluator/PolicyEvaluatorTest.java @@ -2,8 +2,13 @@ package org.openmetadata.service.security.policyevaluator; import static org.junit.jupiter.api.Assertions.*; +import java.util.*; +import java.util.stream.Collectors; import org.junit.jupiter.api.Test; +import org.openmetadata.schema.type.MetadataOperation; +import org.openmetadata.schema.type.Permission; import org.openmetadata.schema.type.Permission.Access; +import org.openmetadata.schema.type.ResourcePermission; class PolicyEvaluatorTest { @Test @@ -55,4 +60,173 @@ class PolicyEvaluatorTest { // newAccess (ConditionalAllow|NotAllow) and currentAccess ConditionalDeny takes precedence assertFalse(CompiledRule.overrideAccess(Access.NOT_ALLOW, Access.NOT_ALLOW)); } + + @Test + void trimResourcePermissions() { + MetadataOperation[] op1 = { + MetadataOperation.ALL, + MetadataOperation.VIEW_ALL, + MetadataOperation.VIEW_BASIC, + MetadataOperation.VIEW_QUERIES, + MetadataOperation.EDIT_ALL, + MetadataOperation.EDIT_LINEAGE, + MetadataOperation.EDIT_CUSTOM_FIELDS + }; + ResourcePermission rp1 = getResourcePermission("r1", Access.DENY, op1); + List expectedOp1 = + new ArrayList(List.of(MetadataOperation.ALL, MetadataOperation.VIEW_ALL, MetadataOperation.EDIT_ALL)); + + MetadataOperation[] op2 = { + MetadataOperation.ALL, + MetadataOperation.VIEW_BASIC, + MetadataOperation.VIEW_USAGE, + MetadataOperation.EDIT_ALL, + MetadataOperation.EDIT_LINEAGE, + MetadataOperation.EDIT_CUSTOM_FIELDS, + MetadataOperation.EDIT_DISPLAY_NAME + }; + ResourcePermission rp2 = getResourcePermission("r2", Access.ALLOW, op2); + List expectedOp2 = + new ArrayList( + List.of( + MetadataOperation.ALL, + MetadataOperation.VIEW_BASIC, + MetadataOperation.VIEW_USAGE, + MetadataOperation.EDIT_ALL)); + + List rpList = List.of(rp1, rp2); + PolicyEvaluator.trimResourcePermissions(rpList); + assertEqualsPermissions(expectedOp1, rpList.get(0).getPermissions()); + assertEqualsPermissions(expectedOp2, rpList.get(1).getPermissions()); + } + + @Test + void trimResourcePermission() { + MetadataOperation[] operations = { + MetadataOperation.ALL, + MetadataOperation.VIEW_ALL, + MetadataOperation.VIEW_BASIC, + MetadataOperation.VIEW_QUERIES, + MetadataOperation.EDIT_ALL, + MetadataOperation.EDIT_LINEAGE, + MetadataOperation.EDIT_CUSTOM_FIELDS + }; + ResourcePermission rp = getResourcePermission("testResource", Access.ALLOW, operations); + ResourcePermission trimmedRp = PolicyEvaluator.trimResourcePermission(rp); + List expectedOperations = + new ArrayList(List.of(MetadataOperation.ALL, MetadataOperation.VIEW_ALL, MetadataOperation.EDIT_ALL)); + assertEqualsPermissions(expectedOperations, trimmedRp.getPermissions()); + } + + @Test + void trimPermissions_withAllowAccess_trimmed() { + List permissions = getPermissions(OperationContext.getAllOperations(), Access.ALLOW); + List expectedOperations = + Arrays.asList( + MetadataOperation.ALL, + MetadataOperation.DELETE, + MetadataOperation.CREATE, + MetadataOperation.VIEW_ALL, + MetadataOperation.EDIT_ALL); + List actual = PolicyEvaluator.trimPermissions(permissions); + assertEqualsPermissions(expectedOperations, actual); + } + + @Test + void trimPermissions_withDenyAccess_trimmed() { + List permissions = getPermissions(OperationContext.getAllOperations(), Access.DENY); + List expectedOperations = + Arrays.asList( + MetadataOperation.ALL, + MetadataOperation.DELETE, + MetadataOperation.CREATE, + MetadataOperation.VIEW_ALL, + MetadataOperation.EDIT_ALL); + List actual = PolicyEvaluator.trimPermissions(permissions); + assertEqualsPermissions(expectedOperations, actual); + } + + @Test + void trimPermissions_withNotAllowAccessToViewAll_viewOpsNotTrimmed() { + List permissions = getPermissions(OperationContext.getAllOperations(), Access.ALLOW); + List expectedOperations = + Arrays.stream(MetadataOperation.values()) + .filter(operation -> (!operation.value().startsWith("Edit"))) + .collect(Collectors.toList()); + expectedOperations.add(MetadataOperation.EDIT_ALL); + updateAccess(permissions, MetadataOperation.VIEW_ALL, Access.NOT_ALLOW); + + List actual = PolicyEvaluator.trimPermissions(permissions); + assertEqualsPermissions(expectedOperations, actual); + } + + @Test + void trimPermissions_withConditionalAllowAccessToEditAll_editOpsNotTrimmed() { + List permissions = getPermissions(OperationContext.getAllOperations(), Access.ALLOW); + List expectedOperations = + Arrays.stream(MetadataOperation.values()) + .filter(operation -> (!operation.value().startsWith("View"))) + .collect(Collectors.toList()); + expectedOperations.add(MetadataOperation.VIEW_ALL); + updateAccess(permissions, MetadataOperation.EDIT_ALL, Access.CONDITIONAL_ALLOW); + + List actual = PolicyEvaluator.trimPermissions(permissions); + assertEqualsPermissions(expectedOperations, actual); + } + + @Test + void trimPermissions_withConditionalAccess_notTrimmed() { + List permissions = getPermissions(OperationContext.getAllOperations(), Access.ALLOW); + List expectedOperations = OperationContext.getAllOperations(); + updateAccess(permissions, MetadataOperation.VIEW_ALL, Access.CONDITIONAL_ALLOW); + updateAccess(permissions, MetadataOperation.EDIT_ALL, Access.CONDITIONAL_DENY); + + List actual = PolicyEvaluator.trimPermissions(permissions); + assertEqualsPermissions(expectedOperations, actual); + } + + public static void assertEqualsPermissions(List expectedOperations, List actual) { + assertEquals(expectedOperations.size(), actual.size()); + + Comparator comparator = Comparator.comparing(Permission::getOperation); + actual.sort(comparator); + Collections.sort(expectedOperations); + for (int i = 0; i < expectedOperations.size(); i++) { + assertEquals(expectedOperations.get(i).value(), actual.get(i).getOperation().value()); + } + } + + public static List getPermissions(List operations, Access access) { + ArrayList permissions = new ArrayList<>(); + operations.stream().forEach(operation -> permissions.add(getPermission(operation, access))); + return permissions; + } + + public static List updateAccess( + List permissions, final MetadataOperation operation, Access access) { + permissions.stream() + .forEach( + permission -> { + if (permission.getOperation().equals(operation)) permission.setAccess(access); + }); + return permissions; + } + + public static ResourcePermission getResourcePermission( + String resourceName, Access access, MetadataOperation... operations) { + ResourcePermission rp = new ResourcePermission(); + List permissions = new ArrayList<>(); + rp.setResource(resourceName); + for (int i = 0; i < operations.length; i++) { + permissions.add(new Permission().withAccess(access).withOperation(operations[i])); + } + rp.setPermissions(permissions); + return rp; + } + + public static Permission getPermission(MetadataOperation operation, Access access) { + Permission permission = new Permission().withOperation(operation); + permission.setAccess(access); + return permission; + } }