Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

fix no permission when preview detector with detector id #294

Merged
merged 1 commit into from
Oct 26, 2020
Merged

fix no permission when preview detector with detector id #294

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion .../java/com/amazon/opendistroforelasticsearch/ad/rest/RestExecuteAnomalyDetectorAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.elasticsearch.cluster.service.ClusterService;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
Expand Down Expand Up @@ -168,7 +169,12 @@ private String validateAdExecutionInput(AnomalyDetectorExecutionInput input) {
private void preivewAnomalyDetector(NodeClient client, RestChannel channel, AnomalyDetectorExecutionInput input) {
if (!StringUtils.isBlank(input.getDetectorId())) {
GetRequest getRequest = new GetRequest(AnomalyDetector.ANOMALY_DETECTORS_INDEX).id(input.getDetectorId());
client.get(getRequest, onGetAnomalyDetectorResponse(channel, input));
try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) {
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()));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ylwu-amzn ylwu-amzn Oct 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, makes sense

}
} else {
channel.sendResponse(new BytesRestResponse(RestStatus.NOT_FOUND, "Wrong input, no detector id"));
}
Expand Down