diff --git a/src/sentry/integrations/gitlab/issue_sync.py b/src/sentry/integrations/gitlab/issue_sync.py index 2ffb5f14770737..5ed12bd4d3115a 100644 --- a/src/sentry/integrations/gitlab/issue_sync.py +++ b/src/sentry/integrations/gitlab/issue_sync.py @@ -7,7 +7,11 @@ from sentry import features from sentry.integrations.gitlab.types import GitLabIssueAction, GitLabIssueStatus -from sentry.integrations.mixins.issues import IssueSyncIntegration, ResolveSyncAction +from sentry.integrations.mixins.issues import ( + IntegrationSyncTargetNotFound, + IssueSyncIntegration, + ResolveSyncAction, +) 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 @@ -20,6 +24,11 @@ logger = logging.getLogger("sentry.integrations.gitlab.issue_sync") +def _add_lifecycle_extras(lifecycle: Any, extras: Mapping[str, Any]) -> None: + if lifecycle is not None and hasattr(lifecycle, "add_extras"): + lifecycle.add_extras(extras) + + class GitlabIssueSyncSpec(IssueSyncIntegration): comment_key = "sync_comments" outbound_assignee_key = "sync_forward_assignment" @@ -70,20 +79,24 @@ def sync_assignee_outbound( If assign=True, we're assigning the issue. Otherwise, deassign. """ client = self.get_client() + lifecycle = kwargs.get("lifecycle") project_id, issue_id = self.split_external_issue_key(external_issue.key) + log_context: dict[str, Any] = { + "provider": self.model.provider, + "integration_id": external_issue.integration_id, + "external_issue_key": external_issue.key, + "external_issue_id": external_issue.id, + "project_path": project_id, + "issue_iid": issue_id, + "assign": assign, + "user_id": user.id if user else None, + } + _add_lifecycle_extras(lifecycle, log_context) if not project_id or not issue_id: - logger.error( - "assignee-outbound.invalid-key", - extra={ - "provider": self.model.provider, - "integration_id": external_issue.integration_id, - "external_issue_key": external_issue.key, - "external_issue_id": external_issue.id, - }, - ) - return + logger.warning("assignee-outbound.invalid-key", extra=log_context) + raise IntegrationSyncTargetNotFound("Invalid GitLab issue key") gitlab_user_id = None @@ -98,57 +111,78 @@ def sync_assignee_outbound( organization=external_issue.organization, ).first() if not external_actor: - logger.info( - "assignee-outbound.external-actor-not-found", - extra={ - "provider": self.model.provider, - "integration_id": external_issue.integration_id, - "user_id": user.id, - }, + logger.info("assignee-outbound.external-actor-not-found", extra=log_context) + raise IntegrationSyncTargetNotFound("No matching GitLab external actor found") + + log_context.update( + { + "external_actor_id": external_actor.id, + "external_actor_name": external_actor.external_name, + "external_actor_external_id": external_actor.external_id, + } + ) + _add_lifecycle_extras(lifecycle, log_context) + + if external_actor.external_id: + try: + gitlab_user_id = int(external_actor.external_id) + except ValueError: + logger.warning("assignee-outbound.invalid-external-id", extra=log_context) + raise IntegrationSyncTargetNotFound("Invalid GitLab external actor ID") + + log_context.update( + { + "assignee_resolution_method": "external_id", + "resolved_gitlab_user_id": gitlab_user_id, + } ) - return + logger.info("assignee-outbound.gitlab-user-found", extra=log_context) + _add_lifecycle_extras(lifecycle, log_context) # Strip the @ from the username stored in external_name - gitlab_username = external_actor.external_name.lstrip("@") - - # Search for the GitLab user by username to get their user ID - try: - users = client.search_users(gitlab_username) - if not users or len(users) == 0: - logger.warning( - "assignee-outbound.gitlab-user-not-found", - extra={ - "provider": self.model.provider, - "integration_id": external_issue.integration_id, - "gitlab_username": gitlab_username, - }, - ) - return - - # Take the first matching user (exact username match) - gitlab_user = users[0] - gitlab_user_id = gitlab_user["id"] - - logger.info( - "assignee-outbound.gitlab-user-found", - extra={ - "provider": self.model.provider, - "integration_id": external_issue.integration_id, - "gitlab_username": gitlab_username, - "gitlab_user_id": gitlab_user_id, - }, + if gitlab_user_id is None: + gitlab_username = external_actor.external_name.lstrip("@") + + # Search for the GitLab user by username to get their user ID + try: + users = client.search_users(gitlab_username) or [] + except ApiError as e: + log_context["error"] = str(e) + logger.warning("assignee-outbound.gitlab-user-search-failed", extra=log_context) + _add_lifecycle_extras(lifecycle, log_context) + raise IntegrationSyncTargetNotFound("GitLab user search failed") + + usernames = [ + found_user.get("username") + for found_user in users[:5] + if found_user.get("username") + ] + log_context.update( + { + "assignee_resolution_method": "username_search", + "gitlab_search_result_count": len(users), + "gitlab_search_usernames": usernames, + } ) - except ApiError as e: - logger.warning( - "assignee-outbound.gitlab-user-search-failed", - extra={ - "provider": self.model.provider, - "integration_id": external_issue.integration_id, - "gitlab_username": gitlab_username, - "error": str(e), - }, + + gitlab_user = next( + ( + found_user + for found_user in users + if found_user.get("username", "").lower() == gitlab_username.lower() + ), + None, ) - return + if gitlab_user is None: + logger.warning("assignee-outbound.gitlab-user-not-found", extra=log_context) + _add_lifecycle_extras(lifecycle, log_context) + raise IntegrationSyncTargetNotFound("No matching GitLab user found") + + gitlab_user_id = gitlab_user["id"] + log_context["resolved_gitlab_user_id"] = gitlab_user_id + + logger.info("assignee-outbound.gitlab-user-found", extra=log_context) + _add_lifecycle_extras(lifecycle, log_context) # Update GitLab issue assignees try: diff --git a/src/sentry/integrations/gitlab/webhooks.py b/src/sentry/integrations/gitlab/webhooks.py index a2c69efc3d0d8a..b58822f8e675bb 100644 --- a/src/sentry/integrations/gitlab/webhooks.py +++ b/src/sentry/integrations/gitlab/webhooks.py @@ -192,6 +192,7 @@ def _handle_assignment( # GitLab supports multiple assignees, but Sentry currently only supports one # Take the first assignee from the current state first_assignee = assignees[0] + assignee_id = first_assignee.get("id") assignee_username = first_assignee.get("username") if not assignee_username: @@ -212,6 +213,7 @@ def _handle_assignment( external_user_name=assignee_name, external_issue_key=external_issue_key, assign=True, + external_user_id=assignee_id, ) logger.info( @@ -219,6 +221,7 @@ def _handle_assignment( extra={ "integration_id": integration.id, "external_issue_key": external_issue_key, + "assignee_id": assignee_id, "assignee_name": assignee_name, "total_assignees": len(assignees), }, diff --git a/src/sentry/integrations/tasks/sync_assignee_outbound.py b/src/sentry/integrations/tasks/sync_assignee_outbound.py index 053bace711ecba..7e10ebe9efe4ef 100644 --- a/src/sentry/integrations/tasks/sync_assignee_outbound.py +++ b/src/sentry/integrations/tasks/sync_assignee_outbound.py @@ -88,7 +88,11 @@ def sync_assignee_outbound( # Assume unassign if None. user = user_service.get_user(user_id) if user_id else None installation.sync_assignee_outbound( - external_issue, user, assign=assign, assignment_source=parsed_assignment_source + external_issue, + user, + assign=assign, + assignment_source=parsed_assignment_source, + lifecycle=lifecycle, ) analytics.record( IntegrationIssueAssigneeSyncedEvent( diff --git a/src/sentry/integrations/utils/sync.py b/src/sentry/integrations/utils/sync.py index 2855825f10125c..abb0f5f5b0ee8f 100644 --- a/src/sentry/integrations/utils/sync.py +++ b/src/sentry/integrations/utils/sync.py @@ -1,6 +1,7 @@ from __future__ import annotations import logging +from collections.abc import Sequence from enum import StrEnum from typing import TYPE_CHECKING @@ -74,8 +75,9 @@ def _get_affected_groups( def _handle_deassign( - groups: QuerySet[Group], integration: RpcIntegration | Integration -) -> QuerySet[Group]: + groups: Sequence[Group], integration: RpcIntegration | Integration +) -> list[Group]: + groups_deassigned: list[Group] = [] for group in groups: if not should_sync_assignee_inbound(group.organization, integration.provider): continue @@ -84,11 +86,12 @@ def _handle_deassign( group, assignment_source=AssignmentSource.from_integration(integration), ) - return groups + groups_deassigned.append(group) + return groups_deassigned def _handle_assign( - affected_groups: QuerySet[Group], + affected_groups: Sequence[Group], integration: RpcIntegration | Integration, users: list[RpcUser], ) -> list[Group]: @@ -137,45 +140,69 @@ def sync_group_assignee_inbound_by_external_actor( external_user_name: str, external_issue_key: str | None, assign: bool = True, + external_user_id: str | int | None = None, ) -> QuerySet[Group] | list[Group]: logger = logging.getLogger(f"sentry.integrations.{integration.provider}") with ProjectManagementEvent( action_type=ProjectManagementActionType.INBOUND_ASSIGNMENT_SYNC, integration=integration ).capture() as lifecycle: - affected_groups = _get_affected_groups(integration, external_issue_key) + affected_groups = list(_get_affected_groups(integration, external_issue_key)) + external_user_id_str = str(external_user_id) if external_user_id is not None else None log_context = { "integration_id": integration.id, "external_user_name": external_user_name, + "external_user_id": external_user_id_str, "issue_key": external_issue_key, "method": AssigneeInboundSyncMethod.EXTERNAL_ACTOR.value, "assign": assign, + "affected_group_ids": [group.id for group in affected_groups], } + lifecycle.add_extras(log_context) if not affected_groups: logger.info("no-affected-groups", extra=log_context) return [] if not assign: - return _handle_deassign(affected_groups, integration) + groups_deassigned = _handle_deassign(affected_groups, integration) + log_context["unassigned_group_ids"] = [group.id for group in groups_deassigned] + lifecycle.add_extras(log_context) + return groups_deassigned - external_actors = ExternalActor.objects.filter( + base_external_actors = ExternalActor.objects.filter( provider=EXTERNAL_PROVIDERS_REVERSE[ExternalProviderEnum(integration.provider)].value, - external_name__iexact=external_user_name, integration_id=integration.id, user_id__isnull=False, - ).values_list("user_id", flat=True) + ) + + match_method = "external_name" + external_actors = base_external_actors.filter(external_name__iexact=external_user_name) + if external_user_id_str is not None: + external_actors_by_id = base_external_actors.filter(external_id=external_user_id_str) + if external_actors_by_id.exists(): + external_actors = external_actors_by_id + match_method = "external_id" + external_actor_user_ids = list(external_actors.values_list("user_id", flat=True)) user_ids = [ - external_actor for external_actor in external_actors if external_actor is not None + external_actor_user_id + for external_actor_user_id in external_actor_user_ids + if external_actor_user_id is not None ] + log_context["match_method"] = match_method + log_context["external_actor_count"] = len(user_ids) + log_context["matched_user_ids"] = user_ids log_context["user_ids"] = user_ids logger.info("sync_group_assignee_inbound_by_external_actor.user_ids", extra=log_context) + lifecycle.add_extras(log_context) users = user_service.get_many_by_id(ids=user_ids) groups_assigned = _handle_assign(affected_groups, integration, users) + log_context["assigned_group_ids"] = [group.id for group in groups_assigned] + lifecycle.add_extras(log_context) if len(groups_assigned) != len(affected_groups): log_context["groups_assigned_count"] = len(groups_assigned) diff --git a/tests/sentry/integrations/gitlab/test_integration.py b/tests/sentry/integrations/gitlab/test_integration.py index d9290a180bf94d..63883d1fa4e25e 100644 --- a/tests/sentry/integrations/gitlab/test_integration.py +++ b/tests/sentry/integrations/gitlab/test_integration.py @@ -14,6 +14,7 @@ from sentry.integrations.gitlab.client import GitLabApiClient, GitLabSetupApiClient from sentry.integrations.gitlab.constants import GITLAB_WEBHOOK_VERSION, GITLAB_WEBHOOK_VERSION_KEY from sentry.integrations.gitlab.integration import GitlabIntegration, GitlabIntegrationProvider +from sentry.integrations.mixins.issues import IntegrationSyncTargetNotFound from sentry.integrations.models.integration import Integration from sentry.integrations.models.integration_external_project import IntegrationExternalProject from sentry.integrations.models.organization_integration import OrganizationIntegration @@ -178,7 +179,7 @@ def _setup_assignee_sync_test( self, user_email: str = "foo@example.com", external_name: str = "@gitlab_user", - external_id: str = "123", + external_id: str | None = None, issue_key: str = "example.gitlab.com/group-x:cool-group/sentry#45", create_external_user: bool = True, ) -> tuple: @@ -298,6 +299,32 @@ def test_update_organization_config_preserves_existing(self) -> None: assert org_integration.config["sync_reverse_assignment"] is True assert org_integration.config["new_key"] == "new_value" + @responses.activate + @with_feature("organizations:integrations-gitlab-project-management") + def test_sync_assignee_outbound_uses_external_id(self) -> None: + """Test assigning a GitLab issue using the mapped GitLab user ID.""" + user, installation, external_issue, _, _ = self._setup_assignee_sync_test(external_id="123") + + responses.add( + responses.PUT, + "https://example.gitlab.com/api/v4/projects/cool-group%2Fsentry/issues/45", + json={"assignees": [{"id": 123}]}, + status=200, + ) + + responses.calls.reset() + + with assume_test_silo_mode(SiloMode.CELL): + installation.sync_assignee_outbound(external_issue, user, assign=True) + + assert len(responses.calls) == 1 + request = responses.calls[0].request + assert ( + "https://example.gitlab.com/api/v4/projects/cool-group%2Fsentry/issues/45" + == request.url + ) + assert orjson.loads(request.body) == {"assignee_ids": [123]} + @responses.activate @with_feature("organizations:integrations-gitlab-project-management") def test_sync_assignee_outbound(self) -> None: @@ -336,6 +363,38 @@ def test_sync_assignee_outbound(self) -> None: ) assert orjson.loads(request.body) == {"assignee_ids": [123]} + @responses.activate + @with_feature("organizations:integrations-gitlab-project-management") + def test_sync_assignee_outbound_requires_exact_username_match(self) -> None: + user, installation, external_issue, _, _ = self._setup_assignee_sync_test( + external_name="@first.last" + ) + + responses.add( + responses.GET, + "https://example.gitlab.com/api/v4/users?username=first.last", + json=[ + {"id": 999, "username": "first", "name": "Wrong User"}, + {"id": 123, "username": "first.last", "name": "GitLab User"}, + ], + status=200, + ) + responses.add( + responses.PUT, + "https://example.gitlab.com/api/v4/projects/cool-group%2Fsentry/issues/45", + json={"assignees": [{"id": 123}]}, + status=200, + ) + + responses.calls.reset() + + with assume_test_silo_mode(SiloMode.CELL): + installation.sync_assignee_outbound(external_issue, user, assign=True) + + assert len(responses.calls) == 2 + request = responses.calls[1].request + assert orjson.loads(request.body) == {"assignee_ids": [123]} + @responses.activate @with_feature("organizations:integrations-gitlab-project-management") def test_sync_assignee_outbound_strips_at_symbol(self) -> None: @@ -406,7 +465,8 @@ def test_sync_assignee_outbound_no_external_actor(self) -> None: responses.calls.reset() with assume_test_silo_mode(SiloMode.CELL): - installation.sync_assignee_outbound(external_issue, user, assign=True) + with pytest.raises(IntegrationSyncTargetNotFound): + installation.sync_assignee_outbound(external_issue, user, assign=True) # Should not make any API calls assert len(responses.calls) == 0 @@ -422,7 +482,8 @@ def test_sync_assignee_outbound_invalid_key_format(self) -> None: responses.calls.reset() with assume_test_silo_mode(SiloMode.CELL): - installation.sync_assignee_outbound(external_issue, user, assign=True) + with pytest.raises(IntegrationSyncTargetNotFound): + installation.sync_assignee_outbound(external_issue, user, assign=True) # Should not make any API calls assert len(responses.calls) == 0 @@ -483,12 +544,34 @@ def test_sync_assignee_outbound_user_not_found(self) -> None: responses.calls.reset() with assume_test_silo_mode(SiloMode.CELL): - installation.sync_assignee_outbound(external_issue, user, assign=True) + with pytest.raises(IntegrationSyncTargetNotFound): + installation.sync_assignee_outbound(external_issue, user, assign=True) # Should only call user search, not issue update assert len(responses.calls) == 1 assert "users?username=gitlab_user" in responses.calls[0].request.url + @responses.activate + @with_feature("organizations:integrations-gitlab-project-management") + def test_sync_assignee_outbound_user_search_failed(self) -> None: + user, installation, external_issue, _, _ = self._setup_assignee_sync_test() + + responses.add( + responses.GET, + "https://example.gitlab.com/api/v4/users?username=gitlab_user", + json={"message": "Internal server error"}, + status=500, + ) + + responses.calls.reset() + + with assume_test_silo_mode(SiloMode.CELL): + with pytest.raises(IntegrationSyncTargetNotFound): + installation.sync_assignee_outbound(external_issue, user, assign=True) + + assert len(responses.calls) == 1 + assert "users?username=gitlab_user" in responses.calls[0].request.url + def test_create_comment(self) -> None: """Test creating a comment on a GitLab issue""" self.user.name = "Sentry Admin" diff --git a/tests/sentry/integrations/gitlab/test_webhook.py b/tests/sentry/integrations/gitlab/test_webhook.py index 993d8759f1d3b5..0419212ce10923 100644 --- a/tests/sentry/integrations/gitlab/test_webhook.py +++ b/tests/sentry/integrations/gitlab/test_webhook.py @@ -473,6 +473,31 @@ def test_issue_assigned(self, mock_sync: MagicMock) -> None: call_args[1]["external_issue_key"] == "example.gitlab.com/group-x:cool-group/sentry#23" ) assert call_args[1]["assign"] is True + assert call_args[1]["external_user_id"] == 1 + + @patch("sentry.integrations.gitlab.webhooks.sync_group_assignee_inbound_by_external_actor") + def test_issue_assigned_with_dotted_username(self, mock_sync: MagicMock) -> None: + event = orjson.loads(ISSUE_ASSIGNED_EVENT) + event["assignees"][0]["id"] = 123 + event["assignees"][0]["username"] = "first.last" + + response = self.client.post( + self.url, + data=orjson.dumps(event), + content_type="application/json", + HTTP_X_GITLAB_TOKEN=WEBHOOK_TOKEN, + HTTP_X_GITLAB_EVENT="Issue Hook", + ) + assert response.status_code == 204 + + assert mock_sync.called + call_args = mock_sync.call_args + assert call_args[1]["external_user_name"] == "@first.last" + assert call_args[1]["external_user_id"] == 123 + assert ( + call_args[1]["external_issue_key"] == "example.gitlab.com/group-x:cool-group/sentry#23" + ) + assert call_args[1]["assign"] is True @patch("sentry.integrations.gitlab.webhooks.sync_group_assignee_inbound_by_external_actor") def test_issue_unassigned(self, mock_sync: MagicMock) -> None: diff --git a/tests/sentry/integrations/tasks/test_sync_assignee_outbound.py b/tests/sentry/integrations/tasks/test_sync_assignee_outbound.py index fbebce25b26158..960e8979566a39 100644 --- a/tests/sentry/integrations/tasks/test_sync_assignee_outbound.py +++ b/tests/sentry/integrations/tasks/test_sync_assignee_outbound.py @@ -56,7 +56,11 @@ def test_syncs_outbound_assignee( sync_assignee_outbound(self.external_issue.id, self.user.id, True, None) mock_sync_assignee.assert_called_once() mock_sync_assignee.assert_called_with( - self.external_issue, mock.ANY, assign=True, assignment_source=None + self.external_issue, + mock.ANY, + assign=True, + assignment_source=None, + lifecycle=mock.ANY, ) user_arg = mock_sync_assignee.call_args_list[0][0][1] diff --git a/tests/sentry/integrations/utils/test_sync.py b/tests/sentry/integrations/utils/test_sync.py index d8af02f4908d04..66a658930796e8 100644 --- a/tests/sentry/integrations/utils/test_sync.py +++ b/tests/sentry/integrations/utils/test_sync.py @@ -378,6 +378,59 @@ def test_assignment_with_external_actor_case_insensitive( assert updated_assignee.email == "test@example.com" mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None, False, None) + @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") + def test_assignment_with_external_actor_external_id( + self, + mock_record_event: mock.MagicMock, + ) -> None: + """Test assigning a group to a user via external actor ID before username.""" + gitlab_integration = self.create_integration( + organization=self.organization, + external_id="gitlab:org", + provider="gitlab", + oi_params={ + "config": { + "sync_comments": True, + "sync_status_forward": True, + "sync_status_reverse": True, + "sync_forward_assignment": True, + "sync_reverse_assignment": True, + } + }, + ) + external_issue = self.create_integration_external_issue( + group=self.group, + key="example.gitlab.com/group-x:cool-group/sentry#23", + integration=gitlab_integration, + ) + + self.create_external_user( + user=self.test_user, + external_name="@different.username", + external_id="123", + provider=ExternalProviders.GITLAB.value, + integration=gitlab_integration, + ) + + with self.feature( + [ + "organizations:integrations-issue-sync", + "organizations:integrations-gitlab-project-management", + ] + ): + sync_group_assignee_inbound_by_external_actor( + integration=gitlab_integration, + external_user_name="@first.last", + external_user_id=123, + external_issue_key=external_issue.key, + assign=True, + ) + + updated_assignee = self.group.get_assignee() + assert updated_assignee is not None + assert updated_assignee.id == self.test_user.id + mock_record_event.assert_called_with(EventLifecycleOutcome.SUCCESS, None, False, None) + @mock.patch("sentry.integrations.utils.sync.where_should_sync") @mock.patch("sentry.integrations.utils.metrics.EventLifecycle.record_event") def test_assign_with_multiple_groups( @@ -465,10 +518,16 @@ def test_assign_with_no_external_actor_found(self, mock_record_halt: mock.MagicM extra={ "integration_id": self.example_integration.id, "external_user_name": "unknownuser", + "external_user_id": None, "issue_key": external_issue.key, "method": AssigneeInboundSyncMethod.EXTERNAL_ACTOR.value, "assign": True, + "affected_group_ids": [self.group.id], + "match_method": "external_name", + "external_actor_count": 0, + "matched_user_ids": [], "user_ids": [], + "assigned_group_ids": [], "groups_assigned_count": 0, "affected_groups_count": 1, }, @@ -510,10 +569,16 @@ def test_assign_with_user_not_in_project(self, mock_record_halt: mock.MagicMock) extra={ "integration_id": self.example_integration.id, "external_user_name": "outsider", + "external_user_id": None, "issue_key": external_issue.key, "method": AssigneeInboundSyncMethod.EXTERNAL_ACTOR.value, "assign": True, + "affected_group_ids": [self.group.id], + "match_method": "external_name", + "external_actor_count": 1, + "matched_user_ids": [other_user.id], "user_ids": [other_user.id], + "assigned_group_ids": [], "groups_assigned_count": 0, "affected_groups_count": 1, },