Skip to content
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

chore(sentry apps): Use new error design for token exchange process #83450

Merged
merged 26 commits into from
Jan 28, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
4e5edcd
revise bases errors to use new design
Christinarlong Jan 13, 2025
7eb854c
fix tests
Christinarlong Jan 13, 2025
6799898
correct sentryappsentry errors
Christinarlong Jan 14, 2025
44013d1
code changes for grant excahnger, validator
Christinarlong Jan 14, 2025
54341ef
adjust logging in endpoint
Christinarlong Jan 14, 2025
6f51a8a
code changes for token exchange
Christinarlong Jan 14, 2025
9a566f3
fix grant exchanger tests
Christinarlong Jan 14, 2025
d002803
refresher tests
Christinarlong Jan 14, 2025
eea24cd
fix tests for refresjer
Christinarlong Jan 14, 2025
ae33e83
fix validator tests
Christinarlong Jan 14, 2025
7a7d792
fix authorizations tests
Christinarlong Jan 14, 2025
260020a
update status codes to be more accurate
Christinarlong Jan 14, 2025
fadefc9
fix tests and address PR
Christinarlong Jan 14, 2025
a512641
fix typing issues
Christinarlong Jan 15, 2025
151d828
Merge branch 'crl/new-sa-error-w-fixes' into crl/new-sa-errors-token-…
Christinarlong Jan 15, 2025
90751f4
cmon mypy :C
Christinarlong Jan 15, 2025
c1cded0
break out extras to public and webhook context properties
Christinarlong Jan 15, 2025
6c22173
Merge branch 'crl/new-sa-error-w-fixes' into crl/new-sa-errors-token-…
Christinarlong Jan 15, 2025
7d03094
fix token exchange to use seperated out contexts
Christinarlong Jan 15, 2025
0060e24
make error codes and authorizations consistent
Christinarlong Jan 17, 2025
1dee8e0
update tests & add installation uuid
Christinarlong Jan 18, 2025
07f4f90
fix status codes
Christinarlong Jan 18, 2025
7df4cc2
Merge branch 'master' into crl/new-sa-errors-token-exchange
Christinarlong Jan 21, 2025
3aa129a
make the char limit constant
Christinarlong Jan 24, 2025
12965c5
pr fixes
Christinarlong Jan 24, 2025
83feba5
fix tests to match changs
Christinarlong Jan 27, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions src/sentry/sentry_apps/api/endpoints/sentry_app_authorizations.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import logging

import sentry_sdk
from rest_framework import serializers, status
from rest_framework import serializers
from rest_framework.request import Request
from rest_framework.response import Response

Expand All @@ -10,11 +10,11 @@
from sentry.api.base import control_silo_endpoint
from sentry.api.serializers.models.apitoken import ApiTokenSerializer
from sentry.auth.services.auth.impl import promote_request_api_user
from sentry.coreapi import APIUnauthorized
from sentry.sentry_apps.api.bases.sentryapps import SentryAppAuthorizationsBaseEndpoint
from sentry.sentry_apps.token_exchange.grant_exchanger import GrantExchanger
from sentry.sentry_apps.token_exchange.refresher import Refresher
from sentry.sentry_apps.token_exchange.util import GrantTypes
from sentry.sentry_apps.utils.errors import SentryAppIntegratorError

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -52,7 +52,7 @@ def post(self, request: Request, installation) -> Response:
)

if not auth_serializer.is_valid():
return Response(auth_serializer.errors, status=status.HTTP_400_BAD_REQUEST)
return Response(auth_serializer.errors, status=400)

