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

Support + as the scope separator in OIDC authorize URL #5591

Merged
merged 2 commits into from
Jan 11, 2023

Conversation

leleuj
Copy link
Contributor

@leleuj leleuj commented Jan 10, 2023

When doing integration tests between the pac4j OIDC client (6.0.0-RC5-SNAPSHOT) and the CAS server v7.0.0-SNAPSHOT for an OIDC service with approval, I noticed that the scopes were not properly taken into account.

This is due to the + between the scopes in the /authorize URL instead of %20.

My feeling is that it's better to use %20, but it may be interesting to support + as well for backward compatibility.

I can of course patch pac4j v6.0.0-RC5-SNAPSHOT (I already did that locally) if you prefer not to fix it on the CAS server side.

To demonstrate the problem, this PR adds 2 Puppeteer tests: one with %20 which passes and one with + which fails.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@apereocas-bot apereocas-bot added this to the 7.0.0-RC4 milestone Jan 10, 2023
@leleuj leleuj changed the title Support + as the scope separator in URL Support + as the scope separator in OIDC authorize URL Jan 10, 2023
@mmoayyed mmoayyed merged commit e93bb5d into apereo:master Jan 11, 2023
@leleuj leleuj deleted the scopeplussep branch January 11, 2023 06:32
@mmoayyed
Copy link
Member

While this is reasonable to fix and accept, I think the correct expectation is one that requires all parameter values to be properly encoded. Note that scopes, per the spec, should be required to be separated by a space, or %20. Using a ‍‍‍+‍‍ might not be super compliant, as is not a comma.

If you have not done so already, I would also vote for allowing pac4j to perhaps handle both, with a strong preference for %20.

@leleuj
Copy link
Contributor Author

leleuj commented Jan 11, 2023

Sure: pac4j/pac4j@5479eef

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants