search

labbsr0x / whisper

A cloud-native Identity and OAuth Provider implemented with Golang and ORY Hydra
MIT License
12 stars 4 forks source link

Update User Credential Bug #33

Closed claudiosegala closed 4 years ago

claudiosegala commented 4 years ago

@abilioesteves in whisper we have this function bellow that updates the user email. Should we let the user update the email?

https://github.com/labbsr0x/whisper/blob/c940534789f77d04116d59b8ec5d98a286a7e1c0/db/usercredentials.go#L113-L132

Another thing is that I believe that the function name is not mnemonic. Since it will create another salt regardless of what you are trying to update, meaning that this function is intended to update the password.

PS: I did not see that it would update the salt every time and it gave me a headache in #5.

eabili0 commented 4 years ago

@abilioesteves in whisper we have this function bellow that updates the user email. Should we let the user update the email?

Yes, we should. The username, though, I think is not wise. The email confirmation flow should be in place after an update to the user's email.

Another thing is that I believe that the function name is not mnemonic. Since it will create another salt regardless of what you are trying to update, meaning that this function is intended to update the password.

This function is intended to update user credential information. That includes passwords. We could change it to identify if the password has changed, and only then calculate a new salt.

@claudiosegala

eabili0 commented 4 years ago

@claudiosegala, just noticed that the UpdateUserCredential does not invalidate the email if it changes its value. Please, could you fix that in this issue?

In other words, we need to verify the new provided email with the same validation logic we use at registration.

claudiosegala commented 4 years ago

@abilioesteves it would be interesting to log out the user if we change his email.

eabili0 commented 4 years ago

@claudiosegala, we should implement the logout following Hydra's guidelines: https://www.ory.sh/docs/hydra/oauth2#logout

eabili0 commented 4 years ago

@claudiosegala another option is to just revoke the login sessions: https://www.ory.sh/docs/hydra/oauth2#revoking-consent-and-login-sessions

claudiosegala commented 4 years ago

@abilioesteves should I create another issue for implementing this logout?

eabili0 commented 4 years ago

Yes, please.

claudiosegala commented 4 years ago

@abilioesteves should I merge this function with the new version of the update function?

https://github.com/labbsr0x/whisper/blob/369eed1e2185bcc4ac021cb589dd31692642c970/db/usercredentials.go#L99-L108

But if I merge, I would be querying the db twice (once to get the default values and another to update), since it would be something like:

user, _ := dao.db.GetUserCredential(username)

err := dao.db. UpdateUserCredential(user.Username, user.Password, user.Email, true)
eabili0 commented 4 years ago

@claudiosegala, no need to merge them... this way, the validation logic can evolve on its own.

claudiosegala commented 4 years ago

@claudiosegala another option is to just revoke the login sessions: https://www.ory.sh/docs/hydra/oauth2#revoking-consent-and-login-sessions

@abilioesteves the documentation says:

This endpoint is not compatible with OpenID Connect Front-/Backchannel logout and does not revoke any tokens.

Since we work with OpenID, should I worry?

eabili0 commented 4 years ago

@claudiosegala, we should implement the logout flow proposed by https://www.ory.sh/docs/hydra/oauth2#logout

I'll open an issue for that. Thanks!