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

feat(auth): Add /userinfo endpoint for OIDC #52493

Merged
merged 6 commits into from
Jul 11, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
42 changes: 42 additions & 0 deletions src/sentry/api/endpoints/oauth_userinfo.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
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")
EricHasegawa marked this conversation as resolved.
Show resolved Hide resolved
except IndexError:
return Response("No access token found.", status=401)
EricHasegawa marked this conversation as resolved.
Show resolved Hide resolved
try:
token_details = ApiToken.objects.get(token=access_token)
except ApiToken.DoesNotExist:
return Response(status=400)
EricHasegawa marked this conversation as resolved.
Show resolved Hide resolved

scopes = token_details.get_scopes()
if "openid" not in scopes:
return Response(status=403)
EricHasegawa marked this conversation as resolved.
Show resolved Hide resolved

user = token_details.user
user_output = {"sub": user.id}
if "profile" in scopes:
profile_details = {
"name": user.name,
"avatar_type": user.avatar_type,
"avatar_url": user.avatar_url,
"date_joined": user.date_joined,
Comment on lines +40 to +43
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Generally our API outputs properties with camelBacked identifiers. Is there a reason to diverge from that convention here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah - the OAuth/OIDC requires snake case in order for this to be considered a valid implementation - here's a thread discussing this exact thing.

However - this is only for the required OAuth stuff like client_id, access_token etc. So technically we could return those in snake case, and return the custom userinfo stuff in camelCase, but, I figured that would be even more confusing. LMK if you prefer it that way though and I can change it over!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point about the OIDC spec having standards for response keys. 🤔 Having two conventions in the same response is clunky. Lets go forward with what you have. Having consistency within the response which needs to follow the standard for some properties seems more important than our local API response key conventions.

}
user_output.update(profile_details)
if "email" in scopes:
email = UserEmail.objects.get(user=user)
EricHasegawa marked this conversation as resolved.
Show resolved Hide resolved
email_details = {"email": email.email, "email_verified": email.is_verified}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The email_verified attribute is generally serialized as is_verified. While It is unfortunately a snake cased identifier we should try to use consistent names across various endpoints.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I named this one email_verified since it's consistent with standard OIDC language (example from Auth0).

Also - this response contains more details on the user than just email stuff, if we just have is_verified do you think clients might be unsure as to what exactly is verified?

On the other hand, maybe is_verified, by extension, implies that the user is verified, and not just the email, plus we would be consistent with our codebase, rather than the OAuth standards, so there's a trade-off here.

I'd leave it this way but I'm good either way - lmk if you want me to change it over still and I will!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good points. Combined with the OIDC conventions discussed above, let's leave this as you have it.

user_output.update(email_details)
return Response(user_output)
6 changes: 6 additions & 0 deletions src/sentry/web/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -164,6 +165,11 @@
r"^token/$",
OAuthTokenView.as_view(),
),
re_path(
r"userinfo/$",
OAuthUserInfoEndpoint.as_view(),
name="sentry-api-0-oauth-userinfo",
),
]
),
),
Expand Down
101 changes: 101 additions & 0 deletions tests/sentry/api/endpoints/test_oauth_userinfo.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
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."
EricHasegawa marked this conversation as resolved.
Show resolved Hide resolved

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)

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