Require verification after email change by loay · Pull Request #2765 · strongloop/loopback

loay · 2016-09-20T15:11:07Z

connect to https://github.com/strongloop-internal/scrum-loopback/issues/1029

loay · 2016-09-22T21:46:49Z

@bajtos @raymondfeng
PTAL, this function updateEmailVerified
if I comment out line 714, it gets stuck and times out .. if I use this line, it gives the error Error: Callback was already called. and tests fail. thoughts?

bajtos · 2016-09-23T13:36:13Z

If you add console.log statements add the beginning of your two new functions deleteSessions and updateEmailVerified, then you will see they are repeatedly called in an infinite loop:

ctx.Model.updateAll triggers "before save"/"after save" hooks
there is a bug in the implementation where you treat missing data.email as an email change
as a result, the nested "before/after" save hooks thinks the email was changed and triggers another update of the flag

I would like you to address this complexly.

Create a new PR to fix the problem I described in Invalidate sessions after email change #2693 (comment) and that you haven't addressed in the pull request:

Also when performing a partial update, the email may not change at all. For example:
user.updateAttributes({age: user.age+1},cb);
You should skip the session-invalidation logic in such case too.

As for this pull request, I think it will be better to move the code resetting emailVerified flag to the "before save" hook. You already know whether the email is going to be changed at that point, thus you can modify the database query to reset the flag too. This will save us one database call.

bajtos

☝️

loay · 2016-09-23T14:00:28Z

Thanks @bajtos having a look

loay · 2016-09-28T19:33:10Z

@bajtos PTAL. Thanks

bajtos · 2016-09-29T07:13:34Z

As for this pull request, I think it will be better to move the code resetting emailVerified flag to the "before save" hook. You already know whether the email is going to be changed at that point, thus you can modify the database query to reset the flag too. This will save us one database call.

@loay AFAICT, you are still resetting emailVerified in after save hook. Did you try to reset it in before save, as I suggested? Were there any problems?

loay · 2016-09-30T02:06:01Z

yeah, it didn't work for me, as I would run to the same problem as before, either timeout or infinite loop, as the validations of newEmail are not in the before hook

 if (!ctx.instance && !ctx.data) return next();
var newEmail = (ctx.instance || ctx.data).email;
if (!newEmail) return next();

bajtos · 2016-10-05T12:33:50Z

yeah, it didn't work for me, as I would run to the same problem as before, either timeout or infinite loop, as the validations of newEmail are not in the before hook

What is stopping you from adding the validation to the "before save" hook? It should also improve the performance a bit, because there will be one database query less when email is not changed.

loay · 2016-11-04T15:43:04Z

@bajtos I moved the emailVerified part up to the before save function as requested. PTAL. Thanks.

bajtos

Almost there, @loay! I would like to you clean up the code a bit more, see my comments below.

bajtos · 2016-11-08T14:05:20Z

 });

+ describe('Verification after updating email', function() {
+ var nonVerifiedUser;


I find the variable name confusing. You are creating the user with emailVerified set to true and then store it in a variable called nonVerifiedUser.
Can you use a different name please? As far as I am concerned, user, userInstance or testUser are good enough. Feel free to come up with your own name though!

bajtos · 2016-11-08T14:06:34Z

 });
+ if (ctx.instance) {
+ var emailChanged = ctx.instance.email !== ctx.hookState.originalUserData[0].email;
+ if (!ctx.Model.settings.emailVerificationRequired) return next();


How about simplifying this and the following line to the followin?
if(emailChanged&&ctx.Model.settings.emailVerificationRequired){ctx.instance.emailVerified=false;}

bajtos · 2016-11-08T14:07:17Z

+ var emailChanged = ctx.hookState.originalUserData.some(function(data) {
+ return data.email != ctx.data.email;
+ });
+ if (!ctx.Model.settings.emailVerificationRequired) return next();


Same comment applies here.
if(emailChanged&&ctx.Model.settings.emailVerificationRequired){ctx.data.emailVerified=false;}

bajtos · 2016-11-08T14:09:16Z

+ User.settings.emailVerificationRequired = true;
+ async.series([
+ function createUser(next) {
+ User.create(originalMyEmailCred, function(err, specialInstance) {


AFAICT, this setup is shared by both tests. Please extract it into a shared beforeEach hook.
beforeEach(setupTestUser);it(...);it(...);functionsetupTestUser(done){User.create(originalMyEmailCred,function(err,inst){if(err)returndone(err);nonVerifiedUser=inst;// use a different name than nonVerifiedUserdone();});}

loay · 2016-11-09T05:27:00Z

@bajtos I have addressed the changes. PTAL. Thanks.

When the User model is configured to require email verification, then any change of the email address should trigger re-verification.

bajtos

I made few final coding style tweaks myself and force-pushed to your feature branch. The patch is good to be merged now.

bajtos · 2016-11-09T12:24:47Z

@loay landed, thank you. Please back-port to 2.x yourself.

loay added the #review label Sep 20, 2016

loay force-pushed the verifyEmail branch from 9d9eb16 to e565bffCompare September 22, 2016 21:39

loay assigned bajtos Sep 23, 2016

bajtos suggested changes Sep 23, 2016
View reviewed changes

bajtos assigned loay and unassigned bajtos Sep 23, 2016

loay force-pushed the verifyEmail branch from a2e2be4 to 782daffCompare September 28, 2016 17:34

loay assigned bajtos Sep 28, 2016

loay force-pushed the verifyEmail branch from 56cc640 to 8225eadCompare November 4, 2016 15:34

bajtos suggested changes Nov 8, 2016
View reviewed changes

loay force-pushed the verifyEmail branch from a9cfd6f to 5e90c02Compare November 9, 2016 05:20

bajtos force-pushed the verifyEmail branch from 5e90c02 to d7d465cCompare November 9, 2016 12:04

Require verification after email change
eb640d8
When the User model is configured to require email verification, then any change of the email address should trigger re-verification.

bajtos force-pushed the verifyEmail branch from d7d465c to eb640d8Compare November 9, 2016 12:06

bajtos approved these changes Nov 9, 2016
View reviewed changes

bajtos changed the title ~~Ensure verification for updated emails~~Require verification after email changeNov 9, 2016

bajtos merged commit 6b6ca95 into masterNov 9, 2016

bajtos deleted the verifyEmail branch November 9, 2016 12:24

bajtos removed the #review label Nov 9, 2016

loay mentioned this pull request Nov 9, 2016
Backport/Require verification after email change #2927
Merged

Uh oh!

Conversation

loay commented Sep 20, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

loay commented Sep 22, 2016

Uh oh!

bajtos commented Sep 23, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

loay commented Sep 23, 2016

Uh oh!

loay commented Sep 28, 2016

Uh oh!

bajtos commented Sep 29, 2016

Uh oh!

loay commented Sep 30, 2016

Uh oh!

bajtos commented Oct 5, 2016

Uh oh!

loay commented Nov 4, 2016

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

bajtosNov 8, 2016

Choose a reason for hiding this comment

Uh oh!

bajtosNov 8, 2016

Choose a reason for hiding this comment

Uh oh!

bajtosNov 8, 2016

Choose a reason for hiding this comment

Uh oh!

bajtosNov 8, 2016

Choose a reason for hiding this comment

Uh oh!

loay commented Nov 9, 2016

Uh oh!

bajtos left a comment

Choose a reason for hiding this comment

Uh oh!

bajtos commented Nov 9, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

loay commented Sep 20, 2016•
edited
Loading

bajtos commented Sep 23, 2016•
edited
Loading