fix(auth): Require client secret in /token (#52953)

Sooo it looks to me like the OAuth provider we've been running for ~6
years doesn't actually require a client secret to be passed to the
`/token` endpoint. This is, obviously, not great.

This PR requires the client secret to be passed in and validates it, _so
long as the application was created after July 16th, 2023_. I did this
to preserve backwards compatibility, but if we want to break backwards
compatibility here in favor of security - let me know and I can remove
the date-checking logic.

Also - if I'm missing something here please let me know, I assumed
originally that credentials were being validated in one of the base
views with something like `ClientIdSecretAuthentication`
([source](https://github.com/getsentry/sentry/blob/17433defa2b99c98166f7e491653fccef24a4975/src/sentry/api/authentication.py#L159))
but I don't think anything of that sort is happening.

src/sentry/web/frontend/oauth_token.py

@@ -1,11 +1,13 @@
 import logging
 import secrets
+from typing import Optional
 
 from django.http import HttpRequest, HttpResponse
 from django.utils import timezone
 from django.views.decorators.cache import never_cache
 from django.views.decorators.csrf import csrf_exempt
 from django.views.generic.base import View
+from rest_framework.request import Request
 
 from sentry.mediators import GrantTypes
 from sentry.models import ApiApplication, ApiApplicationStatus, ApiGrant, ApiToken
@@ -44,83 +46,99 @@ class OAuthTokenView(View):
     def post(self, request: HttpRequest) -> HttpResponse:
         grant_type = request.POST.get("grant_type")
         client_id = request.POST.get("client_id")
+        client_secret = request.POST.get("client_secret")
+
         if not client_id:
-            return self.error(request, "invalid_client", "missing client_id")
-        if grant_type == GrantTypes.AUTHORIZATION:
-            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")
+            return self.error(request=request, name="missing_client_id", reason="missing client_id")
+        if not client_secret:
+            return self.error(
+                request=request, name="missing_client_secret", reason="missing client_secret"
+            )
 
-    def _get_access_tokens(self, request, client_id):
-        redirect_uri = request.POST.get("redirect_uri")
-        code = request.POST.get("code")
+        if grant_type not in [GrantTypes.AUTHORIZATION, GrantTypes.REFRESH]:
+            return self.error(request=request, name="unsupported_grant_type")
 
         try:
             application = ApiApplication.objects.get(
-                client_id=client_id, status=ApiApplicationStatus.active
+                client_id=client_id, client_secret=client_secret, status=ApiApplicationStatus.active
             )
         except ApiApplication.DoesNotExist:
-            return self.error(request, "invalid_client", "invalid client_id")
+            return self.error(
+                request=request,
+                name="invalid_credentials",
+                reason="invalid client_id or client_secret",
+                status=401,
+            )
 
+        if grant_type == GrantTypes.AUTHORIZATION:
+            token_data = self.get_access_tokens(request=request, application=application)
+        else:
+            token_data = self.get_refresh_token(request=request, application=application)
+        if "error" in token_data:
+            return self.error(
+                request=request,
+                name=token_data["error"],
+                reason=token_data["reason"] if "reason" in token_data else None,
+            )
+        return self.process_token_details(
+            token=token_data["token"],
+            id_token=token_data["id_token"] if "id_token" in token_data else None,
+        )
+
+    def get_access_tokens(self, request: Request, application: ApiApplication) -> dict:
+        code = request.POST.get("code")
         try:
             grant = ApiGrant.objects.get(application=application, code=code)
         except ApiGrant.DoesNotExist:
-            return self.error(request, "invalid_grant", "invalid grant")
+            return {"error": "invalid_grant", "reason": "invalid grant"}
 
         if grant.is_expired():
-            return self.error(request, "invalid_grant", "grant expired")
+            return {"error": "invalid_grant", "reason": "grant expired"}
 
+        redirect_uri = request.POST.get("redirect_uri")
         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")
+            return {"error": "invalid_grant", "reason": "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):
+        token_data = {"token": ApiToken.from_grant(grant=grant)}
+        id_token = None
         if grant.has_scope("openid"):
-            open_id_token = OpenIDToken(
+            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(grant=grant)
-        return None
+            token_data["id_token"] = id_token.get_encrypted_id_token(grant=grant)
+
+        return token_data
 
-    def _get_refresh_token(self, request, client_id):
-        refresh_token = request.POST.get("refresh_token")
+    def get_refresh_token(self, request: Request, application: ApiApplication) -> dict:
+        refresh_token_code = request.POST.get("refresh_token")
         scope = request.POST.get("scope")
 
-        if not refresh_token:
-            return self.error(request, "invalid_request")
+        if not refresh_token_code:
+            return {"error": "invalid_request"}
 
         # TODO(dcramer): support scope
         if scope:
-            return self.error(request, "invalid_request")
+            return {"error": "invalid_request"}
 
         try:
-            application = ApiApplication.objects.get(
-                client_id=client_id, status=ApiApplicationStatus.active
+            refresh_token = ApiToken.objects.get(
+                application=application, refresh_token=refresh_token_code
             )
-        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 {"error": "invalid_grant", "reason": "invalid request"}
+        refresh_token.refresh()
 
-        return self._process_token_details(token)
+        return {"token": refresh_token}
 
-    def _process_token_details(self, token, id_token=None):
+    def process_token_details(
+        self, token: ApiToken, id_token: Optional[OpenIDToken] = None
+    ) -> HttpResponse:
         token_information = {
             "access_token": token.token,
             "refresh_token": token.refresh_token,

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, {"client_id": "abcd"})
+        resp = self.client.post(self.path, {"client_id": "abcd", "client_secret": "abcd"})
 
         assert resp.status_code == 400
         assert json.loads(resp.content) == {"error": "unsupported_grant_type"}
@@ -32,7 +32,9 @@ class OAuthTokenTest(TestCase):
     def test_invalid_grant_type(self):
         self.login_as(self.user)
 
-        resp = self.client.post(self.path, {"grant_type": "foo", "client_id": "abcd"})
+        resp = self.client.post(
+            self.path, {"grant_type": "foo", "client_id": "abcd", "client_secret": "abcd"}
+        )
 
         assert resp.status_code == 400
         assert json.loads(resp.content) == {"error": "unsupported_grant_type"}
@@ -49,6 +51,7 @@ class OAuthTokenCodeTest(TestCase):
         self.application = ApiApplication.objects.create(
             owner=self.user, redirect_uris="https://example.com"
         )
+        self.client_secret = self.application.client_secret
         self.grant = ApiGrant.objects.create(
             user=self.user, application=self.application, redirect_uri="https://example.com"
         )
@@ -62,15 +65,15 @@ class OAuthTokenCodeTest(TestCase):
                 "grant_type": "authorization_code",
                 "redirect_uri": self.application.get_default_redirect_uri(),
                 "code": self.grant.code,
+                "client_secret": self.client_secret,
             },
         )
 
         assert resp.status_code == 400
-        assert json.loads(resp.content) == {"error": "invalid_client"}
+        assert json.loads(resp.content) == {"error": "missing_client_id"}
 
     def test_invalid_client_id(self):
         self.login_as(self.user)
-
         resp = self.client.post(
             self.path,
             {
@@ -78,11 +81,44 @@ class OAuthTokenCodeTest(TestCase):
                 "redirect_uri": self.application.get_default_redirect_uri(),
                 "code": self.grant.code,
                 "client_id": "def",
+                "client_secret": self.client_secret,
+            },
+        )
+        assert resp.status_code == 401
+        assert json.loads(resp.content) == {"error": "invalid_credentials"}
+
+    def test_missing_client_secret(self):
+        self.login_as(self.user)
+
+        resp = self.client.post(
+            self.path,
+            {
+                "grant_type": "authorization_code",
+                "redirect_uri": self.application.get_default_redirect_uri(),
+                "client_id": self.application.client_id,
+                "code": self.grant.code,
             },
         )
 
         assert resp.status_code == 400
-        assert json.loads(resp.content) == {"error": "invalid_client"}
+        assert json.loads(resp.content) == {"error": "missing_client_secret"}
+
+    def test_invalid_client_secret(self):
+        self.login_as(self.user)
+
+        resp = self.client.post(
+            self.path,
+            {
+                "grant_type": "authorization_code",
+                "redirect_uri": self.application.get_default_redirect_uri(),
+                "code": self.grant.code,
+                "client_id": self.application.client_id,
+                "client_secret": "rodrick_rules",
+            },
+        )
+
+        assert resp.status_code == 401
+        assert json.loads(resp.content) == {"error": "invalid_credentials"}
 
     def test_missing_code(self):
         self.login_as(self.user)
@@ -93,6 +129,7 @@ class OAuthTokenCodeTest(TestCase):
                 "grant_type": "authorization_code",
                 "redirect_uri": self.application.get_default_redirect_uri(),
                 "client_id": self.application.client_id,
+                "client_secret": self.client_secret,
             },
         )
 
@@ -109,6 +146,7 @@ class OAuthTokenCodeTest(TestCase):
                 "redirect_uri": self.application.get_default_redirect_uri(),
                 "code": "abc",
                 "client_id": self.application.client_id,
+                "client_secret": self.client_secret,
             },
         )
 
