From b399650d7acb5ba939473f56365f7fccdec54f82 Mon Sep 17 00:00:00 2001 From: SK Akram Date: Sat, 3 Jan 2026 08:50:07 +0000 Subject: [PATCH 1/2] fix: auto-correct Slack channel names from IDs --- .../serializers/alert_rule_trigger_action.py | 19 +++++- src/sentry/integrations/slack/actions/form.py | 15 ++++- .../integrations/slack/utils/channel.py | 41 +++++++----- .../test_organization_alert_rule_details.py | 66 +++++++++---------- .../integrations/slack/test_notify_action.py | 39 +++++++++-- .../integrations/slack/utils/test_channel.py | 23 ++++--- 6 files changed, 130 insertions(+), 73 deletions(-) diff --git a/src/sentry/incidents/serializers/alert_rule_trigger_action.py b/src/sentry/incidents/serializers/alert_rule_trigger_action.py index d0d13dc104e99e..87095e8664a031 100644 --- a/src/sentry/incidents/serializers/alert_rule_trigger_action.py +++ b/src/sentry/incidents/serializers/alert_rule_trigger_action.py @@ -192,16 +192,27 @@ def validate(self, attrs): should_validate_channel_id = self.context.get("validate_channel_id", True) # validate_channel_id is assumed to be true unless explicitly passed as false if attrs["input_channel_id"] and should_validate_channel_id: - validate_slack_entity_id( + # validate_slack_entity_id returns the actual name from Slack API + # This enables auto-correction when users provide channel IDs as names + actual_slack_name = validate_slack_entity_id( integration_id=attrs["integration_id"], input_name=identifier, input_id=attrs["input_channel_id"], ) + # Store the actual name for use in target_display later + attrs["actual_slack_name"] = actual_slack_name return attrs def create(self, validated_data): for key in ("id", "sentry_app_installation_uuid"): validated_data.pop(key, None) + + # If we have the actual Slack name from validation, use it for target_identifier + # This fixes the issue where channel IDs are used as channel names (issue #105478) + actual_slack_name = validated_data.pop("actual_slack_name", None) + if actual_slack_name is not None: + validated_data["target_identifier"] = actual_slack_name + try: action = create_alert_rule_trigger_action( trigger=self.context["trigger"], **validated_data @@ -229,6 +240,12 @@ def update(self, instance, validated_data): for key in ("id", "sentry_app_installation_uuid"): validated_data.pop(key, None) + # If we have the actual Slack name from validation, use it for target_identifier + # This fixes the issue where channel IDs are used as channel names (issue #105478) + actual_slack_name = validated_data.pop("actual_slack_name", None) + if actual_slack_name is not None: + validated_data["target_identifier"] = actual_slack_name + try: action = update_alert_rule_trigger_action(instance, **validated_data) except (ApiRateLimitedError, InvalidTriggerActionError) as e: diff --git a/src/sentry/integrations/slack/actions/form.py b/src/sentry/integrations/slack/actions/form.py index 863d1f48641618..9001b333d50f79 100644 --- a/src/sentry/integrations/slack/actions/form.py +++ b/src/sentry/integrations/slack/actions/form.py @@ -52,6 +52,10 @@ def clean(self) -> dict[str, Any] | None: or self.data.get("input_channel_id") or self.data.get("channel_id") ) + # Track the actual name from Slack API for auto-correction + actual_slack_name: str | None = None + channel_prefix = "" + if channel_id: logger.info( "rule.slack.provide_channel_id", @@ -80,7 +84,8 @@ def clean(self) -> dict[str, Any] | None: if channel_id: try: - validate_slack_entity_id( + # validate_slack_entity_id now returns the actual name from Slack API + actual_slack_name = validate_slack_entity_id( integration_id=workspace, input_name=self.data["channel"], input_id=channel_id, @@ -102,7 +107,6 @@ def clean(self) -> dict[str, Any] | None: channel = cleaned_data.get("channel", "") timed_out = False - channel_prefix = "" # XXX(meredith): If the user is creating/updating a rule via the API and provides # the channel_id in the request, we don't need to call the channel_transformer - we @@ -130,7 +134,12 @@ def clean(self) -> dict[str, Any] | None: code="invalid", ) - channel = strip_channel_name(channel) + # Use the actual name from Slack API if available (auto-correction for channel ID as name) + if actual_slack_name: + channel = actual_slack_name + else: + channel = strip_channel_name(channel) + if channel_id is None and timed_out: cleaned_data["channel"] = channel_prefix + channel self._pending_save = True diff --git a/src/sentry/integrations/slack/utils/channel.py b/src/sentry/integrations/slack/utils/channel.py index 1393d2c2ce6f70..011a41617f034c 100644 --- a/src/sentry/integrations/slack/utils/channel.py +++ b/src/sentry/integrations/slack/utils/channel.py @@ -91,24 +91,25 @@ def is_input_a_user_id(input_id: str) -> bool: return input_id.startswith("U") or input_id.startswith("W") -def validate_slack_entity_id(*, integration_id: int, input_name: str, input_id: str) -> None: +def validate_slack_entity_id(*, integration_id: int, input_name: str, input_id: str) -> str: """ Accepts a name and input ID that could correspond to a user or channel. + Returns the actual name from Slack API for auto-correction. """ if is_input_a_user_id(input_id): - validate_user_id( + return validate_user_id( input_name=input_name, input_user_id=input_id, integration_id=integration_id ) else: - validate_channel_id( + return validate_channel_id( name=input_name, input_channel_id=input_id, integration_id=integration_id ) -def validate_user_id(*, input_name: str, input_user_id: str, integration_id: int) -> None: +def validate_user_id(*, input_name: str, input_user_id: str, integration_id: int) -> str: """ Validates that a user-input name and ID correspond to the same valid slack user. - Functionally identical to validate_channel_id, but for users. + Returns the actual username from Slack API for auto-correction. """ client = SlackSdkClient(integration_id=integration_id) try: @@ -139,22 +140,25 @@ def validate_user_id(*, input_name: str, input_user_id: str, integration_id: int if not isinstance(results, dict): raise IntegrationError("Bad slack user list response.") - stripped_user_name = strip_channel_name(input_name) - possible_name_matches = [ - results.get("user", {}).get("name"), - results.get("user", {}).get("profile", {}).get("display_name"), - results.get("user", {}).get("profile", {}).get("display_name_normalized"), - ] - if not any(possible_name_matches): + # Get the actual username from Slack API + # Prefer the 'name' field which is the username + actual_username = results.get("user", {}).get("name") + if not actual_username: + # Fallback to display name if username not available + actual_username = results.get("user", {}).get("profile", {}).get("display_name") + if not actual_username: raise ValidationError("Did not receive user name from API results") - if stripped_user_name not in possible_name_matches: - raise ValidationError("Slack username from ID does not match input username.") + return actual_username -def validate_channel_id(name: str, integration_id: int, input_channel_id: str) -> None: + +def validate_channel_id(name: str, integration_id: int, input_channel_id: str) -> str: """ In the case that the user is creating an alert via the API and providing the channel ID and name themselves, we want to make sure both values are correct. + + Returns the actual channel name from Slack API, which can be used to auto-correct + user-provided channel names when they mistakenly use channel IDs. """ client = SlackSdkClient(integration_id=integration_id) @@ -190,12 +194,13 @@ def validate_channel_id(name: str, integration_id: int, input_channel_id: str) - if not isinstance(results, dict): raise IntegrationError("Bad slack channel list response.") - stripped_channel_name = strip_channel_name(name) results_channel_name = results.get("channel", {}).get("name") if not results_channel_name: raise ValidationError("Did not receive channel name from API results") - if stripped_channel_name != results_channel_name: - raise ValidationError("Slack channel name from ID does not match input channel name.") + + # Return the actual channel name from Slack API + # This allows auto-correction when user provides channel ID as name + return results_channel_name def get_channel_id_with_timeout( diff --git a/tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py b/tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py index ba604aeb626ccd..6706c0f7827381 100644 --- a/tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py +++ b/tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py @@ -1606,70 +1606,66 @@ def test_create_slack_alert_with_name_and_channel_id_sdk(self) -> None: assert stored_action["inputChannelId"] == str(channelID) assert stored_action["targetIdentifier"] == channelName - def test_create_slack_alert_with_mismatch_name_and_channel_id_sdk(self) -> None: + def test_channel_name_auto_corrected_from_slack_api(self) -> None: """ - The user specifies the Slack channel and channel ID but they do not match. + When the user provides a channel name that doesn't match the actual channel name from Slack, + the system auto-corrects it to the actual channel name (instead of returning an error). + This is part of the fix for issue #105478. """ self.create_member( user=self.user, organization=self.organization, role="owner", teams=[self.team] ) self.login_as(self.user) - otherChannel = "some-other-channel" - channelName = "my-channel" + actualChannelName = "some-other-channel" + inputChannelName = "my-channel" # User typed wrong name # Specifying an inputChannelID will cause the validate_channel_id logic to be triggered channelID = "C12345678" - channel = {"name": otherChannel} + channel = {"name": actualChannelName} with self.mock_conversations_info(channel): with ( assume_test_silo_mode(SiloMode.REGION), override_settings(SILO_MODE=SiloMode.REGION), ): resp = self._organization_alert_rule_api_call( - channelName=channelName, channelID=channelID + channelName=inputChannelName, channelID=channelID ) - assert resp.status_code == 400 - assert resp.data == { - "nonFieldErrors": [ - ErrorDetail( - string="Slack channel name from ID does not match input channel name.", - code="invalid", - ) - ] - } + # The form should be valid and auto-correct the channel name + assert resp.status_code == 200 + stored_action = resp.data["triggers"][0]["actions"][0] + assert stored_action["inputChannelId"] == str(channelID) + # The target identifier should be auto-corrected to the actual channel name + assert stored_action["targetIdentifier"] == actualChannelName - def test_create_slack_alert_with_mismatch_name_and_user_id_sdk(self) -> None: + def test_username_auto_corrected_from_slack_api(self) -> None: """ - The user specifies the Slack user and user ID but they do not match. + When the user provides a username that doesn't match the actual username from Slack, + the system auto-corrects it to the actual username (instead of returning an error). + This is part of the fix for issue #105478. """ self.create_member( user=self.user, organization=self.organization, role="owner", teams=[self.team] ) self.login_as(self.user) - otherUserId = "U12345678" - otherUser = { - "id": otherUserId, - "name": "kim.possible", + userId = "U12345678" + actualUser = { + "id": userId, + "name": "kim.possible", # This is the actual username "profile": { "display_name": "Kim Possible 🕵️‍♀️", "display_name_normalized": "Kim Possible", }, } - inputName = "Ron Stoppable" + inputName = "Ron Stoppable" # User typed wrong name - with self.mock_users_info(user=otherUser): - resp = self._organization_alert_rule_api_call( - channelName=inputName, channelID=otherUserId - ) - assert resp.status_code == 400 - assert resp.data == { - "nonFieldErrors": [ - ErrorDetail( - string="Slack username from ID does not match input username.", - code="invalid", - ) - ] - } + with self.mock_users_info(user=actualUser): + resp = self._organization_alert_rule_api_call(channelName=inputName, channelID=userId) + # The form should be valid and auto-correct the username + assert resp.status_code == 200 + stored_action = resp.data["triggers"][0]["actions"][0] + assert stored_action["inputChannelId"] == str(userId) + # The target identifier should be auto-corrected to the actual username + assert stored_action["targetIdentifier"] == "kim.possible" def test_create_slack_alert_with_missing_name_from_sdk(self) -> None: """ diff --git a/tests/sentry/integrations/slack/test_notify_action.py b/tests/sentry/integrations/slack/test_notify_action.py index 0bb071fc468adc..e73261cba56fb6 100644 --- a/tests/sentry/integrations/slack/test_notify_action.py +++ b/tests/sentry/integrations/slack/test_notify_action.py @@ -255,6 +255,28 @@ def test_channel_id_provided_sdk(self) -> None: form = rule.get_form_instance() assert form.is_valid() + def test_channel_id_as_channel_name_auto_corrected(self) -> None: + """ + Test that when a user provides a channel ID as the channel name, + the form auto-corrects it to the actual channel name from Slack. + This is for issue #105478. + """ + # User mistakenly puts the channel ID in the channel name field + channel = {"name": "public", "id": "C013TMFDEAV"} + with self.mock_conversations_info(channel): + rule = self.get_rule( + data={ + "workspace": self.integration.id, + "channel": "#C013TMFDEAV", + "input_channel_id": "C013TMFDEAV", + "tags": "", + } + ) + + form = rule.get_form_instance() + # Form should be valid and auto-correct the channel name + self.assert_form_valid(form, "C013TMFDEAV", "#public") + def test_invalid_channel_id_provided_sdk(self) -> None: with patch( "slack_sdk.web.client.WebClient.conversations_info", @@ -273,23 +295,26 @@ def test_invalid_channel_id_provided_sdk(self) -> None: assert not form.is_valid() assert "Channel not found. Invalid ID provided." in str(form.errors.values()) - def test_invalid_channel_name_provided_sdk(self) -> None: + def test_mismatched_channel_name_auto_corrected(self) -> None: + """ + Test that when the user-provided channel name doesn't match the actual name from Slack, + the form auto-corrects it to the actual channel name (instead of raising an error). + This is part of the fix for issue #105478. + """ channel = {"name": "my-channel", "id": "C2349874"} with self.mock_conversations_info(channel): rule = self.get_rule( data={ "workspace": self.integration.id, - "channel": "#my-chanel", - "input_channel_id": "C1234567", + "channel": "#my-chanel", # User typed wrong name + "input_channel_id": "C2349874", "tags": "", } ) form = rule.get_form_instance() - assert not form.is_valid() - assert "Slack: Slack channel name from ID does not match input channel name." in str( - form.errors.values() - ) + # Form should be valid and auto-correct the channel name + self.assert_form_valid(form, "C2349874", "#my-channel") def test_invalid_workspace(self) -> None: # the workspace _should_ be the integration id diff --git a/tests/sentry/integrations/slack/utils/test_channel.py b/tests/sentry/integrations/slack/utils/test_channel.py index 28c4c1a6d81064..07dc9d83ba55c7 100644 --- a/tests/sentry/integrations/slack/utils/test_channel.py +++ b/tests/sentry/integrations/slack/utils/test_channel.py @@ -386,19 +386,24 @@ def test_no_names_from_slack(self) -> None: ) assert mock_client_call.call_count == 1 - def test_no_matches_from_slack(self) -> None: + def test_username_auto_corrected_from_slack(self) -> None: + """ + When user provides a username that doesn't match what Slack returns, + the function should return the actual username from Slack (auto-correction). + This is part of the fix for issue #105478. + """ with patch( "slack_sdk.web.client.WebClient.users_info", return_value=create_user_response(user=self.slack_user), ) as mock_client_call: - with pytest.raises( - ValidationError, match="Slack username from ID does not match input username." - ): - validate_user_id( - input_name="waldo", - input_user_id=self.input_id, - integration_id=self.integration.id, - ) + # Instead of raising ValidationError, it should return the actual username + result = validate_user_id( + input_name="waldo", + input_user_id=self.input_id, + integration_id=self.integration.id, + ) + # Should return the actual username from Slack + assert result == "carmen.sandiego" assert mock_client_call.call_count == 1 def test_happy_path_does_not_raise(self) -> None: From af932e78eb9ac6b5469a0f069e02337b8a1bbaf7 Mon Sep 17 00:00:00 2001 From: SK Akram Date: Sat, 3 Jan 2026 11:38:12 +0000 Subject: [PATCH 2/2] fix(slack): handle user vs channel prefix and add regression test --- src/sentry/integrations/slack/actions/form.py | 6 ++++ .../integrations/slack/test_notify_action.py | 36 +++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/src/sentry/integrations/slack/actions/form.py b/src/sentry/integrations/slack/actions/form.py index 9001b333d50f79..c64505a24edc87 100644 --- a/src/sentry/integrations/slack/actions/form.py +++ b/src/sentry/integrations/slack/actions/form.py @@ -11,6 +11,7 @@ from sentry.integrations.services.integration import integration_service from sentry.integrations.slack.utils.channel import ( get_channel_id, + is_input_a_user_id, strip_channel_name, validate_slack_entity_id, ) @@ -137,6 +138,11 @@ def clean(self) -> dict[str, Any] | None: # Use the actual name from Slack API if available (auto-correction for channel ID as name) if actual_slack_name: channel = actual_slack_name + # If we auto-corrected, we need to ensure the prefix is correct based on the ID type + if is_input_a_user_id(channel_id): + channel_prefix = "@" + else: + channel_prefix = "#" else: channel = strip_channel_name(channel) diff --git a/tests/sentry/integrations/slack/test_notify_action.py b/tests/sentry/integrations/slack/test_notify_action.py index e73261cba56fb6..ce26e51f8317a6 100644 --- a/tests/sentry/integrations/slack/test_notify_action.py +++ b/tests/sentry/integrations/slack/test_notify_action.py @@ -277,6 +277,42 @@ def test_channel_id_as_channel_name_auto_corrected(self) -> None: # Form should be valid and auto-correct the channel name self.assert_form_valid(form, "C013TMFDEAV", "#public") + def test_user_id_as_channel_name_auto_corrected(self) -> None: + """ + Test that when a user provides a User ID as the channel name, + the form auto-corrects it to the actual username with @ prefix. + """ + # User mistakenly puts the User ID in the channel name field + user = {"name": "morty", "id": "U1234567"} + with self.mock_conversations_info(user): + rule = self.get_rule( + data={ + "workspace": self.integration.id, + "channel": "U1234567", + "input_channel_id": "U1234567", + "tags": "", + } + ) + + form = rule.get_form_instance() + # Form should be valid and auto-correct to @morty + # Note: The mock helpers in this test file might behave differently for users vs channels + # We need to ensure mock_conversations_info mocks user lookup correctly or use a different mock + + # The validate_user_id calls client.users_info. + # mock_conversations_info mocks conversations_info BUT validate_slack_entity_id + # will call validate_user_id if ID starts with U, which calls users_info. + # So we need to mock users_info. + + with patch( + "slack_sdk.web.client.WebClient.users_info", + return_value={ + "ok": True, + "user": {"name": "morty", "profile": {"display_name": "Morty Smith"}}, + }, + ): + self.assert_form_valid(form, "U1234567", "@morty") + def test_invalid_channel_id_provided_sdk(self) -> None: with patch( "slack_sdk.web.client.WebClient.conversations_info",