[HLRC] Put Role #36209

albertzaharovits · 2018-12-04T10:53:28Z

This commit adds support for the put role API in the java high level rest client.

Related #29827
cc @elastic/es-security

This reverts commit 345838a.

elasticmachine · 2018-12-04T10:53:33Z

Pinging @elastic/es-core-features

albertzaharovits · 2018-12-04T11:01:26Z

...level/src/main/java/org/elasticsearch/client/security/user/privileges/IndicesPrivileges.java

@@ -217,12 +217,12 @@ public XContentBuilder toXContent(XContentBuilder builder, Params params) throws
        builder.startObject();
        builder.field(NAMES.getPreferredName(), indices);
        builder.field(PRIVILEGES.getPreferredName(), privileges);
-        if (isUsingFieldLevelSecurity()) {
+        if (grantedFields != null || deniedFields != null) {


Although the previous condition was correctly from the server logic stand point, it would hinder testing in IndicesPrivilegesTests.

albertzaharovits · 2018-12-04T11:05:45Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/security/user/privileges/Role.java

-        PARSER.declareObject(constructorArg(), (parser, c) -> parser.map(), METADATA);
-        PARSER.declareObject(constructorArg(), (parser, c) -> parser.map(), TRANSIENT_METADATA);
+        PARSER.declareObject(optionalConstructorArg(), (parser, c) -> parser.map(), METADATA);
+        PARSER.declareObject(optionalConstructorArg(), (parser, c) -> parser.map(), TRANSIENT_METADATA);


Again this was correct judging by the server logic but XContentTests serialize and deserialize roles with null metadata and then test for equality.

albertzaharovits · 2018-12-04T11:07:46Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/security/user/privileges/Role.java

-                Objects.requireNonNull(transientMetadata, "Transient metadata cannot be null. Pass an empty map instead.");
-            return this;
-        }
-


Transient metadata cannot be set in the builder, by the client, it is created only by the server.

albertzaharovits · 2018-12-04T11:10:30Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/security/user/privileges/Role.java

    }

    @Override
    public int hashCode() {
        return Objects.hash(name, clusterPrivileges, globalApplicationPrivileges, indicesPrivileges, applicationResourcePrivileges,
-            runAsPrivilege, metadata, transientMetadata);
+            runAsPrivilege, metadata);


transient metadata is a read only field when retrieving the Role and cannot be set on a put Role.
It should not matter in an equals because a user should be able to create a role that is equal to another one, except for the transient part over which it has no control (cannot set it himself)

Is a (retrieved) enabled role equal (for a client) to a disabled role ?

In principle yes because the enabled status is included in the transientMetadata which is not counted by equals. In practice I don't know how the client could get hold of a disabled role (probably before disabling it).

Do you have some scenario in mind when you think transientMetadata would be something the client should touch?

In principle yes because the enabled status is included in the transientMetadata which is not counted by equals.

Yes, this is what we have now. My (poorly stated) stated question was whether we think it should be this way or not.

In practice I don't know how the client could get hold of a disabled role (probably before disabling it).

A role that has DLS/FLS for a user that was on platinum but is now on Gold maybe.

Do you have some scenario in mind when you think transientMetadata would be something the client should touch?

No not specifically. I was just wondering if the client should be able to distinguish between the same role being enabled or disabled by comparing the role objects

This is similar to the enabled flag on the user. I think the Role that is included in a PutRoleRequest should contain only fields that the client can change. transientMetadata is a field that is read only, and it is a compromise because the Role is also part of the GetRolesResponse . We can completely pull out transientMetadata from the Role and present it other way in the Get Roles API. GetRolesResponse would be having some sort of transientMetadata field. Do you see this preferable?

I think it's clearer.. I'm now circling back to our discussion regarding Role when I started with the GetRoles API and it's much clearer what you were saying back then. I probably should have done it in the get roles effort but it wasn't obvious at that point, sorry ❤️

