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, )