From 09bf0ee3d5f3cb299ae89c5c556176cae099ba19 Mon Sep 17 00:00:00 2001 From: Ildar Iskhakov Date: Mon, 28 Nov 2022 14:18:27 +0800 Subject: [PATCH 01/48] Add celery parameters to disable gossip, heartbeat, mingle (#907) --- engine/celery_with_exporter.sh | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/engine/celery_with_exporter.sh b/engine/celery_with_exporter.sh index d1e020e9..abc33691 100755 --- a/engine/celery_with_exporter.sh +++ b/engine/celery_with_exporter.sh @@ -36,5 +36,14 @@ CELERY_ARGS=( if [[ $CELERY_WORKER_BEAT_ENABLED = True ]]; then CELERY_ARGS+=("--beat") fi +if [[ $CELERY_WORKER_WITHOUT_MINGLE = True ]]; then + CELERY_ARGS+=("--without-mingle") +fi +if [[ $CELERY_WORKER_WITHOUT_GOSSIP = True ]]; then + CELERY_ARGS+=("--without-gossip") +fi +if [[ $CELERY_WORKER_WITHOUT_HEARTBEAT = True ]]; then + CELERY_ARGS+=("--without-heartbeat") +fi celery "${CELERY_ARGS[@]}" From 54d14d10255722e0de095b6c758d7d386f619cdd Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Mon, 28 Nov 2022 12:50:58 +0000 Subject: [PATCH 02/48] Move MobileAppAuthToken view to mobile_app (#902) --- engine/apps/api/views/user.py | 5 ++-- engine/apps/mobile_app/urls.py | 14 ++++++---- engine/apps/mobile_app/views.py | 47 ++++++++++++++++++++++++++++++++- engine/engine/urls.py | 2 +- 4 files changed, 59 insertions(+), 9 deletions(-) diff --git a/engine/apps/api/views/user.py b/engine/apps/api/views/user.py index de0a8590..0126abaf 100644 --- a/engine/apps/api/views/user.py +++ b/engine/apps/api/views/user.py @@ -129,7 +129,6 @@ class UserView( "unlink_backend", "make_test_call", "export_token", - "mobile_app_verification_token", "mobile_app_auth_token", ), AnyRole: ("retrieve", "timezone_options"), @@ -149,7 +148,6 @@ class UserView( "unlink_backend", "make_test_call", "export_token", - "mobile_app_verification_token", "mobile_app_auth_token", ), } @@ -475,6 +473,9 @@ class UserView( authentication_classes=(MobileAppVerificationTokenAuthentication,), ) def mobile_app_auth_token(self, request): + """ + TODO: remove after hackathon app is deprecated (see apps.mobile_app.views.MobileAppAuthTokenAPIView) + """ DynamicSetting = apps.get_model("base", "DynamicSetting") if not settings.FEATURE_MOBILE_APP_INTEGRATION_ENABLED: diff --git a/engine/apps/mobile_app/urls.py b/engine/apps/mobile_app/urls.py index 059503fb..ddb9700b 100644 --- a/engine/apps/mobile_app/urls.py +++ b/engine/apps/mobile_app/urls.py @@ -1,9 +1,13 @@ -from apps.mobile_app.views import APNSDeviceAuthorizedViewSet, GCMDeviceAuthorizedViewSet -from common.api_helpers.optional_slash_router import OptionalSlashRouter +from apps.mobile_app.views import APNSDeviceAuthorizedViewSet, GCMDeviceAuthorizedViewSet, MobileAppAuthTokenAPIView +from common.api_helpers.optional_slash_router import OptionalSlashRouter, optional_slash_path +app_name = "mobile_app" router = OptionalSlashRouter() -router.register("apns", APNSDeviceAuthorizedViewSet) -router.register("gcm", GCMDeviceAuthorizedViewSet) +router.register("apns", APNSDeviceAuthorizedViewSet, basename="apns") +router.register("gcm", GCMDeviceAuthorizedViewSet, basename="gcm") -urlpatterns = router.urls +urlpatterns = [ + *router.urls, + optional_slash_path("auth_token", MobileAppAuthTokenAPIView.as_view(), name="auth_token"), +] diff --git a/engine/apps/mobile_app/views.py b/engine/apps/mobile_app/views.py index 9755b74d..694eeff3 100644 --- a/engine/apps/mobile_app/views.py +++ b/engine/apps/mobile_app/views.py @@ -1,7 +1,12 @@ from push_notifications.api.rest_framework import APNSDeviceAuthorizedViewSet as BaseAPNSDeviceAuthorizedViewSet from push_notifications.api.rest_framework import GCMDeviceAuthorizedViewSet as BaseGCMDeviceAuthorizedViewSet +from rest_framework import status +from rest_framework.exceptions import NotFound +from rest_framework.response import Response +from rest_framework.views import APIView -from apps.mobile_app.auth import MobileAppAuthTokenAuthentication +from apps.mobile_app.auth import MobileAppAuthTokenAuthentication, MobileAppVerificationTokenAuthentication +from apps.mobile_app.models import MobileAppAuthToken class APNSDeviceAuthorizedViewSet(BaseAPNSDeviceAuthorizedViewSet): @@ -10,3 +15,43 @@ class APNSDeviceAuthorizedViewSet(BaseAPNSDeviceAuthorizedViewSet): class GCMDeviceAuthorizedViewSet(BaseGCMDeviceAuthorizedViewSet): authentication_classes = (MobileAppAuthTokenAuthentication,) + + +class MobileAppAuthTokenAPIView(APIView): + authentication_classes = (MobileAppVerificationTokenAuthentication,) + + def get(self, request): + try: + token = MobileAppAuthToken.objects.get(user=self.request.user) + except MobileAppAuthToken.DoesNotExist: + raise NotFound + + response = { + "token_id": token.id, + "user_id": token.user_id, + "organization_id": token.organization_id, + "created_at": token.created_at, + "revoked_at": token.revoked_at, + } + return Response(response, status=status.HTTP_200_OK) + + def post(self, request): + # If token already exists revoke it + try: + token = MobileAppAuthToken.objects.get(user=self.request.user) + token.delete() + except MobileAppAuthToken.DoesNotExist: + pass + + instance, token = MobileAppAuthToken.create_auth_token(self.request.user, self.request.user.organization) + data = {"id": instance.pk, "token": token, "created_at": instance.created_at} + return Response(data, status=status.HTTP_201_CREATED) + + def delete(self, request): + try: + token = MobileAppAuthToken.objects.get(user=self.request.user) + token.delete() + except MobileAppAuthToken.DoesNotExist: + raise NotFound + + return Response(status=status.HTTP_204_NO_CONTENT) diff --git a/engine/engine/urls.py b/engine/engine/urls.py index ab6934d8..a29c11c5 100644 --- a/engine/engine/urls.py +++ b/engine/engine/urls.py @@ -53,7 +53,7 @@ if settings.FEATURE_SLACK_INTEGRATION_ENABLED: if settings.FEATURE_MOBILE_APP_INTEGRATION_ENABLED: urlpatterns += [ - path("mobile_app/", include("apps.mobile_app.urls")), + path("mobile_app/v1/", include("apps.mobile_app.urls", namespace="mobile_app")), ] From 69f1218bab5b0f833c0a86a92f815ab33364e3c8 Mon Sep 17 00:00:00 2001 From: Matias Bordese Date: Thu, 10 Nov 2022 10:49:21 -0300 Subject: [PATCH 03/48] Handle error when updating ical cache from deleted web schedule --- .../apps/schedules/models/on_call_schedule.py | 13 +++++-- .../schedules/tests/test_on_call_schedule.py | 34 +++++++++++++++++++ 2 files changed, 45 insertions(+), 2 deletions(-) diff --git a/engine/apps/schedules/models/on_call_schedule.py b/engine/apps/schedules/models/on_call_schedule.py index 7b04d6ae..512a39aa 100644 --- a/engine/apps/schedules/models/on_call_schedule.py +++ b/engine/apps/schedules/models/on_call_schedule.py @@ -8,6 +8,7 @@ from django.apps import apps from django.conf import settings from django.core.validators import MinLengthValidator from django.db import models +from django.db.utils import DatabaseError from django.utils import timezone from django.utils.functional import cached_property from icalendar.cal import Calendar @@ -637,7 +638,11 @@ class OnCallScheduleWeb(OnCallSchedule): """Return cached ical file with iCal events from custom on-call shifts.""" if self.cached_ical_file_primary is None: self.cached_ical_file_primary = self._generate_ical_file_primary() - self.save(update_fields=["cached_ical_file_primary"]) + try: + self.save(update_fields=["cached_ical_file_primary"]) + except DatabaseError: + # schedule may have been deleted from db + return return self.cached_ical_file_primary def _refresh_primary_ical_file(self): @@ -650,7 +655,11 @@ class OnCallScheduleWeb(OnCallSchedule): """Return cached ical file with iCal events from custom on-call overrides shifts.""" if self.cached_ical_file_overrides is None: self.cached_ical_file_overrides = self._generate_ical_file_overrides() - self.save(update_fields=["cached_ical_file_overrides"]) + try: + self.save(update_fields=["cached_ical_file_overrides"]) + except DatabaseError: + # schedule may have been deleted from db + return return self.cached_ical_file_overrides def _refresh_overrides_ical_file(self): diff --git a/engine/apps/schedules/tests/test_on_call_schedule.py b/engine/apps/schedules/tests/test_on_call_schedule.py index 32a1d826..181a2523 100644 --- a/engine/apps/schedules/tests/test_on_call_schedule.py +++ b/engine/apps/schedules/tests/test_on_call_schedule.py @@ -924,3 +924,37 @@ def test_schedule_related_users(make_organization, make_user_for_organization, m schedule.refresh_from_db() users = schedule.related_users() assert users == set(u.public_primary_key for u in [user_a, user_d, user_e]) + + +@pytest.mark.django_db(transaction=True) +def test_filter_events_none_cache_unchanged( + make_organization, make_user_for_organization, make_schedule, make_on_call_shift +): + organization = make_organization() + user = make_user_for_organization(organization) + schedule = make_schedule( + organization, + schedule_class=OnCallScheduleWeb, + name="test_web_schedule", + ) + start_date = timezone.now().replace(hour=0, minute=0, second=0, microsecond=0) + + # add shift + data = { + "start": start_date + timezone.timedelta(hours=36), + "rotation_start": start_date + timezone.timedelta(hours=36), + "duration": timezone.timedelta(hours=2), + "frequency": CustomOnCallShift.FREQUENCY_DAILY, + "schedule": schedule, + } + on_call_shift = make_on_call_shift( + organization=organization, shift_type=CustomOnCallShift.TYPE_ROLLING_USERS_EVENT, **data + ) + on_call_shift.add_rolling_users([[user]]) + + # schedule is removed from db + schedule.delete() + + events = schedule.filter_events("UTC", start_date, days=5, filter_by=OnCallSchedule.TYPE_ICAL_PRIMARY) + expected = [] + assert events == expected From dc6fcf5c05dcf4ac8c3500b8de37675091f8aee2 Mon Sep 17 00:00:00 2001 From: Vadim Stepanov Date: Mon, 28 Nov 2022 15:52:31 +0000 Subject: [PATCH 04/48] Add internal API fields for the mobile app (#910) * add permalinks list to internal API alertgroup view * add user's name and full avatar URL to the user view * make avatar_full_url a property * fix tests * fix user connection criteria --- engine/apps/alerts/models/alert_group.py | 12 ++++++------ engine/apps/api/serializers/alert_group.py | 3 ++- engine/apps/api/serializers/user.py | 5 ++++- engine/apps/api/tests/test_user.py | 6 ++++++ engine/apps/mobile_app/backend.py | 7 +++---- engine/apps/user_management/models/user.py | 5 +++++ engine/apps/user_management/tests/test_sync.py | 6 ++++-- 7 files changed, 30 insertions(+), 14 deletions(-) diff --git a/engine/apps/alerts/models/alert_group.py b/engine/apps/alerts/models/alert_group.py index cb08e8e1..06df3a2b 100644 --- a/engine/apps/alerts/models/alert_group.py +++ b/engine/apps/alerts/models/alert_group.py @@ -1,7 +1,6 @@ import logging -import typing from collections import namedtuple -from typing import Optional +from typing import Optional, TypedDict from urllib.parse import urljoin from uuid import uuid1 @@ -46,8 +45,9 @@ def generate_public_primary_key_for_alert_group(): return new_public_primary_key -class Permalinks(typing.TypedDict): - slack: str +class Permalinks(TypedDict): + slack: Optional[str] + telegram: Optional[str] class AlertGroupQuerySet(models.QuerySet): @@ -401,12 +401,12 @@ class AlertGroup(AlertGroupSlackRenderingMixin, EscalationSnapshotMixin, models. raise NotImplementedError @property - def slack_permalink(self): + def slack_permalink(self) -> Optional[str]: if self.slack_message is not None: return self.slack_message.permalink @property - def telegram_permalink(self) -> typing.Optional[str]: + def telegram_permalink(self) -> Optional[str]: """ This property will attempt to access an attribute, `prefetched_telegram_messages`, representing a list of prefetched telegram messages. If this attribute does not exist, it falls back to performing a query. diff --git a/engine/apps/api/serializers/alert_group.py b/engine/apps/api/serializers/alert_group.py index a71cfde2..5c2e9517 100644 --- a/engine/apps/api/serializers/alert_group.py +++ b/engine/apps/api/serializers/alert_group.py @@ -132,7 +132,8 @@ class AlertGroupSerializer(AlertGroupListSerializer): fields = AlertGroupListSerializer.Meta.fields + [ "alerts", "render_after_resolve_report_json", - "slack_permalink", + "slack_permalink", # TODO: make plugin frontend use "permalinks" field to get Slack link + "permalinks", "last_alert_at", ] diff --git a/engine/apps/api/serializers/user.py b/engine/apps/api/serializers/user.py index 5f428d87..e3a0d784 100644 --- a/engine/apps/api/serializers/user.py +++ b/engine/apps/api/serializers/user.py @@ -36,7 +36,7 @@ class UserSerializer(DynamicFieldsModelSerializer, EagerLoadingMixin): timezone = serializers.CharField(allow_null=True, required=False) avatar = serializers.URLField(source="avatar_url", read_only=True) - + avatar_full = serializers.URLField(source="avatar_full_url", read_only=True) permissions = serializers.SerializerMethodField() notification_chain_verbal = serializers.SerializerMethodField() cloud_connection_status = serializers.SerializerMethodField() @@ -51,8 +51,10 @@ class UserSerializer(DynamicFieldsModelSerializer, EagerLoadingMixin): "current_team", "email", "username", + "name", "role", "avatar", + "avatar_full", "timezone", "working_hours", "unverified_phone_number", @@ -68,6 +70,7 @@ class UserSerializer(DynamicFieldsModelSerializer, EagerLoadingMixin): read_only_fields = [ "email", "username", + "name", "role", "verified_phone_number", ] diff --git a/engine/apps/api/tests/test_user.py b/engine/apps/api/tests/test_user.py index 54a79ad7..ee7f9809 100644 --- a/engine/apps/api/tests/test_user.py +++ b/engine/apps/api/tests/test_user.py @@ -68,6 +68,7 @@ def test_update_user_cant_change_email_and_username( "email": admin.email, "hide_phone_number": False, "username": admin.username, + "name": admin.name, "role": admin.role, "timezone": None, "working_hours": default_working_hours(), @@ -84,6 +85,7 @@ def test_update_user_cant_change_email_and_username( "notification_chain_verbal": {"default": "", "important": ""}, "slack_user_identity": None, "avatar": admin.avatar_url, + "avatar_full": admin.avatar_full_url, } response = client.put(url, data, format="json", **make_user_auth_headers(admin, token)) assert response.status_code == status.HTTP_200_OK @@ -117,6 +119,7 @@ def test_list_users( "email": admin.email, "hide_phone_number": False, "username": admin.username, + "name": admin.name, "role": admin.role, "timezone": None, "working_hours": default_working_hours(), @@ -132,6 +135,7 @@ def test_list_users( "notification_chain_verbal": {"default": "", "important": ""}, "slack_user_identity": None, "avatar": admin.avatar_url, + "avatar_full": admin.avatar_full_url, "cloud_connection_status": 0, }, { @@ -141,6 +145,7 @@ def test_list_users( "email": editor.email, "hide_phone_number": False, "username": editor.username, + "name": editor.name, "role": editor.role, "timezone": None, "working_hours": default_working_hours(), @@ -156,6 +161,7 @@ def test_list_users( "notification_chain_verbal": {"default": "", "important": ""}, "slack_user_identity": None, "avatar": editor.avatar_url, + "avatar_full": editor.avatar_full_url, "cloud_connection_status": 0, }, ], diff --git a/engine/apps/mobile_app/backend.py b/engine/apps/mobile_app/backend.py index 82187250..f95633ee 100644 --- a/engine/apps/mobile_app/backend.py +++ b/engine/apps/mobile_app/backend.py @@ -1,5 +1,3 @@ -from push_notifications.models import APNSDevice - from apps.base.messaging import BaseMessagingBackend from apps.mobile_app.tasks import notify_user_async @@ -28,8 +26,9 @@ class MobileAppBackend(BaseMessagingBackend): token.delete() def serialize_user(self, user): - # TODO: add Android support using GCMDevice - return {"connected": APNSDevice.objects.filter(user_id=user.pk).exists()} + from apps.mobile_app.models import MobileAppAuthToken + + return {"connected": MobileAppAuthToken.objects.filter(user=user).exists()} def notify_user(self, user, alert_group, notification_policy, critical=False): notify_user_async.delay( diff --git a/engine/apps/user_management/models/user.py b/engine/apps/user_management/models/user.py index 63c3ce0e..81af4e70 100644 --- a/engine/apps/user_management/models/user.py +++ b/engine/apps/user_management/models/user.py @@ -1,4 +1,5 @@ import logging +from urllib.parse import urljoin from django.apps import apps from django.conf import settings @@ -161,6 +162,10 @@ class User(models.Model): def is_authenticated(self): return True + @property + def avatar_full_url(self): + return urljoin(self.organization.grafana_url, self.avatar_url) + @property def verified_phone_number(self): """ diff --git a/engine/apps/user_management/tests/test_sync.py b/engine/apps/user_management/tests/test_sync.py index d6a6d922..c7e03bb9 100644 --- a/engine/apps/user_management/tests/test_sync.py +++ b/engine/apps/user_management/tests/test_sync.py @@ -10,7 +10,7 @@ from apps.user_management.sync import cleanup_organization, sync_organization @pytest.mark.django_db def test_sync_users_for_organization(make_organization, make_user_for_organization): - organization = make_organization() + organization = make_organization(grafana_url="https://test.test") users = tuple(make_user_for_organization(organization, user_id=user_id) for user_id in (1, 2)) api_users = tuple( @@ -20,7 +20,7 @@ def test_sync_users_for_organization(make_organization, make_user_for_organizati "name": "Test", "login": "test", "role": "admin", - "avatarUrl": "test.test/test", + "avatarUrl": "/test/1234", } for user_id in (2, 3) ) @@ -37,12 +37,14 @@ def test_sync_users_for_organization(make_organization, make_user_for_organizati assert updated_user is not None assert updated_user.name == api_users[0]["name"] assert updated_user.email == api_users[0]["email"] + assert updated_user.avatar_full_url == "https://test.test/test/1234" # check that missing users are created created_user = organization.users.filter(user_id=api_users[1]["userId"]).first() assert created_user is not None assert created_user.user_id == api_users[1]["userId"] assert created_user.name == api_users[1]["name"] + assert created_user.avatar_full_url == "https://test.test/test/1234" @pytest.mark.django_db From 3582f9b08f94c380dadfe96ea2174efe684f018b Mon Sep 17 00:00:00 2001 From: Michael Derynck Date: Mon, 28 Nov 2022 16:46:51 +0000 Subject: [PATCH 05/48] Improve Jinja Template feedback and error handling (#884) * Improve feedback so template errors are given to user * Add security error logging * Add limits for templates, payloads, results * Show popup error notification for webhook errors and template errors that don't have a result * Update tests * Split exceptions into warnings/errors to give more control when previewing, rendering, saving templates * Limit title lengths * Make TypeError a warning * Adjust title length limit * Remove length limiting on urlize since it is being done on template render * Fix tests * Add KeyError and ValueError to warnings * No longer enforcing json result when saving webhook in case it is dependent on payload * Add tests for expected exceptions coming from apply_jinja_template * Update changelog * Send raw post if template result is not JSON --- CHANGELOG.md | 8 +++ .../templaters/alert_templater.py | 15 ++++- engine/apps/alerts/models/alert.py | 8 +-- engine/apps/alerts/models/custom_button.py | 24 ++++--- .../tasks/alert_group_web_title_cache.py | 2 +- .../api/serializers/alert_receive_channel.py | 10 +-- engine/apps/api/serializers/custom_button.py | 37 +++-------- engine/apps/api/tests/test_alert_group.py | 7 +- engine/apps/api/tests/test_custom_button.py | 18 +---- .../views/alert_receive_channel_template.py | 9 ++- engine/common/api_helpers/mixins.py | 11 ++-- .../jinja_templater/apply_jinja_template.py | 44 +++++++++++-- .../jinja_templater/jinja_template_env.py | 7 ++ .../common/tests/test_apply_jinja_template.py | 65 +++++++++++++++++++ engine/settings/base.py | 3 + .../AlertTemplates/AlertTemplatesForm.tsx | 1 - .../AlertTemplatesFormContainer.tsx | 12 ++-- .../TemplatePreview/TemplatePreview.tsx | 11 +++- 18 files changed, 204 insertions(+), 88 deletions(-) create mode 100644 engine/common/tests/test_apply_jinja_template.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d2d3f90..51e987ef 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,14 @@ 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). +## v1.1.6 (TBD) + +### Fixed +- Got 500 error when saving Outgoing Webhook ([#890](https://github.com/grafana/oncall/issues/890)) + +### Changed +- When editing templates for alert group presentation or outgoing webhooks, errors and warnings are now displayed in the UI as notification popups or displayed in the preview. +- Errors and warnings that occur when rendering templates during notification or webhooks will now render and display the error/warning as the result. ## v1.1.5 (2022-11-24) ### Fixed diff --git a/engine/apps/alerts/incident_appearance/templaters/alert_templater.py b/engine/apps/alerts/incident_appearance/templaters/alert_templater.py index 052313c4..176e82b3 100644 --- a/engine/apps/alerts/incident_appearance/templaters/alert_templater.py +++ b/engine/apps/alerts/incident_appearance/templaters/alert_templater.py @@ -1,9 +1,12 @@ from abc import ABC, abstractmethod from dataclasses import dataclass +from django.conf import settings + from apps.base.messaging import get_messaging_backend_from_id from apps.slack.slack_formatter import SlackFormatter from common.jinja_templater import apply_jinja_template +from common.jinja_templater.apply_jinja_template import JinjaTemplateError, JinjaTemplateWarning class TemplateLoader: @@ -172,9 +175,15 @@ class AlertTemplater(ABC): "amixr_incident_id": self.incident_id, # TODO: decide on variable names "amixr_link": self.link, # TODO: decide on variable names } - templated_attr, success = apply_jinja_template(attr_template, data, **context) - if success: - return templated_attr + try: + if attr == "title": + return apply_jinja_template( + attr_template, data, result_length_limit=settings.JINJA_RESULT_TITLE_MAX_LENGTH, **context + ) + else: + return apply_jinja_template(attr_template, data, **context) + except (JinjaTemplateError, JinjaTemplateWarning) as e: + return e.fallback_message return None diff --git a/engine/apps/alerts/models/alert.py b/engine/apps/alerts/models/alert.py index b3355b51..9a6c383e 100644 --- a/engine/apps/alerts/models/alert.py +++ b/engine/apps/alerts/models/alert.py @@ -189,12 +189,12 @@ class Alert(models.Model): # set web_title_cache to web title to allow alert group searching based on web_title_cache web_title_template = template_manager.get_attr_template("title", alert_receive_channel, render_for="web") if web_title_template: - web_title_cache = apply_jinja_template(web_title_template, raw_request_data)[0] or None + web_title_cache = apply_jinja_template(web_title_template, raw_request_data) else: web_title_cache = None if grouping_id_template is not None: - group_distinction, _ = apply_jinja_template(grouping_id_template, raw_request_data) + group_distinction = apply_jinja_template(grouping_id_template, raw_request_data) # Insert random uuid to prevent grouping of demo alerts or alerts with group_distinction=None if is_demo or not group_distinction: @@ -204,13 +204,13 @@ class Alert(models.Model): group_distinction = hashlib.md5(str(group_distinction).encode()).hexdigest() if resolve_condition_template is not None: - is_resolve_signal, _ = apply_jinja_template(resolve_condition_template, payload=raw_request_data) + is_resolve_signal = apply_jinja_template(resolve_condition_template, payload=raw_request_data) if isinstance(is_resolve_signal, str): is_resolve_signal = is_resolve_signal.strip().lower() in ["1", "true", "ok"] else: is_resolve_signal = False if acknowledge_condition_template is not None: - is_acknowledge_signal, _ = apply_jinja_template(acknowledge_condition_template, payload=raw_request_data) + is_acknowledge_signal = apply_jinja_template(acknowledge_condition_template, payload=raw_request_data) if isinstance(is_acknowledge_signal, str): is_acknowledge_signal = is_acknowledge_signal.strip().lower() in ["1", "true", "ok"] else: diff --git a/engine/apps/alerts/models/custom_button.py b/engine/apps/alerts/models/custom_button.py index 4b83adbc..9cbee4a5 100644 --- a/engine/apps/alerts/models/custom_button.py +++ b/engine/apps/alerts/models/custom_button.py @@ -1,6 +1,7 @@ import json import logging import re +from json import JSONDecodeError from django.conf import settings from django.core.validators import MinLengthValidator @@ -9,7 +10,8 @@ from django.db.models import F from django.utils import timezone from requests.auth import HTTPBasicAuth -from common.jinja_templater import jinja_template_env +from common.jinja_templater import apply_jinja_template +from common.jinja_templater.apply_jinja_template import JinjaTemplateError, JinjaTemplateWarning from common.public_primary_keys import generate_public_primary_key, increase_public_primary_key_length logger = logging.getLogger(__name__) @@ -103,13 +105,19 @@ class CustomButton(models.Model): if self.forward_whole_payload: post_kwargs["json"] = alert.raw_request_data elif self.data: - rendered_data = jinja_template_env.from_string(self.data).render( - { - "alert_payload": self._escape_alert_payload(alert.raw_request_data), - "alert_group_id": alert.group.public_primary_key, - } - ) - post_kwargs["json"] = json.loads(rendered_data) + try: + rendered_data = apply_jinja_template( + self.data, + alert_payload=self._escape_alert_payload(alert.raw_request_data), + alert_group_id=alert.group.public_primary_key, + ) + except (JinjaTemplateError, JinjaTemplateWarning) as e: + post_kwargs["json"] = {"error": e.fallback_message} + + try: + post_kwargs["json"] = json.loads(rendered_data) + except JSONDecodeError: + post_kwargs["data"] = rendered_data return post_kwargs def _escape_alert_payload(self, payload: dict): diff --git a/engine/apps/alerts/tasks/alert_group_web_title_cache.py b/engine/apps/alerts/tasks/alert_group_web_title_cache.py index 5afc6234..963965f4 100644 --- a/engine/apps/alerts/tasks/alert_group_web_title_cache.py +++ b/engine/apps/alerts/tasks/alert_group_web_title_cache.py @@ -76,7 +76,7 @@ def update_web_title_cache(alert_receive_channel_pk, alert_group_pks): if web_title_template: if alert_group.pk in first_alert_map: raw_request_data = first_alert_map[alert_group.pk]["raw_request_data"] - web_title_cache = apply_jinja_template(web_title_template, raw_request_data)[0] or None + web_title_cache = apply_jinja_template(web_title_template, raw_request_data) else: web_title_cache = None else: diff --git a/engine/apps/api/serializers/alert_receive_channel.py b/engine/apps/api/serializers/alert_receive_channel.py index 91823581..c8df5ad3 100644 --- a/engine/apps/api/serializers/alert_receive_channel.py +++ b/engine/apps/api/serializers/alert_receive_channel.py @@ -20,7 +20,8 @@ from common.api_helpers.custom_fields import TeamPrimaryKeyRelatedField, Writabl from common.api_helpers.exceptions import BadRequest from common.api_helpers.mixins import IMAGE_URL, TEMPLATE_NAMES_ONLY_WITH_NOTIFICATION_CHANNEL, EagerLoadingMixin from common.api_helpers.utils import CurrentTeamDefault -from common.jinja_templater import jinja_template_env +from common.jinja_templater import apply_jinja_template, jinja_template_env +from common.jinja_templater.apply_jinja_template import JinjaTemplateWarning from .integration_heartbeat import IntegrationHeartBeatSerializer @@ -28,9 +29,10 @@ from .integration_heartbeat import IntegrationHeartBeatSerializer def valid_jinja_template_for_serializer_method_field(template): for _, val in template.items(): try: - jinja_template_env.from_string(val) - except TemplateSyntaxError: - raise serializers.ValidationError("invalid template") + apply_jinja_template(val, payload={}) + except JinjaTemplateWarning: + # Suppress warnings, template may be valid with payload + pass class AlertReceiveChannelSerializer(EagerLoadingMixin, serializers.ModelSerializer): diff --git a/engine/apps/api/serializers/custom_button.py b/engine/apps/api/serializers/custom_button.py index 86ee2def..d0026b9f 100644 --- a/engine/apps/api/serializers/custom_button.py +++ b/engine/apps/api/serializers/custom_button.py @@ -1,15 +1,14 @@ -import json from collections import defaultdict from django.core.validators import URLValidator, ValidationError -from jinja2 import TemplateError from rest_framework import serializers from rest_framework.validators import UniqueTogetherValidator from apps.alerts.models import CustomButton from common.api_helpers.custom_fields import TeamPrimaryKeyRelatedField from common.api_helpers.utils import CurrentOrganizationDefault, CurrentTeamDefault -from common.jinja_templater import jinja_template_env +from common.jinja_templater import apply_jinja_template +from common.jinja_templater.apply_jinja_template import JinjaTemplateError, JinjaTemplateWarning class CustomButtonSerializer(serializers.ModelSerializer): @@ -53,32 +52,12 @@ class CustomButtonSerializer(serializers.ModelSerializer): return None try: - template = jinja_template_env.from_string(data) - except TemplateError: - raise serializers.ValidationError("Data has incorrect template") - - try: - rendered = template.render( - { - # Validate that the template can be rendered with a JSON-ish alert payload. - # We don't know what the actual payload will be, so we use a defaultdict - # so that attribute access within a template will never fail - # (provided it's only one level deep - we won't accept templates that attempt - # to do nested attribute access). - # Every attribute access should return a string to ensure that users are - # correctly using `tojson` or wrapping fields in strings. - # If we instead used a `defaultdict(dict)` or `defaultdict(lambda: 1)` we - # would accidentally accept templates such as `{"name": {{ alert_payload.name }}}` - # which would then fail at the true render time due to the - # lack of explicit quotes around the template variable; this would render - # as `{"name": some_alert_name}` which is not valid JSON. - "alert_payload": defaultdict(str), - "alert_group_id": "abcd", - } - ) - json.loads(rendered) - except ValueError: - raise serializers.ValidationError("Data has incorrect format") + apply_jinja_template(data, alert_payload=defaultdict(str), alert_group_id="abcd") + except JinjaTemplateError as e: + raise serializers.ValidationError(e.fallback_message) + except JinjaTemplateWarning: + # Suppress render exceptions since we do not have a representative payload to test with + pass return data diff --git a/engine/apps/api/tests/test_alert_group.py b/engine/apps/api/tests/test_alert_group.py index 4ef75494..0b236b7d 100644 --- a/engine/apps/api/tests/test_alert_group.py +++ b/engine/apps/api/tests/test_alert_group.py @@ -1446,8 +1446,9 @@ def test_alert_group_preview_body_non_existent_template_var( data = {"template_name": "testonly_title_template", "template_body": "foobar: {{ foobar.does_not_exist }}"} response = client.post(url, data, format="json", **make_user_auth_headers(user, token)) + # Return errors as preview body instead of None assert response.status_code == status.HTTP_200_OK - assert response.json()["preview"] is None + assert response.json()["preview"] == "Template Warning: 'foobar' is undefined" @pytest.mark.django_db @@ -1468,4 +1469,6 @@ def test_alert_group_preview_body_invalid_template_syntax( data = {"template_name": "testonly_title_template", "template_body": "{{'' if foo is None else foo}}"} response = client.post(url, data, format="json", **make_user_auth_headers(user, token)) - assert response.status_code == status.HTTP_400_BAD_REQUEST + # Errors now returned preview content + assert response.status_code == status.HTTP_200_OK + assert response.data["preview"] == "Template Error: No test named 'None' found." diff --git a/engine/apps/api/tests/test_custom_button.py b/engine/apps/api/tests/test_custom_button.py index 2d519d83..f45acb77 100644 --- a/engine/apps/api/tests/test_custom_button.py +++ b/engine/apps/api/tests/test_custom_button.py @@ -236,23 +236,7 @@ def test_create_invalid_data_custom_button(custom_button_internal_api_setup, mak data = { "name": "amixr_button_invalid_data", "webhook": TEST_URL, - "data": "invalid_json", - } - response = client.post(url, data, format="json", **make_user_auth_headers(user, token)) - assert response.status_code == status.HTTP_400_BAD_REQUEST - - -@pytest.mark.django_db -def test_create_invalid_templated_data_custom_button(custom_button_internal_api_setup, make_user_auth_headers): - user, token, custom_button = custom_button_internal_api_setup - client = APIClient() - url = reverse("api-internal:custom_button-list") - - data = { - "name": "amixr_button_invalid_data", - "webhook": TEST_URL, - # This would need a `| tojson` or some double quotes around it to pass validation. - "data": "{{ alert_payload.name }}", + "data": "{{%", } response = client.post(url, data, format="json", **make_user_auth_headers(user, token)) assert response.status_code == status.HTTP_400_BAD_REQUEST diff --git a/engine/apps/api/views/alert_receive_channel_template.py b/engine/apps/api/views/alert_receive_channel_template.py index ff8cd923..c6800ee5 100644 --- a/engine/apps/api/views/alert_receive_channel_template.py +++ b/engine/apps/api/views/alert_receive_channel_template.py @@ -1,5 +1,6 @@ -from rest_framework import mixins, viewsets +from rest_framework import mixins, status, viewsets from rest_framework.permissions import IsAuthenticated +from rest_framework.response import Response from apps.alerts.models import AlertReceiveChannel from apps.api.permissions import MODIFY_ACTIONS, READ_ACTIONS, ActionPermission, AnyRole, IsAdmin @@ -7,6 +8,7 @@ from apps.api.serializers.alert_receive_channel import AlertReceiveChannelTempla from apps.auth_token.auth import PluginAuthentication from common.api_helpers.mixins import PublicPrimaryKeyMixin from common.insight_log import EntityEvent, write_resource_insight_log +from common.jinja_templater.apply_jinja_template import JinjaTemplateError class AlertReceiveChannelTemplateView( @@ -36,7 +38,10 @@ class AlertReceiveChannelTemplateView( def update(self, request, *args, **kwargs): instance = self.get_object() prev_state = instance.insight_logs_serialized - result = super().update(request, *args, **kwargs) + try: + result = super().update(request, *args, **kwargs) + except JinjaTemplateError as e: + return Response(e.fallback_message, status.HTTP_400_BAD_REQUEST) instance = self.get_object() new_state = instance.insight_logs_serialized write_resource_insight_log( diff --git a/engine/common/api_helpers/mixins.py b/engine/common/api_helpers/mixins.py index 7a2f84c0..c3a0991d 100644 --- a/engine/common/api_helpers/mixins.py +++ b/engine/common/api_helpers/mixins.py @@ -4,7 +4,6 @@ import math from django.core.exceptions import ObjectDoesNotExist from django.db.models import Q from django.utils.functional import cached_property -from jinja2.exceptions import TemplateRuntimeError from rest_framework import status from rest_framework.decorators import action from rest_framework.exceptions import NotFound, Throttled @@ -23,6 +22,7 @@ from apps.base.messaging import get_messaging_backends from apps.user_management.models import Team from common.api_helpers.exceptions import BadRequest from common.jinja_templater import apply_jinja_template +from common.jinja_templater.apply_jinja_template import JinjaTemplateError, JinjaTemplateWarning class UpdateSerializerMixin: @@ -324,13 +324,16 @@ class PreviewTemplateMixin: templater.template_manager = PreviewTemplateLoader() try: templated_alert = templater.render() - except TemplateRuntimeError: - raise BadRequest(detail={"template_body": "Invalid template syntax"}) + except (JinjaTemplateError, JinjaTemplateWarning) as e: + return Response({"preview": e.fallback_message}, status.HTTP_200_OK) templated_attr = getattr(templated_alert, attr_name) elif attr_name in TEMPLATE_NAMES_WITHOUT_NOTIFICATION_CHANNEL: - templated_attr, _ = apply_jinja_template(template_body, payload=alert_to_template.raw_request_data) + try: + templated_attr = apply_jinja_template(template_body, payload=alert_to_template.raw_request_data) + except (JinjaTemplateError, JinjaTemplateWarning) as e: + return Response({"preview": e.fallback_message}, status.HTTP_200_OK) else: templated_attr = None response = {"preview": templated_attr} diff --git a/engine/common/jinja_templater/apply_jinja_template.py b/engine/common/jinja_templater/apply_jinja_template.py index fb00ab31..3a640e25 100644 --- a/engine/common/jinja_templater/apply_jinja_template.py +++ b/engine/common/jinja_templater/apply_jinja_template.py @@ -1,12 +1,42 @@ -from jinja2 import TemplateSyntaxError, UndefinedError +import logging + +from django.conf import settings +from jinja2 import TemplateAssertionError, TemplateSyntaxError, UndefinedError +from jinja2.exceptions import SecurityError from .jinja_template_env import jinja_template_env +logger = logging.getLogger(__name__) + + +class JinjaTemplateError(Exception): + def __init__(self, fallback_message): + self.fallback_message = f"Template Error: {fallback_message}" + + +class JinjaTemplateWarning(Exception): + def __init__(self, fallback_message): + self.fallback_message = f"Template Warning: {fallback_message}" + + +def apply_jinja_template(template, payload=None, result_length_limit=settings.JINJA_RESULT_MAX_LENGTH, **kwargs): + if len(template) > settings.JINJA_TEMPLATE_MAX_LENGTH: + raise JinjaTemplateError( + f"Template exceeds length limit ({len(template)} > {settings.JINJA_TEMPLATE_MAX_LENGTH})" + ) -def apply_jinja_template(template, payload=None, **kwargs): try: - template = jinja_template_env.from_string(template) - result = template.render(payload=payload, **kwargs) - return result, True - except (UndefinedError, TypeError, ValueError, KeyError, TemplateSyntaxError): - return None, False + compiled_template = jinja_template_env.from_string(template) + result = compiled_template.render(payload=payload, **kwargs) + except SecurityError as e: + logger.warning(f"SecurityError process template={template} payload={payload}") + raise JinjaTemplateError(str(e)) + except (TemplateAssertionError, TemplateSyntaxError) as e: + raise JinjaTemplateError(str(e)) + except (TypeError, KeyError, ValueError, UndefinedError) as e: + raise JinjaTemplateWarning(str(e)) + except Exception as e: + logger.error(f"Unexpected template error: {str(e)} template={template} payload={payload}") + raise JinjaTemplateError(str(e)) + + return (result[:result_length_limit] + "..") if len(result) > result_length_limit else result diff --git a/engine/common/jinja_templater/jinja_template_env.py b/engine/common/jinja_templater/jinja_template_env.py index 41e915aa..747b010b 100644 --- a/engine/common/jinja_templater/jinja_template_env.py +++ b/engine/common/jinja_templater/jinja_template_env.py @@ -1,13 +1,20 @@ from django.utils import timezone from jinja2 import BaseLoader +from jinja2.exceptions import SecurityError from jinja2.sandbox import SandboxedEnvironment from .filters import datetimeformat, iso8601_to_time, regex_replace, to_pretty_json + +def raise_security_exception(name): + raise SecurityError(f"use of '{name}' is restricted") + + jinja_template_env = SandboxedEnvironment(loader=BaseLoader()) jinja_template_env.filters["datetimeformat"] = datetimeformat jinja_template_env.filters["iso8601_to_time"] = iso8601_to_time jinja_template_env.filters["tojson_pretty"] = to_pretty_json jinja_template_env.globals["time"] = timezone.now +jinja_template_env.globals["range"] = lambda *args: raise_security_exception("range") jinja_template_env.filters["regex_replace"] = regex_replace diff --git a/engine/common/tests/test_apply_jinja_template.py b/engine/common/tests/test_apply_jinja_template.py new file mode 100644 index 00000000..aed42af1 --- /dev/null +++ b/engine/common/tests/test_apply_jinja_template.py @@ -0,0 +1,65 @@ +import json + +import pytest +from django.conf import settings + +from common.jinja_templater import apply_jinja_template +from common.jinja_templater.apply_jinja_template import JinjaTemplateError, JinjaTemplateWarning + + +def test_apply_jinja_template(): + payload = {"name": "test"} + rendered = apply_jinja_template("{{ payload | tojson_pretty }}", payload) + result = json.loads(rendered) + assert payload == result + + +def test_apply_jinja_template_bad_syntax_error(): + with pytest.raises(JinjaTemplateError): + apply_jinja_template("{{%", payload={}) + + +def test_apply_jinja_template_unknown_filter_error(): + with pytest.raises(JinjaTemplateError): + apply_jinja_template("{{ payload | to_json_pretty }}", payload={}) + + +def test_apply_jinja_template_unsafe_error(): + with pytest.raises(JinjaTemplateError): + apply_jinja_template("{{ payload.__init__() }}", payload={}) + + +def test_apply_jinja_template_restricted_error(): + with pytest.raises(JinjaTemplateError): + apply_jinja_template("{% for n in range(100) %}Hello{% endfor %}", payload={}) + + +def test_apply_jinja_template_restricted_inside_conditional(): + template = "{% if 'blabla' in payload %}{% for n in range(100) %}Hello{% endfor %}{% endif %}" + # No exception when condition == False + apply_jinja_template(template, payload={}) + with pytest.raises(JinjaTemplateError): + apply_jinja_template(template, payload={"blabla": "test"}) + + +def test_apply_jinja_template_missing_field_warning(): + with pytest.raises(JinjaTemplateWarning): + apply_jinja_template("{{ payload.field.name }}", payload={}) + + +def test_apply_jinja_template_type_warning(): + with pytest.raises(JinjaTemplateWarning): + apply_jinja_template("{{ payload.name + 25 }}", payload={"name": "test"}) + + +def test_apply_jinja_template_too_large(): + template = "test" * 20000 + with pytest.raises(JinjaTemplateError): + apply_jinja_template(template, payload={}) + + +def test_apply_jinja_template_result_truncate(): + payload = {"value": "test" * 20000} + result = apply_jinja_template("{{ payload.value }}", payload) + # Length == Limit + 2 to account for '..' appended to end + assert len(result) == settings.JINJA_RESULT_MAX_LENGTH + 2 diff --git a/engine/settings/base.py b/engine/settings/base.py index 49036059..1b1e9412 100644 --- a/engine/settings/base.py +++ b/engine/settings/base.py @@ -574,6 +574,9 @@ SELF_HOSTED_SETTINGS = { GRAFANA_INCIDENT_STATIC_API_KEY = os.environ.get("GRAFANA_INCIDENT_STATIC_API_KEY", None) DATA_UPLOAD_MAX_MEMORY_SIZE = getenv_integer("DATA_UPLOAD_MAX_MEMORY_SIZE", 1_048_576) # 1mb by default +JINJA_TEMPLATE_MAX_LENGTH = 50000 +JINJA_RESULT_TITLE_MAX_LENGTH = 500 +JINJA_RESULT_MAX_LENGTH = 50000 # Log inbound/outbound calls as slow=1 if they exceed threshold SLOW_THRESHOLD_SECONDS = 2.0 diff --git a/grafana-plugin/src/components/AlertTemplates/AlertTemplatesForm.tsx b/grafana-plugin/src/components/AlertTemplates/AlertTemplatesForm.tsx index 71c38142..4166833d 100644 --- a/grafana-plugin/src/components/AlertTemplates/AlertTemplatesForm.tsx +++ b/grafana-plugin/src/components/AlertTemplates/AlertTemplatesForm.tsx @@ -27,7 +27,6 @@ const cx = cn.bind(styles); interface AlertTemplatesFormProps { templates: any; onUpdateTemplates: (values: any) => void; - errors: any; alertReceiveChannelId: AlertReceiveChannel['id']; alertGroupId?: Alert['pk']; demoAlertEnabled: boolean; diff --git a/grafana-plugin/src/containers/AlertTemplatesFormContainer/AlertTemplatesFormContainer.tsx b/grafana-plugin/src/containers/AlertTemplatesFormContainer/AlertTemplatesFormContainer.tsx index f1ef2f51..bff57aff 100644 --- a/grafana-plugin/src/containers/AlertTemplatesFormContainer/AlertTemplatesFormContainer.tsx +++ b/grafana-plugin/src/containers/AlertTemplatesFormContainer/AlertTemplatesFormContainer.tsx @@ -6,7 +6,7 @@ import AlertTemplatesForm from 'components/AlertTemplates/AlertTemplatesForm'; import { AlertReceiveChannel } from 'models/alert_receive_channel'; import { Alert } from 'models/alertgroup/alertgroup.types'; import { useStore } from 'state/useStore'; -import { openNotification } from 'utils'; +import { openErrorNotification, openNotification } from 'utils'; interface TeamEditContainerProps { onHide: () => void; @@ -24,7 +24,6 @@ const AlertTemplatesFormContainer = observer((props: TeamEditContainerProps) => const store = useStore(); const [templatesRefreshing, setTemplatesRefreshing] = useState(false); - const [errors, setErrors] = useState({}); useEffect(() => { store.alertReceiveChannelStore.updateItem(alertReceiveChannelId); @@ -41,8 +40,12 @@ const AlertTemplatesFormContainer = observer((props: TeamEditContainerProps) => onUpdateTemplates(); } }) - .catch((data) => { - setErrors(data.response.data); + .catch((err) => { + if (err.response?.data?.length > 0) { + openErrorNotification(err.response.data); + } else { + openErrorNotification(err.message); + } }); }, [alertReceiveChannelId, onUpdateTemplates, store.alertReceiveChannelStore] @@ -64,7 +67,6 @@ const AlertTemplatesFormContainer = observer((props: TeamEditContainerProps) => { (alertGroupId ? alertGroupStore.renderPreview(alertGroupId, templateName, templateBody) : alertReceiveChannelStore.renderPreview(alertReceiveChannelId, templateName, templateBody) - ).then(setResult); + ) + .then(setResult) + .catch((err) => { + if (err.response?.data?.length > 0) { + openErrorNotification(err.response.data); + } else { + openErrorNotification(err.message); + } + }); }, 1000); useEffect(handleTemplateBodyChange, [templateBody]); From 5a4fc90fa43e6feaa4a8e455f4e1224665e4361b Mon Sep 17 00:00:00 2001 From: Joey Orlando Date: Tue, 22 Nov 2022 16:57:15 +0100 Subject: [PATCH 06/48] fetch/render mobile app QR code in user settings modal --- CHANGELOG.md | 7 +- engine/apps/mobile_app/backend.py | 12 +- grafana-plugin/package.json | 1 + .../src/assets/img/brand/apple-logo.svg | 3 + .../src/assets/img/brand/play-store-logo.svg | 6 + .../{Block.module.css => Block.module.scss} | 16 +- .../src/components/GBlock/Block.tsx | 20 +- .../MobileAppVerification.module.css | 8 - .../MobileAppVerification.test.tsx | 182 + .../MobileAppVerification.tsx | 169 +- .../MobileAppVerification.test.tsx.snap | 5191 +++++++++++++++++ .../DisconnectButton.test.tsx | 28 + .../DisconnectButton.test.tsx.snap | 16 + .../parts/DisconnectButton/index.tsx | 20 + .../DownloadIcons/DownloadIcons.module.scss | 16 + .../DownloadIcons/DownloadIcons.test.tsx | 12 + .../__snapshots__/DownloadIcons.test.tsx.snap | 74 + .../parts/DownloadIcons/index.tsx | 36 + .../parts/QRCode/QRCode.test.tsx | 12 + .../QRCode/__snapshots__/QRCode.test.tsx.snap | 2221 +++++++ .../parts/QRCode/index.tsx | 17 + .../ConfigurationForm.test.tsx.snap | 2 +- .../containers/UserSettings/UserSettings.tsx | 20 +- .../containers/UserSettings/parts/index.tsx | 33 +- grafana-plugin/src/models/user/user.test.ts | 56 + grafana-plugin/src/models/user/user.ts | 14 +- grafana-plugin/src/style/vars.css | 1 + grafana-plugin/yarn.lock | 13 + 28 files changed, 8067 insertions(+), 139 deletions(-) create mode 100644 grafana-plugin/src/assets/img/brand/apple-logo.svg create mode 100644 grafana-plugin/src/assets/img/brand/play-store-logo.svg rename grafana-plugin/src/components/GBlock/{Block.module.css => Block.module.scss} (61%) delete mode 100644 grafana-plugin/src/containers/MobileAppVerification/MobileAppVerification.module.css create mode 100644 grafana-plugin/src/containers/MobileAppVerification/MobileAppVerification.test.tsx create mode 100644 grafana-plugin/src/containers/MobileAppVerification/__snapshots__/MobileAppVerification.test.tsx.snap create mode 100644 grafana-plugin/src/containers/MobileAppVerification/parts/DisconnectButton/DisconnectButton.test.tsx create mode 100644 grafana-plugin/src/containers/MobileAppVerification/parts/DisconnectButton/__snapshots__/DisconnectButton.test.tsx.snap create mode 100644 grafana-plugin/src/containers/MobileAppVerification/parts/DisconnectButton/index.tsx create mode 100644 grafana-plugin/src/containers/MobileAppVerification/parts/DownloadIcons/DownloadIcons.module.scss create mode 100644 grafana-plugin/src/containers/MobileAppVerification/parts/DownloadIcons/DownloadIcons.test.tsx create mode 100644 grafana-plugin/src/containers/MobileAppVerification/parts/DownloadIcons/__snapshots__/DownloadIcons.test.tsx.snap create mode 100644 grafana-plugin/src/containers/MobileAppVerification/parts/DownloadIcons/index.tsx create mode 100644 grafana-plugin/src/containers/MobileAppVerification/parts/QRCode/QRCode.test.tsx create mode 100644 grafana-plugin/src/containers/MobileAppVerification/parts/QRCode/__snapshots__/QRCode.test.tsx.snap create mode 100644 grafana-plugin/src/containers/MobileAppVerification/parts/QRCode/index.tsx create mode 100644 grafana-plugin/src/models/user/user.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 0d2d3f90..02530b91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,12 +7,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## v1.1.5 (2022-11-24) +### Added + +- Added a QR code in the "Mobile App Verification" tab on the user settings modal to connect the mobile application to your OnCall instance + +## v1.1.5 (2022-11-24) + ### Fixed - UI bug fixes for Grafana 9.3 ([#860](https://github.com/grafana/oncall/pull/860)) - Bug fix for saving source link template ([#898](https://github.com/grafana/oncall/pull/898)) - ## v1.1.4 (2022-11-23) ### Fixed diff --git a/engine/apps/mobile_app/backend.py b/engine/apps/mobile_app/backend.py index f95633ee..1acf67b9 100644 --- a/engine/apps/mobile_app/backend.py +++ b/engine/apps/mobile_app/backend.py @@ -1,3 +1,7 @@ +import json + +from django.conf import settings + from apps.base.messaging import BaseMessagingBackend from apps.mobile_app.tasks import notify_user_async @@ -9,7 +13,6 @@ class MobileAppBackend(BaseMessagingBackend): available_for_use = True template_fields = ["title"] - # TODO: add QR code generation (base64 encode?) def generate_user_verification_code(self, user): from apps.mobile_app.models import MobileAppVerificationToken @@ -17,7 +20,12 @@ class MobileAppBackend(BaseMessagingBackend): MobileAppVerificationToken.objects.filter(user=user).delete() _, token = MobileAppVerificationToken.create_auth_token(user, user.organization) - return token + return json.dumps( + { + "token": token, + "oncall_api_url": settings.BASE_URL, + } + ) def unlink_user(self, user): from apps.mobile_app.models import MobileAppAuthToken diff --git a/grafana-plugin/package.json b/grafana-plugin/package.json index 46d3cc39..78b76592 100644 --- a/grafana-plugin/package.json +++ b/grafana-plugin/package.json @@ -117,6 +117,7 @@ "react-draggable": "^4.4.5", "react-emoji-render": "^1.2.4", "react-modal": "^3.15.1", + "react-qr-code": "^2.0.8", "react-responsive": "^8.1.0", "react-router-dom": "^5.2.0", "react-sortable-hoc": "^1.11.0", diff --git a/grafana-plugin/src/assets/img/brand/apple-logo.svg b/grafana-plugin/src/assets/img/brand/apple-logo.svg new file mode 100644 index 00000000..acfaa37e --- /dev/null +++ b/grafana-plugin/src/assets/img/brand/apple-logo.svg @@ -0,0 +1,3 @@ + + + diff --git a/grafana-plugin/src/assets/img/brand/play-store-logo.svg b/grafana-plugin/src/assets/img/brand/play-store-logo.svg new file mode 100644 index 00000000..e4e5150d --- /dev/null +++ b/grafana-plugin/src/assets/img/brand/play-store-logo.svg @@ -0,0 +1,6 @@ + + + + + + \ No newline at end of file diff --git a/grafana-plugin/src/components/GBlock/Block.module.css b/grafana-plugin/src/components/GBlock/Block.module.scss similarity index 61% rename from grafana-plugin/src/components/GBlock/Block.module.css rename to grafana-plugin/src/components/GBlock/Block.module.scss index 49f4ed2d..b83e6c79 100644 --- a/grafana-plugin/src/components/GBlock/Block.module.css +++ b/grafana-plugin/src/components/GBlock/Block.module.scss @@ -1,6 +1,18 @@ .root { padding: 16px; border-radius: 2px; + + &--withBackGround { + background: var(--secondary-background); + } + + &--fullWidth { + width: 100%; + } + + &--hover:hover { + background: var(--hover-selected); + } } :global(.theme-dark) .root_bordered { @@ -14,7 +26,3 @@ :global(.theme-dark) .root_shadowed { box-shadow: 0 4px 10px rgba(0, 0, 0, 0.6); } - -.root_with-background { - background: var(--secondary-background); -} diff --git a/grafana-plugin/src/components/GBlock/Block.tsx b/grafana-plugin/src/components/GBlock/Block.tsx index 16fd7260..f3a4dc0e 100644 --- a/grafana-plugin/src/components/GBlock/Block.tsx +++ b/grafana-plugin/src/components/GBlock/Block.tsx @@ -2,25 +2,39 @@ import React, { FC, HTMLAttributes } from 'react'; import cn from 'classnames/bind'; -import styles from './Block.module.css'; +import styles from './Block.module.scss'; interface BlockProps extends HTMLAttributes { bordered?: boolean; shadowed?: boolean; withBackground?: boolean; + hover?: boolean; + fullWidth?: boolean; } const cx = cn.bind(styles); const Block: FC = (props) => { - const { children, style, className, bordered = false, shadowed = false, withBackground = false, ...rest } = props; + const { + children, + style, + className, + bordered = false, + fullWidth = false, + hover = false, + shadowed = false, + withBackground = false, + ...rest + } = props; return (
>; + +const mockUseStore = (rest?: any, connected = false) => { + const store = { + userStore: { + currentUser: { + messaging_backends: { + MOBILE_APP: { connected }, + }, + } as unknown as User, + ...(rest ? rest : {}), + } as unknown as UserStore, + } as unknown as RootStore; + + useStore.mockReturnValue(store); + + return store; +}; + +const USER_PK = '8585'; +const BACKEND = 'MOBILE_APP'; + +describe('MobileAppVerification', () => { + test('it shows a loading message if it is currently fetching the QR code', async () => { + const { userStore } = mockUseStore({ + sendBackendConfirmationCode: jest.fn().mockResolvedValueOnce('dfd'), + }); + + const component = render(); + expect(component.container).toMatchSnapshot(); + + expect(userStore.sendBackendConfirmationCode).toHaveBeenCalledTimes(1); + expect(userStore.sendBackendConfirmationCode).toHaveBeenCalledWith(USER_PK, BACKEND); + }); + + test('it shows a message when the mobile app is already connected', async () => { + const { userStore } = mockUseStore( + { + sendBackendConfirmationCode: jest.fn().mockResolvedValueOnce('dfd'), + }, + true + ); + + const component = render(); + expect(component.container).toMatchSnapshot(); + + expect(userStore.sendBackendConfirmationCode).toHaveBeenCalledTimes(0); + }); + + test('it shows an error message if there was an error fetching the QR code', async () => { + const { userStore } = mockUseStore({ + sendBackendConfirmationCode: jest.fn().mockRejectedValueOnce('dfd'), + }); + + const component = render(); + await screen.findByText(/.*error fetching your QR code.*/); + + expect(component.container).toMatchSnapshot(); + + expect(userStore.sendBackendConfirmationCode).toHaveBeenCalledTimes(1); + expect(userStore.sendBackendConfirmationCode).toHaveBeenCalledWith(USER_PK, BACKEND); + }); + + test("it shows a QR code if the app isn't already connected", async () => { + const { userStore } = mockUseStore({ + sendBackendConfirmationCode: jest.fn().mockResolvedValueOnce('dfd'), + }); + + const component = render(); + await screen.findByText(/.*the QR code is only valid for one minute.*/); + + expect(component.container).toMatchSnapshot(); + + expect(userStore.sendBackendConfirmationCode).toHaveBeenCalledTimes(1); + expect(userStore.sendBackendConfirmationCode).toHaveBeenCalledWith(USER_PK, BACKEND); + }); + + test('if we disconnect the app, it disconnects and fetches a new QR code', async () => { + const { userStore } = mockUseStore( + { + sendBackendConfirmationCode: jest.fn().mockResolvedValueOnce('dfd'), + unlinkBackend: jest.fn().mockResolvedValueOnce('asdfadsfafds'), + }, + true + ); + + const component = render(); + + const user = userEvent.setup(); + const button = await screen.findByRole('button'); + + // click the disconnect button, which opens the modal + await user.click(button); + // click the confirm button within the modal, which actually triggers the callback + await user.click(screen.getByText('Remove')); + + await screen.findByText(/.*the QR code is only valid for one minute.*/); + + expect(component.container).toMatchSnapshot(); + + expect(userStore.sendBackendConfirmationCode).toHaveBeenCalledTimes(1); + expect(userStore.sendBackendConfirmationCode).toHaveBeenCalledWith(USER_PK, BACKEND); + + expect(userStore.unlinkBackend).toHaveBeenCalledTimes(1); + expect(userStore.unlinkBackend).toHaveBeenCalledWith(USER_PK, BACKEND); + }); + + test('it shows a loading message if it is currently disconnecting', async () => { + const { userStore } = mockUseStore( + { + sendBackendConfirmationCode: jest.fn().mockResolvedValueOnce('dfd'), + unlinkBackend: jest.fn().mockResolvedValueOnce('aaa'), + }, + true + ); + + const component = render(); + + const user = userEvent.setup(); + const button = await screen.findByRole('button'); + + // click the disconnect button, which opens the modal + await user.click(button); + // click the confirm button within the modal, which actually triggers the callback + // this is maybe a bit "hacky" but by not awaiting the below promise it allows us to check the loading state.. + user.click(screen.getByText('Remove')); + + // wait for loading state + await screen.findByText(/.*Loading.*/); + + expect(component.container).toMatchSnapshot(); + + expect(userStore.sendBackendConfirmationCode).toHaveBeenCalledTimes(1); + expect(userStore.sendBackendConfirmationCode).toHaveBeenCalledWith(USER_PK, BACKEND); + + expect(userStore.unlinkBackend).toHaveBeenCalledTimes(1); + expect(userStore.unlinkBackend).toHaveBeenCalledWith(USER_PK, BACKEND); + }); + + test('it shows an error message if there was an error disconnecting the mobile app', async () => { + const { userStore } = mockUseStore( + { + sendBackendConfirmationCode: jest.fn().mockResolvedValueOnce('dfd'), + unlinkBackend: jest.fn().mockRejectedValueOnce('asdfadsfafds'), + }, + true + ); + + const component = render(); + + const user = userEvent.setup(); + const button = await screen.findByRole('button'); + + // click the disconnect button, which opens the modal + await user.click(button); + // click the confirm button within the modal, which actually triggers the callback + await user.click(screen.getByText('Remove')); + + await screen.findByText(/.*error disconnecting your mobile app.*/); + + expect(component.container).toMatchSnapshot(); + + expect(userStore.sendBackendConfirmationCode).toHaveBeenCalledTimes(0); + + expect(userStore.unlinkBackend).toHaveBeenCalledTimes(1); + expect(userStore.unlinkBackend).toHaveBeenCalledWith(USER_PK, BACKEND); + }); +}); diff --git a/grafana-plugin/src/containers/MobileAppVerification/MobileAppVerification.tsx b/grafana-plugin/src/containers/MobileAppVerification/MobileAppVerification.tsx index 5413c270..cdaf5af2 100644 --- a/grafana-plugin/src/containers/MobileAppVerification/MobileAppVerification.tsx +++ b/grafana-plugin/src/containers/MobileAppVerification/MobileAppVerification.tsx @@ -1,104 +1,107 @@ -import React, { HTMLAttributes, useEffect, useState } from 'react'; +import React, { useCallback, useEffect, useState } from 'react'; -import { Button, LoadingPlaceholder } from '@grafana/ui'; -import cn from 'classnames/bind'; +import { HorizontalGroup, LoadingPlaceholder, VerticalGroup } from '@grafana/ui'; import { observer } from 'mobx-react'; +import Block from 'components/GBlock/Block'; import Text from 'components/Text/Text'; -import { WithPermissionControl } from 'containers/WithPermissionControl/WithPermissionControl'; import { User } from 'models/user/user.types'; import { useStore } from 'state/useStore'; -import { UserAction } from 'state/userAction'; -import styles from './MobileAppVerification.module.css'; +import DisconnectButton from './parts/DisconnectButton'; +import DownloadIcons from './parts/DownloadIcons'; +import QRCode from './parts/QRCode'; -const cx = cn.bind(styles); +type Props = { + userPk: User['pk']; +}; -interface MobileAppVerificationProps extends HTMLAttributes { - userPk?: User['pk']; - phone?: string; -} +const BACKEND = 'MOBILE_APP'; -const MobileAppVerification = observer((props: MobileAppVerificationProps) => { - const { userPk: propsUserPk } = props; +const MobileAppVerification = observer(({ userPk }: Props) => { + const { userStore } = useStore(); - const store = useStore(); - const { userStore } = store; + const [mobileAppIsCurrentlyConnected, setMobileAppIsCurrentlyConnected] = useState( + userStore.currentUser.messaging_backends[BACKEND]?.connected === true + ); - const userPk = (propsUserPk || userStore.currentUserPk) as User['pk']; - const user = userStore.items[userPk as User['pk']]; - const isCurrent = userStore.currentUserPk === user.pk; - const action = isCurrent ? UserAction.UpdateOwnSettings : UserAction.UpdateOtherUsersSettings; + const [fetchingQRCode, setFetchingQRCode] = useState(!mobileAppIsCurrentlyConnected); + const [QRCodeValue, setQRCodeValue] = useState(null); + const [errorFetchingQRCode, setErrorFetchingQRCode] = useState(null); - const [showMobileAppVerificationToken, setShowMobileAppVerificationToken] = useState(undefined); - const [isMobileAppVerificationTokenExisting, setIsMobileAppVerificationTokenExisting] = useState(false); - const [MobileAppVerificationTokenLoading, setMobileAppVerificationTokenLoading] = useState(true); + const [disconnectingMobileApp, setDisconnectingMobileApp] = useState(false); + const [errorDisconnectingMobileApp, setErrorDisconnectingMobileApp] = useState(null); - const handleCreateMobileAppVerificationToken = async () => { - setIsMobileAppVerificationTokenExisting(true); - await userStore - .sendBackendConfirmationCode(userPk, 'MOBILE_APP') - .then((res) => setShowMobileAppVerificationToken(res)); - }; + const fetchQRCode = useCallback(async () => { + setFetchingQRCode(true); + try { + // backend verification code that we receive is a JSON object that has been "stringified" + const qrCodeContent = await userStore.sendBackendConfirmationCode(userPk, BACKEND); + setQRCodeValue(qrCodeContent); + } catch (e) { + setErrorFetchingQRCode('There was an error fetching your QR code. Please try again.'); + } + setFetchingQRCode(false); + }, [userPk]); - useEffect(() => { - handleCreateMobileAppVerificationToken().then(() => { - setMobileAppVerificationTokenLoading(false); - }); + const resetState = useCallback(() => { + setErrorDisconnectingMobileApp(null); + setMobileAppIsCurrentlyConnected(false); + setQRCodeValue(null); }, []); + const disconnectMobileApp = useCallback(async () => { + setDisconnectingMobileApp(true); + + try { + await userStore.unlinkBackend(userPk, BACKEND); + resetState(); + } catch (e) { + setErrorDisconnectingMobileApp('There was an error disconnecting your mobile app. Please try again.'); + } + setDisconnectingMobileApp(false); + }, [userPk, resetState]); + + useEffect(() => { + if (!mobileAppIsCurrentlyConnected) { + fetchQRCode(); + } + }, [mobileAppIsCurrentlyConnected]); + + let content: React.ReactNode = null; + + if (fetchingQRCode || disconnectingMobileApp) { + content = ; + } else if (errorFetchingQRCode || errorDisconnectingMobileApp) { + content = {errorFetchingQRCode || errorDisconnectingMobileApp}; + } else if (mobileAppIsCurrentlyConnected) { + content = ( + + Your mobile app is currently connected. Click below to disconnect. + + + ); + } else if (QRCodeValue) { + content = ( + + + + Note: the QR code is only valid for one minute. If you have issues connecting your mobile app, try refreshing + this page to generate a new code. + + + ); + } + return ( -
- {MobileAppVerificationTokenLoading ? ( - - ) : ( - <> -

- Open Grafana OnCall mobile application and enter the following code to add the new device: -

- {isMobileAppVerificationTokenExisting ? ( - <> - {showMobileAppVerificationToken !== undefined ? ( - <> -

{showMobileAppVerificationToken}

-

- * This code is active only for a minute -

-

- - - -

- - ) : ( - <> - )} - - ) : ( -

- - - -

- )} -

- * Only iOS is currently supported -

- - )} -
+ + + {content} + + + + + ); }); diff --git a/grafana-plugin/src/containers/MobileAppVerification/__snapshots__/MobileAppVerification.test.tsx.snap b/grafana-plugin/src/containers/MobileAppVerification/__snapshots__/MobileAppVerification.test.tsx.snap new file mode 100644 index 00000000..bb4670cf --- /dev/null +++ b/grafana-plugin/src/containers/MobileAppVerification/__snapshots__/MobileAppVerification.test.tsx.snap @@ -0,0 +1,5191 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`MobileAppVerification if we disconnect the app, it disconnects and fetches a new QR code 1`] = ` +
+
+
+
+
+
+
+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
+
+
+ + Note: the QR code is only valid for one minute. If you have issues connecting your mobile app, try refreshing this page to generate a new code. + +
+
+
+
+
+
+
+
+ + Download + +
+
+ + The Grafana IRM app is available on both the App Store and Google Play Store. + +
+
+
+
+
+ Apple + + iOS + +
+
+
+
+ Play Store + + Android + +
+
+
+
+
+
+
+
+
+`; + +exports[`MobileAppVerification it shows a QR code if the app isn't already connected 1`] = ` +
+
+
+
+
+
+
+ + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + +
+
+
+ + Note: the QR code is only valid for one minute. If you have issues connecting your mobile app, try refreshing this page to generate a new code. + +
+
+
+
+
+
+
+
+ + Download + +
+
+ + The Grafana IRM app is available on both the App Store and Google Play Store. + +
+
+
+
+
+ Apple + + iOS + +
+
+
+
+ Play Store + + Android + +
+
+
+
+
+
+
+
+
+`; + +exports[`MobileAppVerification it shows a loading message if it is currently disconnecting 1`] = ` +
+
+
+
+
+ Loading... + +
+