feat(auth): Return ID token from /token endpoint (#52492)

Returns an HS256 encrypted ID token from the `/oauth/token` endpoint if
the access token has the right scopes.
Also:
- Adds the necessary scopes to achieve this.
- Refactors the `token` code to avoid having one really long `POST`
method.
- Improves testing for the `token` endpoint.
- Uses a secure random value to encrypt ID tokens until we can implement
proper shared secrets (i.e. this is safe to release).

migrations_lockfile.txt

@@ -6,5 +6,5 @@ To resolve this, rebase against latest master and regenerate your migration. Thi
 will then be regenerated, and you should be able to merge without conflicts.
 
 nodestore: 0002_nodestore_no_dictfield
-sentry: 0508_index_checkin_monitorenvironment
+sentry: 0509_merging_migrations
 social_auth: 0001_initial

src/sentry/conf/server.py

@@ -2097,6 +2097,9 @@ SENTRY_SCOPES = {
     "event:admin",
     "alerts:write",
     "alerts:read",
+    "openid",
+    "profile",
+    "email",
 }
 
 SENTRY_SCOPE_SETS = (
@@ -2131,6 +2134,14 @@ SENTRY_SCOPE_SETS = (
         ("alerts:write", "Read and write alerts"),
         ("alerts:read", "Read alerts"),
     ),
+    (("openid", "Confirms authentication status and provides basic information."),),
+    (
+        (
+            "profile",
+            "Read personal information like name, avatar, date of joining etc. Requires openid scope.",
+        ),
+    ),
+    (("email", "Read email address and verification status. Requires openid scope."),),
 )
 
 SENTRY_DEFAULT_ROLE = "member"

src/sentry/migrations/0507_add_oidc_scopes.py

@@ -0,0 +1,55 @@
+# Generated by Django 2.2.28 on 2023-07-08 19:10
+
+from django.db import migrations
+
+import bitfield.models
+from sentry.new_migrations.migrations import CheckedMigration
+
+
+class Migration(CheckedMigration):
+    # This flag is used to mark that a migration shouldn't be automatically run in production. For
+    # the most part, this should only be used for operations where it's safe to run the migration
+    # after your code has deployed. So this should not be used for most operations that alter the
+    # schema of a table.
+    # Here are some things that make sense to mark as dangerous:
+    # - Large data migrations. Typically we want these to be run manually by ops so that they can
+    #   be monitored and not block the deploy for a long period of time while they run.
+    # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to
+    #   have ops run this and not block the deploy. Note that while adding an index is a schema
+    #   change, it's completely safe to run the operation after the code has deployed.
+    is_dangerous = False
+
+    dependencies = [
+        ("sentry", "0506_null_boolean_fields"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="apigrant",
+            name="scopes",
+            field=bitfield.models.BitField(
+                [
+                    "project:read",
+                    "project:write",
+                    "project:admin",
+                    "project:releases",
+                    "team:read",
+                    "team:write",
+                    "team:admin",
+                    "event:read",
+                    "event:write",
+                    "event:admin",
+                    "org:read",
+                    "org:write",
+                    "org:admin",
+                    "member:read",
+                    "member:write",
+                    "member:admin",
+                    "openid",
+                    "profile",
+                    "email",
+                ],
+                default=None,
+            ),
+        ),
+    ]

src/sentry/migrations/0508_merging_migrations.py

@@ -0,0 +1,24 @@
+# Generated by Django 2.2.28 on 2023-07-10 20:12
+
+from sentry.new_migrations.migrations import CheckedMigration
+
+
+class Migration(CheckedMigration):
+    # This flag is used to mark that a migration shouldn't be automatically run in production. For
+    # the most part, this should only be used for operations where it's safe to run the migration
+    # after your code has deployed. So this should not be used for most operations that alter the
+    # schema of a table.
+    # Here are some things that make sense to mark as dangerous:
+    # - Large data migrations. Typically we want these to be run manually by ops so that they can
+    #   be monitored and not block the deploy for a long period of time while they run.
+    # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to
+    #   have ops run this and not block the deploy. Note that while adding an index is a schema
+    #   change, it's completely safe to run the operation after the code has deployed.
+    is_dangerous = False
+
+    dependencies = [
+        ("sentry", "0507_delete_pending_deletion_rules"),
+        ("sentry", "0507_add_oidc_scopes"),
+    ]
+
+    operations = []

src/sentry/migrations/0509_merging_migrations.py

@@ -0,0 +1,25 @@
+# Generated by Django 2.2.28 on 2023-07-10 20:30
+
+
+from sentry.new_migrations.migrations import CheckedMigration
+
+
+class Migration(CheckedMigration):
+    # This flag is used to mark that a migration shouldn't be automatically run in production. For
+    # the most part, this should only be used for operations where it's safe to run the migration
+    # after your code has deployed. So this should not be used for most operations that alter the
+    # schema of a table.
+    # Here are some things that make sense to mark as dangerous:
+    # - Large data migrations. Typically we want these to be run manually by ops so that they can
+    #   be monitored and not block the deploy for a long period of time while they run.
+    # - Adding indexes to large tables. Since this can take a long time, we'd generally prefer to
+    #   have ops run this and not block the deploy. Note that while adding an index is a schema
+    #   change, it's completely safe to run the operation after the code has deployed.
+    is_dangerous = False
+
+    dependencies = [
+        ("sentry", "0508_index_checkin_monitorenvironment"),
+        ("sentry", "0508_merging_migrations"),
+    ]
+
+    operations = []

src/sentry/models/apigrant.py

@@ -54,6 +54,9 @@ class ApiGrant(Model):
                 "member:read": bool,
                 "member:write": bool,
                 "member:admin": bool,
+                "openid": bool,
+                "profile": bool,
+                "email": bool,
             },
         )
     )

