Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

HLRC: add change password API support #33509

Merged
merged 13 commits into from
Oct 2, 2018
Original file line number Diff line number Diff line change
Expand Up @@ -460,11 +460,11 @@ public final BulkByScrollResponse updateByQuery(UpdateByQueryRequest updateByQue
* Asynchronously executes an update by query request.
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-update-by-query.html">
* Update By Query API on elastic.co</a>
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @param listener the listener to be notified upon request completion
*/
public final void updateByQueryAsync(UpdateByQueryRequest reindexRequest, RequestOptions options,
ActionListener<BulkByScrollResponse> listener) {
ActionListener<BulkByScrollResponse> listener) {
performRequestAsyncAndParseEntity(
reindexRequest, RequestConverters::updateByQuery, options, BulkByScrollResponse::fromXContent, listener, emptySet()
);
Expand All @@ -475,7 +475,7 @@ public final void updateByQueryAsync(UpdateByQueryRequest reindexRequest, Reques
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-delete-by-query.html">
* Delete By Query API on elastic.co</a>
* @param deleteByQueryRequest the request
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @return the response
* @throws IOException in case there is a problem sending the request or parsing back the response
*/
Expand All @@ -489,7 +489,7 @@ public final BulkByScrollResponse deleteByQuery(DeleteByQueryRequest deleteByQue
* Asynchronously executes a delete by query request.
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/docs-delete-by-query.html">
* Delete By Query API on elastic.co</a>
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @param listener the listener to be notified upon request completion
*/
public final void deleteByQueryAsync(DeleteByQueryRequest reindexRequest, RequestOptions options,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,10 @@
package org.elasticsearch.client;

import org.elasticsearch.action.ActionListener;
import org.elasticsearch.client.security.EmptyResponse;
import org.elasticsearch.client.security.PutUserRequest;
import org.elasticsearch.client.security.PutUserResponse;
import org.elasticsearch.client.security.ChangePasswordRequest;

import java.io.IOException;

Expand All @@ -44,6 +46,7 @@ public final class SecurityClient {
* Create/update a user in the native realm synchronously.
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-users.html">
* the docs</a> for more.
*
* @param request the request with the user's information
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @return the response from the put user call
Expand All @@ -58,12 +61,44 @@ public PutUserResponse putUser(PutUserRequest request, RequestOptions options) t
* Asynchronously create/update a user in the native realm.
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-users.html">
* the docs</a> for more.
* @param request the request with the user's information
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
*
* @param request the request with the user's information
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @param listener the listener to be notified upon request completion
*/
public void putUserAsync(PutUserRequest request, RequestOptions options, ActionListener<PutUserResponse> listener) {
restHighLevelClient.performRequestAsyncAndParseEntity(request, SecurityRequestConverters::putUser, options,
PutUserResponse::fromXContent, listener, emptySet());
}

/**
* Change the password of a user in the native realm synchronously.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe say of a native realm or built-in user

* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-change-password.html">
* the docs</a> for more.
*
* @param request the request with the user's new password
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @return the response from the change user password call
* @throws IOException in case there is a problem sending the request or parsing back the response
*/
public EmptyResponse changePassword(ChangePasswordRequest request, RequestOptions options) throws IOException {
return restHighLevelClient.performRequestAndParseEntity(request, SecurityRequestConverters::changePassword, options,
EmptyResponse::fromXContent, emptySet());
}

/**
* Change the password of a user in the native realm asynchronously.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe say of a native realm or built-in user

* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-change-password.html">
* the docs</a> for more.
*
* @param request the request with the user's new password
* @param options the request options (e.g. headers), use {@link RequestOptions#DEFAULT} if nothing needs to be customized
* @param listener the listener to be notified upon request completion
*/
public void changePasswordAsync(ChangePasswordRequest request, RequestOptions options, ActionListener<EmptyResponse>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you wrap the type of the parameter too? Things line up better and are easier to read IMO

listener) {
restHighLevelClient.performRequestAsyncAndParseEntity(request, SecurityRequestConverters::changePassword, options,
EmptyResponse::fromXContent, listener, emptySet());

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove empty line

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@

package org.elasticsearch.client;

import org.apache.http.client.methods.HttpPost;
import org.apache.http.client.methods.HttpPut;
import org.elasticsearch.client.security.ChangePasswordRequest;
import org.elasticsearch.client.security.PutUserRequest;

import java.io.IOException;
Expand All @@ -31,6 +33,20 @@ public final class SecurityRequestConverters {

private SecurityRequestConverters() {}

static Request changePassword(ChangePasswordRequest changePasswordRequest) throws IOException {
String endpoint = new RequestConverters.EndpointBuilder()
.addPathPartAsIs("_xpack/security/user")
.addPathPart(changePasswordRequest.getUsername())
.addPathPartAsIs("_password")
.build();
Request request = new Request(HttpPost.METHOD_NAME, endpoint);
request.setEntity(createEntity(changePasswordRequest, REQUEST_BODY_CONTENT_TYPE));
RequestConverters.Params params = new RequestConverters.Params(request);
params.withRefreshPolicy(changePasswordRequest.getRefreshPolicy());
return request;
}


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra line

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to this nit

static Request putUser(PutUserRequest putUserRequest) throws IOException {
String endpoint = new RequestConverters.EndpointBuilder()
.addPathPartAsIs("_xpack/security/user")
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.client.security;

import org.elasticsearch.client.Validatable;
import org.elasticsearch.client.ValidationException;
import org.elasticsearch.common.CharArrays;
import org.elasticsearch.common.xcontent.ToXContentObject;
import org.elasticsearch.common.xcontent.XContentBuilder;

import java.io.Closeable;
import java.io.IOException;
import java.util.Arrays;
import java.util.Objects;
import java.util.Optional;

public final class ChangePasswordRequest implements Validatable, Closeable, ToXContentObject {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add class level javadocs?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be easier for clients if we don't implement Closeable and consequently not "owning" the password array.
I think whenever we can choose not to "own" the password (because we don't need it cached for retries or otherwise) - or any other resource for that matter - we should not own the resource. I think it's easier for us to reason that all arguments are passed by reference, the callee does not "own" any, unless we have to cache the password locally - and in this case state it explicitly in the docs!

Let me know what you think! In any case we should be pedantic and mention the "ownership" in the javadocs.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see no issue with your line of argumentation @albertzaharovits. As this applies to other request objects, I'd summon @jaymode to weigh-in, so that we can decide and aim for consistency.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think @albertzaharovits's line of thinking makes sense and let's not implement closeable. In the PutUserRequest I did not claim ownership but the array is cleared by the close method. I'll open a PR to resolve the issue there.


private final String username;
private final char[] password;
private final RefreshPolicy refreshPolicy;

public ChangePasswordRequest(String username, char[] password, RefreshPolicy refreshPolicy) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would add a javadoc to this constructor:

  • make it clear that username can be null (I would also add @nullable and/or a constructor without the first parameter)
  • state that the request object does not own the password array, and the caller is responsible to clear it.

I am being pedantic because this is the interface to clients.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 to add javadocs; I need to go back and do so for my HLRC APIs.

this.username = username;
this.password = Objects.requireNonNull(password, "password is required");
this.refreshPolicy = refreshPolicy == null ? RefreshPolicy.getDefault() : refreshPolicy;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove extra new line


public String getUsername() {
return username;
}

public char[] getPassword() {
return password;
}

public RefreshPolicy getRefreshPolicy() {
return refreshPolicy;
}

@Override
public boolean equals(Object o){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need the equals/hashcode implementations?

if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
ChangePasswordRequest that = (ChangePasswordRequest) o;
return Objects.equals(username, that.username) &&
Arrays.equals(password, that.password) &&
refreshPolicy == that.refreshPolicy;
}

@Override
public int hashCode() {
int result = Objects.hash(username, refreshPolicy);
result = 31 * result + Arrays.hashCode(password);
return result;
}
@Override
public void close() throws IOException {
if (password != null) {
Arrays.fill(password, '\u0000');
}
}

@Override
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {
builder.startObject();
if (null != username){
builder.field("username", username);
}
if (password != null) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

password should never be null based on the constructor check?

byte[] charBytes = CharArrays.toUtf8Bytes(password);
builder.field("password").utf8Value(charBytes, 0, charBytes.length);
}
return builder.endObject();
}
jaymode marked this conversation as resolved.
Show resolved Hide resolved

@Override
public Optional<ValidationException> validate() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to implement/override this method if no validation is required

return Optional.empty();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* Licensed to Elasticsearch under one or more contributor
* license agreements. See the NOTICE file distributed with
* this work for additional information regarding copyright
* ownership. Elasticsearch licenses this file to you under
* the Apache License, Version 2.0 (the "License"); you may
* not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

package org.elasticsearch.client.security;

import org.elasticsearch.common.xcontent.ObjectParser;
import org.elasticsearch.common.xcontent.XContentParser;

import java.io.IOException;

/**
* Response for a request which simply returns an empty object.
*/
public final class EmptyResponse {

private static final ObjectParser<EmptyResponse, Void> PARSER = new ObjectParser<>("empty_response", false, EmptyResponse::new);

public static EmptyResponse fromXContent(XContentParser parser) throws IOException {
return PARSER.parse(parser, null);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@

package org.elasticsearch.client;

import org.apache.http.client.methods.HttpPost;
import org.apache.http.client.methods.HttpPut;
import org.elasticsearch.client.security.ChangePasswordRequest;
import org.elasticsearch.client.security.PutUserRequest;
import org.elasticsearch.client.security.RefreshPolicy;
import org.elasticsearch.test.ESTestCase;
Expand Down Expand Up @@ -67,4 +69,39 @@ public void testPutUser() throws IOException {
assertEquals(expectedParams, request.getParameters());
assertToXContentBody(putUserRequest, request.getEntity());
}

public void testChangePassword() throws IOException {
final String username = randomAlphaOfLengthBetween(4, 12);
final char[] password = randomAlphaOfLengthBetween(8, 12).toCharArray();
final RefreshPolicy refreshPolicy = randomFrom(RefreshPolicy.values());
final Map<String, String> expectedParams;
if (refreshPolicy != RefreshPolicy.NONE) {
expectedParams = Collections.singletonMap("refresh", refreshPolicy.getValue());
} else {
expectedParams = Collections.emptyMap();
}
ChangePasswordRequest changePasswordRequest = new ChangePasswordRequest(username, password, refreshPolicy);
Request request = SecurityRequestConverters.changePassword(changePasswordRequest);
assertEquals(HttpPost.METHOD_NAME, request.getMethod());
assertEquals("/_xpack/security/user/" + changePasswordRequest.getUsername() + "/_password", request.getEndpoint());
assertEquals(expectedParams, request.getParameters());
assertToXContentBody(changePasswordRequest, request.getEntity());
}

public void testSelfChangePassword() throws IOException {
final char[] password = randomAlphaOfLengthBetween(8, 12).toCharArray();
final RefreshPolicy refreshPolicy = randomFrom(RefreshPolicy.values());
final Map<String, String> expectedParams;
if (refreshPolicy != RefreshPolicy.NONE) {
expectedParams = Collections.singletonMap("refresh", refreshPolicy.getValue());
} else {
expectedParams = Collections.emptyMap();
}
ChangePasswordRequest changePasswordRequest = new ChangePasswordRequest(null, password, refreshPolicy);
Request request = SecurityRequestConverters.changePassword(changePasswordRequest);
assertEquals(HttpPost.METHOD_NAME, request.getMethod());
assertEquals("/_xpack/security/user/_password", request.getEndpoint());
assertEquals(expectedParams, request.getParameters());
assertToXContentBody(changePasswordRequest, request.getEntity());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@
import org.elasticsearch.client.ESRestHighLevelClientTestCase;
import org.elasticsearch.client.RequestOptions;
import org.elasticsearch.client.RestHighLevelClient;
import org.elasticsearch.client.security.ChangePasswordRequest;
import org.elasticsearch.client.security.EmptyResponse;
import org.elasticsearch.client.security.PutUserRequest;
import org.elasticsearch.client.security.PutUserResponse;
import org.elasticsearch.client.security.RefreshPolicy;
Expand Down Expand Up @@ -81,4 +83,50 @@ public void onFailure(Exception e) {
assertTrue(latch.await(30L, TimeUnit.SECONDS));
}
}

public void testChangePassword() throws Exception {
RestHighLevelClient client = highLevelClient();
char[] password = new char[]{'p', 'a', 's', 's', 'w', 'o', 'r', 'd'};
char[] newPassword = new char[]{'n', 'e', 'w', 'p', 'a', 's', 's', 'w', 'o', 'r', 'd'};
PutUserRequest putUserRequest =
new PutUserRequest("change_password_user", password, Collections.singletonList("superuser"), null, null, true, null,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you change the wrapping so it is only on two lines instead of three?

RefreshPolicy.NONE);
PutUserResponse putUserResponse = client.security().putUser(putUserRequest, RequestOptions.DEFAULT);
assertTrue(putUserResponse.isCreated());
{
//tag::change-password-execute
ChangePasswordRequest request = new ChangePasswordRequest("change_password_user", newPassword, RefreshPolicy.NONE);
EmptyResponse response = client.security().changePassword(request, RequestOptions.DEFAULT);
//end::change-password-execute

assertNotNull(response);

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: extra line

}
{
//tag::change-password-execute-listener
ChangePasswordRequest request = new ChangePasswordRequest("change_password_user", password, RefreshPolicy.NONE);
ActionListener<EmptyResponse> listener = new ActionListener<EmptyResponse>() {
@Override
public void onResponse(EmptyResponse emptyResponse) {
// <1>
}

@Override
public void onFailure(Exception e) {
// <2>
}
};
//end::change-password-execute-listener

// Replace the empty listener by a blocking listener in test
final CountDownLatch latch = new CountDownLatch(1);
listener = new LatchedActionListener<>(listener, latch);

//tag::x-pack-put-user-execute-async
client.security().changePasswordAsync(request, RequestOptions.DEFAULT, listener); // <1>
//end::x-pack-put-user-execute-async

assertTrue(latch.await(30L, TimeUnit.SECONDS));
}
}
}
Loading