I think it's clearer too. I will make the change in this PR, instead of opening a new one, because Role was "synthetically" created and was expected to change as APIs suit it. But we need to get this PR in before 6.6 so as not to be breaking because it will change GetRolesResponse.
Sounds like a plan?

another approach that might help here is PutRoleRequest.Builder which implicitly uses Role.Builder but does not build the transient part to build Role.

@bizybot I prefer to pass the Role to the PutRoleRequest and not take fields apart to call builder methods.

...nt/rest-high-level/src/main/java/org/elasticsearch/client/security/user/privileges/Role.java

jkakavas · 2018-12-04T11:39:55Z

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

@@ -1023,18 +1019,63 @@ public void onFailure(Exception e) {
        }
    }

-    // TODO: move all calls to high-level REST client once APIs for adding new role exist


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

docs/java-rest/high-level/security/put-role.asciidoc

bizybot

Overall LGTM, left some comments.

bizybot · 2018-12-05T15:05:21Z

...-level/src/main/java/org/elasticsearch/client/security/user/privileges/GlobalPrivileges.java

@@ -49,7 +49,7 @@

    // When categories change, adapting this field should suffice. Categories are NOT
    // opaque "named_objects", we wish to maintain control over these namespaces
-    static final List<String> CATEGORIES = Collections.unmodifiableList(Arrays.asList("application"));
+    public static final List<String> CATEGORIES = Collections.unmodifiableList(Arrays.asList("application"));


Collections.singletonList if we do not have other categories for now.

bizybot · 2018-12-05T15:10:38Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/security/user/privileges/Role.java

