Skip to content
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

feat: Add conditional email verification via dynamic Parse Server options verifyUserEmails, sendUserEmailVerification that now accept functions #8425

Merged
merged 26 commits into from
Jun 20, 2023

Conversation

dblythy
Copy link
Member

@dblythy dblythy commented Feb 5, 2023

Pull Request

Issue

Currently it is not possible to decide what users should and shouldn't receive verification emails.

Closes: #7144

Approach

wip

Tasks

  • Add tests

@parse-github-assistant
Copy link

I will reformat the title to use the proper commit message syntax.

@parse-github-assistant parse-github-assistant bot changed the title feat: add conditional email feat: Add conditional email Feb 5, 2023
@parse-github-assistant
Copy link

parse-github-assistant bot commented Feb 5, 2023

Thanks for opening this pull request!

@codecov
Copy link

codecov bot commented Feb 5, 2023

Codecov Report

Patch coverage: 98.55% and project coverage change: +0.03 🎉

Comparison is base (45301a6) 94.41% compared to head (931e589) 94.44%.

❗ Current head 931e589 differs from pull request most recent head 9d87f1e. Consider uploading reports for the commit 9d87f1e to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##            alpha    #8425      +/-   ##
==========================================
+ Coverage   94.41%   94.44%   +0.03%     
==========================================
  Files         184      184              
  Lines       14613    14644      +31     
==========================================
+ Hits        13797    13831      +34     
+ Misses        816      813       -3     
Impacted Files Coverage Δ
src/Options/Definitions.js 100.00% <ø> (ø)
src/Options/index.js 100.00% <ø> (ø)
src/Controllers/UserController.js 95.91% <97.14%> (-0.24%) ⬇️
src/RestWrite.js 95.05% <100.00%> (+0.24%) ⬆️
src/Routers/PagesRouter.js 97.71% <100.00%> (ø)
src/Routers/PublicAPIRouter.js 93.38% <100.00%> (ø)
src/Routers/UsersRouter.js 97.13% <100.00%> (+0.01%) ⬆️

... and 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@dblythy dblythy marked this pull request as ready for review February 5, 2023 07:36
@dblythy dblythy requested a review from a team February 5, 2023 07:36
@mtrezza
Copy link
Member

mtrezza commented Feb 27, 2023

The discussion in the issue crystallized 3 distinct functionalities:

a) Requiring or omitting email verification on a per-user basis, regardless of Parse Server option
b) Manually setting the _User.emailVerified field, just like any other non-internal field
c) (Re)-sending or suppressing the verification email, so that a developer can design their own email verification flow

Looking at the PR it seem to cover (b) and (c) but not (a), is that correct?

@dblythy
Copy link
Member Author

dblythy commented Feb 27, 2023

Looking at the PR it seem to cover (b) and (c) but not (a), is that correct?

Yes, that is correct. How would option (a) work? Perhaps preventLoginWithUnverifiedEmail could be a function, where if false is returned for a given user, they are permitted to login

spec/EmailVerificationToken.spec.js Outdated Show resolved Hide resolved
src/RestWrite.js Outdated Show resolved Hide resolved
@mtrezza
Copy link
Member

mtrezza commented Feb 27, 2023

How would option (a) work?

Maybe we can make use of triggers. I guess it depends where in the code it is relevant whether a user needs to verify their email. Is that checked at the login endpoint, before trying to create session?

@dblythy
Copy link
Member Author

dblythy commented Feb 27, 2023

I'm thinking:

  • Server configuration verifyUserEmails (currently bool) could be a function, and whether true or false is returned from the function, determines whether the verification email is send (c)

  • Server configuration preventLoginWithUnverifiedEmail (currently bool) could be a function, and whether true or false is returned from the function, determines whether a user is prevented from logging in with unverified email (a)

  • beforeSave can manually set emailVerified in either case (b)

With #8449, this could be defined using:

Parse.Server.setVerifyUserEmails(req => {
  return req.object.email !== 'test@example.com'
});
Parse.Server.setPreventLoginWithUnverifiedEmail(req => {
  return req.object.email !== 'test@example.com'
});

@mtrezza
Copy link
Member

mtrezza commented Feb 27, 2023

Server configuration verifyUserEmails (currently bool) could be a function, and whether true or false is returned from the function, determines whether the verification email is send (c)