src/sentry/web/frontend/oauth_token.py

@@ -1,4 +1,5 @@
 import logging
+import secrets
 
 from django.http import HttpRequest, HttpResponse
 from django.utils import timezone
@@ -9,6 +10,7 @@ from django.views.generic.base import View
 from sentry.mediators import GrantTypes
 from sentry.models import ApiApplication, ApiApplicationStatus, ApiGrant, ApiToken
 from sentry.utils import json
+from sentry.web.frontend.openidtoken import OpenIDToken
 
 logger = logging.getLogger("sentry.api")
 
@@ -41,85 +43,103 @@ class OAuthTokenView(View):
     @never_cache
     def post(self, request: HttpRequest) -> HttpResponse:
         grant_type = request.POST.get("grant_type")
-
+        client_id = request.POST.get("client_id")
+        if not client_id:
+            return self.error(request, "invalid_client", "missing client_id")
         if grant_type == GrantTypes.AUTHORIZATION:
-            client_id = request.POST.get("client_id")
-            redirect_uri = request.POST.get("redirect_uri")
-            code = request.POST.get("code")
-
-            if not client_id:
-                return self.error(request, "invalid_client", "missing client_id")
-
-            try:
-                application = ApiApplication.objects.get(
-                    client_id=client_id, status=ApiApplicationStatus.active
-                )
-            except ApiApplication.DoesNotExist:
-                return self.error(request, "invalid_client", "invalid client_id")
-
-            try:
-                grant = ApiGrant.objects.get(application=application, code=code)
-            except ApiGrant.DoesNotExist:
-                return self.error(request, "invalid_grant", "invalid grant")
-
-            if grant.is_expired():
-                return self.error(request, "invalid_grant", "grant expired")
-
-            if not redirect_uri:
-                redirect_uri = application.get_default_redirect_uri()
-            elif grant.redirect_uri != redirect_uri:
-                return self.error(request, "invalid_grant", "invalid redirect_uri")
-
-            token = ApiToken.from_grant(grant)
-        elif grant_type == "refresh_token":
-            refresh_token = request.POST.get("refresh_token")
-            scope = request.POST.get("scope")
-            client_id = request.POST.get("client_id")
-
-            if not refresh_token:
-                return self.error(request, "invalid_request")
-
-            # TODO(dcramer): support scope
-            if scope:
-                return self.error(request, "invalid_request")
-
-            if not client_id:
-                return self.error(request, "invalid_client", "missing client_id")
-
-            try:
-                application = ApiApplication.objects.get(
-                    client_id=client_id, status=ApiApplicationStatus.active
-                )
-            except ApiApplication.DoesNotExist:
-                return self.error(request, "invalid_client", "invalid client_id")
-
-            try:
-                token = ApiToken.objects.get(application=application, refresh_token=refresh_token)
-            except ApiToken.DoesNotExist:
-                return self.error(request, "invalid_grant", "invalid token")
-
-            token.refresh()
+            return self._get_access_tokens(request, client_id)
+        elif grant_type == GrantTypes.REFRESH:
+            return self._get_refresh_token(request, client_id)
         else:
             return self.error(request, "unsupported_grant_type")
 
