[HLRC] Add support for get role mappings API#34637
Conversation
This commit adds support for get role mappings API in HLRC.
|
Pinging @elastic/es-security |
|
Pinging @elastic/es-core-infra |
| * 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). |
| */ | ||
| public final class ExpressionRoleMapping { | ||
|
|
||
| static final ObjectParser<Builder, String> PARSER = new ObjectParser<>("role-mapping", Builder::new); |
There was a problem hiding this comment.
What about using a ConstructingObjectParser? You get the arguments and then can just return an ExpressionRoleMapping instead of the builder.
There was a problem hiding this comment.
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.
| * Request object to get role mappings | ||
| */ | ||
| public final class GetRoleMappingsRequest implements Validatable, ToXContentObject { | ||
| private Set<String> roleMappingNames = new HashSet<>(); |
There was a problem hiding this comment.
make it final, set the value in the constructor, and wrap in an unmodifiable set
| } | ||
|
|
||
| @Override | ||
| public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { |
There was a problem hiding this comment.
Do we need this? I do not think everything needs to implement ToXContentObject since it is ok to have an empty body IIUC
There was a problem hiding this comment.
We do not need it, removed it, Thanks.
| */ | ||
| public final class GetRoleMappingsResponse { | ||
|
|
||
| private List<ExpressionRoleMapping> mappings; |
| public static GetRoleMappingsResponse fromXContent(XContentParser parser) throws IOException { | ||
| List<ExpressionRoleMapping> roleMappings = new ArrayList<>(); | ||
|
|
||
| parser.nextToken(); |
There was a problem hiding this comment.
use XContentParserUtils#ensureExpectedToken in this parsing code. There are also other useful methods in that class.
|
|
||
| public void testGetRoleMappings() throws IOException { | ||
| int noOfRoleMappingNames = randomIntBetween(0, 2); | ||
| final String[] roleMappingNames = randomArray(noOfRoleMappingNames, noOfRoleMappingNames, String[]::new, () -> randomAlphaOfLength( |
There was a problem hiding this comment.
this might read better if you wrap after the = so the right hand side is all on one line
| if (noOfRoleMappingNames == 0) { | ||
| assertEquals("/_xpack/security/role_mapping", request.getEndpoint()); | ||
| } else { | ||
| assertEquals("/_xpack/security/role_mapping/" + Strings.collectionToCommaDelimitedString(getRoleMappingsRequest |
There was a problem hiding this comment.
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.
| public final class ExpressionRoleMapping { | ||
|
|
||
| @SuppressWarnings("unchecked") | ||
| static final ConstructingObjectParser<ExpressionRoleMapping.Builder, Void> PARSER = new ConstructingObjectParser<>("role-mapping", true, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
This commit adds support for get role mappings API in HLRC.
This commit adds support for get role mappings API in HLRC.
This commit adds support for get role mappings API in HLRC.