-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
[FEATURE][ML] User config appropriate permission checks on creating/running analytics #38928
[FEATURE][ML] User config appropriate permission checks on creating/running analytics #38928
Conversation
Pinging @elastic/ml-core |
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.
This looks good to me apart from a couple of minor comments. But I would also prefer for this to be reviewed by @droberts195 as well as Dave is has better understanding of the security framework.
...n/core/src/main/java/org/elasticsearch/xpack/core/ml/dataframe/DataFrameAnalyticsConfig.java
Outdated
Show resolved
Hide resolved
...n/core/src/main/java/org/elasticsearch/xpack/core/ml/dataframe/DataFrameAnalyticsConfig.java
Show resolved
Hide resolved
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.
Looks good. I just left a few minor comments.
...ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportPutDataFrameAnalyticsAction.java
Outdated
Show resolved
Hide resolved
...ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportPutDataFrameAnalyticsAction.java
Outdated
Show resolved
Hide resolved
...ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportPutDataFrameAnalyticsAction.java
Show resolved
Hide resolved
.../plugin/ml/src/main/java/org/elasticsearch/xpack/ml/dataframe/DataFrameAnalyticsManager.java
Outdated
Show resolved
Hide resolved
@@ -32,6 +33,10 @@ public DataFrameAnalysisConfig(Map<String, Object> config) { | |||
} | |||
} | |||
|
|||
public DataFrameAnalysisConfig(DataFrameAnalysisConfig config) { | |||
this.config = new HashMap<>(config.config); |
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.
Shall we also use this.config = Collections.unmodifiableMap(config.config)
?
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.
To avoid the nested wrapping problem with Collections.unmodifiableMap
we would have to use this.config = Collections.unmodifiableMap(new HashMap<>(config.config))
.
However, it would be better to use Collections.unmodifiableMap
in the two other constructors instead, and then this one can simply copy the reference safe in the knowledge that the other map is unmodifiable. If we make the class immutable like this then there's actually no need for this copy constructor at all...
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
This PR adds headers to the
DataFrameAnalyticsConfig
and utilizes those headers when running the analysis.NOTE:
CreateIndexAction.NAME
!=create_index
permission. Had a long chat with the security folks and apparentlyCreateIndexAction.NAME
is treated as a prefix wildcard, where ascreate_index
is that specific permission for that index pattern.When creating a role via the UI, the permission that auto-populates is
create_index
, and since that permission is enough (verified through testing), I opted to use it instead of the more expansive one.There were many calls that were simply
client.execute
but these will all fail when security is turned on due to the default user not having any perms. I used the user's headers where I thought appropriate and changed the calls to be from theML_ORIGIN