Allow guest users to checkout with conflicting email address

[mattgills]

Is your feature request related to a problem? Please describe.
See slack discussion here

We currently support two stores, one requires login and one does not allow for login at all. Since all checkouts in the second store are considered guest checkouts, the user is asked to enter their name and email address. If the email address matches an email address for the first store, then the user cannot checkout in the second store.

Describe the solution you'd like
It would be great to allow the user to checkout as a guest user and not be associated with any existing customer accounts.

Describe alternatives you've considered
Some alternatives discussed in the slack conversation.

Additional context
N/A

[michaelbromley]

Posting the chat from Slack before the context is lost:

matt Hi we are currently working to add multi channel support, in which one store requires login and the second store does not allow login (i.e. guest checkout only). As a result if a customer has an account in store 1 and attempts to purchase something in store 2, then they can run into an email address conflict error. This will occur if the email address of the guest checkout matches an email of an existing customer/user. We've pinpointed the blocking point to be the customer.service.ts file (see snippet below) and are wondering if there is any advice on how to handle this. I understand the logic in the snippet below - a guest order from store 2 should not be able to update an existing user account. However, a guest order from store 2 also shouldn't be blocked by an existing user account.

        if (existing) {
            if (existing.user && errorOnExistingUser) {
                // It is not permitted to modify an existing *registered* Customer
                return new EmailAddressConflictError();
            }
            customer = patchEntity(existing, input);
            customer.channels.push(await this.connection.getEntityOrThrow(ctx, Channel, ctx.channelId));
        }

michael This is an interesting edge case!
What would you expect to happen here? If a guest checkout featured different address details to the existing registered Customer? Just update it because we can safely assume it is the same person?

matt I think we can generally assume it's the same person, because the order confirmation and details would be going to that email address. Even that customer record is updated, then the session still would show as unauthenticated, correct? We wouldn't want the user to be able access any account information based on the email they entered in the checkout process (i.e. previously used addresses, previous orders, etc.). I assume this wouldn't be an issue because the session service would still show it as unauthenticated with no active user, correct?

michael I tend to agree with your assessment, but the problem is that doing so might open up a vulnerability where it would become possible to alter another person's address data if you know their email address. Although no data would be exposed, it is still an undesirable exploit which could conceivably be automated to cause trouble for the merchant.

matt That certainly makes sense! Would it be possible to allow the guest checkout to proceed without trying to link to the customer? We don't need that link to occur since there is no need for the customer to login and view past orders.

michael I think you should open a feature request for making this configurable in some way. I'm not totally sure of the right way to handle it at the moment, but I think we should be able to come up with a design that accommodates both the current behaviour (which is often desirable) and also allow you to opt-out of this constraint as in your use-case.

[mattgills]

Hey Michael, any thoughts or updates on this feature request?

[michaelbromley]

Hi Matt,

No updates yet. It's in the "planned" queue, so I do plan to address it. I just have other issues that I plan to address first.

What could speed up the process is some input around this:

I'm not totally sure of the right way to handle it at the moment, but I think we should be able to come up with a design that accommodates both the current behaviour (which is often desirable) and also allow you to opt-out of this constraint as in your use-case.

and once we agree on something, a PR would be welcome. It could conceivable make it into the next minor release in that case.

[mattgills]

Hi Michael,

I spoke with a fellow team member @WilliamMilne about this item and we came up with some ideas for discussion. We specifically looked at the customer.service.ts createOrUpdate function, since that is where we are experiencing the email address conflict (original function):

async createOrUpdate(
    ctx: RequestContext,
    input: Partial<CreateCustomerInput> & { emailAddress: string },
    errorOnExistingUser: boolean = false,
): Promise<Customer | EmailAddressConflictError> {
    input.emailAddress = normalizeEmailAddress(input.emailAddress);
    let customer: Customer;
    const existing = await this.connection.getRepository(ctx, Customer).findOne({
        relations: ['channels'],
        where: {
            emailAddress: input.emailAddress,
            deletedAt: null,
        },
    });
    if (existing) {
        if (existing.user && errorOnExistingUser) {
            // It is not permitted to modify an existing *registered* Customer
            return new EmailAddressConflictError();
        }
        customer = patchEntity(existing, input);
        customer.channels.push(await this.connection.getEntityOrThrow(ctx, Channel, ctx.channelId));
    } else {
        customer = new Customer(input);
        this.channelService.assignToCurrentChannel(customer, ctx);
    }
    return this.connection.getRepository(ctx, Customer).save(customer);
}

Prior to listing the options we discussed below, we decided that the solution should come from the server side. The opt out concept shouldn't be implemented through the GraqphQL API, since that would allow any client to define the behavior. That would not solve the security concern you mentioned above.

Option 1
Define an additional field on the Channel entity named userSharingEnabled, which would allow a channel to enable/disable the user sharing feature. This field would be enabled by default and could be disabled if a specific channel should create and manage customers separately from all channels. With this additional field the createOrUpdate function would have a single line change of:

if (existing && ctx.channel.userSharingEnabled) { // THIS LINE CHANGED
    if (existing.user && errorOnExistingUser) {
        // It is not permitted to modify an existing *registered* Customer
        return new EmailAddressConflictError();
    }
    customer = patchEntity(existing, input);
    customer.channels.push(await this.connection.getEntityOrThrow(ctx, Channel, ctx.channelId));
} else {
    customer = new Customer(input);
    this.channelService.assignToCurrentChannel(customer, ctx);
}

