Repository navigation
Conversation
There was a problem hiding this comment.
@marsluca, should UpdateTotp get the same move? It has the order this PR fixes in WebAuthnRegister, and I can hit it with a duplicate OTP label.
A user with one session in another browser and an OTP labelled phone sets up a second OTP, also labelled phone, with "Sign out from other devices" checked. The setup fails with "Device already exists with the same name", no second OTP is saved, and the other session is gone. With the box unchecked, the same attempt keeps both sessions.
In UpdateTotp.processAction, logoutOtherSessions runs before CredentialHelper.createOTPCredential, which can still return false or throw ModelDuplicateException. UpdatePassword also logs out before updateCredentialAndGet, which throws ModelException when the password breaks the policy. I only read that one, I didn't run it.
How I tested:
- Added three cases to
RequiredActionTotpSetupTeston this branch: log in from a second browser, set up an OTP labelledphone, then set up a second OTP throughCONFIGURE_TOTP. - Ran them with
-Pauth-server-quarkusand printed the session count and the result.
| Second OTP | Expected | Actual |
|---|---|---|
label tablet, sign-out checked (control) |
saved, other session ended | saved, 2 sessions to 1 |
label phone, sign-out unchecked (control) |
label error, 2 sessions | label error, 2 sessions |
label phone, sign-out checked |
label error, 2 sessions | label error, 2 sessions to 1 |
WebAuthn registration logged out the user's other sessions before it checked the attestation. When the registration failed, the other sessions were gone anyway. The logout now runs after the credential is created. The OTP setup and the password update had the same order: a duplicate OTP label or a password rejected by the policy still ended the other sessions. They now log out after the credential is saved too. Closes keycloak#54352 Signed-off-by: Luca Marsella <34103646+marsluca@users.noreply.github.com>
db680e9 to
cfd453e
Compare
|
@oleksii-udovychenko good catch, thanks. |
Closes #54352
WebAuthnRegister.processActionlogged out the other sessions before it parsed and verified the attestation, so a failed registration still ended them. I moved the logout to right aftercreateCredential, so it only runs once the passkey is saved. This also covers the passwordless register action, which extends this class.After the review I did the same in
UpdateTotpandUpdatePassword: a duplicate OTP label or a password rejected by the policy also ended the other sessions.Testing:
AppInitiatedActionWebAuthnTest#failedSetupWebAuthnKeepsOtherSessions: two sessions, "sign out other devices" checked, and an invalid attestation object, so the server rejects the registration. It checks that both sessions are still there. It fails on main (2 sessions become 1) and passes with the fix. It also runs inAppInitiatedActionPwdLessTestandAppInitiatedActionWebAuthnSkipIfExistsTest.tests/webauthnmodule (71 tests), locally.RequiredActionTotpSetupTest#failedTotpSetupKeepsOtherSessions(duplicate OTP label) andRequiredActionResetPasswordTest#rejectedPasswordKeepsOtherSessions(password shorter than the policy), both with the sign-out box checked. I ran both classes locally.AI-assisted (Claude Code).