-
-
Notifications
You must be signed in to change notification settings - Fork 4.4k
feat(integrations): add endpoint for rotating client secret of Internal/Public integrations #69015
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 4 commits
29d60fb
5ec75fb
2e4aa9c
a8c75ed
c429586
6588418
4310cc2
6c64156
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,62 @@ | ||||||
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.services.hybrid_cloud.organization import organization_service | ||||||
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): | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Something to help us understand our decisions and debug in the future: should we log when we make a decision in this function? i.e.
Or, maybe the exception we raise has a better error message to help both the user and us (for showing in the UI and debugging when users come to us for support) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added logging for now. |
||||||
# organization that owns an integration | ||||||
org_context = organization_service.get_organization_by_id(id=sentry_app.owner_id) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since this is an RPC call, which could have network/side effect errors, would it be worth wrapping in a try/catch, and defaulting to return false for safety? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I checked other uses of |
||||||
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 = ( | ||||||
user_service.get_organizations(user_id=request.user.id) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same question here regarding the RPC try/catch |
||||||
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) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it make more sense to just return false/boolean and have the parent/calling method decide what error code it should use? I think instead of 404, a failed permission is more of a 403? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think maybe even a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because technically a 404 would mean the sentry_app itself doesn't exist, which isn't really true in this case right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically 403 means failed permission, but in this case we don't want to leak info if a specific integration slug exists with different status codes. Take a look from an attacker's perspective:
We're taking a similar approach in other places, for example: sentry/src/sentry/api/bases/sentryapps.py Lines 223 to 224 in 0491f7d
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahhh, I see what you're saying, that makes sense! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I doubt we are, but hope we're that consistent across our other API endpoints in terms of behavior and expected status codes |
||||||
|
||||||
|
||||||
@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: | ||||||
if sentry_app.application is None: | ||||||
return Response(status=404) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we add some more details to the response? |
||||||
|
||||||
new_token = generate_token() | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mdtro I know we're making some changes to this method in another PR, is this still safe/intended to be used in this flow? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ykamo001 Yes, this is okay for now. :) When we come back through to improve the app API tokens we can adjust these calls then. |
||||||
sentry_app.application.update(client_secret=new_token) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should we add a try/catch here and respond accordingly? And also log the error? Because this is a DB transaction, there could be errors |
||||||
return Response(serialize({"clientSecret": new_token})) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cathteng should superusers be allowed to do this for other orgs on their behalf for support cases?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fair. maybe we only let superusers with write privileges do this?