@@ -130,6 +168,7 @@ class OAuthTokenCodeTest(TestCase):
                 "redirect_uri": self.application.get_default_redirect_uri(),
                 "code": expired_grant.code,
                 "client_id": self.application.client_id,
+                "client_secret": self.client_secret,
             },
         )
         assert resp.status_code == 400
@@ -145,6 +184,7 @@ class OAuthTokenCodeTest(TestCase):
                 "code": self.grant.code,
                 "client_id": self.application.client_id,
                 "redirect_uri": "cheese.org",
+                "client_secret": self.client_secret,
             },
         )
         assert resp.status_code == 400
@@ -162,6 +202,7 @@ class OAuthTokenCodeTest(TestCase):
                 "grant_type": "authorization_code",
                 "code": self.grant.code,
                 "client_id": self.application.client_id,
+                "client_secret": self.client_secret,
             },
         )
 
@@ -181,6 +222,7 @@ class OAuthTokenCodeTest(TestCase):
                 "grant_type": "authorization_code",
                 "code": self.grant.code,
                 "client_id": self.application.client_id,
+                "client_secret": self.client_secret,
             },
         )
 
@@ -208,6 +250,7 @@ class OAuthTokenCodeTest(TestCase):
                 "redirect_uri": self.application.get_default_redirect_uri(),
                 "code": self.grant.code,
                 "client_id": self.application.client_id,
