Use 'should' clause instead of 'filter' when querying native privileg…

…es (#47019) (#47271) When we added support for wildcard application names, we started to build the prefix query along with the term query but we used 'filter' clause instead of 'should', so this would not fetch the correct application privilege descriptor thereby failing the _has_privilege checks. This commit changes the clause to use should and with minimum_should_match as 1.
elastic · Sep 30, 2019 · 2be351c · 2be351c
1 parent cec2ff5
commit 2be351c
Show file tree

Hide file tree

Showing 3 changed files with 89 additions and 3 deletions.
diff --git a/...rity/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/...rity/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java
@@ -179,12 +179,13 @@ private QueryBuilder getApplicationNameQuery(Collection<String> applications) {
         }
         final BoolQueryBuilder boolQuery = QueryBuilders.boolQuery();
         if (termsQuery != null) {
-            boolQuery.filter(termsQuery);
+            boolQuery.should(termsQuery);
         }
         for (String wildcard : wildcardNames) {
             final String prefix = wildcard.substring(0, wildcard.length() - 1);
-            boolQuery.filter(QueryBuilders.prefixQuery(APPLICATION.getPreferredName(), prefix));
+            boolQuery.should(QueryBuilders.prefixQuery(APPLICATION.getPreferredName(), prefix));
         }
+        boolQuery.minimumShouldMatch(1);
         return boolQuery;
     }
 

diff --git a/...src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java b/...src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java
@@ -191,7 +191,7 @@ public void testGetPrivilegesByWildcardApplicationName() throws Exception {
         assertThat(request.indices(), arrayContaining(RestrictedIndicesNames.SECURITY_MAIN_ALIAS));
 
         final String query = Strings.toString(request.source().query());
-        assertThat(query, containsString("{\"bool\":{\"filter\":[{\"terms\":{\"application\":[\"yourapp\"]"));
+        assertThat(query, containsString("{\"bool\":{\"should\":[{\"terms\":{\"application\":[\"yourapp\"]"));
         assertThat(query, containsString("{\"prefix\":{\"application\":{\"value\":\"myapp-\""));
         assertThat(query, containsString("{\"term\":{\"type\":{\"value\":\"application-privilege\""));
 

diff --git a/x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/20_has_application_privs.yml b/x-pack/plugin/src/test/resources/rest-api-spec/test/privileges/20_has_application_privs.yml
@@ -109,6 +109,27 @@ setup:
               ]
             }
 
+  - do:
+      security.put_role:
+        name: "role_containing_wildcard_app_name_and_plain_app_name"
+        body:  >
+          {
+            "cluster": [],
+            "indices": [],
+            "applications": [
+              {
+                "application": "myapp",
+                "privileges": ["user"],
+                "resources": ["*"]
+              },
+              {
+                "application": "yourapp-*",
+                "privileges": ["read"],
+                "resources": ["*"]
+              }
+            ]
+          }
+
   # And a user for each role
   - do:
       security.put_user:
@@ -134,6 +155,14 @@ setup:
                 "password": "p@ssw0rd",
                 "roles" : [ "yourapp_read_config" ]
               }
+  - do:
+      security.put_user:
+        username: "myapp_yourapp_wildard_role_user"
+        body:  >
+          {
+            "password": "p@ssw0rd",
+            "roles" : [ "role_containing_wildcard_app_name_and_plain_app_name" ]
+          }
 
 ---
 teardown:
@@ -168,6 +197,11 @@ teardown:
         username: "your_read"
         ignore: 404
 
+  - do:
+      security.delete_user:
+        username: "myapp_yourapp_wildard_role_user"
+        ignore: 404
+
   - do:
       security.delete_role:
         name: "myapp_engineering_read"
@@ -182,6 +216,12 @@ teardown:
       security.delete_role:
         name: "yourapp_read_config"
         ignore: 404
+
+  - do:
+      security.delete_role:
+        name: "role_containing_wildcard_app_name_and_plain_app_name"
+        ignore: 404
+
 ---
 "Test has_privileges with application-privileges":
   - do:
@@ -291,3 +331,48 @@ teardown:
         }
       }
     } }
+
+  - do:
+      headers: { Authorization: "Basic bXlhcHBfeW91cmFwcF93aWxkYXJkX3JvbGVfdXNlcjpwQHNzdzByZA==" } # myapp_yourapp_wildard_role_user
+      security.has_privileges:
+        user: null
+        body: >
+          {
+            "application": [
+              {
+                "application" : "myapp",
+                "resources" : [ "*" ],
+                "privileges" : [ "action:login" ]
+              },
+              {
+                "application" : "yourapp-v1",
+                "resources" : [ "*" ],
+                "privileges" : [ "read" ]
+              },
+              {
+                "application" : "yourapp-v2",
+                "resources" : [ "*" ],
+                "privileges" : [ "read" ]
+              }
+            ]
+          }
+
+  - match: { "username" : "myapp_yourapp_wildard_role_user" }
+  - match: { "has_all_requested" : true }
+  - match: { "application" : {
+    "myapp" : {
+      "*" : {
+        "action:login" : true
+      }
+    },
+    "yourapp-v1": {
+      "*": {
+        "read": true
+      }
+    },
+    "yourapp-v2": {
+      "*": {
+        "read": true
+      }
+    }
+  } }