From 687e6d918206d54291813c92adfe4a2a41f7aa6d Mon Sep 17 00:00:00 2001 From: Darshit Chanpura Date: Tue, 31 Dec 2024 01:20:10 -0500 Subject: [PATCH] Introduces resource permissions for detectors Signed-off-by: Darshit Chanpura --- .../ad/constant/ADResourceScope.java | 18 +++++ .../IndexAnomalyDetectorTransportAction.java | 35 ++------- ...PreviewAnomalyDetectorTransportAction.java | 5 -- .../ForecastRunOnceTransportAction.java | 5 -- .../IndexForecasterTransportAction.java | 32 +------- .../timeseries/TimeSeriesAnalyticsPlugin.java | 70 +++++++++++++++-- .../BaseDeleteConfigTransportAction.java | 61 ++++++--------- .../BaseGetConfigTransportAction.java | 5 -- .../transport/BaseJobTransportAction.java | 2 - .../timeseries/util/ParseUtils.java | 77 +++++++------------ ...alyDetectorJobTransportActionWithUser.java | 2 - 11 files changed, 144 insertions(+), 168 deletions(-) create mode 100644 src/main/java/org/opensearch/ad/constant/ADResourceScope.java diff --git a/src/main/java/org/opensearch/ad/constant/ADResourceScope.java b/src/main/java/org/opensearch/ad/constant/ADResourceScope.java new file mode 100644 index 000000000..3344b79f4 --- /dev/null +++ b/src/main/java/org/opensearch/ad/constant/ADResourceScope.java @@ -0,0 +1,18 @@ +package org.opensearch.ad.constant; + +import org.opensearch.accesscontrol.resources.ResourceAccessScope; + +public enum ADResourceScope implements ResourceAccessScope { + AD_READ_ACCESS("ad_read_access"), + AD_FULL_ACCESS("ad_full_access"); + + private final String scopeName; + + ADResourceScope(String scopeName) { + this.scopeName = scopeName; + } + + public String getScopeName() { + return scopeName; + } +} diff --git a/src/main/java/org/opensearch/ad/transport/IndexAnomalyDetectorTransportAction.java b/src/main/java/org/opensearch/ad/transport/IndexAnomalyDetectorTransportAction.java index 5d9b69910..19fd7ec79 100644 --- a/src/main/java/org/opensearch/ad/transport/IndexAnomalyDetectorTransportAction.java +++ b/src/main/java/org/opensearch/ad/transport/IndexAnomalyDetectorTransportAction.java @@ -14,8 +14,7 @@ import static org.opensearch.ad.constant.ADCommonMessages.FAIL_TO_CREATE_DETECTOR; import static org.opensearch.ad.constant.ADCommonMessages.FAIL_TO_UPDATE_DETECTOR; import static org.opensearch.ad.settings.AnomalyDetectorSettings.AD_FILTER_BY_BACKEND_ROLES; -import static org.opensearch.timeseries.util.ParseUtils.checkFilterByBackendRoles; -import static org.opensearch.timeseries.util.ParseUtils.getConfig; +import static org.opensearch.timeseries.util.ParseUtils.*; import static org.opensearch.timeseries.util.RestHandlerUtils.wrapRestActionListener; import java.util.List; @@ -45,7 +44,6 @@ import org.opensearch.rest.RestRequest; import org.opensearch.search.builder.SearchSourceBuilder; import org.opensearch.tasks.Task; -import org.opensearch.timeseries.common.exception.TimeSeriesException; import org.opensearch.timeseries.feature.SearchFeatureDao; import org.opensearch.timeseries.function.ExecutorFunction; import org.opensearch.timeseries.util.ParseUtils; @@ -100,7 +98,7 @@ protected void doExecute(Task task, IndexAnomalyDetectorRequest request, ActionL String errorMessage = method == RestRequest.Method.PUT ? FAIL_TO_UPDATE_DETECTOR : FAIL_TO_CREATE_DETECTOR; ActionListener listener = wrapRestActionListener(actionListener, errorMessage); try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) { - resolveUserAndExecute(user, detectorId, method, listener, (detector) -> adExecute(request, user, detector, context, listener)); + resolveUserAndExecute(detectorId, method, listener, (detector) -> adExecute(request, user, detector, context, listener)); } catch (Exception e) { LOG.error(e); listener.onFailure(e); @@ -108,40 +106,17 @@ protected void doExecute(Task task, IndexAnomalyDetectorRequest request, ActionL } private void resolveUserAndExecute( - User requestedUser, String detectorId, RestRequest.Method method, ActionListener listener, Consumer function ) { try { - // Check if user has backend roles - // When filter by is enabled, block users creating/updating detectors who do not have backend roles. - if (filterByEnabled) { - String error = checkFilterByBackendRoles(requestedUser); - if (error != null) { - listener.onFailure(new TimeSeriesException(error)); - return; - } - } + if (method == RestRequest.Method.PUT) { - // requestedUser == null means security is disabled or user is superadmin. In this case we don't need to - // check if request user have access to the detector or not. But we still need to get current detector for - // this case, so we can keep current detector's user data. - boolean filterByBackendRole = requestedUser == null ? false : filterByEnabled; // Update detector request, check if user has permissions to update the detector // Get detector and verify backend roles - getConfig( - requestedUser, - detectorId, - listener, - function, - client, - clusterService, - xContentRegistry, - filterByBackendRole, - AnomalyDetector.class - ); + getConfig(detectorId, listener, function, client, clusterService, xContentRegistry, AnomalyDetector.class); } else { // Create Detector. No need to get current detector. function.accept(null); @@ -175,6 +150,8 @@ protected void adExecute( checkIndicesAndExecute(detector.getIndices(), () -> { // Don't replace detector's user when update detector // Github issue: https://github.com/opensearch-project/anomaly-detection/issues/124 + // TODO this and similar code should be updated to remove reference to a user + User detectorUser = currentDetector == null ? user : currentDetector.getUser(); IndexAnomalyDetectorActionHandler indexAnomalyDetectorActionHandler = new IndexAnomalyDetectorActionHandler( clusterService, diff --git a/src/main/java/org/opensearch/ad/transport/PreviewAnomalyDetectorTransportAction.java b/src/main/java/org/opensearch/ad/transport/PreviewAnomalyDetectorTransportAction.java index ef82c43b2..909cc51f4 100644 --- a/src/main/java/org/opensearch/ad/transport/PreviewAnomalyDetectorTransportAction.java +++ b/src/main/java/org/opensearch/ad/transport/PreviewAnomalyDetectorTransportAction.java @@ -43,7 +43,6 @@ import org.opensearch.common.inject.Inject; import org.opensearch.common.settings.Settings; import org.opensearch.common.util.concurrent.ThreadContext; -import org.opensearch.commons.authuser.User; import org.opensearch.core.action.ActionListener; import org.opensearch.core.rest.RestStatus; import org.opensearch.core.xcontent.NamedXContentRegistry; @@ -55,7 +54,6 @@ import org.opensearch.timeseries.common.exception.TimeSeriesException; import org.opensearch.timeseries.constant.CommonMessages; import org.opensearch.timeseries.constant.CommonName; -import org.opensearch.timeseries.util.ParseUtils; import org.opensearch.timeseries.util.RestHandlerUtils; import org.opensearch.transport.TransportService; @@ -103,13 +101,10 @@ protected void doExecute( ActionListener actionListener ) { String detectorId = request.getId(); - User user = ParseUtils.getUserContext(client); ActionListener listener = wrapRestActionListener(actionListener, FAIL_TO_PREVIEW_DETECTOR); try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) { resolveUserAndExecute( - user, detectorId, - filterByEnabled, listener, (anomalyDetector) -> previewExecute(request, context, listener), client, diff --git a/src/main/java/org/opensearch/forecast/transport/ForecastRunOnceTransportAction.java b/src/main/java/org/opensearch/forecast/transport/ForecastRunOnceTransportAction.java index f157c4d6f..e7f4f4e9d 100644 --- a/src/main/java/org/opensearch/forecast/transport/ForecastRunOnceTransportAction.java +++ b/src/main/java/org/opensearch/forecast/transport/ForecastRunOnceTransportAction.java @@ -32,7 +32,6 @@ import org.opensearch.common.settings.Settings; import org.opensearch.common.unit.TimeValue; import org.opensearch.common.util.concurrent.ThreadContext; -import org.opensearch.commons.authuser.User; import org.opensearch.core.action.ActionListener; import org.opensearch.core.xcontent.NamedXContentRegistry; import org.opensearch.forecast.constant.ForecastCommonMessages; @@ -71,7 +70,6 @@ import org.opensearch.timeseries.stats.StatNames; import org.opensearch.timeseries.task.TaskCacheManager; import org.opensearch.timeseries.transport.ResultProcessor; -import org.opensearch.timeseries.util.ParseUtils; import org.opensearch.timeseries.util.SecurityClientUtil; import org.opensearch.transport.TransportService; @@ -154,13 +152,10 @@ public ForecastRunOnceTransportAction( @Override protected void doExecute(Task task, ForecastResultRequest request, ActionListener listener) { String forecastID = request.getConfigId(); - User user = ParseUtils.getUserContext(client); try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) { resolveUserAndExecute( - user, forecastID, - filterByEnabled, listener, (forecaster) -> executeRunOnce(forecastID, request, listener), client, diff --git a/src/main/java/org/opensearch/forecast/transport/IndexForecasterTransportAction.java b/src/main/java/org/opensearch/forecast/transport/IndexForecasterTransportAction.java index c9bc28b72..ff5c1610b 100644 --- a/src/main/java/org/opensearch/forecast/transport/IndexForecasterTransportAction.java +++ b/src/main/java/org/opensearch/forecast/transport/IndexForecasterTransportAction.java @@ -14,9 +14,7 @@ import static org.opensearch.forecast.constant.ForecastCommonMessages.FAIL_TO_CREATE_FORECASTER; import static org.opensearch.forecast.constant.ForecastCommonMessages.FAIL_TO_UPDATE_FORECASTER; import static org.opensearch.forecast.settings.ForecastSettings.FORECAST_FILTER_BY_BACKEND_ROLES; -import static org.opensearch.timeseries.util.ParseUtils.checkFilterByBackendRoles; -import static org.opensearch.timeseries.util.ParseUtils.getConfig; -import static org.opensearch.timeseries.util.ParseUtils.getUserContext; +import static org.opensearch.timeseries.util.ParseUtils.*; import static org.opensearch.timeseries.util.RestHandlerUtils.wrapRestActionListener; import java.util.List; @@ -100,7 +98,6 @@ protected void doExecute(Task task, IndexForecasterRequest request, ActionListen ActionListener listener = wrapRestActionListener(actionListener, errorMessage); try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) { resolveUserAndExecute( - user, forecasterId, method, listener, @@ -113,41 +110,16 @@ protected void doExecute(Task task, IndexForecasterRequest request, ActionListen } private void resolveUserAndExecute( - User requestedUser, String forecasterId, RestRequest.Method method, ActionListener listener, Consumer function ) { try { - // requestedUser == null means security is disabled or user is superadmin. In this case we don't need to - // check if request user have access to the forecaster or not. But we still need to get current forecaster for - // this case, so we can keep current forecaster's user data. - boolean filterByBackendRole = requestedUser == null ? false : filterByEnabled; - - // Check if user has backend roles - // When filter by is enabled, block users creating/updating detectors who do not have backend roles. - if (filterByEnabled) { - String error = checkFilterByBackendRoles(requestedUser); - if (error != null) { - listener.onFailure(new IllegalArgumentException(error)); - return; - } - } if (method == RestRequest.Method.PUT) { // Update forecaster request, check if user has permissions to update the forecaster // Get forecaster and verify backend roles - getConfig( - requestedUser, - forecasterId, - listener, - function, - client, - clusterService, - xContentRegistry, - filterByBackendRole, - Forecaster.class - ); + getConfig(forecasterId, listener, function, client, clusterService, xContentRegistry, Forecaster.class); } else { // Create Detector. No need to get current detector. function.accept(null); diff --git a/src/main/java/org/opensearch/timeseries/TimeSeriesAnalyticsPlugin.java b/src/main/java/org/opensearch/timeseries/TimeSeriesAnalyticsPlugin.java index 719ec88ac..a138621b1 100644 --- a/src/main/java/org/opensearch/timeseries/TimeSeriesAnalyticsPlugin.java +++ b/src/main/java/org/opensearch/timeseries/TimeSeriesAnalyticsPlugin.java @@ -42,6 +42,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.opensearch.SpecialPermission; +import org.opensearch.accesscontrol.resources.ResourceService; import org.opensearch.action.ActionRequest; import org.opensearch.ad.ADJobProcessor; import org.opensearch.ad.ADTaskProfileRunner; @@ -161,6 +162,10 @@ import org.opensearch.cluster.metadata.IndexNameExpressionResolver; import org.opensearch.cluster.node.DiscoveryNodes; import org.opensearch.cluster.service.ClusterService; +import org.opensearch.common.inject.Inject; +import org.opensearch.common.lifecycle.Lifecycle; +import org.opensearch.common.lifecycle.LifecycleComponent; +import org.opensearch.common.lifecycle.LifecycleListener; import org.opensearch.common.settings.ClusterSettings; import org.opensearch.common.settings.IndexScopedSettings; import org.opensearch.common.settings.Setting; @@ -267,10 +272,7 @@ import org.opensearch.jobscheduler.spi.ScheduledJobRunner; import org.opensearch.monitor.jvm.JvmInfo; import org.opensearch.monitor.jvm.JvmService; -import org.opensearch.plugins.ActionPlugin; -import org.opensearch.plugins.Plugin; -import org.opensearch.plugins.ScriptPlugin; -import org.opensearch.plugins.SystemIndexPlugin; +import org.opensearch.plugins.*; import org.opensearch.repositories.RepositoriesService; import org.opensearch.rest.RestController; import org.opensearch.rest.RestHandler; @@ -327,7 +329,13 @@ /** * Entry point of time series analytics plugin. */ -public class TimeSeriesAnalyticsPlugin extends Plugin implements ActionPlugin, ScriptPlugin, SystemIndexPlugin, JobSchedulerExtension { +public class TimeSeriesAnalyticsPlugin extends Plugin + implements + ActionPlugin, + ScriptPlugin, + SystemIndexPlugin, + JobSchedulerExtension, + ResourcePlugin { private static final Logger LOG = LogManager.getLogger(TimeSeriesAnalyticsPlugin.class); @@ -1758,4 +1766,56 @@ public void close() { } } } + + @Override + public String getResourceType() { + return "detectors"; + } + + @Override + public String getResourceIndex() { + return CommonName.CONFIG_INDEX; + } + + @Override + public Collection> getGuiceServiceClasses() { + final List> services = new ArrayList<>(1); + services.add(GuiceHolder.class); + return services; + } + + public static class GuiceHolder implements LifecycleComponent { + + private static ResourceService resourceService; + + @Inject + public GuiceHolder(final ResourceService resourceService) { + GuiceHolder.resourceService = resourceService; + } + + public static ResourceService getResourceService() { + return resourceService; + } + + @Override + public void close() {} + + @Override + public Lifecycle.State lifecycleState() { + return null; + } + + @Override + public void addLifecycleListener(LifecycleListener listener) {} + + @Override + public void removeLifecycleListener(LifecycleListener listener) {} + + @Override + public void start() {} + + @Override + public void stop() {} + + } } diff --git a/src/main/java/org/opensearch/timeseries/transport/BaseDeleteConfigTransportAction.java b/src/main/java/org/opensearch/timeseries/transport/BaseDeleteConfigTransportAction.java index 6245464a9..b97f17ffa 100644 --- a/src/main/java/org/opensearch/timeseries/transport/BaseDeleteConfigTransportAction.java +++ b/src/main/java/org/opensearch/timeseries/transport/BaseDeleteConfigTransportAction.java @@ -30,7 +30,6 @@ import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Settings; import org.opensearch.common.util.concurrent.ThreadContext; -import org.opensearch.commons.authuser.User; import org.opensearch.core.action.ActionListener; import org.opensearch.core.rest.RestStatus; import org.opensearch.core.xcontent.NamedXContentRegistry; @@ -49,7 +48,6 @@ import org.opensearch.timeseries.model.TimeSeriesTask; import org.opensearch.timeseries.task.TaskCacheManager; import org.opensearch.timeseries.task.TaskManager; -import org.opensearch.timeseries.util.ParseUtils; import org.opensearch.timeseries.util.RestHandlerUtils; import org.opensearch.transport.TransportService; @@ -106,44 +104,33 @@ public BaseDeleteConfigTransportAction( protected void doExecute(Task task, DeleteConfigRequest request, ActionListener actionListener) { String configId = request.getConfigID(); LOG.info("Delete job {}", configId); - User user = ParseUtils.getUserContext(client); ActionListener listener = wrapRestActionListener(actionListener, FAIL_TO_DELETE_CONFIG); // By the time request reaches here, the user permissions are validated by Security plugin. try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) { - resolveUserAndExecute( - user, - configId, - filterByEnabled, - listener, - (input) -> nodeStateManager.getConfig(configId, analysisType, config -> { - if (config.isEmpty()) { - // In a mixed cluster, if delete detector request routes to node running AD1.0, then it will - // not delete detector tasks. User can re-delete these deleted detector after cluster upgraded, - // in that case, the detector is not present. - LOG.info("Can't find config {}", configId); - taskManager.deleteTasks(configId, () -> deleteJobDoc(configId, listener), listener); - return; - } - // Check if there is realtime job or batch analysis task running. If none of these running, we - // can delete the config. - getJob(configId, listener, () -> { - taskManager.getAndExecuteOnLatestConfigLevelTask(configId, batchTaskTypes, configTask -> { - if (configTask.isPresent() && !configTask.get().isDone()) { - String batchTaskName = configTask.get() instanceof ADTask ? "Historical" : "Run once"; - listener.onFailure(new OpenSearchStatusException(batchTaskName + " is running", RestStatus.BAD_REQUEST)); - } else { - taskManager.deleteTasks(configId, () -> deleteJobDoc(configId, listener), listener); - } - // false means don't reset task state as inactive/stopped state. We are checking if task has finished or not. - // So no need to reset task state. - }, transportService, false, listener); - }); - }, listener), - client, - clusterService, - xContentRegistry, - configTypeClass - ); + resolveUserAndExecute(configId, listener, (input) -> nodeStateManager.getConfig(configId, analysisType, config -> { + if (config.isEmpty()) { + // In a mixed cluster, if delete detector request routes to node running AD1.0, then it will + // not delete detector tasks. User can re-delete these deleted detector after cluster upgraded, + // in that case, the detector is not present. + LOG.info("Can't find config {}", configId); + taskManager.deleteTasks(configId, () -> deleteJobDoc(configId, listener), listener); + return; + } + // Check if there is realtime job or batch analysis task running. If none of these running, we + // can delete the config. + getJob(configId, listener, () -> { + taskManager.getAndExecuteOnLatestConfigLevelTask(configId, batchTaskTypes, configTask -> { + if (configTask.isPresent() && !configTask.get().isDone()) { + String batchTaskName = configTask.get() instanceof ADTask ? "Historical" : "Run once"; + listener.onFailure(new OpenSearchStatusException(batchTaskName + " is running", RestStatus.BAD_REQUEST)); + } else { + taskManager.deleteTasks(configId, () -> deleteJobDoc(configId, listener), listener); + } + // false means don't reset task state as inactive/stopped state. We are checking if task has finished or not. + // So no need to reset task state. + }, transportService, false, listener); + }); + }, listener), client, clusterService, xContentRegistry, configTypeClass); } catch (Exception e) { LOG.error(e); listener.onFailure(e); diff --git a/src/main/java/org/opensearch/timeseries/transport/BaseGetConfigTransportAction.java b/src/main/java/org/opensearch/timeseries/transport/BaseGetConfigTransportAction.java index b803a4851..29ceffd95 100644 --- a/src/main/java/org/opensearch/timeseries/transport/BaseGetConfigTransportAction.java +++ b/src/main/java/org/opensearch/timeseries/transport/BaseGetConfigTransportAction.java @@ -37,7 +37,6 @@ import org.opensearch.common.settings.Setting; import org.opensearch.common.settings.Settings; import org.opensearch.common.util.concurrent.ThreadContext; -import org.opensearch.commons.authuser.User; import org.opensearch.core.action.ActionListener; import org.opensearch.core.action.ActionResponse; import org.opensearch.core.common.Strings; @@ -67,7 +66,6 @@ import org.opensearch.timeseries.task.TaskCacheManager; import org.opensearch.timeseries.task.TaskManager; import org.opensearch.timeseries.util.DiscoveryNodeFilterer; -import org.opensearch.timeseries.util.ParseUtils; import org.opensearch.timeseries.util.RestHandlerUtils; import org.opensearch.timeseries.util.SecurityClientUtil; import org.opensearch.transport.TransportService; @@ -160,13 +158,10 @@ public BaseGetConfigTransportAction( public void doExecute(Task task, ActionRequest request, ActionListener actionListener) { GetConfigRequest getConfigRequest = GetConfigRequest.fromActionRequest(request); String configID = getConfigRequest.getConfigID(); - User user = ParseUtils.getUserContext(client); ActionListener listener = wrapRestActionListener(actionListener, FAIL_TO_GET_CONFIG_MSG); try (ThreadContext.StoredContext context = client.threadPool().getThreadContext().stashContext()) { resolveUserAndExecute( - user, configID, - filterByEnabled, listener, (config) -> getExecute(getConfigRequest, listener), client, diff --git a/src/main/java/org/opensearch/timeseries/transport/BaseJobTransportAction.java b/src/main/java/org/opensearch/timeseries/transport/BaseJobTransportAction.java index 99f4a69b3..5d504cffb 100644 --- a/src/main/java/org/opensearch/timeseries/transport/BaseJobTransportAction.java +++ b/src/main/java/org/opensearch/timeseries/transport/BaseJobTransportAction.java @@ -98,9 +98,7 @@ protected void doExecute(Task task, JobRequest request, ActionListener executeConfig(listener, configId, dateRange, historical, rawPath, requestTimeout, user, context), client, diff --git a/src/main/java/org/opensearch/timeseries/util/ParseUtils.java b/src/main/java/org/opensearch/timeseries/util/ParseUtils.java index 7d7ac7889..666e0590d 100644 --- a/src/main/java/org/opensearch/timeseries/util/ParseUtils.java +++ b/src/main/java/org/opensearch/timeseries/util/ParseUtils.java @@ -39,6 +39,7 @@ import org.opensearch.action.get.GetRequest; import org.opensearch.action.get.GetResponse; import org.opensearch.action.search.SearchResponse; +import org.opensearch.ad.constant.ADResourceScope; import org.opensearch.client.Client; import org.opensearch.cluster.service.ClusterService; import org.opensearch.common.xcontent.LoggingDeprecationHandler; @@ -70,6 +71,7 @@ import org.opensearch.search.aggregations.bucket.range.DateRangeAggregationBuilder; import org.opensearch.search.aggregations.metrics.Max; import org.opensearch.search.builder.SearchSourceBuilder; +import org.opensearch.timeseries.TimeSeriesAnalyticsPlugin; import org.opensearch.timeseries.common.exception.TimeSeriesException; import org.opensearch.timeseries.constant.CommonMessages; import org.opensearch.timeseries.constant.CommonName; @@ -484,9 +486,7 @@ public static User getUserContext(Client client) { /** * run the given function based on given user * @param Config response type. Can be either GetAnomalyDetectorResponse or GetForecasterResponse - * @param requestedUser requested user * @param configId config Id - * @param filterByEnabled filter by backend is enabled * @param listener listener. We didn't provide the generic type of listener and therefore can return anything using the listener. * @param function Function to execute * @param client Client to OS. @@ -495,9 +495,7 @@ public static User getUserContext(Client client) { * @param configTypeClass the class of the ConfigType, used by the ConfigFactory to parse the correct type of Config */ public static void resolveUserAndExecute( - User requestedUser, String configId, - boolean filterByEnabled, ActionListener listener, Consumer function, Client client, @@ -506,22 +504,11 @@ public static configTypeClass ) { try { - if (requestedUser == null || configId == null) { - // requestedUser == null means security is disabled or user is superadmin. In this case we don't need to - // check if request user have access to the detector or not. + if (configId == null) { + // configId == null indicates this is a new config creation request. We do not check for resource permission on new creation function.accept(null); } else { - getConfig( - requestedUser, - configId, - listener, - function, - client, - clusterService, - xContentRegistry, - filterByEnabled, - configTypeClass - ); + getConfig(configId, listener, function, client, clusterService, xContentRegistry, configTypeClass); } } catch (Exception e) { listener.onFailure(e); @@ -529,46 +516,35 @@ public static void getConfig( - User requestUser, String configId, ActionListener listener, Consumer function, Client client, ClusterService clusterService, NamedXContentRegistry xContentRegistry, - boolean filterByBackendRole, Class configTypeClass ) { if (clusterService.state().metadata().indices().containsKey(CommonName.CONFIG_INDEX)) { + // Check whether current user has access to update the detector + validatePermissions(configId, listener); + GetRequest request = new GetRequest(CommonName.CONFIG_INDEX).id(configId); client .get( request, ActionListener .wrap( - response -> onGetConfigResponse( - response, - requestUser, - configId, - listener, - function, - xContentRegistry, - filterByBackendRole, - configTypeClass - ), + response -> onGetConfigResponse(response, configId, listener, function, xContentRegistry, configTypeClass), exception -> { logger.error("Failed to get config: " + configId, exception); listener.onFailure(exception); @@ -594,22 +570,18 @@ public static The type of Config to be processed in this method, which extends from the Config base type. * @param The type of ActionResponse to be used, which extends from the ActionResponse base type. * @param response The GetResponse from the getConfig request. This contains the information about the config that is to be processed. - * @param requestUser The User from the request. This user's permissions will be checked to ensure they have access to the config. * @param configId The ID of the config. This is used for logging and error messages. * @param listener The ActionListener to call if an error occurs. Any errors that occur during the processing of the config will be passed to this listener. * @param function The Consumer function to apply to the ConfigType. If the user has permission to access the config, this function will be applied. * @param xContentRegistry The XContentRegistry used to create the XContentParser. This is used to parse the response into a ConfigType. - * @param filterByBackendRole A boolean indicating whether to filter by backend role. If true, the user's backend roles will be checked to ensure they have access to the config. * @param configTypeClass The class of the ConfigType, used by the ConfigFactory to parse the correct type of Config. */ public static void onGetConfigResponse( GetResponse response, - User requestUser, String configId, ActionListener listener, Consumer function, NamedXContentRegistry xContentRegistry, - boolean filterByBackendRole, Class configTypeClass ) { if (response.isExists()) { @@ -619,17 +591,11 @@ public static getFieldNamesForFeature(Feature feature, NamedXConten return ParseUtils.parseAggregationRequest(parser); } + public static void validatePermissions(String detectorId, ActionListener listener) { + // TODO the scope supplied here needs to be dynamically applicable to each function call type + // i.e. it should be different for adExecute(), forecastExecute(), etc. + boolean hasPermission = TimeSeriesAnalyticsPlugin.GuiceHolder + .getResourceService() + .getResourceAccessControlPlugin() + .hasPermission(detectorId, CommonName.CONFIG_INDEX, ADResourceScope.AD_FULL_ACCESS.getScopeName()); + + if (!hasPermission) { + logger.debug("Current user does not have permissions to access detector: " + detectorId); + listener + .onFailure(new OpenSearchStatusException(CommonMessages.NO_PERMISSION_TO_ACCESS_CONFIG + detectorId, RestStatus.FORBIDDEN)); + } + } + } diff --git a/src/test/java/org/opensearch/ad/mock/transport/MockAnomalyDetectorJobTransportActionWithUser.java b/src/test/java/org/opensearch/ad/mock/transport/MockAnomalyDetectorJobTransportActionWithUser.java index 3adeead1c..3f93a8d33 100644 --- a/src/test/java/org/opensearch/ad/mock/transport/MockAnomalyDetectorJobTransportActionWithUser.java +++ b/src/test/java/org/opensearch/ad/mock/transport/MockAnomalyDetectorJobTransportActionWithUser.java @@ -99,9 +99,7 @@ protected void doExecute(Task task, JobRequest request, ActionListener executeDetector(listener, detectorId, rawPath, requestTimeout, user, detectionDateRange, historical), client,