Ability to expire email verify token #2216

cherukumilli · 2016-07-06T14:08:41Z

Notes for this PR:

Provides an additional setting / config variables
- emailVerifyTokenValidityDuration (not set by default)

if `emailVerifyTokenValidityDuration` is set to a number greater than `0` and `verifyUserEmails` is set to `true` then

Saves _email_verify_token_expires_at in the _User class along with the _email_verify_token at SignUp time
- sets _email_verify_token_expires_at = currentTime + emailVerifyTokenValidityDuration
If the user clicks on the email verification link BEFORE _email_verify_token_expires_at then
- update emailVerified to true in the User class (this code is already in place)
- delete the _email_verify_token and _email_verify_token_expires_at fields for this user (new code)
- respond back with a success (this code is already in place)
If the user completed email verification successfully but clicks on the link for second time then return an error message (i.e., fail gracefully)
If the user clicks on the email verification link AFTER the _email_verify_token_expires_at then
- respond back with an error: email verification token expired.
- user can try again and create a new account
  - deletion of rows in the _User class with expired verify email tokens is out of scope for this PR. Clean up should be handled by a job outside the normal auth flow.

Documentation updates

Updated README.md with the following:

   // if `verifyUserEmails` is `true` and
   //     if `emailVerifyTokenValidityDuration` is `undefined` then
   //        email verify token never expires
   //     else
   //        email verify token expires after `emailVerifyTokenValidityDuration`
   //
   // `emailVerifyTokenValidityDuration` defaults to `undefined`
   //
   // email verify token below expires in 2 hours (= 2 * 60 * 60 == 7200 seconds)
   emailVerifyTokenValidityDuration = 2 * 60 * 60, // in seconds (2 hours = 7200 seconds)

Testing

Added 10 new test cases in a new spec file EmailVerificationToken.spec.js

cherukumilli · 2016-07-06T16:27:23Z

@flovilmart
Is there anyway you can run the tests once more?
The tests ran fine on my local pc one of them seems to be failing on travis

flovilmart · 2016-07-06T16:34:18Z

Just restarted it :)

cherukumilli · 2016-07-07T17:09:09Z

@flovilmart
Did you get a chance to review this pull request?

flovilmart · 2016-07-07T17:22:46Z

Not yet, I'll do it before the weekend

drew-gross · 2016-07-09T05:06:03Z

I think we should only add 1 new setting, the emailVerifyTokenValidityDuration. If the user leaves that as unset, then we can assume they don't want the token to expire.

drew-gross · 2016-07-09T05:20:27Z

src/Controllers/UserController.js

      username: username,
      _email_verify_token: token
