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

Customer Accounts #33

Closed
michaelbromley opened this issue Oct 24, 2018 · 5 comments
Closed

Customer Accounts #33

michaelbromley opened this issue Oct 24, 2018 · 5 comments

Comments

@michaelbromley
Copy link
Contributor

@michaelbromley michaelbromley commented Oct 24, 2018

A Customer can be a guest (has no associated User) or registered (has an associated User).

Account creation

An account can be created in one of two ways:

  1. Ad-hoc account creation by the customer outside the order process
  2. Conversion of a guest account into an authenticated account after the placing of an order.

customer-account-order-activity-diagram

As the diagram shows, a customer does not need to create an account after placing an order. Indeed, the customer is free to only every do guest checkouts. For the checkout, however, the email address would be used to create a guest Customer, and then on subsequent checkouts with the same email address, the same guest Customer would be assumed.

At any time, the guest Customer can be converted into an authenticated Customer by registering in either of the two ways outlined above.

Registration process

There are 2 questions to resolve regarding registration:

  1. Do we support 3rd party authentication methods in the future (social login, Google account etc)? If so, what (if any) changes to the current User entity would be needed?
  2. Do we require email account verification? If so, what is the purpose? Do unverified users have a restricted set of possibilities compared to verified ones?

Account Deletion (GDPR)

A user should have the ability to delete their account, as should an administrator. In this case, we want to keep the physical row in the DB for the purposes of data consistency and reporting, but we should completely remove any personally identifiable information (PII):

“[A]n identifiable natural person is one who can be identified, directly or indirectly, in particular by reference to an identifier such as a name, an identification number, location data, an online identifier or to one or more factors specific to the physical, physiological, genetic, mental, economic, cultural or social identity of that natural person.” source

@michaelbromley
Copy link
Contributor Author

@michaelbromley michaelbromley commented Oct 24, 2018

Issue: Account enumeration attacks

Since Customer's emailAddress is constrained to be unique, and we allow the customer to change their emailAddress, this opens the door to an enumeration attack. Consider:

  1. Attacker creates an account with the email address "hacker@leet.net"
  2. Attacker goes to own account page and attempts to update own account with new address "joe.smith@gmail.com"
  3. If a customer with the email "joe.smith@gmail.com" already exists, we cannot allow this change as it would violate the unique constraint. We return an error of some kind.
  4. Now the attacker knows that Joe Smith has an account with us.
  5. Attacker repeats this process (likely in an automated fashion directly against the GraphQL API) to build up a list of all of our registered users' email addresses.

The same process could also be taken against the "create account" endpoint, but would be a little less efficient since it would create a lot of new accounts for non-matching email addresses.

Severity

In the case outlined above, the disclosure of the existing email address is necessary and unavoidable. However, is it so bad?

Many well-known sites also allow such enumeration: https://markitzeroday.com/enumeration/privacy/2017/06/20/to-be-enumerated-or-not-to-be.html

This answer basically says it is a non-issue: https://security.stackexchange.com/a/42874

I tend to think it is of low severity since email addresses are public information in general.

Mitigation

The most practical mitigation I can think of would be some kind of throttling on the frequency that a given Customer may attempt to change their emailAddress.

@michaelbromley
Copy link
Contributor Author

@michaelbromley michaelbromley commented Nov 6, 2018

Email address verification

Email address verification is quite a common pattern. It usually works like this:

  1. User signs up with email address and a password.
  2. An email is sent to that address with a verification link. The link contains a unique token which is usually time-limited.
  3. Clicking the link tells the site that the email link has been clicked, so we can safely assume that the person who registered under this email address does indeed control the account.

The purpose of this process is:

  • We ensure the user actually owns the account. Spam prevention & also protection against a mis-typed email address.
  • We can decide to withhold certain critical information from a non-verified account, so that, e.g. if an email address was mis-typed, the inadvertant account holder now does not get lots of unwanted correspondance.
  • If a person accidentally creates an account under a wrong email address, and later wants to reset the password, there will be no way to do this since the password reset email will go to an inbox not actually controlled by the user. Requiring registration up-front better alerts the user to the mistake earlier on, before potentially many transactions are made.

How to handle

Some companies may want to implement email verification and others not. We should make it optional, i.e. a Customer entity would have a verified property, and the developer can choose to use this or not when deciding whether certain functions are permissible.

In general, much of the implementation will be contained in however we handle emails in general (issue to be created).

@michaelbromley
Copy link
Contributor Author

@michaelbromley michaelbromley commented Nov 9, 2018

Review this discussion on the Sylius repo for another argument for verification: Sylius/Sylius#6883

@michaelbromley
Copy link
Contributor Author

@michaelbromley michaelbromley commented Nov 20, 2018

Customer Registration Flow

customer-registration-activity-diagram

In the diagram above, the password is only set after the verification link has been clicked. This works in the same way as a typical the "forgotten password" flow.

The reasoning is that there is no additional security to setting & then verifying a password on registration, since the user could also register with a password and then after that invoke the forgotten password flow, which we must provide, otherwise a forgotten initial registration password would render that email account void and unusable.

Therefore there is no additional security to be gained by requiring a password up-front.

Token expiry

The verification token expires after a configured time period (7 days in the current default config). After expiry a new token can be issued to the email address. Why then bother with an expiry if a new token can be immediately issued? The reason is that this prevents the scenario where the initial token is leaked somehow (email address compromised, email traffic intercepted) but now then real owner once again has control over the email account and the attacker does not.

michaelbromley added a commit that referenced this issue Nov 20, 2018
Relates to #33. Edge cases need checking and test should be written.
michaelbromley added a commit that referenced this issue Nov 20, 2018
Relates to #33
@michaelbromley michaelbromley added this to To do in Alpha Dec 31, 2018
@michaelbromley michaelbromley moved this from To do to In progress in Alpha Jan 1, 2019
@michaelbromley michaelbromley moved this from In progress to Done in Alpha Jan 9, 2019
michaelbromley added a commit that referenced this issue Jan 9, 2019
@marwand
Copy link

@marwand marwand commented Oct 21, 2020

Is there a reason why creating an account with an email already registered doesn't return an error?

Update - more information

Running the following mutation multiple times with the same credentials always returns success.

mutation Register($input: RegisterCustomerInput!) {
  registerCustomerAccount(input: $input) {
    __typename
    ... on Success {
      success
    }
    ...ErrorResult
  }
}

fragment ErrorResult on ErrorResult {
  errorCode
  message
}

Here are the two cases I could think of:

  1. If the user registered, but has not yet verified his account. Any subsequent registration attempt with the same credentials returns success. Also, multiple verification emails are sent. If different passwords were specified during the registration attempts, the first one is the one recorded in the database. You could see how this might confuse customers.

  2. If the account is verified, any subsequent registration attempt returns success.

Looking at the error codes currently used, it seems like it should return:

  • EMAIL_ADDRESS_CONFLICT_ERROR for case (2)

  • Either EMAIL_ADDRESS_CONFLICT_ERROR or NOT_VERIFIED (subject to issue #500) for case (1), although I prefer the latter as it is more informative to the customer.

I'm now aware of commit b1ffa1e that makes the registration mutation silently fail to protect user data. You're trading off extra security for customer experience. As a user, I often forget if I have an account on a particular website - especially if I don't use it often. I'm curious to hear what you think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Alpha
  
Done
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants