Skip to content
This repository has been archived by the owner on Mar 11, 2021. It is now read-only.

User's emails should not be updated without verification #62

Closed
alexeykazakov opened this issue Aug 18, 2017 · 7 comments · Fixed by #227
Closed

User's emails should not be updated without verification #62

alexeykazakov opened this issue Aug 18, 2017 · 7 comments · Fixed by #227

Comments

@alexeykazakov
Copy link
Contributor

Currently users can change email without verification

@burrsitis
Copy link

For now, can we just disable the field?

@alexeykazakov
Copy link
Contributor Author

Sure we can. @burrsitis do you think it's better not to allow to change the email at all until we start verifying emails?

@joshuawilson
Copy link
Member

I would think so. We don't want users changing to fake email addresses.

@alexeykazakov
Copy link
Contributor Author

  1. Add new fields to Users table + user payload: email_verified
  2. Set email_verified to true when user account is created
  3. When the existing user logs in update the Keycloak if needed (email, name, company, approved, etc). The Auth DB is the source of truth now. Not Keycloak. So, if user updates anything in Keycloak directly (email, approved, etc) it won't affect anything. Auth API should be used for that.
    • If KC profile should be updated because something is not in sync (especially if some critical fields/claims like email & approved). Then update KC and then refresh the access token so we have a new token with updated claims. Return this refreshed token instead of the original own to the client.
  4. When email is changed via Auth API then:
    • Update email in Auth.
    • Set email_verified to false.
    • Update email in Keycloak.
    • Generate some random code (string). Save this code in a new table. Send user an email with a link to verify the email: https://auth.openshift.io/api/verifyemail?code={generatedCode} or something like this.
    • When the user clicks on the link and the new verifyemail endpoint sets the email_verified to true

Later on we should think about some sync between RHD and OSIO accounts (names, emails, etc).

@sbose78
Copy link
Member

sbose78 commented Dec 14, 2017

So, if user updates anything in Keycloak directly (email, approved, etc) it won't affect anything. Auth API should be used for that.

So a lower priority task should be that -
When a user does a re-login ( not a first time user ) and he had updated his email/approved value directly in keycloak, we will not be sync-ing from keycloak --> auth-db. Correct ?
https://github.com/fabric8-services/fabric8-auth/blob/master/login/service.go#L785

Edit: Or maybe should be done together in the same PR so as to not introduce a hole.

@alexeykazakov
Copy link
Contributor Author

We should sync. But use Auth as the source of truth. We should update KC user based on what we have in Auth. We have to update because we need updated info in the token.

  1. Get the token from KC
  2. Check if it matches what we have in Auth.
  3. If it doesn’t then update KC
  4. Refresh the token (so it now updated)
  5. Return the updated token

@sbose78
Copy link
Member

sbose78 commented Dec 14, 2017

Understood, so we change the direction of sync! :)

@sbose78 sbose78 mentioned this issue Dec 14, 2017
5 tasks
@alexeykazakov alexeykazakov added this to the Sprint 142-2 milestone Dec 19, 2017
aslakknutsen pushed a commit to fabric8-services/fabric8-notification that referenced this issue Dec 22, 2017
A caller can add custom variables that get rendered as part of the notification as
well as add Notification Type level validators of request data.

Request data.attributes.custom.* is made available to the template.

Include basic mail template for fabric8-auth email verification support.

Related to fabric8-services/fabric8-auth#62
sbose78 added a commit that referenced this issue Jan 4, 2018
When user updates email address, an email with verification URL is sent to the email.

fixes #62
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants