From ca8851f1d41ffd7e7ba6f960c20c86009c4da232 Mon Sep 17 00:00:00 2001 From: Ashish Agrawal Date: Mon, 17 Jul 2023 18:14:23 -0700 Subject: [PATCH] Fix getAlerts RBAC problem (#991) (#1045) Signed-off-by: Ashish Agrawal --- .../transport/TransportGetAlertsAction.kt | 52 ++++++++++--------- .../resthandler/SecureMonitorRestApiIT.kt | 44 ++++++++++++++++ 2 files changed, 72 insertions(+), 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 index 1325b547c..a33bb2508 100644 --- 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 @@ -38,13 +36,13 @@ 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.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( AlertingActions.GET_ALERTS_ACTION_NAME, transportService, actionFilters, ::GetAlertsRequest ), @@ -150,30 +147,37 @@ 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) AlertIndices.ALL_ALERT_INDEX_PATTERN else 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 index f9a3e65e4..641608fe3 100644 --- a/alerting/src/test/kotlin/org/opensearch/alerting/resthandler/SecureMonitorRestApiIT.kt +++ b/alerting/src/test/kotlin/org/opensearch/alerting/resthandler/SecureMonitorRestApiIT.kt @@ -1206,6 +1206,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() + 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()