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

@@ -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

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

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

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