+    def _get_access_tokens(self, request, client_id):
+        redirect_uri = request.POST.get("redirect_uri")
+        code = request.POST.get("code")
+
+        try:
+            application = ApiApplication.objects.get(
+                client_id=client_id, status=ApiApplicationStatus.active
+            )
+        except ApiApplication.DoesNotExist:
+            return self.error(request, "invalid_client", "invalid client_id")
+
+        try:
+            grant = ApiGrant.objects.get(application=application, code=code)
+        except ApiGrant.DoesNotExist:
+            return self.error(request, "invalid_grant", "invalid grant")
+
+        if grant.is_expired():
+            return self.error(request, "invalid_grant", "grant expired")
+
+        if not redirect_uri:
+            redirect_uri = application.get_default_redirect_uri()
+        elif grant.redirect_uri != redirect_uri:
+            return self.error(request, "invalid_grant", "invalid redirect_uri")
+
+        access_token = ApiToken.from_grant(grant)
+        id_token = self._get_open_id_token(grant, request)
+        return self._process_token_details(access_token, id_token)
+
+    def _get_open_id_token(self, grant, request):
+        if grant.has_scope("openid"):
+            open_id_token = OpenIDToken(
+                request.POST.get("client_id"),
+                grant.user_id,
+                # Encrypt with a random secret until we implement secure shared secrets in prod
+                secrets.token_urlsafe(),
+                nonce=request.POST.get("nonce"),
+            )
+            return open_id_token.get_encrypted_id_token()
+        return None
+
+    def _get_refresh_token(self, request, client_id):
+        refresh_token = request.POST.get("refresh_token")
+        scope = request.POST.get("scope")
+
+        if not refresh_token:
+            return self.error(request, "invalid_request")
+
+        # TODO(dcramer): support scope
+        if scope:
+            return self.error(request, "invalid_request")
+
+        try:
+            application = ApiApplication.objects.get(
+                client_id=client_id, status=ApiApplicationStatus.active
+            )
+        except ApiApplication.DoesNotExist:
+            return self.error(request, "invalid_client", "invalid client_id")
+
+        try:
+            token = ApiToken.objects.get(application=application, refresh_token=refresh_token)
+        except ApiToken.DoesNotExist:
+            return self.error(request, "invalid_grant", "invalid refresh token")
+
+        token.refresh()
+
+        return self._process_token_details(token)
+
+    def _process_token_details(self, token, id_token=None):
+        token_information = {
+            "access_token": token.token,
+            "refresh_token": token.refresh_token,
+            "expires_in": int((token.expires_at - timezone.now()).total_seconds())
+            if token.expires_at
+            else None,
+            "expires_at": token.expires_at,
+            "token_type": "bearer",
+            "scope": " ".join(token.get_scopes()),
+            "user": {
+                "id": str(token.user.id),
+                # we might need these to become scope based
+                "name": token.user.name,
+                "email": token.user.email,
+            },
+        }
+        if id_token:
+            token_information["id_token"] = id_token
         return HttpResponse(
-            json.dumps(
-                {
-                    "access_token": token.token,
-                    "refresh_token": token.refresh_token,
-                    "expires_in": int((token.expires_at - timezone.now()).total_seconds())
-                    if token.expires_at
-                    else None,
-                    "expires_at": token.expires_at,
-                    "token_type": "bearer",
-                    "scope": " ".join(token.get_scopes()),
-                    "user": {
-                        "id": str(token.user.id),
-                        # we might need these to become scope based
-                        "name": token.user.name,
-                        "email": token.user.email,
-                    },
-                }
-            ),
+            json.dumps(token_information),
             content_type="application/json",
         )

src/sentry/web/frontend/openidtoken.py

@@ -21,11 +21,13 @@ class OpenIDToken:
         self,
         aud,
         sub,
+        shared_secret,
         iss="https://sentry.io",
         exp=None,
         iat=None,
         nonce=None,
     ):
+        self.shared_secret = shared_secret
         self.aud = aud
         self.sub = sub
         self.iss = iss
@@ -47,4 +49,4 @@ class OpenIDToken:
         }
         if self.nonce:
             claims["nonce"] = self.nonce
-        return jwt_utils.encode(claims, "secret", headers={**headers, "alg": "HS256"})
+        return jwt_utils.encode(claims, self.shared_secret, headers={**headers, "alg": "HS256"})

tests/sentry/api/serializers/test_organization.py

@@ -20,7 +20,10 @@ from sentry.models.organizationonboardingtask import OnboardingTask, OnboardingT
 from sentry.testutils import TestCase
 from sentry.testutils.silo import region_silo_test
 
