-
Notifications
You must be signed in to change notification settings - Fork 75
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
don't replace detector user when update #126
don't replace detector user when update #126
Conversation
Signed-off-by: Yaliang Wu <ylwu@amazon.com>
Codecov Report
@@ Coverage Diff @@
## main #126 +/- ##
============================================
+ Coverage 75.97% 76.35% +0.37%
- Complexity 2886 2896 +10
============================================
Files 264 264
Lines 12435 12431 -4
Branches 1222 1221 -1
============================================
+ Hits 9448 9492 +44
+ Misses 2478 2421 -57
- Partials 509 518 +9
Flags with carried forward coverage won't be shown. Click here to find out more.
|
@@ -102,7 +102,7 @@ protected void doExecute(Task task, AnomalyDetectorJobRequest request, ActionLis | |||
detectorId, | |||
filterByEnabled, | |||
listener, | |||
() -> executeDetector(listener, detectorId, seqNo, primaryTerm, rawPath, requestTimeout, user), | |||
(anomalyDetector) -> executeDetector(listener, detectorId, seqNo, primaryTerm, rawPath, requestTimeout, user), |
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.
Curiously, it seems the anomalyDetector was not used in the executeDetector method?
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, actually the executeDetector
will get detector with detectorId. This PR is mainly to fix the security issue. Will refactor code in separate PR to reuse detector rather than query it again. Will add some TODO to make it clear
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!
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.
Create an issue to track this #127
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 to me
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.
Had a chat with @ylwu-amzn , not really happy with the path from the customer perspective.
But I agree security comes first.
Thanks for the changes, they look good to me!
* don't replace detector user when update Signed-off-by: Yaliang Wu <ylwu@amazon.com> * fix wrong doc link
* don't replace detector user when update Signed-off-by: Yaliang Wu <ylwu@amazon.com> * fix wrong doc link
* don't replace detector user when update Signed-off-by: Yaliang Wu <ylwu@amazon.com> * fix wrong doc link
Signed-off-by: Yaliang Wu ylwu@amazon.com
Description
In current implementation, when user update detector, we will replace detector user as current user. In some cases, it may cause detector creator lose access of detector. Check more details in #124
Based on discussion with @skkosuri-amzn and PM, we plan to fix this issue quickly by blocking replacing detector user when update. User can't update detector user after creation.
Issues Resolved
#124
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.