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

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<Permission> trimPermissions(List<Permission> 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<Permission> 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<ResourcePermission> trimResourcePermissions(List<ResourcePermission> resourcePermissions) {
+    for (ResourcePermission resourcePermission : resourcePermissions) {
+      trimResourcePermission(resourcePermission);
+    }
+    return resourcePermissions;
+  }
 }

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<ResourcePermission> 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);

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<MetadataOperation> 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<MetadataOperation> expectedOp2 =
+        new ArrayList(
+            List.of(
+                MetadataOperation.ALL,
+                MetadataOperation.VIEW_BASIC,
+                MetadataOperation.VIEW_USAGE,
+                MetadataOperation.EDIT_ALL));
+
+    List<ResourcePermission> 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<MetadataOperation> expectedOperations =
+        new ArrayList(List.of(MetadataOperation.ALL, MetadataOperation.VIEW_ALL, MetadataOperation.EDIT_ALL));
+    assertEqualsPermissions(expectedOperations, trimmedRp.getPermissions());
+  }
+
+  @Test
+  void trimPermissions_withAllowAccess_trimmed() {
+    List<Permission> permissions = getPermissions(OperationContext.getAllOperations(), Access.ALLOW);
+    List<MetadataOperation> expectedOperations =
+        Arrays.asList(
+            MetadataOperation.ALL,
+            MetadataOperation.DELETE,
+            MetadataOperation.CREATE,
+            MetadataOperation.VIEW_ALL,
+            MetadataOperation.EDIT_ALL);
+    List<Permission> actual = PolicyEvaluator.trimPermissions(permissions);
+    assertEqualsPermissions(expectedOperations, actual);
+  }
+
+  @Test
+  void trimPermissions_withDenyAccess_trimmed() {
+    List<Permission> permissions = getPermissions(OperationContext.getAllOperations(), Access.DENY);
+    List<MetadataOperation> expectedOperations =
+        Arrays.asList(
+            MetadataOperation.ALL,
+            MetadataOperation.DELETE,
+            MetadataOperation.CREATE,
+            MetadataOperation.VIEW_ALL,
+            MetadataOperation.EDIT_ALL);
+    List<Permission> actual = PolicyEvaluator.trimPermissions(permissions);
+    assertEqualsPermissions(expectedOperations, actual);
+  }
+
+  @Test
+  void trimPermissions_withNotAllowAccessToViewAll_viewOpsNotTrimmed() {
+    List<Permission> permissions = getPermissions(OperationContext.getAllOperations(), Access.ALLOW);
+    List<MetadataOperation> 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<Permission> actual = PolicyEvaluator.trimPermissions(permissions);
+    assertEqualsPermissions(expectedOperations, actual);
+  }
+
+  @Test
+  void trimPermissions_withConditionalAllowAccessToEditAll_editOpsNotTrimmed() {
+    List<Permission> permissions = getPermissions(OperationContext.getAllOperations(), Access.ALLOW);
+    List<MetadataOperation> 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<Permission> actual = PolicyEvaluator.trimPermissions(permissions);
+    assertEqualsPermissions(expectedOperations, actual);
+  }
+
+  @Test
+  void trimPermissions_withConditionalAccess_notTrimmed() {
+    List<Permission> permissions = getPermissions(OperationContext.getAllOperations(), Access.ALLOW);
+    List<MetadataOperation> expectedOperations = OperationContext.getAllOperations();
+    updateAccess(permissions, MetadataOperation.VIEW_ALL, Access.CONDITIONAL_ALLOW);
+    updateAccess(permissions, MetadataOperation.EDIT_ALL, Access.CONDITIONAL_DENY);
+
+    List<Permission> actual = PolicyEvaluator.trimPermissions(permissions);
+    assertEqualsPermissions(expectedOperations, actual);
+  }
+
+  public static void assertEqualsPermissions(List<MetadataOperation> expectedOperations, List<Permission> actual) {
+    assertEquals(expectedOperations.size(), actual.size());
+
+    Comparator<Permission> 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<Permission> getPermissions(List<MetadataOperation> operations, Access access) {
+    ArrayList<Permission> permissions = new ArrayList<>();
+    operations.stream().forEach(operation -> permissions.add(getPermission(operation, access)));
+    return permissions;
+  }
+
+  public static List<Permission> updateAccess(
+      List<Permission> 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<Permission> 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;
+  }
 }