Save Remember Me authNote when an organization scope is set#47023
Save Remember Me authNote when an organization scope is set#47023Esurnir wants to merge 3 commits intokeycloak:mainfrom
Conversation
|
Currently writing tests for the pull request. I'll reopen once the tests are written. |
d819b53 to
646bdfe
Compare
|
Tests added to OrganizationAuthenticationTests (those I'll admit I let claude create them). I ran those test with the pre-PR code and they fail, and only pass once the PR code is added. Those tests were written using Generative AI. For the tests I originally tried to use a similar test to the login test but unfortunately the behavior is different, the box doesn't seem pre-checked when the remember me flag is set. |
646bdfe to
3759e09
Compare
closes keycloak#47023 Signed-off-by: mposolda <mposolda@gmail.com>
|
Thanks, @Esurnir. LGTM. Running CI. |
3a8005c to
3eddf64
Compare
|
@Esurnir Added a commit to keep remember-me enabled in the login form after the session expires or is destroyed (not on logouts, as the current behavior does not keep the cookie). Also updated tests. |
3eddf64 to
1a8d354
Compare
2223e61 to
5120bdd
Compare
|
@Esurnir Would it be possible to create the new test case in the new test framework? In the Otherwise, the Testsuite Deprecation Check will always wail. Thanks! cc: @pedroigor |
I'll try my best ! There's a few tests I modified in the old arquilian based test. Should I : |
5120bdd to
206865b
Compare
Move testRememberMeWithOrganizationScope and testRememberMeNotCheckedWithOrganizationScope from the legacy Arquillian testsuite into tests/base under the new Keycloak Test Framework, addressing the Testsuite Deprecation Check raised on PR keycloak#47023. The new file uses the identity-first LoginUsernamePage for the username step and LoginPage (with username hidden) for the password step, the Events injection for login event assertions, and realm.admin().deleteSession for session expiry. A small awaitNextEvent helper retries events.poll() with a short backoff to handle the brief window between OAuth callback completion and login event persistence in the event store. The legacy OrganizationAuthenticationTest no longer contains those two methods. The AssertEvents @rule field and five imports it required (Details, EventRepresentation, UserSessionRepresentation, AssertEvents, org.junit.Rule) are removed since no remaining test in the legacy class uses them. Closes keycloak#47017 Co-authored-by: Pedro Igor <pigor.craveiro@gmail.com> Signed-off-by: Jean-Baptiste Zeller <esurnir@gmail.com>
|
@mabartos done. pushed in the latest commit. A couple of choices worth flagging:
Thanks for your guidance! cc: @pedroigor |
|
AI code reviewer claim the bug would still be there in a multi org flow. So I'm letting it run a tiny bit more before shipping the PR. |
|
Ok confirmed locally and fixed it (with gen ai assitance). A new test case has been added to cover the 'multi organization' flow so that remember me doesn't get cleared if the user go through the select-organization switch. The change become based on if the user has sent the rememberMe box inside the form or not. Tests done locally :
|
Modify the OrganizationAuthenticator to save the rememberMe note since it is the Authenticator receiving the username form request during organization enabled login flow. Prevent cleanup of Remember Me by AbstractUsernameFormAuthenticator if the checkbox is absent from the page. Add test case for OrganizationAuthenticationTest that check the session is created with the rememberMe flag active. Closes keycloak#47017 Signed-off-by: Jean-Baptiste Zeller <esurnir@gmail.com>
Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>
ecfdc30 to
22cdb65
Compare
OrganizationAuthenticator.action unconditionally called processRememberMe on every form submission. The select-organization.ftl form, posted by multi-org members during the org-selection step, does not carry the rememberMe field, so the call fell into AuthenticatorUtils.processRememberMe's else branch and removed the authNote previously saved by the same authenticator on the identity-first submission. Gate the call by presence of the rememberMe parameter, mirroring WebAuthnPasswordlessAuthenticator. Also use the local form variable in createLoginForm for consistency with the loginHint branch above. Migrate the existing Remember Me Organization scope tests to the new test framework, and add testRememberMePreservedThroughSelectOrganization covering the multi-org case (member of two organizations, picks one on the org selection page, verifies the user session still has rememberMe enabled). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Jean-Baptiste Zeller <esurnir@gmail.com>
22cdb65 to
14aa03d
Compare
Prevent cleanup of Remember Me by AbstractUsernameFormAuthenticator if the checkbox is absent from the page (which is the case when using organization flow).
This PR make the assumption that OrganizationAuthenticator will execute before AbstractUserNameFormAuthenticator when the organization scope is specified.
Generative AI was used during the writing of this pull request for the code. Originally it was just to figure out where the issue lied but since the fix it proposed (which I tested locally) worked. I figured I would make a pull request about it.
I made some adjustment to mimic the "remove auth note when rememberMe is absent". Otherwise it looked like the note would never get cleaned up from a session.
Closes #47017