-default_owner_scopes = frozenset(filter(lambda scope: scope != "org:ci", settings.SENTRY_SCOPES))
+non_default_owner_scopes = ["org:ci", "openid", "email", "profile"]
+default_owner_scopes = frozenset(
+    filter(lambda scope: scope not in non_default_owner_scopes, settings.SENTRY_SCOPES)
+)
 
 mock_options_as_features = {
     "sentry:set_no_value": [

tests/sentry/web/frontend/test_oauth_token.py

@@ -24,7 +24,7 @@ class OAuthTokenTest(TestCase):
     def test_missing_grant_type(self):
         self.login_as(self.user)
 
-        resp = self.client.post(self.path)
+        resp = self.client.post(self.path, {"client_id": "abcd"})
 
         assert resp.status_code == 400
         assert json.loads(resp.content) == {"error": "unsupported_grant_type"}
@@ -32,7 +32,7 @@ class OAuthTokenTest(TestCase):
     def test_invalid_grant_type(self):
         self.login_as(self.user)
 
-        resp = self.client.post(self.path, {"grant_type": "foo"})
+        resp = self.client.post(self.path, {"grant_type": "foo", "client_id": "abcd"})
 
         assert resp.status_code == 400
         assert json.loads(resp.content) == {"error": "unsupported_grant_type"}
@@ -115,6 +115,89 @@ class OAuthTokenCodeTest(TestCase):
         assert resp.status_code == 400
         assert json.loads(resp.content) == {"error": "invalid_grant"}
 
+    def test_expired_grant(self):
+        self.login_as(self.user)
+        expired_grant = ApiGrant.objects.create(
+            user=self.user,
+            application=self.application,
+            redirect_uri="https://example.com",
+            expires_at="2022-01-01 11:11",
+        )
+        resp = self.client.post(
+            self.path,
+            {
+                "grant_type": "authorization_code",
+                "redirect_uri": self.application.get_default_redirect_uri(),
+                "code": expired_grant.code,
+                "client_id": self.application.client_id,
+            },
+        )
+        assert resp.status_code == 400
+        assert json.loads(resp.content) == {"error": "invalid_grant"}
+
+    def test_invalid_redirect_uri(self):
+        self.login_as(self.user)
+
+        resp = self.client.post(
+            self.path,
+            {
+                "grant_type": "authorization_code",
+                "code": self.grant.code,
+                "client_id": self.application.client_id,
+                "redirect_uri": "cheese.org",
+            },
+        )
+        assert resp.status_code == 400
+        assert json.loads(resp.content) == {"error": "invalid_grant"}
+
+    def test_no_open_id_token(self):
+        """
+        Checks that the OIDC token is not returned unless the right scope is approved.
+        """
+        self.login_as(self.user)
+
+        resp = self.client.post(
+            self.path,
+            {
+                "grant_type": "authorization_code",
+                "code": self.grant.code,
+                "client_id": self.application.client_id,
+            },
+        )
+
+        assert resp.status_code == 200
+        data = json.loads(resp.content)
+        assert "id_token" not in data
+
+    def test_valid_no_redirect_uri(self):
+        """
+        Checks that we get the correct redirect URI if we don't pass one in
+        """
+        self.login_as(self.user)
+
+        resp = self.client.post(
+            self.path,
+            {
+                "grant_type": "authorization_code",
+                "code": self.grant.code,
+                "client_id": self.application.client_id,
+            },
+        )
+
+        assert resp.status_code == 200
+        data = json.loads(resp.content)
+
+        token = ApiToken.objects.get(token=data["access_token"])
+        assert token.application == self.application
+        assert token.user == self.grant.user
+        assert token.get_scopes() == self.grant.get_scopes()
+
+        assert data["access_token"] == token.token
+        assert data["refresh_token"] == token.refresh_token
+        assert isinstance(data["expires_in"], int)
+        assert data["token_type"] == "bearer"
+        assert data["user"]["id"] == str(token.user_id)
+
     def test_valid_params(self):
         self.login_as(self.user)
 
@@ -142,6 +225,37 @@ class OAuthTokenCodeTest(TestCase):
         assert data["token_type"] == "bearer"
         assert data["user"]["id"] == str(token.user_id)
 
+    def test_valid_params_id_token(self):
+        self.login_as(self.user)
+        open_id_grant = ApiGrant.objects.create(
+            user=self.user,
+            application=self.application,
+            redirect_uri="https://example.com",
+            scope_list=["openid", "profile", "email"],
+        )
+        resp = self.client.post(
+            self.path,
+            {
+                "grant_type": "authorization_code",
+                "redirect_uri": self.application.get_default_redirect_uri(),
+                "code": open_id_grant.code,
+                "client_id": self.application.client_id,
+            },
+        )
+        assert resp.status_code == 200
+
+        data = json.loads(resp.content)
+        token = ApiToken.objects.get(token=data["access_token"])
+
+        assert token.get_scopes() == ["openid", "profile", "email"]
+        assert data["refresh_token"] == token.refresh_token
+        assert data["access_token"] == token.token
+        assert isinstance(data["expires_in"], int)
+        assert data["token_type"] == "bearer"
+        assert data["user"]["id"] == str(token.user_id)
+
+        assert data["id_token"].count(".") == 2
+
 
 @control_silo_test(stable=True)
 class OAuthTokenRefreshTokenTest(TestCase):