From 7c35e8ef7d247a87dbc496ee11324a0aa13b54bd Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Thu, 25 May 2023 08:40:28 -0400 Subject: [PATCH 1/7] update logging for slack interactive_api_endpoint --- engine/apps/slack/views.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/engine/apps/slack/views.py b/engine/apps/slack/views.py index 21097f1b0d..09be7229c2 100644 --- a/engine/apps/slack/views.py +++ b/engine/apps/slack/views.py @@ -152,14 +152,7 @@ def post(self, request): if isinstance(payload, str): payload = json.JSONDecoder().decode(payload) - logger.info( - "team_id: %s channel_id: %s user_id: %s command: %s event: %s", - payload.get("team_id"), - payload.get("channel_id"), - payload.get("user_id"), - payload.get("command"), - payload.get("event", {}).get("type"), - ) + logger.debug(f"Slack payload is {payload}") # Checking if it's repeated Slack request if "HTTP_X_SLACK_RETRY_NUM" in request.META and int(request.META["HTTP_X_SLACK_RETRY_NUM"]) > 1: From ad05085b12be037960cb287183ab0a4d4d872873 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Fri, 26 May 2023 15:25:46 -0400 Subject: [PATCH 2/7] address Organization.DoesNotExist HTTP 500 errors in slack interactive_api_endpoint --- CHANGELOG.md | 2 + engine/apps/slack/views.py | 157 +++++++++++++++++++++---------------- 2 files changed, 91 insertions(+), 68 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 796b3afab5..b76d403cef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Improve plugin authentication by @vadimkerr ([#1995](https://github.com/grafana/oncall/pull/1995)) - Fix MultipleObjectsReturned error on webhook endpoints by @vadimkerr ([#1996](https://github.com/grafana/oncall/pull/1996)) - Remove user defined time period from "you're going oncall" mobile push by @iskhakov ([#2001](https://github.com/grafana/oncall/pull/2001)) +- Properly address `Organization.DoesNotExist` exceptions thrown which result in HTTP 500 for the Slack `interactive_api_endpoint` + endpoint by @joeyorlando ([#XYZ](https://github.com/grafana/oncall/pull/XYZ)) ## v1.2.27 (2023-05-23) diff --git a/engine/apps/slack/views.py b/engine/apps/slack/views.py index 09be7229c2..603b20bfdc 100644 --- a/engine/apps/slack/views.py +++ b/engine/apps/slack/views.py @@ -55,6 +55,7 @@ from apps.slack.slack_client import SlackClientWithErrorHandling from apps.slack.slack_client.exceptions import SlackAPIException, SlackAPITokenException from apps.slack.tasks import clean_slack_integration_leftovers, unpopulate_slack_user_identities +from apps.user_management.models import Organization from common.insight_log import ChatOpsEvent, ChatOpsTypePlug, write_chatops_insight_log from common.oncall_gateway import delete_slack_connector @@ -285,6 +286,21 @@ def post(self, request): # Open pop-up to inform user why OnCall bot doesn't work if any action was triggered self._open_warning_window_if_needed(payload, slack_team_identity, warning_text) return Response(status=200) + elif organization is None: + # see this GitHub issue for more context on how this situation can arise + # https://github.com/grafana/oncall-private/issues/1836 + # tldr; > 2 stacks, stack A and stack B, use the same Slack workspace. Stack A get deleted + # the + warning_text = ( + "OnCall is not able to process this action because one of the following scenarios: \n" + "1. The Slack chatops integration was disconnected from the instance that the Alert Group belongs " + "to, BUT the Slack workspace is still connected to another instance as well. In this case, simply log " + "in to the OnCall web interface and re-install the Slack Integration with this workspace again.\n" + "2. (Less likely) The Grafana instance belonging to this Alert Group was delete. In this case the Alert Group is orphaned and cannot be acted upon." + ) + # Open pop-up to inform user why OnCall bot doesn't work if any action was triggered + self._open_warning_window_if_needed(payload, slack_team_identity, warning_text) + return Response(status=200) elif not slack_user_identity.users.exists(): # Means that slack_user_identity doesn't have any connected user # Open pop-up to inform user why OnCall bot doesn't work if any action was triggered @@ -420,76 +436,81 @@ def _get_organization_from_payload(self, payload, slack_team_identity): channel_id = None organization = None - # view submission or actions in view - if "view" in payload: - organization_id = None - private_metadata = payload["view"].get("private_metadata") - # steps with private_metadata in which we know organization before open view - if private_metadata and "organization_id" in private_metadata: - organization_id = json.loads(private_metadata).get("organization_id") - # steps with organization selection in view (e.g. slash commands) - elif SELECT_ORGANIZATION_AND_ROUTE_BLOCK_ID in payload["view"].get("state", {}).get("values", {}): - payload_values = payload["view"]["state"]["values"] - selected_value = payload_values[SELECT_ORGANIZATION_AND_ROUTE_BLOCK_ID][ - SELECT_ORGANIZATION_AND_ROUTE_BLOCK_ID - ]["selected_option"]["value"] - organization_id = int(selected_value.split("-")[0]) - if organization_id: - organization = slack_team_identity.organizations.get(pk=organization_id) - return organization - # buttons and actions - elif payload.get("type") in [ - PAYLOAD_TYPE_BLOCK_ACTIONS, - PAYLOAD_TYPE_INTERACTIVE_MESSAGE, - PAYLOAD_TYPE_MESSAGE_ACTION, - ]: - # for cases when we put organization_id into action value (e.g. public suggestion) - if ( - payload.get("actions") - and payload["actions"][0].get("value", {}) - and "organization_id" in payload["actions"][0]["value"] - ): - organization_id = int(json.loads(payload["actions"][0]["value"])["organization_id"]) - organization = slack_team_identity.organizations.get(pk=organization_id) - return organization - - channel_id = payload["channel"]["id"] - if "message" in payload: - message_ts = payload["message"].get("thread_ts") or payload["message"]["ts"] - # for interactive message - elif "message_ts" in payload: - message_ts = payload["message_ts"] - else: - return - # events - elif payload.get("type") == PAYLOAD_TYPE_EVENT_CALLBACK: - if "channel" in payload["event"]: # events without channel: user_change, events with subteam, etc. - channel_id = payload["event"]["channel"] - - if "message" in payload["event"]: - message_ts = payload["event"]["message"].get("thread_ts") or payload["event"]["message"]["ts"] - elif "thread_ts" in payload["event"]: - message_ts = payload["event"]["thread_ts"] - else: - return + try: + # view submission or actions in view + if "view" in payload: + organization_id = None + private_metadata = payload["view"].get("private_metadata") + # steps with private_metadata in which we know organization before open view + if private_metadata and "organization_id" in private_metadata: + organization_id = json.loads(private_metadata).get("organization_id") + # steps with organization selection in view (e.g. slash commands) + elif SELECT_ORGANIZATION_AND_ROUTE_BLOCK_ID in payload["view"].get("state", {}).get("values", {}): + payload_values = payload["view"]["state"]["values"] + selected_value = payload_values[SELECT_ORGANIZATION_AND_ROUTE_BLOCK_ID][ + SELECT_ORGANIZATION_AND_ROUTE_BLOCK_ID + ]["selected_option"]["value"] + organization_id = int(selected_value.split("-")[0]) + if organization_id: + organization = slack_team_identity.organizations.get(pk=organization_id) + return organization + # buttons and actions + elif payload.get("type") in [ + PAYLOAD_TYPE_BLOCK_ACTIONS, + PAYLOAD_TYPE_INTERACTIVE_MESSAGE, + PAYLOAD_TYPE_MESSAGE_ACTION, + ]: + # for cases when we put organization_id into action value (e.g. public suggestion) + if ( + payload.get("actions") + and payload["actions"][0].get("value", {}) + and "organization_id" in payload["actions"][0]["value"] + ): + organization_id = int(json.loads(payload["actions"][0]["value"])["organization_id"]) + organization = slack_team_identity.organizations.get(pk=organization_id) + return organization + + channel_id = payload["channel"]["id"] + if "message" in payload: + message_ts = payload["message"].get("thread_ts") or payload["message"]["ts"] + # for interactive message + elif "message_ts" in payload: + message_ts = payload["message_ts"] + else: + return + # events + elif payload.get("type") == PAYLOAD_TYPE_EVENT_CALLBACK: + if "channel" in payload["event"]: # events without channel: user_change, events with subteam, etc. + channel_id = payload["event"]["channel"] + + if "message" in payload["event"]: + message_ts = payload["event"]["message"].get("thread_ts") or payload["event"]["message"]["ts"] + elif "thread_ts" in payload["event"]: + message_ts = payload["event"]["thread_ts"] + else: + return - if not (message_ts and channel_id): - return + if not (message_ts and channel_id): + return - try: - slack_message = SlackMessage.objects.get( - slack_id=message_ts, - _slack_team_identity=slack_team_identity, - channel_id=channel_id, - ) - except SlackMessage.DoesNotExist: - pass - else: - alert_group = slack_message.get_alert_group() - if alert_group: - organization = alert_group.channel.organization - return organization - return organization + try: + slack_message = SlackMessage.objects.get( + slack_id=message_ts, + _slack_team_identity=slack_team_identity, + channel_id=channel_id, + ) + except SlackMessage.DoesNotExist: + pass + else: + alert_group = slack_message.get_alert_group() + if alert_group: + organization = alert_group.channel.organization + return organization + return organization + except Organization.DoesNotExist: + # see this GitHub issue for more context on how this situation can arise + # https://github.com/grafana/oncall-private/issues/1836 + return None def _open_warning_window_if_needed(self, payload, slack_team_identity, warning_text) -> None: if payload.get("trigger_id") is not None: From cccb227951d316b559c4558512525f75e8d8065b Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Fri, 26 May 2023 15:28:29 -0400 Subject: [PATCH 3/7] update comment --- engine/apps/slack/views.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/engine/apps/slack/views.py b/engine/apps/slack/views.py index 603b20bfdc..5350f549f3 100644 --- a/engine/apps/slack/views.py +++ b/engine/apps/slack/views.py @@ -289,8 +289,6 @@ def post(self, request): elif organization is None: # see this GitHub issue for more context on how this situation can arise # https://github.com/grafana/oncall-private/issues/1836 - # tldr; > 2 stacks, stack A and stack B, use the same Slack workspace. Stack A get deleted - # the warning_text = ( "OnCall is not able to process this action because one of the following scenarios: \n" "1. The Slack chatops integration was disconnected from the instance that the Alert Group belongs " From 3e7c662af69d49768f5845d83f8213271e9659f0 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Fri, 26 May 2023 15:28:57 -0400 Subject: [PATCH 4/7] update warning_text grammar --- engine/apps/slack/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine/apps/slack/views.py b/engine/apps/slack/views.py index 509080ba57..7572c05bad 100644 --- a/engine/apps/slack/views.py +++ b/engine/apps/slack/views.py @@ -294,7 +294,7 @@ def post(self, request): "1. The Slack chatops integration was disconnected from the instance that the Alert Group belongs " "to, BUT the Slack workspace is still connected to another instance as well. In this case, simply log " "in to the OnCall web interface and re-install the Slack Integration with this workspace again.\n" - "2. (Less likely) The Grafana instance belonging to this Alert Group was delete. In this case the Alert Group is orphaned and cannot be acted upon." + "2. (Less likely) The Grafana instance belonging to this Alert Group was deleted. In this case the Alert Group is orphaned and cannot be acted upon." ) # Open pop-up to inform user why OnCall bot doesn't work if any action was triggered self._open_warning_window_if_needed(payload, slack_team_identity, warning_text) From eb10ca96d1be0385db328717e772e1c35d614205 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Fri, 26 May 2023 15:29:41 -0400 Subject: [PATCH 5/7] update changelog --- CHANGELOG.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cd22e40cd5..980cfc67b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,13 +10,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added - Add models and framework to use different services (Phone, SMS, Verify) in Twilio depending on -the destination country code by @mderynck ([#1976](https://github.com/grafana/oncall/pull/1976)) + the destination country code by @mderynck ([#1976](https://github.com/grafana/oncall/pull/1976)) - Prometheus exporter backend for alert groups related metrics ### Fixed - Fix error when updating closed modal window in Slack by @vadimkerr ([#2019](https://github.com/grafana/oncall/pull/2019)) - Fix final schedule export failing to update when ical imported events set start/end as date ([#2025](https://github.com/grafana/oncall/pull/2025)) +- Properly address `Organization.DoesNotExist` exceptions thrown which result in HTTP 500 for the Slack `interactive_api_endpoint` + endpoint by @joeyorlando ([#XYZ](https://github.com/grafana/oncall/pull/XYZ)) ### Changed @@ -52,8 +54,6 @@ the destination country code by @mderynck ([#1976](https://github.com/grafana/on - Improve plugin authentication by @vadimkerr ([#1995](https://github.com/grafana/oncall/pull/1995)) - Fix MultipleObjectsReturned error on webhook endpoints by @vadimkerr ([#1996](https://github.com/grafana/oncall/pull/1996)) - Remove user defined time period from "you're going oncall" mobile push by @iskhakov ([#2001](https://github.com/grafana/oncall/pull/2001)) -- Properly address `Organization.DoesNotExist` exceptions thrown which result in HTTP 500 for the Slack `interactive_api_endpoint` - endpoint by @joeyorlando ([#XYZ](https://github.com/grafana/oncall/pull/XYZ)) ## v1.2.27 (2023-05-23) From 5a25cf84763946ac1c98e504fc9da5f63b93984d Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Fri, 26 May 2023 15:29:53 -0400 Subject: [PATCH 6/7] update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 980cfc67b5..7ce44c6ecb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix error when updating closed modal window in Slack by @vadimkerr ([#2019](https://github.com/grafana/oncall/pull/2019)) - Fix final schedule export failing to update when ical imported events set start/end as date ([#2025](https://github.com/grafana/oncall/pull/2025)) - Properly address `Organization.DoesNotExist` exceptions thrown which result in HTTP 500 for the Slack `interactive_api_endpoint` - endpoint by @joeyorlando ([#XYZ](https://github.com/grafana/oncall/pull/XYZ)) + endpoint by @joeyorlando ([#2040](https://github.com/grafana/oncall/pull/2040)) ### Changed From 6024a28086d755f722c57025b8bd037ccd140b00 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Mon, 29 May 2023 12:58:35 -0400 Subject: [PATCH 7/7] add integration tests --- .../tests/test_interactive_api_endpoint.py | 96 +++++++++++++++++++ 1 file changed, 96 insertions(+) create mode 100644 engine/apps/slack/tests/test_interactive_api_endpoint.py diff --git a/engine/apps/slack/tests/test_interactive_api_endpoint.py b/engine/apps/slack/tests/test_interactive_api_endpoint.py new file mode 100644 index 0000000000..99a6acdd0b --- /dev/null +++ b/engine/apps/slack/tests/test_interactive_api_endpoint.py @@ -0,0 +1,96 @@ +import json +from unittest.mock import call, patch + +import pytest +from rest_framework import status +from rest_framework.test import APIClient + +from apps.slack.scenarios.scenario_step import PAYLOAD_TYPE_BLOCK_ACTIONS + +EVENT_TRIGGER_ID = "5333959822612.4122782784722.4734ff484b2ac4d36a185bb242ee9932" +WARNING_TEXT = ( + "OnCall is not able to process this action because one of the following scenarios: \n" + "1. The Slack chatops integration was disconnected from the instance that the Alert Group belongs " + "to, BUT the Slack workspace is still connected to another instance as well. In this case, simply log " + "in to the OnCall web interface and re-install the Slack Integration with this workspace again.\n" + "2. (Less likely) The Grafana instance belonging to this Alert Group was deleted. In this case the Alert Group is orphaned and cannot be acted upon." +) + + +def _make_request(payload): + return APIClient().post( + "/slack/interactive_api_endpoint/", + format="json", + data=payload, + **{ + "HTTP_X_SLACK_SIGNATURE": "asdfasdf", + "HTTP_X_SLACK_REQUEST_TIMESTAMP": "xxcxcvx", + }, + ) + + +@patch("apps.slack.views.SlackEventApiEndpointView.verify_signature", return_value=True) +@patch("apps.slack.views.SlackEventApiEndpointView._open_warning_window_if_needed") +@pytest.mark.django_db +def test_organization_not_found_scenario_properly_handled( + mock_open_warning_window_if_needed, + _mock_verify_signature, + make_organization, + make_slack_user_identity, + make_slack_team_identity, +): + slack_team_id = "T043LP0P2M8" + slack_access_token = "asdfasdf" + slack_bot_access_token = "cmncvmnvcnm" + slack_bot_user_id = "mncvnmvcmnvcmncv,,cx," + + slack_user_id = "iurtiurituritu" + + def _make_slack_team_identity(): + return make_slack_team_identity( + slack_id=slack_team_id, + detected_token_revoked=None, + access_token=slack_access_token, + bot_access_token=slack_bot_access_token, + bot_user_id=slack_bot_user_id, + ) + + # SCENARIO 1 + # two orgs connected to same slack workspace, the one belonging to the alert group/slack message + # is no longer connected to the slack workspace, but another org still is + slack_team_identity = _make_slack_team_identity() + make_slack_user_identity(slack_team_identity=slack_team_identity, slack_id=slack_user_id) + + make_organization(slack_team_identity=slack_team_identity) + org2 = make_organization() + event_payload_actions = [ + { + "value": json.dumps({"organization_id": org2.id}), + } + ] + + event_payload = { + "type": PAYLOAD_TYPE_BLOCK_ACTIONS, + "trigger_id": EVENT_TRIGGER_ID, + "user": { + "id": slack_user_id, + }, + "team": { + "id": slack_team_id, + }, + "actions": event_payload_actions, + } + + response = _make_request(event_payload) + assert response.status_code == status.HTTP_200_OK + + # SCENARIO 2 + # the org that was associated w/ the alert group, has since been deleted + # and the slack message is now orphaned + org2.hard_delete() + + response = _make_request(event_payload) + assert response.status_code == status.HTTP_200_OK + + mock_call = call(event_payload, slack_team_identity, WARNING_TEXT) + mock_open_warning_window_if_needed.assert_has_calls([mock_call, mock_call])