Check if AD backend role is enabled (#968) (#989)

* check if ad filter is enabled

* updated integ tests

---------

Signed-off-by: Amit Galitzky <[email protected]>
(cherry picked from commit 62d3d7e)

alerting/src/main/kotlin/org/opensearch/alerting/AlertingPlugin.kt

@@ -252,7 +252,7 @@ internal class AlertingPlugin : PainlessExtension, ActionPlugin, ScriptPlugin, R
             .registerSettings(settings)
             .registerThreadPool(threadPool)
             .registerAlertIndices(alertIndices)
-            .registerInputService(InputService(client, scriptService, namedWriteableRegistry, xContentRegistry))
+            .registerInputService(InputService(client, scriptService, namedWriteableRegistry, xContentRegistry, clusterService, settings))
             .registerTriggerService(TriggerService(scriptService))
             .registerAlertService(AlertService(client, xContentRegistry, alertIndices))
             .registerDocLevelMonitorQueries(DocLevelMonitorQueries(client, clusterService))
@@ -267,7 +267,7 @@ internal class AlertingPlugin : PainlessExtension, ActionPlugin, ScriptPlugin, R
             .registerSettings(settings)
             .registerThreadPool(threadPool)
             .registerAlertIndices(alertIndices)
-            .registerInputService(InputService(client, scriptService, namedWriteableRegistry, xContentRegistry))
+            .registerInputService(InputService(client, scriptService, namedWriteableRegistry, xContentRegistry, clusterService, settings))
             .registerTriggerService(TriggerService(scriptService))
             .registerAlertService(AlertService(client, xContentRegistry, alertIndices))
             .registerDocLevelMonitorQueries(DocLevelMonitorQueries(client, clusterService))

alerting/src/main/kotlin/org/opensearch/alerting/InputService.kt

@@ -15,12 +15,15 @@ import org.opensearch.alerting.opensearchapi.suspendUntil
 import org.opensearch.alerting.util.AggregationQueryRewriter
 import org.opensearch.alerting.util.addUserBackendRolesFilter
 import org.opensearch.alerting.util.executeTransportAction
+import org.opensearch.alerting.util.getRoleFilterEnabled
 import org.opensearch.alerting.util.toMap
 import org.opensearch.alerting.workflow.WorkflowRunContext
 import org.opensearch.client.Client
+import org.opensearch.cluster.service.ClusterService
 import org.opensearch.common.io.stream.BytesStreamOutput
 import org.opensearch.common.io.stream.NamedWriteableAwareStreamInput
 import org.opensearch.common.io.stream.NamedWriteableRegistry
+import org.opensearch.common.settings.Settings
 import org.opensearch.common.xcontent.LoggingDeprecationHandler
 import org.opensearch.common.xcontent.XContentType
 import org.opensearch.commons.alerting.model.ClusterMetricsInput
@@ -44,7 +47,9 @@ class InputService(
     val client: Client,
     val scriptService: ScriptService,
     val namedWriteableRegistry: NamedWriteableRegistry,
-    val xContentRegistry: NamedXContentRegistry
+    val xContentRegistry: NamedXContentRegistry,
+    val clusterService: ClusterService,
+    val settings: Settings
 ) {
 
     private val logger = LogManager.getLogger(InputService::class.java)
@@ -197,13 +202,6 @@ class InputService(
 
             // Add user role filter for AD result
             client.threadPool().threadContext.stashContext().use {
-                // Currently we have no way to verify if user has AD read permission or not. So we always add user
-                // role filter here no matter AD backend role filter enabled or not. If we don't add user role filter
-                // when AD backend filter disabled, user can run monitor on any detector and get anomaly data even
-                // they have no AD read permission. So if domain disabled AD backend role filter, monitor runner
-                // still can't get AD result with different user backend role, even the monitor user has permission
-                // to read AD result. This is a short term solution to trade off between user experience and security.
-                //
                 // Possible long term solution:
                 // 1.Use secure rest client to send request to AD search result API. If no permission exception,
                 // that mean user has read access on AD result. Then don't need to add user role filter when query
@@ -212,7 +210,9 @@ class InputService(
                 // Monitor runner will send transport request to check permission first. If security plugin response
                 // is yes, user has permission to query AD result. If AD role filter enabled, we will add user role
                 // filter to protect data at user role level; otherwise, user can query any AD result.
-                addUserBackendRolesFilter(monitor.user, searchRequest.source())
+                if (getRoleFilterEnabled(clusterService, settings, "plugins.anomaly_detection.filter_by_backend_roles")) {
+                    addUserBackendRolesFilter(monitor.user, searchRequest.source())
+                }
                 val searchResponse: SearchResponse = client.suspendUntil { client.search(searchRequest, it) }
                 results += searchResponse.convertToMap()
             }

alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportIndexMonitorAction.kt

@@ -45,6 +45,7 @@ import org.opensearch.alerting.util.AlertingException
 import org.opensearch.alerting.util.DocLevelMonitorQueries
 import org.opensearch.alerting.util.IndexUtils
 import org.opensearch.alerting.util.addUserBackendRolesFilter
+import org.opensearch.alerting.util.getRoleFilterEnabled
 import org.opensearch.alerting.util.isADMonitor
 import org.opensearch.client.Client
 import org.opensearch.cluster.service.ClusterService
@@ -278,7 +279,9 @@ class TransportIndexMonitorAction @Inject constructor(
                     request.monitor = request.monitor
                         .copy(user = User(user.name, user.backendRoles, user.roles, user.customAttNames))
                     val searchSourceBuilder = SearchSourceBuilder().size(0)
-                    addUserBackendRolesFilter(user, searchSourceBuilder)
+                    if (getRoleFilterEnabled(clusterService, settings, "plugins.anomaly_detection.filter_by_backend_roles")) {
+                        addUserBackendRolesFilter(user, searchSourceBuilder)
+                    }
                     val searchRequest = SearchRequest().indices(".opendistro-anomaly-detectors").source(searchSourceBuilder)
                     client.search(
                         searchRequest,

alerting/src/main/kotlin/org/opensearch/alerting/util/AlertingUtils.kt

@@ -9,6 +9,8 @@ import org.apache.logging.log4j.LogManager
 import org.opensearch.alerting.model.BucketLevelTriggerRunResult
 import org.opensearch.alerting.model.destination.Destination
 import org.opensearch.alerting.settings.DestinationSettings
+import org.opensearch.cluster.service.ClusterService
+import org.opensearch.common.settings.Settings
 import org.opensearch.commons.alerting.model.AggregationResultBucket
 import org.opensearch.commons.alerting.model.Monitor
 import org.opensearch.commons.alerting.model.action.Action
@@ -38,6 +40,28 @@ fun isValidEmail(email: String): Boolean {
     return validEmailPattern.matches(email)
 }
 
+fun getRoleFilterEnabled(clusterService: ClusterService, settings: Settings, settingPath: String): Boolean {
+    var adBackendRoleFilterEnabled: Boolean
+    val metaData = clusterService.state().metadata()
+
+    // get default value for setting
+    if (clusterService.clusterSettings.get(settingPath) != null) {
+        adBackendRoleFilterEnabled = clusterService.clusterSettings.get(settingPath).getDefault(settings) as Boolean
+    } else {
+        // default setting doesn't exist, so returning false as it means AD plugins isn't in cluster anyway
+        return false
+    }
+
+    // Transient settings are prioritized so those are checked first.
+    return if (metaData.transientSettings().get(settingPath) != null) {
+        metaData.transientSettings().getAsBoolean(settingPath, adBackendRoleFilterEnabled)
+    } else if (metaData.persistentSettings().get(settingPath) != null) {
+        metaData.persistentSettings().getAsBoolean(settingPath, adBackendRoleFilterEnabled)
+    } else {
+        adBackendRoleFilterEnabled
+    }
+}
+
 /** Allowed Destinations are ones that are specified in the [DestinationSettings.ALLOW_LIST] setting. */
 fun Destination.isAllowed(allowList: List<String>): Boolean = allowList.contains(this.type.value)
 

alerting/src/test/kotlin/org/opensearch/alerting/MonitorRunnerServiceIT.kt

@@ -998,7 +998,7 @@ class MonitorRunnerServiceIT : AlertingRestTestCase() {
         }
     }
 
-    fun `test execute AD monitor doesn't return search result without user`() {
+    fun `test execute AD monitor returns search result without user`() {
         // TODO: change to REST API call to test security enabled case
         if (!securityEnabled()) {
             val user = randomUser()
@@ -1018,14 +1018,14 @@ class MonitorRunnerServiceIT : AlertingRestTestCase() {
             val searchResult = (output.objectMap("input_results")["results"] as List<Map<String, Any>>).first()
             @Suppress("UNCHECKED_CAST")
             val total = searchResult.stringMap("hits")?.get("total") as Map<String, String>
-            assertEquals("Incorrect search result", 1, total["value"])
+            assertEquals("Incorrect search result", 5, total["value"])
             @Suppress("UNCHECKED_CAST")
             val maxAnomalyGrade = searchResult.stringMap("aggregations")?.get("max_anomaly_grade") as Map<String, String>
-            assertEquals("Incorrect search result", 0.75, maxAnomalyGrade["value"])
+            assertEquals("Incorrect search result", 0.9, maxAnomalyGrade["value"])
         }
     }
 
-    fun `test execute AD monitor doesn't return search result with empty backend role`() {
+    fun `test execute AD monitor returns search result with empty backend role`() {
         // TODO: change to REST API call to test security enabled case
         if (!securityEnabled()) {
             val user = randomUser()
@@ -1048,7 +1048,7 @@ class MonitorRunnerServiceIT : AlertingRestTestCase() {
             val searchResult = (output.objectMap("input_results")["results"] as List<Map<String, Any>>).first()
             @Suppress("UNCHECKED_CAST")
             val total = searchResult.stringMap("hits")?.get("total") as Map<String, String>
-            assertEquals("Incorrect search result", 1, total["value"])
+            assertEquals("Incorrect search result", 5, total["value"])
             @Suppress("UNCHECKED_CAST")
             val maxAnomalyGrade = searchResult.stringMap("aggregations")?.get("max_anomaly_grade") as Map<String, String>
             assertEquals("Incorrect search result", 0.9, maxAnomalyGrade["value"])
@@ -1074,10 +1074,10 @@ class MonitorRunnerServiceIT : AlertingRestTestCase() {
             val searchResult = (output.objectMap("input_results")["results"] as List<Map<String, Any>>).first()
             @Suppress("UNCHECKED_CAST")
             val total = searchResult.stringMap("hits")?.get("total") as Map<String, String>
-            assertEquals("Incorrect search result", 3, total["value"])
+            assertEquals("Incorrect search result", 5, total["value"])
             @Suppress("UNCHECKED_CAST")
             val maxAnomalyGrade = searchResult.stringMap("aggregations")?.get("max_anomaly_grade") as Map<String, String>
-            assertEquals("Incorrect search result", 0.8, maxAnomalyGrade["value"])
+            assertEquals("Incorrect search result", 0.9, maxAnomalyGrade["value"])
         }
     }
 
@@ -1097,13 +1097,13 @@ class MonitorRunnerServiceIT : AlertingRestTestCase() {
             @Suppress("UNCHECKED_CAST")
             (output["trigger_results"] as HashMap<String, Any>).forEach {
                     _, v ->
-                assertFalse((v as HashMap<String, Boolean>)["triggered"] as Boolean)
+                assertTrue((v as HashMap<String, Boolean>)["triggered"] as Boolean)
             }
             @Suppress("UNCHECKED_CAST")
             val searchResult = (output.objectMap("input_results")["results"] as List<Map<String, Any>>).first()
             @Suppress("UNCHECKED_CAST")
             val total = searchResult.stringMap("hits")?.get("total") as Map<String, String>
-            assertEquals("Incorrect search result", 0, total["value"])
+            assertEquals("Incorrect search result", 5, total["value"])
         }
     }