HLRC: add change password API support #33509

jkakavas · 2018-09-07T15:00:15Z

This change adds support for the change password APIs to the high
level rest client.

The response for this API adopts the empty response object introduced in #33481

elasticmachine · 2018-09-07T15:00:17Z

Pinging @elastic/es-core-infra

elasticmachine · 2018-09-07T15:00:19Z

Pinging @elastic/es-security

jaymode · 2018-09-07T15:45:57Z

client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java

     * @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.


maybe say of a native realm or built-in user

jaymode · 2018-09-07T15:46:03Z

client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java

+    }
+
+    /**
+     * Change the password of a user in the native realm asynchronously.


maybe say of a native realm or built-in user

jaymode · 2018-09-07T15:49:14Z

client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java

+     * @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>


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

jaymode · 2018-09-07T15:51:55Z

client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityClient.java

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


nit: remove empty line

jaymode · 2018-09-07T15:52:57Z

...t/rest-high-level/src/main/java/org/elasticsearch/client/security/ChangePasswordRequest.java

+import java.util.Objects;
+import java.util.Optional;
+
+public final class ChangePasswordRequest implements Validatable, Closeable, ToXContentObject {


can you add class level javadocs?

jaymode · 2018-09-07T15:53:49Z

...t/rest-high-level/src/main/java/org/elasticsearch/client/security/ChangePasswordRequest.java

+        if (null != username){
+            builder.field("username", username);
+        }
+        if (password != null) {


password should never be null based on the constructor check?

jaymode · 2018-09-07T15:54:27Z

...t/rest-high-level/src/main/java/org/elasticsearch/client/security/ChangePasswordRequest.java

+    }
+
+    @Override
+    public Optional<ValidationException> validate() {


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

jaymode · 2018-09-07T15:55:07Z

...t/rest-high-level/src/main/java/org/elasticsearch/client/security/ChangePasswordRequest.java

+    }
+
+    @Override
+    public boolean equals(Object o){


do we need the equals/hashcode implementations?

jaymode · 2018-09-07T15:58:28Z

...high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java

+        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,


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

jaymode · 2018-09-07T15:59:42Z

docs/java-rest/high-level/security/change-password.asciidoc

+--------------------------------------------------
+include-tagged::{doc-tests}/SecurityDocumentationIT.java[change-password-execute-listener]
+--------------------------------------------------
+<1> Called when the execution is successfully completed. The response is


nit: add periods to the end of these two sentences

...t/rest-high-level/src/main/java/org/elasticsearch/client/security/ChangePasswordRequest.java

albertzaharovits · 2018-09-11T08:28:15Z

client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityRequestConverters.java

+        return request;
+    }
+
+


nit: extra line

+1 to this nit

albertzaharovits · 2018-09-11T09:00:41Z

...t/rest-high-level/src/main/java/org/elasticsearch/client/security/ChangePasswordRequest.java

+    private final char[] password;
+    private final RefreshPolicy refreshPolicy;
+
+    public ChangePasswordRequest(String username, char[] password, RefreshPolicy refreshPolicy) {


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.

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

albertzaharovits · 2018-09-11T09:34:29Z

...t/rest-high-level/src/main/java/org/elasticsearch/client/security/ChangePasswordRequest.java

+/**
+ * Request object to change the password of a user of a native realm or a built-in user.
+ */
+public final class ChangePasswordRequest implements Validatable, Closeable, ToXContentObject {


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.

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.

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.

albertzaharovits

I have some qualms about the password handling in the request. At the minimum, documentation will please me :)

jaymode

This is pretty close, I think we just need to get some javadocs in here and address some of the things @albertzaharovits brought up.

jaymode · 2018-10-01T02:28:10Z

client/rest-high-level/src/main/java/org/elasticsearch/client/SecurityRequestConverters.java

+        return request;
+    }
+
+


+1 to this nit

jaymode · 2018-10-01T02:31:20Z

...t/rest-high-level/src/main/java/org/elasticsearch/client/security/ChangePasswordRequest.java

