feat: Add new Parse Server option preventSignupWithUnverifiedEmail to prevent returning a user without session token on sign-up with unverified email address

[dblythy]

Pull Request

• Report security issues confidentially.
• Any contribution is under this license.
• Link this pull request to an issue.

Issue

Current, when using preventLoginWithUnverifiedEmail, the save response is a 201, and the JS SDK assigns Parse.User.current() with an objectId, but no session token. Instead, the same response should return as a login, which is:

new Parse.Error(Parse.Error.EMAIL_NOT_FOUND, 'User email is not verified.')

Closes: #2863

Approach

All other operations should occur, just the response should throw. There will need to be some refactoring, as currently the feature is implemented as:

await user.signup();
await user.logout();
// we've sent you an email, please verify your email to continue

New:

try {
  await user.signup();
} catch (e) {
  if (e.code === Parse.Error.EMAIL_NOT_FOUND) {
     // we've sent you an email, please verify your email to continue
  }
}

Perhaps we could add an option to the JS SDK signup method such as {login: false} that can determine whether signup can throw User email is not verified

Tasks

• Add tests

[parse-github-assistant]

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

[parse-github-assistant]

Thanks for opening this pull request!

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.15 ⚠️

Comparison is base (0ce3692) 94.34% compared to head (d5b4757) 94.19%.

❗ Current head d5b4757 differs from pull request most recent head 18b920d. Consider uploading reports for the commit 18b920d to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##            alpha    #8451      +/-   ##
==========================================
- Coverage   94.34%   94.19%   -0.15%     
==========================================
  Files         183      183              
  Lines       14576    14579       +3     
==========================================
- Hits        13751    13732      -19     
- Misses        825      847      +22     

Impacted Files	Coverage Δ
src/Options/Definitions.js	100.00% <ø> (ø)
src/Options/index.js	100.00% <ø> (ø)
src/RestWrite.js	94.96% <100.00%> (+0.02%)	⬆️

... and 5 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.

[mtrezza]

Currently the response is 201 success and with the PR the response will be a 400 error? Isn't that a breaking change?

[dblythy]

Is it considered a breaking change if it's a bug in the expected behaviour?

[mtrezza]

There's no fixed rule. Generally, if it's incorrect behavior but the feature is usable, then we can assume developers found a workaround. Breaking that workaround would make it a breaking change, even if it's just correcting the incorrect behavior. If there is no practical workaround then it's not a breaking change. If the bug exists only in an alpha or beta pre-release then it's also not a breaking change, because we can fix that before official release. For security fixes there are other additional considerations.

Here, if the http response code is 201 instead of 400, and developers work around this by checking for an existing session token, then I guess it would be breaking. Technically it should be 400, but with some imagination one could say it can be 201 because an object ID is assigned, there's just no session token. What do you think?

I guess a simple test is always to imagine your own app is using the feature and depending on the status quo. Then you upgrade Parse Server and millions of clients expecting hard-codedly a 201 response will suddenly break. Would you have wanted this to be communicated as a breaking change?

[dblythy]

It is a breaking change, so should we wait for a major release? How do you think we should approach this?

If we leave it as a status 200, but return a response with no session token, the client SDKs will think the login was successful, assign a Parse.User with no session token, and future requests will fail with cannot modify user. This is the current status-quo of the feature:

 const user = new Parse.User();
user.setPassword('asdf');
user.setUsername('zxcv');
user.set('email', '[email protected]');
await user.signUp(null);
console.log(Parse.User.current()) // is defined
user.set('foo', 'bar');
await user.save(); // cannot modify user

The current work around is:

 const user = new Parse.User();
user.setPassword('asdf');
user.setUsername('zxcv');
user.set('email', '[email protected]');
await user.signUp(null);
await Parse.User.logOut()

[mtrezza]

If we say this is breaking then we can

• leave as it until next major release, or
• add a server option with a deprecation that can released immediately as feature release

The challenge with this correction is that it's not just server side code that needs to be updated, but possibly clients. That often mans a long-winding migration process over +1 year. So we could:

• Add an option to allow developers to switch to the correct behavior immediately.
• In Jan 2024 with Parse Server 7 we can change the option default to the correct behavior, and deprecate the option, so developers are informed that they have 1 year to migrate.
• In Jan 2025 with Parse Server 8 we can remove the option, unless we get feedback that more time is needed to migrate.

[dblythy]

Refactored this to be a Parse Server option: preventSignupWithUnverifiedEmail

[mtrezza]

I've added some more details to the option docs. Could you check whether I got this right and rebuild the definitions?

[mtrezza]

The new PR title is somewhat long, but I thought "prevent sign-up with unverified email" sounds strange because an email address is always unverified on sign-up.

[dblythy]

Sounds good to me!

[mtrezza]

Looks good, is this ready for merge?

[dblythy]

Yep!

[parseplatformorg]

🎉 This change has been released in version 6.1.0-alpha.17

[parseplatformorg]

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

[parseplatformorg]

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

[parseplatformorg]

🎉 This change has been released in version 6.3.0