Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: url_extension params also go in the request object #354

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

paulswartz
Copy link
Collaborator

@paulswartz paulswartz commented Jun 7, 2024

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
Copy link
Member

@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
Copy link

coveralls commented Jun 12, 2024

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 Coverage Status
Change from base Build 177: 0.2%
Covered Lines: 1062
Relevant Lines: 1153

💛 - Coveralls

@paulswartz
Copy link
Collaborator Author

Thanks for fixing the tests!

I dug back into the spec:

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
Copy link
Member

@paulswartz Agreed. Let‘s do that 👍

@paulswartz paulswartz marked this pull request as draft June 14, 2024 14:36
Originally done in erlef#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.
@coveralls
Copy link

coveralls commented Jun 17, 2024

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%
Totals Coverage Status
Change from base Build 177: 0.09%
Covered Lines: 1061
Relevant Lines: 1153

💛 - Coveralls

@paulswartz paulswartz marked this pull request as ready for review June 17, 2024 13:48
@paulswartz
Copy link
Collaborator Author

@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.

@maennchen maennchen merged commit 493b0d0 into erlef:main Jun 18, 2024
28 checks passed
@maennchen maennchen self-assigned this Jun 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants