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

Do not make OIDC state cookie name unique if multiple code flows are not allowed #34784

Merged
merged 1 commit into from
Jul 17, 2023

Conversation

sberyozkin
Copy link
Member

Fixes #34760

Quarkus OIDC enables multiple authorization code flows (multi-tab authentication) by default - a user can start a login process and without completing it open a new tab and start a new one.

State cookies are used to coordinate a given authorization code flow and for multiple code flows to be supported their names have to be unique for the state cookies not to interfere with each other.

However if such a multi-tab authentication is disabled, the state cookie should not be unique - as it prevents whitelisting supported cookies as explained in #34760.

So this PR makes a very simple fix - if multiple code flows are not allowed, do not try to make a state cookie name unique. Tests are updated to show that by default the state cookie name contains UUID, but not if the multiple code flows are not allowed.

@sberyozkin sberyozkin requested a review from gastaldi July 17, 2023 10:04
@sberyozkin
Copy link
Member Author

OOM dev test test failure, I'll need to create another issue as it is not related to this PR

@sberyozkin
Copy link
Member Author

I've tried to cancel the workflow as one of the OIDC tests is indeed failing, sorry

@sberyozkin
Copy link
Member Author

sberyozkin commented Jul 17, 2023

The OIDC wiremock test failure is not related, it is a millisec related comparison:

2023-07-17T11:00:12.6787362Z [ERROR] io.quarkus.it.keycloak.CodeFlowAuthorizationTest.testCodeFlowUserInfo  Time elapsed: 0.256 s  <<< FAILURE!
2023-07-17T11:00:12.6788167Z org.opentest4j.AssertionFailedError: expected: <25200> but was: <25201>

I'll need to fix it first

@sberyozkin
Copy link
Member Author

Thanks @gastaldi, can you please check #34791, I'd like to get that one fixed first as that test is flaky

@sberyozkin sberyozkin force-pushed the oidc_state_cookie_without_uuid branch from eb26230 to 8007ddf Compare July 17, 2023 14:05
@quarkus-bot
Copy link

quarkus-bot bot commented Jul 17, 2023

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

@gastaldi gastaldi merged commit 3f3af25 into quarkusio:main Jul 17, 2023
@quarkus-bot quarkus-bot bot added the kind/enhancement New feature or request label Jul 17, 2023
@quarkus-bot quarkus-bot bot added this to the 3.3 - main milestone Jul 17, 2023
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.

Do not make state cookie name unique if OIDC multi-tab authentication is disabled
2 participants