Skip to content

Commit 4ac6ca7

Browse files
authored
fix(aci): filter out non-alertable and broken sentry apps in available actions endpoint (#101866)
fixes 2 bugs that were causing the available actions endpoint to return more results than expected: - we weren't filtering all sentry apps by `is_alertable`, causing the webhook action to return extra SentryAppServices that are not alertable - we weren't calling `prepare_ui_component` on the sentry app components, causing us to return broken sentry apps that fail component preparation (see below) https://github.com/getsentry/sentry/blob/da98041d2de6f134a8ebd92c7f39306aab0ac7cd/src/sentry/sentry_apps/models/sentry_app_installation.py#L255-L256
1 parent 6adafa9 commit 4ac6ca7

File tree

3 files changed

+81
-31
lines changed

3 files changed

+81
-31
lines changed

src/sentry/workflow_engine/endpoints/organization_available_action_index.py

Lines changed: 30 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
from sentry.apidocs.utils import inline_sentry_response_serializer
2121
from sentry.integrations.services.integration import RpcIntegration
2222
from sentry.rules.actions.services import PluginService, SentryAppService
23+
from sentry.sentry_apps.models.sentry_app_installation import prepare_ui_component
2324
from sentry.sentry_apps.services.app import app_service
2425
from sentry.workflow_engine.endpoints.serializers.action_handler_serializer import (
2526
ActionHandlerSerializer,
@@ -77,19 +78,29 @@ def get(self, request, organization):
7778
AvailableIntegration(integration=integration, services=services)
7879
)
7980

80-
sentry_app_component_contexts = app_service.get_installation_component_contexts(
81+
all_sentry_app_contexts = app_service.get_installation_component_contexts(
8182
filter={"organization_id": organization.id},
8283
component_type="alert-rule-action",
8384
include_contexts_without_component=True,
8485
)
8586

86-
# Split contexts into those with and without components
87-
sentry_app_contexts_with_components = [
88-
context for context in sentry_app_component_contexts if context.component
89-
]
90-
sentry_app_contexts_without_components = [
91-
context for context in sentry_app_component_contexts if not context.component
92-
]
87+
# filter for alertable apps and split contexts into those with and without components
88+
alertable_apps_with_components = []
89+
alertable_apps_without_components = []
90+
for context in all_sentry_app_contexts:
91+
if not context.installation.sentry_app.is_alertable:
92+
continue
93+
94+
if context.component:
95+
# filter out broken apps by checking if prepare_ui_component succeeds
96+
prepared_component = prepare_ui_component(
97+
installation=context.installation,
98+
component=context.component,
99+
)
100+
if prepared_component is not None:
101+
alertable_apps_with_components.append(context)
102+
else:
103+
alertable_apps_without_components.append(context)
93104

94105
actions = []
95106
for action_type, handler in action_handler_registry.registrations.items():
@@ -112,27 +123,26 @@ def get(self, request, organization):
112123
)
113124

114125
# add alertable sentry app actions
115-
# sentry app actions are only for sentry apps with components
126+
# sentry app actions are only for alertable sentry apps with components
116127
elif action_type == Action.Type.SENTRY_APP:
117-
for context in sentry_app_contexts_with_components:
118-
if context.installation.sentry_app.is_alertable:
119-
actions.append(
120-
serialize(
121-
handler,
122-
request.user,
123-
ActionHandlerSerializer(),
124-
action_type=action_type,
125-
sentry_app_context=context,
126-
)
128+
for context in alertable_apps_with_components:
129+
actions.append(
130+
serialize(
131+
handler,
132+
request.user,
133+
ActionHandlerSerializer(),
134+
action_type=action_type,
135+
sentry_app_context=context,
127136
)
137+
)
128138

129139
# add webhook action
130140
# service options include plugins and sentry apps without components
131141
elif action_type == Action.Type.WEBHOOK:
132142
plugins = get_notification_plugins_for_org(organization)
133143
sentry_apps: list[PluginService] = [
134144
SentryAppService(context.installation.sentry_app)
135-
for context in sentry_app_contexts_without_components
145+
for context in alertable_apps_without_components
136146
]
137147
available_services: list[PluginService] = plugins + sentry_apps
138148

src/sentry/workflow_engine/endpoints/serializers/action_handler_serializer.py

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33

44
from sentry.api.serializers import Serializer, register
55
from sentry.rules.actions.notify_event_service import PLUGINS_WITH_FIRST_PARTY_EQUIVALENTS
6-
from sentry.sentry_apps.models.sentry_app_installation import prepare_ui_component
76
from sentry.workflow_engine.types import ActionHandler
87

98

@@ -76,16 +75,9 @@ def serialize(
7675
"status": installation.sentry_app.status,
7776
}
7877
if component:
79-
prepared_component = prepare_ui_component(
80-
installation=installation,
81-
component=component,
82-
project_slug=None,
83-
values=None,
84-
)
85-
if prepared_component:
86-
sentry_app["settings"] = prepared_component.app_schema.get("settings", {})
87-
if prepared_component.app_schema.get("title"):
88-
sentry_app["title"] = prepared_component.app_schema.get("title")
78+
sentry_app["settings"] = component.app_schema.get("settings", {})
79+
if component.app_schema.get("title"):
80+
sentry_app["title"] = component.app_schema.get("title")
8981
result["sentryApp"] = sentry_app
9082

9183
services = kwargs.get("services")

tests/sentry/workflow_engine/endpoints/test_organization_available_action_index.py

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,32 @@ class SentryAppActionHandler(ActionHandler):
206206
slug=self.sentry_app.slug, organization=self.organization
207207
)
208208

209+
# should not return sentry apps that are not alertable
210+
self.not_alertable_sentry_app = self.create_sentry_app(
211+
name="Not Alertable Sentry App",
212+
organization=self.organization,
213+
is_alertable=False,
214+
)
215+
self.not_alertable_sentry_app_installation = self.create_sentry_app_installation(
216+
slug=self.not_alertable_sentry_app.slug, organization=self.organization
217+
)
218+
219+
self.not_alertable_sentry_app = self.create_sentry_app(
220+
name="Not Alertable Sentry App With Component",
221+
organization=self.organization,
222+
schema={
223+
"elements": [
224+
self.sentry_app_settings_schema,
225+
]
226+
},
227+
is_alertable=False,
228+
)
229+
self.not_alertable_sentry_app_with_component_installation = (
230+
self.create_sentry_app_installation(
231+
slug=self.not_alertable_sentry_app.slug, organization=self.organization
232+
)
233+
)
234+
209235
# should not return sentry apps that are not installed
210236
self.create_sentry_app(
211237
name="Bad Sentry App",
@@ -388,6 +414,28 @@ def test_sentry_apps(self, mock_sentry_app_component_preparer: MagicMock) -> Non
388414
},
389415
]
390416

417+
@patch(
418+
"sentry.workflow_engine.endpoints.organization_available_action_index.prepare_ui_component"
419+
)
420+
def test_sentry_apps_filters_failed_component_preparation(
421+
self, mock_prepare_ui_component: MagicMock
422+
) -> None:
423+
"""Test that sentry apps whose components fail to prepare are filtered out"""
424+
self.setup_sentry_apps()
425+
426+
# make prepare_ui_component return None to simulate a broken app
427+
mock_prepare_ui_component.return_value = None
428+
429+
response = self.get_success_response(
430+
self.organization.slug,
431+
status_code=200,
432+
)
433+
434+
# verify prepare_ui_component was called
435+
assert mock_prepare_ui_component.called
436+
# should return no sentry apps since component preparation failed
437+
assert len(response.data) == 0
438+
391439
def test_webhooks(self) -> None:
392440
self.setup_webhooks()
393441

0 commit comments

Comments
 (0)