+/**
+ * Request object to change the password of a user of a native realm or a built-in user.
+ */
+public final class ChangePasswordRequest implements Validatable, Closeable, ToXContentObject {


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.

jaymode · 2018-10-01T02:31:44Z

...t/rest-high-level/src/main/java/org/elasticsearch/client/security/ChangePasswordRequest.java

+    private final char[] password;
+    private final RefreshPolicy refreshPolicy;
+
+    public ChangePasswordRequest(String username, char[] password, RefreshPolicy refreshPolicy) {


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

...t/rest-high-level/src/main/java/org/elasticsearch/client/security/ChangePasswordRequest.java

jaymode · 2018-10-01T02:35:58Z

...high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java

@@ -169,6 +170,52 @@ public void onFailure(Exception e) {
            // tag::disable-user-execute-async
            client.security().disableUserAsync(request, RequestOptions.DEFAULT, listener); // <1>
            // end::disable-user-execute-async
+            // end::x-pack-put-user-execute-async


this seems out of place?

jkakavas · 2018-10-01T17:14:35Z

@jaymode , @albertzaharovits . I have addressed your feedback, I think this is ready for a final round

jaymode

LGTM

jaymode · 2018-10-01T18:03:12Z

...t/rest-high-level/src/main/java/org/elasticsearch/client/security/ChangePasswordRequest.java

+    /**
+     * @param username      The username of the user whose password should be changed or null for the current user.
+     * @param password      The new password. The password array is not cleared by the {@link ChangePasswordRequest} object so the
+     *                      calling code must clear it after handling the response.


s/handling/receiving ?

jaymode · 2018-10-01T18:09:05Z

...high-level/src/test/java/org/elasticsearch/client/documentation/SecurityDocumentationIT.java

+            //end::change-password-execute
+
+            assertNotNull(response);
+


nit: extra line

The PutUserRequest implemented closeable as it assumed ownership of the password provided to the class. This change removes the ownership of the password, documents it in the javadoc, and removes the closeable implementation. Additionally, the intermediate bytes used for writing the password to XContent are now cleared. This makes the PutUserRequest consistent with the behavior discussed in elastic#33509.

albertzaharovits

LGTM

This change adds support for the change password APIs to the high level rest client. Relates #29827

The PutUserRequest implemented closeable as it assumed ownership of the password provided to the class. This change removes the ownership of the password, documents it in the javadoc, and removes the closeable implementation. Additionally, the intermediate bytes used for writing the password to XContent are now cleared. This makes the PutUserRequest consistent with the behavior discussed in #33509.

This change adds support for the change password APIs to the high level rest client. Relates #29827

The PutUserRequest implemented closeable as it assumed ownership of the password provided to the class. This change removes the ownership of the password, documents it in the javadoc, and removes the closeable implementation. Additionally, the intermediate bytes used for writing the password to XContent are now cleared. This makes the PutUserRequest consistent with the behavior discussed in #33509.

jkakavas added 3 commits August 31, 2018 20:59

WIP

ebc99c9

Merge remote-tracking branch 'origin/master' into change-password-hlrc

397a019

Complete the implementation

931ec38

jkakavas added >enhancement v7.0.0 :Core/Java High Level REST Client :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) v6.5.0 labels Sep 7, 2018

jkakavas requested review from jaymode and albertzaharovits September 7, 2018 15:00

undo whitespace changes

168fe4f

jaymode requested changes Sep 7, 2018

View reviewed changes

jkakavas added 4 commits September 10, 2018 10:31

Address feedback

54830e8

Username should not be part of the request body

dcb091d

Merge remote-tracking branch 'origin/master' into change-password-hlrc

b2bbed7

Remove unused imports

2d45731

albertzaharovits reviewed Sep 11, 2018

View reviewed changes

...t/rest-high-level/src/main/java/org/elasticsearch/client/security/ChangePasswordRequest.java Show resolved Hide resolved

albertzaharovits reviewed Sep 11, 2018

View reviewed changes

albertzaharovits requested changes Sep 11, 2018

View reviewed changes

Merge remote-tracking branch 'origin/master' into change-password-hlrc

6c19675

elasticmachine mentioned this pull request Sep 26, 2018

Java high-level REST client completeness for xpack APIs #29827

Closed

98 tasks

Address feedback

e5404d0

jaymode requested changes Oct 1, 2018

View reviewed changes

jkakavas added 2 commits October 1, 2018 14:18

Merge remote-tracking branch 'origin/master' into change-password-hlrc

7578bd1

address feedback

5eaaa42

jaymode approved these changes Oct 1, 2018

View reviewed changes

jaymode mentioned this pull request Oct 1, 2018

HLRC: PutUserRequest should not be closeable #34196

Merged

address feedback

1cbd8a7

albertzaharovits approved these changes Oct 2, 2018

View reviewed changes

jkakavas merged commit 300896d into elastic:master Oct 2, 2018

jkakavas added a commit that referenced this pull request Oct 2, 2018

HLRC: add change password API support (#33509)

9d828cc

This change adds support for the change password APIs to the high level rest client. Relates #29827

jkakavas deleted the change-password-hlrc branch October 2, 2018 09:39

kcm pushed a commit that referenced this pull request Oct 30, 2018

HLRC: add change password API support (#33509)

afb0219

This change adds support for the change password APIs to the high level rest client. Relates #29827

colings86 removed the :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) label Nov 2, 2018

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HLRC: add change password API support #33509

HLRC: add change password API support #33509

jkakavas commented Sep 7, 2018

elasticmachine commented Sep 7, 2018

elasticmachine commented Sep 7, 2018

jaymode Sep 7, 2018

jaymode Sep 7, 2018

jaymode Sep 7, 2018

jaymode Sep 7, 2018

jaymode Sep 7, 2018

jaymode Sep 7, 2018

jaymode Sep 7, 2018

jaymode Sep 7, 2018

jaymode Sep 7, 2018

jaymode Sep 7, 2018

albertzaharovits Sep 11, 2018

jaymode Oct 1, 2018

albertzaharovits Sep 11, 2018

jaymode Oct 1, 2018

albertzaharovits Sep 11, 2018

jkakavas Sep 11, 2018

jaymode Oct 1, 2018

albertzaharovits left a comment

jaymode left a comment

jaymode Oct 1, 2018

jaymode Oct 1, 2018

jaymode Oct 1, 2018

jaymode Oct 1, 2018

jkakavas commented Oct 1, 2018

jaymode left a comment

jaymode Oct 1, 2018

jaymode Oct 1, 2018

albertzaharovits left a comment

HLRC: add change password API support #33509

HLRC: add change password API support #33509

Conversation

jkakavas commented Sep 7, 2018

elasticmachine commented Sep 7, 2018

elasticmachine commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits left a comment

Choose a reason for hiding this comment

jaymode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkakavas commented Oct 1, 2018

jaymode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits left a comment

Choose a reason for hiding this comment