Formatted code, added docs, removed unused tests and asserts

Signed-off-by: Jochen Kressin <[email protected]>
opensearch-project · Mar 8, 2022 · e91e565 · e91e565
1 parent 988948e
commit e91e565
Show file tree

Hide file tree

Showing 12 changed files with 30 additions and 49 deletions.
diff --git a/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java b/src/main/java/org/opensearch/security/configuration/DlsFlsValveImpl.java
@@ -99,7 +99,7 @@ public DlsFlsValveImpl(Settings settings, Client nodeClient, ClusterService clus
     }
 
     /**
-     * 
+     *
      * @param request
      * @param listener
      * @return false on error
@@ -143,9 +143,9 @@ public boolean invoke(String action, ActionRequest request, final ActionListener
 
                 if (doFilterLevelDls) {
                     setDlsModeHeader(Mode.FILTER_LEVEL);
-                    log.debug("Doing filter-level DLS because query contains TLQ");
+                    log.debug("Doing filter-level DLS because the query contains a TLQ");
                 } else {
-                    log.debug("Doing lucene-level DLS because query does not contain TLQ");
+                    log.debug("Doing lucene-level DLS because the query does not contain a TLQ");
                 }
             }
         }

diff --git a/src/main/java/org/opensearch/security/configuration/DlsQueryParser.java b/src/main/java/org/opensearch/security/configuration/DlsQueryParser.java
@@ -53,7 +53,7 @@ public final class DlsQueryParser {
 
     static {
         //Match all documents but not the nested ones
-        //Nested document types start with __ 
+        //Nested document types start with __
         //https://discuss.elastic.co/t/whats-nested-documents-layout-inside-the-lucene/59944/9
         NON_NESTED_QUERY = new BooleanQuery.Builder().add(new MatchAllDocsQuery(), Occur.FILTER)
                 .add(new PrefixQuery(new Term("_type", "__")), Occur.MUST_NOT).build();
@@ -80,7 +80,7 @@ public BooleanQuery.Builder parse(Set<String> unparsedDlsQueries, QueryShardCont
         if (unparsedDlsQueries == null || unparsedDlsQueries.isEmpty()) {
             return null;
         }
-        
+
         boolean hasNestedMapping = queryShardContext.getMapperService().hasNested();
 
         BooleanQuery.Builder dlsQueryBuilder = new BooleanQuery.Builder();
@@ -118,8 +118,7 @@ public QueryBuilder parse(String unparsedDlsQuery) {
                 public QueryBuilder call() throws Exception {
                     final XContentParser parser = JsonXContent.jsonXContent.createParser(namedXContentRegistry,
                             DeprecationHandler.THROW_UNSUPPORTED_OPERATION, unparsedDlsQuery);
-                    final QueryBuilder qb = AbstractQueryBuilder.parseInnerQueryBuilder(parser);
-                    return qb;
+                    return AbstractQueryBuilder.parseInnerQueryBuilder(parser);
                 }
 
             });

diff --git a/src/main/java/org/opensearch/security/privileges/DocumentAllowList.java b/src/main/java/org/opensearch/security/privileges/DocumentAllowList.java
@@ -21,6 +21,12 @@
 import org.opensearch.common.util.concurrent.ThreadContext;
 import org.opensearch.security.support.ConfigConstants;
 
+/**
+ * Defines which indices and documents are implicitly accessible although a user does not have
+ * explicit permissions for it. This is required for executing TLQ in DLS queries. In this case
+ * the user does not have direct access to the index for the term lookup. However, we need to allow
+ * access for executing the actual TLQ. The document allow list is scoped to individual requests.
+ */
 public class DocumentAllowList {
 
     private final Set<Entry> entries = new HashSet<>();

diff --git a/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java b/src/main/java/org/opensearch/security/privileges/PrivilegesEvaluator.java
@@ -700,7 +700,7 @@ private boolean checkDocAllowListHeader(User user, String action, ActionRequest
 
             if (documentAllowList.isAllowed(getRequest.index(), getRequest.id())) {
                 if (log.isDebugEnabled()) {
-                    log.debug("Request " + request + " is whitelisted by " + documentAllowList);
+                    log.debug("Request " + request + " is allowed by " + documentAllowList);
                 }
 
                 return true;
@@ -709,7 +709,7 @@ private boolean checkDocAllowListHeader(User user, String action, ActionRequest
             }
 
         } catch (Exception e) {
-            log.error("Error while handling document whitelist: " + docAllowListHeader, e);
+            log.error("Error while handling document allow list: " + docAllowListHeader, e);
             return false;
         }
     }

diff --git a/src/main/java/org/opensearch/security/queries/QueryBuilderTraverser.java b/src/main/java/org/opensearch/security/queries/QueryBuilderTraverser.java
@@ -99,60 +99,46 @@ public boolean check(QueryBuilder queryBuilder) {
 
             if (queryBuilder instanceof BoolQueryBuilder) {
                 BoolQueryBuilder boolQueryBuilder = (BoolQueryBuilder) queryBuilder;
-
                 return check(boolQueryBuilder.must()) || check(boolQueryBuilder.mustNot()) || check(boolQueryBuilder.should())
                         || check(boolQueryBuilder.filter());
             } else if (queryBuilder instanceof BoostingQueryBuilder) {
                 BoostingQueryBuilder boostingQueryBuilder = (BoostingQueryBuilder) queryBuilder;
-
                 return check(boostingQueryBuilder.positiveQuery()) || check(boostingQueryBuilder.negativeQuery());
             } else if (queryBuilder instanceof ConstantScoreQueryBuilder) {
                 ConstantScoreQueryBuilder constantScoreQueryBuilder = (ConstantScoreQueryBuilder) queryBuilder;
-
                 return check(constantScoreQueryBuilder.innerQuery());
             } else if (queryBuilder instanceof DisMaxQueryBuilder) {
                 DisMaxQueryBuilder disMaxQueryBuilder = (DisMaxQueryBuilder) queryBuilder;
-
                 return check(disMaxQueryBuilder.innerQueries());
             } else if (queryBuilder instanceof FieldMaskingSpanQueryBuilder) {
                 FieldMaskingSpanQueryBuilder fieldMaskingSpanQueryBuilder = (FieldMaskingSpanQueryBuilder) queryBuilder;
-
                 return check(fieldMaskingSpanQueryBuilder.innerQuery());
             } else if (queryBuilder instanceof FunctionScoreQueryBuilder) {
                 FunctionScoreQueryBuilder functionScoreQueryBuilder = (FunctionScoreQueryBuilder) queryBuilder;
-
                 return check(functionScoreQueryBuilder.query());
             } else if (queryBuilder instanceof NestedQueryBuilder) {
                 NestedQueryBuilder nestedQueryBuilder = (NestedQueryBuilder) queryBuilder;
-
                 return check(nestedQueryBuilder.query());
             } else if (queryBuilder instanceof SpanContainingQueryBuilder) {
                 SpanContainingQueryBuilder spanContainingQueryBuilder = (SpanContainingQueryBuilder) queryBuilder;
-
                 return check(spanContainingQueryBuilder.bigQuery()) || check(spanContainingQueryBuilder.littleQuery());
             } else if (queryBuilder instanceof SpanFirstQueryBuilder) {
                 SpanFirstQueryBuilder spanFirstQueryBuilder = (SpanFirstQueryBuilder) queryBuilder;
-
                 return check(spanFirstQueryBuilder.innerQuery());
             } else if (queryBuilder instanceof SpanMultiTermQueryBuilder) {
                 SpanMultiTermQueryBuilder spanMultiTermQueryBuilder = (SpanMultiTermQueryBuilder) queryBuilder;
-
                 return check(spanMultiTermQueryBuilder.innerQuery());
             } else if (queryBuilder instanceof SpanNearQueryBuilder) {
                 SpanNearQueryBuilder spanNearQueryBuilder = (SpanNearQueryBuilder) queryBuilder;
-
                 return check(spanNearQueryBuilder.clauses());
             } else if (queryBuilder instanceof SpanNotQueryBuilder) {
                 SpanNotQueryBuilder spanNotQueryBuilder = (SpanNotQueryBuilder) queryBuilder;
-
                 return check(spanNotQueryBuilder.excludeQuery()) || check(spanNotQueryBuilder.includeQuery());
             } else if (queryBuilder instanceof SpanOrQueryBuilder) {
                 SpanOrQueryBuilder spanOrQueryBuilder = (SpanOrQueryBuilder) queryBuilder;
-
                 return check(spanOrQueryBuilder.clauses());
             } else if (queryBuilder instanceof SpanWithinQueryBuilder) {
                 SpanWithinQueryBuilder spanWithinQueryBuilder = (SpanWithinQueryBuilder) queryBuilder;
-
                 return check(spanWithinQueryBuilder.bigQuery()) || check(spanWithinQueryBuilder.littleQuery());
             } else {
                 return false;

diff --git a/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java b/src/main/java/org/opensearch/security/securityconf/ConfigModelV7.java
@@ -357,7 +357,7 @@ public EvaluatedDlsFlsConfig getDlsFls(User user, IndexNameExpressionResolver re
 
             if (!containsDlsFlsConfig()) {
                 if(log.isDebugEnabled()) {
-                    log.debug("No fls or dls found for {} in {} sg roles", user, roles.size());
+                    log.debug("No fls or dls found for {} in {} security roles", user, roles.size());
                 }
 
                 return EvaluatedDlsFlsConfig.EMPTY;
@@ -367,8 +367,8 @@ public EvaluatedDlsFlsConfig getDlsFls(User user, IndexNameExpressionResolver re
             Map<String, Set<String>> flsFields = new HashMap<String, Set<String>>();
             Map<String, Set<String>> maskedFieldsMap = new HashMap<String, Set<String>>();
 
-            for (SecurityRole sgr : roles) {
-                for (IndexPattern ip : sgr.getIpatterns()) {
+            for (SecurityRole role : roles) {
+                for (IndexPattern ip : role.getIpatterns()) {
 					Set<String> concreteIndices;
 					concreteIndices = ip.getResolvedIndexPattern(user, resolver, cs);
 					String dls = ip.getDlsQuery(user);

diff --git a/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java b/src/main/java/org/opensearch/security/transport/SecurityInterceptor.java
@@ -285,16 +285,26 @@ public void handleResponse(T response) {
 
             contextToRestore.restore();
 
+            final boolean isDebugEnabled = log.isDebugEnabled();
             if (response instanceof ClusterSearchShardsResponse) {           
                 if (flsResponseHeader != null && !flsResponseHeader.isEmpty()) {
+                    if (isDebugEnabled) {
+                        log.debug("add flsResponseHeader as transient");
+                    }
                     threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_FLS_FIELDS_CCS, flsResponseHeader.get(0));
                 }
 
                 if (dlsResponseHeader != null && !dlsResponseHeader.isEmpty()) {
+                    if (isDebugEnabled) {
+                        log.debug("add dlsResponseHeader as transient");
+                    }
                     threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_DLS_QUERY_CCS, dlsResponseHeader.get(0));
                 }
 
                 if (maskedFieldsResponseHeader != null && !maskedFieldsResponseHeader.isEmpty()) {
+                    if (isDebugEnabled) {
+                        log.debug("add maskedFieldsResponseHeader as transient");
+                    }
                     threadContext.putTransient(ConfigConstants.OPENDISTRO_SECURITY_MASKED_FIELD_CCS, maskedFieldsResponseHeader.get(0));
                 }
             }

diff --git a/src/test/java/org/opensearch/security/dlic/dlsfls/AbstractDlsFlsTest.java b/src/test/java/org/opensearch/security/dlic/dlsfls/AbstractDlsFlsTest.java
@@ -118,4 +118,4 @@ protected MultiGetResponse executeMGet(String user, String password, Map<String,
 
     abstract void populateData(Client tc);
 
-}
+}
diff --git a/src/test/java/org/opensearch/security/dlic/dlsfls/DlsTermLookupQueryTest.java b/src/test/java/org/opensearch/security/dlic/dlsfls/DlsTermLookupQueryTest.java
@@ -240,18 +240,6 @@ public void testSimpleSearch_AccessCodes_noAccessCodes() throws Exception {
 		Assert.assertEquals(searchResponse.toString(), 0, searchResponse.getHits().getTotalHits().value);
 	}
 
-	@Test
-	// TODO: Isn't that the same as above???
-	public void testSimpleSearch_AccessCodes_noEntryInUserIndex() throws Exception {
-		setup(new DynamicSecurityConfig().setConfig("securityconfig_tlq.yml")
-				.setSecurityInternalUsers("internal_users_tlq.yml").setSecurityRoles("roles_tlq.yml")
-				.setSecurityRolesMapping("roles_mapping_tlq.yml"));
-
-		SearchResponse searchResponse = executeSearch("tlqdocuments", "tlq_no_codes", "password");
-
-		Assert.assertEquals(searchResponse.toString(), 0, searchResponse.getHits().getTotalHits().value);
-	}
-
 	@Test
 	public void testSimpleSearch_AllIndices_All_AccessCodes_1337() throws Exception {
 		setup(new DynamicSecurityConfig().setConfig("securityconfig_tlq.yml")
@@ -514,7 +502,6 @@ public void testGet_UserAccessCodesIndex_1337() throws Exception {
 		HttpResponse response = rh.executeGetRequest("/user_access_codes/_doc/tlq_1337",
 				encodeBasicHeader("tlq_1337", "password"));
 		Assert.assertEquals(403, response.getStatusCode());
-		Assert.assertEquals("Forbidden", response.getStatusReason());
 	}
 
 	@Test
@@ -610,4 +597,4 @@ public static List<NamedXContentRegistry.Entry> getDefaultNamedXContents() {
 	            .collect(Collectors.toList());
 	  return entries;
 	}
-}
+}
diff --git a/src/test/java/org/opensearch/security/test/AbstractSecurityUnitTest.java b/src/test/java/org/opensearch/security/test/AbstractSecurityUnitTest.java
@@ -228,9 +228,7 @@ protected Settings.Builder minimumSecuritySettingsBuilder(int node, boolean sslO
             builder.putList("plugins.security.authcz.admin_dn", "CN=kirk,OU=client,O=client,l=tEst, C=De");
             builder.put(ConfigConstants.SECURITY_BACKGROUND_INIT_IF_SECURITYINDEX_NOT_EXIST, false);
         }
-        // disable disk threshold by default, this sometimes interferes with
-        // running the tests locally and is not needed for unit and integration tests
-        builder.put("cluster.routing.allocation.disk.threshold_enabled", false);
+
         builder.put(other);
 
         return builder;

diff --git a/src/test/resources/dlsfls/internal_users_tlq.yml b/src/test/resources/dlsfls/internal_users_tlq.yml
@@ -26,7 +26,3 @@ tlq_empty_access_codes:
 tlq_no_codes:
   hash: "$2y$12$SP9z.rBgEHTlueKkiqSK/OxqB2PLJN/eRoNJ8WOPoHWIpirvbFAAy" # "password"
   backend_roles: ["os_dls_tlq_lookup"]
-
-tlq_no_entry_in_user_index:
-  hash: "$2y$12$SP9z.rBgEHTlueKkiqSK/OxqB2PLJN/eRoNJ8WOPoHWIpirvbFAAy" # "password"
-  backend_roles: ["os_dls_tlq_lookup"]
diff --git a/src/test/resources/dlsfls/securityconfig_tlq.yml b/src/test/resources/dlsfls/securityconfig_tlq.yml
@@ -11,7 +11,6 @@ config:
         transport_enabled: true
         order: 0
         http_authenticator:
-          challenge: false
           type: "basic"
         authentication_backend:
           type: "intern"
Original file line number	Diff line number	Diff line change
Expand Up		@@ -118,4 +118,4 @@ protected MultiGetResponse executeMGet(String user, String password, Map<String,

		abstract void populateData(Client tc);

		}
		}