fix no permission when preview detector with detector id #294
Conversation
Codecov Report
@@ Coverage Diff @@
## master #294 +/- ##
============================================
- Coverage 70.98% 70.98% -0.01%
- Complexity 1866 1867 +1
============================================
Files 195 195
Lines 9024 9029 +5
Branches 764 764
============================================
+ Hits 6406 6409 +3
- Misses 2252 2256 +4
+ Partials 366 364 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
ylwu-amzn
changed the title
| client.get(getRequest, onGetAnomalyDetectorResponse(channel, input)); | ||
| } catch (Exception e) { | ||
| logger.error("Fail to get detector for preview", e); | ||
| channel.sendResponse(new BytesRestResponse(RestStatus.INTERNAL_SERVER_ERROR, e.getMessage())); |
There was a problem hiding this comment.
Do we always throw RestStatus.INTERNAL_SERVER_ERROR or is there any one specific to FGAC permissions errors? I'm just wondering if the frontend will propagate this correctly as a permissions issue.
There was a problem hiding this comment.
Good catch. This change will run without any user info just like super admin. So if any exception happen, it should not be permission error. For permission error we should return "RestStatus.FORBIDDEN".
It's hard to test this error as admin can always get detector. Let's tune the error message part later if it really happens.
There was a problem hiding this comment.
This might be a problem.
We are not validating user's permission to preview the detector.
Which means any user can do a preview. Is that safe to assume?
Also Stashing User context in Rest API will lead into security vulnerabilities.
Longer term I think we have to move preview to transport layer and have a dedicated action for it.
Preview returns sampled data, most results are interpolated values. And we have read permission check of data index when create detector. So when user preview detector, they already have read permission to get feature data. Definitely, this is some short term solution to quickly fix the alerting side user experience. We should move preview to transport action in long term. |
Issue #, if available:
Description of changes:
When FGAC enabled and preview detector with detector id, it throws no permission exception as can't read detector config index.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.