fix: Identity providers should not be hidden from login page when the user goes back from the reset credentials page by vilmosnagy · Pull Request #48814 · keycloak/keycloak

vilmosnagy · 2026-05-07T22:30:44Z

Reopens #33254, closes #33204.

First of all, sorry for disappearing — a newborn arrived ~1.5 years ago and I never found the time to get back to this. Big thanks to @mposolda for keeping the previous PR open as long as he did.

I rebased the commits from #33254 and verified that the tests from last year still fail against the current main branch, so the issue is still present and the fix is still needed. I also tried to address the review discussion from last year. Specifically:

Cleared the RESET_CREDENTIAL_USER_CHOSEN auth note after calling context.clearUser(), so that subsequent renders of the login page within the same auth session don't needlessly clear the user again.
Added ResetPasswordTest::resetPasswordEmailLinkWorksAfterNavigatingBackToLoginPage to cover the scenario @mposolda raised: the user initiates a password reset, navigates back to the login page (which now clears the user from the auth session), and then clicks the email link. The test verifies the reset flow still completes successfully — it seems that a user who navigates back and then clicks the email link can still change their password and get logged in, which appears fine from a security standpoint. This test was roughly half AI-generated and half hand-reworked, using the tests I wrote for the previous PR as reference — all other code in this PR is hand-written from last year.

What the fix does

When a user submits a username on the "Forgot Password?" page, Keycloak sets that user on the authentication session. If the user then navigates back to the login page without completing the reset flow, Keycloak was showing only the identity providers linked to that specific user's account — leaking which social providers they had connected, which is a user enumeration vulnerability per the OWASP Forgot Password Cheat Sheet.

The fix detects the RESET_CREDENTIAL_USER_CHOSEN auth note in UsernamePasswordForm and clears the user from context before rendering the login page, so all configured IDPs are shown regardless of which user was identified during the reset flow.

vilmosnagy · 2026-05-08T21:13:46Z

Is it possible that the failing test (PasskeysUsernameFormTest::passwordLoginWithNonDiscoverableKey) is a flaky one? On my machine it passes both from Jetbrains IDEA, and with running ./mvnw test -f tests/webauthn/pom.xml

If it's not flaky, can you help me reproduce the failure?

Apart from that I fixed all failing tests.

vilmosnagy · 2026-05-12T07:05:40Z

After opening this PR, I worked on another PR that initially failed CI because I added tests to the old test framework instead of the new one. The tests in this PR were originally rebased from my ~2-year-old unfinished PR. Given that, do you think I should rewrite these newly added tests using the new test framework instead? I’d rather not migrate the existing tests that I only had to touch because the flow changed slightly, since that would make it harder to follow their lifecycle/history.

What do you think, @mabartos? I'm happy to do that if it would help.

mabartos · 2026-05-12T07:14:08Z

@vilmosnagy Thanks for the PR! The old testsuite is deprecated, and we should not add another tests there if possible. Please try to find a relevant test in the new testsuite, and add the additional test cases. If there's none, it'd be good to create a new one there with the same/similar name, and the rest of the test cases will be migrated afterwards.

I'll review the core fix logic introduced in this PR later.

vilmosnagy · 2026-05-12T21:32:52Z

Please try to find a relevant test in the new testsuite, and add the additional test cases. If there's none, it'd be good to create a new one there with the same/similar name, and the rest of the test cases will be migrated afterwards.

@mabartos I migrated all the newly added tests to the new test framework. We should squash all the commits at the end for simplicity, but I tried to be as detailed as possible in this phase. I didn't find a one-to-one mapping between the old test suite and the new test framework's files, so I created a ResetPasswordTest and added all 3 tests there. This seems logical to me since all three tests cover different edge cases of the reset password flow - but if they belong elsewhere, please advise.

The commits are currently in this order:

Original failing test in the old test suite - this was my original bug report
All tests migrated to the new test framework. Both tests from the first commit and tests added during review are included here. No main code is touched, so you can easily verify that these tests would fail on the main branch
Fix for the issue
Fixing pre-existing tests - my commit fixed the way the test setup was breaking Keycloak, but the same breakage could be achieved through different user paths.
Addressing the review from last year's PR and adding a test to the old test suite - this is migrated to the new test framework in commit 2
Removing all additions from the old test suite

