From 61fdcfdc724aa054028508bac0666f4515da4ebe Mon Sep 17 00:00:00 2001 From: Innokentii Konstantinov Date: Tue, 21 Feb 2023 09:47:52 +0100 Subject: [PATCH 01/11] Add ratelimit for phone number verification (#1354) # What this PR does ## Which issue(s) this PR fixes ## Checklist - [x] Tests updated - [x] `CHANGELOG.md` updated --------- Co-authored-by: Joey Orlando --- CHANGELOG.md | 6 ++ engine/apps/api/tests/test_user.py | 93 ++++++++++++++++++- engine/apps/api/throttlers/__init__.py | 7 ++ .../phone_verification_throttler.py | 49 ++++++++++ .../api/throttlers/test_call_throttler.py | 6 ++ engine/apps/api/views/user.py | 21 ++++- .../apps/public_api/tests/test_ratelimit.py | 30 +++--- .../public_api/throttlers/user_throttle.py | 41 +------- .../custom_rate_scoped_throttler.py | 56 +++++++++++ 9 files changed, 250 insertions(+), 59 deletions(-) create mode 100644 engine/apps/api/throttlers/phone_verification_throttler.py create mode 100644 engine/apps/api/throttlers/test_call_throttler.py create mode 100644 engine/common/api_helpers/custom_rate_scoped_throttler.py diff --git a/CHANGELOG.md b/CHANGELOG.md index bbbcd3d4..b9c27be4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## Unreleased + +### Changed + +- Add ratelimits for phone verification + ## v1.1.26 (2023-02-20) ### Fixed diff --git a/engine/apps/api/tests/test_user.py b/engine/apps/api/tests/test_user.py index 9ad3b6d5..ad90e264 100644 --- a/engine/apps/api/tests/test_user.py +++ b/engine/apps/api/tests/test_user.py @@ -1,6 +1,7 @@ from unittest.mock import Mock, patch import pytest +from django.core.cache import cache from django.core.exceptions import ObjectDoesNotExist from django.urls import reverse from django.utils import timezone @@ -13,6 +14,12 @@ from apps.base.models import UserNotificationPolicy from apps.user_management.models.user import default_working_hours +@pytest.fixture(autouse=True) +def clear_cache(): + # Ratelimit keys are stored in cache, clean to prevent ratelimits + cache.clear() + + @pytest.mark.django_db def test_update_user( make_organization, @@ -653,7 +660,6 @@ def test_admin_can_verify_own_phone( make_user_auth_headers, ): _, user, token = make_organization_and_user_with_plugin_token(role=LegacyAccessControlRole.ADMIN) - client = APIClient() url = reverse("api-internal:user-verify-number", kwargs={"pk": user.public_primary_key}) @@ -1499,3 +1505,88 @@ def test_check_availability_other_user(make_organization_and_user_with_plugin_to response = client.get(url, **make_user_auth_headers(user, token)) assert response.status_code == status.HTTP_200_OK + + +@patch("apps.twilioapp.phone_manager.PhoneManager.send_verification_code", return_value=Mock()) +@patch("apps.twilioapp.phone_manager.PhoneManager.verify_phone_number", return_value=(True, None)) +@patch( + "apps.api.throttlers.GetPhoneVerificationCodeThrottlerPerUser.get_throttle_limits", + return_value=(1, 10 * 60), +) +@patch("apps.api.throttlers.VerifyPhoneNumberThrottlerPerUser.get_throttle_limits", return_value=(1, 10 * 60)) +@pytest.mark.django_db +def test_phone_number_verification_flow_ratelimit_per_user( + mock_verification_start, + mocked_verification_check, + mocked_get_phone_verification_code_get_throttle_limits, + mocked_get_phone_verify_phone_number_limits, + make_organization_and_user_with_plugin_token, + make_user_auth_headers, +): + _, user, token = make_organization_and_user_with_plugin_token() + + client = APIClient() + url = reverse("api-internal:user-get-verification-code", kwargs={"pk": user.public_primary_key}) + + # first get_verification_code request is succesfull + response = client.get(url, format="json", **make_user_auth_headers(user, token)) + assert response.status_code == status.HTTP_200_OK + + # second get_verification_code request is ratelimited + response = client.get(url, format="json", **make_user_auth_headers(user, token)) + assert response.status_code == status.HTTP_429_TOO_MANY_REQUESTS + + url = reverse("api-internal:user-verify-number", kwargs={"pk": user.public_primary_key}) + + # first verify_number request is succesfull, because it uses different ratelimit scope + response = client.put(f"{url}?token=12345", format="json", **make_user_auth_headers(user, token)) + assert response.status_code == status.HTTP_200_OK + + url = reverse("api-internal:user-verify-number", kwargs={"pk": user.public_primary_key}) + + # second verify_number request is succesfull, because it ratelimited + response = client.put(f"{url}?token=12345", format="json", **make_user_auth_headers(user, token)) + assert response.status_code == status.HTTP_429_TOO_MANY_REQUESTS + + +@patch("apps.twilioapp.phone_manager.PhoneManager.send_verification_code", return_value=Mock()) +@patch("apps.twilioapp.phone_manager.PhoneManager.verify_phone_number", return_value=(True, None)) +@patch( + "apps.api.throttlers.GetPhoneVerificationCodeThrottlerPerOrg.get_throttle_limits", + return_value=(1, 10 * 60), +) +@patch("apps.api.throttlers.VerifyPhoneNumberThrottlerPerOrg.get_throttle_limits", return_value=(1, 10 * 60)) +@pytest.mark.django_db +def test_phone_number_verification_flow_ratelimit_per_org( + mock_verification_start, + mocked_verification_check, + mocked_get_phone_verification_code_get_throttle_limits, + mocked_get_phone_verify_phone_number_limits, + make_organization_and_user_with_plugin_token, + make_user_auth_headers, + make_user_for_organization, +): + """ + This test is checks per-org ratelimits for phone verification flow. + It makes two get_verification_code and two verify_number requests from different users and expect that second call will be ratelimited. + """ + org, user, token = make_organization_and_user_with_plugin_token() + second_user = make_user_for_organization(org) + + client = APIClient() + + url = reverse("api-internal:user-get-verification-code", kwargs={"pk": user.public_primary_key}) + response = client.get(url, format="json", **make_user_auth_headers(user, token)) + assert response.status_code == status.HTTP_200_OK + + url = reverse("api-internal:user-get-verification-code", kwargs={"pk": second_user.public_primary_key}) + response = client.get(url, format="json", **make_user_auth_headers(second_user, token)) + assert response.status_code == status.HTTP_429_TOO_MANY_REQUESTS + + url = reverse("api-internal:user-verify-number", kwargs={"pk": user.public_primary_key}) + response = client.put(f"{url}?token=12345", format="json", **make_user_auth_headers(user, token)) + assert response.status_code == status.HTTP_200_OK + + url = reverse("api-internal:user-verify-number", kwargs={"pk": second_user.public_primary_key}) + response = client.put(f"{url}?token=12345", format="json", **make_user_auth_headers(second_user, token)) + assert response.status_code == status.HTTP_429_TOO_MANY_REQUESTS diff --git a/engine/apps/api/throttlers/__init__.py b/engine/apps/api/throttlers/__init__.py index 3d04dedf..6eb19e81 100644 --- a/engine/apps/api/throttlers/__init__.py +++ b/engine/apps/api/throttlers/__init__.py @@ -1 +1,8 @@ from .demo_alert_throttler import DemoAlertThrottler # noqa: F401 +from .phone_verification_throttler import ( # noqa: F401 + GetPhoneVerificationCodeThrottlerPerOrg, + GetPhoneVerificationCodeThrottlerPerUser, + VerifyPhoneNumberThrottlerPerOrg, + VerifyPhoneNumberThrottlerPerUser, +) +from .test_call_throttler import TestCallThrottler # noqa: F401 diff --git a/engine/apps/api/throttlers/phone_verification_throttler.py b/engine/apps/api/throttlers/phone_verification_throttler.py new file mode 100644 index 00000000..1bf6fe02 --- /dev/null +++ b/engine/apps/api/throttlers/phone_verification_throttler.py @@ -0,0 +1,49 @@ +from common.api_helpers.custom_rate_scoped_throttler import CustomRateScopedThrottler + + +class GetPhoneVerificationCodeThrottlerPerUser(CustomRateScopedThrottler): + def get_scope(self): + return "get_phone_verification_code_per_user" + + def get_throttle_limits(self): + return 5, 10 * 60 + + +class VerifyPhoneNumberThrottlerPerUser(CustomRateScopedThrottler): + def get_scope(self): + return "verify_phone_number_per_user" + + def get_throttle_limits(self): + return 50, 10 * 60 + + +class GetPhoneVerificationCodeThrottlerPerOrg(CustomRateScopedThrottler): + def get_scope(self): + return "get_phone_verification_code_per_org" + + def get_throttle_limits(self): + return 50, 10 * 60 + + def get_cache_key(self, request, view): + if request.user.is_authenticated: + ident = request.user.organization.pk + else: + ident = self.get_ident(request) + + return self.cache_format % {"scope": self.scope, "ident": ident} + + +class VerifyPhoneNumberThrottlerPerOrg(CustomRateScopedThrottler): + def get_scope(self): + return "verify_phone_number_per_org" + + def get_throttle_limits(self): + return 50, 10 * 60 + + def get_cache_key(self, request, view): + if request.user.is_authenticated: + ident = request.user.organization.pk + else: + ident = self.get_ident(request) + + return self.cache_format % {"scope": self.scope, "ident": ident} diff --git a/engine/apps/api/throttlers/test_call_throttler.py b/engine/apps/api/throttlers/test_call_throttler.py new file mode 100644 index 00000000..93517163 --- /dev/null +++ b/engine/apps/api/throttlers/test_call_throttler.py @@ -0,0 +1,6 @@ +from rest_framework.throttling import UserRateThrottle + + +class TestCallThrottler(UserRateThrottle): + scope = "make_test_call" + rate = "5/m" diff --git a/engine/apps/api/views/user.py b/engine/apps/api/views/user.py index d5fc8af7..6fcd2123 100644 --- a/engine/apps/api/views/user.py +++ b/engine/apps/api/views/user.py @@ -24,6 +24,13 @@ from apps.api.permissions import ( ) from apps.api.serializers.team import TeamSerializer from apps.api.serializers.user import FilterUserSerializer, UserHiddenFieldsSerializer, UserSerializer +from apps.api.throttlers import ( + GetPhoneVerificationCodeThrottlerPerOrg, + GetPhoneVerificationCodeThrottlerPerUser, + TestCallThrottler, + VerifyPhoneNumberThrottlerPerOrg, + VerifyPhoneNumberThrottlerPerUser, +) from apps.auth_token.auth import PluginAuthentication from apps.auth_token.constants import SCHEDULE_EXPORT_TOKEN_NAME from apps.auth_token.models import UserScheduleExportAuthToken @@ -279,7 +286,11 @@ class UserView( def timezone_options(self, request): return Response(pytz.common_timezones) - @action(detail=True, methods=["get"]) + @action( + detail=True, + methods=["get"], + throttle_classes=[GetPhoneVerificationCodeThrottlerPerUser, GetPhoneVerificationCodeThrottlerPerOrg], + ) def get_verification_code(self, request, pk): user = self.get_object() phone_manager = PhoneManager(user) @@ -289,7 +300,11 @@ class UserView( return Response(status=status.HTTP_400_BAD_REQUEST) return Response(status=status.HTTP_200_OK) - @action(detail=True, methods=["put"]) + @action( + detail=True, + methods=["put"], + throttle_classes=[VerifyPhoneNumberThrottlerPerUser, VerifyPhoneNumberThrottlerPerOrg], + ) def verify_number(self, request, pk): target_user = self.get_object() code = request.query_params.get("token", None) @@ -327,7 +342,7 @@ class UserView( ) return Response(status=status.HTTP_200_OK) - @action(detail=True, methods=["post"]) + @action(detail=True, methods=["post"], throttle_classes=[TestCallThrottler]) def make_test_call(self, request, pk): user = self.get_object() phone_number = user.verified_phone_number diff --git a/engine/apps/public_api/tests/test_ratelimit.py b/engine/apps/public_api/tests/test_ratelimit.py index eb71827c..d6a74587 100644 --- a/engine/apps/public_api/tests/test_ratelimit.py +++ b/engine/apps/public_api/tests/test_ratelimit.py @@ -1,4 +1,4 @@ -from unittest.mock import patch +from unittest.mock import PropertyMock, patch import pytest from django.core.cache import cache @@ -7,27 +7,25 @@ from rest_framework import status from rest_framework.test import APIClient -@patch("apps.public_api.throttlers.user_throttle.UserThrottle.get_throttle_limits") @pytest.mark.django_db -def test_throttling(mocked_throttle_limits, make_organization_and_user_with_token): - MAX_REQUESTS = 1 - PERIOD = 360 +def test_throttling(make_organization_and_user_with_token): + with patch("apps.public_api.throttlers.user_throttle.UserThrottle.rate", new_callable=PropertyMock) as mocked_rate: + mocked_rate.return_value = "1/m" - _, _, token = make_organization_and_user_with_token() - cache.clear() + _, _, token = make_organization_and_user_with_token() + cache.clear() - client = APIClient() + client = APIClient() - mocked_throttle_limits.return_value = MAX_REQUESTS, PERIOD - url = reverse("api-public:alert_groups-list") + url = reverse("api-public:alert_groups-list") - response = client.get(url, format="json", HTTP_AUTHORIZATION=f"{token}") + response = client.get(url, format="json", HTTP_AUTHORIZATION=f"{token}") - assert response.status_code == status.HTTP_200_OK + assert response.status_code == status.HTTP_200_OK - response = client.get(url, format="json", HTTP_AUTHORIZATION=f"{token}") + response = client.get(url, format="json", HTTP_AUTHORIZATION=f"{token}") - assert response.status_code == status.HTTP_429_TOO_MANY_REQUESTS + assert response.status_code == status.HTTP_429_TOO_MANY_REQUESTS - # make sure RateLimitHeadersMixin used - assert response.has_header("RateLimit-Reset") + # make sure RateLimitHeadersMixin used + assert response.has_header("RateLimit-Reset") diff --git a/engine/apps/public_api/throttlers/user_throttle.py b/engine/apps/public_api/throttlers/user_throttle.py index 4c46e259..7c176f2e 100644 --- a/engine/apps/public_api/throttlers/user_throttle.py +++ b/engine/apps/public_api/throttlers/user_throttle.py @@ -2,42 +2,5 @@ from rest_framework.throttling import UserRateThrottle class UserThrottle(UserRateThrottle): - """ - __init__ and allow_request are overridden because we want rate 300/5m, - but default rate parser implementation doesn't allow to specify length of period (only m, d, etc.) - (See SimpleRateThrottle.parse_rate) - - """ - - def __init__(self): - self.num_requests, self.duration = self.get_throttle_limits() - - def get_throttle_limits(self): - """ - This method exits for speed up tests. - :return tuple requests/seconds - """ - return 300, 60 - - def allow_request(self, request, view): - """ - Implement the check to see if the request should be throttled. - - On success calls `throttle_success`. - On failure calls `throttle_failure`. - """ - - self.key = self.get_cache_key(request, view) - if self.key is None: - return True - - self.history = self.cache.get(self.key, []) - self.now = self.timer() - - # Drop any requests from the history which have now passed the - # throttle duration - while self.history and self.history[-1] <= self.now - self.duration: - self.history.pop() - if len(self.history) >= self.num_requests: - return self.throttle_failure() - return self.throttle_success() + scope = "public_api" + rate = "300/m" diff --git a/engine/common/api_helpers/custom_rate_scoped_throttler.py b/engine/common/api_helpers/custom_rate_scoped_throttler.py new file mode 100644 index 00000000..c8965d20 --- /dev/null +++ b/engine/common/api_helpers/custom_rate_scoped_throttler.py @@ -0,0 +1,56 @@ +from rest_framework.throttling import SimpleRateThrottle + + +class CustomRateScopedThrottler(SimpleRateThrottle): + """ + Abstract class to create throttlers with custom amount of seconds and custom scope. + The unique cache key will be generated by concatenating the + user id of the request, and the scope from get_scope() method. + + Should not be used directly. + """ + + def __init__(self): + self.scope = self.get_scope() + self.num_requests, self.duration = self.get_throttle_limits() + + def get_throttle_limits(self): + """ + :return tuple requests/seconds + """ + raise NotImplementedError + + def get_scope(self): + """ + :return ratelimit scope + """ + raise NotImplementedError + + def allow_request(self, request, view): + """ + Overriden allow_request method. + The difference is that overriden method doesn't check rate property. + """ + + self.key = self.get_cache_key(request, view) + if self.key is None: + return True + + self.history = self.cache.get(self.key, []) + self.now = self.timer() + + # Drop any requests from the history which have now passed the + # throttle duration + while self.history and self.history[-1] <= self.now - self.duration: + self.history.pop() + if len(self.history) >= self.num_requests: + return self.throttle_failure() + return self.throttle_success() + + def get_cache_key(self, request, view): + if request.user.is_authenticated: + ident = request.user.pk + else: + ident = self.get_ident(request) + + return self.cache_format % {"scope": self.scope, "ident": ident} From 951416e3ad89ce106860c36ff3965c1266c7bd18 Mon Sep 17 00:00:00 2001 From: Jack Baldry Date: Tue, 21 Feb 2023 09:19:19 +0000 Subject: [PATCH 02/11] Update publishing workflows to use organization secret (#1301) The new tokens are managed centrally and have a longer expiry. Administrators of the grafanabot account will be notified of the pending expiry and the secret can be rotated centrally without the need for a repository administrator to update their secrets. The existing repository secrets can safely be removed. The tokens for those secrets will be removed by the end of this week. Signed-off-by: Jack Baldry Signed-off-by: Jack Baldry Co-authored-by: Joey Orlando --- .../publish-technical-documentation-next.yml | 16 ++++++++++------ .../publish-technical-documentation-release.yml | 16 ++++++++++------ 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/.github/workflows/publish-technical-documentation-next.yml b/.github/workflows/publish-technical-documentation-next.yml index 5f8190a1..f8d1adcb 100644 --- a/.github/workflows/publish-technical-documentation-next.yml +++ b/.github/workflows/publish-technical-documentation-next.yml @@ -28,9 +28,11 @@ jobs: uses: "actions/checkout@v3" - name: "Clone website-sync Action" - # WEBSITE_SYNC_ONCALL is a fine-grained GitHub Personal Access Token that expires. - # It must be updated in the grafanabot GitHub account. - run: "git clone --single-branch --no-tags --depth 1 -b master https://grafanabot:${{ secrets.WEBSITE_SYNC_ONCALL }}@github.com/grafana/website-sync ./.github/actions/website-sync" + # WEBSITE_SYNC_TOKEN is a fine-grained GitHub Personal Access Token that expires. + # It must be regenerated in the grafanabot GitHub account and requires a Grafana organization + # GitHub administrator to update the organization secret. + # The IT helpdesk can update the organization secret. + run: "git clone --single-branch --no-tags --depth 1 -b master https://grafanabot:${{ secrets.WEBSITE_SYNC_TOKEN }}@github.com/grafana/website-sync ./.github/actions/website-sync" - name: "Publish to website repository (next)" uses: "./.github/actions/website-sync" @@ -39,8 +41,10 @@ jobs: repository: "grafana/website" branch: "master" host: "github.com" - # PUBLISH_TO_WEBSITE_ONCALL is a fine-grained GitHub Personal Access Token that expires. - # It must be updated in the grafanabot GitHub account. - github_pat: "grafanabot:${{ secrets.PUBLISH_TO_WEBSITE_ONCALL }}" + # PUBLISH_TO_WEBSITE_TOKEN is a fine-grained GitHub Personal Access Token that expires. + # It must be regenerated in the grafanabot GitHub account and requires a Grafana organization + # GitHub administrator to update the organization secret. + # The IT helpdesk can update the organization secret. + github_pat: "grafanabot:${{ secrets.PUBLISH_TO_WEBSITE_TOKEN }}" source_folder: "docs/sources" target_folder: "content/docs/oncall/next" diff --git a/.github/workflows/publish-technical-documentation-release.yml b/.github/workflows/publish-technical-documentation-release.yml index fefa3b5e..82c26cd4 100644 --- a/.github/workflows/publish-technical-documentation-release.yml +++ b/.github/workflows/publish-technical-documentation-release.yml @@ -58,9 +58,11 @@ jobs: - name: "Clone website-sync Action" if: "steps.has-matching-release-tag.outputs.bool == 'true'" - # WEBSITE_SYNC_ONCALL is a fine-grained GitHub Personal Access Token that expires. - # It must be updated in the grafanabot GitHub account. - run: "git clone --single-branch --no-tags --depth 1 -b master https://grafanabot:${{ secrets.WEBSITE_SYNC_ONCALL }}@github.com/grafana/website-sync ./.github/actions/website-sync" + # WEBSITE_SYNC_TOKEN is a fine-grained GitHub Personal Access Token that expires. + # It must be regenerated in the grafanabot GitHub account and requires a Grafana organization + # GitHub administrator to update the organization secret. + # The IT helpdesk can update the organization secret. + run: "git clone --single-branch --no-tags --depth 1 -b master https://grafanabot:${{ secrets.WEBSITE_SYNC_TOKEN }}@github.com/grafana/website-sync ./.github/actions/website-sync" - name: "Publish to website repository (release)" if: "steps.has-matching-release-tag.outputs.bool == 'true'" @@ -70,9 +72,11 @@ jobs: repository: "grafana/website" branch: "master" host: "github.com" - # PUBLISH_TO_WEBSITE_ONCALL is a fine-grained GitHub Personal Access Token that expires. - # It must be updated in the grafanabot GitHub account. - github_pat: "grafanabot:${{ secrets.PUBLISH_TO_WEBSITE_ONCALL }}" + # PUBLISH_TO_WEBSITE_TOKEN is a fine-grained GitHub Personal Access Token that expires. + # It must be regenerated in the grafanabot GitHub account and requires a Grafana organization + # GitHub administrator to update the organization secret. + # The IT helpdesk can update the organization secret. + github_pat: "grafanabot:${{ secrets.PUBLISH_TO_WEBSITE_TOKEN }}" source_folder: "docs/sources" # Append ".x" to target to produce a v..x directory. target_folder: "content/docs/oncall/${{ steps.target.outputs.target }}.x" From 11531119d2d772b9533bdaeb1388a0e5a7673f59 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 21 Feb 2023 10:21:05 +0100 Subject: [PATCH 03/11] code-ify dependabot configuration (#1347) # What this PR does This PR is meant to code-ify our dependabot configuration. This is being done mainly to override the default labels that dependabot adds to the PRs it opens. ## Checklist - [ ] Tests updated (N/A) - [ ] Documentation added (N/A) - [ ] `CHANGELOG.md` updated (N/A) --- .github/dependabot.yml | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 .github/dependabot.yml diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 00000000..0e0b793d --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,39 @@ +version: 2 +updates: + - package-ecosystem: "npm" + directory: "/grafana-plugin" + schedule: + interval: "daily" + labels: + - "pr:dependencies" + - "pr:no changelog" + - "pr:no public docs" + + - package-ecosystem: "pip" + directory: "/" + schedule: + interval: "daily" + labels: + - "pr:dependencies" + - "pr:no changelog" + - "pr:no public docs" + + - package-ecosystem: "github-actions" + # Workflow files stored in the + # default location of `.github/workflows` + directory: "/" + schedule: + interval: "daily" + labels: + - "pr:dependencies" + - "pr:no changelog" + - "pr:no public docs" + + - package-ecosystem: "docker" + directory: "/" + schedule: + interval: "daily" + labels: + - "pr:dependencies" + - "pr:no changelog" + - "pr:no public docs" From b5f88cfc8fe84ce0f87f6caa200fc696053880d1 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 21 Feb 2023 10:37:26 +0100 Subject: [PATCH 04/11] bump dependabot schedule interval to monthly (#1372) --- .github/dependabot.yml | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/.github/dependabot.yml b/.github/dependabot.yml index 0e0b793d..2dc81fc8 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -3,7 +3,7 @@ updates: - package-ecosystem: "npm" directory: "/grafana-plugin" schedule: - interval: "daily" + interval: "monthly" labels: - "pr:dependencies" - "pr:no changelog" @@ -12,7 +12,7 @@ updates: - package-ecosystem: "pip" directory: "/" schedule: - interval: "daily" + interval: "monthly" labels: - "pr:dependencies" - "pr:no changelog" @@ -23,7 +23,7 @@ updates: # default location of `.github/workflows` directory: "/" schedule: - interval: "daily" + interval: "monthly" labels: - "pr:dependencies" - "pr:no changelog" @@ -32,7 +32,7 @@ updates: - package-ecosystem: "docker" directory: "/" schedule: - interval: "daily" + interval: "monthly" labels: - "pr:dependencies" - "pr:no changelog" From 2cf83a965061c16a04aac949c560a9cac1a807b8 Mon Sep 17 00:00:00 2001 From: Yulia Shanyrova Date: Tue, 21 Feb 2023 12:47:21 +0100 Subject: [PATCH 05/11] 1305 backlink tosource (#1316) # What this PR does - Header of Incident page was reworked: clickable labels instead of just names, users section was deleted - Go to Integration button was deleted, because the functionality moved to clickable labels - Link to source was added - ## Which issue(s) this PR fixes https://github.com/grafana/oncall-private/issues/1305 ## Checklist - [ ] Tests updated - [ ] Documentation added - [ v] `CHANGELOG.md` updated --- CHANGELOG.md | 3 + .../src/models/alertgroup/alertgroup.types.ts | 1 + .../src/pages/incident/Incident.helpers.tsx | 13 ++- .../src/pages/incident/Incident.module.scss | 39 +++++++++ .../src/pages/incident/Incident.tsx | 79 +++++++++++++++---- 5 files changed, 115 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b9c27be4..74dc54b6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Incidents - Removed buttons column and replaced status with toggler ([#1237](https://github.com/grafana/oncall/issues/1237)) - Responsiveness changes across multiple pages (Incidents, Integrations, Schedules) ([#1237](https://github.com/grafana/oncall/issues/1237)) +- Link to source was added +- Header of Incident page was reworked: clickable labels instead of just names, users section was deleted +- "Go to Integration" button was deleted, because the functionality was moved to clickable labels ## v1.1.23 (2023-02-06) diff --git a/grafana-plugin/src/models/alertgroup/alertgroup.types.ts b/grafana-plugin/src/models/alertgroup/alertgroup.types.ts index 5705064e..5c6d622a 100644 --- a/grafana-plugin/src/models/alertgroup/alertgroup.types.ts +++ b/grafana-plugin/src/models/alertgroup/alertgroup.types.ts @@ -87,4 +87,5 @@ interface RenderForWeb { message: any; title: any; image_url: string; + source_link: string; } diff --git a/grafana-plugin/src/pages/incident/Incident.helpers.tsx b/grafana-plugin/src/pages/incident/Incident.helpers.tsx index 461ff01c..cd3e70b1 100644 --- a/grafana-plugin/src/pages/incident/Incident.helpers.tsx +++ b/grafana-plugin/src/pages/incident/Incident.helpers.tsx @@ -1,6 +1,7 @@ import React from 'react'; import { Button, HorizontalGroup, IconButton, Tooltip, VerticalGroup } from '@grafana/ui'; +import cn from 'classnames/bind'; import Avatar from 'components/Avatar/Avatar'; import { MatchMediaTooltip } from 'components/MatchMediaTooltip/MatchMediaTooltip'; @@ -16,11 +17,15 @@ import { move } from 'state/helpers'; import { UserActions } from 'utils/authorization'; import { TABLE_COLUMN_MAX_WIDTH } from 'utils/consts'; +import styles from './Incident.module.scss'; + +const cx = cn.bind(styles); + export function getIncidentStatusTag(alert: Alert) { switch (alert.status) { case IncidentStatus.Firing: return ( - + Firing @@ -28,7 +33,7 @@ export function getIncidentStatusTag(alert: Alert) { ); case IncidentStatus.Acknowledged: return ( - + Acknowledged @@ -36,7 +41,7 @@ export function getIncidentStatusTag(alert: Alert) { ); case IncidentStatus.Resolved: return ( - + Resolved @@ -44,7 +49,7 @@ export function getIncidentStatusTag(alert: Alert) { ); case IncidentStatus.Silenced: return ( - + Silenced diff --git a/grafana-plugin/src/pages/incident/Incident.module.scss b/grafana-plugin/src/pages/incident/Incident.module.scss index 9f598e01..c790c622 100644 --- a/grafana-plugin/src/pages/incident/Incident.module.scss +++ b/grafana-plugin/src/pages/incident/Incident.module.scss @@ -122,6 +122,45 @@ margin-left: 4px; } +.integration-logo { + margin-right: 8px; +} + +.label-button { + padding: 0 8px; + font-weight: 400; +} + +.label-button:disabled { + border: var(--border-strong); +} + +.label-button-text { + max-width: 160px; + overflow: hidden; + position: relative; + display: inline-block; + text-align: center; + text-decoration: none; + text-overflow: ellipsis; + white-space: nowrap; +} + +.source-name { + display: flex; + align-items: center; +} + +.status-tag-container { + margin-right: 8px; + display: inherit; +} + +.status-tag { + height: 24px; + padding: 0 8px; + border-radius: 2px; +} .paged-users { width: 100%; } diff --git a/grafana-plugin/src/pages/incident/Incident.tsx b/grafana-plugin/src/pages/incident/Incident.tsx index 0ae0ccee..4cb11a17 100644 --- a/grafana-plugin/src/pages/incident/Incident.tsx +++ b/grafana-plugin/src/pages/incident/Incident.tsx @@ -58,11 +58,12 @@ import { UserActions } from 'utils/authorization'; import { PLUGIN_ROOT } from 'utils/consts'; import sanitize from 'utils/sanitize'; -import { getActionButtons, getIncidentStatusTag, renderRelatedUsers } from './Incident.helpers'; +import { getActionButtons, getIncidentStatusTag } from './Incident.helpers'; import styles from './Incident.module.scss'; import PagedUsers from './parts/PagedUsers'; const cx = cn.bind(styles); +const INTEGRATION_NAME_LENGTH_LIMIT = 30; interface IncidentPageProps extends WithStoreProps, PageProps, RouteComponentProps<{ id: string }> {} @@ -235,6 +236,9 @@ class IncidentPage extends React.Component const integration = store.alertReceiveChannelStore.getIntegration(incident.alert_receive_channel); const showLinkTo = !incident.dependent_alert_groups.length && !incident.root_alert_group && !incident.resolved; + + const integrationNameWithEmojies = ; + return ( @@ -269,7 +273,7 @@ class IncidentPage extends React.Component {showLinkTo && (
- {getIncidentStatusTag(incident)} | | - - {integration && {integration?.display_name}} - {integration && '|'} - {renderRelatedUsers(incident, true)} +
{getIncidentStatusTag(incident)}
+ + + + + {integration && ( + <> + + + + + + + )}
@@ -310,7 +364,7 @@ class IncidentPage extends React.Component })} - @@ -324,14 +378,7 @@ class IncidentPage extends React.Component value={prepareForEdit(incident.paged_users)} onUpdateEscalationVariants={this.handleAddResponders} /> - - - + - + <> + + + + + + + )} - - - - ); } diff --git a/grafana-plugin/src/models/user/user.ts b/grafana-plugin/src/models/user/user.ts index fffa31c1..101a5f8b 100644 --- a/grafana-plugin/src/models/user/user.ts +++ b/grafana-plugin/src/models/user/user.ts @@ -233,9 +233,10 @@ export class UserStore extends BaseStore { } @action - async fetchVerificationCode(userPk: User['pk']) { + async fetchVerificationCode(userPk: User['pk'], recaptchaToken: string) { await makeRequest(`/users/${userPk}/get_verification_code/`, { method: 'GET', + headers: { 'X-OnCall-Recaptcha': recaptchaToken }, }); } diff --git a/grafana-plugin/src/network/index.ts b/grafana-plugin/src/network/index.ts index 53b1f579..175cda1f 100644 --- a/grafana-plugin/src/network/index.ts +++ b/grafana-plugin/src/network/index.ts @@ -32,12 +32,15 @@ interface RequestConfig { data?: any; withCredentials?: boolean; validateStatus?: (status: number) => boolean; + headers?: { + [key: string]: string | number; + }; } export const isNetworkError = axios.isAxiosError; export const makeRequest = async (path: string, config: RequestConfig) => { - const { method = 'GET', params, data, validateStatus } = config; + const { method = 'GET', params, data, validateStatus, headers } = config; const url = `${API_PROXY_PREFIX}${API_PATH_PREFIX}${path}`; const otel = FaroHelper.faro?.api?.getOTEL(); @@ -63,6 +66,7 @@ export const makeRequest = async (path: string, config: RequestConfig) params, data, validateStatus, + headers, }) .then((response) => { FaroHelper.faro.api.pushEvent('Request completed', { url }); @@ -86,6 +90,7 @@ export const makeRequest = async (path: string, config: RequestConfig) params, data, validateStatus, + headers, }) .then((response) => { FaroHelper.faro?.api.pushEvent('Request completed', { url }); diff --git a/grafana-plugin/src/plugin/GrafanaPluginRootPage.tsx b/grafana-plugin/src/plugin/GrafanaPluginRootPage.tsx index 9c0dbd3f..d71910c6 100644 --- a/grafana-plugin/src/plugin/GrafanaPluginRootPage.tsx +++ b/grafana-plugin/src/plugin/GrafanaPluginRootPage.tsx @@ -40,6 +40,8 @@ import 'interceptors'; import { rootStore } from 'state'; import { useStore } from 'state/useStore'; import { isUserActionAllowed } from 'utils/authorization'; +import { reCAPTCHA_site_key } from 'utils/consts'; +import loadJs from 'utils/loadJs'; dayjs.extend(utc); dayjs.extend(timezone); @@ -89,6 +91,10 @@ export const Root = observer((props: AppRootProps) => { }; }, []); + useEffect(() => { + loadJs(`https://www.google.com/recaptcha/api.js?render=${reCAPTCHA_site_key}`); + }, []); + const updateBasicData = async () => { await store.updateBasicData(); setDidFinishLoading(true); diff --git a/grafana-plugin/src/style/global.css b/grafana-plugin/src/style/global.css index 85cf4a32..5d47bdc4 100644 --- a/grafana-plugin/src/style/global.css +++ b/grafana-plugin/src/style/global.css @@ -48,3 +48,7 @@ padding-left: 4px; padding-right: 4px; } + +.grecaptcha-badge { + visibility: hidden; +} diff --git a/grafana-plugin/src/utils/consts.ts b/grafana-plugin/src/utils/consts.ts index 57e004ba..fdc0636f 100644 --- a/grafana-plugin/src/utils/consts.ts +++ b/grafana-plugin/src/utils/consts.ts @@ -15,6 +15,9 @@ export const DEFAULT_PAGE = 'incidents'; export const PLUGIN_ROOT = '/a/grafana-oncall-app'; +// https://developers.google.com/recaptcha/docs/v3 +export const reCAPTCHA_site_key = '6LeIPJ8kAAAAAJdUfjO3uUtQtVxsYf93y46mTec1'; + // Environment options list for onCallApiUrl export const ONCALL_PROD = 'https://oncall-prod-us-central-0.grafana.net/oncall'; export const ONCALL_OPS = 'https://oncall-ops-us-east-0.grafana.net/oncall'; diff --git a/grafana-plugin/src/utils/loadJs.ts b/grafana-plugin/src/utils/loadJs.ts new file mode 100644 index 00000000..83e7d4e6 --- /dev/null +++ b/grafana-plugin/src/utils/loadJs.ts @@ -0,0 +1,6 @@ +export default function loadJs(url: string) { + let script = document.createElement('script'); + script.src = url; + + document.head.appendChild(script); +} From c733d8b9f2c6a564239885e29fd9234057852428 Mon Sep 17 00:00:00 2001 From: Innokentii Konstantinov Date: Tue, 21 Feb 2023 20:22:11 +0100 Subject: [PATCH 07/11] Cleanup ScenarioStep (#1213) # What this PR does This PR cleanup ScenarioStep. It's needed to simplify moving Slack to the messaging backends in future. 1. Introduce AlertGroupSlackService to move logic from ScenarioStep. Also it allowed to get rid of importing ScenarioSteps in the code not related to processing of slack callbacks. 2. Remove tags from ScenarioSteps, they are unused. 3. Remove ScenarioStep.dispatch method. It just was calling ScenarioStep.process_scenario. 4. Remove "action" param from process_scenario, it was unused. 5. Remove creation of SlackActionRecord on handling SlackEvents. We are not using it, but it generates INSERT query on most of the user-slack interactions. 6. Remove "random_prefix_for_routing" from ScenarioStep, it was unused. ## Which issue(s) this PR fixes ## Checklist - [ ] Tests updated - [ ] Documentation added - [ ] `CHANGELOG.md` updated --------- Co-authored-by: Joey Orlando --- .../escalation_snapshot_mixin.py | 7 +- .../renderers/slack_renderer.py | 77 ++++- engine/apps/integrations/tasks.py | 3 +- .../apps/slack/alert_group_slack_service.py | 136 ++++++++ .../apps/slack/models/slack_action_record.py | 20 +- engine/apps/slack/models/slack_message.py | 29 ++ .../slack/scenarios/alertgroup_appearance.py | 14 +- .../apps/slack/scenarios/declare_incident.py | 6 +- .../apps/slack/scenarios/distribute_alerts.py | 171 +++------- .../slack/scenarios/invited_to_channel.py | 6 +- .../apps/slack/scenarios/manual_incident.py | 14 +- .../slack/scenarios/notification_delivery.py | 12 +- engine/apps/slack/scenarios/onboarding.py | 14 +- engine/apps/slack/scenarios/paging.py | 16 +- engine/apps/slack/scenarios/profile_update.py | 8 +- .../apps/slack/scenarios/resolution_note.py | 23 +- engine/apps/slack/scenarios/scenario_step.py | 295 +----------------- engine/apps/slack/scenarios/schedules.py | 4 +- engine/apps/slack/scenarios/slack_channel.py | 36 +-- .../scenarios/slack_channel_integration.py | 11 +- .../apps/slack/scenarios/slack_usergroup.py | 18 +- engine/apps/slack/scenarios/step_mixins.py | 4 +- engine/apps/slack/tasks.py | 10 +- engine/apps/slack/views.py | 156 +++++---- 24 files changed, 431 insertions(+), 659 deletions(-) create mode 100644 engine/apps/slack/alert_group_slack_service.py diff --git a/engine/apps/alerts/escalation_snapshot/escalation_snapshot_mixin.py b/engine/apps/alerts/escalation_snapshot/escalation_snapshot_mixin.py index 40f983e5..8b36f03b 100644 --- a/engine/apps/alerts/escalation_snapshot/escalation_snapshot_mixin.py +++ b/engine/apps/alerts/escalation_snapshot/escalation_snapshot_mixin.py @@ -18,10 +18,13 @@ from apps.alerts.escalation_snapshot.snapshot_classes import ( ) from apps.alerts.escalation_snapshot.utils import eta_for_escalation_step_notify_if_time from apps.alerts.tasks import calculate_escalation_finish_time, escalate_alert_group -from apps.slack.scenarios.scenario_step import ScenarioStep logger = logging.getLogger(__name__) +# Is a delay to prevent intermediate activity by system in case user is doing some multi-step action. +# For example if user wants to unack and ack we don't need to launch escalation right after unack. +START_ESCALATION_DELAY = 10 + class EscalationSnapshotMixin: """ @@ -239,7 +242,7 @@ class EscalationSnapshotMixin: if raw_next_step_eta: return parse(raw_next_step_eta).replace(tzinfo=pytz.UTC) - def start_escalation_if_needed(self, countdown=ScenarioStep.CROSS_ACTION_DELAY, eta=None): + def start_escalation_if_needed(self, countdown=START_ESCALATION_DELAY, eta=None): """ :type self:AlertGroup """ diff --git a/engine/apps/alerts/incident_appearance/renderers/slack_renderer.py b/engine/apps/alerts/incident_appearance/renderers/slack_renderer.py index 696b2ef6..c81a53d6 100644 --- a/engine/apps/alerts/incident_appearance/renderers/slack_renderer.py +++ b/engine/apps/alerts/incident_appearance/renderers/slack_renderer.py @@ -214,13 +214,9 @@ class AlertGroupSlackRenderer(AlertGroupBaseRenderer): ) if self.alert_group.invitations.filter(is_active=True).count() < 5: - slack_team_identity = self.alert_group.channel.organization.slack_team_identity action_id = ScenarioStep.get_step("distribute_alerts", "InviteOtherPersonToIncident").routing_uid() text = "Invite..." - invitation_element = ScenarioStep( - slack_team_identity, - self.alert_group.channel.organization, - ).get_select_user_element(action_id, text=text) + invitation_element = self._get_select_user_element(action_id, text=text) buttons.append(invitation_element) if not self.alert_group.acknowledged: if not self.alert_group.silenced: @@ -362,3 +358,74 @@ class AlertGroupSlackRenderer(AlertGroupBaseRenderer): "actions": buttons, } ] + + def _get_select_user_element( + self, action_id, multi_select=False, initial_user=None, initial_users_list=None, text=None + ): + MAX_STATIC_SELECT_OPTIONS = 100 + + if not text: + text = f"Select User{'s' if multi_select else ''}" + element = { + "action_id": action_id, + "type": "multi_static_select" if multi_select else "static_select", + "placeholder": { + "type": "plain_text", + "text": text, + "emoji": True, + }, + } + + users = self.alert_group.channel.organization.users.all().select_related("slack_user_identity") + + users_count = users.count() + options = [] + + for user in users: + user_verbal = f"{user.get_user_verbal_for_team_for_slack()}" + if len(user_verbal) > 75: + user_verbal = user_verbal[:72] + "..." + option = {"text": {"type": "plain_text", "text": user_verbal}, "value": json.dumps({"user_id": user.pk})} + options.append(option) + + if users_count > MAX_STATIC_SELECT_OPTIONS: + option_groups = [] + option_groups_chunks = [ + options[x : x + MAX_STATIC_SELECT_OPTIONS] for x in range(0, len(options), MAX_STATIC_SELECT_OPTIONS) + ] + for option_group in option_groups_chunks: + option_group = {"label": {"type": "plain_text", "text": " "}, "options": option_group} + option_groups.append(option_group) + element["option_groups"] = option_groups + elif users_count == 0: # strange case when there are no users to select + option = { + "text": {"type": "plain_text", "text": "No users to select"}, + "value": json.dumps({"user_id": None}), + } + options.append(option) + element["options"] = options + return element + else: + element["options"] = options + + # add initial option + if multi_select and initial_users_list: + if users_count <= MAX_STATIC_SELECT_OPTIONS: + initial_options = [] + for user in users: + user_verbal = f"{user.get_user_verbal_for_team_for_slack()}" + option = { + "text": {"type": "plain_text", "text": user_verbal}, + "value": json.dumps({"user_id": user.pk}), + } + initial_options.append(option) + element["initial_options"] = initial_options + elif not multi_select and initial_user: + user_verbal = f"{initial_user.get_user_verbal_for_team_for_slack()}" + initial_option = { + "text": {"type": "plain_text", "text": user_verbal}, + "value": json.dumps({"user_id": initial_user.pk}), + } + element["initial_option"] = initial_option + + return element diff --git a/engine/apps/integrations/tasks.py b/engine/apps/integrations/tasks.py index 2897b1e4..c864fe15 100644 --- a/engine/apps/integrations/tasks.py +++ b/engine/apps/integrations/tasks.py @@ -9,7 +9,8 @@ from django.core.cache import cache from apps.alerts.models.alert_group_counter import ConcurrentUpdateError from apps.alerts.tasks import resolve_alert_group_by_source_if_needed -from apps.slack.scenarios.scenario_step import SlackAPIException, SlackClientWithErrorHandling +from apps.slack.slack_client import SlackClientWithErrorHandling +from apps.slack.slack_client.exceptions import SlackAPIException from common.custom_celery_tasks import shared_dedicated_queue_retry_task from common.custom_celery_tasks.create_alert_base_task import CreateAlertBaseTask diff --git a/engine/apps/slack/alert_group_slack_service.py b/engine/apps/slack/alert_group_slack_service.py new file mode 100644 index 00000000..53391005 --- /dev/null +++ b/engine/apps/slack/alert_group_slack_service.py @@ -0,0 +1,136 @@ +import logging + +from django.apps import apps + +from apps.slack.constants import SLACK_RATE_LIMIT_DELAY +from apps.slack.slack_client import SlackClientWithErrorHandling +from apps.slack.slack_client.exceptions import ( + SlackAPIChannelArchivedException, + SlackAPIException, + SlackAPIRateLimitException, + SlackAPITokenException, +) + +logger = logging.getLogger(__name__) + + +class AlertGroupSlackService: + def __init__(self, slack_team_identity, slack_client=None): + self.slack_team_identity = slack_team_identity + if slack_client is not None: + self._slack_client = slack_client + else: + self._slack_client = SlackClientWithErrorHandling(slack_team_identity.bot_access_token) + + def update_alert_group_slack_message(self, alert_group): + logger.info(f"Started _update_slack_message for alert_group {alert_group.pk}") + SlackMessage = apps.get_model("slack", "SlackMessage") + AlertReceiveChannel = apps.get_model("alerts", "AlertReceiveChannel") + + slack_message = alert_group.slack_message + attachments = alert_group.render_slack_attachments() + blocks = alert_group.render_slack_blocks() + logger.info(f"Update message for alert_group {alert_group.pk}") + try: + self._slack_client.api_call( + "chat.update", + channel=slack_message.channel_id, + ts=slack_message.slack_id, + attachments=attachments, + blocks=blocks, + ) + logger.info(f"Message has been updated for alert_group {alert_group.pk}") + except SlackAPIRateLimitException as e: + if alert_group.channel.integration != AlertReceiveChannel.INTEGRATION_MAINTENANCE: + if not alert_group.channel.is_rate_limited_in_slack: + delay = e.response.get("rate_limit_delay") or SLACK_RATE_LIMIT_DELAY + alert_group.channel.start_send_rate_limit_message_task(delay) + logger.info( + f"Message has not been updated for alert_group {alert_group.pk} due to slack rate limit." + ) + else: + raise e + + except SlackAPIException as e: + if e.response["error"] == "message_not_found": + logger.info(f"message_not_found for alert_group {alert_group.pk}, trying to post new message") + result = self._slack_client.api_call( + "chat.postMessage", channel=slack_message.channel_id, attachments=attachments, blocks=blocks + ) + slack_message_updated = SlackMessage( + slack_id=result["ts"], + organization=slack_message.organization, + _slack_team_identity=slack_message.slack_team_identity, + channel_id=slack_message.channel_id, + alert_group=alert_group, + ) + slack_message_updated.save() + alert_group.slack_message = slack_message_updated + alert_group.save(update_fields=["slack_message"]) + logger.info(f"Message has been posted for alert_group {alert_group.pk}") + elif e.response["error"] == "is_inactive": # deleted channel error + logger.info(f"Skip updating slack message for alert_group {alert_group.pk} due to is_inactive") + elif e.response["error"] == "account_inactive": + logger.info(f"Skip updating slack message for alert_group {alert_group.pk} due to account_inactive") + elif e.response["error"] == "channel_not_found": + logger.info(f"Skip updating slack message for alert_group {alert_group.pk} due to channel_not_found") + else: + raise e + logger.info(f"Finished _update_slack_message for alert_group {alert_group.pk}") + + def publish_message_to_alert_group_thread( + self, alert_group, attachments=[], mrkdwn=True, unfurl_links=True, text=None + ): + # TODO: refactor checking the possibility of sending message to slack + # do not try to post message to slack if integration is rate limited + if alert_group.channel.is_rate_limited_in_slack: + return + + SlackMessage = apps.get_model("slack", "SlackMessage") + slack_message = alert_group.get_slack_message() + channel_id = slack_message.channel_id + try: + result = self._slack_client.api_call( + "chat.postMessage", + channel=channel_id, + text=text, + attachments=attachments, + thread_ts=slack_message.slack_id, + mrkdwn=mrkdwn, + unfurl_links=unfurl_links, + ) + except SlackAPITokenException as e: + logger.warning( + f"Unable to post message to thread in slack. " + f"Slack team identity pk: {self.slack_team_identity.pk}.\n" + f"{e}" + ) + except SlackAPIChannelArchivedException: + logger.warning( + f"Unable to post message to thread in slack. " + f"Slack team identity pk: {self.slack_team_identity.pk}.\n" + f"Reason: 'is_archived'" + ) + except SlackAPIException as e: + if e.response["error"] == "channel_not_found": # channel was deleted + logger.warning( + f"Unable to post message to thread in slack. " + f"Slack team identity pk: {self.slack_team_identity.pk}.\n" + f"Reason: 'channel_not_found'" + ) + elif e.response["error"] == "invalid_auth": + logger.warning( + f"Unable to post message to thread in slack. " + f"Slack team identity pk: {self.slack_team_identity.pk}.\n" + f"Reason: 'invalid_auth'" + ) + else: + raise e + else: + SlackMessage( + slack_id=result["ts"], + organization=alert_group.channel.organization, + _slack_team_identity=self.slack_team_identity, + channel_id=channel_id, + alert_group=alert_group, + ).save() diff --git a/engine/apps/slack/models/slack_action_record.py b/engine/apps/slack/models/slack_action_record.py index 391abaff..3f8fba6a 100644 --- a/engine/apps/slack/models/slack_action_record.py +++ b/engine/apps/slack/models/slack_action_record.py @@ -1,17 +1,21 @@ from django.db import models -from apps.slack.scenarios.scenario_step import ScenarioStep +# from apps.slack.scenarios.scenario_step import ScenarioStep class SlackActionRecord(models.Model): + """ + Legacy model, should be removed. + """ + ON_CALL_ROUTINE = [ - ScenarioStep.get_step("distribute_alerts", "CustomButtonProcessStep").routing_uid(), - ScenarioStep.get_step("distribute_alerts", "StopInvitationProcess").routing_uid(), - ScenarioStep.get_step("distribute_alerts", "InviteOtherPersonToIncident").routing_uid(), - ScenarioStep.get_step("distribute_alerts", "AcknowledgeGroupStep").routing_uid(), - ScenarioStep.get_step("distribute_alerts", "UnAcknowledgeGroupStep").routing_uid(), - ScenarioStep.get_step("distribute_alerts", "ResolveGroupStep").routing_uid(), - ScenarioStep.get_step("distribute_alerts", "SilenceGroupStep").routing_uid(), + # ScenarioStep.get_step("distribute_alerts", "CustomButtonProcessStep").routing_uid(), + # ScenarioStep.get_step("distribute_alerts", "StopInvitationProcess").routing_uid(), + # ScenarioStep.get_step("distribute_alerts", "InviteOtherPersonToIncident").routing_uid(), + # ScenarioStep.get_step("distribute_alerts", "AcknowledgeGroupStep").routing_uid(), + # ScenarioStep.get_step("distribute_alerts", "UnAcknowledgeGroupStep").routing_uid(), + # ScenarioStep.get_step("distribute_alerts", "ResolveGroupStep").routing_uid(), + # ScenarioStep.get_step("distribute_alerts", "SilenceGroupStep").routing_uid(), ] organization = models.ForeignKey("user_management.Organization", on_delete=models.CASCADE, related_name="actions") diff --git a/engine/apps/slack/models/slack_message.py b/engine/apps/slack/models/slack_message.py index f373611c..3248508b 100644 --- a/engine/apps/slack/models/slack_message.py +++ b/engine/apps/slack/models/slack_message.py @@ -217,3 +217,32 @@ class SlackMessage(models.Model): pass else: raise e + + @classmethod + def get_alert_group_from_slack_message_payload(cls, slack_team_identity, payload): + + message_ts = payload.get("message_ts") or payload["container"]["message_ts"] # interactive message or block + channel_id = payload["channel"]["id"] + + try: + slack_message = cls.objects.get( + slack_id=message_ts, + _slack_team_identity=slack_team_identity, + channel_id=channel_id, + ) + alert_group = slack_message.get_alert_group() + except cls.DoesNotExist as e: + logger.error( + f"Tried to get SlackMessage from message_ts:" + f"slack_team_identity_id={slack_team_identity.pk}," + f"message_ts={message_ts}" + ) + raise e + except cls.alert.RelatedObjectDoesNotExist as e: + logger.error( + f"Tried to get AlertGroup from SlackMessage:" + f"slack_team_identity_id={slack_team_identity.pk}," + f"message_ts={message_ts}" + ) + raise e + return alert_group diff --git a/engine/apps/slack/scenarios/alertgroup_appearance.py b/engine/apps/slack/scenarios/alertgroup_appearance.py index 8208919d..875c0371 100644 --- a/engine/apps/slack/scenarios/alertgroup_appearance.py +++ b/engine/apps/slack/scenarios/alertgroup_appearance.py @@ -16,15 +16,10 @@ from .step_mixins import CheckAlertIsUnarchivedMixin, IncidentActionsAccessContr class OpenAlertAppearanceDialogStep( CheckAlertIsUnarchivedMixin, IncidentActionsAccessControlMixin, scenario_step.ScenarioStep ): - - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - REQUIRED_PERMISSIONS = [RBACPermission.Permissions.CHATOPS_WRITE] ACTION_VERBOSE = "open Alert Appearance" - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): AlertGroup = apps.get_model("alerts", "AlertGroup") AlertReceiveChannel = apps.get_model("alerts", "AlertReceiveChannel") @@ -216,12 +211,7 @@ class OpenAlertAppearanceDialogStep( class UpdateAppearanceStep(scenario_step.ScenarioStep): - - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): AlertGroup = apps.get_model("alerts", "AlertGroup") AlertReceiveChannel = apps.get_model("alerts", "AlertReceiveChannel") diff --git a/engine/apps/slack/scenarios/declare_incident.py b/engine/apps/slack/scenarios/declare_incident.py index 0c1337b5..e97f7134 100644 --- a/engine/apps/slack/scenarios/declare_incident.py +++ b/engine/apps/slack/scenarios/declare_incident.py @@ -2,11 +2,7 @@ from apps.slack.scenarios import scenario_step class DeclareIncidentStep(scenario_step.ScenarioStep): - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): """ Slack sends a POST request to the backend upon clicking a button with a redirect link to Incident. This is a dummy step, that is used to prevent raising 'Step is undefined' exception. diff --git a/engine/apps/slack/scenarios/distribute_alerts.py b/engine/apps/slack/scenarios/distribute_alerts.py index ea0bcb6b..420b0379 100644 --- a/engine/apps/slack/scenarios/distribute_alerts.py +++ b/engine/apps/slack/scenarios/distribute_alerts.py @@ -16,6 +16,7 @@ from apps.alerts.tasks import custom_button_result from apps.alerts.utils import render_curl_command from apps.api.permissions import RBACPermission from apps.slack.constants import CACHE_UPDATE_INCIDENT_SLACK_MESSAGE_LIFETIME, SLACK_RATE_LIMIT_DELAY +from apps.slack.models import SlackMessage from apps.slack.scenarios import scenario_step from apps.slack.scenarios.slack_renderer import AlertGroupLogSlackRenderer from apps.slack.slack_client import SlackClientWithErrorHandling @@ -41,13 +42,7 @@ logger.setLevel(logging.DEBUG) class AlertShootingStep(scenario_step.ScenarioStep): - - tags = [ - scenario_step.ScenarioStep.TAG_TRIGGERED_BY_SYSTEM, - ] - def publish_slack_messages(self, slack_team_identity, alert_group, alert, attachments, channel_id, blocks): - SlackMessage = apps.get_model("slack", "SlackMessage") # channel_id can be None if general log channel for slack_team_identity is not set if channel_id is None: logger.info(f"Failed to post message to Slack for alert_group {alert_group.pk} because channel_id is None") @@ -208,7 +203,7 @@ class AlertShootingStep(scenario_step.ScenarioStep): countdown=1, # delay for message so that the log report is published first ) - def process_scenario(self, slack_user_identity, slack_team_identity, alert, payload=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): pass @@ -218,17 +213,13 @@ class InviteOtherPersonToIncident( scenario_step.ScenarioStep, ): - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - REQUIRED_PERMISSIONS = [RBACPermission.Permissions.CHATOPS_WRITE] ACTION_VERBOSE = "invite to incident" - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): User = apps.get_model("user_management", "User") - alert_group = self.get_alert_group_from_slack_message(payload) + alert_group = SlackMessage.get_alert_group_from_slack_message_payload(slack_team_identity, payload) selected_user = None if not self.check_alert_is_unarchived(slack_team_identity, payload, alert_group): @@ -246,11 +237,11 @@ class InviteOtherPersonToIncident( if selected_user is not None: Invitation.invite_user(selected_user, alert_group, self.user) else: - self._update_slack_message(alert_group) + self.alert_group_slack_service.update_alert_group_slack_message(alert_group) def process_signal(self, log_record): alert_group = log_record.alert_group - self._update_slack_message(alert_group) + self.alert_group_slack_service.update_alert_group_slack_message(alert_group) class SilenceGroupStep( @@ -259,28 +250,24 @@ class SilenceGroupStep( scenario_step.ScenarioStep, ): - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - REQUIRED_PERMISSIONS = [RBACPermission.Permissions.CHATOPS_WRITE] ACTION_VERBOSE = "silence incident" - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): try: silence_delay = int(payload["actions"][0]["selected_options"][0]["value"]) except KeyError: silence_delay = int(payload["actions"][0]["selected_option"]["value"]) - alert_group = self.get_alert_group_from_slack_message(payload) + alert_group = SlackMessage.get_alert_group_from_slack_message_payload(slack_team_identity, payload) if self.check_alert_is_unarchived(slack_team_identity, payload, alert_group): alert_group.silence_by_user(self.user, silence_delay, action_source=ActionSource.SLACK) def process_signal(self, log_record): alert_group = log_record.alert_group - self._update_slack_message(alert_group) + self.alert_group_slack_service.update_alert_group_slack_message(alert_group) class UnSilenceGroupStep( @@ -288,23 +275,18 @@ class UnSilenceGroupStep( IncidentActionsAccessControlMixin, scenario_step.ScenarioStep, ): - - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - REQUIRED_PERMISSIONS = [RBACPermission.Permissions.CHATOPS_WRITE] ACTION_VERBOSE = "unsilence incident" - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): - alert_group = self.get_alert_group_from_slack_message(payload) + alert_group = SlackMessage.get_alert_group_from_slack_message_payload(slack_team_identity, payload) if self.check_alert_is_unarchived(slack_team_identity, payload, alert_group): alert_group.un_silence_by_user(self.user, action_source=ActionSource.SLACK) def process_signal(self, log_record): alert_group = log_record.alert_group - self._update_slack_message(alert_group) + self.alert_group_slack_service.update_alert_group_slack_message(alert_group) class SelectAttachGroupStep( @@ -312,15 +294,10 @@ class SelectAttachGroupStep( IncidentActionsAccessControlMixin, scenario_step.ScenarioStep, ): - - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - REQUIRED_PERMISSIONS = [RBACPermission.Permissions.CHATOPS_WRITE] ACTION_VERBOSE = "Select Incident for Attaching to" - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): AlertGroup = apps.get_model("alerts", "AlertGroup") value = json.loads(payload["actions"][0]["value"]) alert_group_pk = value.get("alert_group_pk") @@ -468,11 +445,6 @@ class AttachGroupStep( IncidentActionsAccessControlMixin, scenario_step.ScenarioStep, ): - - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - REQUIRED_PERMISSIONS = [RBACPermission.Permissions.CHATOPS_WRITE] ACTION_VERBOSE = "Attach incident" @@ -481,7 +453,7 @@ class AttachGroupStep( if log_record.type == AlertGroupLogRecord.TYPE_ATTACHED and log_record.alert_group.is_maintenance_incident: text = f"{log_record.rendered_log_line_action(for_slack=True)}" - self.publish_message_to_thread(alert_group, text=text) + self.alert_group_slack_service.publish_message_to_alert_group_thread(alert_group, text=text) if log_record.type == AlertGroupLogRecord.TYPE_FAILED_ATTACHMENT: ephemeral_text = log_record.rendered_log_line_action(for_slack=True) @@ -496,9 +468,9 @@ class AttachGroupStep( unfurl_links=True, ) - self._update_slack_message(alert_group) + self.alert_group_slack_service.update_alert_group_slack_message(alert_group) - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): # submit selection in modal window if payload["type"] == scenario_step.PAYLOAD_TYPE_VIEW_SUBMISSION: @@ -516,14 +488,14 @@ class AttachGroupStep( root_alert_group_pk = int(payload["actions"][0]["selected_option"]["value"]) root_alert_group = AlertGroup.all_objects.get(pk=root_alert_group_pk) - alert_group = self.get_alert_group_from_slack_message(payload) + alert_group = SlackMessage.get_alert_group_from_slack_message_payload(slack_team_identity, payload) if self.check_alert_is_unarchived(slack_team_identity, payload, alert_group) and self.check_alert_is_unarchived( slack_team_identity, payload, root_alert_group ): alert_group.attach_by_user(self.user, root_alert_group, action_source=ActionSource.SLACK) else: - self._update_slack_message(alert_group) + self.alert_group_slack_service.update_alert_group_slack_message(alert_group) class UnAttachGroupStep( @@ -531,35 +503,25 @@ class UnAttachGroupStep( IncidentActionsAccessControlMixin, scenario_step.ScenarioStep, ): - - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - REQUIRED_PERMISSIONS = [RBACPermission.Permissions.CHATOPS_WRITE] ACTION_VERBOSE = "Unattach incident" - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): - alert_group = self.get_alert_group_from_slack_message(payload) + def process_scenario(self, slack_user_identity, slack_team_identity, payload): + alert_group = SlackMessage.get_alert_group_from_slack_message_payload(slack_team_identity, payload) if self.check_alert_is_unarchived(slack_team_identity, payload, alert_group): alert_group.un_attach_by_user(self.user, action_source=ActionSource.SLACK) def process_signal(self, log_record): alert_group = log_record.alert_group - self._update_slack_message(alert_group) + self.alert_group_slack_service.update_alert_group_slack_message(alert_group) class StopInvitationProcess(CheckAlertIsUnarchivedMixin, IncidentActionsAccessControlMixin, scenario_step.ScenarioStep): - - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - REQUIRED_PERMISSIONS = [RBACPermission.Permissions.CHATOPS_WRITE] ACTION_VERBOSE = "stop invitation" - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): - alert_group = self.get_alert_group_from_slack_message(payload) + def process_scenario(self, slack_user_identity, slack_team_identity, payload): + alert_group = SlackMessage.get_alert_group_from_slack_message_payload(slack_team_identity, payload) if not self.check_alert_is_unarchived(slack_team_identity, payload, alert_group): return @@ -567,7 +529,7 @@ class StopInvitationProcess(CheckAlertIsUnarchivedMixin, IncidentActionsAccessCo Invitation.stop_invitation(invitation_pk, self.user) def process_signal(self, log_record): - self._update_slack_message(log_record.invitation.alert_group) + self.alert_group_slack_service.update_alert_group_slack_message(log_record.invitation.alert_group) class CustomButtonProcessStep( @@ -575,18 +537,13 @@ class CustomButtonProcessStep( IncidentActionsAccessControlMixin, scenario_step.ScenarioStep, ): - - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - # TODO: REQUIRED_PERMISSIONS = [RBACPermission.Permissions.CHATOPS_WRITE] ACTION_VERBOSE = "click custom button" - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): CustomButtom = apps.get_model("alerts", "CustomButton") - alert_group = self.get_alert_group_from_slack_message(payload) + alert_group = SlackMessage.get_alert_group_from_slack_message_payload(slack_team_identity, payload) if self.check_alert_is_unarchived(slack_team_identity, payload, alert_group): custom_button_pk = payload["actions"][0]["name"].split("_")[1] alert_group_pk = payload["actions"][0]["name"].split("_")[2] @@ -595,7 +552,7 @@ class CustomButtonProcessStep( except CustomButtom.DoesNotExist: warning_text = "Oops! This button was deleted" self.open_warning_window(payload, warning_text=warning_text) - self._update_slack_message(alert_group) + self.alert_group_slack_service.update_alert_group_slack_message(alert_group) else: custom_button_result.apply_async( args=( @@ -630,7 +587,9 @@ class CustomButtonProcessStep( attachments = [ {"callback_id": "alert", "text": debug_message}, ] - self.publish_message_to_thread(alert_group, attachments=attachments, text=text) + self.alert_group_slack_service.publish_message_to_alert_group_thread( + alert_group, attachments=attachments, text=text + ) class ResolveGroupStep( @@ -638,18 +597,13 @@ class ResolveGroupStep( IncidentActionsAccessControlMixin, scenario_step.ScenarioStep, ): - - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - REQUIRED_PERMISSIONS = [RBACPermission.Permissions.CHATOPS_WRITE] ACTION_VERBOSE = "resolve incident" - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): ResolutionNoteModalStep = scenario_step.ScenarioStep.get_step("resolution_note", "ResolutionNoteModalStep") - alert_group = self.get_alert_group_from_slack_message(payload) + alert_group = SlackMessage.get_alert_group_from_slack_message_payload(slack_team_identity, payload) if not self.check_alert_is_unarchived(slack_team_identity, payload, alert_group): return @@ -676,7 +630,7 @@ class ResolveGroupStep( alert_group = log_record.alert_group if not alert_group.happened_while_maintenance: - self._update_slack_message(alert_group) + self.alert_group_slack_service.update_alert_group_slack_message(alert_group) class UnResolveGroupStep( @@ -684,22 +638,17 @@ class UnResolveGroupStep( IncidentActionsAccessControlMixin, scenario_step.ScenarioStep, ): - - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - REQUIRED_PERMISSIONS = [RBACPermission.Permissions.CHATOPS_WRITE] ACTION_VERBOSE = "unresolve incident" - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): - alert_group = self.get_alert_group_from_slack_message(payload) + def process_scenario(self, slack_user_identity, slack_team_identity, payload): + alert_group = SlackMessage.get_alert_group_from_slack_message_payload(slack_team_identity, payload) if self.check_alert_is_unarchived(slack_team_identity, payload, alert_group): alert_group.un_resolve_by_user(self.user, action_source=ActionSource.SLACK) def process_signal(self, log_record): alert_group = log_record.alert_group - self._update_slack_message(alert_group) + self.alert_group_slack_service.update_alert_group_slack_message(alert_group) class AcknowledgeGroupStep( @@ -707,16 +656,11 @@ class AcknowledgeGroupStep( IncidentActionsAccessControlMixin, scenario_step.ScenarioStep, ): - - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - REQUIRED_PERMISSIONS = [RBACPermission.Permissions.CHATOPS_WRITE] ACTION_VERBOSE = "acknowledge incident" - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): - alert_group = self.get_alert_group_from_slack_message(payload) + def process_scenario(self, slack_user_identity, slack_team_identity, payload): + alert_group = SlackMessage.get_alert_group_from_slack_message_payload(slack_team_identity, payload) logger.debug(f"process_scenario in AcknowledgeGroupStep for alert_group {alert_group.pk}") if self.check_alert_is_unarchived(slack_team_identity, payload, alert_group): alert_group.acknowledge_by_user(self.user, action_source=ActionSource.SLACK) @@ -724,7 +668,7 @@ class AcknowledgeGroupStep( def process_signal(self, log_record): alert_group = log_record.alert_group logger.debug(f"Started process_signal in AcknowledgeGroupStep for alert_group {alert_group.pk}") - self._update_slack_message(alert_group) + self.alert_group_slack_service.update_alert_group_slack_message(alert_group) logger.debug(f"Finished process_signal in AcknowledgeGroupStep for alert_group {alert_group.pk}") @@ -733,16 +677,11 @@ class UnAcknowledgeGroupStep( IncidentActionsAccessControlMixin, scenario_step.ScenarioStep, ): - - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - REQUIRED_PERMISSIONS = [RBACPermission.Permissions.CHATOPS_WRITE] ACTION_VERBOSE = "unacknowledge incident" - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): - alert_group = self.get_alert_group_from_slack_message(payload) + def process_scenario(self, slack_user_identity, slack_team_identity, payload): + alert_group = SlackMessage.get_alert_group_from_slack_message_payload(slack_team_identity, payload) logger.debug(f"process_scenario in UnAcknowledgeGroupStep for alert_group {alert_group.pk}") if self.check_alert_is_unarchived(slack_team_identity, payload, alert_group): alert_group.un_acknowledge_by_user(self.user, action_source=ActionSource.SLACK) @@ -782,7 +721,9 @@ class UnAcknowledgeGroupStep( except SlackAPIException as e: # post to thread if ack reminder message was deleted in Slack if e.response["error"] == "message_not_found": - self.publish_message_to_thread(alert_group, attachments=message_attachments, text=text) + self.alert_group_slack_service.publish_message_to_alert_group_thread( + alert_group, attachments=message_attachments, text=text + ) elif e.response["error"] == "account_inactive": logger.info( f"Skip unacknowledge slack message for alert_group {alert_group.pk} due to account_inactive" @@ -790,15 +731,17 @@ class UnAcknowledgeGroupStep( else: raise else: - self.publish_message_to_thread(alert_group, attachments=message_attachments, text=text) - self._update_slack_message(alert_group) + self.alert_group_slack_service.publish_message_to_alert_group_thread( + alert_group, attachments=message_attachments, text=text + ) + self.alert_group_slack_service.update_alert_group_slack_message(alert_group) logger.debug(f"Finished process_signal in UnAcknowledgeGroupStep for alert_group {alert_group.pk}") class AcknowledgeConfirmationStep(AcknowledgeGroupStep): ACTION_VERBOSE = "confirm acknowledge status" - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): AlertGroup = apps.get_model("alerts", "AlertGroup") alert_group_id = payload["actions"][0]["value"].split("_")[1] alert_group = AlertGroup.all_objects.get(pk=alert_group_id) @@ -926,31 +869,21 @@ class AcknowledgeConfirmationStep(AcknowledgeGroupStep): alert_group.slack_message.save(update_fields=["ack_reminder_message_ts"]) else: text = f"This is a reminder that the incident is still acknowledged by {user_verbal}" - self.publish_message_to_thread(alert_group, text=text) + self.alert_group_slack_service.publish_message_to_alert_group_thread(alert_group, text=text) class WipeGroupStep(scenario_step.ScenarioStep): - - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - ACTION_VERBOSE = "wipe incident" def process_signal(self, log_record): alert_group = log_record.alert_group user_verbal = log_record.author.get_user_verbal_for_team_for_slack() text = f"Wiped by {user_verbal}" - self.publish_message_to_thread(alert_group, text=text) - self._update_slack_message(alert_group) + self.alert_group_slack_service.publish_message_to_alert_group_thread(alert_group, text=text) + self.alert_group_slack_service.update_alert_group_slack_message(alert_group) class DeleteGroupStep(scenario_step.ScenarioStep): - - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - ACTION_VERBOSE = "delete incident" def process_signal(self, log_record): diff --git a/engine/apps/slack/scenarios/invited_to_channel.py b/engine/apps/slack/scenarios/invited_to_channel.py index 976884c1..e2ff83f1 100644 --- a/engine/apps/slack/scenarios/invited_to_channel.py +++ b/engine/apps/slack/scenarios/invited_to_channel.py @@ -10,11 +10,7 @@ logger.setLevel(logging.DEBUG) class InvitedToChannelStep(scenario_step.ScenarioStep): - tags = [ - scenario_step.ScenarioStep.TAG_TRIGGERED_BY_SYSTEM, - ] - - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): if payload["event"]["user"] == slack_team_identity.bot_user_id: channel_id = payload["event"]["channel"] slack_client = SlackClientWithErrorHandling(slack_team_identity.bot_access_token) diff --git a/engine/apps/slack/scenarios/manual_incident.py b/engine/apps/slack/scenarios/manual_incident.py index d5f59404..f3251f0a 100644 --- a/engine/apps/slack/scenarios/manual_incident.py +++ b/engine/apps/slack/scenarios/manual_incident.py @@ -28,7 +28,7 @@ class StartCreateIncidentFromMessage(scenario_step.ScenarioStep): "incident_create_develop", ] - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): input_id_prefix = _generate_input_id_prefix() channel_id = payload["channel"]["id"] @@ -67,7 +67,7 @@ class FinishCreateIncidentFromMessage(scenario_step.ScenarioStep): FinishCreateIncidentFromMessage creates a manual incident from the slack message via submenu """ - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): Alert = apps.get_model("alerts", "Alert") private_metadata = json.loads(payload["view"]["private_metadata"]) @@ -154,7 +154,7 @@ class StartCreateIncidentFromSlashCommand(scenario_step.ScenarioStep): TITLE_INPUT_BLOCK_ID = "TITLE_INPUT" MESSAGE_INPUT_BLOCK_ID = "MESSAGE_INPUT" - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): input_id_prefix = _generate_input_id_prefix() try: @@ -188,7 +188,7 @@ class FinishCreateIncidentFromSlashCommand(scenario_step.ScenarioStep): FinishCreateIncidentFromSlashCommand creates a manual incident from the slack message via slash message """ - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): Alert = apps.get_model("alerts", "Alert") title = _get_title_from_payload(payload) @@ -264,7 +264,7 @@ class FinishCreateIncidentFromSlashCommand(scenario_step.ScenarioStep): class OnOrgChange(scenario_step.ScenarioStep): - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): private_metadata = json.loads(payload["view"]["private_metadata"]) with_title_and_message_inputs = private_metadata.get("with_title_and_message_inputs", False) submit_routing_uid = private_metadata.get("submit_routing_uid") @@ -308,7 +308,7 @@ class OnOrgChange(scenario_step.ScenarioStep): class OnTeamChange(scenario_step.ScenarioStep): - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): private_metadata = json.loads(payload["view"]["private_metadata"]) with_title_and_message_inputs = private_metadata.get("with_title_and_message_inputs", False) submit_routing_uid = private_metadata.get("submit_routing_uid") @@ -355,7 +355,7 @@ class OnRouteChange(scenario_step.ScenarioStep): OnRouteChange is just a plug to handle change of value on route select """ - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): pass diff --git a/engine/apps/slack/scenarios/notification_delivery.py b/engine/apps/slack/scenarios/notification_delivery.py index 73ed7d05..f45edb77 100644 --- a/engine/apps/slack/scenarios/notification_delivery.py +++ b/engine/apps/slack/scenarios/notification_delivery.py @@ -21,7 +21,7 @@ class NotificationDeliveryStep(scenario_step.ScenarioStep): log_record.notification_error_code == UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_SMS_LIMIT_EXCEEDED ): - self.post_message_to_channel( + self._post_message_to_channel( f"Attempt to send an SMS to {user_verbal_with_mention} has been failed due to a plan limit", alert_group.slack_message.channel_id, ) @@ -29,7 +29,7 @@ class NotificationDeliveryStep(scenario_step.ScenarioStep): log_record.notification_error_code == UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_PHONE_CALLS_LIMIT_EXCEEDED ): - self.post_message_to_channel( + self._post_message_to_channel( f"Attempt to call to {user_verbal_with_mention} has been failed due to a plan limit", alert_group.slack_message.channel_id, ) @@ -37,7 +37,7 @@ class NotificationDeliveryStep(scenario_step.ScenarioStep): log_record.notification_error_code == UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_MAIL_LIMIT_EXCEEDED ): - self.post_message_to_channel( + self._post_message_to_channel( f"Failed to send email to {user_verbal_with_mention}. Exceeded limit for mails", alert_group.slack_message.channel_id, ) @@ -46,17 +46,17 @@ class NotificationDeliveryStep(scenario_step.ScenarioStep): == UserNotificationPolicyLogRecord.ERROR_NOTIFICATION_PHONE_NUMBER_IS_NOT_VERIFIED ): if log_record.notification_channel == UserNotificationPolicy.NotificationChannel.SMS: - self.post_message_to_channel( + self._post_message_to_channel( f"Failed to send an SMS to {user_verbal_with_mention}. Phone number is not verified", alert_group.slack_message.channel_id, ) elif log_record.notification_channel == UserNotificationPolicy.NotificationChannel.PHONE_CALL: - self.post_message_to_channel( + self._post_message_to_channel( f"Failed to call to {user_verbal_with_mention}. Phone number is not verified", alert_group.slack_message.channel_id, ) - def post_message_to_channel(self, text, channel): + def _post_message_to_channel(self, text, channel): blocks = [ { "type": "section", diff --git a/engine/apps/slack/scenarios/onboarding.py b/engine/apps/slack/scenarios/onboarding.py index 0caf0a67..8d325e65 100644 --- a/engine/apps/slack/scenarios/onboarding.py +++ b/engine/apps/slack/scenarios/onboarding.py @@ -6,26 +6,16 @@ logger = logging.getLogger(__name__) class ImOpenStep(scenario_step.ScenarioStep): - - tags = [ - scenario_step.ScenarioStep.TAG_TRIGGERED_BY_SYSTEM, - ] - """ Empty step to handle event and avoid 500's. In case we need it in the future. """ - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): logger.info("InOpenStep, doing nothing.") class AppHomeOpenedStep(scenario_step.ScenarioStep): - - tags = [ - scenario_step.ScenarioStep.TAG_TRIGGERED_BY_SYSTEM, - ] - - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): pass diff --git a/engine/apps/slack/scenarios/paging.py b/engine/apps/slack/scenarios/paging.py index 57151865..9b9272a2 100644 --- a/engine/apps/slack/scenarios/paging.py +++ b/engine/apps/slack/scenarios/paging.py @@ -84,7 +84,7 @@ class StartDirectPaging(scenario_step.ScenarioStep): command_name = [settings.SLACK_DIRECT_PAGING_SLASH_COMMAND] - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): input_id_prefix = _generate_input_id_prefix() try: @@ -111,7 +111,7 @@ class StartDirectPaging(scenario_step.ScenarioStep): class FinishDirectPaging(scenario_step.ScenarioStep): """Handle page command dialog submit.""" - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): title = _get_title_from_payload(payload) message = _get_message_from_payload(payload) private_metadata = json.loads(payload["view"]["private_metadata"]) @@ -168,7 +168,7 @@ class FinishDirectPaging(scenario_step.ScenarioStep): class OnPagingOrgChange(scenario_step.ScenarioStep): """Reload form with updated organization.""" - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): updated_payload = reset_items(payload) view = render_dialog(slack_user_identity, slack_team_identity, updated_payload) self._slack_client.api_call( @@ -193,7 +193,7 @@ class OnPagingUserChange(scenario_step.ScenarioStep): It will perform a user availability check, pushing a new modal for additional confirmation if needed. """ - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): private_metadata = json.loads(payload["view"]["private_metadata"]) selected_organization = _get_selected_org_from_payload(payload, private_metadata["input_id_prefix"]) selected_team = _get_selected_team_from_payload(payload, private_metadata["input_id_prefix"]) @@ -250,7 +250,7 @@ class OnPagingItemActionChange(scenario_step.ScenarioStep): class OnPagingConfirmUserChange(scenario_step.ScenarioStep): """Confirm user selection despite not being available.""" - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): metadata = json.loads(payload["view"]["private_metadata"]) # recreate original view state and metadata @@ -579,10 +579,10 @@ def _get_users_select(organization, team, input_id_prefix): "action_id": OnPagingUserChange.routing_uid(), }, } - - if len(user_options) > scenario_step.MAX_STATIC_SELECT_OPTIONS: + MAX_STATIC_SELECT_OPTIONS = 100 + if len(user_options) > MAX_STATIC_SELECT_OPTIONS: # paginate user options in groups - max_length = scenario_step.MAX_STATIC_SELECT_OPTIONS + max_length = MAX_STATIC_SELECT_OPTIONS chunks = [user_options[x : x + max_length] for x in range(0, len(user_options), max_length)] option_groups = [ { diff --git a/engine/apps/slack/scenarios/profile_update.py b/engine/apps/slack/scenarios/profile_update.py index 3fe2b0e6..5e6cb182 100644 --- a/engine/apps/slack/scenarios/profile_update.py +++ b/engine/apps/slack/scenarios/profile_update.py @@ -3,13 +3,7 @@ from apps.slack.scenarios import scenario_step class ProfileUpdateStep(scenario_step.ScenarioStep): - tags = [ - scenario_step.ScenarioStep.TAG_TRIGGERED_BY_SYSTEM, - ] - # Avoid logging this step to prevent collecting sensitive data of our customers - need_to_be_logged = False - - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): """ Triggered by action: Any update in Slack Profile. Dangerous because it's often triggered by internal client's company systems. diff --git a/engine/apps/slack/scenarios/resolution_note.py b/engine/apps/slack/scenarios/resolution_note.py index e7ee417d..50af3e80 100644 --- a/engine/apps/slack/scenarios/resolution_note.py +++ b/engine/apps/slack/scenarios/resolution_note.py @@ -21,11 +21,8 @@ class AddToResolutionNoteStep(CheckAlertIsUnarchivedMixin, scenario_step.Scenari "add_resolution_note_staging", "add_resolution_note_develop", ] - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): SlackMessage = apps.get_model("slack", "SlackMessage") ResolutionNoteSlackMessage = apps.get_model("alerts", "ResolutionNoteSlackMessage") ResolutionNote = apps.get_model("alerts", "ResolutionNote") @@ -153,7 +150,7 @@ class AddToResolutionNoteStep(CheckAlertIsUnarchivedMixin, scenario_step.Scenari except SlackAPIException: pass - self._update_slack_message(alert_group) + self.alert_group_slack_service.update_alert_group_slack_message(alert_group) else: warning_text = "Unable to add this message to resolution note." self.open_warning_window(payload, warning_text) @@ -329,7 +326,7 @@ class UpdateResolutionNoteStep(scenario_step.ScenarioStep): def update_alert_group_resolution_note_button(self, alert_group): if alert_group.slack_message is not None: - self._update_slack_message(alert_group) + self.alert_group_slack_service.update_alert_group_slack_message(alert_group) def add_resolution_note_reaction(self, slack_thread_message): try: @@ -376,15 +373,10 @@ class UpdateResolutionNoteStep(scenario_step.ScenarioStep): class ResolutionNoteModalStep(CheckAlertIsUnarchivedMixin, scenario_step.ScenarioStep): - - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - RESOLUTION_NOTE_TEXT_BLOCK_ID = "resolution_note_text" RESOLUTION_NOTE_MESSAGES_MAX_COUNT = 25 - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None, data=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload, data=None): AlertGroup = apps.get_model("alerts", "AlertGroup") value = data or json.loads(payload["actions"][0]["value"]) resolution_note_window_action = value.get("resolution_note_window_action", "") or value.get("action_value", "") @@ -666,12 +658,7 @@ class ReadEditPostmortemStep(ResolutionNoteModalStep): class AddRemoveThreadMessageStep(UpdateResolutionNoteStep, scenario_step.ScenarioStep): - - tags = [ - scenario_step.ScenarioStep.TAG_INCIDENT_ROUTINE, - ] - - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): AlertGroup = apps.get_model("alerts", "AlertGroup") ResolutionNoteSlackMessage = apps.get_model("alerts", "ResolutionNoteSlackMessage") ResolutionNote = apps.get_model("alerts", "ResolutionNote") diff --git a/engine/apps/slack/scenarios/scenario_step.py b/engine/apps/slack/scenarios/scenario_step.py index 16be0f21..8b19c6bc 100644 --- a/engine/apps/slack/scenarios/scenario_step.py +++ b/engine/apps/slack/scenarios/scenario_step.py @@ -1,18 +1,8 @@ import importlib -import json import logging -from django.apps import apps -from django.core.cache import cache - -from apps.slack.constants import SLACK_RATE_LIMIT_DELAY +from apps.slack.alert_group_slack_service import AlertGroupSlackService from apps.slack.slack_client import SlackClientWithErrorHandling -from apps.slack.slack_client.exceptions import ( - SlackAPIChannelArchivedException, - SlackAPIException, - SlackAPIRateLimitException, - SlackAPITokenException, -) logger = logging.getLogger(__name__) @@ -62,81 +52,22 @@ PAYLOAD_TYPE_MESSAGE_ACTION = "message_action" THREAD_MESSAGE_SUBTYPE = "bot_message" -MAX_STATIC_SELECT_OPTIONS = 100 - class ScenarioStep(object): - - # Is a delay to prevent intermediate activity by system in case user is doing some multi-step action. - # For example if user wants to unack and ack we don't need to launch escalation right after unack. - CROSS_ACTION_DELAY = 10 - SELECT_ORGANIZATION_AND_ROUTE_BLOCK_ID = "SELECT_ORGANIZATION_AND_ROUTE" - - need_to_be_logged = True - random_prefix_for_routing = "" - - # Some blocks are sending context via action_id, which is limited by 255 chars - - TAG_ONBOARDING = "onboarding" - TAG_DASHBOARD = "dashboard" - TAG_SUBSCRIPTION = "subscription" - TAG_REPORTING = "reporting" - - TAG_TEAM_SETTINGS = "team_settings" - - TAG_TRIGGERED_BY_SYSTEM = "triggered_by_system" - TAG_INCIDENT_ROUTINE = "incident_routine" - TAG_INCIDENT_MANAGEMENT = "incident_management" - - TAG_ON_CALL_SCHEDULES = "on_call_schedules" - - tags = [] - def __init__(self, slack_team_identity, organization=None, user=None): self._slack_client = SlackClientWithErrorHandling(slack_team_identity.bot_access_token) self.slack_team_identity = slack_team_identity self.organization = organization self.user = user - cache_tag = "step_tags_populated_{}".format(self.routing_uid()) + self.alert_group_slack_service = AlertGroupSlackService(slack_team_identity, self._slack_client) - if cache.get(cache_tag) is None: - cache.set(cache_tag, 1, 180) - - def dispatch(self, user, team, payload, action=None): - return self.process_scenario(user, team, payload, action) - - def process_scenario(self, user, team, payload, action=None): + def process_scenario(self, user, team, payload): pass - def ts(self, payload): - if "message_ts" in payload: - ts = payload["message_ts"] - elif ( - "view" in payload - and "private_metadata" in payload["view"] - and payload["view"]["private_metadata"] - and "ts" in json.loads(payload["view"]["private_metadata"]) - ): - ts = json.loads(payload["view"]["private_metadata"])["ts"] - elif "container" in payload and "message_ts" in payload["container"]: - ts = payload["container"]["message_ts"] - elif "state" in payload and "message_ts" in json.loads(payload["state"]): - ts = json.loads(payload["state"])["message_ts"] - else: - ts = "random" - return ts - - def channel(self, user, payload): - if "channel" in payload and "id" in payload["channel"]: - channel = payload["channel"]["id"] - else: - channel = user.im_channel_id - return channel - @classmethod def routing_uid(cls): - return cls.random_prefix_for_routing + cls.__name__ + return cls.__name__ @classmethod def get_step(cls, scenario, step): @@ -152,15 +83,6 @@ class ScenarioStep(object): except ImportError as e: raise Exception("Check import spelling! Scenario: {}, Step:{}, Error: {}".format(scenario, step, e)) - def process_scenario_from_other_step( - self, slack_user_identity, slack_team_identity, payload, step_class, action=None, kwargs={} - ): - """ - Allows to trigger other step from current step - """ - step = step_class(slack_team_identity) - step.process_scenario(slack_user_identity, slack_team_identity, payload, action=action, **kwargs) - def open_warning_window(self, payload, warning_text, title=None): if title is None: title = ":warning: Warning" @@ -191,212 +113,3 @@ class ScenarioStep(object): trigger_id=payload["trigger_id"], view=view, ) - - def get_alert_group_from_slack_message(self, payload): - SlackMessage = apps.get_model("slack", "SlackMessage") - - message_ts = payload.get("message_ts") or payload["container"]["message_ts"] # interactive message or block - channel_id = payload["channel"]["id"] - - try: - slack_message = SlackMessage.objects.get( - slack_id=message_ts, - _slack_team_identity=self.slack_team_identity, - channel_id=channel_id, - ) - alert_group = slack_message.get_alert_group() - except SlackMessage.DoesNotExist as e: - print( - f"Tried to get SlackMessage from message_ts:" - f"Slack Team Identity pk: {self.slack_team_identity.pk}," - f"Message ts: {message_ts}" - ) - raise e - except SlackMessage.alert.RelatedObjectDoesNotExist as e: - print( - f"Tried to get Alert Group from SlackMessage:" - f"Slack Team Identity pk: {self.slack_team_identity.pk}," - f"SlackMessage pk: {slack_message.pk}" - ) - raise e - return alert_group - - def _update_slack_message(self, alert_group): - logger.info(f"Started _update_slack_message for alert_group {alert_group.pk}") - SlackMessage = apps.get_model("slack", "SlackMessage") - AlertReceiveChannel = apps.get_model("alerts", "AlertReceiveChannel") - - slack_message = alert_group.slack_message - attachments = alert_group.render_slack_attachments() - blocks = alert_group.render_slack_blocks() - logger.info(f"Update message for alert_group {alert_group.pk}") - try: - self._slack_client.api_call( - "chat.update", - channel=slack_message.channel_id, - ts=slack_message.slack_id, - attachments=attachments, - blocks=blocks, - ) - logger.info(f"Message has been updated for alert_group {alert_group.pk}") - except SlackAPIRateLimitException as e: - if alert_group.channel.integration != AlertReceiveChannel.INTEGRATION_MAINTENANCE: - if not alert_group.channel.is_rate_limited_in_slack: - delay = e.response.get("rate_limit_delay") or SLACK_RATE_LIMIT_DELAY - alert_group.channel.start_send_rate_limit_message_task(delay) - logger.info( - f"Message has not been updated for alert_group {alert_group.pk} due to slack rate limit." - ) - else: - raise e - - except SlackAPIException as e: - if e.response["error"] == "message_not_found": - logger.info(f"message_not_found for alert_group {alert_group.pk}, trying to post new message") - result = self._slack_client.api_call( - "chat.postMessage", channel=slack_message.channel_id, attachments=attachments, blocks=blocks - ) - slack_message_updated = SlackMessage( - slack_id=result["ts"], - organization=slack_message.organization, - _slack_team_identity=slack_message.slack_team_identity, - channel_id=slack_message.channel_id, - alert_group=alert_group, - ) - slack_message_updated.save() - alert_group.slack_message = slack_message_updated - alert_group.save(update_fields=["slack_message"]) - logger.info(f"Message has been posted for alert_group {alert_group.pk}") - elif e.response["error"] == "is_inactive": # deleted channel error - logger.info(f"Skip updating slack message for alert_group {alert_group.pk} due to is_inactive") - elif e.response["error"] == "account_inactive": - logger.info(f"Skip updating slack message for alert_group {alert_group.pk} due to account_inactive") - elif e.response["error"] == "channel_not_found": - logger.info(f"Skip updating slack message for alert_group {alert_group.pk} due to channel_not_found") - else: - raise e - logger.info(f"Finished _update_slack_message for alert_group {alert_group.pk}") - - def publish_message_to_thread(self, alert_group, attachments=[], mrkdwn=True, unfurl_links=True, text=None): - # TODO: refactor checking the possibility of sending message to slack - # do not try to post message to slack if integration is rate limited - if alert_group.channel.is_rate_limited_in_slack: - return - - SlackMessage = apps.get_model("slack", "SlackMessage") - slack_message = alert_group.get_slack_message() - channel_id = slack_message.channel_id - try: - result = self._slack_client.api_call( - "chat.postMessage", - channel=channel_id, - text=text, - attachments=attachments, - thread_ts=slack_message.slack_id, - mrkdwn=mrkdwn, - unfurl_links=unfurl_links, - ) - except SlackAPITokenException as e: - logger.warning( - f"Unable to post message to thread in slack. " - f"Slack team identity pk: {self.slack_team_identity.pk}.\n" - f"{e}" - ) - except SlackAPIChannelArchivedException: - logger.warning( - f"Unable to post message to thread in slack. " - f"Slack team identity pk: {self.slack_team_identity.pk}.\n" - f"Reason: 'is_archived'" - ) - except SlackAPIException as e: - if e.response["error"] == "channel_not_found": # channel was deleted - logger.warning( - f"Unable to post message to thread in slack. " - f"Slack team identity pk: {self.slack_team_identity.pk}.\n" - f"Reason: 'channel_not_found'" - ) - elif e.response["error"] == "invalid_auth": - logger.warning( - f"Unable to post message to thread in slack. " - f"Slack team identity pk: {self.slack_team_identity.pk}.\n" - f"Reason: 'invalid_auth'" - ) - else: - raise e - else: - SlackMessage( - slack_id=result["ts"], - organization=alert_group.channel.organization, - _slack_team_identity=self.slack_team_identity, - channel_id=channel_id, - alert_group=alert_group, - ).save() - - def get_select_user_element( - self, action_id, multi_select=False, initial_user=None, initial_users_list=None, text=None - ): - if not text: - text = f"Select User{'' if not multi_select else 's'}" - element = { - "action_id": action_id, - "type": "static_select" if not multi_select else "multi_static_select", - "placeholder": { - "type": "plain_text", - "text": text, - "emoji": True, - }, - } - - users = self.organization.users.all().select_related("slack_user_identity") - - users_count = users.count() - options = [] - - for user in users: - user_verbal = f"{user.get_user_verbal_for_team_for_slack()}" - if len(user_verbal) > 75: - user_verbal = user_verbal[:72] + "..." - option = {"text": {"type": "plain_text", "text": user_verbal}, "value": json.dumps({"user_id": user.pk})} - options.append(option) - - if users_count > MAX_STATIC_SELECT_OPTIONS: - option_groups = [] - option_groups_chunks = [ - options[x : x + MAX_STATIC_SELECT_OPTIONS] for x in range(0, len(options), MAX_STATIC_SELECT_OPTIONS) - ] - for option_group in option_groups_chunks: - option_group = {"label": {"type": "plain_text", "text": " "}, "options": option_group} - option_groups.append(option_group) - element["option_groups"] = option_groups - elif users_count == 0: # strange case when there are no users to select - option = { - "text": {"type": "plain_text", "text": "No users to select"}, - "value": json.dumps({"user_id": None}), - } - options.append(option) - element["options"] = options - return element - else: - element["options"] = options - - # add initial option - if multi_select and initial_users_list: - if users_count <= MAX_STATIC_SELECT_OPTIONS: - initial_options = [] - for user in users: - user_verbal = f"{user.get_user_verbal_for_team_for_slack()}" - option = { - "text": {"type": "plain_text", "text": user_verbal}, - "value": json.dumps({"user_id": user.pk}), - } - initial_options.append(option) - element["initial_options"] = initial_options - elif not multi_select and initial_user: - user_verbal = f"{initial_user.get_user_verbal_for_team_for_slack()}" - initial_option = { - "text": {"type": "plain_text", "text": user_verbal}, - "value": json.dumps({"user_id": initial_user.pk}), - } - element["initial_option"] = initial_option - - return element diff --git a/engine/apps/slack/scenarios/schedules.py b/engine/apps/slack/scenarios/schedules.py index c7741e7c..606fbc30 100644 --- a/engine/apps/slack/scenarios/schedules.py +++ b/engine/apps/slack/scenarios/schedules.py @@ -10,14 +10,12 @@ from common.insight_log import EntityEvent, write_resource_insight_log class EditScheduleShiftNotifyStep(scenario_step.ScenarioStep): - tags = [scenario_step.ScenarioStep.TAG_ON_CALL_SCHEDULES] - notify_empty_oncall_options = {choice[0]: choice[1] for choice in OnCallSchedule.NotifyEmptyOnCall.choices} notify_oncall_shift_freq_options = {choice[0]: choice[1] for choice in OnCallSchedule.NotifyOnCallShiftFreq.choices} mention_oncall_start_options = {1: "Mention person in slack", 0: "Inform in channel without mention"} mention_oncall_next_options = {1: "Mention person in slack", 0: "Inform in channel without mention"} - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): if payload["actions"][0].get("value", None) and payload["actions"][0]["value"].startswith("edit"): self.open_settings_modal(payload) elif payload["actions"][0].get("type", None) and payload["actions"][0]["type"] == "static_select": diff --git a/engine/apps/slack/scenarios/slack_channel.py b/engine/apps/slack/scenarios/slack_channel.py index 1f8ac416..a5ad6770 100644 --- a/engine/apps/slack/scenarios/slack_channel.py +++ b/engine/apps/slack/scenarios/slack_channel.py @@ -8,14 +8,7 @@ from apps.slack.tasks import clean_slack_channel_leftovers class SlackChannelCreatedOrRenamedEventStep(scenario_step.ScenarioStep): - tags = [ - scenario_step.ScenarioStep.TAG_TRIGGERED_BY_SYSTEM, - ] - - # Avoid logging this step to prevent collecting sensitive data of our customers - need_to_be_logged = False - - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): """ Triggered by action: Create or rename channel """ @@ -35,14 +28,7 @@ class SlackChannelCreatedOrRenamedEventStep(scenario_step.ScenarioStep): class SlackChannelDeletedEventStep(scenario_step.ScenarioStep): - tags = [ - scenario_step.ScenarioStep.TAG_TRIGGERED_BY_SYSTEM, - ] - - # Avoid logging this step to prevent collecting sensitive data of our customers - need_to_be_logged = False - - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): """ Triggered by action: Delete channel """ @@ -59,14 +45,7 @@ class SlackChannelDeletedEventStep(scenario_step.ScenarioStep): class SlackChannelArchivedEventStep(scenario_step.ScenarioStep): - tags = [ - scenario_step.ScenarioStep.TAG_TRIGGERED_BY_SYSTEM, - ] - - # Avoid logging this step to prevent collecting sensitive data of our customers - need_to_be_logged = False - - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): """ Triggered by action: Archive channel """ @@ -82,14 +61,7 @@ class SlackChannelArchivedEventStep(scenario_step.ScenarioStep): class SlackChannelUnArchivedEventStep(scenario_step.ScenarioStep): - tags = [ - scenario_step.ScenarioStep.TAG_TRIGGERED_BY_SYSTEM, - ] - - # Avoid logging this step to prevent collecting sensitive data of our customers - need_to_be_logged = False - - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): """ Triggered by action: UnArchive channel """ diff --git a/engine/apps/slack/scenarios/slack_channel_integration.py b/engine/apps/slack/scenarios/slack_channel_integration.py index bae779cd..348b6209 100644 --- a/engine/apps/slack/scenarios/slack_channel_integration.py +++ b/engine/apps/slack/scenarios/slack_channel_integration.py @@ -11,14 +11,7 @@ logger.setLevel(logging.DEBUG) class SlackChannelMessageEventStep(scenario_step.ScenarioStep): - tags = [ - scenario_step.ScenarioStep.TAG_TRIGGERED_BY_SYSTEM, - ] - - # Avoid logging this step to prevent collecting sensitive data of our customers - need_to_be_logged = False - - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): """ Triggered by action: Any new message in channel. Dangerous because it's often triggered by internal client's company systems. @@ -149,7 +142,7 @@ class SlackChannelMessageEventStep(scenario_step.ScenarioStep): else: alert_group = slack_thread_message.alert_group slack_thread_message.delete() - self._update_slack_message(alert_group) + self.alert_group_slack_service.update_alert_group_slack_message(alert_group) def create_alert_for_slack_channel_integration_if_needed(self, payload): if "subtype" in payload["event"] and payload["event"]["subtype"] != scenario_step.EVENT_SUBTYPE_FILE_SHARE: diff --git a/engine/apps/slack/scenarios/slack_usergroup.py b/engine/apps/slack/scenarios/slack_usergroup.py index c978f05d..07cf1326 100644 --- a/engine/apps/slack/scenarios/slack_usergroup.py +++ b/engine/apps/slack/scenarios/slack_usergroup.py @@ -5,14 +5,7 @@ from apps.slack.scenarios import scenario_step class SlackUserGroupEventStep(scenario_step.ScenarioStep): - tags = [ - scenario_step.ScenarioStep.TAG_TRIGGERED_BY_SYSTEM, - ] - - # Avoid logging this step to prevent collecting sensitive data of our customers - need_to_be_logged = False - - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): """ Triggered by action: creation user groups or changes in user groups except its members. """ @@ -38,14 +31,7 @@ class SlackUserGroupEventStep(scenario_step.ScenarioStep): class SlackUserGroupMembersChangedEventStep(scenario_step.ScenarioStep): - tags = [ - scenario_step.ScenarioStep.TAG_TRIGGERED_BY_SYSTEM, - ] - - # Avoid logging this step to prevent collecting sensitive data of our customers - need_to_be_logged = False - - def process_scenario(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): """ Triggered by action: changed members in user group. """ diff --git a/engine/apps/slack/scenarios/step_mixins.py b/engine/apps/slack/scenarios/step_mixins.py index 03ebdd91..ee322761 100644 --- a/engine/apps/slack/scenarios/step_mixins.py +++ b/engine/apps/slack/scenarios/step_mixins.py @@ -10,9 +10,9 @@ class AccessControl(ABC): REQUIRED_PERMISSIONS = [] ACTION_VERBOSE = "" - def dispatch(self, slack_user_identity, slack_team_identity, payload, action=None): + def process_scenario(self, slack_user_identity, slack_team_identity, payload): if self.check_membership(): - return super().dispatch(slack_user_identity, slack_team_identity, payload, action=None) + return super().process_scenario(slack_user_identity, slack_team_identity, payload) else: self.send_denied_message(payload) diff --git a/engine/apps/slack/tasks.py b/engine/apps/slack/tasks.py index 29a92691..410169fc 100644 --- a/engine/apps/slack/tasks.py +++ b/engine/apps/slack/tasks.py @@ -9,8 +9,8 @@ from django.core.cache import cache from django.utils import timezone from apps.alerts.tasks.compare_escalations import compare_escalations +from apps.slack.alert_group_slack_service import AlertGroupSlackService from apps.slack.constants import CACHE_UPDATE_INCIDENT_SLACK_MESSAGE_LIFETIME, SLACK_BOT_ID -from apps.slack.scenarios.escalation_delivery import EscalationDeliveryStep from apps.slack.scenarios.scenario_step import ScenarioStep from apps.slack.slack_client import SlackClientWithErrorHandling from apps.slack.slack_client.exceptions import SlackAPIException, SlackAPITokenException @@ -55,7 +55,7 @@ def update_incident_slack_message(slack_team_identity_pk, alert_group_pk): return "Skip message update in Slack due to rate limit" if alert_group.slack_message is None: return "Skip message update in Slack due to absence of slack message" - ScenarioStep(slack_team_identity, alert_group.channel.organization)._update_slack_message(alert_group) + AlertGroupSlackService(slack_team_identity).update_alert_group_slack_message(alert_group) @shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True) @@ -98,9 +98,7 @@ def check_slack_message_exists_before_post_message_to_thread( slack_message = alert_group.get_slack_message() if slack_message is not None: - EscalationDeliveryStep(slack_team_identity, alert_group.channel.organization).publish_message_to_thread( - alert_group, text=text - ) + AlertGroupSlackService(slack_team_identity).publish_message_to_alert_group_thread(alert_group, text=text) # check how much time has passed since alert group was created # to prevent eternal loop of restarting check_slack_message_before_post_message_to_thread @@ -240,7 +238,7 @@ def send_message_to_thread_if_bot_not_in_channel(alert_group_pk, slack_team_iden members = slack_team_identity.get_conversation_members(sc, channel_id) if bot_user_id not in members: text = f"Please invite <@{bot_user_id}> to this channel to make all features " f"available :wink:" - ScenarioStep(slack_team_identity).publish_message_to_thread(alert_group, text=text) + AlertGroupSlackService(slack_team_identity, sc).publish_message_to_alert_group_thread(alert_group, text=text) @shared_dedicated_queue_retry_task(autoretry_for=(Exception,), retry_backoff=True, max_retries=1) diff --git a/engine/apps/slack/views.py b/engine/apps/slack/views.py index 377c3f15..3d91d01b 100644 --- a/engine/apps/slack/views.py +++ b/engine/apps/slack/views.py @@ -56,7 +56,7 @@ from apps.slack.tasks import clean_slack_integration_leftovers, unpopulate_slack from common.insight_log import ChatOpsEvent, ChatOpsType, write_chatops_insight_log from common.oncall_gateway import delete_slack_connector_async -from .models import SlackActionRecord, SlackMessage, SlackTeamIdentity, SlackUserIdentity +from .models import SlackMessage, SlackTeamIdentity, SlackUserIdentity SCENARIOS_ROUTES = [] # Add all other routes here SCENARIOS_ROUTES.extend(ONBOARDING_STEPS_ROUTING) @@ -75,6 +75,8 @@ SCENARIOS_ROUTES.extend(DECLARE_INCIDENT_ROUTING) logger = logging.getLogger(__name__) +SELECT_ORGANIZATION_AND_ROUTE_BLOCK_ID = "SELECT_ORGANIZATION_AND_ROUTE" + class StopAnalyticsReporting(APIView): def get(self, request): @@ -289,8 +291,6 @@ class SlackEventApiEndpointView(APIView): self._open_warning_for_unconnected_user(sc, payload) return Response(status=200) - action_record = SlackActionRecord(user=user, organization=organization, payload=payload) - # Capture cases when we expect stateful message from user if not step_was_found and "type" in payload and payload["type"] == PAYLOAD_TYPE_EVENT_CALLBACK: # Message event is from channel @@ -313,98 +313,86 @@ class SlackEventApiEndpointView(APIView): Step = route["step"] logger.info("Routing to {}".format(Step)) step = Step(slack_team_identity, organization, user) - step.dispatch(slack_user_identity, slack_team_identity, payload) + step.process_scenario(slack_user_identity, slack_team_identity, payload) step_was_found = True # We don't do anything on app mention, but we doesn't want to unsubscribe from this event yet. if payload["event"]["type"] == EVENT_TYPE_APP_MENTION: logger.info(f"Received event of type {EVENT_TYPE_APP_MENTION} from slack. Skipping.") return Response(status=200) # Routing to Steps based on routing rules - try: - if not step_was_found: - for route in SCENARIOS_ROUTES: - # Slash commands have to "type" - if "command" in payload and route["payload_type"] == PAYLOAD_TYPE_SLASH_COMMAND: - if payload["command"] in route["command_name"]: - Step = route["step"] - action_record.step = Step.routing_uid() - logger.info("Routing to {}".format(Step)) - step = Step(slack_team_identity, organization, user) - step.dispatch(slack_user_identity, slack_team_identity, payload) - step_was_found = True + if not step_was_found: + for route in SCENARIOS_ROUTES: + # Slash commands have to "type" + if "command" in payload and route["payload_type"] == PAYLOAD_TYPE_SLASH_COMMAND: + if payload["command"] in route["command_name"]: + Step = route["step"] + logger.info("Routing to {}".format(Step)) + step = Step(slack_team_identity, organization, user) + step.process_scenario(slack_user_identity, slack_team_identity, payload) + step_was_found = True - if "type" in payload and payload["type"] == route["payload_type"]: - if payload["type"] == PAYLOAD_TYPE_EVENT_CALLBACK: - if payload["event"]["type"] == route["event_type"]: - # event_name is used for stateful - if "event_name" not in route: + if "type" in payload and payload["type"] == route["payload_type"]: + if payload["type"] == PAYLOAD_TYPE_EVENT_CALLBACK: + if payload["event"]["type"] == route["event_type"]: + # event_name is used for stateful + if "event_name" not in route: + Step = route["step"] + logger.info("Routing to {}".format(Step)) + step = Step(slack_team_identity, organization, user) + step.process_scenario(slack_user_identity, slack_team_identity, payload) + step_was_found = True + + if payload["type"] == PAYLOAD_TYPE_INTERACTIVE_MESSAGE: + for action in payload["actions"]: + if action["type"] == route["action_type"]: + # Action name may also contain action arguments. + # So only beginning is used for routing. + if action["name"].startswith(route["action_name"]): Step = route["step"] - action_record.step = Step.routing_uid() logger.info("Routing to {}".format(Step)) step = Step(slack_team_identity, organization, user) - step.dispatch(slack_user_identity, slack_team_identity, payload) + result = step.process_scenario(slack_user_identity, slack_team_identity, payload) + if result is not None: + return result step_was_found = True - if payload["type"] == PAYLOAD_TYPE_INTERACTIVE_MESSAGE: - for action in payload["actions"]: - if action["type"] == route["action_type"]: - # Action name may also contain action arguments. - # So only beginning is used for routing. - if action["name"].startswith(route["action_name"]): - Step = route["step"] - action_record.step = Step.routing_uid() - logger.info("Routing to {}".format(Step)) - step = Step(slack_team_identity, organization, user) - result = step.dispatch(slack_user_identity, slack_team_identity, payload) - if result is not None: - return result - step_was_found = True + if payload["type"] == PAYLOAD_TYPE_BLOCK_ACTIONS: + for action in payload["actions"]: + if action["type"] == route["block_action_type"]: + if action["action_id"].startswith(route["block_action_id"]): + Step = route["step"] + logger.info("Routing to {}".format(Step)) + step = Step(slack_team_identity, organization, user) + step.process_scenario(slack_user_identity, slack_team_identity, payload) + step_was_found = True - if payload["type"] == PAYLOAD_TYPE_BLOCK_ACTIONS: - for action in payload["actions"]: - if action["type"] == route["block_action_type"]: - if action["action_id"].startswith(route["block_action_id"]): - Step = route["step"] - action_record.step = Step.routing_uid() - logger.info("Routing to {}".format(Step)) - step = Step(slack_team_identity, organization, user) - step.dispatch(slack_user_identity, slack_team_identity, payload) - step_was_found = True + if payload["type"] == PAYLOAD_TYPE_DIALOG_SUBMISSION: + if payload["callback_id"] == route["dialog_callback_id"]: + Step = route["step"] + logger.info("Routing to {}".format(Step)) + step = Step(slack_team_identity, organization, user) + result = step.process_scenario(slack_user_identity, slack_team_identity, payload) + if result is not None: + return result + step_was_found = True - if payload["type"] == PAYLOAD_TYPE_DIALOG_SUBMISSION: - if payload["callback_id"] == route["dialog_callback_id"]: - Step = route["step"] - action_record.step = Step.routing_uid() - logger.info("Routing to {}".format(Step)) - step = Step(slack_team_identity, organization, user) - result = step.dispatch(slack_user_identity, slack_team_identity, payload) - if result is not None: - return result - step_was_found = True + if payload["type"] == PAYLOAD_TYPE_VIEW_SUBMISSION: + if payload["view"]["callback_id"].startswith(route["view_callback_id"]): + Step = route["step"] + logger.info("Routing to {}".format(Step)) + step = Step(slack_team_identity, organization, user) + result = step.process_scenario(slack_user_identity, slack_team_identity, payload) + if result is not None: + return result + step_was_found = True - if payload["type"] == PAYLOAD_TYPE_VIEW_SUBMISSION: - if payload["view"]["callback_id"].startswith(route["view_callback_id"]): - Step = route["step"] - action_record.step = Step.routing_uid() - logger.info("Routing to {}".format(Step)) - step = Step(slack_team_identity, organization, user) - result = step.dispatch(slack_user_identity, slack_team_identity, payload) - if result is not None: - return result - step_was_found = True - - if payload["type"] == PAYLOAD_TYPE_MESSAGE_ACTION: - if payload["callback_id"] in route["message_action_callback_id"]: - Step = route["step"] - action_record.step = Step.routing_uid() - logger.info("Routing to {}".format(Step)) - step = Step(slack_team_identity, organization, user) - step.dispatch(slack_user_identity, slack_team_identity, payload) - step_was_found = True - - finally: - if Step is not None and Step.need_to_be_logged and organization: - action_record.save() + if payload["type"] == PAYLOAD_TYPE_MESSAGE_ACTION: + if payload["callback_id"] in route["message_action_callback_id"]: + Step = route["step"] + logger.info("Routing to {}".format(Step)) + step = Step(slack_team_identity, organization, user) + step.process_scenario(slack_user_identity, slack_team_identity, payload) + step_was_found = True if not step_was_found: raise Exception("Step is undefined" + str(payload)) @@ -440,12 +428,10 @@ class SlackEventApiEndpointView(APIView): 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 ScenarioStep.SELECT_ORGANIZATION_AND_ROUTE_BLOCK_ID in payload["view"].get("state", {}).get( - "values", {} - ): + elif SELECT_ORGANIZATION_AND_ROUTE_BLOCK_ID in payload["view"].get("state", {}).get("values", {}): payload_values = payload["view"]["state"]["values"] - selected_value = payload_values[ScenarioStep.SELECT_ORGANIZATION_AND_ROUTE_BLOCK_ID][ - ScenarioStep.SELECT_ORGANIZATION_AND_ROUTE_BLOCK_ID + 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: From b02dc6bd36917e9fb58de7c8695b3a05ffda43b6 Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Tue, 21 Feb 2023 23:16:49 -0300 Subject: [PATCH 08/11] Revert "Rework schedules cached ical file values" (#1377) Reverts grafana/oncall#1312 This change seems to have introduced some unexpected behavior with slack user groups. Reverting to reproduce locally and push an improved update. --- .../apps/schedules/models/on_call_schedule.py | 12 +++---- .../schedules/tests/test_on_call_schedule.py | 32 ------------------- 2 files changed, 4 insertions(+), 40 deletions(-) diff --git a/engine/apps/schedules/models/on_call_schedule.py b/engine/apps/schedules/models/on_call_schedule.py index 1a06968f..df012d59 100644 --- a/engine/apps/schedules/models/on_call_schedule.py +++ b/engine/apps/schedules/models/on_call_schedule.py @@ -128,11 +128,9 @@ class OnCallSchedule(PolymorphicModel): """Returns list of calendars. Primary calendar should always be the first""" calendar_primary = None calendar_overrides = None - # if self._ical_file_(primary|overrides) is None -> no cache, will trigger a refresh - # if self._ical_file_(primary|overrides) == "" -> cached value for an empty schedule - if self._ical_file_primary: + if self._ical_file_primary is not None: calendar_primary = icalendar.Calendar.from_ical(self._ical_file_primary) - if self._ical_file_overrides: + if self._ical_file_overrides is not None: calendar_overrides = icalendar.Calendar.from_ical(self._ical_file_overrides) return calendar_primary, calendar_overrides @@ -565,8 +563,7 @@ class OnCallScheduleCalendar(OnCallSchedule): """ Generate iCal events file from custom on-call shifts (created via API) """ - # default to empty string since it is not possible to have a no-events ical file - ical = "" + ical = None if self.custom_on_call_shifts.exists(): end_line = "END:VCALENDAR" calendar = Calendar() @@ -597,8 +594,7 @@ class OnCallScheduleWeb(OnCallSchedule): def _generate_ical_file_from_shifts(self, qs, extra_shifts=None, allow_empty_users=False): """Generate iCal events file from custom on-call shifts.""" - # default to empty string since it is not possible to have a no-events ical file - ical = "" + ical = None if qs.exists() or extra_shifts is not None: if extra_shifts is None: extra_shifts = [] diff --git a/engine/apps/schedules/tests/test_on_call_schedule.py b/engine/apps/schedules/tests/test_on_call_schedule.py index 208bf760..e95cbbfa 100644 --- a/engine/apps/schedules/tests/test_on_call_schedule.py +++ b/engine/apps/schedules/tests/test_on_call_schedule.py @@ -965,35 +965,3 @@ def test_filter_events_none_cache_unchanged( events = schedule.filter_events("UTC", start_date, days=5, filter_by=OnCallSchedule.TYPE_ICAL_PRIMARY) expected = [] assert events == expected - - -@pytest.mark.django_db -def test_schedules_ical_shift_cache(make_organization, make_schedule): - organization = make_organization() - schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) - - # initial values are None - assert schedule.cached_ical_file_primary is None - assert schedule.cached_ical_file_overrides is None - - # accessing the properties will trigger a refresh of the ical files (both empty) - assert schedule._ical_file_primary == "" - assert schedule._ical_file_overrides == "" - - # after the refresh, cached values are updated - # (not None means no need to refresh cached value) - assert schedule.cached_ical_file_primary == "" - assert schedule.cached_ical_file_overrides == "" - - # same for Terraform/API schedules - schedule = make_schedule(organization, schedule_class=OnCallScheduleCalendar) - - # initial values is None - assert schedule.cached_ical_file_primary is None - - # accessing the property will trigger a refresh of the ical file (empty) - assert schedule._ical_file_primary == "" - - # after the refresh, cached value is updated - # (not None means no need to refresh cached value) - assert schedule.cached_ical_file_primary == "" From f4ee99eb7b58d71c3dd77a233e938716e9b7f135 Mon Sep 17 00:00:00 2001 From: Innokentii Konstantinov Date: Wed, 22 Feb 2023 07:29:59 +0100 Subject: [PATCH 09/11] Fix load of ical file from google (#1381) --- engine/apps/schedules/ical_utils.py | 11 ++++++++++- engine/common/api_helpers/utils.py | 3 ++- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/engine/apps/schedules/ical_utils.py b/engine/apps/schedules/ical_utils.py index 5680b652..45032c98 100644 --- a/engine/apps/schedules/ical_utils.py +++ b/engine/apps/schedules/ical_utils.py @@ -576,7 +576,7 @@ def fetch_ical_file_or_get_error(ical_url): cached_ical_file = None ical_file_error = None try: - new_ical_file = requests.get(ical_url, timeout=10).text + new_ical_file = fetch_ical_file(ical_url) Calendar.from_ical(new_ical_file) cached_ical_file = new_ical_file except requests.exceptions.RequestException: @@ -587,6 +587,15 @@ def fetch_ical_file_or_get_error(ical_url): return cached_ical_file, ical_file_error +def fetch_ical_file(ical_url): + # without user-agent header google calendar sometimes returns text/html instead of text/calendar + headers = {"User-Agent": "Grafana OnCall"} + r = requests.get(ical_url, headers=headers, timeout=10) + logger.info(f"fetch_ical_file: content-type={r.headers.get('Content-Type')}") + ical_file = r.text + return ical_file + + def create_base_icalendar(name: str) -> Calendar: cal = Calendar() cal.add("calscale", "GREGORIAN") diff --git a/engine/common/api_helpers/utils.py b/engine/common/api_helpers/utils.py index aef44887..b90904c9 100644 --- a/engine/common/api_helpers/utils.py +++ b/engine/common/api_helpers/utils.py @@ -7,6 +7,7 @@ from django.utils import dateparse, timezone from icalendar import Calendar from rest_framework import serializers +from apps.schedules.ical_utils import fetch_ical_file from common.api_helpers.exceptions import BadRequest from common.timezones import raise_exception_if_not_valid_timezone @@ -49,7 +50,7 @@ def validate_ical_url(url): if settings.BASE_URL in url: raise serializers.ValidationError("Potential self-reference") try: - ical_file = requests.get(url).text + ical_file = fetch_ical_file(url) Calendar.from_ical(ical_file) except requests.exceptions.RequestException: raise serializers.ValidationError("Ical download failed") From 59f83ed331ab4544db6e923db8525cad489839d6 Mon Sep 17 00:00:00 2001 From: Innokentii Konstantinov Date: Wed, 22 Feb 2023 07:30:19 +0100 Subject: [PATCH 10/11] Revert "Revert "Rework schedules cached ical file values"" (#1382) Reverts grafana/oncall#1377 --- .../apps/schedules/models/on_call_schedule.py | 12 ++++--- .../schedules/tests/test_on_call_schedule.py | 32 +++++++++++++++++++ 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/engine/apps/schedules/models/on_call_schedule.py b/engine/apps/schedules/models/on_call_schedule.py index df012d59..1a06968f 100644 --- a/engine/apps/schedules/models/on_call_schedule.py +++ b/engine/apps/schedules/models/on_call_schedule.py @@ -128,9 +128,11 @@ class OnCallSchedule(PolymorphicModel): """Returns list of calendars. Primary calendar should always be the first""" calendar_primary = None calendar_overrides = None - if self._ical_file_primary is not None: + # if self._ical_file_(primary|overrides) is None -> no cache, will trigger a refresh + # if self._ical_file_(primary|overrides) == "" -> cached value for an empty schedule + if self._ical_file_primary: calendar_primary = icalendar.Calendar.from_ical(self._ical_file_primary) - if self._ical_file_overrides is not None: + if self._ical_file_overrides: calendar_overrides = icalendar.Calendar.from_ical(self._ical_file_overrides) return calendar_primary, calendar_overrides @@ -563,7 +565,8 @@ class OnCallScheduleCalendar(OnCallSchedule): """ Generate iCal events file from custom on-call shifts (created via API) """ - ical = None + # default to empty string since it is not possible to have a no-events ical file + ical = "" if self.custom_on_call_shifts.exists(): end_line = "END:VCALENDAR" calendar = Calendar() @@ -594,7 +597,8 @@ class OnCallScheduleWeb(OnCallSchedule): def _generate_ical_file_from_shifts(self, qs, extra_shifts=None, allow_empty_users=False): """Generate iCal events file from custom on-call shifts.""" - ical = None + # default to empty string since it is not possible to have a no-events ical file + ical = "" if qs.exists() or extra_shifts is not None: if extra_shifts is None: extra_shifts = [] diff --git a/engine/apps/schedules/tests/test_on_call_schedule.py b/engine/apps/schedules/tests/test_on_call_schedule.py index e95cbbfa..208bf760 100644 --- a/engine/apps/schedules/tests/test_on_call_schedule.py +++ b/engine/apps/schedules/tests/test_on_call_schedule.py @@ -965,3 +965,35 @@ def test_filter_events_none_cache_unchanged( events = schedule.filter_events("UTC", start_date, days=5, filter_by=OnCallSchedule.TYPE_ICAL_PRIMARY) expected = [] assert events == expected + + +@pytest.mark.django_db +def test_schedules_ical_shift_cache(make_organization, make_schedule): + organization = make_organization() + schedule = make_schedule(organization, schedule_class=OnCallScheduleWeb) + + # initial values are None + assert schedule.cached_ical_file_primary is None + assert schedule.cached_ical_file_overrides is None + + # accessing the properties will trigger a refresh of the ical files (both empty) + assert schedule._ical_file_primary == "" + assert schedule._ical_file_overrides == "" + + # after the refresh, cached values are updated + # (not None means no need to refresh cached value) + assert schedule.cached_ical_file_primary == "" + assert schedule.cached_ical_file_overrides == "" + + # same for Terraform/API schedules + schedule = make_schedule(organization, schedule_class=OnCallScheduleCalendar) + + # initial values is None + assert schedule.cached_ical_file_primary is None + + # accessing the property will trigger a refresh of the ical file (empty) + assert schedule._ical_file_primary == "" + + # after the refresh, cached value is updated + # (not None means no need to refresh cached value) + assert schedule.cached_ical_file_primary == "" From b3ad70abb310bf2c8d1042881b47915aff406617 Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Wed, 22 Feb 2023 07:42:17 +0100 Subject: [PATCH 11/11] update CHANGELOG --- CHANGELOG.md | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index eafc9c10..59399e00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,15 +5,20 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## Unreleased +## v1.1.27 (2023-02-22) ### Added -- Add reCAPTCHA validation for requesting a mobile verification code +- Added reCAPTCHA validation for requesting a mobile verification code ### Changed -- Add ratelimits for phone verification +- Added ratelimits for phone verification + +### Fixed + +- Fixed HTTP request to Google where when fetching an iCal, the response would sometimes contain HTML instead + of the expected iCal data ## v1.1.26 (2023-02-20)