Uh oh!
There was an error while loading. Please reload this page.
Fix false emailVerified on user model update - #3053
Conversation
Yesterday, the loopback we are using in our system was upgraded via npm, and since the upgrade, we noticed that every time the user model updates, the emailVerified column would change to false. I took a look and realized there might be an error in strongloop@eb640d8 The intent of the commit just mention is to make emailVerified false when the email gets changed, but notice that ctx.data.email is null on updates, so the condition is always met and emailVerified always becomes false. This commit fixes the issue just mentioned.
slnode
commented
Dec 23, 2016
Can one of the admins verify this patch? |
3 similar comments
slnode
commented
Dec 23, 2016
Can one of the admins verify this patch? |
slnode
commented
Dec 23, 2016
Can one of the admins verify this patch? |
slnode
commented
Dec 23, 2016
Can one of the admins verify this patch? |
slnode
commented
Dec 23, 2016
Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test." |
| } else { | ||
| var emailChanged = ctx.hookState.originalUserData.some(function(data) { | ||
| return data.email != ctx.data.email; | ||
| return ctx.data.email && data.email != ctx.data.email; |
There was a problem hiding this comment.
It seems that with the falsy ctx.data.email, you don't even need to iterate over ctx.hookState.originalUserData
davidcheung
commented
Jan 4, 2017
@slnode ok to test |
davidcheung
commented
Jan 4, 2017
bajtos
commented
Jan 4, 2017
@yumenohosi thank you for the pull request, and thank you @davidcheung for the first round of review. I have assigned the patch to myself and I'll take a look in next few days. |
benbrown
commented
Jan 4, 2017
This also threw a major monkeywrench into our application and caused 2 days of lost progress. I think all of @loay's recent changes to the emailVerified process should be carefully reviewed. For example, it is NOT desirable to log users out of applications just because they change their email address. |
bajtos
commented
Jan 5, 2017
@benbrown thank you for chiming in, we are sorry for the difficulties created :(
Logging out users is important for security reasons. Consider the case when somebody hacks your email that you used when registering with a LoopBack-powered app. After you find this, you log into the app and change the email to a different one that wasn't hacked. Without session (access token) invalidation, the attacker would remain logged into your account and you would have no way how to log them out. Having said that, I agree that this makes the user experience less optimal. I am proposing to allow the end user to decide whether they want to log out other sessions or not, see #3071 |
bajtos
commented
Jan 5, 2017
Landed 🎉 Thank you @yumenohosi for your contribution! 👏 |
bajtos
commented
Jan 5, 2017
For posterity: I did not realise there were two commits in this pull request and I forgot to squash them before landing. My fault :( |
Description
Yesterday, the loopback we are using in our system was upgraded
via npm, and since the upgrade, we noticed that every time
the user model updates, the emailVerified column would change to false.
I took a look and realized there might be an error in eb640d8
The intent of the commit just mention is to make emailVerified false
when the email gets changed, but notice that ctx.data.email is null
on updates, so the condition is always met and emailVerified always
becomes false.
This commit fixes the issue just mentioned.
Related issues
Checklist
guide