That sounds like mixing (a) and (c). The use case originally discussed (see lower part of #7144 (comment)) is an interesting one where (c) is controlled discretely.

Server configuration preventLoginWithUnverifiedEmail (currently bool) could be a function

Sounds good. The function could offer the request object as argument to determine user and installation. I think this mechanism is an additional feature to (a), (b), (c).

beforeSave can manually set emailVerified in either case (b)

Yes, we need to get the order right then. For example if a user is set to emailVerified in Cloud Code (i.e. the developer decides to permanently fake-verify an email) then the question of sending a verification email or whether to require email verification becomes obsolete.

There seems to be an implicit hierarchy, which we could use as guidance to design the flow.

  1. Parse.User.emailVerified
    If set to true in Parse.User.beforeSave then ignore the following points. Implements feature (b).
  2. verifyUserEmails
    If the server option is false or a function that returns false then ignore the following points. Implements feature (a).
  3. sendUserEmailVerification
    A new Parse Server option that allows to decide on a per-user basis whether to send an email, based on its return value. Implements feature (c).

(3) could be merged into (2) by returning a tuple of bools or maybe allowing to modify the arguments and then reading them out. At first glance a separate function seems cleaner, maybe that will require to lookup a user twice as I think there wouldn't be any way to pass values between (2) and (3).

If instead of allowing functions as server options we add new CC triggers then a context container would be available to pass values around. I'm wondering whether extending server options to functions is a good direction if we have triggers that are a more capable feature. But it may be easier to implement.

@dblythy
Copy link
Member Author

dblythy commented Mar 2, 2023

I'm not sure the difference in your suggestion between sendUserEmailVerification and verifyUserEmails. If a developer doesn't want user email verification to send for a given user, they can return false in verifyUserEmails.

Some comments on my implementation:

  • beforeSave triggers can mutate emailVerified. If they do, verification email won't send. If the developer wants to send verification email, they can call Parse.User.requestEmailVerification(email)
  • requestEmailVerification can be intercepted by sendUserEmailVerification (returning or setting the Parse Server value to false will prevent email from being sent
  • If they want to customise whether emails should be verified per user, set the Parse Server option emailVerified to a function. if false is returned, Parse Server won't send email verification.
  • If they want to customise whether users should be able to login unverified, set the Parse Server option preventLoginWithUnverifiedEmail to a function. if false is returned, Parse Server will allow unverified users to login.

@mtrezza mtrezza changed the title feat: Add conditional email feat: Add conditional email verification Mar 6, 2023
@mtrezza
Copy link
Member

mtrezza commented Mar 6, 2023

I'm not sure the difference in your suggestion between sendUserEmailVerification and verifyUserEmails. If a developer doesn't want user email verification to send for a given user, they can return false in verifyUserEmails.

The general idea is to disassemble the flow to allow developers to control each of the steps.

So your question is, whether there is a use case where a developer wants to say "this email is not verified", but also "don't send a verification email". If both are in one step, then the developer can only say:

  • either the email is verified and thus login proceeds
  • or the email is not verified and thus the login stops and a verification email is sent

For sign-up:

  • A use case could be a custom verification process where they use the email verification mechanism to add a manual vetting step before that. So a user signs up, an employee checks for user details, then manually triggers to send the email verification. So they don't have to build an additional verification mechanism.

For login:

  • A use case could be where where the server denies login because the email is not verified and the client shows a dialog where the user can requests to re-send the verification email, but it's not sent automatically. Every email costs. On an app with millions of users this can be a cost saving measure.
  • Another argument could be that it's simply unnecessary to spam the user with another email if they already have one.
  • Another use case could be that the email is only reset if the previous verification link expires (do they expire?).

So how does the flow currently look like? I'm guessing:

  1. Check if user requires email verification.
  2. Check if user email is verified.
  3. Send verification email.

And how does it look like for sign-up vs. login?

@dblythy
Copy link
Member Author

dblythy commented Mar 29, 2023

I have added a config option sendUserEmailVerification, so:

  • verifyUserEmails can be set per user if you want to disable / enable the whole user email verification system for a user
  • sendUserEmailVerification can be set if the email verification system is still required, but the email shouldn't be sent in a given case

@dblythy dblythy requested a review from a team April 10, 2023 03:31
src/Options/Definitions.js Outdated Show resolved Hide resolved
mtrezza and others added 4 commits May 21, 2023 03:50
Signed-off-by: Manuel <5673677+mtrezza@users.noreply.github.com>
@mtrezza
Copy link
Member

mtrezza commented May 22, 2023

Is this ready for merge? I just added #8425 (comment), if you agree...

@mtrezza
Copy link
Member

mtrezza commented Jun 9, 2023

There seems to be an issue with a test in this PR.

@mtrezza
Copy link
Member

mtrezza commented Jun 19, 2023

@dblythy The tests pass, can we merge?

@dblythy
Copy link
Member Author

dblythy commented Jun 20, 2023

Yep!

@mtrezza mtrezza merged commit 44acd6d into parse-community:alpha Jun 20, 2023
parseplatformorg pushed a commit that referenced this pull request Jun 20, 2023
# [6.3.0-alpha.2](6.3.0-alpha.1...6.3.0-alpha.2) (2023-06-20)

### Features

* Add conditional email verification via dynamic Parse Server options `verifyUserEmails`, `sendUserEmailVerification` that now accept functions ([#8425](#8425)) ([44acd6d](44acd6d))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 6.3.0-alpha.2

@parseplatformorg parseplatformorg added the state:released-alpha Released as alpha version label Jun 20, 2023
parseplatformorg pushed a commit that referenced this pull request Sep 16, 2023
# [6.4.0-beta.1](6.3.0...6.4.0-beta.1) (2023-09-16)

### Bug Fixes

* Parse Server option `fileUpload.fileExtensions` does not work with an array of extensions ([#8688](#8688)) ([6a4a00c](6a4a00c))
* Redis 4 does not reconnect after unhandled error ([#8706](#8706)) ([2b3d4e5](2b3d4e5))
* Remove config logging when launching Parse Server via CLI ([#8710](#8710)) ([ae68f0c](ae68f0c))
* Server does not start via CLI when `auth` option is set ([#8666](#8666)) ([4e2000b](4e2000b))

### Features

* Add conditional email verification via dynamic Parse Server options `verifyUserEmails`, `sendUserEmailVerification` that now accept functions ([#8425](#8425)) ([44acd6d](44acd6d))
* Add property `Parse.Server.version` to determine current version of Parse Server in Cloud Code ([#8670](#8670)) ([a9d376b](a9d376b))
* Add TOTP authentication adapter ([#8457](#8457)) ([cc079a4](cc079a4))

### Performance Improvements

* Improve performance of recursive pointer iterations ([#8741](#8741)) ([45a3ed0](45a3ed0))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 6.4.0-beta.1

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Sep 16, 2023
parseplatformorg pushed a commit that referenced this pull request Sep 20, 2023
# [6.4.0-alpha.1](6.3.0...6.4.0-alpha.1) (2023-09-20)

### Bug Fixes

* Parse Server option `fileUpload.fileExtensions` does not work with an array of extensions ([#8688](#8688)) ([6a4a00c](6a4a00c))
* Redis 4 does not reconnect after unhandled error ([#8706](#8706)) ([2b3d4e5](2b3d4e5))
* Remove config logging when launching Parse Server via CLI ([#8710](#8710)) ([ae68f0c](ae68f0c))
* Server does not start via CLI when `auth` option is set ([#8666](#8666)) ([4e2000b](4e2000b))

### Features

* Add conditional email verification via dynamic Parse Server options `verifyUserEmails`, `sendUserEmailVerification` that now accept functions ([#8425](#8425)) ([44acd6d](44acd6d))
* Add context to Cloud Code Triggers `beforeLogin` and `afterLogin` ([#8724](#8724)) ([a9c34ef](a9c34ef))
* Add property `Parse.Server.version` to determine current version of Parse Server in Cloud Code ([#8670](#8670)) ([a9d376b](a9d376b))
* Add TOTP authentication adapter ([#8457](#8457)) ([cc079a4](cc079a4))

### Performance Improvements

* Improve performance of recursive pointer iterations ([#8741](#8741)) ([45a3ed0](45a3ed0))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 6.4.0-alpha.1

parseplatformorg pushed a commit that referenced this pull request Nov 16, 2023
# [6.4.0](6.3.1...6.4.0) (2023-11-16)

### Bug Fixes

* Parse Server option `fileUpload.fileExtensions` does not work with an array of extensions ([#8688](#8688)) ([6a4a00c](6a4a00c))
* Redis 4 does not reconnect after unhandled error ([#8706](#8706)) ([2b3d4e5](2b3d4e5))
* Remove config logging when launching Parse Server via CLI ([#8710](#8710)) ([ae68f0c](ae68f0c))
* Server does not start via CLI when `auth` option is set ([#8666](#8666)) ([4e2000b](4e2000b))

### Features

* Add conditional email verification via dynamic Parse Server options `verifyUserEmails`, `sendUserEmailVerification` that now accept functions ([#8425](#8425)) ([44acd6d](44acd6d))
* Add property `Parse.Server.version` to determine current version of Parse Server in Cloud Code ([#8670](#8670)) ([a9d376b](a9d376b))
* Add TOTP authentication adapter ([#8457](#8457)) ([cc079a4](cc079a4))

### Performance Improvements

* Improve performance of recursive pointer iterations ([#8741](#8741)) ([45a3ed0](45a3ed0))
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 6.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released Released as stable version state:released-alpha Released as alpha version state:released-beta Released as beta version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Conditional email verification
3 participants