From f70755f0cca6911d3e6896282bcbfbf4eeec02c6 Mon Sep 17 00:00:00 2001 From: Matt Date: Wed, 19 Jan 2022 08:49:01 -0800 Subject: [PATCH] Make PolicyEvaluator a Singleton (#2281) - Make PolicyEvaluator a Singleton - Add functionality to refreshRules when new Access Control Policy is stored - Remove poor-Matt's time based synchronization logic from DefaultAuthorizer and EntityResourceTest - Delete old PolicyEvaluatorTest, since it is redundant with existing *ResourceTest --- .../catalog/jdbi3/PolicyRepository.java | 8 + .../catalog/security/DefaultAuthorizer.java | 32 +--- .../policyevaluator/PolicyEvaluator.java | 50 ++++-- .../catalog/resources/EntityResourceTest.java | 8 - .../policyevaluator/PolicyEvaluatorTest.java | 142 ------------------ 5 files changed, 50 insertions(+), 190 deletions(-) delete mode 100644 catalog-rest-service/src/test/java/org/openmetadata/catalog/security/policyevaluator/PolicyEvaluatorTest.java 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 3617f335971..0285fbdb650 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 @@ -31,6 +31,7 @@ import org.openmetadata.catalog.entity.policies.Policy; import org.openmetadata.catalog.entity.policies.accessControl.Rule; import org.openmetadata.catalog.exception.CatalogExceptionMessage; import org.openmetadata.catalog.resources.policies.PolicyResource; +import org.openmetadata.catalog.security.policyevaluator.PolicyEvaluator; import org.openmetadata.catalog.type.ChangeDescription; import org.openmetadata.catalog.type.EntityReference; import org.openmetadata.catalog.type.Include; @@ -48,6 +49,8 @@ public class PolicyRepository extends EntityRepository { new Fields(PolicyResource.FIELD_LIST, "displayName,description,owner,policyUrl,enabled,rules,location"); public static final String ENABLED = "enabled"; + private PolicyEvaluator policyEvaluator; + public PolicyRepository(CollectionDAO dao) { super( PolicyResource.COLLECTION_PATH, @@ -60,6 +63,7 @@ public class PolicyRepository extends EntityRepository { false, true, false); + policyEvaluator = PolicyEvaluator.getInstance(); } public static String getFQN(Policy policy) { @@ -149,6 +153,10 @@ public class PolicyRepository extends EntityRepository { policy.withOwner(null).withLocation(null).withHref(null); store(policy.getId(), policy, update); + if (PolicyType.AccessControl.equals(policy.getPolicyType())) { + // Refresh rules in PolicyEvaluator right after an Access Control policy has been stored. + policyEvaluator.refreshRules(); + } // Restore the relationships policy.withOwner(owner).withLocation(location).withHref(href); diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/security/DefaultAuthorizer.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/security/DefaultAuthorizer.java index dd743f4f379..2c88540a10e 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/security/DefaultAuthorizer.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/security/DefaultAuthorizer.java @@ -21,9 +21,6 @@ import java.util.HashSet; import java.util.List; import java.util.Set; import java.util.UUID; -import java.util.concurrent.Executors; -import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.TimeUnit; import org.apache.commons.lang3.exception.ExceptionUtils; import org.jdbi.v3.core.Jdbi; import org.openmetadata.catalog.Entity; @@ -47,16 +44,11 @@ public class DefaultAuthorizer implements Authorizer { private Set botUsers; private String principalDomain; - private CollectionDAO collectionDAO; private UserRepository userRepository; - // policyEvaluator has to be thread-safe. A background thread will be updating the value. - private volatile PolicyEvaluator policyEvaluator; + private PolicyEvaluator policyEvaluator; private static final String fieldsParam = "roles,teams"; - public static final int POLICY_LOADER_INITIAL_DELAY = 5; // seconds. - private static final int POLICY_LOADER_SCHEDULE_INTERVAL = 300; // seconds. - @Override public void init(AuthorizerConfiguration config, Jdbi dbi) throws IOException { LOG.debug("Initializing DefaultAuthorizer with config {}", config); @@ -64,28 +56,12 @@ public class DefaultAuthorizer implements Authorizer { this.botUsers = new HashSet<>(config.getBotPrincipals()); this.principalDomain = config.getPrincipalDomain(); LOG.debug("Admin users: {}", adminUsers); - this.collectionDAO = dbi.onDemand(CollectionDAO.class); + CollectionDAO collectionDAO = dbi.onDemand(CollectionDAO.class); this.userRepository = new UserRepository(collectionDAO); mayBeAddAdminUsers(); mayBeAddBotUsers(); - - // Use a 15-min schedule to refresh policies. This should be replaced by a better solution which can load policies - // only when a policy change event occurs. - ScheduledExecutorService scheduleService = Executors.newSingleThreadScheduledExecutor(); - scheduleService.scheduleWithFixedDelay( - new PolicyLoader(), POLICY_LOADER_INITIAL_DELAY, POLICY_LOADER_SCHEDULE_INTERVAL, TimeUnit.SECONDS); - } - - private class PolicyLoader implements Runnable { - @Override - public void run() { - LOG.info("Loading access control policies for DefaultAuthorizer Policy Evaluator"); - try { - policyEvaluator = new PolicyEvaluator(new PolicyRepository(collectionDAO).getAccessControlPolicyRules()); - } catch (IOException e) { - LOG.warn("Could not update access control policies for DefaultAuthorizer Policy Evaluator: {}", e.getMessage()); - } - } + policyEvaluator = PolicyEvaluator.getInstance(); + policyEvaluator.setPolicyRepository(new PolicyRepository(collectionDAO)); } private void mayBeAddAdminUsers() { diff --git a/catalog-rest-service/src/main/java/org/openmetadata/catalog/security/policyevaluator/PolicyEvaluator.java b/catalog-rest-service/src/main/java/org/openmetadata/catalog/security/policyevaluator/PolicyEvaluator.java index aedce54b6ec..7525ce459b6 100644 --- a/catalog-rest-service/src/main/java/org/openmetadata/catalog/security/policyevaluator/PolicyEvaluator.java +++ b/catalog-rest-service/src/main/java/org/openmetadata/catalog/security/policyevaluator/PolicyEvaluator.java @@ -1,6 +1,8 @@ package org.openmetadata.catalog.security.policyevaluator; -import java.util.List; +import java.io.IOException; +import java.util.concurrent.atomic.AtomicReference; +import lombok.extern.slf4j.Slf4j; import org.jeasy.rules.api.Rule; import org.jeasy.rules.api.Rules; import org.jeasy.rules.api.RulesEngine; @@ -8,13 +10,14 @@ import org.jeasy.rules.api.RulesEngineParameters; import org.jeasy.rules.core.DefaultRulesEngine; import org.jeasy.rules.core.RuleBuilder; import org.openmetadata.catalog.entity.teams.User; +import org.openmetadata.catalog.jdbi3.PolicyRepository; import org.openmetadata.catalog.type.MetadataOperation; /** * PolicyEvaluator for {@link MetadataOperation metadata operations} based on OpenMetadata's internal {@link * org.openmetadata.catalog.entity.policies.Policy} format to make access decisions. * - *

This class uses {@link DefaultRulesEngine} provided by This Singleton class uses {@link DefaultRulesEngine} provided by j-easy/easy-rules package. * *

The rules defined as {@link org.openmetadata.catalog.entity.policies.accessControl.Rule} are to be fetched from @@ -29,21 +32,44 @@ import org.openmetadata.catalog.type.MetadataOperation; * *

- {@link org.openmetadata.catalog.Entity} (object) on which to operate on. */ +@Slf4j public class PolicyEvaluator { - private final Rules rules; - private final RulesEngine rulesEngine; + private PolicyRepository policyRepository; + private AtomicReference rules; + private RulesEngine rulesEngine; - public PolicyEvaluator(List rules) { - this.rules = new Rules(); - rules.stream() + // Eager initialization of Singleton since PolicyEvaluator is lightweight. + private static final PolicyEvaluator policyEvaluator = new PolicyEvaluator(); + + private PolicyEvaluator() { + RulesEngineParameters parameters = + new RulesEngineParameters().skipOnFirstAppliedRule(true); // When first rule applies, stop the matching. + rulesEngine = new DefaultRulesEngine(parameters); + // Initialize with empty set of rules. If not, the RulesEngine would throw NullPointerException. + rules = new AtomicReference<>(new Rules()); + } + + public static PolicyEvaluator getInstance() { + return policyEvaluator; + } + + public void setPolicyRepository(PolicyRepository policyRepository) { + this.policyRepository = policyRepository; + } + + /** Refresh rules within {@link PolicyEvaluator} to be used by {@link DefaultRulesEngine}. */ + public void refreshRules() throws IOException { + log.warn("{} rules are available for Access Control", rules.get().size()); + Rules newRules = new Rules(); + policyRepository.getAccessControlPolicyRules().stream() // Add rules only if they are enabled. .filter(org.openmetadata.catalog.entity.policies.accessControl.Rule::getEnabled) .map(this::convertRule) - .forEach(this.rules::register); - RulesEngineParameters parameters = - new RulesEngineParameters().skipOnFirstAppliedRule(true); // When first rule applies, stop the matching. - this.rulesEngine = new DefaultRulesEngine(parameters); + .forEach(newRules::register); + // Atomic swap of rules. + rules.set(newRules); + log.warn("Loaded new set of {} rules for Access Control", rules.get().size()); } public boolean hasPermission(User user, Object entity, MetadataOperation operation) { @@ -53,7 +79,7 @@ public class PolicyEvaluator { .withEntity(entity) .withOperation(operation) .build(); - this.rulesEngine.fire(rules, facts.getFacts()); + rulesEngine.fire(rules.get(), facts.getFacts()); return facts.hasPermission(); } diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java index 1578e693eea..13ba9a6e150 100644 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java +++ b/catalog-rest-service/src/test/java/org/openmetadata/catalog/resources/EntityResourceTest.java @@ -26,7 +26,6 @@ import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; import static org.openmetadata.catalog.exception.CatalogExceptionMessage.ENTITY_ALREADY_EXISTS; import static org.openmetadata.catalog.exception.CatalogExceptionMessage.entityNotFound; import static org.openmetadata.catalog.resources.teams.RoleResourceTest.createRole; @@ -218,13 +217,6 @@ public abstract class EntityResourceTest extends CatalogApplicationTest { ROLE1 = createRole(roleResourceTest.create(test), adminAuthHeaders()); ROLE_REF1 = new EntityReference().withId(ROLE1.getId()).withType("role"); - // Ensure that DefaultAuthorizer gets enough time to load policies before running tests. - try { - Thread.sleep(8000); - } catch (InterruptedException e) { - fail(); - } - // Create snowflake database service DatabaseServiceResourceTest databaseServiceResourceTest = new DatabaseServiceResourceTest(); CreateDatabaseService createDatabaseService = diff --git a/catalog-rest-service/src/test/java/org/openmetadata/catalog/security/policyevaluator/PolicyEvaluatorTest.java b/catalog-rest-service/src/test/java/org/openmetadata/catalog/security/policyevaluator/PolicyEvaluatorTest.java deleted file mode 100644 index be0bed1e97b..00000000000 --- a/catalog-rest-service/src/test/java/org/openmetadata/catalog/security/policyevaluator/PolicyEvaluatorTest.java +++ /dev/null @@ -1,142 +0,0 @@ -package org.openmetadata.catalog.security.policyevaluator; - -import static org.junit.jupiter.api.Assertions.assertFalse; -import static org.junit.jupiter.api.Assertions.assertTrue; - -import com.google.common.collect.ImmutableList; -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; -import java.util.Random; -import java.util.stream.Collectors; -import org.junit.jupiter.api.BeforeAll; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; -import org.openmetadata.catalog.entity.data.Table; -import org.openmetadata.catalog.entity.policies.accessControl.Rule; -import org.openmetadata.catalog.entity.teams.Team; -import org.openmetadata.catalog.entity.teams.User; -import org.openmetadata.catalog.jdbi3.TeamRepository; -import org.openmetadata.catalog.type.EntityReference; -import org.openmetadata.catalog.type.MetadataOperation; -import org.openmetadata.catalog.type.TagLabel; -import org.openmetadata.catalog.util.PolicyUtils; - -public class PolicyEvaluatorTest { - - // User Roles - private static final String DATA_CONSUMER = "DataConsumer"; - private static final String DATA_STEWARD = "DataSteward"; - private static final String AUDITOR = "Auditor"; - private static final String LEGAL = "Legal"; - private static final String DEV_OPS = "DevOps"; - - // Tags - private static final String PII_SENSITIVE = "PII.Sensitive"; - - private static Random random = new Random(); - private static List rules; - private PolicyEvaluator policyEvaluator; - - @BeforeAll - static void setup() { - rules = new ArrayList<>(); - rules.add(PolicyUtils.accessControlRule(null, "table", DATA_STEWARD, MetadataOperation.UpdateOwner, true, 1, true)); - rules.add(PolicyUtils.accessControlRule(PII_SENSITIVE, null, LEGAL, MetadataOperation.UpdateTags, true, 2, true)); - rules.add( - PolicyUtils.accessControlRule( - PII_SENSITIVE, null, DATA_CONSUMER, MetadataOperation.SuggestTags, true, 3, true)); - rules.add( - PolicyUtils.accessControlRule(null, null, DATA_CONSUMER, MetadataOperation.SuggestDescription, true, 4, true)); - // Add a disabled rule. - rules.add(PolicyUtils.accessControlRule(null, null, DEV_OPS, MetadataOperation.UpdateTags, true, 5, false)); - rules.add(PolicyUtils.accessControlRule(null, null, DEV_OPS, MetadataOperation.UpdateTags, false, 6, true)); - rules.add(PolicyUtils.accessControlRule(null, null, DEV_OPS, MetadataOperation.UpdateDescription, false, 7, true)); - rules.add(PolicyUtils.accessControlRule(null, null, DEV_OPS, MetadataOperation.SuggestDescription, true, 8, true)); - } - - @BeforeEach - void beforeEach() { - Collections.shuffle(rules); // Shuffle in an attempt to throw off the PolicyEvaluator if the logic is incorrect. - policyEvaluator = new PolicyEvaluator(rules); - } - - @Test - void dataConsumer_cannot_update_owner() { - User dataConsumer = createUser(ImmutableList.of(DATA_CONSUMER)); - Table table = createTable(); - boolean hasPermission = policyEvaluator.hasPermission(dataConsumer, table, MetadataOperation.UpdateOwner); - assertFalse(hasPermission); - } - - @Test - void dataSteward_can_update_owner() { - User dataConsumer = createUser(ImmutableList.of(DATA_STEWARD)); - Table table = createTable(); - boolean hasPermission = policyEvaluator.hasPermission(dataConsumer, table, MetadataOperation.UpdateOwner); - assertTrue(hasPermission); - } - - @Test - void dataConsumer_can_suggest_description() { - User dataConsumer = createUser(ImmutableList.of(DATA_CONSUMER)); - Table table = createTable(); - boolean hasPermission = policyEvaluator.hasPermission(dataConsumer, table, MetadataOperation.SuggestDescription); - assertTrue(hasPermission); - } - - @Test - void legal_can_update_tags_for_pii_tables() { - User dataConsumer = createUser(ImmutableList.of(LEGAL)); - Table table = createTable(); - boolean hasPermission = policyEvaluator.hasPermission(dataConsumer, table, MetadataOperation.UpdateTags); - assertTrue(hasPermission); - } - - @Test - void auditor_cannot_update_tags_for_pii_tables() { - User dataConsumer = createUser(ImmutableList.of(AUDITOR)); - Table table = createTable(); - boolean hasPermission = policyEvaluator.hasPermission(dataConsumer, table, MetadataOperation.UpdateTags); - assertFalse(hasPermission); - } - - @Test - void devops_can_suggest_description() { - User dataConsumer = createUser(ImmutableList.of(DEV_OPS)); - Table table = createTable(); - boolean hasPermission = policyEvaluator.hasPermission(dataConsumer, table, MetadataOperation.SuggestDescription); - assertTrue(hasPermission); - } - - @Test - void devops_cannot_update_description() { - User dataConsumer = createUser(ImmutableList.of(DEV_OPS)); - Table table = createTable(); - boolean hasPermission = policyEvaluator.hasPermission(dataConsumer, table, MetadataOperation.UpdateDescription); - assertFalse(hasPermission); - } - - @Test - void devops_cannot_update_tags() { - User dataConsumer = createUser(ImmutableList.of(DEV_OPS)); - Table table = createTable(); - boolean hasPermission = policyEvaluator.hasPermission(dataConsumer, table, MetadataOperation.UpdateTags); - assertFalse(hasPermission); - } - - private User createUser(List teamNames) { - // TODO: Use role instead of team when user schema is extended to accommodate role. - List teams = - teamNames.stream() - .map(teamName -> new TeamRepository.TeamEntityInterface(new Team().withName(teamName)).getEntityReference()) - .collect(Collectors.toList()); - return new User().withName("John Doe").withTeams(teams); - } - - private Table createTable() { - List tags = new ArrayList<>(); - tags.add(new TagLabel().withTagFQN(PII_SENSITIVE)); - return new Table().withName("random-table").withTags(tags); - } -}