Refactor role descriptor parsing#107430
Merged
slobodanadamovic merged 5 commits intoelastic:mainfrom Apr 16, 2024
Merged
Conversation
The method for `RoleDescriptor` parsing is becoming more complex due to its usage being shared between API keys, file-based and native roles. In some cases we allow 2.X format, in others we disallow restrictions. Having a single method with multiple boolean flags that control inclusion/exclusion of fields is becoming hard to extend. This refactoring aims to allow easier introduction of new fields that should be conditionally supported in some cases but not others. One of such cases is introduction of `description` field that should only be supported for file-based and native roles but not for roles embedded in API keys.
| return parse(name, parser, allow2xFormat, allowRestriction); | ||
| } | ||
| } | ||
| public static final class Parser { |
Contributor
Author
There was a problem hiding this comment.
The main change here is pulling parse method to the RoleDescriptor.Parser class and making allow2xFormat and allowRestriction, properties of the parser class - instead of parameters of the parse method.
Collaborator
|
Pinging @elastic/es-security (Team:Security) |
…tor-role-parsing # Conflicts: # x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/FileRolesStore.java
n1v0lg
approved these changes
Apr 15, 2024
Contributor
n1v0lg
left a comment
There was a problem hiding this comment.
LGTM! This is a nice clean up 🧹 One optional suggestion around avoiding instantiating objects on every parse call (see my inline comment).
| XContentParserUtils.ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, p); | ||
| XContentParserUtils.ensureExpectedToken(XContentParser.Token.START_OBJECT, p.nextToken(), p); | ||
| roleDescriptors.add(RoleDescriptor.parse(p.currentName(), p, false)); | ||
| roleDescriptors.add(RoleDescriptor.parser().allowRestriction(true).parse(p.currentName(), p)); |
Contributor
There was a problem hiding this comment.
Might extract this into a private static field to avoid instantiating every time we parse (here and in other prod code spots).
Contributor
Author
There was a problem hiding this comment.
Totally, thanks for suggesting it!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The method for
RoleDescriptorparsing is becoming more complex due toits usage being shared between API keys, file-based and native roles.
In some cases we allow 2.X format, in others we disallow
restrictions. Having a single method with multiple boolean flags that
control inclusion/exclusion of fields is becoming hard to extend.
This refactoring aims to allow easier introduction of new fields that
should be conditionally supported in some cases but not others.
One of such cases is introduction of
descriptionfield that should onlybe supported for file-based and native roles but not for roles embedded
in API keys.
Relates to: #107088