-    }, {emailVerified: true}).then(document => {
-      if (!document) {
+    }, {limit: 1}).then(results => {


Instead of this, I'd suggest adding a condition to the update that the current time must be before the expiry date, then if that fails you can do a query for the object to find the reason why it failed. That way the happy path requires one less query, and it will be less racy.

Sounds good. I will incorporate your suggestion. I will add _email_verify_expires_at checking in the query for the update statement.

btw: You may know this already...if there is a failure then we redirect the user to the invalid_link. We currently don't send the error message/failure reason back with the invalid_link. If there is a request to send the failure reason then I will take a shot at it in another PR.

cherukumilli · 2016-07-09T16:49:44Z

@flovilmart
In our earlier discussions, you indicated that it will be good if we have an additional setting like allowEmailVerifyTokenToExpire.

Are you ok if I change the code to use only one setting emailVerifyTokenValidityDuration as per the feedback from @drew-gross ?

If you are ok then I will:

remove following line from ParseServer.js to unset the value by default
emailVerifyTokenValidityDuration = 31536000, // 1 Year in seconds
remove all references to allowEmailVerifyTokenToExpire
Refactor all the code
update test cases and documentation and
submit another pull request

Please let me know either way :)

flovilmart · 2016-07-09T17:45:48Z

IM all good with that :)

ghost · 2016-07-09T21:40:53Z

@cherukumilli updated the pull request.

ghost · 2016-07-09T21:41:22Z

@cherukumilli updated the pull request.

ghost · 2016-07-09T21:56:49Z

@cherukumilli updated the pull request.

ghost · 2016-07-09T21:57:14Z

@cherukumilli updated the pull request.

drew-gross · 2016-07-09T23:33:44Z

src/cli/cli-definitions.js

@@ -151,6 +151,11 @@ export default {
    help: "Prevent user from login if email is not verified and PARSE_SERVER_VERIFY_USER_EMAILS is true, defaults to false",
    action: booleanParser
  },
+  "emailVerifyTokenValidityDuration": {
+    env: "PARSE_EMAIL_VERIFY_TOKEN_VALIDITY_DURATION",
+    help: "Email verification token validity duration, defaults to 1 year",


Doc needs update

Updated doc

ghost · 2016-07-10T00:47:13Z

@cherukumilli updated the pull request.

drew-gross · 2016-07-10T06:23:11Z

Cool. I think is solid, but because it involves a new public API I'd like to give it a few more days for comments. We maya want to eg. discuss what happens to existing tokens when people enable this for the first time. Another alternative would be to store the time the token was created rather than the time it expires, then if someone shortens the validity duration, existing tokens will be valid for the length of the new duration, not the old one. Session expiry doesn't work that way, though, so it might be confusing.

cherukumilli · 2016-07-10T18:50:29Z

@drew-gross
Your feedback got me thinking on other ways to expire email verify tokens without adding a new Public API.

I wonder if the following is a better solution than this PR:

Developer maintains an internal setting called: emailVerifyTokenExpireDuration (either using a Config variable using Parse Dashboard or a constant in cloud code or a DB table entry, etc...)
Developer creates a job / cloud code to run every 5 minutes
Cloud code queries the Parse.User class to get all the Users with expired tokens using a query like:
- emailVerified set to false
- createdAt is less than currentTime - emailVerifyTokenExpireDuration
Clean up all the Users returned in step 3

Please let me know if you see any gaps in the logic above.

drew-gross · 2016-07-10T19:53:48Z

Hmm, I missed the context about deleting users who have not verified their email. I don't think thats a great idea, I think a better plan would be to resend the verification email (which can be done today by setting the email on the user. It will resend the email even if it's the same address). Then people could recover their account if they delete the original verification email.

That plan for doing user cleanup without a new public API seems solid, but adding expiry as a new API seems like it would be useful as well.

cherukumilli · 2016-07-10T22:22:35Z

I didn't know that setting the email on the user will trigger another verification email.

Based on the feedback, I will add the following new test cases:

clicking on the email verify link by an email VERIFIED user that was setup before enabling the expire email verify token should show an invalid link
clicking on the email verify link by an email UNVERIFIED user that was setup before enabling the expire email verify token should show an invalid link
setting the email on the user should set a new email verification token and new expiration date for the token when expire email verify token flag is set

ghost · 2016-07-10T22:32:54Z

@cherukumilli updated the pull request.

ghost · 2016-07-10T22:33:14Z

@cherukumilli updated the pull request.

drew-gross · 2016-07-16T02:50:39Z

Code looks super solid and very well tested. One more api decision which we should consider is whether to let the client see this field, and/or edit it via regular Parse Queries, or strip it and not allow the client to see it.

If we decided we do want to allow the client to see/edit this field, we can merge as is.

flovilmart · 2016-07-16T02:51:42Z

IMHO we should not (same as emailVerified)

cherukumilli · 2016-07-17T01:37:42Z

@drew-gross
When you say whether to let the client see this field, is the field that you are talking about _email_verify_token_expires_at ?

If it is then it is hidden. I made it to mimic the _email_verify_token field to a large extent.

drew-gross · 2016-07-17T03:28:54Z

I noticed in the tests you are using user.get('_email_verify_token_expires_at') though?

cherukumilli · 2016-07-17T18:53:29Z

Let me fix that and push another update with a test case to ensure that the client cannot see the _email_verify_token_expires_at field.

facebook-github-bot · 2016-07-17T19:58:52Z

@cherukumilli updated the pull request.

ghost · 2016-07-17T19:59:11Z

@cherukumilli updated the pull request.

cherukumilli · 2016-07-18T16:33:33Z

@flovilmart
can you please restart the build?

flovilmart · 2016-07-18T16:34:45Z

there you have it :)

flovilmart · 2016-07-18T16:36:29Z

spec/EmailVerificationToken.spec.js

