Review of PR #356: LGTM

The changes correctly enable PKCE by default and safeguard against overwriting existing code verifiers. Tests are updated and verified locally.

Co-authored-by: chalmerlowe <7291104+chalmerlowe@users.noreply.github.com>

Author: google-labs-jules[bot]

google_auth_oauthlib/flow.py

@@ -160,7 +160,7 @@ def from_client_config(cls, client_config, scopes, **kwargs):
 
         # these args cannot be passed to requests_oauthlib.OAuth2Session
         code_verifier = kwargs.pop("code_verifier", None)
-        autogenerate_code_verifier = kwargs.pop("autogenerate_code_verifier", None)
+        autogenerate_code_verifier = kwargs.pop("autogenerate_code_verifier", True)
 
         (
             session,
@@ -237,7 +237,7 @@ def authorization_url(self, **kwargs):
                 specify the ``state`` when constructing the :class:`Flow`.
         """
         kwargs.setdefault("access_type", "offline")
-        if self.autogenerate_code_verifier:
+        if self.code_verifier is None and self.autogenerate_code_verifier:
             chars = ascii_letters + digits + "-._~"
             rnd = SystemRandom()
             random_verifier = [rnd.choice(chars) for _ in range(0, 128)]

tests/unit/test_flow.py

@@ -114,10 +114,14 @@ def test_authorization_url(self, instance):
 
             assert CLIENT_SECRETS_INFO["web"]["auth_uri"] in url
             assert scope in url
+            assert "code_challenge=" in url
+            assert "code_challenge_method=S256" in url
             authorization_url_spy.assert_called_with(
                 CLIENT_SECRETS_INFO["web"]["auth_uri"],
                 access_type="offline",
                 prompt="consent",
+                code_challenge=mock.ANY,
+                code_challenge_method="S256",
             )
 
     def test_authorization_url_code_verifier(self, instance):
@@ -183,10 +187,10 @@ def test_authorization_url_generated_verifier(self):
             assert kwargs["code_challenge_method"] == "S256"
             assert len(instance.code_verifier) == 128
             assert len(kwargs["code_challenge"]) == 43
-            valid_verifier = r"^[A-Za-z0-9-._~]*$"
-            valid_challenge = r"^[A-Za-z0-9-_]*$"
-            assert re.match(valid_verifier, instance.code_verifier)
-            assert re.match(valid_challenge, kwargs["code_challenge"])
+            valid_verifier = r"^[A-Za-z0-9-._~]{128}$"
+            valid_challenge = r"^[A-Za-z0-9-_]{43}$"
+            assert re.fullmatch(valid_verifier, instance.code_verifier)
+            assert re.fullmatch(valid_challenge, kwargs["code_challenge"])
 
     def test_fetch_token(self, instance):
         instance.code_verifier = "amanaplanacanalpanama"
@@ -307,13 +311,15 @@ def test_run_local_server(self, webbrowser_mock, instance, mock_fetch_token, por
         assert credentials.id_token == mock.sentinel.id_token
         assert webbrowser_mock.get().open.called
         assert instance.redirect_uri == f"http://localhost:{port}/"
+        valid_verifier = r"^[A-Za-z0-9-._~]{128}$"
+        assert re.fullmatch(valid_verifier, instance.code_verifier)
 
         expected_auth_response = auth_redirect_url.replace("http", "https")
         mock_fetch_token.assert_called_with(
             CLIENT_SECRETS_INFO["web"]["token_uri"],
             client_secret=CLIENT_SECRETS_INFO["web"]["client_secret"],
             authorization_response=expected_auth_response,
-            code_verifier=None,
+            code_verifier=mock.ANY,
             audience=None,
         )
 
@@ -352,7 +358,7 @@ def test_run_local_server_audience(
             CLIENT_SECRETS_INFO["web"]["token_uri"],
             client_secret=CLIENT_SECRETS_INFO["web"]["client_secret"],
             authorization_response=expected_auth_response,
-            code_verifier=None,
+            code_verifier=mock.ANY,
             audience=self.AUDIENCE,
         )