From 5554411789a4a3b5b51094c417f37e7a4d0ed80e Mon Sep 17 00:00:00 2001 From: Stephen Cefali Date: Wed, 28 Aug 2024 06:02:52 +0800 Subject: [PATCH] feat: enable updating webhook events for published integration (#75903) This PR enables updating events for published apps. There is a bug where we would prevent updating the webhook events because the scopes were out of order which actually turned out to be a blessing in disguise because the current implementation for updating the service hooks was not correct for published integrations which are installed by multiple integrations. As it happens, for that to work we have to update _every_ `ServiceHook` row that is attached to that integration. Because there could be thousands of them, I put that logic in a task because it would almost certainly time out if there are many installations. Note I still have to update the UI next so we allow updating the installation; right now the UI will prevent the update. --- src/sentry/sentry_apps/apps.py | 53 ++--- src/sentry/tasks/sentry_apps.py | 18 ++ .../utils/sentry_apps/service_hook_manager.py | 33 +++ .../api/endpoints/test_sentry_app_details.py | 211 +++++++++++++++++- 4 files changed, 288 insertions(+), 27 deletions(-) create mode 100644 src/sentry/utils/sentry_apps/service_hook_manager.py diff --git a/src/sentry/sentry_apps/apps.py b/src/sentry/sentry_apps/apps.py index 05004356624aa..f48c0b2a1af5a 100644 --- a/src/sentry/sentry_apps/apps.py +++ b/src/sentry/sentry_apps/apps.py @@ -37,9 +37,12 @@ SentryAppInstallationCreator, SentryAppInstallationTokenCreator, ) -from sentry.sentry_apps.services.hook import hook_service +from sentry.tasks.sentry_apps import create_or_update_service_hooks_for_sentry_app from sentry.users.models.user import User from sentry.users.services.user.model import RpcUser +from sentry.utils.sentry_apps.service_hook_manager import ( + create_or_update_service_hooks_for_installation, +) Schema = Mapping[str, Any] @@ -151,10 +154,9 @@ def _update_status(self, user: User) -> None: def _update_scopes(self) -> None: if self.scopes is not None: - if ( - self.sentry_app.status == SentryAppStatus.PUBLISHED - and self.sentry_app.scope_list != self.scopes - ): + if self.sentry_app.status == SentryAppStatus.PUBLISHED and set( + self.sentry_app.scope_list + ) != set(self.scopes): raise APIError("Cannot update permissions on a published integration.") # We are using a pre_save signal to enforce scope hierarchy on the ApiToken model. @@ -185,30 +187,31 @@ def _update_events(self) -> None: self.sentry_app.events = expand_events(self.events) def _update_service_hooks(self) -> None: - hooks = hook_service.update_webhook_and_events( - organization_id=self.sentry_app.owner_id, - application_id=self.sentry_app.application_id, + if self.sentry_app.is_published: + # if it's a published integration, we need to do many updates so we have to do it in a task so we don't time out + # the client won't know it succeeds but there's not much we can do about that unfortunately + create_or_update_service_hooks_for_sentry_app.apply_async( + kwargs={ + "sentry_app_id": self.sentry_app.id, + "webhook_url": self.sentry_app.webhook_url, + "events": self.sentry_app.events, + } + ) + return + + # for unpublished integrations that aren't installed yet, we may not have an installation + # if we don't, then won't have any service hooks + try: + installation = SentryAppInstallation.objects.get(sentry_app_id=self.sentry_app.id) + except SentryAppInstallation.DoesNotExist: + return + + create_or_update_service_hooks_for_installation( + installation=installation, webhook_url=self.sentry_app.webhook_url, events=self.sentry_app.events, ) - # if we don't have hooks but we have a webhook url now, need to create it for an internal integration - if self.sentry_app.webhook_url and self.sentry_app.is_internal and not hooks: - installation = SentryAppInstallation.objects.get(sentry_app_id=self.sentry_app.id) - # Note that because the update transaction is disjoint with this transaction, it is still - # possible we redundantly create service hooks in the face of two concurrent requests. - # If this proves a problem, we would need to add an additional semantic, "only create if does not exist". - # But I think, it should be fine. - hook_service.create_service_hook( - application_id=self.sentry_app.application_id, - actor_id=installation.id, - installation_id=installation.id, - organization_id=self.sentry_app.owner_id, - project_ids=[], - events=self.sentry_app.events, - url=self.sentry_app.webhook_url, - ) - def _update_webhook_url(self) -> None: if self.webhook_url is not None: self.sentry_app.webhook_url = self.webhook_url diff --git a/src/sentry/tasks/sentry_apps.py b/src/sentry/tasks/sentry_apps.py index 75611560e6ab2..0d2fec7202bb9 100644 --- a/src/sentry/tasks/sentry_apps.py +++ b/src/sentry/tasks/sentry_apps.py @@ -27,6 +27,9 @@ from sentry.utils import metrics from sentry.utils.http import absolute_uri from sentry.utils.sentry_apps import send_and_save_webhook_request +from sentry.utils.sentry_apps.service_hook_manager import ( + create_or_update_service_hooks_for_installation, +) logger = logging.getLogger("sentry.tasks.sentry_apps") @@ -405,3 +408,18 @@ def send_webhooks(installation, event, **kwargs): request_data, servicehook.sentry_app.webhook_url, ) + + +@instrumented_task( + "sentry.tasks.create_or_update_service_hooks_for_sentry_app", **CONTROL_TASK_OPTIONS +) +def create_or_update_service_hooks_for_sentry_app( + sentry_app_id: int, webhook_url: str, events: list[str], **kwargs: dict +) -> None: + installations = SentryAppInstallation.objects.filter(sentry_app_id=sentry_app_id) + for installation in installations: + create_or_update_service_hooks_for_installation( + installation=installation, + events=events, + webhook_url=webhook_url, + ) diff --git a/src/sentry/utils/sentry_apps/service_hook_manager.py b/src/sentry/utils/sentry_apps/service_hook_manager.py new file mode 100644 index 0000000000000..040f31531d6a3 --- /dev/null +++ b/src/sentry/utils/sentry_apps/service_hook_manager.py @@ -0,0 +1,33 @@ +from sentry.models.integrations.sentry_app_installation import SentryAppInstallation +from sentry.sentry_apps.services.hook import hook_service + + +def create_or_update_service_hooks_for_installation( + installation: SentryAppInstallation, webhook_url: str, events: list[str] +) -> None: + """ + This function creates or updates service hooks for a given Sentry app installation. + It first attempts to update the webhook URL and events for existing service hooks. + If no hooks are found and a webhook URL is provided, it creates a new service hook. + Should only be called in the control silo + """ + hooks = hook_service.update_webhook_and_events( + organization_id=installation.organization_id, + application_id=installation.sentry_app.application_id, + webhook_url=webhook_url, + events=events, + ) + if webhook_url and not hooks: + # Note that because the update transaction is disjoint with this transaction, it is still + # possible we redundantly create service hooks in the face of two concurrent requests. + # If this proves a problem, we would need to add an additional semantic, "only create if does not exist". + # But I think, it should be fine. + hook_service.create_service_hook( + application_id=installation.sentry_app.application_id, + actor_id=installation.id, + installation_id=installation.id, + organization_id=installation.organization_id, + project_ids=[], + events=events, + url=webhook_url, + ) diff --git a/tests/sentry/api/endpoints/test_sentry_app_details.py b/tests/sentry/api/endpoints/test_sentry_app_details.py index 80fdbdd42bef0..14bbb62136d70 100644 --- a/tests/sentry/api/endpoints/test_sentry_app_details.py +++ b/tests/sentry/api/endpoints/test_sentry_app_details.py @@ -11,10 +11,12 @@ from sentry.models.integrations.sentry_app import SentryApp from sentry.models.integrations.sentry_app_installation import SentryAppInstallation from sentry.models.organizationmember import OrganizationMember +from sentry.models.servicehook import ServiceHook from sentry.silo.base import SiloMode from sentry.testutils.cases import APITestCase from sentry.testutils.helpers import with_feature from sentry.testutils.helpers.options import override_options +from sentry.testutils.outbox import outbox_runner from sentry.testutils.silo import assume_test_silo_mode, control_silo_test @@ -194,6 +196,52 @@ def test_update_unpublished_app(self): "description": "Organizations can **open a line to Sentry's stack trace** in another service.", }, ] + assert not SentryAppInstallation.objects.filter(sentry_app=self.unpublished_app).exists() + with assume_test_silo_mode(SiloMode.REGION): + assert not ServiceHook.objects.filter( + application_id=self.unpublished_app.application_id + ).exists() + + def test_update_internal_app(self): + self.get_success_response( + self.internal_integration.slug, + webhookUrl="https://newurl.com", + scopes=("event:read",), + events=("issue",), + status_code=200, + ) + self.internal_integration.refresh_from_db() + assert self.internal_integration.webhook_url == "https://newurl.com" + + installation = SentryAppInstallation.objects.get(sentry_app=self.internal_integration) + with assume_test_silo_mode(SiloMode.REGION): + hook = ServiceHook.objects.get(application_id=self.internal_integration.application_id) + + assert hook.application_id == self.internal_integration.application_id + assert hook.organization_id == self.internal_integration.owner_id + assert hook.actor_id == installation.id + assert hook.url == "https://newurl.com" + assert set(hook.events) == { + "issue.assigned", + "issue.created", + "issue.ignored", + "issue.resolved", + "issue.unresolved", + } + assert hook.project_id is None + + # New test to check if the internal integration's webhook URL is updated correctly + self.get_success_response( + self.internal_integration.slug, + webhookUrl="https://updatedurl.com", + status_code=200, + ) + self.internal_integration.refresh_from_db() + assert self.internal_integration.webhook_url == "https://updatedurl.com" + + # Verify the service hook URL is also updated + hook.refresh_from_db() + assert hook.url == "https://updatedurl.com" def test_can_update_name_with_non_unique_name(self): sentry_app = self.create_sentry_app(name="Foo Bar", organization=self.organization) @@ -219,12 +267,171 @@ def test_cannot_update_scopes_published_app(self): response = self.get_error_response( self.published_app.slug, name="NewName", - webookUrl="https://newurl.com", + webhookUrl="https://newurl.com", scopes=("project:read",), status_code=400, ) assert response.data["detail"] == "Cannot update permissions on a published integration." + def test_add_service_hooks_and_update_scope(self): + # first install the app on two organizations + org1 = self.create_organization(name="Org1") + org2 = self.create_organization(name="Org2") + + installation1 = self.create_sentry_app_installation( + organization=org1, slug=self.published_app.slug + ) + installation2 = self.create_sentry_app_installation( + organization=org2, slug=self.published_app.slug + ) + + assert installation1.organization_id == org1.id + assert installation2.organization_id == org2.id + assert installation1.sentry_app == self.published_app + assert installation2.sentry_app == self.published_app + self.published_app.scope_list = ("event:write", "event:read") + self.published_app.save() + + # for published integrations, it runs in a task + with self.tasks(), outbox_runner(): + self.get_success_response( + self.published_app.slug, + webhookUrl="https://newurl.com", + scopes=("event:read", "event:write"), + events=("issue",), + status_code=200, + ) + self.published_app.refresh_from_db() + assert set(self.published_app.scope_list) == {"event:write", "event:read"} + assert ( + self.published_app.webhook_url == "https://newurl.com" + ), f"Unexpected webhook URL: {self.published_app.webhook_url}" + # Check service hooks for each organization + with assume_test_silo_mode(SiloMode.REGION): + service_hooks_org1 = ServiceHook.objects.filter( + organization_id=org1.id, application_id=self.published_app.application_id + ) + service_hooks_org2 = ServiceHook.objects.filter( + organization_id=org2.id, application_id=self.published_app.application_id + ) + + assert len(service_hooks_org1) > 0, f"No service hooks found for Org1 (ID: {org1.id})" + assert len(service_hooks_org2) > 0, f"No service hooks found for Org2 (ID: {org2.id})" + + for hook in service_hooks_org1: + assert hook.application_id == self.published_app.application_id + assert hook.organization_id == org1.id + assert hook.actor_id == installation1.id + assert hook.url == "https://newurl.com" + assert set(hook.events) == { + "issue.assigned", + "issue.created", + "issue.ignored", + "issue.resolved", + "issue.unresolved", + } + assert hook.project_id is None + + for hook in service_hooks_org2: + assert hook.application_id == self.published_app.application_id + assert hook.organization_id == org2.id + assert hook.actor_id == installation2.id + assert hook.url == "https://newurl.com" + assert set(hook.events) == { + "issue.assigned", + "issue.created", + "issue.ignored", + "issue.resolved", + "issue.unresolved", + } + assert hook.project_id is None + + def test_update_existing_published_integration_with_webhooks(self): + org1 = self.create_organization() + org2 = self.create_organization() + # add the webhooks but no events yet + published_app = self.create_sentry_app( + name="TestApp", + organization=self.organization, + webhook_url="https://oldurl.com", + scopes=("event:read", "event:write"), + published=True, + ) + installation1 = self.create_sentry_app_installation( + slug=published_app.slug, organization=org1 + ) + installation2 = self.create_sentry_app_installation( + slug=published_app.slug, organization=org2 + ) + # Assert initial service hooks are created + with assume_test_silo_mode(SiloMode.REGION): + service_hooks_org1 = ServiceHook.objects.filter( + organization_id=org1.id, application_id=published_app.application_id + ) + service_hooks_org2 = ServiceHook.objects.filter( + organization_id=org2.id, application_id=published_app.application_id + ) + + assert len(service_hooks_org1) > 0, "No service hooks found for Org1" + assert len(service_hooks_org2) > 0, "No service hooks found for Org2" + + for hook in service_hooks_org1: + assert hook.url == "https://oldurl.com" + assert set(hook.events) == set() + + for hook in service_hooks_org2: + assert hook.url == "https://oldurl.com" + assert set(hook.events) == set() + + # Update the webhook URL and events + with self.tasks(): + self.get_success_response( + published_app.slug, + webhookUrl="https://newurl.com", + events=("issue",), + status_code=200, + ) + + # Assert the service hooks are updated + published_app.refresh_from_db() + assert published_app.webhook_url == "https://newurl.com" + + with assume_test_silo_mode(SiloMode.REGION): + service_hooks_org1 = ServiceHook.objects.filter( + organization_id=org1.id, application_id=published_app.application_id + ) + service_hooks_org2 = ServiceHook.objects.filter( + organization_id=org2.id, application_id=published_app.application_id + ) + + for hook in service_hooks_org1: + assert hook.application_id == published_app.application_id + assert hook.organization_id == org1.id + assert hook.actor_id == installation1.id + assert hook.url == "https://newurl.com" + assert set(hook.events) == { + "issue.assigned", + "issue.created", + "issue.ignored", + "issue.resolved", + "issue.unresolved", + } + assert hook.project_id is None + + for hook in service_hooks_org2: + assert hook.application_id == published_app.application_id + assert hook.organization_id == org2.id + assert hook.actor_id == installation2.id + assert hook.url == "https://newurl.com" + assert set(hook.events) == { + "issue.assigned", + "issue.created", + "issue.ignored", + "issue.resolved", + "issue.unresolved", + } + assert hook.project_id is None + def test_cannot_update_features_published_app_permissions(self): response = self.get_error_response( self.published_app.slug, @@ -238,7 +445,7 @@ def test_cannot_update_non_owned_apps(self): self.get_error_response( app.slug, name="NewName", - webookUrl="https://newurl.com", + webhookUrl="https://newurl.com", status_code=404, )