+      user.fetch()
+      .then(() => {
+        expect(user.get('emailVerified')).toEqual(false);
+        expect(typeof user.get('_email_verify_token_expires_at')).toBe('undefined');


flovilmart · 2016-07-18T16:37:44Z

@cherukumilli just 1 micro nit on the environment variable otherwise that looks more than awesome

ghost · 2016-07-18T18:04:07Z

@cherukumilli updated the pull request.

flovilmart · 2016-07-18T18:08:27Z

Awesome, just a note for the future, there is no need to squash your commits as we squash them upon merging :)

cherukumilli · 2016-07-18T18:32:26Z

Thanks. I will not squash them going forward.

can you please re-run the build again?

flovilmart · 2016-07-18T18:50:02Z

@drew-gross I'll let you merge as you reviewed the code more extensively

drew-gross · 2016-07-19T06:10:31Z

Looks awesome to me. So many tests ❤️

ghost added the GH Review: review-needed label Jul 6, 2016

drew-gross reviewed Jul 9, 2016
View reviewed changes

flovilmart modified the milestone: 2.2.17 Jul 12, 2016

flovilmart assigned drew-gross Jul 16, 2016

flovilmart reviewed Jul 18, 2016
View reviewed changes

Adds ability to expire email verify token

c90c532

drew-gross added GH Review: accepted and removed GH Review: review-needed labels Jul 19, 2016

drew-gross merged commit 6f29205 into parse-community:master Jul 19, 2016

cherukumilli deleted the ability-to-expire-email-verify-token branch July 19, 2016 17:47

rsouzas pushed a commit to back4app/parse-server that referenced this pull request Mar 15, 2017

Adds ability to expire email verify token (parse-community#2216)

26363cc

rsouzas pushed a commit to back4app/parse-server that referenced this pull request Mar 16, 2017

Adds ability to expire email verify token (parse-community#2216)

b8a1f34

Uh oh!

Ability to expire email verify token #2216

Ability to expire email verify token #2216

Uh oh!

Conversation

cherukumilli commented Jul 6, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Notes for this PR:

if emailVerifyTokenValidityDuration is set to a number greater than 0 and verifyUserEmails is set to true then

Documentation updates

Testing

Uh oh!

cherukumilli commented Jul 6, 2016

Uh oh!

flovilmart commented Jul 6, 2016

Uh oh!

cherukumilli commented Jul 7, 2016

Uh oh!

flovilmart commented Jul 7, 2016 via email

Uh oh!

drew-gross commented Jul 9, 2016

Uh oh!

drew-gross Jul 9, 2016

Choose a reason for hiding this comment

Uh oh!

cherukumilli Jul 9, 2016

Choose a reason for hiding this comment

Uh oh!

cherukumilli commented Jul 9, 2016

Uh oh!

flovilmart commented Jul 9, 2016

Uh oh!

ghost commented Jul 9, 2016

Uh oh!

ghost commented Jul 9, 2016

Uh oh!

ghost commented Jul 9, 2016

Uh oh!

ghost commented Jul 9, 2016

Uh oh!

drew-gross Jul 9, 2016

Choose a reason for hiding this comment

Uh oh!

cherukumilli Jul 10, 2016

Choose a reason for hiding this comment

Uh oh!

ghost commented Jul 10, 2016

Uh oh!

drew-gross commented Jul 10, 2016

Uh oh!

cherukumilli commented Jul 10, 2016

Uh oh!

drew-gross commented Jul 10, 2016

Uh oh!

cherukumilli commented Jul 10, 2016

Uh oh!

ghost commented Jul 10, 2016

Uh oh!

ghost commented Jul 10, 2016

Uh oh!

drew-gross commented Jul 16, 2016

Uh oh!

flovilmart commented Jul 16, 2016

Uh oh!

cherukumilli commented Jul 17, 2016

Uh oh!

drew-gross commented Jul 17, 2016

Uh oh!

cherukumilli commented Jul 17, 2016

Uh oh!

facebook-github-bot commented Jul 17, 2016

Uh oh!

ghost commented Jul 17, 2016

Uh oh!

cherukumilli commented Jul 18, 2016

Uh oh!

flovilmart commented Jul 18, 2016

Uh oh!

flovilmart Jul 18, 2016

Choose a reason for hiding this comment

cherukumilli commented Jul 6, 2016 •

edited

Loading

if `emailVerifyTokenValidityDuration` is set to a number greater than `0` and `verifyUserEmails` is set to `true` then