Fix getAlerts RBAC problem (#991) (#997) (#998)

Signed-off-by: Ashish Agrawal <[email protected]> (cherry picked from commit c535349) Co-authored-by: Ashish Agrawal <[email protected]>
opensearch-project · Jul 11, 2023 · 65c3d3a · 65c3d3a
1 parent 19869b0
commit 65c3d3a
Show file tree

Hide file tree

Showing 2 changed files with 71 additions and 24 deletions.
diff --git a/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportGetAlertsAction.kt b/alerting/src/main/kotlin/org/opensearch/alerting/transport/TransportGetAlertsAction.kt
@@ -8,17 +8,15 @@ package org.opensearch.alerting.transport
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.Dispatchers
 import kotlinx.coroutines.launch
-import kotlinx.coroutines.withContext
 import org.apache.logging.log4j.LogManager
 import org.opensearch.action.ActionListener
 import org.opensearch.action.ActionRequest
+import org.opensearch.action.get.GetRequest
+import org.opensearch.action.get.GetResponse
 import org.opensearch.action.search.SearchRequest
 import org.opensearch.action.search.SearchResponse
 import org.opensearch.action.support.ActionFilters
 import org.opensearch.action.support.HandledTransportAction
-import org.opensearch.alerting.action.GetMonitorAction
-import org.opensearch.alerting.action.GetMonitorRequest
-import org.opensearch.alerting.action.GetMonitorResponse
 import org.opensearch.alerting.alerts.AlertIndices
 import org.opensearch.alerting.opensearchapi.addFilter
 import org.opensearch.alerting.opensearchapi.suspendUntil
@@ -36,15 +34,15 @@ import org.opensearch.commons.alerting.action.AlertingActions
 import org.opensearch.commons.alerting.action.GetAlertsRequest
 import org.opensearch.commons.alerting.action.GetAlertsResponse
 import org.opensearch.commons.alerting.model.Alert
+import org.opensearch.commons.alerting.model.Monitor
+import org.opensearch.commons.alerting.model.ScheduledJob
 import org.opensearch.commons.authuser.User
 import org.opensearch.commons.utils.recreateObject
 import org.opensearch.core.xcontent.NamedXContentRegistry
 import org.opensearch.core.xcontent.XContentParser
 import org.opensearch.index.query.Operator
 import org.opensearch.index.query.QueryBuilders
-import org.opensearch.rest.RestRequest
 import org.opensearch.search.builder.SearchSourceBuilder
-import org.opensearch.search.fetch.subphase.FetchSourceContext
 import org.opensearch.search.sort.SortBuilders
 import org.opensearch.search.sort.SortOrder
 import org.opensearch.tasks.Task
@@ -60,8 +58,7 @@ class TransportGetAlertsAction @Inject constructor(
     clusterService: ClusterService,
     actionFilters: ActionFilters,
     val settings: Settings,
-    val xContentRegistry: NamedXContentRegistry,
-    val transportGetMonitorAction: TransportGetMonitorAction
+    val xContentRegistry: NamedXContentRegistry
 ) : HandledTransportAction<ActionRequest, GetAlertsResponse>(
     AlertingActions.GET_ALERTS_ACTION_NAME,
     transportService,
@@ -155,23 +152,12 @@ class TransportGetAlertsAction @Inject constructor(
      */
     suspend fun resolveAlertsIndexName(getAlertsRequest: GetAlertsRequest): String {
         var alertIndex = AlertIndices.ALL_ALERT_INDEX_PATTERN
-        if (getAlertsRequest.alertIndex.isNullOrEmpty() == false) {
+        if (!getAlertsRequest.alertIndex.isNullOrEmpty()) {
             alertIndex = getAlertsRequest.alertIndex!!
-        } else if (getAlertsRequest.monitorId.isNullOrEmpty() == false) {
-            withContext(Dispatchers.IO) {
-                val getMonitorRequest = GetMonitorRequest(
-                    getAlertsRequest.monitorId!!,
-                    -3L,
-                    RestRequest.Method.GET,
-                    FetchSourceContext.FETCH_SOURCE
-                )
-                val getMonitorResponse: GetMonitorResponse =
-                    transportGetMonitorAction.client.suspendUntil {
-                        execute(GetMonitorAction.INSTANCE, getMonitorRequest, it)
-                    }
-                if (getMonitorResponse.monitor != null) {
-                    alertIndex = getMonitorResponse.monitor!!.dataSources.alertsIndex
-                }
+        } else if (!getAlertsRequest.monitorId.isNullOrEmpty()) {
+            val retrievedMonitor = getMonitor(getAlertsRequest)
+            if (retrievedMonitor != null) {
+                alertIndex = retrievedMonitor.dataSources.alertsIndex
             }
         }
         return if (alertIndex == AlertIndices.ALERT_INDEX)
@@ -180,6 +166,23 @@ class TransportGetAlertsAction @Inject constructor(
             alertIndex
     }
 
+    private suspend fun getMonitor(getAlertsRequest: GetAlertsRequest): Monitor? {
+        val getRequest = GetRequest(ScheduledJob.SCHEDULED_JOBS_INDEX, getAlertsRequest.monitorId!!)
+        try {
+            val getResponse: GetResponse = client.suspendUntil { client.get(getRequest, it) }
+            if (!getResponse.isExists) {
+                return null
+            }
+            val xcp = XContentHelper.createParser(
+                xContentRegistry, LoggingDeprecationHandler.INSTANCE,
+                getResponse.sourceAsBytesRef, XContentType.JSON
+            )
+            return ScheduledJob.parse(xcp, getResponse.id, getResponse.version) as Monitor
+        } catch (t: Exception) {
+            return null
+        }
+    }
+
     fun getAlerts(
         alertIndex: String,
         searchSourceBuilder: SearchSourceBuilder,

diff --git a/alerting/src/test/kotlin/org/opensearch/alerting/resthandler/SecureMonitorRestApiIT.kt b/alerting/src/test/kotlin/org/opensearch/alerting/resthandler/SecureMonitorRestApiIT.kt
@@ -1215,6 +1215,50 @@ class SecureMonitorRestApiIT : AlertingRestTestCase() {
         }
     }
 
+    fun `test query all alerts in all states with filter by1`() {
+        enableFilterBy()
+        putAlertMappings()
+        val adminUser = User(ADMIN, listOf(ADMIN), listOf(ALL_ACCESS_ROLE), listOf())
+        var monitor = createRandomMonitor(refresh = true).copy(user = adminUser)
+        createAlert(randomAlert(monitor).copy(state = Alert.State.ACKNOWLEDGED))
+        createAlert(randomAlert(monitor).copy(state = Alert.State.COMPLETED))
+        createAlert(randomAlert(monitor).copy(state = Alert.State.ERROR))
+        createAlert(randomAlert(monitor).copy(state = Alert.State.ACTIVE))
+        randomAlert(monitor).copy(id = "foobar")
+
+        val inputMap = HashMap<String, Any>()
+        inputMap["missing"] = "_last"
+        inputMap["monitorId"] = monitor.id
+
+        // search as "admin" - must get 4 docs
+        val adminResponseMap = getAlerts(client(), inputMap).asMap()
+        assertEquals(4, adminResponseMap["totalAlerts"])
+
+        // search as userOne without alerting roles - must return 403 Forbidden
+        try {
+            getAlerts(userClient as RestClient, inputMap).asMap()
+            fail("Expected 403 FORBIDDEN response")
+        } catch (e: ResponseException) {
+            assertEquals("Unexpected status", RestStatus.FORBIDDEN, e.response.restStatus())
+        }
+//        createUserWithTestDataAndCustomRole(
+//            user,
+//            TEST_HR_INDEX,
+//            TEST_HR_ROLE,
+//            listOf(ADMIN),
+//            getClusterPermissionsFromCustomRole(ALERTING_INDEX_MONITOR_ACCESS)
+//        )
+        createUserWithRoles(user, listOf(ALERTING_FULL_ACCESS_ROLE), listOf(ADMIN), false)
+        // add alerting roles and search as userOne - must return 0 docs
+//        createUserRolesMapping(ALERTING_FULL_ACCESS_ROLE, arrayOf(user))
+        try {
+            val responseMap = getAlerts(userClient as RestClient, inputMap).asMap()
+            assertEquals(4, responseMap["totalAlerts"])
+        } finally {
+            deleteRoleMapping(ALERTING_FULL_ACCESS_ROLE)
+        }
+    }
+
     fun `test get alerts with an user with get alerts role`() {
         putAlertMappings()
         val ackAlertsUser = User(ADMIN, listOf(ADMIN), listOf(ALERTING_GET_ALERTS_ACCESS), listOf())