From 29d60fb9e13fa0535b16d2bb7e1bcf8f22dda7c4 Mon Sep 17 00:00:00 2001 From: Alexander Tarasov Date: Tue, 16 Apr 2024 19:38:18 +0200 Subject: [PATCH 1/8] feat(integrations): add endpoint for rotating sentry app clientSecret --- .../integrations/sentry_apps/__init__.py | 2 + .../integrations/sentry_apps/rotate_secret.py | 53 +++++++++++++++++++ src/sentry/api/urls.py | 6 +++ 3 files changed, 61 insertions(+) create mode 100644 src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py diff --git a/src/sentry/api/endpoints/integrations/sentry_apps/__init__.py b/src/sentry/api/endpoints/integrations/sentry_apps/__init__.py index 3824a620983bf5..140934cc533b6e 100644 --- a/src/sentry/api/endpoints/integrations/sentry_apps/__init__.py +++ b/src/sentry/api/endpoints/integrations/sentry_apps/__init__.py @@ -15,6 +15,7 @@ from .organization_sentry_apps import OrganizationSentryAppsEndpoint from .publish_request import SentryAppPublishRequestEndpoint from .requests import SentryAppRequestsEndpoint +from .rotate_secret import SentryAppRotateSecretEndpoint from .stats.details import SentryAppStatsEndpoint from .stats.index import SentryAppsStatsEndpoint @@ -34,6 +35,7 @@ "SentryAppInteractionEndpoint", "SentryAppPublishRequestEndpoint", "SentryAppRequestsEndpoint", + "SentryAppRotateSecretEndpoint", "SentryAppsEndpoint", "SentryAppsStatsEndpoint", "SentryAppStatsEndpoint", diff --git a/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py b/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py new file mode 100644 index 00000000000000..5e7de512c11bf3 --- /dev/null +++ b/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py @@ -0,0 +1,53 @@ +from django.http import Http404 +from rest_framework.request import Request +from rest_framework.response import Response + +from sentry.api.api_owners import ApiOwner +from sentry.api.api_publish_status import ApiPublishStatus +from sentry.api.base import control_silo_endpoint +from sentry.api.bases.sentryapps import SentryAppBaseEndpoint +from sentry.api.permissions import SentryPermission +from sentry.api.serializers import serialize +from sentry.models.apiapplication import generate_token +from sentry.models.integrations.sentry_app import SentryApp +from sentry.models.organization import Organization +from sentry.services.hybrid_cloud.user.service import user_service + + +class SentryAppRotateSecretPermission(SentryPermission): + scope_map = { + "POST": ["org:write", "org:admin"], + } + + def has_object_permission(self, request: Request, view: object, sentry_app: SentryApp): + # organization that owns an integration + organization = Organization.objects.get(id=sentry_app.owner_id) + + # if user is not a member of an organization owning an integration, + # return 404 to avoid leaking integration slug + organizations = ( + user_service.get_organizations(user_id=request.user.id) + if request.user.id is not None + else () + ) + if not any(organization.id == org.id for org in organizations): + raise Http404 + + # permission check inside an organization + self.determine_access(request, organization) + allowed_scopes = set(self.scope_map.get(request.method or "", [])) + return any(request.access.has_scope(s) for s in allowed_scopes) + + +@control_silo_endpoint +class SentryAppRotateSecretEndpoint(SentryAppBaseEndpoint): + publish_status = { + "POST": ApiPublishStatus.PRIVATE, + } + owner = ApiOwner.ENTERPRISE + permission_classes = (SentryAppRotateSecretPermission,) + + def post(self, request: Request, sentry_app: SentryApp) -> Response: + new_token = generate_token() + sentry_app.application.update(client_secret=new_token) + return Response(serialize({"clientSecret": new_token})) diff --git a/src/sentry/api/urls.py b/src/sentry/api/urls.py index 5f8aaee055612e..170962b4c222cb 100644 --- a/src/sentry/api/urls.py +++ b/src/sentry/api/urls.py @@ -282,6 +282,7 @@ SentryAppInteractionEndpoint, SentryAppPublishRequestEndpoint, SentryAppRequestsEndpoint, + SentryAppRotateSecretEndpoint, SentryAppsEndpoint, SentryAppsStatsEndpoint, SentryAppStatsEndpoint, @@ -2838,6 +2839,11 @@ def create_group_urls(name_prefix: str) -> list[URLPattern | URLResolver]: SentryInternalAppTokenDetailsEndpoint.as_view(), name="sentry-api-0-sentry-internal-app-token-details", ), + re_path( + r"^(?P[^\/]+)/rotate-secret/$", + SentryAppRotateSecretEndpoint.as_view(), + name="sentry-api-0-sentry-app-rotate-secret", + ), re_path( r"^(?P[^\/]+)/stats/$", SentryAppStatsEndpoint.as_view(), From 5ec75fb373a170eeeba6501a2925a7ca46272e99 Mon Sep 17 00:00:00 2001 From: Alexander Tarasov Date: Tue, 16 Apr 2024 20:15:25 +0200 Subject: [PATCH 2/8] add tests and fix HC case --- .../integrations/sentry_apps/rotate_secret.py | 4 +- .../test_sentry_app_rotate_secret.py | 62 +++++++++++++++++++ 2 files changed, 64 insertions(+), 2 deletions(-) create mode 100644 tests/sentry/api/endpoints/test_sentry_app_rotate_secret.py diff --git a/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py b/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py index 5e7de512c11bf3..9fb9be8a9c46aa 100644 --- a/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py +++ b/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py @@ -10,7 +10,7 @@ from sentry.api.serializers import serialize from sentry.models.apiapplication import generate_token from sentry.models.integrations.sentry_app import SentryApp -from sentry.models.organization import Organization +from sentry.services.hybrid_cloud.organization import organization_service from sentry.services.hybrid_cloud.user.service import user_service @@ -21,7 +21,7 @@ class SentryAppRotateSecretPermission(SentryPermission): def has_object_permission(self, request: Request, view: object, sentry_app: SentryApp): # organization that owns an integration - organization = Organization.objects.get(id=sentry_app.owner_id) + organization = organization_service.get_org_by_id(id=sentry_app.owner_id) # if user is not a member of an organization owning an integration, # return 404 to avoid leaking integration slug diff --git a/tests/sentry/api/endpoints/test_sentry_app_rotate_secret.py b/tests/sentry/api/endpoints/test_sentry_app_rotate_secret.py new file mode 100644 index 00000000000000..653338596a273b --- /dev/null +++ b/tests/sentry/api/endpoints/test_sentry_app_rotate_secret.py @@ -0,0 +1,62 @@ +from django.urls import reverse + +from sentry.models.apiapplication import ApiApplication +from sentry.models.integrations.sentry_app import SentryApp +from sentry.testutils.cases import APITestCase +from sentry.testutils.silo import control_silo_test + + +@control_silo_test +class SentryAppRotateSecretTest(APITestCase): + def setUp(self): + self.application = ApiApplication.objects.create(owner=self.user) + self.sentry_app = SentryApp.objects.create( + application=self.application, owner_id=self.organization.id, name="a", slug="a" + ) + self.url = reverse("sentry-api-0-sentry-app-rotate-secret", args=[self.sentry_app.slug]) + + def test_unauthenticated_call(self): + response = self.client.post(self.url) + assert response.status_code == 401 + + def test_member_call(self): + """ + Tests that a low privileged user from the same org cannot rotate a secret. + """ + other_user = self.create_user() + other_member = self.create_member( + user=other_user, organization=self.organization, role="member" + ) + self.login_as(other_member) + response = self.client.post(self.url) + assert response.status_code == 403 + + def test_non_owner_call(self): + """ + Tests that an authenticated user cannot rotate the secret for an app from other org. + """ + self.login_as(self.user) + other_user = self.create_user() + other_org = self.create_organization(owner=other_user) + other_app = ApiApplication.objects.create(owner=other_user, name="b") + other_sentry_app = SentryApp.objects.create( + application=other_app, owner_id=other_org.id, name="b", slug="b" + ) + response = self.client.post( + reverse("sentry-api-0-sentry-app-rotate-secret", args=[other_sentry_app.slug]) + ) + assert response.status_code == 404 + + def test_invalid_app_id(self): + self.login_as(self.user) + path_with_invalid_id = reverse("sentry-api-0-sentry-app-rotate-secret", args=["invalid"]) + response = self.client.post(path_with_invalid_id) + assert response.status_code == 404 + + def test_valid_call(self): + self.login_as(self.user) + old_secret = self.sentry_app.application.client_secret + response = self.client.post(self.url) + new_secret = response.data["clientSecret"] + assert len(new_secret) == len(old_secret) + assert new_secret != old_secret From 2e4aa9cb3124cd17276529b7f6b2b61e84d135e1 Mon Sep 17 00:00:00 2001 From: Alexander Tarasov Date: Tue, 16 Apr 2024 21:17:08 +0200 Subject: [PATCH 3/8] add new control silo URL pattern --- static/app/data/controlsiloUrlPatterns.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/static/app/data/controlsiloUrlPatterns.ts b/static/app/data/controlsiloUrlPatterns.ts index dfb13769963a5f..eea5908b6963ed 100644 --- a/static/app/data/controlsiloUrlPatterns.ts +++ b/static/app/data/controlsiloUrlPatterns.ts @@ -94,6 +94,7 @@ const patterns: RegExp[] = [ new RegExp('^api/0/sentry-apps/[^/]+/avatar/$'), new RegExp('^api/0/sentry-apps/[^/]+/api-tokens/$'), new RegExp('^api/0/sentry-apps/[^/]+/api-tokens/[^/]+/$'), + new RegExp('^api/0/sentry-apps/[^/]+/rotate-secret/$'), new RegExp('^api/0/sentry-apps/[^/]+/stats/$'), new RegExp('^api/0/sentry-apps/[^/]+/publish-request/$'), new RegExp('^api/0/sentry-app-installations/[^/]+/$'), From a8c75ed7cb8ddcd1049b7712dfe9e9087c2ba06b Mon Sep 17 00:00:00 2001 From: Alexander Tarasov Date: Tue, 16 Apr 2024 21:32:45 +0200 Subject: [PATCH 4/8] fix typing issues --- .../integrations/sentry_apps/rotate_secret.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py b/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py index 9fb9be8a9c46aa..cd3b0e21951474 100644 --- a/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py +++ b/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py @@ -21,7 +21,13 @@ class SentryAppRotateSecretPermission(SentryPermission): def has_object_permission(self, request: Request, view: object, sentry_app: SentryApp): # organization that owns an integration - organization = organization_service.get_org_by_id(id=sentry_app.owner_id) + org_context = organization_service.get_organization_by_id(id=sentry_app.owner_id) + if org_context is None: + raise Http404 + + organization = org_context.organization + if organization is None: + raise Http404 # if user is not a member of an organization owning an integration, # return 404 to avoid leaking integration slug @@ -48,6 +54,9 @@ class SentryAppRotateSecretEndpoint(SentryAppBaseEndpoint): permission_classes = (SentryAppRotateSecretPermission,) def post(self, request: Request, sentry_app: SentryApp) -> Response: + if sentry_app.application is None: + return Response(status=404) + new_token = generate_token() sentry_app.application.update(client_secret=new_token) return Response(serialize({"clientSecret": new_token})) From c42958644e6f81bb174d39477004beb81d4955a7 Mon Sep 17 00:00:00 2001 From: Alexander Tarasov Date: Thu, 18 Apr 2024 10:25:02 +0200 Subject: [PATCH 5/8] cleanup --- .../integrations/sentry_apps/rotate_secret.py | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py b/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py index cd3b0e21951474..e783b55dce53fd 100644 --- a/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py +++ b/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py @@ -21,14 +21,12 @@ class SentryAppRotateSecretPermission(SentryPermission): def has_object_permission(self, request: Request, view: object, sentry_app: SentryApp): # organization that owns an integration - org_context = organization_service.get_organization_by_id(id=sentry_app.owner_id) + org_context = organization_service.get_organization_by_id( + id=sentry_app.owner_id, user_id=request.user.id if request.user else None + ) if org_context is None: raise Http404 - organization = org_context.organization - if organization is None: - raise Http404 - # if user is not a member of an organization owning an integration, # return 404 to avoid leaking integration slug organizations = ( @@ -36,11 +34,11 @@ def has_object_permission(self, request: Request, view: object, sentry_app: Sent if request.user.id is not None else () ) - if not any(organization.id == org.id for org in organizations): + if not any(sentry_app.owner_id == org.id for org in organizations): raise Http404 # permission check inside an organization - self.determine_access(request, organization) + self.determine_access(request, org_context) allowed_scopes = set(self.scope_map.get(request.method or "", [])) return any(request.access.has_scope(s) for s in allowed_scopes) From 658841826dce632e4e8cede2211f2b1ef13113a1 Mon Sep 17 00:00:00 2001 From: Alexander Tarasov Date: Thu, 18 Apr 2024 10:42:15 +0200 Subject: [PATCH 6/8] allow superuser access --- .../endpoints/integrations/sentry_apps/rotate_secret.py | 7 ++++++- .../api/endpoints/test_sentry_app_rotate_secret.py | 9 +++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py b/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py index e783b55dce53fd..d27a88e2df2302 100644 --- a/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py +++ b/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py @@ -8,6 +8,7 @@ from sentry.api.bases.sentryapps import SentryAppBaseEndpoint from sentry.api.permissions import SentryPermission from sentry.api.serializers import serialize +from sentry.auth.superuser import superuser_has_permission from sentry.models.apiapplication import generate_token from sentry.models.integrations.sentry_app import SentryApp from sentry.services.hybrid_cloud.organization import organization_service @@ -27,6 +28,11 @@ def has_object_permission(self, request: Request, view: object, sentry_app: Sent if org_context is None: raise Http404 + self.determine_access(request, org_context) + + if superuser_has_permission(request): + return True + # if user is not a member of an organization owning an integration, # return 404 to avoid leaking integration slug organizations = ( @@ -38,7 +44,6 @@ def has_object_permission(self, request: Request, view: object, sentry_app: Sent raise Http404 # permission check inside an organization - self.determine_access(request, org_context) allowed_scopes = set(self.scope_map.get(request.method or "", [])) return any(request.access.has_scope(s) for s in allowed_scopes) diff --git a/tests/sentry/api/endpoints/test_sentry_app_rotate_secret.py b/tests/sentry/api/endpoints/test_sentry_app_rotate_secret.py index 653338596a273b..7dc309b7a69068 100644 --- a/tests/sentry/api/endpoints/test_sentry_app_rotate_secret.py +++ b/tests/sentry/api/endpoints/test_sentry_app_rotate_secret.py @@ -60,3 +60,12 @@ def test_valid_call(self): new_secret = response.data["clientSecret"] assert len(new_secret) == len(old_secret) assert new_secret != old_secret + + def test_superuser_has_access(self): + superuser = self.create_user(is_superuser=True) + self.login_as(user=superuser, superuser=True) + old_secret = self.sentry_app.application.client_secret + response = self.client.post(self.url) + new_secret = response.data["clientSecret"] + assert len(new_secret) == len(old_secret) + assert new_secret != old_secret From 4310cc22abae7aa27b9e4c7d88892c762da8dc4c Mon Sep 17 00:00:00 2001 From: Alexander Tarasov Date: Thu, 18 Apr 2024 10:51:57 +0200 Subject: [PATCH 7/8] more details in case of malformed sentry app --- .../integrations/sentry_apps/rotate_secret.py | 2 +- .../api/endpoints/test_sentry_app_rotate_secret.py | 11 +++++++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py b/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py index d27a88e2df2302..ed5e3f9b118937 100644 --- a/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py +++ b/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py @@ -58,7 +58,7 @@ class SentryAppRotateSecretEndpoint(SentryAppBaseEndpoint): def post(self, request: Request, sentry_app: SentryApp) -> Response: if sentry_app.application is None: - return Response(status=404) + return Response({"detail": "Corresponding application was not found."}, status=404) new_token = generate_token() sentry_app.application.update(client_secret=new_token) diff --git a/tests/sentry/api/endpoints/test_sentry_app_rotate_secret.py b/tests/sentry/api/endpoints/test_sentry_app_rotate_secret.py index 7dc309b7a69068..27257c9b8fd292 100644 --- a/tests/sentry/api/endpoints/test_sentry_app_rotate_secret.py +++ b/tests/sentry/api/endpoints/test_sentry_app_rotate_secret.py @@ -69,3 +69,14 @@ def test_superuser_has_access(self): new_secret = response.data["clientSecret"] assert len(new_secret) == len(old_secret) assert new_secret != old_secret + + def test_no_corresponding_application_found(self): + self.login_as(self.user) + other_sentry_app = SentryApp.objects.create( + application=None, owner_id=self.organization.id, name="c", slug="c" + ) + response = self.client.post( + reverse("sentry-api-0-sentry-app-rotate-secret", args=[other_sentry_app.slug]) + ) + assert response.status_code == 404 + assert "Corresponding application was not found." in response.data["detail"] From 6c64156dad211e2458daf9273d26bec0663b04f3 Mon Sep 17 00:00:00 2001 From: Alexander Tarasov Date: Thu, 18 Apr 2024 11:16:35 +0200 Subject: [PATCH 8/8] add logging to has_object_permission --- .../integrations/sentry_apps/rotate_secret.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py b/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py index ed5e3f9b118937..d3a0f081440f4f 100644 --- a/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py +++ b/src/sentry/api/endpoints/integrations/sentry_apps/rotate_secret.py @@ -1,3 +1,5 @@ +import logging + from django.http import Http404 from rest_framework.request import Request from rest_framework.response import Response @@ -14,6 +16,8 @@ from sentry.services.hybrid_cloud.organization import organization_service from sentry.services.hybrid_cloud.user.service import user_service +logger = logging.getLogger(__name__) + class SentryAppRotateSecretPermission(SentryPermission): scope_map = { @@ -21,11 +25,18 @@ class SentryAppRotateSecretPermission(SentryPermission): } def has_object_permission(self, request: Request, view: object, sentry_app: SentryApp): + log_info = { + "user_id": request.user.id, + "sentry_app_name": sentry_app.name, + "organization_id": sentry_app.owner_id, + } + # organization that owns an integration org_context = organization_service.get_organization_by_id( id=sentry_app.owner_id, user_id=request.user.id if request.user else None ) if org_context is None: + logger.warning("owner organization for a sentry app was not found", extra=log_info) raise Http404 self.determine_access(request, org_context) @@ -41,6 +52,9 @@ def has_object_permission(self, request: Request, view: object, sentry_app: Sent else () ) if not any(sentry_app.owner_id == org.id for org in organizations): + logger.info( + "user does not belong to the integration owner organization", extra=log_info + ) raise Http404 # permission check inside an organization