Repository navigation
Update password after email verification during registration of users - #47538
Conversation
ac261e6 to
f321cfb
Compare
f321cfb to
5b56b26
Compare
63a2dd2 to
097eca5
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.model.session.UserSessionProviderModelTest#testStreamsMarshallingKeycloak CI - Store Model Tests |
097eca5 to
3d4038a
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.model.session.UserSessionPersisterProviderTest#testUpdateAndRemoveKeycloak CI - Store Model Tests |
3d4038a to
1975684
Compare
Unreported flaky test detectedIf the flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR. org.keycloak.testsuite.model.session.UserSessionProviderOfflineModelTest#testLoadingOfflineClientSessionWhenCreatedBeforeSessionTimeKeycloak CI - Store Model Tests |
There was a problem hiding this comment.
Thanks a lot for this @mposolda! LGTM in general. I have just three comments. Two of them are easy and the other is a bit harder now you have everything setup (the change for verify email priority instead of the OTP and password). I prefer to have verify email with the highest priority but, if you think that it's a lot of work, the current order is OK because only Terms&Conditions and Verify Profile are higher.
| @@ -156,7 +156,7 @@ public static void addConfigureTotpAction(RealmModel realm) { | |||
| totp.setName("Configure OTP"); | |||
| totp.setProviderId(UserModel.RequiredAction.CONFIGURE_TOTP.name()); | |||
| totp.setDefaultAction(false); | |||
| totp.setPriority(10); | |||
| totp.setPriority(54); | |||
There was a problem hiding this comment.
Marek, would not be better to move the verify email action to the priority 1 (always the first to be presented). This way all the required actions are always after the verify email. It's only terms&Conditions and Update Profile with this change, so maybe it's enough. But I think that, if verify email is the first one, no action is never done at the registration side, everything is done at the verification email side.
Sorry for not thinking about this before.
There was a problem hiding this comment.
For this one, it seems to me that "Verify email" should be done after "Update profile" ?
Thinking that when user has both Update profile and Verify email required actions, it would makes sense that he first updates his profile and then email is verified as a subsequent step (as email was possibly updated by user during updating profile). That is also aligned with registration form (User in fact first registers, which pretty much updating his profile info and then email verification is a next step and then possibly password-update or other credentials update are another subsequent step). WDYT?
There was a problem hiding this comment.
Yes, as I commented the current PR order is OK. Verify email should be before any credential action, and that's OK now. About the update profile I didn't check that exact scenario, I just preferred the verify email to be first, but maybe you are right and update profile should be before verify email.
| String alwaysSetPasswordCfg = context.getAuthenticatorConfig() == null ? null : context.getAuthenticatorConfig().getConfig().get(ALWAYS_SET_PASSWORD_ON_REGISTER_FORM); | ||
| if ("true".equals(alwaysSetPasswordCfg)) return false; | ||
|
|
||
| return context.getRealm().isVerifyEmail(); |
There was a problem hiding this comment.
Probably we should also check if the required action for verify_email is set as default true. Just in case the verify email is enabled using that option instead of the realm.
There was a problem hiding this comment.
Updated, Thanks for the suggestion! Added automated test for this - introduced RegisterWithEmailVerificationTest and added to the new testsuite (with some more RegisterTest related to email verification moved).
|
|
||
| If you still prefer to keep password on the initial registration form and make email verification to be done once user self-registers with his password, you can | ||
| setup the configuration option on the <<_authentication-flows,Registration authentication flow>>. It can be done in the admin console by following tab *Authentication* -> | ||
| flow *registration* (or other flow, which you bind as registration flow) -> Settings of *Password validation* -> Enable *Always set password on register form* | BD0
There was a problem hiding this comment.
Maybe mention that this option is deprecated and probably removed in a future version?
There was a problem hiding this comment.
Updated, Thanks for the suggestion!
1975684 to
f84dc0b
Compare
closes keycloak#45568 Signed-off-by: mposolda <mposolda@gmail.com>
f84dc0b to
6b65c9e
Compare
…l to the new testsuite Signed-off-by: mposolda <mposolda@gmail.com>
6b65c9e to
5b9eb6f
Compare
…keycloak#47538) closes keycloak#45568 Signed-off-by: mposolda <mposolda@gmail.com>
closes #45568
For the case when both registration and verify-email is enabled, this PR updates the behavior:
Few more clarifications:
When the "verify email" link is opened in the new browser, then after the successful email verification in the new browser, the original authentication session in the browser1 is removed. This is for security reasons. If there are any follow-up required actions, they can be performed in the browser2 after email verification, but there is not redirection to the client application after all required actions are finished (
END_AFTER_REQUIRED_ACTIONSis still set in the browser2 authentication session similarly like before). The PR does not contain removal of all user's authentication sessions after email is verified. This should be added as a follow-up to this issue.For the backwards compatibility, I've added switch
Always set password on register formonPassword validationas maybe the new behavior is not desired for everyone and want to update issues with backwards compatibility.