@@ -329,6 +326,9 @@ public Role build() {
        public static final String MANAGE_PIPELINE = "manage_pipeline";
        public static final String MANAGE_CCR = "manage_ccr";
        public static final String READ_CCR = "read_ccr";
+        public static final String[] ARRAY = new String[] { NONE, ALL, MONITOR, MONITOR_ML, MONITOR_WATCHER, MONITOR_ROLLUP, MANAGE,


I think we should rename this constant to something that represents the meaning.

bizybot · 2018-12-05T15:10:47Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/security/user/privileges/Role.java

@@ -349,6 +349,8 @@ public Role build() {
        public static final String CREATE_INDEX = "create_index";
        public static final String VIEW_INDEX_METADATA = "view_index_metadata";
        public static final String MANAGE_FOLLOW_INDEX = "manage_follow_index";
+        public static final String[] ARRAY = new String[] { NONE, ALL, READ, READ_CROSS, CREATE, INDEX, DELETE, WRITE, MONITOR, MANAGE,


bizybot · 2018-12-05T15:30:35Z

...nt/rest-high-level/src/main/java/org/elasticsearch/client/security/user/privileges/Role.java

    }

    @Override
    public int hashCode() {
        return Objects.hash(name, clusterPrivileges, globalApplicationPrivileges, indicesPrivileges, applicationResourcePrivileges,
-            runAsPrivilege, metadata, transientMetadata);
+            runAsPrivilege, metadata);


another approach that might help here is PutRoleRequest.Builder which implicitly uses Role.Builder but does not build the transient part to build Role.

bizybot · 2018-12-05T15:34:34Z

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

+        if (role.getMetadata() != null) {
+            builder.field(Role.METADATA.getPreferredName(), role.getMetadata());
+        }
+        if (role.getTransientMetadata() != null) {


I think we should not write this for PutRoleRequest.

albertzaharovits · 2018-12-06T14:58:07Z

@jkakavas @bizybot Thanks for the eyes. I have made the change to pull out transientMetadata out of Role.
May you please review it again?

jkakavas

LGTM Albert, thanks for taking care of GetRolesResponse too.

bizybot

LGTM, Thank you.

This commit adds support for the put role API in the java high level rest client.

albertzaharovits added 16 commits November 29, 2018 20:17

Put Role request

2999459

RequestConvertTests

53ad4bb

Merge branch 'master' into hlrc_put_role_2

862bdcd

Remove Transient metadata from the Role builder

345838a

Revert "Remove Transient metadata from the Role builder"

e55ee55

This reverts commit 345838a.

Merge branch 'master' into hlrc_put_role_2

c918ca3

Put Role security client and request

d085ef7

Transient metadata

9707795

SecurityDocumentationIT

d087f98

IndicesPrivilegesTests

fcbaad3

PutRoleRequestTests

e09be47

SecurityIT in progress

8c8cf33

transient metadata no longer public

ff5d788

Almost there!

e273930

And docs

86941b5

Trimming

fa48020

albertzaharovits added >enhancement v7.0.0 :Core/Features/Java High Level REST Client v6.6.0 labels Dec 4, 2018

albertzaharovits self-assigned this Dec 4, 2018

albertzaharovits requested review from bizybot and jkakavas December 4, 2018 10:53

elasticmachine mentioned this pull request Dec 4, 2018

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

Closed

98 tasks

Unsaved trimming

730f835

albertzaharovits commented Dec 4, 2018

View reviewed changes

jkakavas reviewed Dec 4, 2018

View reviewed changes

albertzaharovits added 2 commits December 5, 2018 15:26

Merge branch 'master' into hlrc_put_role_2

f1f0550

Review

302f62b

bizybot reviewed Dec 5, 2018

View reviewed changes

albertzaharovits added 3 commits December 6, 2018 14:37

Merge branch 'master' into hlrc_put_role_2

8234a2f

Pull transient metadata out of Role

0499df0

Minor left over

6ae513e

supported-apis

e65f81a

jkakavas approved these changes Dec 7, 2018

View reviewed changes

albertzaharovits added 4 commits December 7, 2018 12:15

Checkstyle

0137e25

Merge branch 'master' into hlrc_put_role_2

e798779

Merge branch 'master' into hlrc_put_role_2

4c94995

Checkstyle

6699ad3

bizybot approved these changes Dec 10, 2018

View reviewed changes

albertzaharovits merged commit dad6f1c into elastic:master Dec 10, 2018

albertzaharovits deleted the hlrc_put_role_2 branch December 10, 2018 10:24

albertzaharovits added a commit that referenced this pull request Dec 10, 2018

[HLRC] Put Role (#36209)

54ef24d

This commit adds support for the put role API in the java high level rest client.

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] Put Role #36209

[HLRC] Put Role #36209

albertzaharovits commented Dec 4, 2018

elasticmachine commented Dec 4, 2018

albertzaharovits Dec 4, 2018

albertzaharovits Dec 4, 2018

albertzaharovits Dec 4, 2018

albertzaharovits Dec 4, 2018 •

edited

Loading

jkakavas Dec 4, 2018

albertzaharovits Dec 4, 2018

jkakavas Dec 4, 2018

albertzaharovits Dec 5, 2018

jkakavas Dec 5, 2018

albertzaharovits Dec 5, 2018

bizybot Dec 5, 2018

albertzaharovits Dec 6, 2018

jkakavas Dec 4, 2018

bizybot left a comment

bizybot Dec 5, 2018

bizybot Dec 5, 2018

bizybot Dec 5, 2018

bizybot Dec 5, 2018

bizybot Dec 5, 2018

albertzaharovits commented Dec 6, 2018

jkakavas left a comment

bizybot left a comment

[HLRC] Put Role #36209

[HLRC] Put Role #36209

Conversation

albertzaharovits commented Dec 4, 2018

elasticmachine commented Dec 4, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertzaharovits Dec 4, 2018 • edited Loading

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

bizybot 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

Choose a reason for hiding this comment

albertzaharovits commented Dec 6, 2018

jkakavas left a comment

Choose a reason for hiding this comment

bizybot left a comment

Choose a reason for hiding this comment

albertzaharovits Dec 4, 2018 •

edited

Loading