+                "client_secret": self.client_secret,
             },
         )
 
@@ -240,6 +283,7 @@ class OAuthTokenCodeTest(TestCase):
                 "redirect_uri": self.application.get_default_redirect_uri(),
                 "code": open_id_grant.code,
                 "client_id": self.application.client_id,
+                "client_secret": self.client_secret,
             },
         )
         assert resp.status_code == 200
@@ -271,6 +315,7 @@ class OAuthTokenCodeTest(TestCase):
                 "redirect_uri": self.application.get_default_redirect_uri(),
                 "code": open_id_grant.code,
                 "client_id": self.application.client_id,
+                "client_secret": self.client_secret,
             },
         )
         assert resp.status_code == 200
@@ -299,6 +344,8 @@ class OAuthTokenRefreshTokenTest(TestCase):
         self.application = ApiApplication.objects.create(
             owner=self.user, redirect_uris="https://example.com"
         )
+        self.client_secret = self.application.client_secret
+
         self.grant = ApiGrant.objects.create(
             user=self.user, application=self.application, redirect_uri="https://example.com"
         )
@@ -310,11 +357,16 @@ class OAuthTokenRefreshTokenTest(TestCase):
         self.login_as(self.user)
 
         resp = self.client.post(
-            self.path, {"grant_type": "refresh_token", "refresh_token": self.token.refresh_token}
+            self.path,
+            {
+                "grant_type": "refresh_token",
+                "refresh_token": self.token.refresh_token,
+                "client_secret": self.client_secret,
+            },
         )
 
         assert resp.status_code == 400
-        assert json.loads(resp.content) == {"error": "invalid_client"}
+        assert json.loads(resp.content) == {"error": "missing_client_id"}
 
     def test_invalid_client_id(self):
         self.login_as(self.user)
@@ -325,17 +377,23 @@ class OAuthTokenRefreshTokenTest(TestCase):
                 "grant_type": "refresh_token",
                 "client_id": "abc",
                 "refresh_token": self.token.refresh_token,
+                "client_secret": self.client_secret,
             },
         )
 
-        assert resp.status_code == 400
-        assert json.loads(resp.content) == {"error": "invalid_client"}
+        assert resp.status_code == 401
+        assert json.loads(resp.content) == {"error": "invalid_credentials"}
 
     def test_missing_refresh_token(self):
         self.login_as(self.user)
 
         resp = self.client.post(
-            self.path, {"grant_type": "refresh_token", "client_id": self.application.client_id}
+            self.path,
+            {
+                "grant_type": "refresh_token",
+                "client_id": self.application.client_id,
+                "client_secret": self.client_secret,
+            },
         )
 
         assert resp.status_code == 400
@@ -350,6 +408,7 @@ class OAuthTokenRefreshTokenTest(TestCase):
                 "grant_type": "refresh_token",
                 "client_id": self.application.client_id,
                 "refresh_token": "foo",
+                "client_secret": self.client_secret,
             },
         )
 
@@ -365,6 +424,7 @@ class OAuthTokenRefreshTokenTest(TestCase):
                 "grant_type": "refresh_token",
                 "client_id": self.application.client_id,
                 "refresh_token": self.token.refresh_token,
+                "client_secret": self.client_secret,
             },
         )