fix: url_extension params also go in the request object

paulswartz commented 5 months ago

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.

~~The test failures in Zitadel seem unrelated, as I also see them on main; @maennchen any ideas there?~~

maennchen commented 5 months ago

@paulswartz I'm wondering if this is the correct course of actions. The parameter is called url_extension. Therefore it should go into the url.

I also haven't seen anywhere the explicitly forbids adding URL parameters when using request objects.

Maybe we have to introduce a new parameter?

That do you think?

coveralls commented 5 months ago

Pull Request Test Coverage Report for Build 189

Details

3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.2%) to 92.108%

Totals
Change from base Build 177:	0.2%
Covered Lines:	1062
Relevant Lines:	1153

💛 - Coveralls

paulswartz commented 5 months ago

Thanks for fixing the tests!

I dug back into the spec:

https://openid.net/specs/openid-connect-core-1_0.html#RequestObject

The request Authorization Request parameter enables OpenID Connect requests to be passed in a single, self-contained parameter and to be optionally signed and/or encrypted. It represents the request as a JWT whose Claims are the request parameters specified in Section 3.1.2. This JWT is called a Request Object. [...] When the request parameter is used, the OpenID Connect request parameter values contained in the JWT supersede those passed using the OAuth 2.0 request syntax. However, parameters MAY also be passed using the OAuth 2.0 request syntax even when a Request Object is used; this would typically be done to enable a cached, pre-signed (and possibly pre-encrypted) Request Object value to be used containing the fixed request parameters, while parameters that can vary with each request, such as state and nonce, are passed as OAuth 2.0 parameters.

I'm thinking that the solution is to send the url_extension parameters both in the URL and the request object: what do you think?

maennchen commented 5 months ago

@paulswartz Agreed. Let‘s do that 👍

coveralls commented 5 months ago

Pull Request Test Coverage Report for Build 190

Details

6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
1 unchanged line in 1 file lost coverage.
Overall coverage increased (+0.09%) to 92.021%

Files with Coverage Reduction	New Missed Lines	%
src/oidcc_authorization.erl	1	94.06%
<!--	Total:	1		-->

Totals
Change from base Build 177:	0.09%
Covered Lines:	1061
Relevant Lines:	1153

💛 - Coveralls

paulswartz commented 5 months ago

@maennchen this is ready for review now. I've tested it with the team that was experiencing the original issue and this addresses their issue.

erlef / oidcc