From 605871db2ec870129336500a70c4cea6c620a8c8 Mon Sep 17 00:00:00 2001 From: Suresh Srinivas Date: Tue, 23 Aug 2022 15:31:42 -0700 Subject: [PATCH] Fixes #6851 Backend : Roles patch API is not working for policies (#6890) --- .../exception/CatalogExceptionMessage.java | 2 + .../catalog/jdbi3/PolicyRepository.java | 22 +++++++- .../catalog/jdbi3/RoleRepository.java | 25 ++++++++++ .../openmetadata/catalog/util/EntityUtil.java | 10 ++++ .../PipelineServiceClientTest.java | 2 +- .../policies/PolicyResourceTest.java | 50 +++++++++++++++++-- .../resources/teams/RoleResourceTest.java | 49 +++++++++++++++++- 7 files changed, 153 insertions(+), 7 deletions(-) diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/exception/CatalogExceptionMessage.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/exception/CatalogExceptionMessage.java index cdf7eb6a503..4b855207440 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/exception/CatalogExceptionMessage.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/exception/CatalogExceptionMessage.java @@ -25,6 +25,8 @@ public final class CatalogExceptionMessage { public static final String FIELD_NOT_TOKENIZED = "Field is not tokenized"; public static final String FIELD_ALREADY_TOKENIZED = "Field is already tokenized"; public static final String INVALID_ENTITY_LINK = "Entity link must have both {arrayFieldName} and {arrayFieldValue}"; + public static final String EMPTY_POLICIES_IN_ROLE = "At least one policy is required in a role"; + public static final String EMPTY_RULES_IN_POLICY = "At least one rule is required in a policy"; private CatalogExceptionMessage() {} diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/PolicyRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/PolicyRepository.java index a0986c869ab..53e2245c831 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/PolicyRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/PolicyRepository.java @@ -17,10 +17,14 @@ import static org.openmetadata.catalog.Entity.FIELD_OWNER; import static org.openmetadata.catalog.Entity.LOCATION; import static org.openmetadata.catalog.Entity.POLICY; import static org.openmetadata.catalog.util.EntityUtil.entityReferenceMatch; +import static org.openmetadata.catalog.util.EntityUtil.resolveRules; +import static org.openmetadata.catalog.util.EntityUtil.ruleMatch; +import static org.openmetadata.common.utils.CommonUtil.listOrEmpty; import java.io.IOException; import java.net.URI; import java.util.ArrayList; +import java.util.Comparator; import java.util.List; import lombok.extern.slf4j.Slf4j; import org.jdbi.v3.sqlobject.transaction.Transaction; @@ -33,6 +37,7 @@ import org.openmetadata.catalog.jdbi3.CollectionDAO.EntityRelationshipRecord; import org.openmetadata.catalog.resources.policies.PolicyResource; import org.openmetadata.catalog.security.policyevaluator.CompiledRule; import org.openmetadata.catalog.type.EntityReference; +import org.openmetadata.catalog.type.MetadataOperation; import org.openmetadata.catalog.type.PolicyType; import org.openmetadata.catalog.type.Relationship; import org.openmetadata.catalog.util.EntityUtil; @@ -140,11 +145,18 @@ public class PolicyRepository extends EntityRepository { // Resolve JSON blobs into Rule object and perform schema based validation List rules = EntityUtil.resolveRules(policy.getRules()); + if (listOrEmpty(rules).isEmpty()) { + throw new IllegalArgumentException(CatalogExceptionMessage.EMPTY_RULES_IN_POLICY); + } + System.out.println("XXX rules count " + rules.size()); // Validate all the expressions in the rule for (Rule rule : rules) { CompiledRule.validateExpression(rule.getCondition(), Boolean.class); + rule.getResources().sort(String.CASE_INSENSITIVE_ORDER); + rule.getOperations().sort(Comparator.comparing(MetadataOperation::value)); } + rules.sort(Comparator.comparing(Rule::getName)); } public List getAccessControlPolicies() throws IOException { @@ -182,8 +194,8 @@ public class PolicyRepository extends EntityRepository { throw new IllegalArgumentException(CatalogExceptionMessage.readOnlyAttribute(POLICY, "policyType")); } recordChange(ENABLED, original.getEnabled(), updated.getEnabled()); - recordChange("rules", original.getRules(), updated.getRules()); updateLocation(original, updated); + updateRules(original.getRules(), updated.getRules()); } private void updateLocation(Policy origPolicy, Policy updatedPolicy) throws IOException { @@ -203,5 +215,13 @@ public class PolicyRepository extends EntityRepository { } recordChange("location", origPolicy.getLocation(), updatedPolicy.getLocation(), true, entityReferenceMatch); } + + private void updateRules(List origRules, List updatedRules) throws IOException { + // Record change description + List deletedRules = new ArrayList<>(); + List addedRules = new ArrayList<>(); + recordListChange( + "rules", resolveRules(origRules), resolveRules(updatedRules), addedRules, deletedRules, ruleMatch); + } } } diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/RoleRepository.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/RoleRepository.java index 6da84c5d3dd..1a1e06b616a 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/RoleRepository.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/jdbi3/RoleRepository.java @@ -14,8 +14,10 @@ package org.openmetadata.catalog.jdbi3; import static org.openmetadata.catalog.Entity.POLICIES; +import static org.openmetadata.catalog.util.EntityUtil.entityReferenceMatch; import static org.openmetadata.common.utils.CommonUtil.listOrEmpty; +import com.fasterxml.jackson.core.JsonProcessingException; import java.io.IOException; import java.util.ArrayList; import java.util.List; @@ -26,6 +28,7 @@ import lombok.extern.slf4j.Slf4j; import org.jdbi.v3.sqlobject.transaction.Transaction; import org.openmetadata.catalog.Entity; import org.openmetadata.catalog.entity.teams.Role; +import org.openmetadata.catalog.exception.CatalogExceptionMessage; import org.openmetadata.catalog.jdbi3.CollectionDAO.EntityRelationshipRecord; import org.openmetadata.catalog.resources.teams.RoleResource; import org.openmetadata.catalog.type.EntityReference; @@ -77,6 +80,9 @@ public class RoleRepository extends EntityRepository { @Override public void prepare(Role role) throws IOException { setFullyQualifiedName(role); + if (listOrEmpty(role.getPolicies()).isEmpty()) { + throw new IllegalArgumentException(CatalogExceptionMessage.EMPTY_POLICIES_IN_ROLE); + } EntityUtil.populateEntityReferences(role.getPolicies()); } @@ -136,6 +142,25 @@ public class RoleRepository extends EntityRepository { @Override public void entitySpecificUpdate() throws IOException { updateDefault(original, updated); + updatePolicies(listOrEmpty(original.getPolicies()), listOrEmpty(updated.getPolicies())); + } + + private void updatePolicies(List origPolicies, List updatedPolicies) + throws JsonProcessingException { + // Record change description + List deletedPolicies = new ArrayList<>(); + List addedPolicies = new ArrayList<>(); + boolean changed = + recordListChange( + "policies", origPolicies, updatedPolicies, addedPolicies, deletedPolicies, entityReferenceMatch); + + if (changed) { + // Remove all the Role to policy relationships + deleteFrom(original.getId(), Entity.ROLE, Relationship.HAS, Entity.POLICY); + + // Add Role to policy relationships back based on Updated entity + storeRelationships(updated); + } } private void updateDefault(Role origRole, Role updatedRole) throws IOException { diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java index 9ac1f7bce89..45814dc501e 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/util/EntityUtil.java @@ -57,6 +57,7 @@ import org.openmetadata.catalog.type.EventFilter; import org.openmetadata.catalog.type.EventType; import org.openmetadata.catalog.type.FailureDetails; import org.openmetadata.catalog.type.FieldChange; +import org.openmetadata.catalog.type.MetadataOperation; import org.openmetadata.catalog.type.MlFeature; import org.openmetadata.catalog.type.MlHyperParameter; import org.openmetadata.catalog.type.Schedule; @@ -87,6 +88,7 @@ public final class EntityUtil { public static final Comparator compareChangeEvent = Comparator.comparing(ChangeEvent::getTimestamp); public static final Comparator compareGlossaryTerm = Comparator.comparing(GlossaryTerm::getName); public static final Comparator compareCustomProperty = Comparator.comparing(CustomProperty::getName); + public static final Comparator compareOperation = Comparator.comparing(MetadataOperation::value); // // Matchers used for matching two items in a list @@ -137,6 +139,14 @@ public final class EntityUtil { public static final BiPredicate customFieldMatch = (ref1, ref2) -> ref1.getName().equals(ref2.getName()); + public static final BiPredicate ruleMatch = + (ref1, ref2) -> + ref1.getName().equals(ref2.getName()) + && ref1.getOperations().equals(ref2.getOperations()) + && ref1.getResources().equals(ref2.getResources()) + && ref1.getEffect().equals(ref2.getEffect()) + && Objects.equals(ref1.getCondition(), ref2.getCondition()); + private EntityUtil() {} /** Validate Ingestion Schedule */ diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/pipelineService/PipelineServiceClientTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/pipelineService/PipelineServiceClientTest.java index 478edba6f10..0a343d405e1 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/pipelineService/PipelineServiceClientTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/pipelineService/PipelineServiceClientTest.java @@ -3,7 +3,7 @@ package org.openmetadata.catalog.pipelineService; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertThrows; -import org.junit.Test; +import org.junit.jupiter.api.Test; import org.openmetadata.catalog.exception.PipelineServiceVersionException; public class PipelineServiceClientTest { diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/policies/PolicyResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/policies/PolicyResourceTest.java index 65f0e886303..06e97d7dcf1 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/policies/PolicyResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/policies/PolicyResourceTest.java @@ -18,6 +18,7 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.openmetadata.catalog.entity.policies.accessControl.Rule.Effect.ALLOW; import static org.openmetadata.catalog.entity.policies.accessControl.Rule.Effect.DENY; +import static org.openmetadata.catalog.util.EntityUtil.resolveRules; import static org.openmetadata.catalog.util.TestUtils.ADMIN_AUTH_HEADERS; import static org.openmetadata.catalog.util.TestUtils.UpdateType.MINOR_UPDATE; import static org.openmetadata.catalog.util.TestUtils.assertListNotNull; @@ -88,13 +89,15 @@ public class PolicyResourceTest extends EntityResourceTest } public void setupPolicies() throws HttpResponseException { - POLICY1 = createEntity(createRequest("policy1"), ADMIN_AUTH_HEADERS); - POLICY2 = createEntity(createRequest("policy2"), ADMIN_AUTH_HEADERS); + POLICY1 = createEntity(createRequest("policy1").withOwner(null), ADMIN_AUTH_HEADERS); + POLICY2 = createEntity(createRequest("policy2").withOwner(null), ADMIN_AUTH_HEADERS); } @Override public CreatePolicy createRequest(String name) { - return new CreatePolicy().withName(name).withPolicyType(PolicyType.Lifecycle); + List rules = new ArrayList<>(); + rules.add(accessControlRule("rule1", List.of("all"), List.of(MetadataOperation.EDIT_DESCRIPTION), ALLOW)); + return createAccessControlPolicyWithRules(name, rules); } @Override @@ -104,7 +107,7 @@ public class PolicyResourceTest extends EntityResourceTest if (createRequest.getLocation() != null) { assertEquals(createRequest.getLocation(), policy.getLocation().getId()); } - assertEquals(createRequest.getRules(), EntityUtil.resolveRules(policy.getRules())); + assertEquals(createRequest.getRules(), resolveRules(policy.getRules())); } @Override @@ -123,6 +126,10 @@ public class PolicyResourceTest extends EntityResourceTest EntityReference expectedLocation = (EntityReference) expected; EntityReference actualLocation = JsonUtils.readValue(actual.toString(), EntityReference.class); assertEquals(expectedLocation.getId(), actualLocation.getId()); + } else if (fieldName.equals("rules")) { + List expectedRule = (List) expected; + List actualRule = resolveRules(JsonUtils.readObjects(actual.toString(), Object.class)); + assertEquals(expectedRule, actualRule); } else { assertCommonFieldChange(fieldName, expected, actual); } @@ -253,6 +260,36 @@ public class PolicyResourceTest extends EntityResourceTest patchEntityAndCheck(policy, origJson, ADMIN_AUTH_HEADERS, MINOR_UPDATE, change); } + @Test + void patch_PolicyRules(TestInfo test) throws IOException { + Rule rule1 = accessControlRule("rule1", List.of("all"), List.of(MetadataOperation.VIEW_ALL), ALLOW); + Policy policy = createAndCheckEntity(createRequest(test).withRules(List.of(rule1)), ADMIN_AUTH_HEADERS); + + // Change an existing rule1 + String origJson = JsonUtils.pojoToJson(policy); + Rule updatedRule1 = accessControlRule("rule1", List.of("all"), List.of(MetadataOperation.ALL), ALLOW); + policy.setRules(List.of(updatedRule1)); + ChangeDescription change = getChangeDescription(policy.getVersion()); + change.getFieldsDeleted().add(new FieldChange().withName("rules").withOldValue(List.of(rule1))); + change.getFieldsAdded().add(new FieldChange().withName("rules").withNewValue(List.of(updatedRule1))); + policy = patchEntityAndCheck(policy, origJson, ADMIN_AUTH_HEADERS, MINOR_UPDATE, change); + + // Add a new rule + origJson = JsonUtils.pojoToJson(policy); + Rule newRule = accessControlRule("newRule", List.of("all"), List.of(MetadataOperation.EDIT_DESCRIPTION), ALLOW); + policy.getRules().add(newRule); + change = getChangeDescription(policy.getVersion()); + change.getFieldsAdded().add(new FieldChange().withName("rules").withNewValue(List.of(newRule))); + policy = patchEntityAndCheck(policy, origJson, ADMIN_AUTH_HEADERS, MINOR_UPDATE, change); + + // Delete newRule1 rule + origJson = JsonUtils.pojoToJson(policy); + policy.setRules(List.of(newRule)); + change = getChangeDescription(policy.getVersion()); + change.getFieldsDeleted().add(new FieldChange().withName("rules").withOldValue(List.of(updatedRule1))); + patchEntityAndCheck(policy, origJson, ADMIN_AUTH_HEADERS, MINOR_UPDATE, change); + } + @Test void get_policyResources() throws HttpResponseException { // Get list of policy resources and make sure it has all the entities and other resources @@ -364,6 +401,11 @@ public class PolicyResourceTest extends EntityResourceTest private static Rule accessControlRule(List resources, List operations, Effect effect) { String name = "rule" + new Random().nextInt(21); + return accessControlRule(name, resources, operations, effect); + } + + private static Rule accessControlRule( + String name, List resources, List operations, Effect effect) { return new Rule().withName(name).withResources(resources).withOperations(operations).withEffect(effect); } } diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/RoleResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/RoleResourceTest.java index 849918c1ae4..9d1274a01fc 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/RoleResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/teams/RoleResourceTest.java @@ -13,6 +13,7 @@ package org.openmetadata.catalog.resources.teams; +import static javax.ws.rs.core.Response.Status.BAD_REQUEST; import static javax.ws.rs.core.Response.Status.FORBIDDEN; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotEquals; @@ -43,9 +44,11 @@ import org.openmetadata.catalog.Entity; import org.openmetadata.catalog.api.teams.CreateRole; import org.openmetadata.catalog.entity.teams.Role; import org.openmetadata.catalog.entity.teams.User; +import org.openmetadata.catalog.exception.CatalogExceptionMessage; import org.openmetadata.catalog.resources.EntityResourceTest; import org.openmetadata.catalog.resources.teams.RoleResource.RoleList; import org.openmetadata.catalog.type.ChangeDescription; +import org.openmetadata.catalog.type.EntityReference; import org.openmetadata.catalog.type.FieldChange; import org.openmetadata.catalog.type.MetadataOperation; import org.openmetadata.catalog.util.JsonUtils; @@ -166,6 +169,41 @@ public class RoleResourceTest extends EntityResourceTest { permissionNotAllowed(TEST_USER_NAME, List.of(MetadataOperation.EDIT_DISPLAY_NAME))); } + @Test + void patch_rolePolicies(TestInfo test) throws IOException { + Role role = createEntity(createRequest(test), ADMIN_AUTH_HEADERS); + + // Add new DATA_STEWARD_POLICY to role in addition to DATA_CONSUMER_POLICY + String originalJson = JsonUtils.pojoToJson(role); + role.getPolicies().addAll(DATA_STEWARD_ROLE.getPolicies()); + ChangeDescription change = getChangeDescription(role.getVersion()); + change.getFieldsAdded().add(new FieldChange().withName("policies").withNewValue(DATA_STEWARD_ROLE.getPolicies())); + role = patchEntityAndCheck(role, originalJson, ADMIN_AUTH_HEADERS, MINOR_UPDATE, change); + + // Remove new DATA_CONSUMER_POLICY + originalJson = JsonUtils.pojoToJson(role); + role.setPolicies(DATA_STEWARD_ROLE.getPolicies()); + change = getChangeDescription(role.getVersion()); + change + .getFieldsDeleted() + .add(new FieldChange().withName("policies").withOldValue(DATA_CONSUMER_ROLE.getPolicies())); + role = patchEntityAndCheck(role, originalJson, ADMIN_AUTH_HEADERS, MINOR_UPDATE, change); + + // Remove all the policies. It should be disallowed + final String originalJson1 = JsonUtils.pojoToJson(role); + final UUID id = role.getId(); + final Role role1 = role; + role1.setPolicies(null); + change = getChangeDescription(role1.getVersion()); + change + .getFieldsDeleted() + .add(new FieldChange().withName("policies").withOldValue(DATA_CONSUMER_ROLE.getPolicies())); + assertResponse( + () -> patchEntity(id, originalJson1, role1, ADMIN_AUTH_HEADERS), + BAD_REQUEST, + CatalogExceptionMessage.EMPTY_POLICIES_IN_ROLE); + } + private static void validateRole( Role role, String expectedDescription, String expectedDisplayName, String expectedUpdatedBy) { assertListNotNull(role.getId(), role.getHref()); @@ -240,6 +278,15 @@ public class RoleResourceTest extends EntityResourceTest { @Override public void assertFieldChange(String fieldName, Object expected, Object actual) throws IOException { - assertCommonFieldChange(fieldName, expected, actual); + if (expected == actual) { + return; + } + if (fieldName.equals("policies")) { + List expectedRefs = (List) expected; + List actualRefs = JsonUtils.readObjects(actual.toString(), EntityReference.class); + assertEntityReferences(expectedRefs, actualRefs); + } else { + assertCommonFieldChange(fieldName, expected, actual); + } } }