[HLRC] Add support for get roles API #35787

jkakavas · 2018-11-21T14:09:57Z

This commits adds support for the Get Roles API to the HLRC

Relates: #29827

TODO: Implement GetRolesResponse once suppporting classes for roles are commited

elasticmachine · 2018-11-21T14:09:59Z

Pinging @elastic/es-security

bizybot

LGTM, few comments once addressed, good to go. Thank you.

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

docs/java-rest/high-level/security/get-roles.asciidoc

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

jkakavas · 2018-11-22T06:02:26Z

Thanks for the feedback @bizybot

albertzaharovits · 2018-11-23T00:08:21Z

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

@@ -407,6 +409,35 @@ public DeleteRoleMappingResponse deleteRoleMapping(DeleteRoleMappingRequest requ
                DeleteRoleMappingResponse::fromXContent, emptySet());
    }

+    /**
+     * Retrieves roles in the native realm.


IMO s/native realm/native roles store/
(I know this comment uses the lingo from the docs)

albertzaharovits · 2018-11-23T00:09:18Z

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

+            builder.addPathPart(Strings.collectionToCommaDelimitedString(getRolesRequest.getRoleNames()));
+        }
+        return new Request(HttpGet.METHOD_NAME, builder.build());
+


nit: extra line

albertzaharovits · 2018-11-23T00:09:48Z

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

+import java.util.Set;
+
+/**
+ * Request object to retrieve roles from the security index


IMO s/security index/native roles store

albertzaharovits · 2018-11-23T00:34:57Z

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

+        XContentParser.Token token;
+        while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) {
+            XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser::getTokenLocation);
+            roles.add(Role.PARSER.parse(parser, null));


I think we lose the role name here. The Role should have a name.
For this, the Role#fromXContent could have a name parameter and be called here ; and not implement ToXContentObject (PutRole would build the XContent).

Nice catch, I agree to have optional name parameter to Role#fromXContent (the javadocs for the Role will need an update)

albertzaharovits · 2018-11-23T00:41:07Z

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

@@ -79,6 +81,7 @@
        PARSER.declareFieldArray(optionalConstructorArg(), ApplicationResourcePrivileges.PARSER, APPLICATIONS, ValueType.OBJECT_ARRAY);
        PARSER.declareStringArray(optionalConstructorArg(), RUN_AS);
        PARSER.declareObject(constructorArg(), (parser, c) -> parser.map(), METADATA);
+        PARSER.declareObject(optionalConstructorArg(), (parser, c) -> parser.map(), TRANSIENT_METADATA);


Is there a difference between metadata and transient_metadata ? If not I would make them both either optionalConstructorArg (cautious) or constructorArg (confident) :)

albertzaharovits

I have recommended that the Role have a name despite the idiosyncratic Response making it hard.
From the client perspective, it's cumbersome to manipulate a list of roles, from the response, in the order of the role names that have been requested. I would argue that returning a Collection with Roles having a name as attribute follows closer the encapsulation precept.

albertzaharovits

LGTM
Only fished the nits of the nits. These are totally optional.

Thanks for tackling this before put role! 🎩

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

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

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

jkakavas · 2018-11-25T12:20:31Z

@elasticmachine run the gradle build tests 1

jkakavas · 2018-11-26T07:32:21Z

23:41:29   1> [0030-11-25T14:41:24,539][ERROR][o.e.d.z.ZenDiscovery     ] [node_t0] unexpected exception while failing [master {node_t1}{BEVzq4gKQS64P6VHKriI8w}{zNOAkJu0SVKiSbw9ZL1Vfg}{127.0.0.1}{127.0.0.1:39245} committed version [3]]
23:41:29   1> java.lang.IllegalArgumentException: can't resolve failed cluster state with uuid [pUMf5_JaTfWkxYQpiownGw], version [3]
23:41:29   1> 	at org.elasticsearch.discovery.zen.PendingClusterStatesQueue.markAsFailed(PendingClusterStatesQueue.java:110) ~[main/:?]
23:41:29   1> 	at org.elasticsearch.discovery.zen.ZenDiscovery$3.onFailure(ZenDiscovery.java:817) [main/:?]
23:41:29   1> 	at org.elasticsearch.discovery.zen.ZenDiscovery$3.onSuccess(ZenDiscovery.java:807) [main/:?]
23:41:29   1> 	at org.elasticsearch.cluster.service.ClusterApplierService$SafeClusterApplyListener.onSuccess(ClusterApplierService.java:524) [main/:?]
23:41:29   1> 	at org.elasticsearch.cluster.service.ClusterApplierService.applyChanges(ClusterApplierService.java:476) [main/:?]
23:41:29   1> 	at org.elasticsearch.cluster.service.ClusterApplierService.runTask(ClusterApplierService.java:418) [main/:?]
23:41:29   1> 	at org.elasticsearch.cluster.service.ClusterApplierService$UpdateTask.run(ClusterApplierService.java:162) [main/:?]
23:41:29   1> 	at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingRunnable.run(ThreadContext.java:627) [main/:?]
23:41:29   1> 	at org.elasticsearch.common.util.concurrent.PrioritizedEsThreadPoolExecutor$TieBreakingPrioritizedRunnable.runAndClean(PrioritizedEsThreadPoolExecutor.java:244) [main/:?]
23:41:29   1> 	at org.elasticsearch.common.util.concurrent.PrioritizedEsThreadPoolExecutor$TieBreakingPrioritizedRunnable.run(PrioritizedEsThreadPoolExecutor.java:207) [main/:?]
23:41:29   1> 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [?:1.8.0_192]
23:41:29   1> 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [?:1.8.0_192]
23:41:29   1> 	at java.lang.Thread.run(Thread.java:748) [?:1.8.0_192]
23:41:29   1> 	Suppressed: java.lang.IllegalStateException: can't resolve processed cluster state with uuid [pUMf5_JaTfWkxYQpiownGw], version [3]
23:41:29   1> 		at org.elasticsearch.discovery.zen.PendingClusterStatesQueue.markAsProcessed(PendingClusterStatesQueue.java:147) ~[main/:?]
23:41:29   1> 		at org.elasticsearch.discovery.zen.ZenDiscovery$3.onSuccess(ZenDiscovery.java:805) [main/:?]
23:41:29   1> 		at org.elasticsearch.cluster.service.ClusterApplierService$SafeClusterApplyListener.onSuccess(ClusterApplierService.java:524) [main/:?]
23:41:29   1> 		at org.elasticsearch.cluster.service.ClusterApplierService.applyChanges(ClusterApplierService.java:476) [main/:?]
23:41:29   1> 		at org.elasticsearch.cluster.service.ClusterApplierService.runTask(ClusterApplierService.java:418) [main/:?]
23:41:29   1> 		at org.elasticsearch.cluster.service.ClusterApplierService$UpdateTask.run(ClusterApplierService.java:162) [main/:?]
23:41:29   1> 		at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingRunnable.run(ThreadContext.java:627) [main/:?]
23:41:29   1> 		at org.elasticsearch.common.util.concurrent.PrioritizedEsThreadPoolExecutor$TieBreakingPrioritizedRunnable.runAndClean(PrioritizedEsThreadPoolExecutor.java:244) [main/:?]
23:41:29   1> 		at org.elasticsearch.common.util.concurrent.PrioritizedEsThreadPoolExecutor$TieBreakingPrioritizedRunnable.run(PrioritizedEsThreadPoolExecutor.java:207) [main/:?]
23:41:29   1> 		at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149) [?:1.8.0_192]
23:41:29   1> 		at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624) [?:1.8.0_192]
23:41:29   1> 		at java.lang.Thread.run(Thread.java:748) [?:1.8.0_192]

@elasticmachine run the gradle build tests 1

This commits adds support for the Get Roles API to the HLRC Relates: #29827

jkakavas added 7 commits October 29, 2018 23:27

[HLRC] Add support for the get roles API

b09881a

TODO: Implement GetRolesResponse once suppporting classes for roles are commited

Merge remote-tracking branch 'origin/master' into hlrc-get-roles

4ea50d3

Finalize API support

da8dd53

Merge remote-tracking branch 'origin/master' into hlrc-get-roles

2bc4ca6

Merge remote-tracking branch 'origin/master' into hlrc-get-roles

22521c6

Completes GetRoles API

ab862e8

Fix doc and doc tests

3f2e410

jkakavas added v7.0.0 :Core/Java High Level REST Client :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC v6.6.0 labels Nov 21, 2018

jkakavas requested review from bizybot and albertzaharovits November 21, 2018 14:09

Merge remote-tracking branch 'origin/master' into hlrc-get-roles

ef00d24

bizybot approved these changes Nov 22, 2018

View reviewed changes

Address feedback

a1f7297

Fix execute-listener tag block

782b66f

albertzaharovits reviewed Nov 23, 2018

View reviewed changes

albertzaharovits requested changes Nov 23, 2018

View reviewed changes

jkakavas added 3 commits November 23, 2018 11:26

Merge remote-tracking branch 'origin/master' into hlrc-get-roles

b88696b

address feedback

556cb25

Fix failing test and add name check in doc test

07e7d0c

albertzaharovits approved these changes Nov 23, 2018

View reviewed changes

address feedback

6579308

remove stray import

d62969a

Merge remote-tracking branch 'origin/master' into hlrc-get-roles

d9afeef

jkakavas merged commit 8daa854 into elastic:master Nov 26, 2018

jkakavas added a commit that referenced this pull request Nov 26, 2018

[HLRC] Add support for get roles API (#35787)

fa04714

This commits adds support for the Get Roles API to the HLRC Relates: #29827

jkakavas deleted the hlrc-get-roles branch November 26, 2018 10:00

tomcallahan added >enhancement and removed :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC labels Dec 18, 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 support for get roles API #35787

[HLRC] Add support for get roles API #35787

jkakavas commented Nov 21, 2018

elasticmachine commented Nov 21, 2018

bizybot left a comment

jkakavas commented Nov 22, 2018

albertzaharovits Nov 23, 2018

albertzaharovits Nov 23, 2018

albertzaharovits Nov 23, 2018

albertzaharovits Nov 23, 2018

bizybot Nov 23, 2018

albertzaharovits Nov 23, 2018

albertzaharovits left a comment

albertzaharovits left a comment •

edited

Loading

jkakavas commented Nov 25, 2018

jkakavas commented Nov 26, 2018

[HLRC] Add support for get roles API #35787

[HLRC] Add support for get roles API #35787

Conversation

jkakavas commented Nov 21, 2018

elasticmachine commented Nov 21, 2018

bizybot left a comment

Choose a reason for hiding this comment

jkakavas commented Nov 22, 2018

albertzaharovits Nov 23, 2018

Choose a reason for hiding this comment

albertzaharovits Nov 23, 2018

Choose a reason for hiding this comment

albertzaharovits Nov 23, 2018

Choose a reason for hiding this comment

albertzaharovits Nov 23, 2018

Choose a reason for hiding this comment

bizybot Nov 23, 2018

Choose a reason for hiding this comment

albertzaharovits Nov 23, 2018

Choose a reason for hiding this comment

albertzaharovits left a comment

Choose a reason for hiding this comment

albertzaharovits left a comment • edited Loading

Choose a reason for hiding this comment

jkakavas commented Nov 25, 2018

jkakavas commented Nov 26, 2018

albertzaharovits left a comment •

edited

Loading