From 29df9345e06730b90bf03d632fa3000f08742c50 Mon Sep 17 00:00:00 2001 From: Eric Hasegawa Date: Sat, 8 Jul 2023 14:10:16 -0700 Subject: [PATCH 1/6] feat(auth): Add /userinfo endpoint for OIDC --- src/sentry/api/endpoints/oauth_userinfo.py | 44 +++++++++ src/sentry/api/urls.py | 6 ++ .../api/endpoints/test_oauth_userinfo.py | 96 +++++++++++++++++++ 3 files changed, 146 insertions(+) create mode 100644 src/sentry/api/endpoints/oauth_userinfo.py create mode 100644 tests/sentry/api/endpoints/test_oauth_userinfo.py diff --git a/src/sentry/api/endpoints/oauth_userinfo.py b/src/sentry/api/endpoints/oauth_userinfo.py new file mode 100644 index 00000000000000..ee3da770ee4b7f --- /dev/null +++ b/src/sentry/api/endpoints/oauth_userinfo.py @@ -0,0 +1,44 @@ +from rest_framework.authentication import get_authorization_header +from rest_framework.request import Request +from rest_framework.response import Response + +from sentry.api.base import Endpoint, control_silo_endpoint +from sentry.models import ApiToken, UserEmail + + +@control_silo_endpoint +class OAuthUserInfoEndpoint(Endpoint): + authentication_classes = () + permission_classes = () + + def get(self, request: Request) -> Response: + try: + access_token = get_authorization_header(request).split()[1].decode("utf-8") + except IndexError: + return Response("No access token found.", status=401) + try: + token_details = ApiToken.objects.get(token=access_token) + except ApiToken.DoesNotExist: + return Response(status=400) + + scopes = token_details.get_scopes() + if "openid" not in scopes: + return Response(status=403) + + user = token_details.user + user_output = {"sub": user.id} + if "profile" in scopes: + user_output["name"] = user.name + user_output["username"]: user.username + user_output["avatar_type"] = user.avatar_type + user_output["avatar_url"] = user.avatar_url + user_output["date_joined"] = user.date_joined + if "email" in scopes: + try: + email = UserEmail.objects.get(user=user) + user_output["email"] = email.email + user_output["email_verified"] = email.is_verified + except UserEmail.DoesNotExist: + user_output["email"] = None + user_output["email_verified"] = None + return Response(user_output) diff --git a/src/sentry/api/urls.py b/src/sentry/api/urls.py index 33c9b3b2d60e07..1b1bd8f441326a 100644 --- a/src/sentry/api/urls.py +++ b/src/sentry/api/urls.py @@ -233,6 +233,7 @@ NotificationActionsDetailsEndpoint, NotificationActionsIndexEndpoint, ) +from .endpoints.oauth_userinfo import OAuthUserInfoEndpoint from .endpoints.organization_access_request_details import OrganizationAccessRequestDetailsEndpoint from .endpoints.organization_activity import OrganizationActivityEndpoint from .endpoints.organization_api_key_details import OrganizationApiKeyDetailsEndpoint @@ -674,6 +675,11 @@ AuthLoginEndpoint.as_view(), name="sentry-api-0-auth-login", ), + re_path( + r"^oauth/userinfo/$", + OAuthUserInfoEndpoint.as_view(), + name="sentry-api-0-oauth-userinfo", + ), ] BROADCAST_URLS = [ diff --git a/tests/sentry/api/endpoints/test_oauth_userinfo.py b/tests/sentry/api/endpoints/test_oauth_userinfo.py new file mode 100644 index 00000000000000..105bc708a348e9 --- /dev/null +++ b/tests/sentry/api/endpoints/test_oauth_userinfo.py @@ -0,0 +1,96 @@ +import datetime + +from django.urls import reverse +from rest_framework.test import APIClient + +from sentry.models import ApiToken +from sentry.testutils import APITestCase +from sentry.testutils.silo import control_silo_test + + +@control_silo_test(stable=True) +class OAuthUserInfoTest(APITestCase): + def setUp(self): + super().setUp() + self.login_as(self.user) + self.path = reverse( + "sentry-api-0-oauth-userinfo", + ) + self.client = APIClient() + + def test_requires_access_token(self): + response = self.client.get(self.path) + + assert response.status_code == 401 + assert response.data == "No access token found." + + def test_declines_if_no_openid_scope(self): + token_without_openid_scope = ApiToken.objects.create(user=self.user, scope_list=[]) + self.client.credentials(HTTP_AUTHORIZATION="Bearer " + token_without_openid_scope.token) + + response = self.client.get(self.path) + + assert response.status_code == 403 + + def test_gets_sub_with_openid_scope(self): + """ + Ensures we get `sub`, and only `sub`, if the only scope is openid. + """ + openid_only_token = ApiToken.objects.create(user=self.user, scope_list=["openid"]) + + self.client.credentials(HTTP_AUTHORIZATION="Bearer " + openid_only_token.token) + + response = self.client.get(self.path) + + assert response.status_code == 200 + assert response.data == {"sub": self.user.id} + + def test_gets_email_information(self): + email_token = ApiToken.objects.create(user=self.user, scope_list=["openid", "email"]) + self.client.credentials(HTTP_AUTHORIZATION="Bearer " + email_token.token) + + response = self.client.get(self.path) + + assert response.status_code == 200 + assert response.data == { + "sub": self.user.id, + "email": self.user.email, + "email_verified": True, + } + + def test_gets_profile_information(self): + profile_token = ApiToken.objects.create(user=self.user, scope_list=["openid", "profile"]) + self.client.credentials(HTTP_AUTHORIZATION="Bearer " + profile_token.token) + + response = self.client.get(self.path) + + assert response.status_code == 200 + + assert response.data["avatar_type"] == 0 + assert response.data["avatar_url"] is None + assert isinstance(response.data["date_joined"], datetime.datetime) + assert response.data["name"] == "" + assert response.data["sub"] == self.user.id + + def test_gets_multiple_scopes(self): + all_access_token = ApiToken.objects.create( + user=self.user, scope_list=["openid", "profile", "email", "address", "phone"] + ) + self.client.credentials(HTTP_AUTHORIZATION="Bearer " + all_access_token.token) + + response = self.client.get(self.path) + + assert response.status_code == 200 + + # profile information + assert response.data["avatar_type"] == 0 + assert response.data["avatar_url"] is None + assert isinstance(response.data["date_joined"], datetime.datetime) + assert response.data["name"] == "" + + # email information + assert response.data["email"] == self.user.email + assert response.data["email_verified"] + + # openid information + assert response.data["sub"] == self.user.id From 3f85abd5beaff532224cef39ec6d49c4df97bec9 Mon Sep 17 00:00:00 2001 From: Eric Hasegawa Date: Sat, 8 Jul 2023 14:18:05 -0700 Subject: [PATCH 2/6] fixing type issue --- src/sentry/api/endpoints/oauth_userinfo.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/sentry/api/endpoints/oauth_userinfo.py b/src/sentry/api/endpoints/oauth_userinfo.py index ee3da770ee4b7f..c266793add50c5 100644 --- a/src/sentry/api/endpoints/oauth_userinfo.py +++ b/src/sentry/api/endpoints/oauth_userinfo.py @@ -29,7 +29,6 @@ def get(self, request: Request) -> Response: user_output = {"sub": user.id} if "profile" in scopes: user_output["name"] = user.name - user_output["username"]: user.username user_output["avatar_type"] = user.avatar_type user_output["avatar_url"] = user.avatar_url user_output["date_joined"] = user.date_joined From 1ab841ad2566a3dd3fc881f17a327a3c0e4f9419 Mon Sep 17 00:00:00 2001 From: Eric Hasegawa Date: Sat, 8 Jul 2023 16:50:13 -0700 Subject: [PATCH 3/6] Adding test case --- src/sentry/api/endpoints/oauth_userinfo.py | 10 +++------- tests/sentry/api/endpoints/test_oauth_userinfo.py | 5 +++++ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/src/sentry/api/endpoints/oauth_userinfo.py b/src/sentry/api/endpoints/oauth_userinfo.py index c266793add50c5..c8f644c52aa701 100644 --- a/src/sentry/api/endpoints/oauth_userinfo.py +++ b/src/sentry/api/endpoints/oauth_userinfo.py @@ -33,11 +33,7 @@ def get(self, request: Request) -> Response: user_output["avatar_url"] = user.avatar_url user_output["date_joined"] = user.date_joined if "email" in scopes: - try: - email = UserEmail.objects.get(user=user) - user_output["email"] = email.email - user_output["email_verified"] = email.is_verified - except UserEmail.DoesNotExist: - user_output["email"] = None - user_output["email_verified"] = None + email = UserEmail.objects.get(user=user) + user_output["email"] = email.email + user_output["email_verified"] = email.is_verified return Response(user_output) diff --git a/tests/sentry/api/endpoints/test_oauth_userinfo.py b/tests/sentry/api/endpoints/test_oauth_userinfo.py index 105bc708a348e9..7ffb6af45add5d 100644 --- a/tests/sentry/api/endpoints/test_oauth_userinfo.py +++ b/tests/sentry/api/endpoints/test_oauth_userinfo.py @@ -24,6 +24,11 @@ def test_requires_access_token(self): assert response.status_code == 401 assert response.data == "No access token found." + def test_declines_invalid_token(self): + self.client.credentials(HTTP_AUTHORIZATION="Bearer abcd") + response = self.client.get(self.path) + assert response.status_code == 400 + def test_declines_if_no_openid_scope(self): token_without_openid_scope = ApiToken.objects.create(user=self.user, scope_list=[]) self.client.credentials(HTTP_AUTHORIZATION="Bearer " + token_without_openid_scope.token) From da08a3ca5b1488dd73878037c585ff04c0be3a5c Mon Sep 17 00:00:00 2001 From: Eric Hasegawa Date: Mon, 10 Jul 2023 14:47:39 -0700 Subject: [PATCH 4/6] Minor refactors --- src/sentry/api/endpoints/oauth_userinfo.py | 15 +++++++++------ src/sentry/api/urls.py | 11 ++++++----- 2 files changed, 15 insertions(+), 11 deletions(-) diff --git a/src/sentry/api/endpoints/oauth_userinfo.py b/src/sentry/api/endpoints/oauth_userinfo.py index c8f644c52aa701..bb7740450e699d 100644 --- a/src/sentry/api/endpoints/oauth_userinfo.py +++ b/src/sentry/api/endpoints/oauth_userinfo.py @@ -28,12 +28,15 @@ def get(self, request: Request) -> Response: user = token_details.user user_output = {"sub": user.id} if "profile" in scopes: - user_output["name"] = user.name - user_output["avatar_type"] = user.avatar_type - user_output["avatar_url"] = user.avatar_url - user_output["date_joined"] = user.date_joined + profile_details = { + "name": user.name, + "avatar_type": user.avatar_type, + "avatar_url": user.avatar_url, + "date_joined": user.date_joined, + } + user_output.update(profile_details) if "email" in scopes: email = UserEmail.objects.get(user=user) - user_output["email"] = email.email - user_output["email_verified"] = email.is_verified + email_details = {"email": email.email, "email_verified": email.is_verified} + user_output.update(email_details) return Response(user_output) diff --git a/src/sentry/api/urls.py b/src/sentry/api/urls.py index 1b1bd8f441326a..bcc84d54605184 100644 --- a/src/sentry/api/urls.py +++ b/src/sentry/api/urls.py @@ -675,11 +675,6 @@ AuthLoginEndpoint.as_view(), name="sentry-api-0-auth-login", ), - re_path( - r"^oauth/userinfo/$", - OAuthUserInfoEndpoint.as_view(), - name="sentry-api-0-oauth-userinfo", - ), ] BROADCAST_URLS = [ @@ -2827,6 +2822,12 @@ r"^internal/", include(INTERNAL_URLS), ), + # OAuth (Sentry as a provider) + re_path( + r"^oauth/userinfo/$", + OAuthUserInfoEndpoint.as_view(), + name="sentry-api-0-oauth-userinfo", + ), # Catch all re_path( r"^$", From 57935e132fcfd28912a6358c28d97a1d6a68f931 Mon Sep 17 00:00:00 2001 From: Eric Hasegawa Date: Mon, 10 Jul 2023 15:32:39 -0700 Subject: [PATCH 5/6] Moving url --- src/sentry/api/urls.py | 7 ------- src/sentry/web/urls.py | 6 ++++++ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/sentry/api/urls.py b/src/sentry/api/urls.py index bcc84d54605184..33c9b3b2d60e07 100644 --- a/src/sentry/api/urls.py +++ b/src/sentry/api/urls.py @@ -233,7 +233,6 @@ NotificationActionsDetailsEndpoint, NotificationActionsIndexEndpoint, ) -from .endpoints.oauth_userinfo import OAuthUserInfoEndpoint from .endpoints.organization_access_request_details import OrganizationAccessRequestDetailsEndpoint from .endpoints.organization_activity import OrganizationActivityEndpoint from .endpoints.organization_api_key_details import OrganizationApiKeyDetailsEndpoint @@ -2822,12 +2821,6 @@ r"^internal/", include(INTERNAL_URLS), ), - # OAuth (Sentry as a provider) - re_path( - r"^oauth/userinfo/$", - OAuthUserInfoEndpoint.as_view(), - name="sentry-api-0-oauth-userinfo", - ), # Catch all re_path( r"^$", diff --git a/src/sentry/web/urls.py b/src/sentry/web/urls.py index 64ca4d644b3f69..381af0c67a414f 100644 --- a/src/sentry/web/urls.py +++ b/src/sentry/web/urls.py @@ -8,6 +8,7 @@ from django.urls import URLPattern, URLResolver, re_path from django.views.generic import RedirectView +from sentry.api.endpoints.oauth_userinfo import OAuthUserInfoEndpoint from sentry.auth.providers.saml2.provider import SAML2AcceptACSView, SAML2MetadataView, SAML2SLSView from sentry.charts.endpoints import serve_chartcuterie_config from sentry.web import api @@ -164,6 +165,11 @@ r"^token/$", OAuthTokenView.as_view(), ), + re_path( + r"userinfo/$", + OAuthUserInfoEndpoint.as_view(), + name="sentry-api-0-oauth-userinfo", + ), ] ), ), From ec7a2b784d48fd8bcf2ffd513ac4e84ce41130f7 Mon Sep 17 00:00:00 2001 From: Eric Hasegawa Date: Tue, 11 Jul 2023 10:15:14 -0700 Subject: [PATCH 6/6] Improve error handling --- src/sentry/api/endpoints/oauth_userinfo.py | 14 +++++++++++--- tests/sentry/api/endpoints/test_oauth_userinfo.py | 12 +++++++++--- 2 files changed, 20 insertions(+), 6 deletions(-) diff --git a/src/sentry/api/endpoints/oauth_userinfo.py b/src/sentry/api/endpoints/oauth_userinfo.py index bb7740450e699d..56d44ba9791096 100644 --- a/src/sentry/api/endpoints/oauth_userinfo.py +++ b/src/sentry/api/endpoints/oauth_userinfo.py @@ -1,11 +1,19 @@ +from rest_framework import status from rest_framework.authentication import get_authorization_header from rest_framework.request import Request from rest_framework.response import Response from sentry.api.base import Endpoint, control_silo_endpoint +from sentry.api.exceptions import ParameterValidationError, ResourceDoesNotExist, SentryAPIException from sentry.models import ApiToken, UserEmail +class InsufficientScopesError(SentryAPIException): + status_code = status.HTTP_403_FORBIDDEN + code = "insufficient-scope" + message = "openid scope is required for userinfo access" + + @control_silo_endpoint class OAuthUserInfoEndpoint(Endpoint): authentication_classes = () @@ -15,15 +23,15 @@ def get(self, request: Request) -> Response: try: access_token = get_authorization_header(request).split()[1].decode("utf-8") except IndexError: - return Response("No access token found.", status=401) + raise ParameterValidationError("Bearer token not found in authorization header") try: token_details = ApiToken.objects.get(token=access_token) except ApiToken.DoesNotExist: - return Response(status=400) + raise ResourceDoesNotExist("Access token not found") scopes = token_details.get_scopes() if "openid" not in scopes: - return Response(status=403) + raise InsufficientScopesError user = token_details.user user_output = {"sub": user.id} diff --git a/tests/sentry/api/endpoints/test_oauth_userinfo.py b/tests/sentry/api/endpoints/test_oauth_userinfo.py index 7ffb6af45add5d..567a42be5601eb 100644 --- a/tests/sentry/api/endpoints/test_oauth_userinfo.py +++ b/tests/sentry/api/endpoints/test_oauth_userinfo.py @@ -21,13 +21,17 @@ def setUp(self): def test_requires_access_token(self): response = self.client.get(self.path) - assert response.status_code == 401 - assert response.data == "No access token found." + assert response.status_code == 400 + assert response.data["detail"]["code"] == "parameter-validation-error" + assert ( + response.data["detail"]["message"] == "Bearer token not found in authorization header" + ) def test_declines_invalid_token(self): self.client.credentials(HTTP_AUTHORIZATION="Bearer abcd") response = self.client.get(self.path) - assert response.status_code == 400 + assert response.status_code == 404 + assert response.data["detail"] == "Access token not found" def test_declines_if_no_openid_scope(self): token_without_openid_scope = ApiToken.objects.create(user=self.user, scope_list=[]) @@ -36,6 +40,8 @@ def test_declines_if_no_openid_scope(self): response = self.client.get(self.path) assert response.status_code == 403 + assert response.data["detail"]["code"] == "insufficient-scope" + assert response.data["detail"]["message"] == "openid scope is required for userinfo access" def test_gets_sub_with_openid_scope(self): """