fix(core): Changes to Native Auth Register Flow

[pevey]

Description

This PR makes changes to the customer registration flow to address two issues:

• Possible email flooding (https://discord.com/channels/1100672177260478564/1229813869548146790)
• Possible data override of existing customers (https://discord.com/channels/1100672177260478564/1228399534745456742)

Breaking changes

This PR DOES include breaking changes.

The existing sign-up flow checks first to see if a user with that email address exists. If so, it pulls that data as a starting point.

• If the existing user is verified, the user record (excluding password) is updated with the submitted data and 'Success' is returned by the mutation. This is not ideal for two reasons: The existing data should not be overridden without authentication, and there is no indication to the frontend that the user already exists. This PR changes this flow to instead return a EmailAddressConflictError.

• If the existing user is not verified, currently a new verification token is generated and emailed to the user upon each sign up attempt. This could be problematic if a user keeps entering the same (incorrect) email address as it generates spam. It could also be used maliciously for this effect. This PR changes the flow to first check to see whether the existing verification token is still valid. If so, a NotVerifiedError is returned so that the frontend can provide a useful error message and prompt the user to check their email. If the token is expired, it is possible the record is very old or from someone who initially entered an invalid email address. For example, joel@example.com accidentally entered joe@example.com. He realizes his error, corrects his email address, and verifies his joel@example.com address. Later, the real joe@example.com shows up and tries to register. Only in this case, where the existing token is expired, will a new token be generated. It will be treated as if the existing user record does not exist.

Checklist

📌 Always:

• I have set a clear title
• My PR is small and contains a single feature
• I have checked my own PR

👍 Most of the time:

• I have added or updated test cases
• I have updated the README if needed

[pevey]

If you're good with this approach, I'll fix the auth-related e2e tests.

[michaelbromley]

Hi,

My only concern with the first change is that it opens up the possibility of an account enumeration attack as discussed here:

• Is there a reason why creating an account with an email already registered doesn't return an error? #746
• The requestPasswordReset method should always return success #1790

Do you have any thoughts on that?

---

On the second part, I agree with this approach. One point I would change is to not reuse the NotVerifiedError since it is semantically a little different:

"""
Returned if `authOptions.requireVerification` is set to `true` (which is the default)
and an unverified user attempts to authenticate.
"""
type NotVerifiedError implements ErrorResult {
    errorCode: ErrorCode!
    message: String!
}

We can create a new error result especially for this case, e.g. VerificationPendingError (just a suggestion on the name).

[pevey]

On the first topic, it is a good question and one that I also have wondered about. Security vs UX is always a careful balance.
There was discussion of this with the author of the Lucia authentication library. In short, I looked into it more, and he changed my mind about this. Basically, it is an old way of doing things that I was holding onto. I no longer think feedback about whether an account exists should be hidden from users. You can see now that pretty much all popular sites give feedback on whether an account exists. Not doing it is a really poor user experience. This is applicable to both the sign up and password reset flows.

To mitigate enumeration, those same sites also use rate-limiting, via actual rate limiting and also bot detection. Rate-limiting is not something Vendure currently supports out of the box, but it easily could since it is built on NestJS. That is another PR I'd like to do. Bot detection is something that everyone hosting ecom should be doing, for this reason but also mostly to protect against carding. But that leaves the whole industry very reliant on the best bot protection, which is currently Cloudflare with no close second.

So bottom line, I think it is really important to provide the useful error. Sometimes, you forget whether you created an account somewhere, especially an online store you may not purchase from frequently. In my checkout flow, if the user is not logged in, I take the email address entered and try to set the customer for order. If an account exists for that customer, it returns an EmailAddressConflictError. Which it should, so that you can prompt the user to log in. This is good UX. But from this functionality, there already exists the possibility to enumerate accounts without other security protections in place.

[pevey]

On the second issue, I did consider adding a new error type, and I'll go ahead and do that if you think it's the best approach. But an alternative would be to slightly change the wording of the existing error.

[michaelbromley]

OK you convinced me too. If we can get some kind of rate-limiting in with the same release that we make this change (v2.3), then I think that'll be fine.

Let's go with the new error type. It's more explicit and has no cost to making a new one.

[pevey]

I'll add the new error and work on the tests.

I'll do a separate PR that implements rate limiting using the NestJS throttler. https://docs.nestjs.com/security/rate-limiting

[pevey]

I'm adding this bit just for future reference to further document the rationale for the change to the flow.

It's been a while since I researched this, so I wanted to make sure the flow proposed is consistent with common industry practices. AWS Cognito provides a good benchmark since it is widely used and has gone through US Dept of Defense certification: https://aws.amazon.com/compliance/services-in-scope/DoD_CC_SRG/

The Cognito API returns a useful error on sign up attempts when a username already exists. See https://docs.aws.amazon.com/AWSJavaScriptSDK/v3/latest/client/cognito-identity-provider/command/SignUpCommand/, toward the bottom, UsernameExistsException. (In Cognito pools, the 'username' can be configured to be email or something else.) There is also a TooManyRequestsException, which shows that the AWS backend is doing some rate limiting.

[pevey]

While we are making changes to the auth flow, I want to bring up another question I have which is very related and should possibly be addressed by this PR.

• As you know, there is a config option for whether to require email verification with the native auth method
• Currently, the logic for adding native auth to a customer account goes like this:
  • If require verification is true, set verified to false, and generate verification token/send notification
  • If require verification is false, set verified to true <- POTENTIAL PROBLEM?
  • (EDIT: This refers to userService.addNativeAuthenticationMethod, which is called by the customer service)
• This logic could be improved, I think. If require verification is false, the account is not really verified. If we later change the setting to require verification, we have a bunch of accounts with verified set to true, but really they have never been verified.
• In the past, changing this would have broken the sign up flow, but here we are changing the sign up flow already. Changing it will also break the authentication flow unless we make another small change that only throws the not verified error if the user is not verified AND the require verification is set to true.

Making these small changes would mean:

• The verified status of a user always reflects reality and is not dependent on whether require verification is enabled or disabled. and
• You could switch back and forth between requiring and not requiring verification without major issues.

EDIT: The case could be made that if you have a store that does not require email verification, and you later enable verification, you may want to treat existing users as verified. But I think this should have be handled explicitly, because you also may not. You may want to trigger verification. If the data reflects reality, it can be more easily handled later. If you choose to treat existing accounts as verified when you enable requiring verification, you can manually verify existing customers via the admin or do it via SQL.

[pevey]

I've hit a wall for now on fixing the e2e tests. I'm going to take a break from it for now and work on something else and look at this fresh in a couple days.

[michaelbromley]

Agreed on the change to verification handling. We'll need to clearly document this change, but it does make sense to do it that way.

Let me know if you need help with the e2e test issues.