diff --git a/src/sentry/integrations/jira/integration.py b/src/sentry/integrations/jira/integration.py index 3f6fd1d3728914..3d1a11ed181e01 100644 --- a/src/sentry/integrations/jira/integration.py +++ b/src/sentry/integrations/jira/integration.py @@ -15,7 +15,6 @@ from sentry import features from sentry.eventstore.models import GroupEvent -from sentry.exceptions import InvalidConfiguration from sentry.integrations.base import ( FeatureDescription, IntegrationData, @@ -25,7 +24,12 @@ ) from sentry.integrations.jira.models.create_issue_metadata import JiraIssueTypeMetadata from sentry.integrations.jira.tasks import migrate_issues -from sentry.integrations.mixins.issues import MAX_CHAR, IssueSyncIntegration, ResolveSyncAction +from sentry.integrations.mixins.issues import ( + MAX_CHAR, + IntegrationSyncTargetNotFound, + IssueSyncIntegration, + ResolveSyncAction, +) from sentry.integrations.models.external_issue import ExternalIssue from sentry.integrations.models.integration_external_project import IntegrationExternalProject from sentry.integrations.pipeline import IntegrationPipeline @@ -1014,16 +1018,20 @@ def sync_assignee_outbound( }, ) if not user.emails: - raise InvalidConfiguration( + raise IntegrationSyncTargetNotFound( { "email": "User must have a verified email on Sentry to sync assignee in Jira", "help": "https://sentry.io/settings/account/emails", } ) - raise InvalidConfiguration({"email": "Unable to find the requested user"}) + raise IntegrationSyncTargetNotFound("No matching Jira user found.") try: id_field = client.user_id_field() client.assign_issue(external_issue.key, jira_user and jira_user.get(id_field)) + except ApiUnauthorized as e: + raise IntegrationInstallationConfigurationError( + "Insufficient permissions to assign user to the Jira issue." + ) from e except ApiError as e: # TODO(jess): do we want to email people about these types of failures? logger.info( @@ -1036,7 +1044,7 @@ def sync_assignee_outbound( "issue_key": external_issue.key, }, ) - raise + raise IntegrationError("There was an error assigning the issue.") from e def sync_status_outbound( self, external_issue: ExternalIssue, is_resolved: bool, project_id: int diff --git a/src/sentry/integrations/jira_server/integration.py b/src/sentry/integrations/jira_server/integration.py index 8940ef4ed74072..9727d19b3c25a6 100644 --- a/src/sentry/integrations/jira_server/integration.py +++ b/src/sentry/integrations/jira_server/integration.py @@ -29,7 +29,7 @@ from sentry.integrations.jira.tasks import migrate_issues from sentry.integrations.jira_server.utils.choice import build_user_choice from sentry.integrations.mixins import ResolveSyncAction -from sentry.integrations.mixins.issues import IssueSyncIntegration +from sentry.integrations.mixins.issues import IntegrationSyncTargetNotFound, IssueSyncIntegration from sentry.integrations.models.external_actor import ExternalActor from sentry.integrations.models.external_issue import ExternalIssue from sentry.integrations.models.integration_external_project import IntegrationExternalProject @@ -1268,30 +1268,32 @@ def sync_assignee_outbound( if jira_user is None: # TODO(jess): do we want to email people about these types of failures? logger.info( - "jira.assignee-not-found", + "jira_server.assignee-not-found", extra=logging_context, ) - raise IntegrationError("Failed to assign user to Jira Server issue") + raise IntegrationSyncTargetNotFound("No matching Jira Server user found") try: id_field = client.user_id_field() client.assign_issue(external_issue.key, jira_user and jira_user.get(id_field)) - except ApiUnauthorized: + except ApiUnauthorized as e: logger.info( - "jira.user-assignment-unauthorized", + "jira_server.user-assignment-unauthorized", extra={ **logging_context, }, ) - raise IntegrationError("Insufficient permissions to assign user to Jira Server issue") + raise IntegrationInstallationConfigurationError( + "Insufficient permissions to assign user to Jira Server issue" + ) from e except ApiError as e: logger.info( - "jira.user-assignment-request-error", + "jira_server.user-assignment-request-error", extra={ **logging_context, "error": str(e), }, ) - raise IntegrationError("Failed to assign user to Jira Server issue") + raise IntegrationError("Failed to assign user to Jira Server issue") from e def sync_status_outbound( self, external_issue: ExternalIssue, is_resolved: bool, project_id: int diff --git a/src/sentry/integrations/mixins/issues.py b/src/sentry/integrations/mixins/issues.py index d3e4a908bae325..1a3b4bcef19804 100644 --- a/src/sentry/integrations/mixins/issues.py +++ b/src/sentry/integrations/mixins/issues.py @@ -24,6 +24,7 @@ from sentry.models.grouplink import GroupLink from sentry.models.project import Project from sentry.notifications.utils import get_notification_group_title +from sentry.shared_integrations.exceptions import IntegrationError from sentry.silo.base import all_silo_function from sentry.users.models.user import User from sentry.users.services.user import RpcUser @@ -370,6 +371,10 @@ def update_comment(self, issue_id, user_id, group_note): pass +class IntegrationSyncTargetNotFound(IntegrationError): + pass + + class IssueSyncIntegration(IssueBasicIntegration, ABC): comment_key: ClassVar[str | None] = None outbound_status_key: ClassVar[str | None] = None diff --git a/src/sentry/integrations/tasks/sync_assignee_outbound.py b/src/sentry/integrations/tasks/sync_assignee_outbound.py index c16d51080c7a59..ce912cda2de5e8 100644 --- a/src/sentry/integrations/tasks/sync_assignee_outbound.py +++ b/src/sentry/integrations/tasks/sync_assignee_outbound.py @@ -2,7 +2,6 @@ from sentry import analytics, features from sentry.constants import ObjectStatus -from sentry.exceptions import InvalidConfiguration from sentry.integrations.errors import OrganizationIntegrationNotFound from sentry.integrations.models.external_issue import ExternalIssue from sentry.integrations.models.integration import Integration @@ -13,7 +12,11 @@ from sentry.integrations.services.assignment_source import AssignmentSource from sentry.integrations.services.integration import integration_service from sentry.models.organization import Organization -from sentry.shared_integrations.exceptions import ApiUnauthorized, IntegrationError +from sentry.shared_integrations.exceptions import ( + ApiUnauthorized, + IntegrationError, + IntegrationInstallationConfigurationError, +) from sentry.silo.base import SiloMode from sentry.tasks.base import instrumented_task, retry from sentry.taskworker.config import TaskworkerConfig @@ -53,6 +56,8 @@ def sync_assignee_outbound( assign: bool, assignment_source_dict: dict[str, Any] | None = None, ) -> None: + from sentry.integrations.mixins.issues import IntegrationSyncTargetNotFound + # Sync Sentry assignee to an external issue. external_issue = ExternalIssue.objects.get(id=external_issue_id) @@ -98,5 +103,10 @@ def sync_assignee_outbound( id=integration.id, organization_id=external_issue.organization_id, ) - except (OrganizationIntegrationNotFound, ApiUnauthorized, InvalidConfiguration) as e: + except ( + OrganizationIntegrationNotFound, + ApiUnauthorized, + IntegrationSyncTargetNotFound, + IntegrationInstallationConfigurationError, + ) as e: lifecycle.record_halt(halt_reason=e) diff --git a/src/sentry/integrations/vsts/issues.py b/src/sentry/integrations/vsts/issues.py index acbfca27f30936..a2d71818eefd54 100644 --- a/src/sentry/integrations/vsts/issues.py +++ b/src/sentry/integrations/vsts/issues.py @@ -11,7 +11,7 @@ from sentry.constants import ObjectStatus from sentry.integrations.mixins import ResolveSyncAction -from sentry.integrations.mixins.issues import IssueSyncIntegration +from sentry.integrations.mixins.issues import IntegrationSyncTargetNotFound, IssueSyncIntegration from sentry.integrations.services.integration import integration_service from sentry.integrations.source_code_management.issues import SourceCodeIssueIntegration from sentry.models.activity import Activity @@ -20,6 +20,7 @@ ApiUnauthorized, IntegrationError, IntegrationFormError, + IntegrationInstallationConfigurationError, ) from sentry.silo.base import all_silo_function from sentry.users.models.identity import Identity @@ -294,11 +295,11 @@ def sync_assignee_outbound( "issue_key": external_issue.key, }, ) - return + raise IntegrationSyncTargetNotFound("No matching VSTS user found.") try: client.update_work_item(external_issue.key, assigned_to=assignee) - except (ApiUnauthorized, ApiError): + except (ApiUnauthorized, ApiError) as e: self.logger.info( "vsts.failed-to-assign", extra={ @@ -307,6 +308,11 @@ def sync_assignee_outbound( "issue_key": external_issue.key, }, ) + if isinstance(e, ApiUnauthorized): + raise IntegrationInstallationConfigurationError( + "Insufficient permissions to assign user to the VSTS issue." + ) from e + raise IntegrationError("There was an error assigning the issue.") from e except Exception as e: self.raise_error(e) diff --git a/src/sentry/shared_integrations/exceptions/__init__.py b/src/sentry/shared_integrations/exceptions/__init__.py index 3100d5b038308d..268e3fae377186 100644 --- a/src/sentry/shared_integrations/exceptions/__init__.py +++ b/src/sentry/shared_integrations/exceptions/__init__.py @@ -169,7 +169,7 @@ class IntegrationError(Exception): pass -class IntegrationInstallationConfigurationError(Exception): +class IntegrationInstallationConfigurationError(IntegrationError): """ Error when external API access is blocked due to configuration issues like permissions, visibility changes, or invalid project settings. diff --git a/tests/sentry/integrations/jira/test_integration.py b/tests/sentry/integrations/jira/test_integration.py index 96bcbf70004245..c63e0bd38fb96b 100644 --- a/tests/sentry/integrations/jira/test_integration.py +++ b/tests/sentry/integrations/jira/test_integration.py @@ -9,9 +9,9 @@ from fixtures.integrations.jira.stub_client import StubJiraApiClient from fixtures.integrations.stub_service import StubService -from sentry.exceptions import InvalidConfiguration from sentry.integrations.jira.integration import JiraIntegrationProvider from sentry.integrations.jira.views import SALT +from sentry.integrations.mixins.issues import IntegrationSyncTargetNotFound from sentry.integrations.models.external_issue import ExternalIssue from sentry.integrations.models.integration import Integration from sentry.integrations.models.integration_external_project import IntegrationExternalProject @@ -19,7 +19,10 @@ from sentry.integrations.services.integration import integration_service from sentry.models.grouplink import GroupLink from sentry.models.groupmeta import GroupMeta -from sentry.shared_integrations.exceptions import IntegrationError +from sentry.shared_integrations.exceptions import ( + IntegrationError, + IntegrationInstallationConfigurationError, +) from sentry.silo.base import SiloMode from sentry.testutils.cases import APITestCase, IntegrationTestCase from sentry.testutils.factories import EventType @@ -825,7 +828,7 @@ def test_sync_assignee_outbound_no_email(self): "https://example.atlassian.net/rest/api/2/user/assignable/search", json=[{"accountId": "deadbeef123", "displayName": "Dead Beef"}], ) - with pytest.raises(InvalidConfiguration): + with pytest.raises(IntegrationSyncTargetNotFound): installation.sync_assignee_outbound(external_issue, user) # No sync made as jira users don't have email addresses @@ -866,6 +869,56 @@ def test_sync_assignee_outbound_use_email_api(self): assert assign_issue_response.status_code == 200 assert assign_issue_response.request.body == b'{"accountId": "deadbeef123"}' + @responses.activate + def test_sync_assignee_outbound_api_unauthorized(self): + user = serialize_rpc_user(self.create_user(email="bob@example.com")) + issue_id = "APP-123" + installation = self.integration.get_installation(self.organization.id) + assign_issue_url = "https://example.atlassian.net/rest/api/2/issue/%s/assignee" % issue_id + + external_issue = ExternalIssue.objects.create( + organization_id=self.organization.id, integration_id=installation.model.id, key=issue_id + ) + + responses.add( + responses.GET, + "https://example.atlassian.net/rest/api/2/user/assignable/search", + json=[{"accountId": "deadbeef123", "emailAddress": "bob@example.com"}], + ) + + responses.add(responses.PUT, assign_issue_url, status=401, json={}) + + with pytest.raises(IntegrationInstallationConfigurationError) as excinfo: + installation.sync_assignee_outbound(external_issue, user) + + assert str(excinfo.value) == "Insufficient permissions to assign user to the Jira issue." + assert len(responses.calls) == 2 + + @responses.activate + def test_sync_assignee_outbound_api_error(self): + user = serialize_rpc_user(self.create_user(email="bob@example.com")) + issue_id = "APP-123" + installation = self.integration.get_installation(self.organization.id) + assign_issue_url = "https://example.atlassian.net/rest/api/2/issue/%s/assignee" % issue_id + + external_issue = ExternalIssue.objects.create( + organization_id=self.organization.id, integration_id=installation.model.id, key=issue_id + ) + + responses.add( + responses.GET, + "https://example.atlassian.net/rest/api/2/user/assignable/search", + json=[{"accountId": "deadbeef123", "emailAddress": "bob@example.com"}], + ) + + responses.add(responses.PUT, assign_issue_url, status=400, json={}) + + with pytest.raises(IntegrationError) as excinfo: + installation.sync_assignee_outbound(external_issue, user) + + assert str(excinfo.value) == "There was an error assigning the issue." + assert len(responses.calls) == 2 + @control_silo_test class JiraIntegrationTest(APITestCase): diff --git a/tests/sentry/integrations/jira_server/test_integration.py b/tests/sentry/integrations/jira_server/test_integration.py index c38ddd634e968d..bfa7e23abe69e0 100644 --- a/tests/sentry/integrations/jira_server/test_integration.py +++ b/tests/sentry/integrations/jira_server/test_integration.py @@ -11,6 +11,7 @@ from fixtures.integrations.jira.stub_client import StubJiraApiClient from fixtures.integrations.stub_service import StubService from sentry.integrations.jira_server.integration import JiraServerIntegration +from sentry.integrations.mixins.issues import IntegrationSyncTargetNotFound from sentry.integrations.models.external_actor import ExternalActor from sentry.integrations.models.external_issue import ExternalIssue from sentry.integrations.models.integration_external_project import IntegrationExternalProject @@ -19,7 +20,12 @@ from sentry.integrations.types import ExternalProviders from sentry.models.grouplink import GroupLink from sentry.models.groupmeta import GroupMeta -from sentry.shared_integrations.exceptions import ApiError, ApiUnauthorized, IntegrationError +from sentry.shared_integrations.exceptions import ( + ApiError, + ApiUnauthorized, + IntegrationError, + IntegrationInstallationConfigurationError, +) from sentry.silo.base import SiloMode from sentry.silo.safety import unguarded_write from sentry.testutils.cases import APITestCase @@ -819,9 +825,9 @@ def test_sync_assignee_outbound_no_emails_for_multiple_users(self): ], ) - with pytest.raises(IntegrationError) as e: + with pytest.raises(IntegrationSyncTargetNotFound) as e: self.installation.sync_assignee_outbound(external_issue, user) - assert str(e.value) == "Failed to assign user to Jira Server issue" + assert str(e.value) == "No matching Jira Server user found" # No sync made as jira users don't have email addresses assert len(responses.calls) == 1 @@ -877,7 +883,7 @@ def test_sync_assignee_outbound_unauthorized(self, mock_assign_issue): } ], ) - with pytest.raises(IntegrationError) as exc_info: + with pytest.raises(IntegrationInstallationConfigurationError) as exc_info: self.installation.sync_assignee_outbound( external_issue=external_issue, user=user, assign=True ) @@ -902,7 +908,7 @@ def test_sync_assignee_outbound_api_error(self, mock_assign_issue): { "accountId": "deadbeef123", "displayName": "Dead Beef", - "username": "bob@example.com", + "emailAddress": "bob@example.com", } ], ) diff --git a/tests/sentry/integrations/tasks/test_sync_assignee_outbound.py b/tests/sentry/integrations/tasks/test_sync_assignee_outbound.py index 26f6aba6bfd367..564fbf2b139099 100644 --- a/tests/sentry/integrations/tasks/test_sync_assignee_outbound.py +++ b/tests/sentry/integrations/tasks/test_sync_assignee_outbound.py @@ -4,10 +4,12 @@ from sentry.integrations.errors import OrganizationIntegrationNotFound from sentry.integrations.example import ExampleIntegration +from sentry.integrations.mixins.issues import IntegrationSyncTargetNotFound from sentry.integrations.models import ExternalIssue, Integration from sentry.integrations.models.organization_integration import OrganizationIntegration from sentry.integrations.tasks import sync_assignee_outbound from sentry.integrations.types import EventLifecycleOutcome +from sentry.shared_integrations.exceptions import IntegrationInstallationConfigurationError from sentry.testutils.asserts import assert_halt_metric, assert_success_metric from sentry.testutils.cases import TestCase from sentry.testutils.silo import assume_test_silo_mode_of @@ -35,19 +37,23 @@ def setUp(self): }, ) - @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") - @mock.patch.object(ExampleIntegration, "sync_assignee_outbound") - def test_syncs_outbound_assignee(self, mock_sync_assignee, mock_record_event): - external_issue = self.create_integration_external_issue( + # Create a shared external issue for most tests + self.external_issue = self.create_integration_external_issue( group=self.group, key="foo-1234", integration=self.example_integration, ) - sync_assignee_outbound(external_issue.id, self.user.id, True, None) + # Verify the external issue was created successfully + assert ExternalIssue.objects.filter(id=self.external_issue.id).exists() + + @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + @mock.patch.object(ExampleIntegration, "sync_assignee_outbound") + def test_syncs_outbound_assignee(self, mock_sync_assignee, mock_record_event): + sync_assignee_outbound(self.external_issue.id, self.user.id, True, None) mock_sync_assignee.assert_called_once() mock_sync_assignee.assert_called_with( - external_issue, mock.ANY, assign=True, assignment_source=None + self.external_issue, mock.ANY, assign=True, assignment_source=None ) user_arg = mock_sync_assignee.call_args_list[0][0][1] @@ -61,14 +67,8 @@ def test_syncs_outbound_assignee(self, mock_sync_assignee, mock_record_event): def test_sync_failure(self, mock_sync_assignee, mock_record_failure): mock_sync_assignee.side_effect = raise_sync_assignee_exception - external_issue = self.create_integration_external_issue( - group=self.group, - key="foo-1234", - integration=self.example_integration, - ) - with pytest.raises(Exception) as exc: - sync_assignee_outbound(external_issue.id, self.user.id, True, None) + sync_assignee_outbound(self.external_issue.id, self.user.id, True, None) assert exc.match("Something went wrong") mock_record_failure.assert_called_once() @@ -86,13 +86,8 @@ def test_skips_syncing_if_should_sync_false( self, mock_should_sync, mock_sync_assignee, mock_record_event ): mock_should_sync.return_value = False - external_issue = self.create_integration_external_issue( - group=self.group, - key="foo-1234", - integration=self.example_integration, - ) - sync_assignee_outbound(external_issue.id, self.user.id, True, None) + sync_assignee_outbound(self.external_issue.id, self.user.id, True, None) mock_sync_assignee.assert_not_called() assert mock_record_event.call_count == 2 @@ -120,38 +115,55 @@ def test_missing_issue_sync(self, mock_sync_assignee, mock_record_event): @mock.patch.object(ExampleIntegration, "sync_assignee_outbound") def test_missing_integration_installation(self, mock_sync_assignee): - external_issue = self.create_integration_external_issue( - group=self.group, - key="foo-1234", - integration=self.example_integration, - ) - # Delete all integrations, but ensure we still have an external issue with assume_test_silo_mode_of(Integration): Integration.objects.filter().delete() - assert ExternalIssue.objects.filter(id=external_issue.id).exists() - sync_assignee_outbound(external_issue.id, self.user.id, True, None) + sync_assignee_outbound(self.external_issue.id, self.user.id, True, None) mock_sync_assignee.assert_not_called() @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") @mock.patch.object(ExampleIntegration, "sync_assignee_outbound") def test_missing_organization_integration(self, mock_sync_assignee, mock_record_event): - external_issue = self.create_integration_external_issue( - group=self.group, - key="foo-1234", - integration=self.example_integration, - ) - # Delete all organization integrations, but ensure we still have an external issue with assume_test_silo_mode_of(OrganizationIntegration): OrganizationIntegration.objects.filter().delete() - assert ExternalIssue.objects.filter(id=external_issue.id).exists() - sync_assignee_outbound(external_issue.id, self.user.id, True, None) + sync_assignee_outbound(self.external_issue.id, self.user.id, True, None) mock_sync_assignee.assert_not_called() assert mock_record_event.call_count == 2 assert_halt_metric( mock_record_event, OrganizationIntegrationNotFound("missing org_integration") ) + + @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + @mock.patch.object(ExampleIntegration, "sync_assignee_outbound") + def test_integration_sync_target_not_found(self, mock_sync_assignee, mock_record_event): + mock_sync_assignee.side_effect = IntegrationSyncTargetNotFound("No matching user found") + + sync_assignee_outbound(self.external_issue.id, self.user.id, True, None) + mock_sync_assignee.assert_called_once() + + assert mock_record_event.call_count == 2 + assert_halt_metric( + mock_record_event, IntegrationSyncTargetNotFound("No matching user found") + ) + + @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + @mock.patch.object(ExampleIntegration, "sync_assignee_outbound") + def test_integration_installation_configuration_error( + self, mock_sync_assignee, mock_record_event + ): + mock_sync_assignee.side_effect = IntegrationInstallationConfigurationError( + "Insufficient permissions to assign user" + ) + + sync_assignee_outbound(self.external_issue.id, self.user.id, True, None) + mock_sync_assignee.assert_called_once() + + assert mock_record_event.call_count == 2 + assert_halt_metric( + mock_record_event, + IntegrationInstallationConfigurationError("Insufficient permissions to assign user"), + )