Fix filter by user.backendroles and integ tests for it

alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/transport/TransportGetAlertsAction.kt

@@ -134,8 +134,8 @@ class TransportGetAlertsAction @Inject constructor(
             } else {
                 // security is enabled and filterby is enabled.
                 try {
-                    addFilter(user as User, searchSourceBuilder, "monitor_user.backend_roles")
                     log.info("Filtering result by: ${user?.backendRoles}")
+                    addFilter(user as User, searchSourceBuilder, "monitor_user.backend_roles.keyword")
                     search(searchSourceBuilder, actionListener)
                 } catch (ex: IOException) {
                     actionListener.onFailure(AlertingException.wrap(ex))

alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/transport/TransportGetDestinationsAction.kt

@@ -135,8 +135,8 @@ class TransportGetDestinationsAction @Inject constructor(
         } else {
             // security is enabled and filterby is enabled.
             try {
-                addFilter(user as User, searchSourceBuilder, "destination.user.backend_roles")
                 log.info("Filtering result by: ${user?.backendRoles}")
+                addFilter(user as User, searchSourceBuilder, "destination.user.backend_roles.keyword")
                 search(searchSourceBuilder, actionListener)
             } catch (ex: IOException) {
                 actionListener.onFailure(AlertingException.wrap(ex))

alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/transport/TransportIndexDestinationAction.kt

@@ -9,6 +9,7 @@ import com.amazon.opendistroforelasticsearch.alerting.settings.AlertingSettings
 import com.amazon.opendistroforelasticsearch.alerting.settings.DestinationSettings
 import com.amazon.opendistroforelasticsearch.alerting.util.AlertingException
 import com.amazon.opendistroforelasticsearch.alerting.util.IndexUtils
+import com.amazon.opendistroforelasticsearch.alerting.util.checkFilterByUserBackendRoles
 import com.amazon.opendistroforelasticsearch.commons.ConfigConstants
 import com.amazon.opendistroforelasticsearch.commons.authuser.User
 import org.apache.logging.log4j.LogManager
@@ -49,18 +50,23 @@ class TransportIndexDestinationAction @Inject constructor(
 
     @Volatile private var indexTimeout = AlertingSettings.INDEX_TIMEOUT.get(settings)
     @Volatile private var allowList = DestinationSettings.ALLOW_LIST.get(settings)
+    @Volatile private var filterByEnabled = AlertingSettings.FILTER_BY_BACKEND_ROLES.get(settings)
     private var user: User? = null
 
     init {
         clusterService.clusterSettings.addSettingsUpdateConsumer(AlertingSettings.INDEX_TIMEOUT) { indexTimeout = it }
         clusterService.clusterSettings.addSettingsUpdateConsumer(DestinationSettings.ALLOW_LIST) { allowList = it }
+        clusterService.clusterSettings.addSettingsUpdateConsumer(AlertingSettings.FILTER_BY_BACKEND_ROLES) { filterByEnabled = it }
     }
 
     override fun doExecute(task: Task, request: IndexDestinationRequest, actionListener: ActionListener<IndexDestinationResponse>) {
         val userStr = client.threadPool().threadContext.getTransient<String>(ConfigConstants.OPENDISTRO_SECURITY_USER_AND_ROLES)
         log.debug("User and roles string from thread context: $userStr")
         user = User.parse(userStr)
 
+        if (!checkFilterByUserBackendRoles(filterByEnabled, user, actionListener)) {
+            return
+        }
         client.threadPool().threadContext.stashContext().use {
             IndexDestinationHandler(client, actionListener, request, user).resolveUserAndStart()
         }

alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/transport/TransportIndexMonitorAction.kt

@@ -24,14 +24,17 @@ import com.amazon.opendistroforelasticsearch.alerting.core.model.ScheduledJob.Co
 import com.amazon.opendistroforelasticsearch.alerting.core.model.ScheduledJob.Companion.SCHEDULED_JOB_TYPE
 import com.amazon.opendistroforelasticsearch.alerting.core.model.SearchInput
 import com.amazon.opendistroforelasticsearch.alerting.model.Monitor
+import com.amazon.opendistroforelasticsearch.alerting.settings.AlertingSettings
 import com.amazon.opendistroforelasticsearch.alerting.settings.AlertingSettings.Companion.ALERTING_MAX_MONITORS
+import com.amazon.opendistroforelasticsearch.alerting.settings.AlertingSettings.Companion.FILTER_BY_BACKEND_ROLES
 import com.amazon.opendistroforelasticsearch.alerting.settings.AlertingSettings.Companion.INDEX_TIMEOUT
 import com.amazon.opendistroforelasticsearch.alerting.settings.AlertingSettings.Companion.MAX_ACTION_THROTTLE_VALUE
 import com.amazon.opendistroforelasticsearch.alerting.settings.AlertingSettings.Companion.REQUEST_TIMEOUT
 import com.amazon.opendistroforelasticsearch.alerting.settings.DestinationSettings.Companion.ALLOW_LIST
 import com.amazon.opendistroforelasticsearch.alerting.util.AlertingException
 import com.amazon.opendistroforelasticsearch.alerting.util.IndexUtils
 import com.amazon.opendistroforelasticsearch.alerting.util.addUserBackendRolesFilter
+import com.amazon.opendistroforelasticsearch.alerting.util.checkFilterByUserBackendRoles
 import com.amazon.opendistroforelasticsearch.alerting.util.isADMonitor
 import com.amazon.opendistroforelasticsearch.commons.ConfigConstants
 import com.amazon.opendistroforelasticsearch.commons.authuser.User
@@ -88,6 +91,7 @@ class TransportIndexMonitorAction @Inject constructor(
     @Volatile private var indexTimeout = INDEX_TIMEOUT.get(settings)
     @Volatile private var maxActionThrottle = MAX_ACTION_THROTTLE_VALUE.get(settings)
     @Volatile private var allowList = ALLOW_LIST.get(settings)
+    @Volatile private var filterByEnabled = AlertingSettings.FILTER_BY_BACKEND_ROLES.get(settings)
     var user: User? = null
 
     init {
@@ -96,6 +100,7 @@ class TransportIndexMonitorAction @Inject constructor(
         clusterService.clusterSettings.addSettingsUpdateConsumer(INDEX_TIMEOUT) { indexTimeout = it }
         clusterService.clusterSettings.addSettingsUpdateConsumer(MAX_ACTION_THROTTLE_VALUE) { maxActionThrottle = it }
         clusterService.clusterSettings.addSettingsUpdateConsumer(ALLOW_LIST) { allowList = it }
+        clusterService.clusterSettings.addSettingsUpdateConsumer(FILTER_BY_BACKEND_ROLES) { filterByEnabled = it }
     }
 
     override fun doExecute(task: Task, request: IndexMonitorRequest, actionListener: ActionListener<IndexMonitorResponse>) {
@@ -104,6 +109,10 @@ class TransportIndexMonitorAction @Inject constructor(
         log.debug("User and roles string from thread context: $userStr")
         user = User.parse(userStr)
 
+        if (!checkFilterByUserBackendRoles(filterByEnabled, user, actionListener)) {
+            return
+        }
+
         if (!isADMonitor(request.monitor)) {
             checkIndicesAndExecute(client, actionListener, request, user)
         } else {

alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/transport/TransportSearchMonitorAction.kt

@@ -72,8 +72,8 @@ class TransportSearchMonitorAction @Inject constructor(
             search(searchMonitorRequest.searchRequest, actionListener)
         } else {
             // security is enabled and filterby is enabled.
-            addFilter(user as User, searchMonitorRequest.searchRequest.source(), "monitor.user.backend_roles")
             log.info("Filtering result by: ${user?.backendRoles}")
+            addFilter(user as User, searchMonitorRequest.searchRequest.source(), "monitor.user.backend_roles.keyword")
             search(searchMonitorRequest.searchRequest, actionListener)
         }
     }

alerting/src/main/kotlin/com/amazon/opendistroforelasticsearch/alerting/util/AlertingUtils.kt

@@ -17,6 +17,10 @@ package com.amazon.opendistroforelasticsearch.alerting.util
 
 import com.amazon.opendistroforelasticsearch.alerting.model.destination.Destination
 import com.amazon.opendistroforelasticsearch.alerting.settings.DestinationSettings
+import com.amazon.opendistroforelasticsearch.commons.authuser.User
+import org.elasticsearch.ElasticsearchStatusException
+import org.elasticsearch.action.ActionListener
+import org.elasticsearch.rest.RestStatus
 
 /**
  * RFC 5322 compliant pattern matching: https://www.ietf.org/rfc/rfc5322.txt
@@ -36,3 +40,35 @@ fun isValidEmail(email: String): Boolean {
 
 /** 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)
+
+/**
+  1. If filterBy is enabled
+     a) Don't allow to create monitor/ destination (throw error) if the logged-on user has no backend roles configured.
+  2. If filterBy is enabled & monitors are created when filterBy is disabled:
+     a) If backend_roles are saved with config, results will get filtered and data is shown
+     b) If backend_roles are not saved with monitor config, results will get filtered and no monitors
+        will be displayed.
+     c) Users can edit and save the monitors to associate their backend_roles.
+  3. If filterBy is enabled & monitors are created by older version:
+     a) No User details are present on monitor.
+     b) No monitors will be displayed.
+     c) Users can edit and save the monitors to associate their backend_roles.
+ */
+fun <T : Any> checkFilterByUserBackendRoles(filterByEnabled: Boolean, user: User?, actionListener: ActionListener<T>): Boolean {
+    if (filterByEnabled) {
+        if (user == null) {
+            actionListener.onFailure(AlertingException.wrap(
+                    ElasticsearchStatusException(
+                            "Filter by user backend roles is not enabled with security disabled.", RestStatus.FORBIDDEN
+                    )
+            ))
+            return false
+        } else if (user.backendRoles.isNullOrEmpty()) {
+            actionListener.onFailure(AlertingException.wrap(
+                    ElasticsearchStatusException("User doesn't have backend roles configured. Contact administrator.", RestStatus.FORBIDDEN)
+            ))
+            return false
+        }
+    }
+    return true
+}

alerting/src/test/kotlin/com/amazon/opendistroforelasticsearch/alerting/resthandler/SecureDestinationRestApiIT.kt

@@ -16,17 +16,76 @@
 package com.amazon.opendistroforelasticsearch.alerting.resthandler
 
 import com.amazon.opendistroforelasticsearch.alerting.AlertingRestTestCase
+import com.amazon.opendistroforelasticsearch.alerting.DESTINATION_BASE_URI
+import com.amazon.opendistroforelasticsearch.alerting.makeRequest
+import com.amazon.opendistroforelasticsearch.alerting.model.destination.Chime
 import com.amazon.opendistroforelasticsearch.alerting.model.destination.Destination
 import com.amazon.opendistroforelasticsearch.alerting.model.destination.Slack
 import com.amazon.opendistroforelasticsearch.alerting.randomUser
 import com.amazon.opendistroforelasticsearch.alerting.util.DestinationType
+import org.elasticsearch.client.ResponseException
+import org.elasticsearch.rest.RestStatus
 import org.elasticsearch.test.junit.annotations.TestLogging
 import java.time.Instant
 
 @TestLogging("level:DEBUG", reason = "Debug for tests.")
 @Suppress("UNCHECKED_CAST")
 class SecureDestinationRestApiIT : AlertingRestTestCase() {
 
+    fun `test create destination with disable filter by`() {
+        disableFilterBy()
+
+        val chime = Chime("http://abc.com")
+        val destination = Destination(
+                type = DestinationType.CHIME,
+                name = "test",
+                user = randomUser(),
+                lastUpdateTime = Instant.now(),
+                chime = chime,
+                slack = null,
+                customWebhook = null,
+                email = null)
+        val createdDestination = createDestination(destination = destination)
+        assertEquals("Incorrect destination name", createdDestination.name, "test")
+        assertEquals("Incorrect destination type", createdDestination.type, DestinationType.CHIME)
+    }
+
+    fun `test create destination with enable filter by`() {
+        enableFilterBy()
+        val chime = Chime("http://abc.com")
+        val destination = Destination(
+                type = DestinationType.CHIME,
+                name = "test",
+                user = randomUser(),
+                lastUpdateTime = Instant.now(),
+                chime = chime,
+                slack = null,
+                customWebhook = null,
+                email = null)
+
+        if (isHttps()) {
+            // when security is enabled. No errors, must succeed.
+            val response = client().makeRequest(
+                    "POST",
+                    "$DESTINATION_BASE_URI?refresh=true",
+                    emptyMap(),
+                    destination.toHttpEntity())
+            assertEquals("Create monitor failed", RestStatus.CREATED, response.restStatus())
+        } else {
+            // when security is disable. Must return Forbidden.
+            try {
+                client().makeRequest(
+                        "POST",
+                        "$DESTINATION_BASE_URI?refresh=true",
+                        emptyMap(),
+                        destination.toHttpEntity())
+                fail("Expected 403 FORBIDDEN response")
+            } catch (e: ResponseException) {
+                assertEquals("Unexpected status", RestStatus.FORBIDDEN, e.response.restStatus())
+            }
+        }
+    }
+
     fun `test get destinations with a destination type and disable filter by`() {
         disableFilterBy()
         val slack = Slack("url")
@@ -48,16 +107,17 @@ class SecureDestinationRestApiIT : AlertingRestTestCase() {
         inputMap["destinationType"] = "slack"
 
         // 2. get destinations as admin user
-        /*val adminResponse = getDestinations(client(), inputMap, getHeader())
-        assertEquals(1, adminResponse.size)*/
-
-        // 3. get destinations as kirk user, super-admin can read all.
-        val kirkResponse = getDestinations(adminClient(), inputMap)
-        assertEquals(1, kirkResponse.size)
+        val adminResponse = getDestinations(client(), inputMap)
+        assertEquals(1, adminResponse.size)
     }
 
     fun `test get destinations with a destination type and filter by`() {
         enableFilterBy()
+        if (!isHttps()) {
+            // if security is disabled and filter by is enabled, we can't create monitor
+            // refer: `test create destination with enable filter by`
+            return
+        }
         val slack = Slack("url")
         val destination = Destination(
                 type = DestinationType.SLACK,
@@ -77,16 +137,7 @@ class SecureDestinationRestApiIT : AlertingRestTestCase() {
         inputMap["destinationType"] = "slack"
 
         // 2. get destinations as admin user
-        /*val adminResponse = getDestinations(client(), inputMap, getHeader())
-        val expected = when (isHttps()) {
-            true -> 1   // when test is run with security - get the correct filtered results.
-            false -> 1  // when test is run without security and filterby is enabled - filtering
-                        // does not work without security, so filtering is ignored and gets a result
-        }
-        assertEquals(expected, adminResponse.size)*/
-
-        // 3. get destinations as kirk user, super-admin can read all.
-        val kirkResponse = getDestinations(adminClient(), inputMap)
-        assertEquals(1, kirkResponse.size)
+        val adminResponse = getDestinations(client(), inputMap)
+        assertEquals(1, adminResponse.size)
     }
 }