vilmosnagy · 2026-05-28T22:05:26Z

Should I do anything more here @mabartos ? Should I regurarly rebase this branch?

mabartos · 2026-05-29T12:19:41Z

@vilmosnagy Sorry, didn't have time to look at it yet. Could you please rebase once again? Seems these CI failures cannot be resolved by retries.

vilmosnagy · 2026-05-29T12:27:44Z

let me try. as far as I remember they passed before my last change - and my last change only migrated the new tests to the new testframework.

vilmosnagy · 2026-05-29T20:47:26Z

@mabartos I think the failing test OrganizationCacheTest::testBoundedDomainNamesInCache is a false-positive failure - it runs on my machine, and I should not have touched anything around organization caching. Could you check, pls?

vilmosnagy · 2026-06-10T17:18:08Z

@mabartos I rebased this PR, and my understanding about the tests:

CodeQL has some authentication error, says Requires authentication, I think it's unrelated to my changes - or if it's related, pls help me resolve
Infinispan IT seems to have some timeout related issue: I/O Error [Unable to pull 'quay.io/infinispan/server:16.0.12' from registry 'quay.io' : {"message":"Get \"[https://quay.io/v2/\](https://quay.io/v2//)": context deadline exceeded"}

Could you please take a look? Thanks.

Signed-off-by: Vilmos Nagy <me@vnagy.eu>

…work. Signed-off-by: Vilmos Nagy <me@vnagy.eu>

Signed-off-by: Vilmos Nagy <me@vnagy.eu>

…ehaviour Signed-off-by: Vilmos Nagy <me@vnagy.eu> # Conflicts: # testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/RegisterTest.java

@mposolda

- Clear the RESET_CREDENTIAL_USER_CHOSEN auth note after clearing the user, so subsequent login page renders do not redundantly clear again (feedback from @mposolda in keycloak#33254) - Add ResetPasswordTest::resetPasswordEmailLinkWorksAfterNavigatingBackToLoginPage to verify that the password reset email link still works after the user navigates back to the login page, covering the scenario raised in the review discussion of keycloak#33254 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Vilmos Nagy <me@vnagy.eu>

They were already rewritten to the new test framework with the same name. Signed-off-by: Vilmos Nagy <me@vnagy.eu>

mabartos

LGTM, thanks!

@rmartinc Do you want to review this small PR as well that avoids some sort of user enumeration? Thanks!

Case:

If the login page changes (for instance, if one or more identity providers become hidden), this indicates that the tested username exists in the realm (and only linked the visible identity providers to their account).

rmartinc

Thanks @vilmosnagy and @mabartos! LGTM!

@mposolda

… user goes back from the reset credentials page (keycloak#48814) * keycloak#33204 add the failing test Signed-off-by: Vilmos Nagy <me@vnagy.eu> * keycloak#33204 Migrated tests from the old testsuite to the new framework. Signed-off-by: Vilmos Nagy <me@vnagy.eu> * keycloak#33204 possible fix (?) Signed-off-by: Vilmos Nagy <me@vnagy.eu> * keycloak#33204 refactor the previous tests to work with the changed behaviour Signed-off-by: Vilmos Nagy <me@vnagy.eu> # Conflicts: # testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/RegisterTest.java * keycloak#33204 add test and address review feedback from keycloak#33254 - Clear the RESET_CREDENTIAL_USER_CHOSEN auth note after clearing the user, so subsequent login page renders do not redundantly clear again (feedback from @mposolda in keycloak#33254) - Add ResetPasswordTest::resetPasswordEmailLinkWorksAfterNavigatingBackToLoginPage to verify that the password reset email link still works after the user navigates back to the login page, covering the scenario raised in the review discussion of keycloak#33254 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Vilmos Nagy <me@vnagy.eu> * Remove the tests added for this PR from the old testsuite. They were already rewritten to the new test framework with the same name. Signed-off-by: Vilmos Nagy <me@vnagy.eu> --------- Signed-off-by: Vilmos Nagy <me@vnagy.eu> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>

vilmosnagy requested review from a team as code owners May 7, 2026 22:30

keycloak-github-bot Bot added team/cloud-native team/core-clients team/core-iam labels May 7, 2026

vilmosnagy force-pushed the vilmos/issue_33204_v2 branch 3 times, most recently from 0d11bb6 to abaabbc Compare May 8, 2026 19:14

vilmosnagy mentioned this pull request May 8, 2026

Identity provider should not be hidden from login page when the a user goes back from the reset credentials page #33254

Closed

mabartos self-requested a review May 11, 2026 16:22

vilmosnagy force-pushed the vilmos/issue_33204_v2 branch 2 times, most recently from e33dc33 to 14a22aa Compare May 12, 2026 21:24

stianst removed team/cloud-native team/core-clients labels May 21, 2026

vilmosnagy force-pushed the vilmos/issue_33204_v2 branch from 14a22aa to 6c6946b Compare May 29, 2026 12:26

vilmosnagy force-pushed the vilmos/issue_33204_v2 branch from 6c6946b to d6548c9 Compare June 3, 2026 06:36

vilmosnagy force-pushed the vilmos/issue_33204_v2 branch from d6548c9 to ef96355 Compare June 10, 2026 16:01

vilmosnagy and others added 6 commits June 16, 2026 22:50

keycloak#33204 add the failing test

63d6e44

Signed-off-by: Vilmos Nagy <me@vnagy.eu>

keycloak#33204 Migrated tests from the old testsuite to the new frame…

f19d8a1

…work. Signed-off-by: Vilmos Nagy <me@vnagy.eu>

keycloak#33204 possible fix (?)

3e50b57

Signed-off-by: Vilmos Nagy <me@vnagy.eu>

keycloak#33204 refactor the previous tests to work with the changed b…

5ba5101

…ehaviour Signed-off-by: Vilmos Nagy <me@vnagy.eu> # Conflicts: # testsuite/integration-arquillian/tests/base/src/test/java/org/keycloak/testsuite/forms/RegisterTest.java

Remove the tests added for this PR from the old testsuite.

4d3286c

They were already rewritten to the new test framework with the same name. Signed-off-by: Vilmos Nagy <me@vnagy.eu>

vilmosnagy force-pushed the vilmos/issue_33204_v2 branch from ef96355 to 4d3286c Compare June 16, 2026 20:50

keycloak-github-bot Bot added the flaky-test label Jun 16, 2026

keycloak-github-bot Bot mentioned this pull request Jun 16, 2026

Flaky test: org.keycloak.testsuite.saml.SamlClientTest#testLoginWithOIDCClient #49985

Closed

mabartos approved these changes Jun 17, 2026

View reviewed changes

rmartinc approved these changes Jun 19, 2026

View reviewed changes

rmartinc merged commit 72c3f6c into keycloak:main Jun 19, 2026
88 checks passed

Navigation Menu

Uh oh!

fix: Identity providers should not be hidden from login page when the user goes back from the reset credentials page - #48814

fix: Identity providers should not be hidden from login page when the user goes back from the reset credentials page#48814
rmartinc merged 6 commits into
keycloak:mainfrom
vilmosnagy:vilmos/issue_33204_v2

vilmosnagy commented May 7, 2026

Uh oh!

vilmosnagy commented May 8, 2026

Uh oh!

vilmosnagy commented May 12, 2026

Uh oh!

mabartos commented May 12, 2026

Uh oh!

vilmosnagy commented May 12, 2026

Uh oh!

vilmosnagy commented May 28, 2026

Uh oh!

mabartos commented May 29, 2026

Uh oh!

vilmosnagy commented May 29, 2026

Uh oh!

vilmosnagy commented May 29, 2026

Uh oh!

vilmosnagy commented Jun 10, 2026

Uh oh!

mabartos left a comment •

edited

Loading

Uh oh!

rmartinc left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

vilmosnagy commented May 7, 2026

What the fix does

Uh oh!

vilmosnagy commented May 8, 2026

Uh oh!

vilmosnagy commented May 12, 2026

Uh oh!

mabartos commented May 12, 2026

Uh oh!

vilmosnagy commented May 12, 2026

Uh oh!

vilmosnagy commented May 28, 2026

Uh oh!

mabartos commented May 29, 2026

Uh oh!

vilmosnagy commented May 29, 2026

Uh oh!

vilmosnagy commented May 29, 2026

Uh oh!

vilmosnagy commented Jun 10, 2026

Uh oh!

mabartos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rmartinc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mabartos left a comment •

edited

Loading