fix(policyEngine): policy evaluation incorrect without type (#13371)

datahub-graphql-core/src/main/resources/entity.graphql

@@ -9323,19 +9323,22 @@ The resources that a DataHub Access Policy applies to
 """
 type ResourceFilter {
   """
+  Deprecated, use filter instead
   The type of the resource the policy should apply to Not required because in the future we want to support filtering by type OR by domain
   """
-  type: String
+  type: String @deprecated
 
   """
+  Deprecated, use filter instead
   A list of specific resource urns to apply the filter to
   """
-  resources: [String!]
+  resources: [String!] @deprecated
 
   """
+  Deprecated, use filter instead
   Whether of not to apply the filter to all resources of the type
   """
-  allResources: Boolean
+  allResources: Boolean @deprecated
 
   """
   Whether of not to apply the filter to all resources of the type
@@ -9469,17 +9472,20 @@ Input required when creating or updating an Access Policies Determines which res
 """
 input ResourceFilterInput {
   """
+  Deprecated, use filter field instead
   The type of the resource the policy should apply to
   Not required because in the future we want to support filtering by type OR by domain
   """
-  type: String
+  type: String @deprecated
 
   """
+  Deprecated, use filter instead
   A list of specific resource urns to apply the filter to
   """
   resources: [String!]
 
   """
+  Deprecated, use empty filter instead
   Whether of not to apply the filter to all resources of the type
   """
   allResources: Boolean

metadata-service/auth-impl/src/main/java/com/datahub/authorization/PolicyEngine.java

@@ -232,9 +232,8 @@ public class PolicyEngine {
               .setValues(
                   new StringArray(Collections.singletonList(policyResourceFilter.getType()))));
     }
-    if (policyResourceFilter.hasType()
+
-        && policyResourceFilter.hasResources()
+    if (policyResourceFilter.hasResources() && !policyResourceFilter.isAllResources()) {
-        && !policyResourceFilter.isAllResources()) {
       criteria.add(
           new PolicyMatchCriterion()
               .setField(EntityFieldType.URN.name())

metadata-service/auth-impl/src/test/java/com/datahub/authorization/PolicyEngineTest.java

@@ -955,6 +955,56 @@ public class PolicyEngineTest {
     verify(_entityClient, times(0)).batchGetV2(any(), any(), any(), any());
   }
 
+  @Test
+  public void testEvaluatePolicyResourceFilterSpecificResourceMatchLegacyWithNoType()
+      throws Exception {
+    final DataHubPolicyInfo dataHubPolicyInfo = new DataHubPolicyInfo();
+    dataHubPolicyInfo.setType(METADATA_POLICY_TYPE);
+    dataHubPolicyInfo.setState(ACTIVE_POLICY_STATE);
+    dataHubPolicyInfo.setPrivileges(new StringArray("EDIT_ENTITY_TAGS"));
+    dataHubPolicyInfo.setDisplayName("My Test Display");
+    dataHubPolicyInfo.setDescription("My test display!");
+    dataHubPolicyInfo.setEditable(true);
+
+    final DataHubActorFilter actorFilter = new DataHubActorFilter();
+    actorFilter.setResourceOwners(true);
+    actorFilter.setAllUsers(true);
+    actorFilter.setAllGroups(true);
+    dataHubPolicyInfo.setActors(actorFilter);
+
+    final DataHubResourceFilter resourceFilter = new DataHubResourceFilter();
+    resourceFilter.setAllResources(true);
+
+    ResolvedEntitySpec resourceSpec = buildEntityResolvers("dataset", RESOURCE_URN);
+    PolicyEngine.PolicyEvaluationResult result =
+        _policyEngine.evaluatePolicy(
+            systemOperationContext,
+            dataHubPolicyInfo,
+            resolvedAuthorizedUserSpec,
+            "EDIT_ENTITY_TAGS",
+            Optional.of(resourceSpec));
+    assertTrue(result.isGranted());
+
+    StringArray resourceUrns = new StringArray();
+    resourceUrns.add(RESOURCE_URN); // Filter applies to specific resource.
+    resourceFilter.setResources(resourceUrns);
+    resourceFilter.setAllResources(false); // This time should match due to RESOURCE_URN
+    dataHubPolicyInfo.setResources(resourceFilter);
+
+    resourceSpec = buildEntityResolvers("dataset", RESOURCE_URN);
+    result =
+        _policyEngine.evaluatePolicy(
+            systemOperationContext,
+            dataHubPolicyInfo,
+            resolvedAuthorizedUserSpec,
+            "EDIT_ENTITY_TAGS",
+            Optional.of(resourceSpec));
+    assertTrue(result.isGranted());
+
+    // Verify no network calls
+    verify(_entityClient, times(0)).batchGetV2(any(), any(), any(), any());
+  }
+
   @Test
   public void testEvaluatePolicyResourceFilterSpecificResourceMatch() throws Exception {
     final DataHubPolicyInfo dataHubPolicyInfo = new DataHubPolicyInfo();
@@ -1037,6 +1087,60 @@ public class PolicyEngineTest {
     verify(_entityClient, times(0)).batchGetV2(any(), any(), any(), any());
   }
 
+  @Test
+  public void testEvaluatePolicyResourceFilterSpecificResourceLegacyNoMatchWithNoType()
+      throws Exception {
+    final DataHubPolicyInfo dataHubPolicyInfo = new DataHubPolicyInfo();
+    dataHubPolicyInfo.setType(METADATA_POLICY_TYPE);
+    dataHubPolicyInfo.setState(ACTIVE_POLICY_STATE);
+    dataHubPolicyInfo.setPrivileges(new StringArray("EDIT_ENTITY_TAGS"));
+    dataHubPolicyInfo.setDisplayName("My Test Display");
+    dataHubPolicyInfo.setDescription("My test display!");
+    dataHubPolicyInfo.setEditable(true);
+
+    final DataHubActorFilter actorFilter = new DataHubActorFilter();
+    actorFilter.setResourceOwners(true);
+    actorFilter.setAllUsers(true);
+    actorFilter.setAllGroups(true);
+    dataHubPolicyInfo.setActors(actorFilter);
+
+    final DataHubResourceFilter resourceFilter = new DataHubResourceFilter();
+    StringArray resourceUrns = new StringArray();
+    resourceUrns.add(RESOURCE_URN); // Filter applies to specific resource.
+    resourceFilter.setResources(resourceUrns);
+    dataHubPolicyInfo.setResources(resourceFilter);
+
+    ResolvedEntitySpec resourceSpec =
+        buildEntityResolvers(
+            "dataset", "urn:li:dataset:random"); // A resource not covered by the policy.
+    PolicyEngine.PolicyEvaluationResult result =
+        _policyEngine.evaluatePolicy(
+            systemOperationContext,
+            dataHubPolicyInfo,
+            resolvedAuthorizedUserSpec,
+            "EDIT_ENTITY_TAGS",
+            Optional.of(resourceSpec));
+    assertFalse(result.isGranted());
+
+    resourceFilter.setAllResources(true);
+    dataHubPolicyInfo.setResources(resourceFilter);
+
+    resourceSpec =
+        buildEntityResolvers(
+            "dataset", "urn:li:dataset:random"); // A resource not covered by the policy.
+    result =
+        _policyEngine.evaluatePolicy(
+            systemOperationContext,
+            dataHubPolicyInfo,
+            resolvedAuthorizedUserSpec,
+            "EDIT_ENTITY_TAGS",
+            Optional.of(resourceSpec));
+    assertTrue(result.isGranted()); // Due to allResources set to true
+
+    // Verify no network calls
+    verify(_entityClient, times(0)).batchGetV2(any(), any(), any(), any());
+  }
+
   @Test
   public void testEvaluatePolicyResourceFilterResourceUrnStartsWithMatch() throws Exception {
     final DataHubPolicyInfo dataHubPolicyInfo = new DataHubPolicyInfo();