mirror of
https://github.com/goauthentik/authentik
synced 2026-04-25 17:15:26 +02:00
sources/oauth: pick a single pkce method from OIDC discovery, not the whole list (#21689)
* sources/oauth: pick a single pkce method from OIDC discovery, not the whole list
When an OAuth source is configured with `oidc_well_known_url`, the API
serializer fetches the upstream's OpenID configuration and merges the
selected endpoints into the source attrs. The merge used a straight
field_map that aliased the pkce TextField to
`code_challenge_methods_supported`:
field_map = {
...
"pkce": "code_challenge_methods_supported",
}
for ak_key, oidc_key in field_map.items():
...
attrs[ak_key] = config.get(oidc_key, "")
`code_challenge_methods_supported` is a JSON array per RFC 8414
(e.g. ["plain", "S256"]), but attrs["pkce"] is backed by a TextField
with choices NONE / PLAIN / S256. Django does not validate choices on
plain assignment, so the list survives serialisation and is later
formatted by the client as
str(pkce_mode) -> "['plain', 'S256']"
which ships as `code_challenge_method=%5B%27plain%27%2C+%27S256%27%5D`
on the /authorize request. The upstream rejects the subsequent /token
exchange with HTTP 400 because it has no PKCE state for that value.
Separate the pkce handling from the rest of the field_map loop: only
fill pkce when the user has not set it, and select one scalar method
from the advertised list (prefer S256, the RFC 7636 MUST for public
clients, then plain, then NONE as a last resort). Non-list / missing
values fall back to NONE. User-supplied pkce still wins, matching the
existing "don't overwrite user-set values" intent.
Fixes #21665
Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
* update test
Signed-off-by: Jens Langhammer <jens@goauthentik.io>
* simplify
Signed-off-by: Jens Langhammer <jens@goauthentik.io>
---------
Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>
Signed-off-by: Jens Langhammer <jens@goauthentik.io>
Co-authored-by: SAY-5 <SAY-5@users.noreply.github.com>
Co-authored-by: Jens Langhammer <jens@goauthentik.io>
This commit is contained in:
@@ -17,7 +17,7 @@ from authentik.core.api.sources import SourceSerializer
|
||||
from authentik.core.api.used_by import UsedByMixin
|
||||
from authentik.core.api.utils import PassiveSerializer
|
||||
from authentik.lib.utils.http import get_http_session
|
||||
from authentik.sources.oauth.models import OAuthSource
|
||||
from authentik.sources.oauth.models import OAuthSource, PKCEMethod
|
||||
from authentik.sources.oauth.types.registry import SourceType, registry
|
||||
|
||||
|
||||
@@ -83,13 +83,24 @@ class OAuthSourceSerializer(SourceSerializer):
|
||||
"authorization_url": "authorization_endpoint",
|
||||
"access_token_url": "token_endpoint",
|
||||
"profile_url": "userinfo_endpoint",
|
||||
"pkce": "code_challenge_methods_supported",
|
||||
}
|
||||
for ak_key, oidc_key in field_map.items():
|
||||
# Don't overwrite user-set values
|
||||
if ak_key in attrs and attrs[ak_key]:
|
||||
continue
|
||||
attrs[ak_key] = config.get(oidc_key, "")
|
||||
# code_challenge_methods_supported is a list per RFC 8414, not a
|
||||
# single method. Pick one (prefer S256, the RFC-recommended method)
|
||||
# rather than letting the list round-trip into the pkce TextField
|
||||
# and later str() into the authorize URL as "['plain', 'S256']".
|
||||
if not attrs.get("pkce"):
|
||||
supported_methods = config.get("code_challenge_methods_supported") or []
|
||||
attrs["pkce"] = PKCEMethod.NONE
|
||||
if isinstance(supported_methods, list):
|
||||
if PKCEMethod.S256 in supported_methods:
|
||||
attrs["pkce"] = PKCEMethod.S256
|
||||
elif PKCEMethod.PLAIN in supported_methods:
|
||||
attrs["pkce"] = PKCEMethod.PLAIN
|
||||
inferred_oidc_jwks_url = config.get("jwks_uri", "")
|
||||
|
||||
# Prefer user-entered URL to inferred URL to default URL
|
||||
|
||||
@@ -79,6 +79,7 @@ class TestOAuthSource(APITestCase):
|
||||
"token_endpoint": "http://mock/oauth/token",
|
||||
"userinfo_endpoint": "http://mock/oauth/userinfo",
|
||||
"jwks_uri": "http://mock/oauth/discovery/keys",
|
||||
"code_challenge_methods_supported": ["S256"],
|
||||
}
|
||||
jwks_config = {"keys": []}
|
||||
with Mocker() as mocker:
|
||||
@@ -109,6 +110,7 @@ class TestOAuthSource(APITestCase):
|
||||
serializer.validated_data["oidc_jwks_url"], "http://mock/oauth/discovery/keys"
|
||||
)
|
||||
self.assertEqual(serializer.validated_data["oidc_jwks"], jwks_config)
|
||||
self.assertEqual(serializer.validated_data["pkce"], PKCEMethod.S256)
|
||||
|
||||
def test_api_validate_openid_connect_invalid(self):
|
||||
"""Test API validation (with OIDC endpoints)"""
|
||||
|
||||
Reference in New Issue
Block a user