Tests and cleanup

Signed-off-by: Nils Bandener <[email protected]>
nibix · Oct 19, 2024 · f59eacf · f59eacf
1 parent e2f5780
commit f59eacf
Show file tree

Hide file tree

Showing 5 changed files with 18 additions and 77 deletions.
diff --git a/...ntegrationTest/java/org/opensearch/security/privileges/dlsfls/DocumentPrivilegesTest.java b/...ntegrationTest/java/org/opensearch/security/privileges/dlsfls/DocumentPrivilegesTest.java
@@ -1164,8 +1164,10 @@ public void equals() throws Exception {
                 xContentRegistry
             );
 
+            assertEquals(query1a, query1a);
             assertEquals(query1a, query1b);
             assertNotEquals(query2, query1a);
+            assertFalse(query1a.equals(query1a.queryString));
         }
     }
 

diff --git a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java
@@ -123,7 +123,7 @@ public DlsFlsValveImpl(
             DlsFlsProcessedConfig config = dlsFlsProcessedConfig.get();
 
             if (config != null) {
-                config.updateIndicesAsync(clusterService, threadPool);
+                config.updateClusterStateMetadataAsync(clusterService, threadPool);
             }
         });
     }

diff --git a/src/main/java/org/opensearch/security/privileges/dlsfls/DlsFlsProcessedConfig.java b/src/main/java/org/opensearch/security/privileges/dlsfls/DlsFlsProcessedConfig.java
@@ -21,6 +21,8 @@
 import org.opensearch.cluster.service.ClusterService;
 import org.opensearch.common.settings.Settings;
 import org.opensearch.core.xcontent.NamedXContentRegistry;
+import org.opensearch.security.privileges.ActionPrivileges;
+import org.opensearch.security.privileges.ClusterStateMetadataDependentPrivileges;
 import org.opensearch.security.securityconf.impl.SecurityDynamicConfiguration;
 import org.opensearch.security.securityconf.impl.v7.RoleV7;
 import org.opensearch.threadpool.ThreadPool;
@@ -29,14 +31,13 @@
  * Encapsulates the processed DLS/FLS configuration from roles.yml.
  * The current instance is held and managed by DlsFlsValveImpl.
  */
-public class DlsFlsProcessedConfig {
+public class DlsFlsProcessedConfig extends ClusterStateMetadataDependentPrivileges {
     private static final Logger log = LogManager.getLogger(DlsFlsProcessedConfig.class);
 
     private final DocumentPrivileges documentPrivileges;
     private final FieldPrivileges fieldPrivileges;
     private final FieldMasking fieldMasking;
     private long metadataVersionEffective = -1;
-    private Future<?> updateFuture;
 
     public DlsFlsProcessedConfig(
         SecurityDynamicConfiguration<RoleV7> rolesConfiguration,
@@ -62,74 +63,22 @@ public FieldMasking getFieldMasking() {
         return this.fieldMasking;
     }
 
-    public void updateIndices(Map<String, IndexAbstraction> indexMetadata) {
+    @Override
+    protected void updateClusterStateMetadata(Metadata metadata) {
         long start = System.currentTimeMillis();
+        Map<String, IndexAbstraction> indexLookup = metadata.getIndicesLookup();
 
-        this.documentPrivileges.updateIndices(indexMetadata);
-        this.fieldPrivileges.updateIndices(indexMetadata);
-        this.fieldMasking.updateIndices(indexMetadata);
+        this.documentPrivileges.updateIndices(indexLookup);
+        this.fieldPrivileges.updateIndices(indexLookup);
+        this.fieldMasking.updateIndices(indexLookup);
 
         long duration = System.currentTimeMillis() - start;
 
         log.debug("Updating DlsFlsProcessedConfig took {} ms", duration);
     }
 
-    /**
-     * Updates the stateful index configuration asynchronously with the index metadata from the current cluster state.
-     * As the update process can take some seconds for clusters with many indices, this method "de-bounces" the updates,
-     * i.e., a further update will be only initiated after the previous update has finished. This is okay as the
-     * underlying DocumentPrivileges/FieldPrivileges classes can handle the case that they do not have the most
-     * recent information. These classes will fall back to slower methods then.
-     */
-    public synchronized void updateIndicesAsync(ClusterService clusterService, ThreadPool threadPool) {
-        long currentMetadataVersion = clusterService.state().metadata().version();
-
-        if (currentMetadataVersion <= this.metadataVersionEffective) {
-            return;
-        }
-
-        if (this.updateFuture == null || this.updateFuture.isDone()) {
-            this.updateFuture = threadPool.generic().submit(() -> {
-                for (int i = 0;; i++) {
-                    if (i > 10) {
-                        try {
-                            // In case we got many consecutive updates, let's sleep a little to let
-                            // other operations catch up.
-                            Thread.sleep(100);
-                        } catch (InterruptedException e) {
-                            return;
-                        }
-                    }
-
-                    Metadata metadata = clusterService.state().metadata();
-
-                    synchronized (DlsFlsProcessedConfig.this) {
-                        if (metadata.version() <= DlsFlsProcessedConfig.this.metadataVersionEffective) {
-                            return;
-                        }
-                    }
-
-                    try {
-                        log.debug("Updating DlsFlsProcessedConfig with metadata version {}", metadata.version());
-                        updateIndices(metadata.getIndicesLookup());
-                    } catch (Exception e) {
-                        log.error("Error while updating DlsFlsProcessedConfig", e);
-                    } finally {
-                        synchronized (DlsFlsProcessedConfig.this) {
-                            DlsFlsProcessedConfig.this.metadataVersionEffective = metadata.version();
-                            if (DlsFlsProcessedConfig.this.updateFuture.isCancelled()) {
-                                return;
-                            }
-                        }
-                    }
-                }
-            });
-        }
-    }
-
-    public synchronized void shutdown() {
-        if (this.updateFuture != null && !this.updateFuture.isDone()) {
-            this.updateFuture.cancel(true);
-        }
+    @Override
+    protected long getCurrentlyUsedMetadataVersion() {
+        return this.metadataVersionEffective;
     }
 }
diff --git a/src/main/java/org/opensearch/security/privileges/dlsfls/DocumentPrivileges.java b/src/main/java/org/opensearch/security/privileges/dlsfls/DocumentPrivileges.java
@@ -14,6 +14,7 @@
 import java.util.Collection;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 
 import org.apache.logging.log4j.util.Strings;
 
@@ -114,14 +115,7 @@ public boolean equals(Object obj) {
                 return false;
             }
             DlsQuery other = (DlsQuery) obj;
-            if (queryString == null) {
-                if (other.queryString != null) {
-                    return false;
-                }
-            } else if (!queryString.equals(other.queryString)) {
-                return false;
-            }
-            return true;
+            return Objects.equals(this.queryString, other.queryString);
         }
 
         protected QueryBuilder parseQuery(String queryString, NamedXContentRegistry xContentRegistry)

diff --git a/src/main/java/org/opensearch/security/privileges/dlsfls/FieldPrivileges.java b/src/main/java/org/opensearch/security/privileges/dlsfls/FieldPrivileges.java
@@ -67,11 +67,7 @@ protected FlsRule fullyRestricted() {
 
     @Override
     protected FlsRule compile(PrivilegesEvaluationContext context, Collection<FlsRule> rules) throws PrivilegesEvaluationException {
-        if (rules.isEmpty()) {
-            return FlsRule.DENY_ALL;
-        } else {
-            return FlsRule.merge(rules);
-        }
+        return FlsRule.merge(rules);
     }
 
     /**