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

Always restore query params on OIDC redirect URIs #25012

Merged
merged 1 commit into from
May 25, 2022

Conversation

sberyozkin
Copy link
Member

Fixes #24882.

This PR only makes sure that the custom query parameters submitted with the original request are restored when an authenticated user is redirected back to Quarkus.

Right now such query parameters are already restored but only if a somewhat obscure Quarkus specific feature is utilized, for example:

Request URI: /hello?a=b

while the redirect URI is configured as /callback, but only for the purpose of while listing the client in OIDC provider console since the redirect URI may have to be the same one for all the users for a given OIDC client, and when the user is redirected back to /callback, the user is finally redirected to /hello?a=b in order to restore the original request URI.

However if the user is just redirected back to /callback then the original ?a=b is lost.

So this PR makes it work for all the cases which also helps to use quarkus-oidc in cases where an endpoint would like to use the state to capture the original request details - right now quarkus-oidc does generate the state itself so the application endpoints don't have a chance to store the request specific details. This PR does not completely address this specific state customization point but we can follow up later with introducing an interface to make it more advanced.

@gsmet
Copy link
Member

gsmet commented Apr 26, 2022

@pedroigor any chance you could have a look? I'm releasing 2.9.0.CR1 tomorrow morning and I would prefer having it in the CR. Thanks.

@sberyozkin
Copy link
Member Author

sberyozkin commented Apr 26, 2022

Hey @pedroigor Just to clarify, what this PR is really about, it tries to address an issue such as #24882 by addressing this small disconnect that we have with quarkus-oidc to do with the fact that it really focuses on hiding away all the OIDC details from the custom endpoints, while, custom endpoints, would sometimes like to get a chance to generate their own state which captures the original request's state (encoded in the custom query for example) and then get these data out of the state in the callback. But we don't allow users to intercept the state creation.

I'm not sure if a different design for the custom endpoints can prevent the need to keep the original query around. For example, many applications, AFAIK, would get a user authenticated first and then start offering some options possibly via the incremental authorization. But it appears in some cases an original request can contain a query.

I stopped short of introducing some interface where users can generate this state - as we'd need to preserve it after a final redirect which actually drops all the query params returned from Keycloak/etc. Additionally, if PKCE is used, the state value is in fact an encrypted bean with a code verifier and I'd rather avoid introducing a hook into this process.

So this PR offers a simple enough way to store the original query in a state cookie and then add it to the final redirect URI.
Perhaps we can get it encrypted over time as well.

But I'm fine to keep discussing it if you are somewhat concerned, would be np at all.

@gsmet
Copy link
Member

gsmet commented May 9, 2022

@pedroigor ping

1 similar comment
@gsmet
Copy link
Member

gsmet commented May 19, 2022

@pedroigor ping

Copy link
Contributor

@pedroigor pedroigor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sberyozkin Sorry, LGTM :)

@sberyozkin
Copy link
Member Author

@pedroigor Cool, np at all :-).
I think we can look a bit further into it later on (having all query parameters stored directly in the cookie is not great), perhaps if the queries are kept then they can be treated the same way when a PKCE verifier is added, encrypted...

@sberyozkin sberyozkin merged commit 748908f into quarkusio:main May 25, 2022
@quarkus-bot quarkus-bot bot added this to the 2.10 - main milestone May 25, 2022
@sberyozkin sberyozkin deleted the oidc_restore_query_params branch May 25, 2022 09:23
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label May 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/oidc kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Passing Custom State Data for OIDC Auth Flows
3 participants