Browse Source

fix(integration): Gitlab integration updates (#63365)

When setting up GitLab integration, we require a secret key from the
OAuth app on GitLab's side. This secret was used as plain text, leading
to potential leakage, so the input type is changed to secret for better
security.

Secondly, upon successful integration, the general response for the
integration pipeline returns an html page that pops up and closes
itself. To properly redirect to the current integration's page, we allow
the provider to return a redirect url that the integrater can use to
provide a better response.

Lastly, updated the error for group integration to say which group was
incorrect and help the user better understand which input was incorrect.

Old error:
![Screenshot 2024-01-16 at 12 04
38 PM](https://github.com/getsentry/sentry/assets/5581484/607214bc-33bd-4aab-95fa-40f53fffb625)

New Experiences:
I side stepped the actual GitLab installation, as it would not be
feasible in a local setup, so the video here is to show that on a
successful installation, we will redirect to the gitlab integration page
to show the success.


https://github.com/getsentry/sentry/assets/5581484/fb4bc976-d942-47ab-b67c-345bd26271a4



https://github.com/getsentry/sentry/assets/5581484/4b6efbda-6fe0-4da4-a6d4-3534293133ee


Resolves: https://github.com/getsentry/sentry/issues/58382
Yash Kamothi 1 year ago
parent
commit
a05fcdf0aa

+ 13 - 4
src/sentry/integrations/gitlab/integration.py

@@ -265,7 +265,7 @@ class InstallationForm(forms.Form):
     )
     client_secret = forms.CharField(
         label=_("GitLab Application Secret"),
-        widget=forms.TextInput(attrs={"placeholder": _("XXXXXXXXXXXXXXXXXXXXXXXXXXX")}),
+        widget=forms.PasswordInput(attrs={"placeholder": _("***********************")}),
     )
 
     def clean_url(self):
@@ -378,8 +378,10 @@ class GitlabIntegrationProvider(IntegrationProvider):
             access_token=access_token,
             verify_ssl=installation_data["verify_ssl"],
         )
+
+        requested_group = installation_data["group"]
         try:
-            resp = client.get_group(installation_data["group"])
+            resp = client.get_group(requested_group)
             return resp.json
         except ApiError as e:
             self.get_logger().info(
@@ -387,13 +389,15 @@ class GitlabIntegrationProvider(IntegrationProvider):
                 extra={
                     "base_url": installation_data["url"],
                     "verify_ssl": installation_data["verify_ssl"],
-                    "group": installation_data["group"],
+                    "group": requested_group,
                     "include_subgroups": installation_data["include_subgroups"],
                     "error_message": str(e),
                     "error_status": e.code,
                 },
             )
-            raise IntegrationError("The requested GitLab group could not be found.")
+            raise IntegrationError(
+                f"The requested GitLab group {requested_group} could not be found."
+            )
 
     def get_pipeline_views(self):
         return [
@@ -457,6 +461,11 @@ class GitlabIntegrationProvider(IntegrationProvider):
                 "scopes": scopes,
                 "data": oauth_data,
             },
+            "post_install_data": {
+                "redirect_url_format": absolute_uri(
+                    f"/settings/{{org_slug}}/integrations/{self.key}/"
+                ),
+            },
         }
         return integration
 

+ 12 - 0
src/sentry/integrations/pipeline.py

@@ -1,5 +1,7 @@
 from __future__ import annotations
 
+from django.http import HttpResponseRedirect
+
 from sentry import features
 from sentry.api.utils import generate_organization_url
 from sentry.models.organizationmapping import OrganizationMapping
@@ -229,6 +231,12 @@ class IntegrationPipeline(Pipeline):
         org_integration = self.integration.add_organization(
             self.organization, self.request.user, default_auth_id=default_auth_id
         )
+
+        extra = data.get("post_install_data", {})
+        # If a particular provider has a redirect for a successful install, use that instead of the generic success
+        redirect_url_format = extra.get("redirect_url_format", None)
+        if redirect_url_format is not None:
+            return self._get_redirect_response(redirect_url_format=redirect_url_format)
         return self._dialog_success(org_integration)
 
     def _dialog_success(self, org_integration):
@@ -251,3 +259,7 @@ class IntegrationPipeline(Pipeline):
             },
         )
         return render_to_response("sentry/integrations/dialog-complete.html", context, self.request)
+
+    def _get_redirect_response(self, redirect_url_format: str) -> HttpResponseRedirect:
+        redirect_url = redirect_url_format.format(org_slug=self.organization.slug)
+        return HttpResponseRedirect(redirect_url)

+ 16 - 7
tests/sentry/integrations/gitlab/test_integration.py

@@ -108,9 +108,11 @@ class GitlabIntegrationTest(IntegrationTestCase):
         assert req_params["client_id"] == ["client_id"]
         assert req_params["client_secret"] == ["client_secret"]
 
-        assert resp.status_code == 200
-
-        self.assertDialogSuccess(resp)
+        assert resp.status_code == 302
+        assert (
+            resp["Location"]
+            == f"http://testserver/settings/{self.organization.slug}/integrations/gitlab/"
+        )
 
     @responses.activate
     @patch("sentry.integrations.gitlab.integration.sha1_text")
@@ -190,9 +192,13 @@ class GitlabIntegrationTest(IntegrationTestCase):
             "https://gitlab.example.com/oauth/token",
             json={"access_token": "access-token-value"},
         )
+
+        group_that_does_not_exist = "cool-group"
         responses.add(responses.GET, "https://gitlab.example.com/api/v4/user", json={"id": 9})
         responses.add(
-            responses.GET, "https://gitlab.example.com/api/v4/groups/cool-group", status=404
+            responses.GET,
+            f"https://gitlab.example.com/api/v4/groups/{group_that_does_not_exist}",
+            status=404,
         )
         resp = self.client.get(
             "{}?{}".format(
@@ -201,7 +207,7 @@ class GitlabIntegrationTest(IntegrationTestCase):
             )
         )
         assert resp.status_code == 200
-        self.assertContains(resp, "GitLab group could not be found")
+        self.assertContains(resp, f"GitLab group {group_that_does_not_exist} could not be found")
 
     @responses.activate
     def test_get_group_id(self):
@@ -668,9 +674,12 @@ class GitlabIntegrationInstanceTest(IntegrationTestCase):
         assert req_params["client_id"] == ["client_id"]
         assert req_params["client_secret"] == ["client_secret"]
 
-        assert resp.status_code == 200
+        assert resp.status_code == 302
 
-        self.assertDialogSuccess(resp)
+        assert (
+            resp["Location"]
+            == f"http://testserver/settings/{self.organization.slug}/integrations/gitlab/"
+        )
 
     @responses.activate
     @patch("sentry.integrations.gitlab.integration.sha1_text")