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-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<Policy> {
       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<Policy> {
         false,
         true,
         false);
+    policyEvaluator = PolicyEvaluator.getInstance();
   }
 
   public static String getFQN(Policy policy) {
@@ -149,6 +153,10 @@ public class PolicyRepository extends EntityRepository<Policy> {
     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);

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<String> 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() {

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.
  *
- * <p>This class uses {@link DefaultRulesEngine} provided by <a
+ * <p>This Singleton class uses {@link DefaultRulesEngine} provided by <a
  * href="https://github.com/j-easy/easy-rules">j-easy/easy-rules</a> package.
  *
  * <p>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;
  *
  * <p>- {@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> rules;
+  private RulesEngine rulesEngine;
 
-  public PolicyEvaluator(List<org.openmetadata.catalog.entity.policies.accessControl.Rule> 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();
   }
 

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<T> 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 =

catalog-rest-service/src/test/java/org/openmetadata/catalog/security/policyevaluator/PolicyEvaluatorTest.java

@@ -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<Rule> 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<String> teamNames) {
-    // TODO: Use role instead of team when user schema is extended to accommodate role.
-    List<EntityReference> 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<TagLabel> tags = new ArrayList<>();
-    tags.add(new TagLabel().withTagFQN(PII_SENSITIVE));
-    return new Table().withName("random-table").withTags(tags);
-  }
-}