New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HLRC] Support for role mapper expression dsl #33745
Conversation
This commit adds support for role mapping expression dsl. Functionally it is similar to what we have on the server side except the rule evaluation which is not required on the client. The role mapper expression can either be field expression or composite expression of one or more expressions. Role mapper expression parser is used to parse json dsl to list of expressions. This forms the base for role mapping APIs (get, post/put and delete)
Pinging @elastic/es-security |
Pinging @elastic/es-core-infra |
|
||
public T build() { | ||
try { | ||
return ctor.newInstance(new Object[] { elements.toArray(new RoleMapperExpression[0]) }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should be using reflection for this.
There's only 2 classes to handle, we can do it more simply and safely with a Function<RoleMapperExpression[], T>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have refactored the code to simplify this. Thank you.
* the provided values. A <em>field</em> expression may have more than one provided value, in which | ||
* case the expression is true if <em>any</em> of the values are matched. | ||
*/ | ||
public abstract class FieldExpressionBase implements RoleMapperExpression { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of having different types for the different fields? The JSON isn't any different based on the field type...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only difference was with Metadata field as the key was dynamic. I have addressed this with refactoring now with a FieldType enum and its usage to distinguish within the builder. Thanks for your feedback.
public FieldExpressionBuilder<T> withKey(String key) { | ||
assert Strings.hasLength(key) : "metadata key cannot be null or empty"; | ||
assert MetadataFieldExpression.class.isAssignableFrom( | ||
clazz) : "metadat key can only be provided when building MetadataFieldExpression"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: s/metadat/metadata/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, changed this.
ParseField ANY = new ParseField("any"); | ||
ParseField ALL = new ParseField("all"); | ||
ParseField EXCEPT = new ParseField("except"); | ||
ParseField FIELD = new ParseField("field"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't have both these fields and also the NAME
fields on the various expression types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. I initially did not add support for expression parser. I will address this. Thank you.
Removed individual expression classes, simiplified the usage using builders. Now there is FieldRoleMapperExpression for Field types (USERNAME, GROUPS, METADATA, DN) and CompositeRoleMapperExpression for Composite types (ALL, ANY, EXCEPT)
...csearch/client/security/support/expressiondsl/expressions/CompositeRoleMapperExpression.java
Outdated
Show resolved
Hide resolved
...rg/elasticsearch/client/security/support/expressiondsl/fields/FieldRoleMapperExpression.java
Outdated
Show resolved
Hide resolved
...rg/elasticsearch/client/security/support/expressiondsl/fields/FieldRoleMapperExpression.java
Outdated
Show resolved
Hide resolved
...g/elasticsearch/client/security/support/expressiondsl/parser/RoleMapperExpressionParser.java
Outdated
Show resolved
Hide resolved
...g/elasticsearch/client/security/support/expressiondsl/parser/RoleMapperExpressionParser.java
Outdated
Show resolved
Hide resolved
Hi @tvernum, I have addressed your review comments, please review when you get some time. Thank you for your review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, with 1 suggestion/preference.
} | ||
|
||
public static FieldRoleMapperExpression ofMetadata(String key, Object... values) { | ||
String fieldName = key.startsWith("metadata.") ? key : "metadata." + key; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer we require one or the other. That is always add the "metadata." prefix or throw an exception if the prefix is missing.
Adding it if it doesn't exist feels convenient, but is just a form of leniency that tends to come back and bite us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with two minor comments, sorry for getting to this too late @bizybot
* </pre> | ||
*/ | ||
public class FieldRoleMapperExpression implements RoleMapperExpression { | ||
private final String NAME = "field"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/NAME/name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed this, Thanks.
return parsedFieldName; | ||
} | ||
|
||
private List<RoleMapperExpression> parseExpressionArray(ParseField field, XContentParser parser, boolean allowExcept) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
parameter allowExcept
is not used
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, Thank you.
@elasticmachine test this, please |
This commit adds support for role mapping expression dsl. Functionally it is similar to what we have on the server side except for the rule evaluation which is not required on the client. The role mapper expression can either be field expression or composite expression of one or more expressions. Role mapper expression parser is used to parse JSON DSL to list of expressions. This forms the base for role mapping APIs (get, post/put and delete)
* master: (25 commits) [DOCS] Synchronize location of Breaking Changes (elastic#33588) [DOCS] Synchronizes captialization in top-level titles (elastic#33605) [SQL] Clean up LogicalPlanBuilder#doJoin (elastic#34048) Fix remote cluster seeds fallback (elastic#34090) [ML][HLRC] Replace REST-based ML test cleanup with the ML client (elastic#34109) Handle MatchNoDocsQuery in span query wrappers (elastic#34106) Update MovAvgIT AwaitsFix bug url Bad regex in CORS settings should throw a nicer error (elastic#34035) [HLRC] Support for role mapper expression dsl (elastic#33745) Watcher: Reduce script cache churn by checking for mustache tags (elastic#33978) Fold EngineSearcher into Engine.Searcher (elastic#34082) Mute SpanMultiTermQueryBuilderTests#testToQuery TESTS: Enable DEBUG Logging in Flaky Test (elastic#34091) TEST: Add engine is closed as expected failure msg Adjust bwc version for max_seq_no_of_updates Build DocStats from SegmentInfos in ReadOnlyEngine (elastic#34079) When creating wildcard queries, use MatchNoDocsQuery when the field type doesn't exist. (elastic#34093) [DOCS] Moves graph to docs folder (elastic#33472) Mute MovAvgIT#testHoltWintersNotEnoughData Security: use default scroll keepalive (elastic#33639) ...
This commit adds support for role mapping expression dsl. Functionally it is similar to what we have on the server side except for the rule evaluation which is not required on the client. The role mapper expression can either be field expression or composite expression of one or more expressions. Role mapper expression parser is used to parse JSON DSL to list of expressions. This forms the base for role mapping APIs (get, post/put and delete)
} | ||
|
||
public String getName() { | ||
return this.getName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/return this.getName();
/return this.name;
/
Noticed this when reviewing #34171 , maybe you can correct this there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, I will address in the other review for the create role mapping API. Thank you.
throw new IllegalArgumentException("null or empty field name (" + field + ")"); | ||
} | ||
if (values == null || values.length == 0) { | ||
throw new IllegalArgumentException("null or empty values (" + values + ")"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case of an empty array, this will invoke toString()
on the array which will offer no useful information. Maybe change this to
throw new IllegalArgumentException("null or empty values for field (" + field + ")");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, will address this in another review.
We added support for role mapper expression DSL in elastic#33745, that allows us to build the role mapper expression used in the role mapping (as rules for determining user roles based on what the boolean expression resolves to). This change now adds support for create/update role mapping API to the high-level rest client.
We added support for role mapper expression DSL in elastic#33745, that allows us to build the role mapper expression used in the role mapping (as rules for determining user roles based on what the boolean expression resolves to). This change now adds support for create/update role mapping API to the high-level rest client.
We added support for role mapper expression DSL in #33745, that allows us to build the role mapper expression used in the role mapping (as rules for determining user roles based on what the boolean expression resolves to). This change now adds support for create/update role mapping API to the high-level rest client.
This commit adds support for role mapping expression dsl. Functionally it is similar to what we have on the server side except for the rule evaluation which is not required on the client. The role mapper expression can either be field expression or composite expression of one or more expressions. Role mapper expression parser is used to parse JSON DSL to list of expressions. This forms the base for role mapping APIs (get, post/put and delete)
We added support for role mapper expression DSL in #33745, that allows us to build the role mapper expression used in the role mapping (as rules for determining user roles based on what the boolean expression resolves to). This change now adds support for create/update role mapping API to the high-level rest client.
This commit adds support for role mapping expression dsl.
Functionally it is similar to what we have on the server side
except for the rule evaluation which is not required on the client.
The role mapper expression can either be field expression or
composite expression of one or more expressions. Role mapper
expression parser is used to parse JSON DSL to list of expressions.
This forms the base for role mapping APIs (get, post/put and delete)