token = GrantExchanger(
install=installation,
Expand All @@ -64,7 +64,7 @@ def post(self, request: Request, installation) -> Response:
refresh_serializer = SentryAppRefreshAuthorizationSerializer(data=request.data)

if not refresh_serializer.is_valid():
return Response(refresh_serializer.errors, status=status.HTTP_400_BAD_REQUEST)
return Response(refresh_serializer.errors, status=400)

token = Refresher(
install=installation,
Expand All @@ -73,19 +73,19 @@ def post(self, request: Request, installation) -> Response:
user=promote_request_api_user(request),
).run()
else:
return Response({"error": "Invalid grant_type"}, status=403)
except APIUnauthorized as e:
logger.warning(
e,
exc_info=True,
raise SentryAppIntegratorError(message="Invalid grant_type", status_code=403)
except SentryAppIntegratorError as e:
logger.info(
"sentry-app-authorizations.error-context",
exc_info=e,
extra={
"user_id": request.user.id,
"sentry_app_installation_id": installation.id,
"organization_id": installation.organization_id,
"sentry_app_id": installation.sentry_app.id,
},
)
return Response({"error": e.msg or "Unauthorized"}, status=403)
raise

attrs = {"state": request.data.get("state"), "application": None}

Expand Down
31 changes: 23 additions & 8 deletions src/sentry/sentry_apps/token_exchange/grant_exchanger.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,15 @@
from django.utils.functional import cached_property

from sentry import analytics
from sentry.coreapi import APIUnauthorized
from sentry.models.apiapplication import ApiApplication
from sentry.models.apigrant import ApiGrant
from sentry.models.apitoken import ApiToken
from sentry.sentry_apps.models.sentry_app import SentryApp
from sentry.sentry_apps.models.sentry_app_installation import SentryAppInstallation
from sentry.sentry_apps.services.app import RpcSentryAppInstallation
from sentry.sentry_apps.token_exchange.util import token_expiration
from sentry.sentry_apps.token_exchange.util import SENSITIVE_CHARACTER_LIMIT, token_expiration
from sentry.sentry_apps.token_exchange.validator import Validator
from sentry.sentry_apps.utils.errors import SentryAppIntegratorError, SentryAppSentryError
from sentry.silo.safety import unguarded_write
from sentry.users.models.user import User

Expand All @@ -41,7 +41,7 @@ def run(self):
# Once it's exchanged it's no longer valid and should not be
# exchangeable, so we delete it.
self._delete_grant()
except APIUnauthorized:
except SentryAppIntegratorError:
logger.info(
"grant-exchanger.context",
extra={
Expand All @@ -65,10 +65,10 @@ def _validate(self) -> None:
Validator(install=self.install, client_id=self.client_id, user=self.user).run()

if not self._grant_belongs_to_install() or not self._sentry_app_user_owns_grant():
raise APIUnauthorized("Forbidden grant")
raise SentryAppIntegratorError(message="Forbidden grant", status_code=401)

if not self._grant_is_active():
raise APIUnauthorized("Grant has already expired")
raise SentryAppIntegratorError("Grant has already expired", status_code=401)

def _grant_belongs_to_install(self) -> bool:
return self.grant.sentry_app_installation.id == self.install.id
Expand Down Expand Up @@ -108,18 +108,33 @@ def grant(self) -> ApiGrant:
.get(code=self.code)
)
except ApiGrant.DoesNotExist:
raise APIUnauthorized("Could not find grant")
raise SentryAppIntegratorError(
"Could not find grant for given code",
webhook_context={"code": self.code, "installation_uuid": self.install.uuid},
status_code=401,
)

@property
def application(self) -> ApiApplication:
try:
return self.grant.application
except ApiApplication.DoesNotExist:
raise APIUnauthorized("Could not find application")
raise SentryAppSentryError(
"Could not find application from grant",
status_code=401,
webhook_context={
"code": self.code[:SENSITIVE_CHARACTER_LIMIT],
"grant_id": self.grant.id,
},
)

@property
def sentry_app(self) -> SentryApp:
try:
return self.application.sentry_app
except SentryApp.DoesNotExist:
raise APIUnauthorized("Could not find sentry app")
raise SentryAppSentryError(
"Could not find integration from application",
status_code=401,
webhook_context={"application_id": self.application.id},
)
50 changes: 43 additions & 7 deletions src/sentry/sentry_apps/token_exchange/refresher.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,14 @@
from django.utils.functional import cached_property

from sentry import analytics
from sentry.coreapi import APIUnauthorized
from sentry.models.apiapplication import ApiApplication
from sentry.models.apitoken import ApiToken
from sentry.sentry_apps.models.sentry_app import SentryApp
from sentry.sentry_apps.models.sentry_app_installation import SentryAppInstallation
from sentry.sentry_apps.services.app import RpcSentryAppInstallation
from sentry.sentry_apps.token_exchange.util import token_expiration
from sentry.sentry_apps.token_exchange.util import SENSITIVE_CHARACTER_LIMIT, token_expiration
from sentry.sentry_apps.token_exchange.validator import Validator
from sentry.sentry_apps.utils.errors import SentryAppIntegratorError, SentryAppSentryError
from sentry.users.models.user import User

logger = logging.getLogger("sentry.token-exchange")
Expand All @@ -37,7 +37,7 @@ def run(self) -> ApiToken:

self._record_analytics()
return self._create_new_token()
except APIUnauthorized:
except (SentryAppIntegratorError, SentryAppSentryError):
logger.info(
"refresher.context",
extra={
Expand All @@ -58,7 +58,22 @@ def _validate(self) -> None:
Validator(install=self.install, client_id=self.client_id, user=self.user).run()

if self.token.application != self.application:
raise APIUnauthorized("Token does not belong to the application")
assert self.token.application is not None, "Application must exist on ApiToken"
webhook_context = {
"client_id_installation_uuid": self.install.uuid,
"client_id": self.client_id,
}
try:
token_installation = ApiToken.objects.get(
refresh_token=self.refresh_token
).sentry_app_installation
webhook_context.update({"token_installation": token_installation.uuid})
except SentryAppInstallation.DoesNotExist:
pass

raise SentryAppIntegratorError(
message="Token does not belong to the application", webhook_context=webhook_context
)

def _create_new_token(self) -> ApiToken:
token = ApiToken.objects.create(
Expand All @@ -78,18 +93,39 @@ def token(self) -> ApiToken:
try:
return ApiToken.objects.get(refresh_token=self.refresh_token)
except ApiToken.DoesNotExist:
raise APIUnauthorized("Token does not exist")
raise SentryAppIntegratorError(
message="Given refresh token does not exist",
status_code=401,
webhook_context={
"installation_uuid": self.install.uuid,
},
)

@cached_property
def application(self) -> ApiApplication:
try:
return ApiApplication.objects.get(client_id=self.client_id)
except ApiApplication.DoesNotExist:
raise APIUnauthorized("Application does not exist")
raise SentryAppIntegratorError(
message="Could not find matching Application for given client_id",
status_code=401,
webhook_context={
"client_id": self.client_id,
"installation_uuid": self.install.uuid,
},
)

@property
def sentry_app(self) -> SentryApp:
try:
return self.application.sentry_app
except SentryApp.DoesNotExist:
raise APIUnauthorized("Sentry App does not exist")
raise SentryAppSentryError(
message="Sentry App does not exist on attached Application",
status_code=401,
webhook_context={
"application_id": self.application.id,
"installation_uuid": self.install.uuid,
"client_id": self.application.client_id[:SENSITIVE_CHARACTER_LIMIT],
},
)
2 changes: 2 additions & 0 deletions src/sentry/sentry_apps/token_exchange/util.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

TOKEN_LIFE_IN_HOURS = 8

SENSITIVE_CHARACTER_LIMIT = 4

AUTHORIZATION = "authorization_code"
REFRESH = "refresh_token"

Expand Down
34 changes: 27 additions & 7 deletions src/sentry/sentry_apps/token_exchange/validator.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,11 @@

from django.utils.functional import cached_property

from sentry.coreapi import APIUnauthorized
from sentry.models.apiapplication import ApiApplication
from sentry.sentry_apps.models.sentry_app import SentryApp
from sentry.sentry_apps.services.app import RpcSentryAppInstallation
from sentry.sentry_apps.token_exchange.util import SENSITIVE_CHARACTER_LIMIT
from sentry.sentry_apps.utils.errors import SentryAppIntegratorError, SentryAppSentryError
from sentry.users.models.user import User


Expand All @@ -27,28 +28,47 @@ def run(self) -> bool:

def _validate_is_sentry_app_making_request(self) -> None:
if not self.user.is_sentry_app:
raise APIUnauthorized("User is not a Sentry App")
raise SentryAppIntegratorError(
"User is not a Sentry App(custom integration)",
webhook_context={
"user": self.user.name,
},
)

def _validate_app_is_owned_by_user(self) -> None:
if self.sentry_app.proxy_user != self.user:
raise APIUnauthorized("Sentry App does not belong to given user")
raise SentryAppIntegratorError(
"Integration does not belong to given user",
webhook_context={
"user": self.user.name,
"integration": self.sentry_app.slug,
"installation_uuid": self.install.uuid,
},
)

def _validate_installation(self) -> None:
if self.install.sentry_app.id != self.sentry_app.id:
raise APIUnauthorized(
f"Sentry App Installation is not for Sentry App: {self.sentry_app.slug}"
raise SentryAppIntegratorError(
f"Given installation is not for integration: {self.sentry_app.slug}",
webhook_context={"installation_uuid": self.install.uuid},
)

@cached_property
def sentry_app(self) -> SentryApp:
try:
return self.application.sentry_app
except SentryApp.DoesNotExist:
raise APIUnauthorized("Sentry App does not exist")
raise SentryAppSentryError(
"Integration does not exist",
webhook_context={"application_id": self.application.id},
)

@cached_property
def application(self) -> ApiApplication:
try:
return ApiApplication.objects.get(client_id=self.client_id)
except ApiApplication.DoesNotExist:
raise APIUnauthorized("Application does not exist")
raise SentryAppSentryError(
"Application does not exist",
webhook_context={"client_id": self.client_id[:SENSITIVE_CHARACTER_LIMIT]},
)
Original file line number Diff line number Diff line change
Expand Up @@ -100,12 +100,12 @@ def test_non_sentry_app_user(self):
)

def test_invalid_grant(self):
self.get_error_response(code="123", status_code=403)
self.get_error_response(code="123", status_code=401)

def test_expired_grant(self):
self.install.api_grant.update(expires_at=timezone.now() - timedelta(minutes=2))
response = self.get_error_response(status_code=403)
assert response.data["error"] == "Grant has already expired"
response = self.get_error_response(status_code=401)
assert response.data["detail"] == "Grant has already expired"

def test_request_with_exchanged_access_token(self):
response = self.get_response()
Expand Down
Loading
Loading