Registration flow fixed

[vilmosnagy]

Closes #21514

Hi,

I've reported a bug in #17644. #19488 should have resolved it, but it seems to fail anyhow.

See my comment here: #19488 (comment)

@mposolda @hmlnarik I think we should implement a similar solution in the reset password flow to the idp linking flow's solution. As far as I remember in the IDP Linking flow the attributes of the newly created user is stored in an AuthNote - would you mind a solution, where:

• instead of the setUser call in ResetCredentialChooseUser:119 we store the user's ID (?) in an auth note
  • we should refactor some of the ResetCredentialChooseUser::authenticate because of this change
• in the ResetCredentialEmail we should use the previously stored AuthNote instead of the getUser method

What do you think about my suggestion? If I'd implement such changes in this PR would you merge it?

Regards,

[vilmosnagy]

@mposolda @hmlnarik could you help me with this issue? Do you agree with me that the user could still break the registration flow, and if so, what do you think about my proposed changes?

[vilmosnagy]

@mposolda @hmlnarik could you take a look at my changes pls? It should solve the bug I've reported, and all the tests pass.

[ahus1]

Thank you for preparing this fix, @vilmosnagy.

@sguilhen and I had a look and found that while your proposed fix prevents the error message being shown, it prevents the safety measures in AuthenticationProcessor#setAutheticatedUser() which check if the user has already been set.

I pushed one more commit to this PR, which now adds the same check before creating the user. This way, when a user uses the back navigation and attempts a registration, they will see an error message, and no user would be created.

I ran the UserStorageTest tests locally and they passed for me. @sguilhen and I also did a manual test, and it looked good to us. I'm now waiting for the CI to complete.

Please let me know your thoughts on this change before I go ahead and merge it.

Thanks!

[vilmosnagy]

@ahus1 I think with your rework this should be removed as well, but I'm not a 100% sure.

Suggested change

	*
	* If there was another user attached to this flow calling this method overrides the previous setting.

[vilmosnagy]

@ahus1 thanks for the review & rework, it looks good to me.

I've added a small suggestion - my original change would have changed the setUser function a bit. But with your rework the javadoc change is not needed.

Thanks again :)

[ahus1]

@sguilhen - could you please add your review to this issue? Once it has been reviewed, I'd then squash and merge it. I might also ask Marek P. for a review as he's involve in the core things.

Thanks!

[sguilhen]

Approving as this reflects what @ahus1 and I discussed last week about the proper way to fix this.

[ahus1]

@mposolda - this is now ready to be merged IMHO, both @sguilhen and me looked into this.
As this touches the "core" parts of Keycloak, I ask you or someone from your team for a review.

Once your review is in, I will squash and merge it.

Thanks!

[mposolda]

@ahus1 @vilmosnagy @sguilhen Thanks for the updates. I've did some testing of it and updated few things when looking into this. I've sent different PR #23064 with your commits and my commit on top of that. We can squash during merge possibly. In summary what I did was:

• Moved the changes to RegisterTest instead of UserStorageTest. As UserStorageTest is related to user storage, but this bug is not specific to user-storage though? In theory, if we later remove old store and possibly UserStorageTest, this test might be lost as it would be removed together with that. Also it allows to simplify test a bit IMO

• Added check also to RegistrationUserCreation.buildPage . I think that this improves usability. With the previous approach, the registration page was displayed and then after filling whole registration page with all the details, the error was displayed to the user. This is not very great for usability (especially in case when registration page contains lots of custom fields)

• Changed the message a bit to Action Expired. instead of Login timeout as there is not technically any timeout and hence Action Expired seems to me a bit more appropriate. Also added some details to the event. But these points are minor IMO.

WDYT about those changes? If you're ok with it, can we go with the alternative PR instead of this one?

[ahus1]

@mposolda - I really like the change you did so the registration page doesn't show up any more. Closing this one, and continuing the discussion on the other PR.