HLRest: add security authenticate API #33552

albertzaharovits · 2018-09-09T19:24:40Z

This adds the security authenticate API.
This API is exceptional because it does not have a request body 🦄
I have added EmptyBodyRequest up from the security package.

elasticmachine · 2018-09-09T19:24:41Z

Pinging @elastic/es-core-infra

elasticmachine · 2018-09-09T19:24:42Z

Pinging @elastic/es-security

albertzaharovits · 2018-09-09T19:25:14Z

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

+ * under the License.
+ */
+
+package org.elasticsearch.client;


not in the .security package.

albertzaharovits · 2018-09-09T19:26:44Z

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

+        return request;
+    }
+
+}


The request is a singleton, so there is no need for a "request converter function". The converted is a member of the singleton.

albertzaharovits · 2018-09-09T19:30:56Z

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

+    static {
+        PARSER.declareString(constructorArg(), USERNAME);
+        PARSER.declareStringArray(optionalConstructorArg(), ROLES);
+        PARSER.declareStringOrNull(optionalConstructorArg(), FULL_NAME);


declareStringOrNull together with optionalConstructorArg is extra cautious.
I am comfortable with it, although I guess optional is unnecessary.

albertzaharovits · 2018-09-09T19:45:23Z

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

+                    (Map<String, Object>) a[4], (Boolean) a[5]));
+    static {
+        PARSER.declareString(constructorArg(), USERNAME);
+        PARSER.declareStringArray(optionalConstructorArg(), ROLES);


If there would have been a null version for this I would have picked it.
I see the XContentBuilder adds null for every object type it serializes but the parser is not expecting null for each of these, maybe it should?
In addition to the empty collection vs null, in JSON we could also have the field missing 🎃 Do we have a rule for these?

XContentBuilder serializes null arrays with a null JSON value

elasticsearch/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/XContentBuilder.java

Line 567 in b8fb83d

if (values == null) {

but ObjectParser will fail to parse it

elasticsearch/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/AbstractObjectParser.java

Line 187 in b8fb83d

public void declareStringArray(BiConsumer<Value, List<String>> consumer, ParseField field) {

.

In practice this should not happen because on the server side we don't serialize null arrays in this case, but there is no guarantee for this, as there are no assertions.

I think the parser should handle nulls; how about adding support to object parser to have a string array or null?

albertzaharovits · 2018-09-09T19:46:40Z

docs/java-rest/high-level/security/authenticate.asciidoc

+
+The returned `AuthenticateResponse` contains a single field, `user`. This field
+, accessed with `getUser`, contains all the information about this
+authenticated user.


I wonder if could link to javadocs here ? I would like to link to the User javadoc class.

I'm not sure about that, maybe @nik9000 knows? I am pretty sure he was working on some improvements around the docs for the HLRC

there is a way to point to the root Javadoc, see /docs/java-rest/high-level/getting-started.asciidoc but I wanted a link to a specific class's javadoc. There does not seem to be any practical way for this (other than crafting rickety links).

@albertzaharovits We can make the links slightly-less rickety by adding shared attributes for the javadoc package URLs. For example, if we define:

:javadoc-client: {rest-high-level-client-javadoc}/org/elasticsearch/client

Then you can link to specific classes in the client package by specifying {javadoc-client}/ClassName.html.

If that seems workable, I can add the attributes to the Versions.asciidoc file: #34934

@debadair
I will be trying it, thank you for the air support!

hub-cap

I dont see any tests other than the documentation ones. I also noticed there is no SecurityIT, like the other tests have in HLRC's test module. Maybe this PR can add it :)

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

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

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

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

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

    }
+
+    /**
+     * Authenticate the current user (pertaining to request headers)


I'd remove pertaining to request headers since there is PKI which doesn't use headers

jaymode · 2018-10-01T02:53:38Z

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

+import org.elasticsearch.client.EmptyBodyRequest;
+import org.elasticsearch.client.Request;
+
+public class AuthenticateRequest extends EmptyBodyRequest {


add javadocs and make the class final

jaymode · 2018-10-01T02:54:15Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/security/AuthenticateResponse.java

+import java.io.IOException;
+import java.util.Objects;
+
+public class AuthenticateResponse {


make the class final and add javadocs

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

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

+                    (Map<String, Object>) a[4], (Boolean) a[5]));
+    static {
+        PARSER.declareString(constructorArg(), USERNAME);
+        PARSER.declareStringArray(optionalConstructorArg(), ROLES);


I think the parser should handle nulls; how about adding support to object parser to have a string array or null?

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

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

+    @Nullable private final String email;
+
+    private User(String username, @Nullable String[] roles, @Nullable String fullName, @Nullable String email,
+            @Nullable Map<String, Object> metadata, Boolean enabled) {


how about we just accept the boolean primitive here?

jaymode · 2018-10-01T03:00:01Z

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

+/**
+ * An authenticated user
+ */
+public class User {


make the class final

jaymode · 2018-10-01T03:02:35Z

docs/java-rest/high-level/security/authenticate.asciidoc

+
+The returned `AuthenticateResponse` contains a single field, `user`. This field
+, accessed with `getUser`, contains all the information about this
+authenticated user.


I'm not sure about that, maybe @nik9000 knows? I am pretty sure he was working on some improvements around the docs for the HLRC

albertzaharovits · 2018-10-23T09:50:14Z

run gradle build tests SUCCESS

albertzaharovits · 2018-10-23T10:08:10Z

run sample packaging tests

hub-cap

super minor stuff. <3 and it looks like its very close to being done!

...nt/rest-high-level/src/main/java/org/elasticsearch/client/security/AuthenticateResponse.java

client/rest-high-level/src/main/java/org/elasticsearch/client/security/user/User.java

client/rest-high-level/src/test/java/org/elasticsearch/client/SecurityIT.java

docs/java-rest/high-level/security/authenticate.asciidoc

jaymode

This is looking good, just a few comments

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

...nt/rest-high-level/src/main/java/org/elasticsearch/client/security/AuthenticateResponse.java

client/rest-high-level/src/main/java/org/elasticsearch/client/security/user/User.java

...nt/rest-high-level/src/test/java/org/elasticsearch/client/ESRestHighLevelClientTestCase.java

docs/java-rest/high-level/security/authenticate.asciidoc

albertzaharovits · 2018-10-29T15:56:11Z

client/rest-high-level/src/main/java/org/elasticsearch/client/security/user/User.java

+public final class User {
+
+    private final String username;
+    private final Collection<String> roles;


this is a Collection now instead of List.

albertzaharovits · 2018-10-29T16:00:36Z

docs/java-rest/high-level/security/authenticate.asciidoc

+--------------------------------------------------
+<1> `getUser` retrieves the `User` instance containing the information,
+see {javadoc-client}/security/user/User.html.
+<2> `enabled` tells if this user is usable or is deactivated.


well... this is always true in the current implementation. If the user would have been disabled the _authenticate call would be failing.

albertzaharovits · 2018-10-29T16:42:42Z

@jaymode @hub-cap thank for your input on this.
May you please have another pass over it?

albertzaharovits · 2018-10-29T17:16:14Z

test this please

jaymode

I left one comment, otherwise LGTM

jaymode · 2018-10-29T20:12:28Z

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

+    }
+
+    public Request getRequest() {
+        return request;


I think we should return a new request with this method; the request member is not immutable so we should not allow it to be returned via a public API

This is a very good point!

albertzaharovits · 2018-10-31T06:01:30Z

https://elasticsearch-ci.elastic.co/job/elastic+elasticsearch+pull-request/1459/console
Is unrelated.
run gradle build tests

This adds the security `_authenticate` API to the HLREST client. It is unlike some of the other APIs because the request does not have a body. The commit also creates the `User` entity. It is important to note that the `User` entity does not have the `enabled` flag. The `enabled` flag is part of the response, alongside the `User` entity. Moreover this adds the `SecurityIT` test class (extending `ESRestHighLevelClientTestCase`). Relates #29827

This follows #33552 , when the `_authenticate` API added a new `User` object for the API's response. This changes the `put_user` API to also employ a `User` object in the request. The User object changed slightly. A bug with put_user only putting/updating enabled (but not disabled) users has been fixed.

albertzaharovits added 5 commits September 7, 2018 19:21

No tests!!! No.

9446b19

Merge branch 'master' into hlrc_authenticate

216d625

Authenticate is done??

52fe8ba

RestHighLevelClient tests

fdde4c3

LOL

090eef6

albertzaharovits 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 9, 2018

albertzaharovits self-assigned this Sep 9, 2018

albertzaharovits requested review from jaymode and jkakavas September 9, 2018 19:24

albertzaharovits commented Sep 9, 2018

View reviewed changes

elasticmachine mentioned this pull request Sep 14, 2018

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

Closed

98 tasks

hub-cap suggested changes Sep 14, 2018

View reviewed changes

client/rest-high-level/src/main/java/org/elasticsearch/client/EmptyBodyRequest.java Outdated Show resolved Hide resolved

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

jkakavas mentioned this pull request Sep 28, 2018

HLRC: Get SSL Certificates API #34135

Merged

jaymode requested changes Oct 1, 2018

View reviewed changes

albertzaharovits added 6 commits October 19, 2018 17:10

Merge branch 'master' into hlrc_authenticate

4443657

Addressed some feedback

d2977ce

User reformat

a390c42

Merge branch 'master' into hlrc_authenticate

4b5bb5e

move User to security.user

4d69fec

Move enabled out of User

54a0479

colings86 added v6.6.0 and removed v6.5.0 labels Oct 25, 2018

hub-cap suggested changes Oct 26, 2018

View reviewed changes

jaymode requested changes Oct 26, 2018

View reviewed changes

albertzaharovits added 4 commits October 29, 2018 14:32

Merge branch 'master' into hlrc_authenticate

2de05b7

Merge foo

a9e5ea9

AuthenticateResponse no ToXContentObject

14b21a4

asciidoc

b0a3d15

albertzaharovits commented Oct 29, 2018

View reviewed changes

Nits and testEqualsAndHashCode

1edce33

jaymode approved these changes Oct 29, 2018

View reviewed changes

hub-cap approved these changes Oct 30, 2018

View reviewed changes

AuthenticateRequest new request every time!

93ecccf

Merge branch 'master' into hlrc_authenticate

75dfd7b

albertzaharovits merged commit bdd8460 into elastic:master Oct 31, 2018

albertzaharovits deleted the hlrc_authenticate branch October 31, 2018 12:49

albertzaharovits mentioned this pull request Nov 1, 2018

HLRest: refactor put_user to utilize User object #35188

Merged

jaymode mentioned this pull request Dec 7, 2018

HLRC: Add get users action #36332

Merged

jpountz removed the :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) label Jan 29, 2019

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

HLRest: add security authenticate API #33552

HLRest: add security authenticate API #33552

Uh oh!

Conversation

albertzaharovits commented Sep 9, 2018

Uh oh!

elasticmachine commented Sep 9, 2018

Uh oh!

elasticmachine commented Sep 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertzaharovits Sep 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertzaharovits Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertzaharovits Sep 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

debadair Oct 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

albertzaharovits commented Oct 23, 2018

Uh oh!

albertzaharovits commented Oct 23, 2018

Uh oh!

hub-cap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jaymode left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

albertzaharovits Sep 9, 2018 •

edited

Loading

albertzaharovits Sep 14, 2018 •

edited

Loading

albertzaharovits Sep 9, 2018 •

edited

Loading

debadair Oct 26, 2018 •

edited

Loading