Option 2
As a possible extension to Option 1, a Channel could have an array of channels that it explicitly shares customers with. For example, channels A and B could be configured to share customers, while channels C, D, and E could be configured to share customers. Similar changes to the createOrUpdate function could be me made so that the logic explicitly checks whether the current channel is sharing customers with any other channels. If it does, then it would modify the definition of const existing = ... to also include a where condition that includes the channels that it shares with.

The primary challenge or difficulty with this one is that it would be a bad UI to have to update every channel to have the exact same shared channels value. For example, if channel A is configured to share with channel B, then channel B must also be configured to share with channel A. Otherwise there would be some gaps in the logic or behavior. Alternatively, there could be a separate configuration/entity that groups user sharing in a single source (loosely similar to customer groups).

Option 3
Define an additional field on the Channel entity named loginType, which would be an enumerated type with possible values REQUIRED, OPTIONAL, and ANONYMOUS. The default value would be OPTIONAL and is the standard behavior that Vendure utilizes. A similar check could be used on createOrUpdate:

if (existing && ctx.channel.loginType !== LoginType.ANONYMOUS) { // THIS LINE CHANGED AND LoginType is an enumerated type
    if (existing.user && errorOnExistingUser) {
        // It is not permitted to modify an existing *registered* Customer
        return new EmailAddressConflictError();
    }
    customer = patchEntity(existing, input);
    customer.channels.push(await this.connection.getEntityOrThrow(ctx, Channel, ctx.channelId));
} else {
    customer = new Customer(input);
    this.channelService.assignToCurrentChannel(customer, ctx);
}

After additional consideration of this approach, I feel this would have more side effects than Option 1 and may change how behavior should be handled by other entities and services. So this doesn't feel like the most generic option.

We are happy to elaborate on any of these options and are very interested in your thoughts!

[michaelbromley]

Hi,

Thanks for taking the time to explore these solutions and lay them out clearly. That's really helpful!

So, here's are the constraints I would like to work with:

1. No breaking schema changes. Otherwise I'd need to defer a solution to v2.0
2. Must not increase complexity in other, unrelated parts of the code base
3. As simple as possible.

Those 3 constraints would rule out the 3 options above, since they all would require adding a field to the Channel entity.

I also want to clarify one point: in all 3 of your examples, in the case where "user sharing" is enabled, you are creating a new Customer entity. Is that a requirement in your case? Or is it acceptable to re-use the existing Customer, potentially update their name/contact details, and add them to the new Channel? The latter is what I understood from your initial issue. In either case, I think we can have a solution that covers both.

So I propose a strategy-based solution whereby you can define authOptions.guestConflictStrategy which allows you to define a function which will be invoked in the case that a guest checkout uses an existing email address:

interface GuestConflictStrategy extends InjectableStrategy {
  handleGuestIdentifierConflict(
    ctx: RequestContext, 
    existingCustomer: Customer, 
    input: CreateCustomerInput): Promise<Customer | EmailAddressConflictError> | Customer | EmailAddressConflictError;
}

This function would return a Customer entity which would then be used for the guest order. Note that this way you could even (if you want to) create a brand new Customer entity rather than returning the existingCustomer argument.

So the CustomerService.createOrUpdate method would then look like this:

    async createOrUpdate(
        ctx: RequestContext,
        input: Partial<CreateCustomerInput> & { emailAddress: string },
        errorOnExistingUser: boolean = false,
    ): Promise<Customer | EmailAddressConflictError> {
        // omitted

        if (existing) {
            if (existing.user && errorOnExistingUser) {
                const customerOrError = await this.configService.authOptions
                  .guestConflictStrategy.handleGuestIdentifierConflict(ctx, customer, input);
                if (result instanceof Customer) {
                  customer = customerOrError ;
                } else {
                  return customerOrError ;
                }
            }
            customer = patchEntity(existing, input);
            customer.channels.push(await this.connection.getEntityOrThrow(ctx, Channel, ctx.channelId));
        } else {
            // same as before
        }
        return this.connection.getRepository(ctx, Customer).save(customer);
    }

Would this solution make sense for you guys?

[mattgills]

Hi Michael,

Thank you for the feedback, I definitely understand the constraints you want to work with and appreciate the alternative solution to a schema change. I think this is a great idea and allows for anyone to define the logic they are looking for. We aren't yet looking for a specific outcome, other than avoiding the email address conflict. If we were to return an existing customer instead of creating a new one, does that leave us vulnerable to the address pollution that you mentioned before? Regardless this solution does offer the flexibility to do whatever we decide on!

[martijnvdbrug]

Referencing this issue: #762

[michaelbromley]

OK current thinking on this:

GuestCheckoutStrategy {
  addCustomerToOrder(ctx, input) {
  // possible actions to take here
  // - create a new guest customer, ignore existing registered customer
  // - return registered customer
  // - return error on conflict
  // - always error (no guest checkout allowed)
  }
}

This strategy would live in config.orderOptions. It would allow us to support pretty much any scenario I can think of, including the idea of Martijn in this comment -allowing you to create new Customers with the same email address as a registered Customer.

Going to play around with an implementation of this and see what comes up.

[martijnvdbrug]

That sounds like a good solution 👌 To create a new guest customer, we do need to make sure that emailadresses dont have to be unique anymore. I am not sure if any other logic is dependant on that?

[michaelbromley]

GuestCheckoutStrategy coming soon to v2!