[HLRC] Add support for get role mappings API #34637

bizybot · 2018-10-19T13:25:45Z

This commit adds support for get role mappings API in HLRC.

elasticmachine · 2018-10-19T13:25:46Z

Pinging @elastic/es-security

elasticmachine · 2018-10-19T13:25:47Z

Pinging @elastic/es-core-infra

jaymode · 2018-10-22T18:56:32Z

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

+     * See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/security-api-get-role-mapping.html">
+     * the docs</a> for more.
+     *
+     * @param request request {@link GetRoleMappingsRequest} with role mapping name(s).


s/request request/request

Fixed, Thank you.

jaymode · 2018-10-22T18:59:45Z

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

+ */
+public final class ExpressionRoleMapping {
+
+    static final ObjectParser<Builder, String> PARSER = new ObjectParser<>("role-mapping", Builder::new);


What about using a ConstructingObjectParser? You get the arguments and then can just return an ExpressionRoleMapping instead of the builder.

I have modified this to use ConstructingObjectParser but could not make it work to create ExpressionRoleMapping instead of the builder. The structure that I was trying to parse is as follows:

{ "kerberosmapping" : { ... }, "ldapmapping" : { ... } }

I tried using the named object parser but it expected a field wrapping the list of objects or else it will throw an error. May be I am missing something here, if you feel that there is a way we can do please suggest. Thank you.

jaymode · 2018-10-22T19:01:16Z

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

+ * Request object to get role mappings
+ */
+public final class GetRoleMappingsRequest implements Validatable, ToXContentObject {
+    private Set<String> roleMappingNames = new HashSet<>();


make it final, set the value in the constructor, and wrap in an unmodifiable set

Done, Thank you.

jaymode · 2018-10-22T19:02:08Z

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

+    }
+
+    @Override
+    public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException {


Do we need this? I do not think everything needs to implement ToXContentObject since it is ok to have an empty body IIUC

We do not need it, removed it, Thanks.

jaymode · 2018-10-22T19:02:30Z

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

+ */
+public final class GetRoleMappingsResponse {
+
+    private List<ExpressionRoleMapping> mappings;


make this variable final

Done, Thank you.

jaymode · 2018-10-22T19:03:42Z

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

+    public static GetRoleMappingsResponse fromXContent(XContentParser parser) throws IOException {
+        List<ExpressionRoleMapping> roleMappings = new ArrayList<>();
+
+        parser.nextToken();


use XContentParserUtils#ensureExpectedToken in this parsing code. There are also other useful methods in that class.

Done, Thank you.

jaymode · 2018-10-22T19:05:27Z

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

@@ -101,6 +104,25 @@ public void testPutRoleMapping() throws IOException {
        assertToXContentBody(putRoleMappingRequest, request.getEntity());
    }

+    public void testGetRoleMappings() throws IOException {
+        int noOfRoleMappingNames = randomIntBetween(0, 2);
+        final String[] roleMappingNames = randomArray(noOfRoleMappingNames, noOfRoleMappingNames, String[]::new, () -> randomAlphaOfLength(


this might read better if you wrap after the = so the right hand side is all on one line

Done, Thank you.

jaymode · 2018-10-22T19:06:29Z

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

+        if (noOfRoleMappingNames == 0) {
+            assertEquals("/_xpack/security/role_mapping", request.getEndpoint());
+        } else {
+            assertEquals("/_xpack/security/role_mapping/" + Strings.collectionToCommaDelimitedString(getRoleMappingsRequest


I don't like splitting a statement across multiple lines like this. I'd prefer to have the whole Strings.collection... call on one line.

Done, Thank you.

jaymode · 2018-10-23T20:04:07Z

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

+public final class ExpressionRoleMapping {
+
+    @SuppressWarnings("unchecked")
+    static final ConstructingObjectParser<ExpressionRoleMapping.Builder, Void> PARSER = new ConstructingObjectParser<>("role-mapping", true,


you can get rid of the builder and just return an expressionrolemapping by changing the Void to a String and when you call this use:

String name = parser.currentName(); ExpressionRoleMapping mapping = ExpressionRoleMapping.PARSER.parse(parser, name);

then the function to build would be:

(args, name) -> new ExpressionRoleMapping(name, (List<String>) args[0], (RoleMapperExpression) args[1], (Map<String, Object>) args[2], (boolean) args[3]);

Obviously that is missing the checks for values, but those should be in the constructor of ExpressionRoleMapping anyway

Thanks, Jay!, this works I missed the context part.
On the missing checks, I think we are not validating in the constructors for the response objects assuming them to be created appropriately. I see this pattern followed in the security rest client response objects, do you think we need to add the checks?

jaymode

LGTM

This commit adds support for get role mappings API in HLRC.

[HLRC] Add support for get role mappings API

4ab8e97

This commit adds support for get role mappings API in HLRC.

bizybot added v7.0.0 :Core/Java High Level REST Client backport pending :Security/Security Security issues without another label v6.5.0 labels Oct 19, 2018

bizybot requested review from jaymode and jkakavas October 19, 2018 13:25

elasticmachine mentioned this pull request Oct 19, 2018

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

Closed

98 tasks

Fix precommit errors.

5567756

jaymode requested changes Oct 22, 2018

View reviewed changes

Yogesh Gaikwad added 2 commits October 23, 2018 15:54

Address review comments

64487a3

Merge branch 'master' into hlrc-get-role-mapping-api

84767c0

jaymode requested changes Oct 23, 2018

View reviewed changes

Yogesh Gaikwad added 3 commits October 24, 2018 09:49

Address review comments

75ae8eb

Merge branch 'master' into hlrc-get-role-mapping-api

413b688

Fix precommit errors

79e3b7f

jaymode approved these changes Oct 25, 2018

View reviewed changes

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

bizybot merged commit a5ee134 into elastic:master Oct 28, 2018

bizybot added a commit that referenced this pull request Oct 28, 2018

[HLRC] Add support for get role mappings API (#34637)

3252dc5

This commit adds support for get role mappings API in HLRC.

bizybot removed the backport pending label Oct 28, 2018

bizybot deleted the hlrc-get-role-mapping-api branch October 28, 2018 23:25

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

[HLRC] Add support for get role mappings API (#34637)

46d4c7a

This commit adds support for get role mappings API in HLRC.

pcsanwald added the >non-issue label Jan 22, 2019

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 role mappings API #34637

[HLRC] Add support for get role mappings API #34637

bizybot commented Oct 19, 2018

elasticmachine commented Oct 19, 2018

elasticmachine commented Oct 19, 2018

jaymode Oct 22, 2018

bizybot Oct 22, 2018

jaymode Oct 22, 2018

bizybot Oct 23, 2018

jaymode Oct 22, 2018

bizybot Oct 22, 2018

jaymode Oct 22, 2018

bizybot Oct 22, 2018

jaymode Oct 22, 2018

bizybot Oct 22, 2018

jaymode Oct 22, 2018

bizybot Oct 23, 2018

jaymode Oct 22, 2018

bizybot Oct 22, 2018

jaymode Oct 22, 2018

bizybot Oct 22, 2018

jaymode Oct 23, 2018

bizybot Oct 23, 2018

jaymode left a comment

[HLRC] Add support for get role mappings API #34637

[HLRC] Add support for get role mappings API #34637

Conversation

bizybot commented Oct 19, 2018

elasticmachine commented Oct 19, 2018

elasticmachine commented Oct 19, 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

Choose a reason for hiding this comment

jaymode left a comment

Choose a reason for hiding this comment