From d0e63f883590c0fe2fbe893aacb8759e37424a4c Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Tue, 24 Sep 2019 23:45:15 +1000 Subject: [PATCH 1/2] Use 'should' clause instead of 'filter' when querying native privileges 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. --- .../authz/store/NativePrivilegeStore.java | 5 +- .../privileges/20_has_application_privs.yml | 85 +++++++++++++++++++ 2 files changed, 88 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java index 8aea63dc34b36..45cd8de8ac974 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStore.java @@ -177,12 +177,13 @@ private QueryBuilder getApplicationNameQuery(Collection 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/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 index eb92cc252b560..b23862c5553da 100644 --- 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 + } + } + } } From e8f909de44b4c1485f3677d8094d2f108594225c Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Wed, 25 Sep 2019 08:20:01 +1000 Subject: [PATCH 2/2] fix test --- .../xpack/security/authz/store/NativePrivilegeStoreTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java index 657a1f47e42d9..b1daf77d395fe 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/store/NativePrivilegeStoreTests.java @@ -189,7 +189,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\""));