Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KEYCLOAK-6455 Ability to require email to be verified before changing #7943

Merged
merged 20 commits into from May 9, 2022

Conversation

reda-alaoui
Copy link
Contributor

@reda-alaoui reda-alaoui commented Apr 18, 2021

Fix #11875

This PR adds an UPDATE_EMAIL action (enabled by default) that can be used as an AIA or a required action. The action is associated with a single email input form. If the realm has email verification disabled, this action will allow to update the email without verification. If the realm has email verification enabled, the action will send an email update action token to the new email address without changing the account email. Only the action token triggering will complete the email update.

In the account application personal info (Keycloak V2), this PR turns the email input field into a permanent readonly field. If the UPDATE_EMAIL action is enabled, an "Update Email" link will allow to trigger UPDATE_EMAIL action as an AIA. If the UPDATE_EMAIL action is disabled, there will be no link and therefore no way to update the email from the personal info page.

This PR conditionally removes the email field from login-update-profile.ftl form:

  • if the form is opened in a brokered identity context, the email field is kept
  • otherwise, the email field is removed

keycloak-1
keycloak-2
keycloak-3
keycloak-4
keycloak-5
keycloak-6
keycloak-7
keycloak-8

@jbman
Copy link

jbman commented Jun 17, 2021

  • if the form is opened in a brokered identity context, the email field is kept
  • otherwise, the email field is removed

Why should it be removed on a brokered context. It is still ok, to let a mapper copy the email value initially in a brokered identity context (Mapper mode "import"). Then it could be managed in the account at Keycloak including verification on updates.
Making any fields like email, first name or last name non-updateable is a different story and probably can not be solved within this PR.

@mabartos
Copy link
Contributor

@reda-alaoui Well done! Could you please resolve the conflicts and rebase it with master? Then we can make a review for this. And last thing, I see there are some failing tests; it'd be great to check it.

@reda-alaoui
Copy link
Contributor Author

Hi @mabartos ,
Sorry for my unresponsiveness.
I will fix the conflict and the test asap. I hope to do that this week.

@marziman
Copy link

@reda-alaoui many thanks for your work, mate.
This is actually an important PR. We really would need this for Security reasons.

Any chance you can please fix conflicts and tests?
That would be aweseome as it is open for some while now.

@stianst will this be possible to come into 16 or 17 release?

Thanks.

@reda-alaoui
Copy link
Contributor Author

reda-alaoui commented Nov 25, 2021

Hello,
I didn't forget this PR :)
But I didn't find the time yet. I hope to work on it by next week 🙏

@lajosthiel
Copy link

@marziman Would be really great to get this feature in the next release. I think it's one of the few major shortcomings of Keycloak at the moment because the current change email process is kind of error prone and a bit insecure. Would love to get this feature from core Keycloak and not have to implement my own workaround in the meantime just to ditch it again in a few months.

@marziman
Copy link

@lajosthiel I agree. We might be even forced to apply this now by hand to Keycloak.
@reda-alaoui let me know if we can help you in any matters. I think without your rebase and fixing tests, it wont be reviewed.
Your work is hihghly appreciated, and you see people are in line for this one :-)

BR Mehmet

@reda-alaoui reda-alaoui marked this pull request as draft December 1, 2021 15:41
@reda-alaoui
Copy link
Contributor Author

reda-alaoui commented Apr 27, 2022

@mposolda It is ready for review. I am polishing a separate PR for the documentation.

@reda-alaoui
Copy link
Contributor Author

Here is the documentation pull request

@reda-alaoui
Copy link
Contributor Author

@mposolda I added 2 commits that seemed important to me. Sorry for the noise.

@yoransabern
Copy link

Has anyone recreated this issue in github issues yet? Would like to follow the progression again now that the original redhat issue is closed https://issues.redhat.com/browse/KEYCLOAK-6455

@yoransabern
Copy link

Thats alright, I just wanted to make sure there's no duplicate or if it needed to be linked to this pr somehow. Anyway here's he new issue on github #11875

Copy link
Contributor

@mposolda mposolda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@reda-alaoui Thanks for the changes!

I am approving. I've just added one small note to the documentation PR and requested review from our documentation guru. However I hope that documentation will be finished soon and this will be merged soon.

@mposolda mposolda merged commit 5d87cdf into keycloak:main May 9, 2022
@reda-alaoui reda-alaoui deleted the email-update-confirmation branch May 9, 2022 16:53
@reda-alaoui
Copy link
Contributor Author

Thank you ! 🥳

@mposolda
Copy link
Contributor

mposolda commented May 9, 2022

@reda-alaoui Finally merged. Should be ready in Keycloak 19. Thanks for your patience and for your hard work on this issue!

@marziman
Copy link

marziman commented May 9, 2022

Many thanks @reda-alaoui for this hard work. And also thx @mposolda for the review.
Would be great to have it in KC v19.0.

@sparkmylife
Copy link

@reda-alaoui Thanks for your work, we have got the latest V19.0.1 for keycloak. This feature works fine when we enable update_email. But once we enable the "User Profile Enabled" from Realm General setting, we are getting error while confirming new email id on update.
image

@sparkmylife
Copy link

sparkmylife commented Aug 5, 2022

@reda-alaoui Also we find that when "User Profile Enabled" is disabled, first name and last name became empty, and on clicking the email validation link. It clears the user profile attributes which were not sent. Also, the error which is shown in the previous field is failing because the first name and last name are mandatory fields and they are not sent in the payload. Only email id should be updated instead of all fields getting updated.

@reda-alaoui
Copy link
Contributor Author

@sparkmylife please create new issues to report this kind of problems.

@sparkmylife
Copy link

@reda-alaoui Raised #13561

@megicivovic
Copy link

Hi all, do you maybe have any info, when will this PR be released? which Keycloak version?

@Guesch
Copy link

Guesch commented Aug 11, 2023

Hey there.

I'm using "email as username" (& enabled "Edit username" specifically for this); but when the users are changing their email using this feature, the username is not updated at all.
Considering that the "change email" button is specifically not displayed if "Edit username" is not enabled for those using "email as username", I reckon this is a bug isn't it?

Or did I miss any settings anywhere that would allow for this feature to properly change both email & username?

@pedroigor
Copy link
Contributor

@Guesch It should be fixed by #22393.

Can you please tell me in which context (Admin API, Update Profile, Account, etc) are you trying to update the e-mail?

@Guesch
Copy link

Guesch commented Aug 11, 2023

@Guesch It should be fixed by #22393.

Can you please tell me in which context (Admin API, Update Profile, Account, etc) are you trying to update the e-mail?

I have been using the Keycloak "Personal information update" UI. Going through all the steps expected from this PR (click on update email, form asking me what email I want to use, confirmation email with link sent, clicking on the link to validate the change). Email is properly changed but not the username.

We have our own Account Management page so eventually we will want to have it work outside of the KC default UI; but testing in the default UI is not conclusive already ;(

Happy to be told this is a misconfiguration on my end though!

@pedroigor
Copy link
Contributor

It looks more like a bug. Which version you are using?

@Guesch
Copy link

Guesch commented Aug 11, 2023

It looks more like a bug. Which version you are using?

v20.0.2 if I'm not mistaken

@pedroigor
Copy link
Contributor

It should be a bug then. I would ask you to try out 21.1.2 or wait for 22.0.2.

The PR I mentioned is fixing a nasty issue in 22.0.0 and 22.0.1 so better try out 22.0.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority/important Must be worked on very soon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ability to require email to be verified before changing