Skip to content

Allow multiple email verification codes, or resend with existing code #7012

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dblythy opened this issue Nov 17, 2020 · 5 comments · Fixed by #7017
Closed

Allow multiple email verification codes, or resend with existing code #7012

dblythy opened this issue Nov 17, 2020 · 5 comments · Fixed by #7017
Labels
type:feature New feature or improvement of existing feature

Comments

@dblythy
Copy link
Member

dblythy commented Nov 17, 2020

Is your feature request related to a problem? Please describe.

I often find that due to the time it takes mail to deliver in my country / with our providers, clients will reclick the "forgot password / resend email". Then, the first email comes through with an "Invalid Link" (as the second request wiped it), leading to increasing frustration with our customers.

Describe the solution you'd like

Cooldowns:

If the request is re-requested within a specified timeframe (e.g 1 min), either:

  • The server responds with an error message such a "please wait blah blah blah"
  • The server resend the email without re-creating another code.

Or, allowing multiple verification codes in an array (e.g, 5), and only one has to be clicked to verify the request.

Describe alternatives you've considered
Using a custom cloud function to check code expiry and date, and then work backwards.

@mtrezza
Copy link
Member

mtrezza commented Nov 17, 2020

Thanks for suggesting.

This seems to be a useful enhancement for Parse Server. I suggest to pursue option 2 because it causes less friction for the user:

The server resend the email without re-creating another code.

It seems we already have most of the logic in place thanks to the resetTokenValidityDuration password policy parameter. If a validity duration is set, a condition could simply check the token validity, and if it is still valid, resend the same token instead of overwriting the existing one.

To not make this a breaking change from a security policy perspective, it requires introducing another password policy option which defines whether an existing token should be used or overwritten. For example:

{
  resetTokenReuseIfValid: false // Optional. Is true if an existing password reset token should be reused when a password reset is requested. This requires `resetTokenValidityDuration` to be set and the existing password reset token must not be expired. If false, an existing password reset token is replaced with a new password reset token when requested. Default is false.
}

Would you want to open a PR?

@dblythy
Copy link
Member Author

dblythy commented Nov 17, 2020

Thanks @mtrezza. Should be too bad for Password Policy, but I'm concerned adding another option for emails on the top level (e.g emailResetTokenReuseIfValid).

I propose:

emailPolicy : {
  verifyUserEmails: true,
  emailVerifyTokenValidityDuration: 2 * 60 * 60,
  preventLoginWithUnverified: false,
  resetTokenReuseIfValid: false
}

Also, as far as I could tell, there are no deeper definitions for the keys in passwordPolicy. Could this potentially be improved?

@mtrezza
Copy link
Member

mtrezza commented Nov 17, 2020

I'm concerned adding another option for emails on the top level

The new parameter resetTokenReuseIfValid should go under the same key as the already existing and related resetTokenValidityDuration, not at the root level.

there are no deeper definitions for the keys in passwordPolicy. Could this potentially be improved?

Yes, however any such improvement should be done in a separate PR because it involves code sections that are not related to the functionality of this PR. But you spotted it correctly, actually the whole Parse Server configuration would benefit from a PR that restructures the keys, because many are at the root level or have verbose or ambiguous names. That may sound simpler than it is however, because it involves changes in tests and other sections of the code.

@dblythy
Copy link
Member Author

dblythy commented Nov 17, 2020

Yes, but as far as I can tell, that is under "passwordPolicy". This feature would also affect resending verification emails, which in my view, wouldn't be expected to be password policy related.

Yes, however any such improvement should be done in a separate PR

Ok. thanks I was just seeing your thoughts. I'll open a new issue.

@mtrezza
Copy link
Member

mtrezza commented Nov 17, 2020

What speaks against introducing two distinct config variables?

The verification email and password reset email are both "emails", but part of different features, hence they are part of distinct key groups.

{
  passwordPolicy: {
    resetTokenValidityDuration: ...,
    resetTokenReuseIfValid: ...
  },
  emailVerifyTokenValidityDuration: ...,
  emailVerifyTokenReuseIfValid: ...
}

A restructuring of Parse Server config keys should be done in a separate PR, for the reasons previously mentioned. I would advice against changing only parts of the config structure without looking at the bigger picture, because these are breaking changes that require many deployments to be adapted. We want to keep the frequency of such changes as low as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature New feature or improvement of existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants