From 8783abcc6f6b02f49d7728ef0e604d98f8582fc6 Mon Sep 17 00:00:00 2001 From: uboness Date: Wed, 19 Nov 2014 12:35:02 +0100 Subject: [PATCH] Empty list of privileges should be skipped When a role is configured with an entry with an empty list of privileges (cluster or indices), the entry should be skipped. Fixes elastic/elasticsearch#339 Original commit: elastic/x-pack-elasticsearch@44d957529eff0b267da7c7800696b85f0b0d1915 --- .../shield/authz/store/FileRolesStore.java | 39 ++++++++++++------- .../authz/store/FileRolesStoreTests.java | 17 +++++--- .../shield/authz/store/roles.yml | 6 +++ 3 files changed, 44 insertions(+), 18 deletions(-) diff --git a/src/main/java/org/elasticsearch/shield/authz/store/FileRolesStore.java b/src/main/java/org/elasticsearch/shield/authz/store/FileRolesStore.java index 29aa800b11b..3cfdb011ec4 100644 --- a/src/main/java/org/elasticsearch/shield/authz/store/FileRolesStore.java +++ b/src/main/java/org/elasticsearch/shield/authz/store/FileRolesStore.java @@ -8,7 +8,6 @@ package org.elasticsearch.shield.authz.store; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.common.Strings; import org.elasticsearch.common.collect.ImmutableMap; -import org.elasticsearch.common.collect.ImmutableSet; import org.elasticsearch.common.component.AbstractComponent; import org.elasticsearch.common.inject.Inject; import org.elasticsearch.common.jackson.dataformat.yaml.snakeyaml.error.YAMLException; @@ -105,34 +104,44 @@ public class FileRolesStore extends AbstractComponent implements RolesStore { if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); } else if ("cluster".equals(currentFieldName)) { - Privilege.Name name; + Privilege.Name name = null; if (token == XContentParser.Token.VALUE_STRING) { - String[] names = COMMA_DELIM.split(parser.text().trim()); - name = new Privilege.Name(names); + String namesStr = parser.text().trim(); + if (Strings.hasLength(namesStr)) { + String[] names = COMMA_DELIM.split(namesStr); + name = new Privilege.Name(names); + } } else if (token == XContentParser.Token.START_ARRAY) { - ImmutableSet.Builder names = ImmutableSet.builder(); + Set names = new HashSet<>(); while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { if (token == XContentParser.Token.VALUE_STRING) { names.add(parser.text()); } } - name = new Privilege.Name(names.build()); + if (!names.isEmpty()) { + name = new Privilege.Name(names); + } } else { throw new ElasticsearchException("Invalid roles file format [" + path.toAbsolutePath() + "]. [cluster] field value can either be a string or a list of strings, but [" + token + "] was found instead in role [" + roleName + "]"); } - permission.set(Privilege.Cluster.get(name)); + if (name != null) { + permission.set(Privilege.Cluster.get(name)); + } } else if ("indices".equals(currentFieldName)) { if (token == XContentParser.Token.START_OBJECT) { while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); - } else { + } else if (Strings.hasLength(currentFieldName)) { String[] indices = COMMA_DELIM.split(currentFieldName); - Privilege.Name name; + Privilege.Name name = null; if (token == XContentParser.Token.VALUE_STRING) { - String[] names = COMMA_DELIM.split(parser.text()); - name = new Privilege.Name(names); + String namesStr = parser.text().trim(); + if (Strings.hasLength(namesStr)) { + String[] names = COMMA_DELIM.split(parser.text()); + name = new Privilege.Name(names); + } } else if (token == XContentParser.Token.START_ARRAY) { Set names = new HashSet<>(); while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { @@ -143,13 +152,17 @@ public class FileRolesStore extends AbstractComponent implements RolesStore { "]. Could not parse [" + token + "] as index privilege in role[" + roleName + "]. Privilege names must be strings"); } } - name = new Privilege.Name(names); + if (!names.isEmpty()) { + name = new Privilege.Name(names); + } } else { throw new ElasticsearchException("Invalid roles file format [" + path.toAbsolutePath() + "]. Could not parse [" + token + "] as index privileges list in role [" + roleName + "]. Privilege lists must either " + "be a comma delimited string or an array of strings"); } - permission.add(Privilege.Index.get(name), indices); + if (name != null) { + permission.add(Privilege.Index.get(name), indices); + } } } } else { diff --git a/src/test/java/org/elasticsearch/shield/authz/store/FileRolesStoreTests.java b/src/test/java/org/elasticsearch/shield/authz/store/FileRolesStoreTests.java index 77c8d21b1f7..84d133336fb 100644 --- a/src/test/java/org/elasticsearch/shield/authz/store/FileRolesStoreTests.java +++ b/src/test/java/org/elasticsearch/shield/authz/store/FileRolesStoreTests.java @@ -41,7 +41,7 @@ public class FileRolesStoreTests extends ElasticsearchTestCase { Path path = Paths.get(getClass().getResource("roles.yml").toURI()); Map roles = FileRolesStore.parseFile(path, logger, mock(AuthorizationService.class)); assertThat(roles, notNullValue()); - assertThat(roles.size(), is(3)); + assertThat(roles.size(), is(4)); Permission.Global.Role role = roles.get("role1"); assertThat(role, notNullValue()); @@ -90,6 +90,13 @@ public class FileRolesStoreTests extends ElasticsearchTestCase { assertThat(group.indices()[0], equalTo("/.*_.*/")); assertThat(group.privilege(), notNullValue()); assertThat(group.privilege().isAlias(Privilege.Index.union(Privilege.Index.READ, Privilege.Index.WRITE)), is(true)); + + role = roles.get("role4"); + assertThat(role, notNullValue()); + assertThat(role.name(), equalTo("role4")); + assertThat(role.cluster(), notNullValue()); + assertThat(role.cluster(), is(Permission.Cluster.Core.NONE)); + assertThat(role.indices(), is(Permission.Indices.Core.NONE)); } /** @@ -139,7 +146,7 @@ public class FileRolesStoreTests extends ElasticsearchTestCase { Permission.Global.Role role = store.role("role1"); assertThat(role, notNullValue()); - role = store.role("role4"); + role = store.role("role5"); assertThat(role, nullValue()); watcherService.start(); @@ -148,7 +155,7 @@ public class FileRolesStoreTests extends ElasticsearchTestCase { writer.newLine(); writer.newLine(); writer.newLine(); - writer.append("role4:").append(System.lineSeparator()); + writer.append("role5:").append(System.lineSeparator()); writer.append(" cluster: 'MONITOR'"); } @@ -156,9 +163,9 @@ public class FileRolesStoreTests extends ElasticsearchTestCase { fail("Waited too long for the updated file to be picked up"); } - role = store.role("role4"); + role = store.role("role5"); assertThat(role, notNullValue()); - assertThat(role.name(), equalTo("role4")); + assertThat(role.name(), equalTo("role5")); assertThat(role.cluster().check("cluster:monitor/foo/bar"), is(true)); assertThat(role.cluster().check("cluster:admin/foo/bar"), is(false)); diff --git a/src/test/resources/org/elasticsearch/shield/authz/store/roles.yml b/src/test/resources/org/elasticsearch/shield/authz/store/roles.yml index 0c9b98823d4..1abff750863 100644 --- a/src/test/resources/org/elasticsearch/shield/authz/store/roles.yml +++ b/src/test/resources/org/elasticsearch/shield/authz/store/roles.yml @@ -11,3 +11,9 @@ role3: indices: '/.*_.*/': READ, WRITE +role4: + cluster: + indices: + 'idx2': + '': READ + 'idx1': [] \ No newline at end of file