Skip to content

Commit

Permalink
fix: url_extension params also go in the request object
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
paulswartz authored and maennchen committed Jun 12, 2024
1 parent 13f5f53 commit 580dd67
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 6 deletions.
6 changes: 3 additions & 3 deletions src/oidcc_authorization.erl
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -158,7 +157,8 @@ 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),
QueryParams8 = QueryParams7 ++ maps:get(url_extension, Opts, []),
{ok, QueryParams} ?= attempt_request_object(QueryParams8, ClientContext),
attempt_par(QueryParams, ClientContext, Opts)
end.

Expand Down
7 changes: 4 additions & 3 deletions test/oidcc_authorization_test.erl
Original file line number Diff line number Diff line change
Expand Up @@ -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">>, <<"object">>}]
}),

?assertMatch(<<"https://my.provider/auth?request=", _/binary>>, iolist_to_binary(Url)),
Expand All @@ -215,11 +215,11 @@ create_redirect_url_with_request_object_test() ->
<<"redirect_uri">> := <<"https://my.server/return">>,
<<"response_type">> := <<"code">>,
<<"scope">> := <<"openid">>,
<<"should_be_in">> := <<"query_string">>,
<<"request">> := _
},
QueryParams
),
?assert(maps:is_key(<<"should_be_in">>, QueryParams) =:= false),

{SignedToken, Jwe} = jose_jwe:block_decrypt(Jwks, maps:get(<<"request">>, QueryParams)),

Expand All @@ -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">> := <<"object">>
}
},
Jwt
Expand Down

0 comments on commit 580dd67

Please sign in to comment.