jimfuqian/BB2-3525 Improve PKCE validation logic and error messages (#1311)

* improve PKCE validation logic and error messages.

* remove 'gender' as required (subject to further cleanup).

* temp comment gender stuff to pass integration tests

* add tests

* Re-add dispatch csrf exempt

* revert the url in openapi.yaml (changed to localhost:8000 for testing things)

* revert temporarily commented out integration tests code.

---------

Co-authored-by: Logan Bertram <loganbertram@navapbc.com>

Author: JFU-NAVA-PBC

apps/pkce/oauth2_server.py

@@ -1,12 +1,16 @@
 import base64
 import hashlib
 from urllib.parse import urlparse
-from oauthlib.oauth2.rfc6749.errors import OAuth2Error
+from oauthlib.oauth2.rfc6749.errors import OAuth2Error, InvalidRequestError
 
 from oauth2_provider.models import get_grant_model
 from oauth2_provider.settings import oauth2_settings
 from django.core.exceptions import ObjectDoesNotExist
 
+ERR_CCM_S256_REQUIRED = "PKCE code_challenge_method required to be S256"
+ERR_CC_REQUIRED = "PKCE code_challenge required"
+ERR_CCM_REQUIRED = "code_challenge_method required for pkce, missing parameter: code_challenge_method=S256"
+
 Grant = get_grant_model()
 
 
@@ -26,18 +30,26 @@ def validate_redirect_uri_pkce(request):
 
             return {}
 
-        raise OAuth2Error("Non http uri scheme's must be used with pkce")
+        raise InvalidRequestError("Non http uri scheme's must be used with pkce")
 
     return {}
 
 
 def validate_code_challenge_method(request):
+    # note: here the request is a sanitized oauthlib.Request object and always
+    # has code_challenge_method, and code_challenge in its internal _params dict,
+    # so it seems hasattr(request, 'code_challenge_method') always holds
     if hasattr(request, 'code_challenge_method') and request.code_challenge_method:
         if request.code_challenge_method != "S256":
-            raise OAuth2Error("S256 code challenge method required for pkce")
+            raise InvalidRequestError(ERR_CCM_S256_REQUIRED)
+        # now code_challenge_method=S256 found, further check
+        # code_challenge=<value> is present
+        if not request.code_challenge:
+            raise InvalidRequestError(ERR_CC_REQUIRED)
 
     elif hasattr(request, 'code_challenge') and request.code_challenge:
-        raise OAuth2Error("S256 code challenge method required for pkce")
+        raise InvalidRequestError(ERR_CCM_REQUIRED)
+
     return {}
 
 

apps/pkce/tests.py

@@ -0,0 +1,68 @@
+from django.test import TestCase
+from oauthlib.common import Request
+from oauthlib.oauth2.rfc6749.errors import InvalidRequestError
+
+from apps.pkce.oauth2_server import validate_code_challenge_method, validate_redirect_uri_pkce
+from apps.pkce.oauth2_server import ERR_CCM_REQUIRED, ERR_CC_REQUIRED, ERR_CCM_S256_REQUIRED
+
+PKCE_URIS = {
+    "MISSING_CODE_CHALLENGE_METHOD_PARAM": ("/v2/o/authorize/b787ff17-e865-4893-aa79-4016ae5677a0/?"
+                                            "response_type=code&client_id=XQFC5iseB84YGOidlqy0L9blhauz0pIffPnMi0qJ&"
+                                            "redirect_uri=http%3A%2F%2Flocalhost%3A8000%2Ftestclient%2Fcallback&"
+                                            "state=QCh2XTdgS3RdfEI1Qz0uOll8KStOdShwfVlzWyNBTGA%3D&"
+                                            "code_challenge=9GLy9RNcZrhuqk-kSSbqOHkkKTV8Og8PIbtkMPA7XjA%3D"),
+    "CODE_CHALLENGE_METHOD_NOT_S256": ("/v2/o/authorize/b787ff17-e865-4893-aa79-4016ae5677a0/?"
+                                       "response_type=code&client_id=XQFC5iseB84YGOidlqy0L9blhauz0pIffPnMi0qJ&"
+                                       "redirect_uri=http%3A%2F%2Flocalhost%3A8000%2Ftestclient%2Fcallback&"
+                                       "state=QCh2XTdgS3RdfEI1Qz0uOll8KStOdShwfVlzWyNBTGA%3D&"
+                                       "code_challenge=9GLy9RNcZrhuqk-kSSbqOHkkKTV8Og8PIbtkMPA7XjA%3D&"
+                                       "code_challenge_method=plain"),
+    "MISSING_CODE_CHALLENGE_PARAM": ("/v2/o/authorize/b787ff17-e865-4893-aa79-4016ae5677a0/?"
+                                     "response_type=code&client_id=XQFC5iseB84YGOidlqy0L9blhauz0pIffPnMi0qJ&"
+                                     "redirect_uri=http%3A%2F%2Flocalhost%3A8000%2Ftestclient%2Fcallback&"
+                                     "state=QCh2XTdgS3RdfEI1Qz0uOll8KStOdShwfVlzWyNBTGA%3D&code_challenge_method=S256"),
+    "MISSING_CODE_CHALLENGE_VAL": ("/v2/o/authorize/b787ff17-e865-4893-aa79-4016ae5677a0/?"
+                                   "response_type=code&client_id=XQFC5iseB84YGOidlqy0L9blhauz0pIffPnMi0qJ&"
+                                   "redirect_uri=http%3A%2F%2Flocalhost%3A8000%2Ftestclient%2Fcallback&"
+                                   "state=QCh2XTdgS3RdfEI1Qz0uOll8KStOdShwfVlzWyNBTGA%3D&code_challenge=&"
+                                   "code_challenge_method=S256"),
+    "AUTH_URI_W_S256_AND_CHALLENGE_CODE": ("/v2/o/authorize/b787ff17-e865-4893-aa79-4016ae5677a0/?"
+                                           "response_type=code&client_id=XQFC5iseB84YGOidlqy0L9blhauz0pIffPnMi0qJ&"
+                                           "redirect_uri=http%3A%2F%2Flocalhost%3A8000%2Ftestclient%2Fcallback&"
+                                           "state=QCh2XTdgS3RdfEI1Qz0uOll8KStOdShwfVlzWyNBTGA%3D&"
+                                           "code_challenge=9GLy9RNcZrhuqk-kSSbqOHkkKTV8Og8PIbtkMPA7XjA%3D&"
+                                           "code_challenge_method=S256"),
+    "AUTH_URI_NO_PKCE_PARAMS": ("/v2/o/authorize/b787ff17-e865-4893-aa79-4016ae5677a0/?"
+                                "response_type=code&client_id=XQFC5iseB84YGOidlqy0L9blhauz0pIffPnMi0qJ&"
+                                "redirect_uri=http%3A%2F%2Flocalhost%3A8000%2Ftestclient%2Fcallback&"
+                                "state=QCh2XTdgS3RdfEI1Qz0uOll8KStOdShwfVlzWyNBTGA%3D"), }
+
+
+class TestOAuth2PKCEValidation(TestCase):
+    def test_redirect_validation(self):
+        r = validate_redirect_uri_pkce(Request(uri=PKCE_URIS['AUTH_URI_W_S256_AND_CHALLENGE_CODE']))
+        self.assertFalse(r)
+
+    def test_non_pkce_validation(self):
+        r = validate_code_challenge_method(Request(uri=PKCE_URIS['AUTH_URI_NO_PKCE_PARAMS']))
+        self.assertFalse(r)
+
+    def test_pkce_validation(self):
+        r = validate_code_challenge_method(Request(uri=PKCE_URIS["AUTH_URI_W_S256_AND_CHALLENGE_CODE"]))
+        self.assertFalse(r)
+
+    def test_pkce_validation_no_ccm(self):
+        with self.assertRaisesMessage(InvalidRequestError, ERR_CCM_REQUIRED):
+            validate_code_challenge_method(Request(uri=PKCE_URIS["MISSING_CODE_CHALLENGE_METHOD_PARAM"]))
+
+    def test_pkce_validation_ccm_not_s256(self):
+        with self.assertRaisesMessage(InvalidRequestError, ERR_CCM_S256_REQUIRED):
+            validate_code_challenge_method(Request(uri=PKCE_URIS["CODE_CHALLENGE_METHOD_NOT_S256"]))
+
+    def test_pkce_validation_no_cc(self):
+        with self.assertRaisesMessage(InvalidRequestError, ERR_CC_REQUIRED):
+            validate_code_challenge_method(Request(uri=PKCE_URIS["MISSING_CODE_CHALLENGE_PARAM"]))
+
+    def test_pkce_validation_cc_no_val(self):
+        with self.assertRaisesMessage(InvalidRequestError, ERR_CC_REQUIRED):
+            validate_code_challenge_method(Request(uri=PKCE_URIS["MISSING_CODE_CHALLENGE_VAL"]))