Browse Source

fix(integrations): allow access to the Client Secret only to Manager and Owner roles (#69566)

Since https://github.com/getsentry/sentry/pull/25458 (3 years ago), only
Managers and Owner could create integrations. It was also not possible
for Members/Admins to see integration details on the frontend till
recently because of the bug described in
https://github.com/getsentry/sentry/pull/68971

As we're limiting integration creation to Managers and Owners only, and
same to the API tokens, it would be natural to also limit the access to
the Client Secret only to those roles.
Alexander Tarasov 10 months ago
parent
commit
965c340bea

+ 13 - 2
src/sentry/api/serializers/models/sentry_app.py

@@ -14,6 +14,7 @@ from sentry.models.avatars.sentry_app_avatar import SentryAppAvatar
 from sentry.models.integrations.integration_feature import IntegrationFeature, IntegrationTypes
 from sentry.models.integrations.sentry_app import MASKED_VALUE, SentryApp
 from sentry.models.user import User
+from sentry.services.hybrid_cloud.organization import organization_service
 from sentry.services.hybrid_cloud.organization_mapping import organization_mapping_service
 from sentry.services.hybrid_cloud.user.service import user_service
 
@@ -95,9 +96,19 @@ class SentryAppSerializer(Serializer):
             )
             if elevated_user or owner.id in user_org_ids:
                 is_secret_visible = obj.date_added > timezone.now() - timedelta(days=1)
-                client_secret = (
-                    obj.application.client_secret if obj.show_auth_info(access) else MASKED_VALUE
+
+                owner_context = organization_service.get_organization_by_id(
+                    id=owner.id, user_id=user.id
                 )
+
+                client_secret = MASKED_VALUE
+                if elevated_user or (
+                    owner_context
+                    and "org:write" in owner_context.member.scopes
+                    and obj.show_auth_info(owner_context.member)
+                ):
+                    client_secret = obj.application.client_secret
+
                 data.update(
                     {
                         "clientId": obj.application.client_id,

+ 1 - 1
static/app/views/settings/organizationDeveloperSettings/permissionsObserver.tsx

@@ -92,7 +92,7 @@ export default class PermissionsObserver extends Component<Props, State> {
       return (
         <Alert type="warning" showIcon>
           {t(
-            'You are going to increase privileges for this integration. Organization members who already had access to the Client Secret may gain extra permissions due to this change. If this is not what you are expecting, consider re-creating the integration.'
+            'You are going to increase privileges for this integration. Organization members who already had access to the Client Secret may gain extra permissions due to this change. If this is not what you are expecting, consider rotating the Client Secret below.'
           )}
         </Alert>
       );

+ 1 - 1
static/app/views/settings/organizationDeveloperSettings/sentryApplicationDetails.tsx

@@ -530,7 +530,7 @@ class SentryApplicationDetails extends DeprecatedAsyncView<Props, State> {
                         position="right"
                         containerDisplayMode="inline"
                         title={t(
-                          'You do not have access to view these credentials because the permissions for this integration exceed those of your role.'
+                          'Only Manager or Owner can view these credentials, or the permissions for this integration exceed those of your role.'
                         )}
                       >
                         <TextCopyInput id={id}>

+ 29 - 6
tests/sentry/api/endpoints/test_sentry_apps.py

@@ -73,6 +73,7 @@ class SentryAppsTest(APITestCase):
         organization: Organization,
         has_features: bool = False,
         mask_secret: bool = False,
+        scopes: list[str] | None = None,
     ) -> None:
         assert sentry_app.application is not None
         data = {
@@ -90,7 +91,7 @@ class SentryAppsTest(APITestCase):
             "popularity": self.default_popularity,
             "redirectUrl": sentry_app.redirect_url,
             "schema": {},
-            "scopes": [],
+            "scopes": scopes if scopes else [],
             "slug": sentry_app.slug,
             "status": sentry_app.get_status_display(),
             "uuid": sentry_app.uuid,
@@ -100,7 +101,6 @@ class SentryAppsTest(APITestCase):
         }
 
         if mask_secret:
-            data["scopes"] = ["project:write"]
             data["clientSecret"] = MASKED_VALUE
 
         if has_features:
@@ -285,14 +285,36 @@ class GetSentryAppsTest(SentryAppsTest):
         assert self.unpublished_app not in response_uuids
         assert self.unowned_unpublished_app.uuid not in response_uuids
 
+    def test_client_secret_is_not_masked(self):
+        manager_user = self.create_user(email="bleep@example.com")
+        self.create_member(organization=self.organization, user=manager_user, role="manager")
+
+        # Create an app with the same permission that what the manager role has.
+        sentry_app = self.create_sentry_app(
+            name="Boo Far", organization=self.organization, scopes=("org:write",)
+        )
+
+        self.login_as(manager_user)
+        response = self.get_success_response(qs_params={"status": "unpublished"}, status_code=200)
+        self.assert_response_has_serialized_sentry_app(
+            response=response,
+            sentry_app=sentry_app,
+            organization=self.organization,
+            has_features=True,
+            mask_secret=False,
+            scopes=["org:write"],
+        )
+
     def test_client_secret_is_masked(self):
-        user = self.create_user(email="bloop@example.com")
-        self.create_member(organization=self.organization, user=user)
-        # Create an app with higher permissions that what the member role has.
+        manager_user = self.create_user(email="bloop@example.com")
+        self.create_member(organization=self.organization, user=manager_user, role="manager")
+
+        # Create an app with higher permissions that what the manager role has.
         sentry_app = self.create_sentry_app(
-            name="Boo Far", organization=self.organization, scopes=("project:write",)
+            name="Boo Far", organization=self.organization, scopes=("org:admin",)
         )
 
+        self.login_as(manager_user)
         response = self.get_success_response(qs_params={"status": "unpublished"}, status_code=200)
         self.assert_response_has_serialized_sentry_app(
             response=response,
@@ -300,6 +322,7 @@ class GetSentryAppsTest(SentryAppsTest):
             organization=self.organization,
             has_features=True,
             mask_secret=True,
+            scopes=["org:admin"],
         )
 
     def test_users_dont_see_unpublished_apps_their_org_owns(self):