From 7009db4c81f6ef5eead99a6766de5cd577eddb4e Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Fri, 21 Jun 2019 22:52:44 +1000 Subject: [PATCH 1/4] Make role descriptors optional when creating API keys This commit changes the `role_descriptors` field from required to optional when creating API key. The default behavior in .NET ES client is to omit properties with `null` value requiring additional workarounds. The behavior for the API does not change. Field names (`id`, `name`) in the invalidate api keys API documentation have been corrected where they were wrong. Closes #42053 --- .../security/create-api-keys.asciidoc | 8 +++--- .../security/invalidate-api-keys.asciidoc | 7 +++--- .../security/action/CreateApiKeyRequest.java | 4 +-- .../action/CreateApiKeyRequestBuilder.java | 2 +- .../CreateApiKeyRequestBuilderTests.java | 18 +++++++++++++ .../action/CreateApiKeyRequestTests.java | 25 +++++++++++++------ 6 files changed, 45 insertions(+), 19 deletions(-) diff --git a/x-pack/docs/en/rest-api/security/create-api-keys.asciidoc b/x-pack/docs/en/rest-api/security/create-api-keys.asciidoc index 3b151d3d47d65..a9cab0f8d545c 100644 --- a/x-pack/docs/en/rest-api/security/create-api-keys.asciidoc +++ b/x-pack/docs/en/rest-api/security/create-api-keys.asciidoc @@ -37,11 +37,11 @@ The following parameters can be specified in the body of a POST or PUT request: `name`:: (string) Specifies the name for this API key. -`role_descriptors` (required):: +`role_descriptors` (optional):: (array-of-role-descriptor) An array of role descriptors for this API key. This -parameter is required but can be an empty array, which applies the permissions -of the authenticated user. If you supply role descriptors, they must be a subset -of the authenticated user's permissions. The structure of role descriptor is the +parameter is optional. When it is not specified or is an empty array, then the API key will have +the permissions of the authenticated user. If you supply role descriptors, they must +be a subset of the authenticated user's permissions. The structure of role descriptor is the same as the request for create role API. For more details, see <>. diff --git a/x-pack/docs/en/rest-api/security/invalidate-api-keys.asciidoc b/x-pack/docs/en/rest-api/security/invalidate-api-keys.asciidoc index 8e496fb58664f..595fb2db2a677 100644 --- a/x-pack/docs/en/rest-api/security/invalidate-api-keys.asciidoc +++ b/x-pack/docs/en/rest-api/security/invalidate-api-keys.asciidoc @@ -31,11 +31,11 @@ pertain to invalidating api keys: `realm_name` (optional):: (string) The name of an authentication realm. This parameter cannot be used with -either `api_key_id` or `api_key_name`. +either `id` or `name`. `username` (optional):: (string) The username of a user. This parameter cannot be used with either -`api_key_id` or `api_key_name`. +`id` or `name`. NOTE: While all parameters are optional, at least one of them is required. @@ -47,8 +47,7 @@ If you create an API key as follows: ------------------------------------------------------------ POST /_security/api_key { - "name": "my-api-key", - "role_descriptors": {} + "name": "my-api-key" } ------------------------------------------------------------ // CONSOLE diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java index 28a872c2222dd..e5f8205b041ed 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java @@ -49,7 +49,7 @@ public CreateApiKeyRequest(String name, List roleDescriptors, @N } else { throw new IllegalArgumentException("name must not be null or empty"); } - this.roleDescriptors = Objects.requireNonNull(roleDescriptors, "role descriptors may not be null"); + this.roleDescriptors = (roleDescriptors == null) ? List.of() : roleDescriptors; this.expiration = expiration; } @@ -86,7 +86,7 @@ public List getRoleDescriptors() { } public void setRoleDescriptors(List roleDescriptors) { - this.roleDescriptors = Collections.unmodifiableList(Objects.requireNonNull(roleDescriptors, "role descriptors may not be null")); + this.roleDescriptors = (roleDescriptors == null) ? List.of() : Collections.unmodifiableList(roleDescriptors); } public WriteRequest.RefreshPolicy getRefreshPolicy() { diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestBuilder.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestBuilder.java index 1a711aa7d9a26..c31dbeb7e4859 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestBuilder.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestBuilder.java @@ -39,7 +39,7 @@ public final class CreateApiKeyRequestBuilder extends ActionRequestBuilder { + PARSER.declareNamedObjects(optionalConstructorArg(), (p, c, n) -> { p.nextToken(); return RoleDescriptor.parse(n, p, false); }, new ParseField("role_descriptors")); diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestBuilderTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestBuilderTests.java index fb4f87089e8e7..181ae27f0ae9a 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestBuilderTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestBuilderTests.java @@ -59,4 +59,22 @@ public void testParserAndCreateApiRequestBuilder() throws IOException { assertThat(request.getExpiration(), equalTo(TimeValue.parseTimeValue("1d", "expiration"))); } } + + public void testParserAndCreateApiRequestBuilderWithNullOrEmptyRoleDescriptors() throws IOException { + boolean withExpiration = randomBoolean(); + boolean noRoleDescriptorsField = randomBoolean(); + final String json = "{ \"name\" : \"my-api-key\"" + + ((withExpiration) ? ", \"expiration\": \"1d\"" : "") + + ((noRoleDescriptorsField) ? "" : ", \"role_descriptors\": {}") + + "}"; + final BytesArray source = new BytesArray(json); + final NodeClient mockClient = mock(NodeClient.class); + final CreateApiKeyRequest request = new CreateApiKeyRequestBuilder(mockClient).source(source, XContentType.JSON).request(); + final List actualRoleDescriptors = request.getRoleDescriptors(); + assertThat(request.getName(), equalTo("my-api-key")); + assertThat(actualRoleDescriptors.size(), is(0)); + if (withExpiration) { + assertThat(request.getExpiration(), equalTo(TimeValue.parseTimeValue("1d", "expiration"))); + } + } } diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestTests.java index 654d56b42130e..809b3c3f87024 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestTests.java @@ -16,6 +16,7 @@ import java.io.IOException; import java.util.ArrayList; +import java.util.Collections; import java.util.List; import static org.hamcrest.Matchers.containsString; @@ -82,10 +83,16 @@ public void testSerialization() throws IOException { final TimeValue expiration = randomBoolean() ? null : TimeValue.parseTimeValue(randomTimeValue(), "test serialization of create api key"); final WriteRequest.RefreshPolicy refreshPolicy = randomFrom(WriteRequest.RefreshPolicy.values()); - final int numDescriptors = randomIntBetween(0, 4); - final List descriptorList = new ArrayList<>(); - for (int i = 0; i < numDescriptors; i++) { - descriptorList.add(new RoleDescriptor("role_" + i, new String[] { "all" }, null, null)); + boolean nullOrEmptyRoleDescriptors = randomBoolean(); + final List descriptorList; + if (nullOrEmptyRoleDescriptors) { + descriptorList = randomBoolean() ? null : List.of(); + } else { + final int numDescriptors = randomIntBetween(1, 4); + descriptorList = new ArrayList<>(); + for (int i = 0; i < numDescriptors; i++) { + descriptorList.add(new RoleDescriptor("role_" + i, new String[] { "all" }, null, null)); + } } final CreateApiKeyRequest request = new CreateApiKeyRequest(); @@ -95,9 +102,7 @@ public void testSerialization() throws IOException { if (refreshPolicy != request.getRefreshPolicy() || randomBoolean()) { request.setRefreshPolicy(refreshPolicy); } - if (descriptorList.isEmpty() == false || randomBoolean()) { - request.setRoleDescriptors(descriptorList); - } + request.setRoleDescriptors(descriptorList); try (BytesStreamOutput out = new BytesStreamOutput()) { request.writeTo(out); @@ -106,7 +111,11 @@ public void testSerialization() throws IOException { assertEquals(name, serialized.getName()); assertEquals(expiration, serialized.getExpiration()); assertEquals(refreshPolicy, serialized.getRefreshPolicy()); - assertEquals(descriptorList, serialized.getRoleDescriptors()); + if (nullOrEmptyRoleDescriptors) { + assertThat(serialized.getRoleDescriptors().isEmpty(), is(true)); + } else { + assertEquals(descriptorList, serialized.getRoleDescriptors()); + } } } } From c392221caa89e1b562ae9ec83c78927bb97b90f3 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Sun, 23 Jun 2019 23:21:40 +1000 Subject: [PATCH 2/4] fix precommit errors --- .../xpack/core/security/action/CreateApiKeyRequestTests.java | 1 - 1 file changed, 1 deletion(-) diff --git a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestTests.java b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestTests.java index 809b3c3f87024..1bb5398529f99 100644 --- a/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestTests.java +++ b/x-pack/plugin/core/src/test/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequestTests.java @@ -16,7 +16,6 @@ import java.io.IOException; import java.util.ArrayList; -import java.util.Collections; import java.util.List; import static org.hamcrest.Matchers.containsString; From 9a88866edecdc51b5d030155cd2fe65dfe9ed55b Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Tue, 25 Jun 2019 14:09:49 +1000 Subject: [PATCH 3/4] address review comments - annotate params which are Nullable --- .../xpack/core/security/action/CreateApiKeyRequest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java index e5f8205b041ed..84f824b877b9c 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java @@ -43,7 +43,7 @@ public CreateApiKeyRequest() {} * @param roleDescriptors list of {@link RoleDescriptor}s * @param expiration to specify expiration for the API key */ - public CreateApiKeyRequest(String name, List roleDescriptors, @Nullable TimeValue expiration) { + public CreateApiKeyRequest(String name, @Nullable List roleDescriptors, @Nullable TimeValue expiration) { if (Strings.hasText(name)) { this.name = name; } else { @@ -77,7 +77,7 @@ public TimeValue getExpiration() { return expiration; } - public void setExpiration(TimeValue expiration) { + public void setExpiration(@Nullable TimeValue expiration) { this.expiration = expiration; } @@ -85,7 +85,7 @@ public List getRoleDescriptors() { return roleDescriptors; } - public void setRoleDescriptors(List roleDescriptors) { + public void setRoleDescriptors(@Nullable List roleDescriptors) { this.roleDescriptors = (roleDescriptors == null) ? List.of() : Collections.unmodifiableList(roleDescriptors); } From 79ac3aba90860efabd28c46a8fd769e7f3a9a7f3 Mon Sep 17 00:00:00 2001 From: Yogesh Gaikwad Date: Tue, 25 Jun 2019 14:16:32 +1000 Subject: [PATCH 4/4] Use List.copyOf instead of Collections.unmodifiableList for immutables --- .../xpack/core/security/action/CreateApiKeyRequest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java index 84f824b877b9c..78ba943f00442 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/CreateApiKeyRequest.java @@ -49,7 +49,7 @@ public CreateApiKeyRequest(String name, @Nullable List roleDescr } else { throw new IllegalArgumentException("name must not be null or empty"); } - this.roleDescriptors = (roleDescriptors == null) ? List.of() : roleDescriptors; + this.roleDescriptors = (roleDescriptors == null) ? List.of() : List.copyOf(roleDescriptors); this.expiration = expiration; } @@ -57,7 +57,7 @@ public CreateApiKeyRequest(StreamInput in) throws IOException { super(in); this.name = in.readString(); this.expiration = in.readOptionalTimeValue(); - this.roleDescriptors = Collections.unmodifiableList(in.readList(RoleDescriptor::new)); + this.roleDescriptors = List.copyOf(in.readList(RoleDescriptor::new)); this.refreshPolicy = WriteRequest.RefreshPolicy.readFrom(in); } @@ -86,7 +86,7 @@ public List getRoleDescriptors() { } public void setRoleDescriptors(@Nullable List roleDescriptors) { - this.roleDescriptors = (roleDescriptors == null) ? List.of() : Collections.unmodifiableList(roleDescriptors); + this.roleDescriptors = (roleDescriptors == null) ? List.of() : List.copyOf(roleDescriptors); } public WriteRequest.RefreshPolicy getRefreshPolicy() {