From 127c0fc604c231d5ee97cdea7bc5ff3d395356c6 Mon Sep 17 00:00:00 2001 From: Raj Joshi Date: Fri, 10 Jan 2025 16:52:49 -0800 Subject: [PATCH 1/4] :bug: fix: fix azure devops migration version on a fresh install --- src/sentry/integrations/vsts/integration.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/sentry/integrations/vsts/integration.py b/src/sentry/integrations/vsts/integration.py index 0d679a7b27aedb..9c159f7f6b0a49 100644 --- a/src/sentry/integrations/vsts/integration.py +++ b/src/sentry/integrations/vsts/integration.py @@ -7,7 +7,6 @@ from typing import Any from urllib.parse import parse_qs, quote, urlencode, urlparse -import sentry_sdk from django import forms from django.http import HttpRequest from django.http.response import HttpResponseBase @@ -552,8 +551,9 @@ def build_integration(self, state: Mapping[str, Any]) -> Mapping[str, Any]: sample_rate=1.0, ) + # Assertion error happens when org_integration does not exist + # KeyError happens when subscription is not found except (IntegrationModel.DoesNotExist, AssertionError, KeyError) as e: - sentry_sdk.capture_exception(e) logger.warning( "vsts.build_integration.error", extra={ @@ -574,6 +574,12 @@ def build_integration(self, state: Mapping[str, Any]) -> Mapping[str, Any]: "secret": subscription_secret, } + if isinstance(e, IntegrationModel.DoesNotExist): + # If there is a new integration, we need to set the migration version to 1 + integration["metadata"][ + "integration_migration_version" + ] = VstsIntegrationProvider.CURRENT_MIGRATION_VERSION + return integration def create_subscription( From c18aa4cd08e2421c164427748ec373b9cfa9a936 Mon Sep 17 00:00:00 2001 From: Raj Joshi Date: Sat, 11 Jan 2025 12:36:19 -0800 Subject: [PATCH 2/4] :bug: fix: update tests to test refresh and install --- src/sentry/integrations/vsts/integration.py | 14 +- tests/sentry/integrations/vsts/test_client.py | 39 ++++ .../integrations/vsts/test_integration.py | 191 ++++++++++++++++++ 3 files changed, 238 insertions(+), 6 deletions(-) diff --git a/src/sentry/integrations/vsts/integration.py b/src/sentry/integrations/vsts/integration.py index 9c159f7f6b0a49..5bbd39bf4ac719 100644 --- a/src/sentry/integrations/vsts/integration.py +++ b/src/sentry/integrations/vsts/integration.py @@ -554,6 +554,14 @@ def build_integration(self, state: Mapping[str, Any]) -> Mapping[str, Any]: # Assertion error happens when org_integration does not exist # KeyError happens when subscription is not found except (IntegrationModel.DoesNotExist, AssertionError, KeyError) as e: + if isinstance(e, IntegrationModel.DoesNotExist) and features.has( + "organizations:migrate-azure-devops-integration", self.pipeline.organization + ): + # If there is a new integration, we need to set the migration version to 1 + integration["metadata"][ + "integration_migration_version" + ] = VstsIntegrationProvider.CURRENT_MIGRATION_VERSION + logger.warning( "vsts.build_integration.error", extra={ @@ -574,12 +582,6 @@ def build_integration(self, state: Mapping[str, Any]) -> Mapping[str, Any]: "secret": subscription_secret, } - if isinstance(e, IntegrationModel.DoesNotExist): - # If there is a new integration, we need to set the migration version to 1 - integration["metadata"][ - "integration_migration_version" - ] = VstsIntegrationProvider.CURRENT_MIGRATION_VERSION - return integration def create_subscription( diff --git a/tests/sentry/integrations/vsts/test_client.py b/tests/sentry/integrations/vsts/test_client.py index 571b7a50bfca65..45606cc2fbe3e5 100644 --- a/tests/sentry/integrations/vsts/test_client.py +++ b/tests/sentry/integrations/vsts/test_client.py @@ -19,6 +19,7 @@ from sentry.shared_integrations.exceptions import ApiError from sentry.silo.base import SiloMode from sentry.silo.util import PROXY_BASE_PATH, PROXY_OI_HEADER, PROXY_PATH, PROXY_SIGNATURE_HEADER +from sentry.testutils.helpers import with_feature from sentry.testutils.helpers.integrations import get_installation_of_type from sentry.testutils.silo import assume_test_silo_mode, control_silo_test from sentry.users.models.identity import Identity, IdentityProvider @@ -70,6 +71,44 @@ def test_refreshes_expired_token(self): assert identity.data["refresh_token"] == "new-refresh-token" assert identity.data["expires"] > int(time()) + @with_feature("organizations:migrate-azure-devops-integration") + def test_refreshes_expired_token_new_integration(self): + self.assert_installation(new=True) + integration, installation = self._get_integration_and_install() + + # Make the Identity have an expired token + idp = IdentityProvider.objects.get(external_id=self.vsts_account_id) + identity = Identity.objects.get(idp_id=idp.id) + identity.data["expires"] = int(time()) - int(123456789) + identity.save() + + # New values VSTS will return on refresh + self.access_token = "new-access-token" + self.refresh_token = "new-refresh-token" + self._stub_vsts() + + # Make a request with expired token + installation.get_client().get_projects() + + # Second to last request, before the Projects request, was to refresh + # the Access Token. + assert ( + responses.calls[-2].request.url + == "https://login.microsoftonline.com/common/oauth2/v2.0/token" + ) + + # Then we request the Projects with the new token + assert ( + responses.calls[-1].request.url.split("?")[0] + == f"{self.vsts_base_url.lower()}_apis/projects" + ) + + identity = Identity.objects.get(id=identity.id) + assert set(identity.scopes) == set(VstsIntegrationProvider.NEW_SCOPES) + assert identity.data["access_token"] == "new-access-token" + assert identity.data["refresh_token"] == "new-refresh-token" + assert identity.data["expires"] > int(time()) + @responses.activate def test_does_not_refresh_valid_tokens(self): self.assert_installation() diff --git a/tests/sentry/integrations/vsts/test_integration.py b/tests/sentry/integrations/vsts/test_integration.py index e123748f648bb4..d0a04db5696f4a 100644 --- a/tests/sentry/integrations/vsts/test_integration.py +++ b/tests/sentry/integrations/vsts/test_integration.py @@ -617,6 +617,197 @@ def test_get_repositories_identity_error(self): installation.get_repositories() +@control_silo_test +@with_feature("organizations:migrate-azure-devops-integration") +class NewVstsIntegrationTest(VstsIntegrationTestCase): + def test_get_organization_config(self): + self.assert_installation() + integration, installation = self._get_integration_and_install() + + fields = installation.get_organization_config() + + assert [field["name"] for field in fields] == [ + "sync_status_forward", + "sync_forward_assignment", + "sync_comments", + "sync_status_reverse", + "sync_reverse_assignment", + ] + + def test_get_organization_config_failure(self): + self.assert_installation() + integration, installation = self._get_integration_and_install() + + # Set the `default_identity` property and force token expiration + installation.get_client() + assert installation.default_identity is not None + identity = Identity.objects.get(id=installation.default_identity.id) + identity.data["expires"] = 1566851050 + identity.save() + + responses.replace( + responses.POST, + "https://app.vssps.visualstudio.com/oauth2/token", + status=400, + json={"error": "invalid_grant", "message": "The provided authorization grant failed"}, + ) + fields = installation.get_organization_config() + assert fields[0]["disabled"], "Fields should be disabled" + + def test_update_organization_config_remove_all(self): + self.assert_installation() + + model = Integration.objects.get(provider="vsts") + integration = VstsIntegration(model, self.organization.id) + + org_integration = OrganizationIntegration.objects.get(organization_id=self.organization.id) + + data = {"sync_status_forward": {}, "other_option": "hello"} + IntegrationExternalProject.objects.create( + organization_integration_id=org_integration.id, + external_id=1, + resolved_status="ResolvedStatus1", + unresolved_status="UnresolvedStatus1", + ) + IntegrationExternalProject.objects.create( + organization_integration_id=org_integration.id, + external_id=2, + resolved_status="ResolvedStatus2", + unresolved_status="UnresolvedStatus2", + ) + IntegrationExternalProject.objects.create( + organization_integration_id=org_integration.id, + external_id=3, + resolved_status="ResolvedStatus3", + unresolved_status="UnresolvedStatus3", + ) + + integration.update_organization_config(data) + + external_projects = IntegrationExternalProject.objects.all().values_list( + "external_id", flat=True + ) + + assert list(external_projects) == [] + + config = OrganizationIntegration.objects.get( + organization_id=org_integration.organization_id, + integration_id=org_integration.integration_id, + ).config + + assert config == {"sync_status_forward": False, "other_option": "hello"} + + def test_update_organization_config(self): + self.assert_installation() + + org_integration = OrganizationIntegration.objects.get(organization_id=self.organization.id) + + model = Integration.objects.get(provider="vsts") + integration = VstsIntegration(model, self.organization.id) + + # test validation + data: dict[str, Any] = { + "sync_status_forward": {1: {"on_resolve": "", "on_unresolve": "UnresolvedStatus1"}} + } + with pytest.raises(IntegrationError): + integration.update_organization_config(data) + + data = { + "sync_status_forward": { + 1: {"on_resolve": "ResolvedStatus1", "on_unresolve": "UnresolvedStatus1"}, + 2: {"on_resolve": "ResolvedStatus2", "on_unresolve": "UnresolvedStatus2"}, + 4: {"on_resolve": "ResolvedStatus4", "on_unresolve": "UnresolvedStatus4"}, + }, + "other_option": "hello", + } + IntegrationExternalProject.objects.create( + organization_integration_id=org_integration.id, + external_id=1, + resolved_status="UpdateMe", + unresolved_status="UpdateMe", + ) + IntegrationExternalProject.objects.create( + organization_integration_id=org_integration.id, + external_id=2, + resolved_status="ResolvedStatus2", + unresolved_status="UnresolvedStatus2", + ) + IntegrationExternalProject.objects.create( + organization_integration_id=org_integration.id, + external_id=3, + resolved_status="ResolvedStatus3", + unresolved_status="UnresolvedStatus3", + ) + + integration.update_organization_config(data) + + external_projects = IntegrationExternalProject.objects.all().order_by("external_id") + + assert external_projects[0].external_id == "1" + assert external_projects[0].resolved_status == "ResolvedStatus1" + assert external_projects[0].unresolved_status == "UnresolvedStatus1" + + assert external_projects[1].external_id == "2" + assert external_projects[1].resolved_status == "ResolvedStatus2" + assert external_projects[1].unresolved_status == "UnresolvedStatus2" + + assert external_projects[2].external_id == "4" + assert external_projects[2].resolved_status == "ResolvedStatus4" + assert external_projects[2].unresolved_status == "UnresolvedStatus4" + + config = OrganizationIntegration.objects.get( + organization_id=org_integration.organization_id, + integration_id=org_integration.integration_id, + ).config + + assert config == {"sync_status_forward": True, "other_option": "hello"} + + def test_update_domain_name(self): + account_name = "MyVSTSAccount.visualstudio.com" + account_uri = "https://MyVSTSAccount.visualstudio.com/" + + self.assert_installation() + + model = Integration.objects.get(provider="vsts") + model.metadata["domain_name"] = account_name + model.save() + + integration = VstsIntegration(model, self.organization.id) + integration.get_client() + + domain_name = integration.model.metadata["domain_name"] + assert domain_name == account_uri + assert Integration.objects.get(provider="vsts").metadata["domain_name"] == account_uri + + def test_get_repositories(self): + self.assert_installation() + integration, installation = self._get_integration_and_install() + + result = installation.get_repositories() + assert len(result) == 1 + assert {"name": "ProjectA/cool-service", "identifier": self.repo_id} == result[0] + + def test_get_repositories_identity_error(self): + self.assert_installation() + integration, installation = self._get_integration_and_install() + + # Set the `default_identity` property and force token expiration + installation.get_client() + assert installation.default_identity is not None + identity = Identity.objects.get(id=installation.default_identity.id) + identity.data["expires"] = 1566851050 + identity.save() + + responses.replace( + responses.POST, + "https://app.vssps.visualstudio.com/oauth2/token", + status=400, + json={"error": "invalid_grant", "message": "The provided authorization grant failed"}, + ) + with pytest.raises(IntegrationError): + installation.get_repositories() + + class RegionVstsIntegrationTest(VstsIntegrationTestCase): def setUp(self): super().setUp() From 15908651b606cd4b54e80caf083dc8ddf02e76e8 Mon Sep 17 00:00:00 2001 From: Raj Joshi Date: Sat, 11 Jan 2025 16:20:12 -0800 Subject: [PATCH 3/4] :mag: nit: don't do the error check --- src/sentry/integrations/vsts/integration.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/sentry/integrations/vsts/integration.py b/src/sentry/integrations/vsts/integration.py index 5bbd39bf4ac719..c8341ab0277291 100644 --- a/src/sentry/integrations/vsts/integration.py +++ b/src/sentry/integrations/vsts/integration.py @@ -553,8 +553,8 @@ def build_integration(self, state: Mapping[str, Any]) -> Mapping[str, Any]: # Assertion error happens when org_integration does not exist # KeyError happens when subscription is not found - except (IntegrationModel.DoesNotExist, AssertionError, KeyError) as e: - if isinstance(e, IntegrationModel.DoesNotExist) and features.has( + except (IntegrationModel.DoesNotExist, AssertionError, KeyError): + if features.has( "organizations:migrate-azure-devops-integration", self.pipeline.organization ): # If there is a new integration, we need to set the migration version to 1 From 6162b829865b56cca93044ffdf4e4749267779af Mon Sep 17 00:00:00 2001 From: Raj Joshi Date: Mon, 13 Jan 2025 13:19:26 -0800 Subject: [PATCH 4/4] :recycle: ref: add test to verify metadata is correct --- tests/sentry/integrations/vsts/test_client.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/sentry/integrations/vsts/test_client.py b/tests/sentry/integrations/vsts/test_client.py index 45606cc2fbe3e5..abad7d5bdc9355 100644 --- a/tests/sentry/integrations/vsts/test_client.py +++ b/tests/sentry/integrations/vsts/test_client.py @@ -164,6 +164,17 @@ def request_callback(request): projects = installation.get_client().get_projects() assert len(projects) == 220 + @with_feature("organizations:migrate-azure-devops-integration") + def test_metadata_is_correct(self): + self.assert_installation(new=True) + integration, installation = self._get_integration_and_install() + assert integration.metadata["domain_name"] == "https://MyVSTSAccount.visualstudio.com/" + assert set(integration.metadata["scopes"]) == set(VstsIntegrationProvider.NEW_SCOPES) + assert ( + integration.metadata["integration_migration_version"] + == VstsIntegrationProvider.CURRENT_MIGRATION_VERSION + ) + @responses.activate def test_simple(self): responses.add(