From f5c1a7c1777843cb2f5fd70cbe78a80509f4dad6 Mon Sep 17 00:00:00 2001 From: Raj Joshi Date: Fri, 14 Jun 2024 10:37:38 -0700 Subject: [PATCH 1/4] refactored link identity --- .../integrations/slack/views/link_identity.py | 82 +++++++++++++++---- 1 file changed, 64 insertions(+), 18 deletions(-) diff --git a/src/sentry/integrations/slack/views/link_identity.py b/src/sentry/integrations/slack/views/link_identity.py index 5388e76acf2a94..6ff4ef0edeb710 100644 --- a/src/sentry/integrations/slack/views/link_identity.py +++ b/src/sentry/integrations/slack/views/link_identity.py @@ -1,7 +1,10 @@ +import logging + from django.core.signing import BadSignature, SignatureExpired -from django.http import HttpResponse +from django.db import IntegrityError +from django.http import Http404, HttpRequest, HttpResponse +from django.http.response import HttpResponseBase from django.utils.decorators import method_decorator -from rest_framework.request import Request from sentry.integrations.types import ExternalProviderEnum, ExternalProviders from sentry.integrations.utils import get_identity_or_404 @@ -9,6 +12,7 @@ from sentry.notifications.notificationcontroller import NotificationController from sentry.notifications.notifications.integration_nudge import IntegrationNudgeNotification from sentry.services.hybrid_cloud.integration.model import RpcIntegration +from sentry.utils import metrics from sentry.utils.signing import unsign from sentry.web.frontend.base import BaseView, control_silo_view from sentry.web.helpers import render_to_response @@ -17,6 +21,8 @@ from . import build_linking_url as base_build_linking_url from . import never_cache +_logger = logging.getLogger(__name__) + SUCCESS_LINKED_MESSAGE = ( "Your Slack identity has been linked to your Sentry account. You're good to go!" ) @@ -40,33 +46,72 @@ class SlackLinkIdentityView(BaseView): Django view for linking user to slack account. Creates an entry on Identity table. """ + _METRICS_SUCCESS_KEY = "sentry.integrations.slack.link_identity_view.success" + _METRICS_FAILURE_KEY = "sentry.integrations.slack.link_identity_view.failure" + @method_decorator(never_cache) - def handle(self, request: Request, signed_params: str) -> HttpResponse: + def dispatch(self, request: HttpRequest, *args, **kwargs) -> HttpResponseBase: try: + signed_params = kwargs.pop("signed_params") params = unsign(signed_params) - except (SignatureExpired, BadSignature): + except (SignatureExpired, BadSignature) as e: + _logger.warning("dispatch.signature_error", exc_info=e) + metrics.incr(self._METRICS_FAILURE_KEY, tags={"error": str(e)}, sample_rate=1.0) return render_to_response( "sentry/integrations/slack/expired-link.html", request=request, ) - organization, integration, idp = get_identity_or_404( - ExternalProviders.SLACK, - request.user, - integration_id=params["integration_id"], + try: + organization, integration, idp = get_identity_or_404( + ExternalProviders.SLACK, + request.user, + integration_id=params["integration_id"], + ) + except Http404: + _logger.exception( + "get_identity_error", extra={"integration_id": params["integration_id"]} + ) + metrics.incr(self._METRICS_FAILURE_KEY + ".get_identity", sample_rate=1.0) + raise + + _logger.info("get_identity_success", extra={"integration_id": params["integration_id"]}) + metrics.incr(self._METRICS_SUCCESS_KEY + ".get_identity", sample_rate=1.0) + params.update({"organization": organization, "integration": integration, "idp": idp}) + return super().dispatch(request, params=params) + + def get(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: + params = kwargs["params"] + organization, integration = params["organization"], params["integration"] + + return render_to_response( + "sentry/auth-link-identity.html", + request=request, + context={"organization": organization, "provider": integration.get_provider()}, + ) + + def post(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: + params = kwargs["params"] + organization, integration, idp = ( + params["organization"], + params["integration"], + params["idp"], ) - if request.method != "POST": - return render_to_response( - "sentry/auth-link-identity.html", - request=request, - context={"organization": organization, "provider": integration.get_provider()}, + try: + Identity.objects.link_identity( + user=request.user, idp=idp, external_id=params["slack_id"] ) - - Identity.objects.link_identity(user=request.user, idp=idp, external_id=params["slack_id"]) + except IntegrityError: + _logger.exception("slack.link.integrity_error") + metrics.incr( + self._METRICS_FAILURE_KEYKEY + ".post.identity.integrity_error", + sample_rate=1.0, + ) + raise Http404 send_slack_response(integration, SUCCESS_LINKED_MESSAGE, params, command="link") - has_slack_settings = None + controller = NotificationController( recipients=[request.user], organization_id=organization.id, @@ -77,8 +122,9 @@ def handle(self, request: Request, signed_params: str) -> HttpResponse: if not has_slack_settings: IntegrationNudgeNotification(organization, request.user, ExternalProviders.SLACK).send() - # TODO(epurkhiser): We could do some fancy slack querying here to - # render a nice linking page with info about the user their linking. + _logger.info("link_identity_success", extra={"slack_id": params["slack_id"]}) + metrics.incr(self._METRICS_SUCCESS_KEY + ".post.link_identity", sample_rate=1.0) + return render_to_response( "sentry/integrations/slack/linked.html", request=request, From afa624a151f4d841d371b49618a62e94985ecd46 Mon Sep 17 00:00:00 2001 From: Raj Joshi Date: Fri, 14 Jun 2024 11:49:55 -0700 Subject: [PATCH 2/4] typo --- src/sentry/integrations/slack/views/link_identity.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/integrations/slack/views/link_identity.py b/src/sentry/integrations/slack/views/link_identity.py index 6ff4ef0edeb710..adf31e14b14c5c 100644 --- a/src/sentry/integrations/slack/views/link_identity.py +++ b/src/sentry/integrations/slack/views/link_identity.py @@ -105,7 +105,7 @@ def post(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: except IntegrityError: _logger.exception("slack.link.integrity_error") metrics.incr( - self._METRICS_FAILURE_KEYKEY + ".post.identity.integrity_error", + self._METRICS_FAILURE_KEY + ".post.identity.integrity_error", sample_rate=1.0, ) raise Http404 From 1b781cefbe689e3238ed83a55959f1f8aca560ca Mon Sep 17 00:00:00 2001 From: Raj Joshi Date: Mon, 17 Jun 2024 12:55:46 -0700 Subject: [PATCH 3/4] use dataclass --- .../integrations/slack/views/link_identity.py | 39 +++++++++++++------ src/sentry/integrations/slack/views/types.py | 23 +++++++++++ .../slack/views/unlink_identity.py | 34 ++++++++++++---- 3 files changed, 77 insertions(+), 19 deletions(-) create mode 100644 src/sentry/integrations/slack/views/types.py diff --git a/src/sentry/integrations/slack/views/link_identity.py b/src/sentry/integrations/slack/views/link_identity.py index adf31e14b14c5c..547b6d3ec73c8b 100644 --- a/src/sentry/integrations/slack/views/link_identity.py +++ b/src/sentry/integrations/slack/views/link_identity.py @@ -6,6 +6,8 @@ from django.http.response import HttpResponseBase from django.utils.decorators import method_decorator +from sentry.integrations.slack.views import render_error_page +from sentry.integrations.slack.views.types import IdentityParams from sentry.integrations.types import ExternalProviderEnum, ExternalProviders from sentry.integrations.utils import get_identity_or_404 from sentry.models.identity import Identity @@ -61,6 +63,14 @@ def dispatch(self, request: HttpRequest, *args, **kwargs) -> HttpResponseBase: "sentry/integrations/slack/expired-link.html", request=request, ) + except KeyError as e: + _logger.warning("dispatch.key_error", exc_info=e) + metrics.incr(self._METRICS_FAILURE_KEY, tags={"error": str(e)}, sample_rate=1.0) + return render_error_page( + request, + status=400, + body_text="HTTP 400: Missing required 'signed_params' parameter", + ) try: organization, integration, idp = get_identity_or_404( @@ -91,16 +101,18 @@ def get(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: ) def post(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: - params = kwargs["params"] - organization, integration, idp = ( - params["organization"], - params["integration"], - params["idp"], + params_dict = kwargs["params"] + params = IdentityParams( + organization=params_dict["organization"], + integration=params_dict["integration"], + idp=params_dict["idp"], + slack_id=params_dict["slack_id"], + channel_id=params_dict["channel_id"], ) try: Identity.objects.link_identity( - user=request.user, idp=idp, external_id=params["slack_id"] + user=request.user, idp=params.idp, external_id=params.slack_id ) except IntegrityError: _logger.exception("slack.link.integrity_error") @@ -110,23 +122,28 @@ def post(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: ) raise Http404 - send_slack_response(integration, SUCCESS_LINKED_MESSAGE, params, command="link") + # TODO: We should use use the dataclass to send the slack response + send_slack_response( + params.integration, SUCCESS_LINKED_MESSAGE, params.__dict__, command="link" + ) controller = NotificationController( recipients=[request.user], - organization_id=organization.id, + organization_id=params.organization.id, provider=ExternalProviderEnum.SLACK, ) has_slack_settings = controller.user_has_any_provider_settings(ExternalProviderEnum.SLACK) if not has_slack_settings: - IntegrationNudgeNotification(organization, request.user, ExternalProviders.SLACK).send() + IntegrationNudgeNotification( + params.organization, request.user, ExternalProviders.SLACK + ).send() - _logger.info("link_identity_success", extra={"slack_id": params["slack_id"]}) + _logger.info("link_identity_success", extra={"slack_id": params.slack_id}) metrics.incr(self._METRICS_SUCCESS_KEY + ".post.link_identity", sample_rate=1.0) return render_to_response( "sentry/integrations/slack/linked.html", request=request, - context={"channel_id": params["channel_id"], "team_id": integration.external_id}, + context={"channel_id": params.channel_id, "team_id": params.integration.external_id}, ) diff --git a/src/sentry/integrations/slack/views/types.py b/src/sentry/integrations/slack/views/types.py new file mode 100644 index 00000000000000..aaeef5b50b80a1 --- /dev/null +++ b/src/sentry/integrations/slack/views/types.py @@ -0,0 +1,23 @@ +from dataclasses import dataclass + +from sentry.models.identity import IdentityProvider +from sentry.models.integrations.integration import Integration +from sentry.services.hybrid_cloud.organization import RpcOrganization + + +@dataclass +class IdentityParams: + organization: RpcOrganization + integration: Integration + idp: IdentityProvider + slack_id: str + channel_id: str + response_url: str | None = None + + def __init__(self, organization, integration, idp, slack_id, channel_id, response_url=None): + self.organization = organization + self.integration = integration + self.idp = idp + self.slack_id = slack_id + self.channel_id = channel_id + self.response_url = response_url diff --git a/src/sentry/integrations/slack/views/unlink_identity.py b/src/sentry/integrations/slack/views/unlink_identity.py index 3fde3d803ace3e..0887c44913840a 100644 --- a/src/sentry/integrations/slack/views/unlink_identity.py +++ b/src/sentry/integrations/slack/views/unlink_identity.py @@ -8,7 +8,8 @@ from sentry.integrations.slack.utils.notifications import send_slack_response from sentry.integrations.slack.views import build_linking_url as base_build_linking_url -from sentry.integrations.slack.views import never_cache +from sentry.integrations.slack.views import never_cache, render_error_page +from sentry.integrations.slack.views.types import IdentityParams from sentry.integrations.types import ExternalProviders from sentry.integrations.utils import get_identity_or_404 from sentry.models.identity import Identity @@ -54,6 +55,14 @@ def dispatch(self, request: HttpRequest, signed_params: str) -> HttpResponseBase "sentry/integrations/slack/expired-link.html", request=request, ) + except KeyError as e: + _logger.warning("dispatch.key_error", exc_info=e) + metrics.incr(self._METRICS_FAILURE_KEY, tags={"error": str(e)}, sample_rate=1.0) + return render_error_page( + request, + status=400, + body_text="HTTP 400: Missing required 'signed_params' parameter", + ) try: organization, integration, idp = get_identity_or_404( @@ -84,26 +93,35 @@ def get(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: ) def post(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: - params = kwargs["params"] - integration, idp = params["integration"], params["idp"] + params_dict = kwargs["params"] + params = IdentityParams( + organization=params_dict["organization"], + integration=params_dict["integration"], + idp=params_dict["idp"], + slack_id=params_dict["slack_id"], + channel_id=params_dict["channel_id"], + ) try: - Identity.objects.filter(idp_id=idp.id, external_id=params["slack_id"]).delete() + Identity.objects.filter(idp_id=params.idp, external_id=params.slack_id).delete() except IntegrityError: - _logger.exception("slack.unlink.integrity-error") + _logger.exception("slack.unlink.integrity_error") metrics.incr( self._METRICS_FAILURE_KEY + ".post.identity.integrity_error", sample_rate=1.0, ) raise Http404 - send_slack_response(integration, SUCCESS_UNLINKED_MESSAGE, params, command="unlink") + # TODO: We should use use the dataclass to send the slack response + send_slack_response( + params.integration, SUCCESS_UNLINKED_MESSAGE, params.__dict__, command="unlink" + ) - _logger.info("unlink_identity_success", extra={"slack_id": params["slack_id"]}) + _logger.info("unlink_identity_success", extra={"slack_id": params.slack_id}) metrics.incr(self._METRICS_SUCCESS_KEY + ".post.unlink_identity", sample_rate=1.0) return render_to_response( "sentry/integrations/slack/unlinked.html", request=request, - context={"channel_id": params["channel_id"], "team_id": integration.external_id}, + context={"channel_id": params.channel_id, "team_id": params.integration.external_id}, ) From f2aabc80e0e77e575ca4033b38b011d4e9bfe117 Mon Sep 17 00:00:00 2001 From: Raj Joshi Date: Mon, 17 Jun 2024 16:21:37 -0700 Subject: [PATCH 4/4] pr feedback --- .../integrations/slack/views/__init__.py | 4 +- .../integrations/slack/views/link_identity.py | 55 +++++++++++-------- .../slack/views/unlink_identity.py | 48 +++++++++------- 3 files changed, 63 insertions(+), 44 deletions(-) diff --git a/src/sentry/integrations/slack/views/__init__.py b/src/sentry/integrations/slack/views/__init__.py index 8a905e76c5f0b5..70069b41afd4de 100644 --- a/src/sentry/integrations/slack/views/__init__.py +++ b/src/sentry/integrations/slack/views/__init__.py @@ -1,6 +1,6 @@ from typing import Any -from django.http import HttpResponse +from django.http import HttpRequest, HttpResponse from django.urls import reverse from django.views.decorators.cache import never_cache as django_never_cache from rest_framework.request import Request @@ -23,7 +23,7 @@ def build_linking_url(endpoint: str, **kwargs: Any) -> str: return url -def render_error_page(request: Request, status: int, body_text: str) -> HttpResponse: +def render_error_page(request: Request | HttpRequest, status: int, body_text: str) -> HttpResponse: return render_to_response( "sentry/integrations/slack/link-team-error.html", request=request, diff --git a/src/sentry/integrations/slack/views/link_identity.py b/src/sentry/integrations/slack/views/link_identity.py index 547b6d3ec73c8b..7b51840c3d4217 100644 --- a/src/sentry/integrations/slack/views/link_identity.py +++ b/src/sentry/integrations/slack/views/link_identity.py @@ -5,6 +5,7 @@ from django.http import Http404, HttpRequest, HttpResponse from django.http.response import HttpResponseBase from django.utils.decorators import method_decorator +from rest_framework.request import Request from sentry.integrations.slack.views import render_error_page from sentry.integrations.slack.views.types import IdentityParams @@ -52,9 +53,8 @@ class SlackLinkIdentityView(BaseView): _METRICS_FAILURE_KEY = "sentry.integrations.slack.link_identity_view.failure" @method_decorator(never_cache) - def dispatch(self, request: HttpRequest, *args, **kwargs) -> HttpResponseBase: + def dispatch(self, request: HttpRequest, signed_params: str) -> HttpResponseBase: try: - signed_params = kwargs.pop("signed_params") params = unsign(signed_params) except (SignatureExpired, BadSignature) as e: _logger.warning("dispatch.signature_error", exc_info=e) @@ -63,14 +63,6 @@ def dispatch(self, request: HttpRequest, *args, **kwargs) -> HttpResponseBase: "sentry/integrations/slack/expired-link.html", request=request, ) - except KeyError as e: - _logger.warning("dispatch.key_error", exc_info=e) - metrics.incr(self._METRICS_FAILURE_KEY, tags={"error": str(e)}, sample_rate=1.0) - return render_error_page( - request, - status=400, - body_text="HTTP 400: Missing required 'signed_params' parameter", - ) try: organization, integration, idp = get_identity_or_404( @@ -83,14 +75,20 @@ def dispatch(self, request: HttpRequest, *args, **kwargs) -> HttpResponseBase: "get_identity_error", extra={"integration_id": params["integration_id"]} ) metrics.incr(self._METRICS_FAILURE_KEY + ".get_identity", sample_rate=1.0) - raise + return render_error_page( + request, + status=404, + body_text="HTTP 404: Could not find the Slack identity.", + ) _logger.info("get_identity_success", extra={"integration_id": params["integration_id"]}) metrics.incr(self._METRICS_SUCCESS_KEY + ".get_identity", sample_rate=1.0) params.update({"organization": organization, "integration": integration, "idp": idp}) - return super().dispatch(request, params=params) + return super().dispatch( + request, organization=organization, integration=integration, idp=idp, params=params + ) - def get(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: + def get(self, request: Request, *args, **kwargs) -> HttpResponse: params = kwargs["params"] organization, integration = params["organization"], params["integration"] @@ -100,15 +98,28 @@ def get(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: context={"organization": organization, "provider": integration.get_provider()}, ) - def post(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: - params_dict = kwargs["params"] - params = IdentityParams( - organization=params_dict["organization"], - integration=params_dict["integration"], - idp=params_dict["idp"], - slack_id=params_dict["slack_id"], - channel_id=params_dict["channel_id"], - ) + def post(self, request: Request, *args, **kwargs) -> HttpResponse: + try: + params_dict = kwargs["params"] + params = IdentityParams( + organization=kwargs["organization"], + integration=kwargs["integration"], + idp=kwargs["idp"], + slack_id=params_dict["slack_id"], + channel_id=params_dict["channel_id"], + ) + except KeyError as e: + _logger.exception("slack.link.missing_params") + metrics.incr( + self._METRICS_FAILURE_KEY + ".post.missing_params", + tags={"error": str(e)}, + sample_rate=1.0, + ) + return render_error_page( + request, + status=400, + body_text="HTTP 400: Missing required parameters.", + ) try: Identity.objects.link_identity( diff --git a/src/sentry/integrations/slack/views/unlink_identity.py b/src/sentry/integrations/slack/views/unlink_identity.py index 0887c44913840a..5a13e55b67f261 100644 --- a/src/sentry/integrations/slack/views/unlink_identity.py +++ b/src/sentry/integrations/slack/views/unlink_identity.py @@ -5,6 +5,7 @@ from django.http import Http404, HttpRequest, HttpResponse from django.http.response import HttpResponseBase from django.utils.decorators import method_decorator +from rest_framework.request import Request from sentry.integrations.slack.utils.notifications import send_slack_response from sentry.integrations.slack.views import build_linking_url as base_build_linking_url @@ -55,14 +56,6 @@ def dispatch(self, request: HttpRequest, signed_params: str) -> HttpResponseBase "sentry/integrations/slack/expired-link.html", request=request, ) - except KeyError as e: - _logger.warning("dispatch.key_error", exc_info=e) - metrics.incr(self._METRICS_FAILURE_KEY, tags={"error": str(e)}, sample_rate=1.0) - return render_error_page( - request, - status=400, - body_text="HTTP 400: Missing required 'signed_params' parameter", - ) try: organization, integration, idp = get_identity_or_404( @@ -75,14 +68,20 @@ def dispatch(self, request: HttpRequest, signed_params: str) -> HttpResponseBase "get_identity_error", extra={"integration_id": params["integration_id"]} ) metrics.incr(self._METRICS_FAILURE_KEY + ".get_identity", sample_rate=1.0) - raise + return render_error_page( + request, + status=404, + body_text="HTTP 404: Could not find the Slack identity.", + ) _logger.info("get_identity_success", extra={"integration_id": params["integration_id"]}) metrics.incr(self._METRICS_SUCCESS_KEY + ".get_identity", sample_rate=1.0) params.update({"organization": organization, "integration": integration, "idp": idp}) - return super().dispatch(request, params=params) + return super().dispatch( + request, organization=organization, integration=integration, idp=idp, params=params + ) - def get(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: + def get(self, request: Request, *args, **kwargs) -> HttpResponse: params = kwargs["params"] organization, integration = params["organization"], params["integration"] @@ -92,15 +91,24 @@ def get(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: context={"organization": organization, "provider": integration.get_provider()}, ) - def post(self, request: HttpRequest, *args, **kwargs) -> HttpResponse: - params_dict = kwargs["params"] - params = IdentityParams( - organization=params_dict["organization"], - integration=params_dict["integration"], - idp=params_dict["idp"], - slack_id=params_dict["slack_id"], - channel_id=params_dict["channel_id"], - ) + def post(self, request: Request, *args, **kwargs) -> HttpResponse: + try: + params_dict = kwargs["params"] + params = IdentityParams( + organization=kwargs["organization"], + integration=kwargs["integration"], + idp=kwargs["idp"], + slack_id=params_dict["slack_id"], + channel_id=params_dict["channel_id"], + ) + except KeyError as e: + _logger.exception("slack.unlink.missing_params", extra={"error": str(e)}) + metrics.incr(self._METRICS_FAILURE_KEY + ".post.missing_params", sample_rate=1.0) + return render_error_page( + request, + status=400, + body_text="HTTP 400: Missing required parameters.", + ) try: Identity.objects.filter(idp_id=params.idp, external_id=params.slack_id).delete()