From 8a008022c8d3ac9383a294f1d87cb15413070213 Mon Sep 17 00:00:00 2001 From: Suresh Srinivas Date: Tue, 1 Nov 2022 12:46:18 -0700 Subject: [PATCH] Fixes #7004 Backend : conditionAllow should not be applicable to create operation (#8483) --- .../java/org/openmetadata/service/Entity.java | 1 + .../service/ResourceRegistry.java | 4 +- .../service/jdbi3/EntityRepository.java | 4 +- .../service/resources/CollectionRegistry.java | 10 + .../service/resources/EntityResource.java | 14 + .../analytics/WebAnalyticEventResource.java | 2 +- .../service/resources/bots/BotResource.java | 2 +- .../dqtests/TestDefinitionResource.java | 2 +- .../resources/events/WebhookResource.java | 2 +- .../resources/policies/PolicyResource.java | 20 +- .../IngestionPipelineResource.java | 1 + .../service/resources/teams/RoleResource.java | 2 +- .../service/resources/teams/TeamResource.java | 2 +- .../service/resources/teams/UserResource.java | 1 + .../service/resources/types/TypeResource.java | 2 +- .../policyevaluator/CompiledRule.java | 2 +- .../policyevaluator/OperationContext.java | 22 +- .../openmetadata/service/util/JsonUtils.java | 6 + .../json/data/ResourceDescriptors.json | 45 +-- .../json/data/policy/OrganizationPolicy.json | 2 +- .../permissions/PermissionsResourceTest.java | 287 +++++++++--------- 21 files changed, 236 insertions(+), 197 deletions(-) diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/Entity.java b/openmetadata-service/src/main/java/org/openmetadata/service/Entity.java index 3a49e27d89b..e4dce36f7cc 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/Entity.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/Entity.java @@ -124,6 +124,7 @@ public final class Entity { // public static final String ADMIN_USER_NAME = "admin"; public static final String ORGANIZATION_NAME = "Organization"; + public static final String ORGANIZATION_POLICY_NAME = "OrganizationPolicy"; public static final String INGESTION_BOT_NAME = "ingestion-bot"; public static final String INGESTION_BOT_ROLE = "IngestionBotRole"; public static final String PROFILER_BOT_NAME = "profiler-bot"; diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/ResourceRegistry.java b/openmetadata-service/src/main/java/org/openmetadata/service/ResourceRegistry.java index 47d138cf789..df681e65f28 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/ResourceRegistry.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/ResourceRegistry.java @@ -12,7 +12,9 @@ public class ResourceRegistry { private ResourceRegistry() {} - public static void add(List resourceDescriptors) { + public static void initialize(List resourceDescriptors) { + RESOURCE_DESCRIPTORS.clear(); + ; RESOURCE_DESCRIPTORS.addAll(resourceDescriptors); RESOURCE_DESCRIPTORS.sort(Comparator.comparing(ResourceDescriptor::getName)); } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java index f07a6eee9eb..5bef9d9fb3c 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/jdbi3/EntityRepository.java @@ -142,10 +142,10 @@ public abstract class EntityRepository { protected final boolean supportsFollower; /** Fields that can be updated during PATCH operation */ - private final Fields patchFields; + @Getter private final Fields patchFields; /** Fields that can be updated during PUT operation */ - protected final Fields putFields; + @Getter protected final Fields putFields; EntityRepository( String collectionPath, diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/CollectionRegistry.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/CollectionRegistry.java index cf71be0005e..0a6d1485931 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/CollectionRegistry.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/CollectionRegistry.java @@ -280,6 +280,16 @@ public final class CollectionRegistry { } catch (Exception ex) { LOG.warn("Encountered exception ", ex); } + + // Call upgrade method, if it exists + try { + Method upgradeMethod = resource.getClass().getMethod("upgrade"); + upgradeMethod.invoke(resource); + } catch (NoSuchMethodException ignored) { + // Method does not exist and initialize is not called + } catch (Exception ex) { + LOG.warn("Encountered exception ", ex); + } return resource; } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/EntityResource.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/EntityResource.java index d38a4bca2dd..22445743468 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/EntityResource.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/EntityResource.java @@ -20,6 +20,7 @@ import org.openmetadata.schema.type.EntityReference; import org.openmetadata.schema.type.Include; import org.openmetadata.schema.type.MetadataOperation; import org.openmetadata.service.Entity; +import org.openmetadata.service.OpenMetadataApplicationConfig; import org.openmetadata.service.jdbi3.EntityRepository; import org.openmetadata.service.jdbi3.ListFilter; import org.openmetadata.service.security.Authorizer; @@ -50,6 +51,19 @@ public abstract class EntityResource webAnalyticEvents = diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/bots/BotResource.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/bots/BotResource.java index 98332774457..ba749f37160 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/bots/BotResource.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/bots/BotResource.java @@ -85,7 +85,7 @@ public class BotResource extends EntityResource { super(Bot.class, new BotRepository(dao), authorizer); } - @SuppressWarnings("unused") // Method is used by reflection + @Override public void initialize(OpenMetadataApplicationConfig config) throws IOException { // Load system bots List bots = dao.getEntitiesFromSeedData(); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestDefinitionResource.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestDefinitionResource.java index 04e1ab80f77..25fc7167252 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestDefinitionResource.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/dqtests/TestDefinitionResource.java @@ -74,7 +74,7 @@ public class TestDefinitionResource extends EntityResource testDefinitions = dao.getEntitiesFromSeedData(".*json/data/tests/.*\\.json$"); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/events/WebhookResource.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/events/WebhookResource.java index 1c8895fdf74..20062165532 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/events/WebhookResource.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/events/WebhookResource.java @@ -89,7 +89,7 @@ public class WebhookResource extends EntityResource webhookDAO = dao.webhookDAO(); } - @SuppressWarnings("unused") // Method used for reflection + @Override public void initialize(OpenMetadataApplicationConfig config) { try { List listAllWebhooks = webhookDAO.listAllWebhooks(webhookDAO.getTableName()); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/policies/PolicyResource.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/policies/PolicyResource.java index 82b24c2c07e..8cfe5260217 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/policies/PolicyResource.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/policies/PolicyResource.java @@ -55,6 +55,7 @@ import org.openmetadata.schema.type.EntityHistory; import org.openmetadata.schema.type.EntityReference; import org.openmetadata.schema.type.Function; import org.openmetadata.schema.type.Include; +import org.openmetadata.schema.type.MetadataOperation; import org.openmetadata.schema.type.ResourceDescriptor; import org.openmetadata.service.Entity; import org.openmetadata.service.FunctionList; @@ -95,11 +96,26 @@ public class PolicyResource extends EntityResource { super(Policy.class, new PolicyRepository(dao), authorizer); } - @SuppressWarnings("unused") // Method is used by reflection + @Override public void initialize(OpenMetadataApplicationConfig config) throws IOException { // Load any existing rules from database, before loading seed data. dao.initSeedDataFromResources(); - ResourceRegistry.add(listOrEmpty(PolicyResource.getResourceDescriptors())); + ResourceRegistry.initialize(listOrEmpty(PolicyResource.getResourceDescriptors())); + } + + @Override + public void upgrade() throws IOException { + // OrganizationPolicy rule change + Policy originalOrgPolicy = dao.getByName(null, Entity.ORGANIZATION_POLICY_NAME, dao.getPatchFields()); + Policy updatedOrgPolicy = JsonUtils.readValue(JsonUtils.pojoToJson(originalOrgPolicy), Policy.class); + + // Rules are in alphabetical order - change second rule "OrganizationPolicy-Owner-Rule" + // from ALL operation to remove CREATE operation and allow all the other operations for the owner + updatedOrgPolicy + .getRules() + .get(1) + .withOperations(List.of(MetadataOperation.EDIT_ALL, MetadataOperation.VIEW_ALL, MetadataOperation.DELETE)); + dao.patch(null, originalOrgPolicy.getId(), "admin", JsonUtils.getJsonPatch(originalOrgPolicy, updatedOrgPolicy)); } public static class PolicyList extends ResultList { diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/services/ingestionpipelines/IngestionPipelineResource.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/services/ingestionpipelines/IngestionPipelineResource.java index 9b6e5e16909..32dbe88ca12 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/services/ingestionpipelines/IngestionPipelineResource.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/services/ingestionpipelines/IngestionPipelineResource.java @@ -99,6 +99,7 @@ public class IngestionPipelineResource extends EntityResource { super(Role.class, new RoleRepository(collectionDAO), authorizer); } - @SuppressWarnings("unused") // Method used for reflection + @Override public void initialize(OpenMetadataApplicationConfig config) throws IOException { List roles = dao.getEntitiesFromSeedData(); for (Role role : roles) { diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/teams/TeamResource.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/teams/TeamResource.java index 7c665386ea2..9987f0d3117 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/teams/TeamResource.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/teams/TeamResource.java @@ -90,7 +90,7 @@ public class TeamResource extends EntityResource { super(Team.class, new TeamRepository(dao), authorizer); } - @SuppressWarnings("unused") // Method used for reflection + @Override public void initialize(OpenMetadataApplicationConfig config) throws IOException { dao.initOrganization(); } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/teams/UserResource.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/teams/UserResource.java index 7c4778c0e70..e0797f424c2 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/teams/UserResource.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/teams/UserResource.java @@ -150,6 +150,7 @@ public class UserResource extends EntityResource { authHandler = authenticatorHandler; } + @Override public void initialize(OpenMetadataApplicationConfig config) throws IOException { this.authenticationConfiguration = config.getAuthenticationConfiguration(); SmtpSettings smtpSettings = config.getSmtpSettings(); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/resources/types/TypeResource.java b/openmetadata-service/src/main/java/org/openmetadata/service/resources/types/TypeResource.java index 5ca885a6eb0..f6df79bb7cd 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/resources/types/TypeResource.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/resources/types/TypeResource.java @@ -91,7 +91,7 @@ public class TypeResource extends EntityResource { super(Type.class, new TypeRepository(dao), authorizer); } - @SuppressWarnings("unused") // Method used for reflection + @Override public void initialize(OpenMetadataApplicationConfig config) throws IOException { // Load types defined in OpenMetadata schemas long now = System.currentTimeMillis(); diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/CompiledRule.java b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/CompiledRule.java index 613cf5de568..9b020af857a 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/CompiledRule.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/CompiledRule.java @@ -219,7 +219,7 @@ public class CompiledRule extends Rule { return Boolean.TRUE.equals(expression.getValue(evaluationContext, Boolean.class)); } - static boolean overrideAccess(Access newAccess, Access currentAccess) { + public static boolean overrideAccess(Access newAccess, Access currentAccess) { // Lower the ordinal number of access overrides higher ordinal number return currentAccess.ordinal() > newAccess.ordinal(); } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/OperationContext.java b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/OperationContext.java index fb8108a4bc7..7ae50f8d6ea 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/OperationContext.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/security/policyevaluator/OperationContext.java @@ -39,8 +39,8 @@ public class OperationContext { return operations; } - public boolean isCreateOperation() { - return operations.contains(MetadataOperation.CREATE); + public static List getAllOperations(MetadataOperation... exclude) { + return getOperations("", exclude); } public static boolean isEditOperation(MetadataOperation operation) { @@ -50,4 +50,22 @@ public class OperationContext { public static boolean isViewOperation(MetadataOperation operation) { return operation.value().startsWith("View"); } + + public static List getViewOperations(MetadataOperation... exclude) { + return getOperations("View", exclude); + } + + private static List getOperations(String startsWith, MetadataOperation... exclude) { + List list = new ArrayList<>(); + List excludeList = new ArrayList<>(List.of(exclude)); + if (!excludeList.isEmpty()) { + excludeList.add(MetadataOperation.ALL); // If any operation is excluded then 'All' operation is excluded + } + for (MetadataOperation operation : MetadataOperation.values()) { + if (!excludeList.contains(operation) && operation.value().startsWith(startsWith)) { + list.add(operation); + } + } + return list; + } } diff --git a/openmetadata-service/src/main/java/org/openmetadata/service/util/JsonUtils.java b/openmetadata-service/src/main/java/org/openmetadata/service/util/JsonUtils.java index 11ea0fb8348..10dbe90f448 100644 --- a/openmetadata-service/src/main/java/org/openmetadata/service/util/JsonUtils.java +++ b/openmetadata-service/src/main/java/org/openmetadata/service/util/JsonUtils.java @@ -274,6 +274,12 @@ public final class JsonUtils { return Json.createDiff(source.asJsonObject(), dest.asJsonObject()); } + public static JsonPatch getJsonPatch(Object v1, Object v2) throws JsonProcessingException { + JsonValue source = readJson(JsonUtils.pojoToJson(v1)); + JsonValue dest = readJson(JsonUtils.pojoToJson(v2)); + return Json.createDiff(source.asJsonObject(), dest.asJsonObject()); + } + public static JsonValue readJson(String s) { try (JsonReader reader = Json.createReader(new StringReader(s))) { return reader.readValue(); diff --git a/openmetadata-service/src/main/resources/json/data/ResourceDescriptors.json b/openmetadata-service/src/main/resources/json/data/ResourceDescriptors.json index 035488fb4e1..cf06ee49db6 100644 --- a/openmetadata-service/src/main/resources/json/data/ResourceDescriptors.json +++ b/openmetadata-service/src/main/resources/json/data/ResourceDescriptors.json @@ -367,18 +367,6 @@ "EditCustomFields" ] }, - { - "name" : "type", - "operations" : [ - "Create", - "Delete", - "ViewAll", - "EditAll", - "EditDescription", - "EditDisplayName", - "EditCustomFields" - ] - }, { "name" : "user", "operations" : [ @@ -441,6 +429,17 @@ "ViewAll" ] }, + { + "name" : "type", + "operations" : [ + "Create", + "Delete", + "ViewAll", + "EditAll", + "EditDescription", + "EditDisplayName" + ] + }, { "name" : "webAnalyticEvent", "operations" : [ @@ -451,27 +450,5 @@ "EditDescription", "EditDisplayName" ] - }, - { - "name" : "type", - "operations" : [ - "Create", - "Delete", - "ViewAll", - "EditAll", - "EditDescription", - "EditDisplayName" - ] - }, - { - "name" : "type", - "operations" : [ - "Create", - "Delete", - "ViewAll", - "EditAll", - "EditDescription", - "EditDisplayName" - ] } ] \ No newline at end of file diff --git a/openmetadata-service/src/main/resources/json/data/policy/OrganizationPolicy.json b/openmetadata-service/src/main/resources/json/data/policy/OrganizationPolicy.json index c4cb55adaf3..a3a45dd51bf 100644 --- a/openmetadata-service/src/main/resources/json/data/policy/OrganizationPolicy.json +++ b/openmetadata-service/src/main/resources/json/data/policy/OrganizationPolicy.json @@ -10,7 +10,7 @@ "name": "OrganizationPolicy-Owner-Rule", "description" : "Allow all the operations on an entity for the owner.", "resources" : ["all"], - "operations": ["All"], + "operations": ["EditAll", "ViewAll", "Delete"], "effect": "allow", "condition": "isOwner()" }, 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 267116b045f..32b91a653d1 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 @@ -15,34 +15,29 @@ package org.openmetadata.service.resources.permissions; import static javax.ws.rs.core.Response.Status.FORBIDDEN; import static org.junit.jupiter.api.Assertions.assertEquals; -import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.openmetadata.schema.type.MetadataOperation.ALL; +import static org.openmetadata.schema.type.MetadataOperation.CREATE; import static org.openmetadata.schema.type.MetadataOperation.EDIT_DESCRIPTION; import static org.openmetadata.schema.type.MetadataOperation.EDIT_DISPLAY_NAME; import static org.openmetadata.schema.type.MetadataOperation.EDIT_LINEAGE; import static org.openmetadata.schema.type.MetadataOperation.EDIT_OWNER; import static org.openmetadata.schema.type.MetadataOperation.EDIT_TAGS; -import static org.openmetadata.schema.type.MetadataOperation.VIEW_ALL; -import static org.openmetadata.schema.type.MetadataOperation.VIEW_BASIC; -import static org.openmetadata.schema.type.MetadataOperation.VIEW_DATA_PROFILE; -import static org.openmetadata.schema.type.MetadataOperation.VIEW_QUERIES; -import static org.openmetadata.schema.type.MetadataOperation.VIEW_SAMPLE_DATA; -import static org.openmetadata.schema.type.MetadataOperation.VIEW_TESTS; -import static org.openmetadata.schema.type.MetadataOperation.VIEW_USAGE; import static org.openmetadata.schema.type.Permission.Access.ALLOW; import static org.openmetadata.schema.type.Permission.Access.CONDITIONAL_ALLOW; import static org.openmetadata.schema.type.Permission.Access.NOT_ALLOW; +import static org.openmetadata.service.security.policyevaluator.OperationContext.getAllOperations; +import static org.openmetadata.service.security.policyevaluator.OperationContext.getViewOperations; import static org.openmetadata.service.util.TestUtils.ADMIN_AUTH_HEADERS; import static org.openmetadata.service.util.TestUtils.assertResponse; import java.io.IOException; import java.net.URISyntaxException; import java.util.ArrayList; -import java.util.Collections; +import java.util.Comparator; import java.util.List; import java.util.Map; import java.util.UUID; import javax.ws.rs.client.WebTarget; +import lombok.Getter; import lombok.extern.slf4j.Slf4j; import org.apache.http.client.HttpResponseException; import org.junit.jupiter.api.BeforeAll; @@ -69,6 +64,7 @@ import org.openmetadata.service.resources.permissions.PermissionsResource.Resour import org.openmetadata.service.resources.policies.PolicyResource; import org.openmetadata.service.resources.policies.PolicyResourceTest; import org.openmetadata.service.security.SecurityUtil; +import org.openmetadata.service.security.policyevaluator.CompiledRule; import org.openmetadata.service.security.policyevaluator.PolicyEvaluator; import org.openmetadata.service.util.TestUtils; @@ -76,7 +72,10 @@ import org.openmetadata.service.util.TestUtils; @TestInstance(TestInstance.Lifecycle.PER_CLASS) class PermissionsResourceTest extends OpenMetadataApplicationTest { private static final String ORG_POLICY_NAME = "OrganizationPolicy"; - private static List ORG_RULES; + private static Rule ORG_IS_OWNER_RULE; + private static Rule ORG_NO_OWNER_RULE; + private static final List ORG_IS_OWNER_RULE_OPERATIONS = getAllOperations(CREATE); + private static final List ORG_NO_OWNER_RULE_OPERATIONS = List.of(EDIT_OWNER); private static final String DATA_STEWARD_ROLE_NAME = "DataSteward"; private static Policy DATA_STEWARD_POLICY; @@ -87,23 +86,16 @@ class PermissionsResourceTest extends OpenMetadataApplicationTest { private static Policy DATA_CONSUMER_POLICY; private static final String DATA_CONSUMER_POLICY_NAME = "DataConsumerPolicy"; private static List DATA_CONSUMER_RULES; - private static List DATA_CONSUMER_ALLOWED = - List.of( - VIEW_ALL, - VIEW_BASIC, - VIEW_USAGE, - VIEW_DATA_PROFILE, - VIEW_SAMPLE_DATA, - VIEW_BASIC, - VIEW_TESTS, - VIEW_QUERIES, - EDIT_DESCRIPTION, - EDIT_TAGS); - - private static List DATA_STEWARD_ALLOWED = new ArrayList<>(DATA_CONSUMER_ALLOWED); + private static final List DATA_CONSUMER_ALLOWED = getViewOperations(); static { - // Additional permissions over DATA_CONSUMER + DATA_CONSUMER_ALLOWED.addAll(List.of(EDIT_DESCRIPTION, EDIT_TAGS)); + } + + private static final List DATA_STEWARD_ALLOWED = new ArrayList<>(DATA_CONSUMER_ALLOWED); + + static { + // DATA_CONSUMER + additional operations DATA_STEWARD_ALLOWED.addAll(List.of(EDIT_OWNER, EDIT_DISPLAY_NAME, EDIT_LINEAGE)); } @@ -119,7 +111,10 @@ class PermissionsResourceTest extends OpenMetadataApplicationTest { Policy orgPolicy = policyResourceTest.getEntityByName(ORG_POLICY_NAME, null, PolicyResource.FIELDS, ADMIN_AUTH_HEADERS); - ORG_RULES = orgPolicy.getRules(); + List orgRules = orgPolicy.getRules(); + // Rules are alphabetically ordered + ORG_NO_OWNER_RULE = orgRules.get(0); + ORG_IS_OWNER_RULE = orgRules.get(1); DATA_STEWARD_POLICY = policyResourceTest.getEntityByName(DATA_STEWARD_POLICY_NAME, null, PolicyResource.FIELDS, ADMIN_AUTH_HEADERS); @@ -154,64 +149,79 @@ class PermissionsResourceTest extends OpenMetadataApplicationTest { @Test void get_dataConsumer_permissions() throws HttpResponseException { - // Ensure data consumer has permissions based on his role and the inherited roles - List conditional = List.of(ALL); // All operations are conditionally allowed - + // // Validate permissions for all resources as logged-in user + // Map authHeaders = SecurityUtil.authHeaders(DATA_CONSUMER_USER_NAME + "@open-metadata.org"); List actualPermissions = getPermissions(authHeaders); - assertDataConsumerPermissions(actualPermissions, conditional); + ResourcePermissionsBuilder permissionsBuilder = new ResourcePermissionsBuilder(); + permissionsBuilder.setPermission( + DATA_CONSUMER_ALLOWED, ALLOW, DATA_CONSUMER_ROLE_NAME, DATA_CONSUMER_POLICY_NAME, DATA_CONSUMER_RULES.get(0)); + // Set permissions based on alphabetical order of roles + permissionsBuilder.setPermission( + ORG_NO_OWNER_RULE_OPERATIONS, CONDITIONAL_ALLOW, null, ORG_POLICY_NAME, ORG_NO_OWNER_RULE); + permissionsBuilder.setPermission( + ORG_IS_OWNER_RULE_OPERATIONS, CONDITIONAL_ALLOW, null, ORG_POLICY_NAME, ORG_IS_OWNER_RULE); + assertResourcePermissions(permissionsBuilder.getResourcePermissions(), actualPermissions); // Validate permissions for all resources for data consumer as admin actualPermissions = getPermissions(DATA_CONSUMER_USER_NAME, ADMIN_AUTH_HEADERS); - assertDataConsumerPermissions(actualPermissions, conditional); + assertResourcePermissions(permissionsBuilder.getResourcePermissions(), actualPermissions); // Validate permission as logged-in user for each resource one at a time + ResourcePermission actualPermission; for (ResourceDescriptor rd : ResourceRegistry.listResourceDescriptors()) { - ResourcePermission actualPermission = getPermission(rd.getName(), null, authHeaders); - assertDataConsumerPermission(actualPermission, conditional); + actualPermission = getPermission(rd.getName(), null, authHeaders); + assertResourcePermission(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()) { - ResourcePermission actualPermission = getPermission(rd.getName(), DATA_CONSUMER_USER_NAME, authHeaders); - assertDataConsumerPermission(actualPermission, conditional); + actualPermission = getPermission(rd.getName(), DATA_CONSUMER_USER_NAME, authHeaders); + assertResourcePermission(permissionsBuilder.getPermission(rd.getName()), actualPermission); } - // - // Test getting permissions for an entity as an owner - // - // Create an entity with data consumer as owner - TableResourceTest tableResourceTest = new TableResourceTest(); - CreateTable createTable = - tableResourceTest.createRequest("permissionTest").withOwner(DATA_CONSUMER_USER.getEntityReference()); - Table table1 = tableResourceTest.createEntity(createTable, ADMIN_AUTH_HEADERS); - - // Data consumer must have all operations allowed based on Organization policy as an owner - ResourcePermission actualPermission = getPermission(Entity.TABLE, table1.getId(), null, authHeaders); - assertAllOperationsAllowed(actualPermission); - - // get permissions by resource entity name - actualPermission = getPermissionByName(Entity.TABLE, table1.getFullyQualifiedName(), null, authHeaders); - assertAllOperationsAllowed(actualPermission); - - // Admin getting permissions for a specific resource on for Data consumer - actualPermission = getPermission(Entity.TABLE, table1.getId(), DATA_CONSUMER_USER_NAME, ADMIN_AUTH_HEADERS); - assertAllOperationsAllowed(actualPermission); - - // Create another table with a different owner and make sure data consumer does not have permission as non owner - createTable = tableResourceTest.createRequest("permissionTest1").withOwner(DATA_STEWARD_USER.getEntityReference()); - Table table2 = tableResourceTest.createEntity(createTable, ADMIN_AUTH_HEADERS); - // // Test getting permissions for an entity user does not own // + permissionsBuilder = new ResourcePermissionsBuilder(); + permissionsBuilder.setPermission( + DATA_CONSUMER_ALLOWED, ALLOW, DATA_CONSUMER_ROLE_NAME, DATA_CONSUMER_POLICY_NAME, DATA_CONSUMER_RULES.get(0)); + // Organization policy of no owner or isOwner does not apply. Hence, not added to expected permissions + + // Create a table with a different owner and make sure data consumer does not have permission as non owner + TableResourceTest tableResourceTest = new TableResourceTest(); + CreateTable createTable = + tableResourceTest.createRequest("permissionTest1").withOwner(DATA_STEWARD_USER.getEntityReference()); + Table table2 = tableResourceTest.createEntity(createTable, ADMIN_AUTH_HEADERS); + // Data consumer has non-owner permissions actualPermission = getPermission(Entity.TABLE, table2.getId(), null, authHeaders); // 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. - assertDataConsumerPermission(actualPermission, Collections.emptyList()); + assertResourcePermission(permissionsBuilder.getPermission(Entity.TABLE), actualPermission); + + // + // Test getting permissions for an entity as an owner - where ORG_POLICY isOwner becomes effective + // + + // Create an entity with data consumer as owner + createTable = tableResourceTest.createRequest("permissionTest").withOwner(DATA_CONSUMER_USER.getEntityReference()); + Table table1 = tableResourceTest.createEntity(createTable, ADMIN_AUTH_HEADERS); + + // Data consumer must have all operations except create allowed based on Organization policy as an owner + permissionsBuilder.setPermission(ORG_IS_OWNER_RULE_OPERATIONS, ALLOW, null, ORG_POLICY_NAME, ORG_IS_OWNER_RULE); + actualPermission = getPermission(Entity.TABLE, table1.getId(), null, authHeaders); + assertResourcePermission(permissionsBuilder.getPermission(Entity.TABLE), actualPermission); + + // get permissions by resource entity name + actualPermission = getPermissionByName(Entity.TABLE, table1.getFullyQualifiedName(), null, authHeaders); + assertResourcePermission(permissionsBuilder.getPermission(Entity.TABLE), actualPermission); + + // Admin getting permissions for a specific resource on for Data consumer + actualPermission = getPermission(Entity.TABLE, table1.getId(), DATA_CONSUMER_USER_NAME, ADMIN_AUTH_HEADERS); + assertResourcePermission(permissionsBuilder.getPermission(Entity.TABLE), actualPermission); } @Test @@ -219,9 +229,16 @@ class PermissionsResourceTest extends OpenMetadataApplicationTest { Map authHeaders = SecurityUtil.authHeaders(DATA_STEWARD_USER_NAME + "@open-metadata.org"); List actualPermissions = getPermissions(authHeaders); - for (ResourcePermission actualPermission : actualPermissions) { // For all resources - assertDataStewardPermission(actualPermission); - } + ResourcePermissionsBuilder permissionsBuilder = new ResourcePermissionsBuilder(); + permissionsBuilder.setPermission( + DATA_STEWARD_ALLOWED, ALLOW, DATA_STEWARD_ROLE_NAME, DATA_STEWARD_POLICY_NAME, DATA_STEWARD_RULES.get(0)); + // Set permissions based on alphabetical order of roles + permissionsBuilder.setPermission( + ORG_NO_OWNER_RULE_OPERATIONS, CONDITIONAL_ALLOW, null, ORG_POLICY_NAME, ORG_NO_OWNER_RULE); + permissionsBuilder.setPermission( + ORG_IS_OWNER_RULE_OPERATIONS, CONDITIONAL_ALLOW, null, ORG_POLICY_NAME, ORG_IS_OWNER_RULE); + + assertResourcePermissions(permissionsBuilder.getResourcePermissions(), actualPermissions); } @Test @@ -229,98 +246,41 @@ class PermissionsResourceTest extends OpenMetadataApplicationTest { // Get permissions for DATA_CONSUMER policy and assert it is correct List policies = new ArrayList<>(List.of(DATA_CONSUMER_POLICY.getId())); List actual = getPermissionsForPolicies(policies, ADMIN_AUTH_HEADERS); - for (ResourcePermission actualPermission : actual) { // For every resource - for (Permission permission : actualPermission.getPermissions()) { - if (DATA_CONSUMER_ALLOWED.contains(permission.getOperation())) { - assertPermissionAllowed(permission, null, DATA_CONSUMER_POLICY_NAME, DATA_CONSUMER_RULES); - } else { - assertPermissionNotAllowed(permission); - } - } - } + ResourcePermissionsBuilder permissionsBuilder = new ResourcePermissionsBuilder(); + permissionsBuilder.setPermission( + DATA_CONSUMER_ALLOWED, ALLOW, null, DATA_CONSUMER_POLICY_NAME, DATA_CONSUMER_RULES.get(0)); + + assertResourcePermissions(permissionsBuilder.getResourcePermissions(), actual); // Get permissions for DATA_CONSUMER and DATA_STEWARD policies and assert it is correct policies.add(DATA_STEWARD_POLICY.getId()); actual = getPermissionsForPolicies(policies, ADMIN_AUTH_HEADERS); - for (ResourcePermission actualPermission : actual) { // For every resource - for (Permission permission : actualPermission.getPermissions()) { - if (DATA_CONSUMER_ALLOWED.contains(permission.getOperation())) { - assertPermissionAllowed(permission, null, DATA_CONSUMER_POLICY_NAME, DATA_CONSUMER_RULES); - } else if (DATA_STEWARD_ALLOWED.contains(permission.getOperation())) { - assertPermissionAllowed(permission, null, DATA_STEWARD_POLICY_NAME, DATA_STEWARD_RULES); - } else { - assertPermissionNotAllowed(permission); - } - } + permissionsBuilder.setPermission( + DATA_STEWARD_ALLOWED, ALLOW, null, DATA_STEWARD_POLICY_NAME, DATA_STEWARD_RULES.get(0)); + assertResourcePermissions(permissionsBuilder.getResourcePermissions(), actual); + } + + private void assertResourcePermissions(List expected, List actual) { + assertEquals(expected.size(), actual.size()); + Comparator resourcePermissionComparator = Comparator.comparing(ResourcePermission::getResource); + expected.sort(resourcePermissionComparator); + actual.sort(resourcePermissionComparator); + for (int i = 0; i < expected.size(); i++) { + assertResourcePermission(expected.get(i), actual.get(i)); } } - private void assertDataStewardPermission(ResourcePermission actualPermission) { - // Only allowed operations in DataConsumerRole. All other operations are conditionalAllow by default - for (Permission permission : actualPermission.getPermissions()) { - if (DATA_STEWARD_ALLOWED.contains(permission.getOperation())) { - assertPermissionAllowed(permission, DATA_STEWARD_ROLE_NAME, DATA_STEWARD_POLICY_NAME, DATA_STEWARD_RULES); - } else { - assertPermissionConditional(permission, null, ORG_POLICY_NAME, ORG_RULES); - } + private void assertResourcePermission(ResourcePermission expected, ResourcePermission actual) { + assertEquals(expected.getPermissions().size(), actual.getPermissions().size()); + Comparator operationComparator = Comparator.comparing(Permission::getOperation); + expected.getPermissions().sort(operationComparator); + actual.getPermissions().sort(operationComparator); + for (int i = 0; i < expected.getPermissions().size(); i++) { + // Using for loop to compare instead of equals to help with debugging the difference between the lists + assertEquals(expected.getPermissions().get(i), actual.getPermissions().get(i)); } } - private void assertAllOperationsAllowed(ResourcePermission actualPermission) { - assertEquals(Entity.TABLE, actualPermission.getResource()); - for (Permission permission : actualPermission.getPermissions()) { - assertEquals(ALLOW, permission.getAccess()); - assertTrue(List.of(ORG_POLICY_NAME, DATA_CONSUMER_POLICY_NAME).contains(permission.getPolicy())); - } - } - - private void assertDataConsumerPermissions( - List actualPermissions, List conditional) { - // Only allowed operations in DataConsumerRole. All other operations are notAllow by default - for (ResourcePermission actualPermission : actualPermissions) { // For every resource - assertDataConsumerPermission(actualPermission, conditional); // assert permission - } - } - - private void assertDataConsumerPermission(ResourcePermission actualPermission, List conditional) { - // Only allowed operations in DataConsumerRole. All other operations are conditional allow or not allow - for (Permission permission : actualPermission.getPermissions()) { - if (DATA_CONSUMER_ALLOWED.contains(permission.getOperation())) { - assertPermissionAllowed(permission, DATA_CONSUMER_ROLE_NAME, DATA_CONSUMER_POLICY_NAME, DATA_CONSUMER_RULES); - } else if (conditional.contains(permission.getOperation()) || conditional.contains(ALL)) { - assertPermissionConditional(permission, null, ORG_POLICY_NAME, ORG_RULES); - } else { - assertPermissionNotAllowed(permission); - } - } - } - - private void assertPermissionAllowed( - Permission permission, String expectedRole, String expectedPolicy, List expectedRules) { - assertPermission(permission, ALLOW, expectedRole, expectedPolicy, expectedRules); - } - - private void assertPermissionConditional( - Permission permission, String expectedRole, String expectedPolicy, List expectedRules) { - assertPermission(permission, CONDITIONAL_ALLOW, expectedRole, expectedPolicy, expectedRules); - } - - private void assertPermissionNotAllowed(Permission permission) { - assertPermission(permission, NOT_ALLOW, null, null, null); - } - - private void assertPermission( - Permission permission, - Access expectedAccess, - String expectedRole, - String expectedPolicy, - List expectedRules) { - assertEquals(expectedAccess, permission.getAccess(), permission.toString()); - assertEquals(expectedRole, permission.getRole(), permission.toString()); - assertEquals(expectedPolicy, permission.getPolicy(), permission.toString()); - assertTrue(expectedRules == null || expectedRules.contains(permission.getRule())); - } - public List getPermissions(Map authHeaders) throws HttpResponseException { WebTarget target = getResource("permissions"); return TestUtils.get(target, ResourcePermissionList.class, authHeaders).getData(); @@ -362,4 +322,37 @@ class PermissionsResourceTest extends OpenMetadataApplicationTest { } return TestUtils.get(target, ResourcePermissionList.class, authHeaders).getData(); } + + /** Build permissions based on role, policies, etc. for testing purposes */ + public static class ResourcePermissionsBuilder { + @Getter + private final List resourcePermissions = PolicyEvaluator.getResourcePermissions(NOT_ALLOW); + + public void setPermission( + List operations, Access access, String role, String policy, Rule rule) { + resourcePermissions.forEach(rp -> setPermission(rp, operations, access, role, policy, rule)); + } + + public ResourcePermission getPermission(String resource) { + return resourcePermissions.stream() + .filter(resourcePermission -> resourcePermission.getResource().equals(resource)) + .findAny() + .orElse(null); + } + + private void setPermission( + ResourcePermission resourcePermission, + List operations, + Access access, + String role, + String policy, + Rule rule) { + for (Permission permission : resourcePermission.getPermissions()) { + if (operations.contains(permission.getOperation()) + && CompiledRule.overrideAccess(access, permission.getAccess())) { + permission.withAccess(access).withRole(role).withPolicy(policy).withRule(rule); + } + } + } + } }