From 493b0d087d2ba93d4cf68631b80675a51e5cd121 Mon Sep 17 00:00:00 2001 From: Paul Swartz Date: Tue, 18 Jun 2024 09:32:40 -0400 Subject: [PATCH] fix: url_extension params also go in the request object (#354) Originally done in #299, this doesn't seem correct in practice. In particular, a team ran into this issue with Keycloak, where passing the `kc_action` parameter only works when it's included in the request object. I also tried this with the conformance suite, and all the tests continue to pass with this change. --- src/oidcc_authorization.erl | 63 +++++++++++++++++++------------ test/oidcc_authorization_test.erl | 7 ++-- 2 files changed, 42 insertions(+), 28 deletions(-) diff --git a/src/oidcc_authorization.erl b/src/oidcc_authorization.erl index ad593de..ffbafe9 100644 --- a/src/oidcc_authorization.erl +++ b/src/oidcc_authorization.erl @@ -117,8 +117,7 @@ create_redirect_url(#oidcc_client_context{} = ClientContext, Opts) -> maybe true ?= lists:member(<<"authorization_code">>, GrantTypesSupported), - {ok, QueryParams0} ?= redirect_params(ClientContext, Opts), - QueryParams = QueryParams0 ++ maps:get(url_extension, Opts, []), + {ok, QueryParams} ?= redirect_params(ClientContext, Opts), QueryString = uri_string:compose_query(QueryParams), {ok, [AuthEndpoint, <<"?">>, QueryString]} else @@ -132,6 +131,7 @@ create_redirect_url(#oidcc_client_context{} = ClientContext, Opts) -> ClientContext :: oidcc_client_context:t(), Opts :: opts(). redirect_params(#oidcc_client_context{client_id = ClientId} = ClientContext, Opts) -> + UrlExtension = maps:get(url_extension, Opts, []), QueryParams0 = [ {<<"response_type">>, maps:get(response_type, Opts, <<"code">>)}, @@ -158,7 +158,7 @@ redirect_params(#oidcc_client_context{client_id = ClientId} = ClientContext, Opt maps:get(scopes, Opts, [openid]), QueryParams5 ), QueryParams7 = maybe_append_dpop_jkt(QueryParams6, ClientContext), - {ok, QueryParams} ?= attempt_request_object(QueryParams7, ClientContext), + {ok, QueryParams} ?= attempt_request_object(QueryParams7, ClientContext, UrlExtension), attempt_par(QueryParams, ClientContext, Opts) end. @@ -252,25 +252,30 @@ maybe_append_dpop_jkt( maybe_append_dpop_jkt(QueryParams, _ClientContext) -> QueryParams. --spec attempt_request_object(QueryParams, ClientContext) -> +-spec attempt_request_object(QueryParams, ClientContext, UrlExtension) -> {ok, QueryParams} | {error, error()} when QueryParams :: oidcc_http_util:query_params(), + UrlExtension :: oidcc_http_util:query_params(), ClientContext :: oidcc_client_context:t(). -attempt_request_object(QueryParams, #oidcc_client_context{ - client_id = ClientId, - client_secret = ClientSecret, - client_jwks = ClientJwks, - provider_configuration = #oidcc_provider_configuration{ - issuer = Issuer, - request_parameter_supported = true, - require_signed_request_object = RequireSignedRequestObject, - request_object_signing_alg_values_supported = SigningAlgSupported0, - request_object_encryption_alg_values_supported = EncryptionAlgSupported0, - request_object_encryption_enc_values_supported = EncryptionEncSupported0 +attempt_request_object( + QueryParams, + #oidcc_client_context{ + client_id = ClientId, + client_secret = ClientSecret, + client_jwks = ClientJwks, + provider_configuration = #oidcc_provider_configuration{ + issuer = Issuer, + request_parameter_supported = true, + require_signed_request_object = RequireSignedRequestObject, + request_object_signing_alg_values_supported = SigningAlgSupported0, + request_object_encryption_alg_values_supported = EncryptionAlgSupported0, + request_object_encryption_enc_values_supported = EncryptionEncSupported0 + }, + jwks = Jwks }, - jwks = Jwks -}) when ClientSecret =/= unauthenticated -> + UrlExtension +) when ClientSecret =/= unauthenticated -> SigningAlgSupported = case SigningAlgSupported0 of undefined -> []; @@ -315,7 +320,7 @@ attempt_request_object(QueryParams, #oidcc_client_context{ <<"exp">> => os:system_time(seconds) + 30, <<"nbf">> => os:system_time(seconds) - MaxClockSkew }, - maps:from_list(QueryParams) + maps:from_list(QueryParams ++ UrlExtension) ), Jwt = jose_jwt:from(Claims), @@ -334,17 +339,25 @@ attempt_request_object(QueryParams, #oidcc_client_context{ ) of {ok, EncryptedRequestObject} -> - {ok, [{<<"request">>, EncryptedRequestObject} | essential_params(QueryParams)]}; + {ok, + [{<<"request">>, EncryptedRequestObject} | essential_params(QueryParams)] ++ + UrlExtension}; {error, no_supported_alg_or_key} -> - {ok, [{<<"request">>, SignedRequestObject} | essential_params(QueryParams)]} + {ok, + [{<<"request">>, SignedRequestObject} | essential_params(QueryParams)] ++ + UrlExtension} end end; -attempt_request_object(_QueryParams, #oidcc_client_context{ - provider_configuration = #oidcc_provider_configuration{require_signed_request_object = true} -}) -> +attempt_request_object( + _QueryParams, + #oidcc_client_context{ + provider_configuration = #oidcc_provider_configuration{require_signed_request_object = true} + }, + _UrlExtension +) -> {error, request_object_required}; -attempt_request_object(QueryParams, _ClientContext) -> - {ok, QueryParams}. +attempt_request_object(QueryParams, _ClientContext, UrlExtension) -> + {ok, QueryParams ++ UrlExtension}. -spec attempt_par(QueryParams, ClientContext, Opts) -> {ok, QueryParams} | {error, error()} diff --git a/test/oidcc_authorization_test.erl b/test/oidcc_authorization_test.erl index 8eea1db..9a68a3e 100644 --- a/test/oidcc_authorization_test.erl +++ b/test/oidcc_authorization_test.erl @@ -197,7 +197,7 @@ create_redirect_url_with_request_object_test() -> {ok, Url} = oidcc_authorization:create_redirect_url(ClientContext, #{ redirect_uri => RedirectUri, - url_extension => [{<<"should_be_in">>, <<"query_string">>}] + url_extension => [{<<"should_be_in">>, <<"both">>}] }), ?assertMatch(<<"https://my.provider/auth?request=", _/binary>>, iolist_to_binary(Url)), @@ -215,7 +215,7 @@ create_redirect_url_with_request_object_test() -> <<"redirect_uri">> := <<"https://my.server/return">>, <<"response_type">> := <<"code">>, <<"scope">> := <<"openid">>, - <<"should_be_in">> := <<"query_string">>, + <<"should_be_in">> := <<"both">>, <<"request">> := _ }, QueryParams @@ -241,7 +241,8 @@ create_redirect_url_with_request_object_test() -> <<"nbf">> := _, <<"redirect_uri">> := RedirectUri, <<"response_type">> := <<"code">>, - <<"scope">> := <<"openid">> + <<"scope">> := <<"openid">>, + <<"should_be_in">> := <<"both">> } }, Jwt