fix(core): Correctly handle multiple external auth methods

Fixes #695

packages/core/e2e/authentication-strategy.e2e-spec.ts

@@ -9,7 +9,11 @@ import path from 'path';
 import { initialData } from '../../../e2e-common/e2e-initial-data';
 import { testConfig, TEST_SETUP_TIMEOUT_MS } from '../../../e2e-common/test-config';
 
-import { TestAuthenticationStrategy, VALID_AUTH_TOKEN } from './fixtures/test-authentication-strategies';
+import {
+    TestAuthenticationStrategy,
+    TestAuthenticationStrategy2,
+    VALID_AUTH_TOKEN,
+} from './fixtures/test-authentication-strategies';
 import { CURRENT_USER_FRAGMENT } from './graphql/fragments';
 import {
     Authenticate,
@@ -36,6 +40,7 @@ describe('AuthenticationStrategy', () => {
                 shopAuthenticationStrategy: [
                     new NativeAuthenticationStrategy(),
                     new TestAuthenticationStrategy(),
+                    new TestAuthenticationStrategy2(),
                 ],
             },
         }),
@@ -83,7 +88,7 @@ describe('AuthenticationStrategy', () => {
             expect(authenticate.authenticationError).toBe('');
         });
 
-        it('fails with an expried token', async () => {
+        it('fails with an expired token', async () => {
             const { authenticate } = await shopClient.query(AUTHENTICATE, {
                 input: {
                     test_strategy: {
@@ -180,7 +185,6 @@ describe('AuthenticationStrategy', () => {
                 },
             });
             currentUserGuard.assertSuccess(authenticate);
-
             expect(authenticate.identifier).toEqual(userData.email);
 
             const { customers: after } = await adminClient.query<GetCustomers.Query>(GET_CUSTOMERS);
@@ -191,6 +195,54 @@ describe('AuthenticationStrategy', () => {
             ]);
         });
 
+        // https://github.com/vendure-ecommerce/vendure/issues/695
+        it('multiple external auth strategies to not interfere with one another', async () => {
+            const EXPECTED_CUSTOMERS = [
+                {
+                    emailAddress: 'hayden.zieme12@hotmail.com',
+                    id: 'T_1',
+                },
+                {
+                    emailAddress: 'test@email.com',
+                    id: 'T_2',
+                },
+            ];
+
+            const { customers: customers1 } = await adminClient.query<GetCustomers.Query>(GET_CUSTOMERS);
+            expect(customers1.items).toEqual(EXPECTED_CUSTOMERS);
+            const { authenticate: auth1 } = await shopClient.query<Authenticate.Mutation>(AUTHENTICATE, {
+                input: {
+                    test_strategy2: {
+                        token: VALID_AUTH_TOKEN,
+                        email: userData.email,
+                    },
+                },
+            });
+
+            currentUserGuard.assertSuccess(auth1);
+            expect(auth1.identifier).toBe(userData.email);
+
+            const { customers: customers2 } = await adminClient.query<GetCustomers.Query>(GET_CUSTOMERS);
+            expect(customers2.items).toEqual(EXPECTED_CUSTOMERS);
+
+            await shopClient.asAnonymousUser();
+
+            const { authenticate: auth2 } = await shopClient.query<Authenticate.Mutation>(AUTHENTICATE, {
+                input: {
+                    test_strategy: {
+                        token: VALID_AUTH_TOKEN,
+                        userData,
+                    },
+                },
+            });
+
+            currentUserGuard.assertSuccess(auth2);
+            expect(auth2.identifier).toBe(userData.email);
+
+            const { customers: customers3 } = await adminClient.query<GetCustomers.Query>(GET_CUSTOMERS);
+            expect(customers3.items).toEqual(EXPECTED_CUSTOMERS);
+        });
+
         it('registerCustomerAccount with external email', async () => {
             const successErrorGuard: ErrorResultGuard<{ success: boolean }> = createErrorResultGuard(
                 input => input.success != null,
@@ -214,8 +266,12 @@ describe('AuthenticationStrategy', () => {
                 id: newCustomerId,
             });
 
-            expect(customer?.user?.authenticationMethods.length).toBe(2);
-            expect(customer?.user?.authenticationMethods[1].strategy).toBe('native');
+            expect(customer?.user?.authenticationMethods.length).toBe(3);
+            expect(customer?.user?.authenticationMethods.map(m => m.strategy)).toEqual([
+                'test_strategy',
+                'test_strategy2',
+                'native',
+            ]);
 
             const { customer: customer2 } = await adminClient.query<
                 GetCustomerHistory.Query,
@@ -223,7 +279,7 @@ describe('AuthenticationStrategy', () => {
             >(GET_CUSTOMER_HISTORY, {
                 id: newCustomerId,
                 options: {
-                    skip: 2,
+                    skip: 4,
                 },
             });
 

packages/core/e2e/fixtures/test-authentication-strategies.ts

@@ -64,3 +64,44 @@ export class TestAuthenticationStrategy implements AuthenticationStrategy<TestAu
         });
     }
 }
+
+export class TestAuthenticationStrategy2 implements AuthenticationStrategy<{ token: string; email: string }> {
+    readonly name = 'test_strategy2';
+    private externalAuthenticationService: ExternalAuthenticationService;
+
+    init(injector: Injector) {
+        this.externalAuthenticationService = injector.get(ExternalAuthenticationService);
+    }
+
+    defineInputType(): DocumentNode {
+        return gql`
+            input TestAuth2Input {
+                token: String!
+                email: String!
+            }
+        `;
+    }
+
+    async authenticate(
+        ctx: RequestContext,
+        data: { token: string; email: string },
+    ): Promise<User | false | string> {
+        const { token, email } = data;
+        if (token !== VALID_AUTH_TOKEN) {
+            return false;
+        }
+        const user = await this.externalAuthenticationService.findCustomerUser(ctx, this.name, token);
+        if (user) {
+            return user;
+        }
+        const result = await this.externalAuthenticationService.createCustomerAndUser(ctx, {
+            strategy: this.name,
+            externalIdentifier: data.token,
+            emailAddress: email,
+            firstName: 'test',
+            lastName: 'test',
+            verified: true,
+        });
+        return result;
+    }
+}

packages/core/src/service/helpers/external-authentication/external-authentication.service.ts

@@ -13,6 +13,7 @@ import { ChannelService } from '../../services/channel.service';
 import { CustomerService } from '../../services/customer.service';
 import { HistoryService } from '../../services/history.service';
 import { RoleService } from '../../services/role.service';
+import { UserService } from '../../services/user.service';
 import { TransactionalConnection } from '../../transaction/transactional-connection';
 
 /**
@@ -31,6 +32,7 @@ export class ExternalAuthenticationService {
         private customerService: CustomerService,
         private administratorService: AdministratorService,
         private channelService: ChannelService,
+        private userService: UserService,
     ) {}
 
     /**
@@ -92,13 +94,20 @@ export class ExternalAuthenticationService {
             lastName?: string;
         },
     ): Promise<User> {
-        const customerRole = await this.roleService.getCustomerRole();
-        const newUser = new User({
-            identifier: config.emailAddress,
-            roles: [customerRole],
-            verified: config.verified || false,
-        });
-
+        let user: User;
+
+        const existingUser = await this.userService.getUserByEmailAddress(ctx, config.emailAddress);
+        if (existingUser) {
+            user = existingUser;
+        } else {
+            const customerRole = await this.roleService.getCustomerRole();
+            user = new User({
+                identifier: config.emailAddress,
+                roles: [customerRole],
+                verified: config.verified || false,
+                authenticationMethods: [],
+            });
+        }
         const authMethod = await this.connection.getRepository(ctx, ExternalAuthenticationMethod).save(
             new ExternalAuthenticationMethod({
                 externalIdentifier: config.externalIdentifier,
@@ -106,15 +115,21 @@ export class ExternalAuthenticationService {
             }),
         );
 
-        newUser.authenticationMethods = [authMethod];
-        const savedUser = await this.connection.getRepository(ctx, User).save(newUser);
+        user.authenticationMethods = [...(user.authenticationMethods || []), authMethod];
+        const savedUser = await this.connection.getRepository(ctx, User).save(user);
 
-        const customer = new Customer({
-            emailAddress: config.emailAddress,
-            firstName: config.firstName,
-            lastName: config.lastName,
-            user: savedUser,
-        });
+        let customer: Customer;
+        const existingCustomer = await this.customerService.findOneByUserId(ctx, savedUser.id);
+        if (existingCustomer) {
+            customer = existingCustomer;
+        } else {
+            customer = new Customer({
+                emailAddress: config.emailAddress,
+                firstName: config.firstName,
+                lastName: config.lastName,
+                user: savedUser,
+            });
+        }
         this.channelService.assignToCurrentChannel(customer, ctx);
         await this.connection.getRepository(ctx, Customer).save(customer);
 
@@ -187,14 +202,25 @@ export class ExternalAuthenticationService {
         return newUser;
     }
 
-    findUser(ctx: RequestContext, strategy: string, externalIdentifier: string): Promise<User | undefined> {
-        return this.connection
+    async findUser(
+        ctx: RequestContext,
+        strategy: string,
+        externalIdentifier: string,
+    ): Promise<User | undefined> {
+        const user = await this.connection
             .getRepository(ctx, User)
             .createQueryBuilder('user')
             .leftJoinAndSelect('user.authenticationMethods', 'authMethod')
-            .where('authMethod.strategy = :strategy', { strategy })
             .andWhere('authMethod.externalIdentifier = :externalIdentifier', { externalIdentifier })
             .andWhere('user.deletedAt IS NULL')
             .getOne();
+
+        const userHasMatchingAuthMethod = !!user?.authenticationMethods.find(m => {
+            return m instanceof ExternalAuthenticationMethod && m.strategy === strategy;
+        });
+
+        if (userHasMatchingAuthMethod) {
+            return user;
+        }
     }
 }

packages/core/src/service/services/customer.service.ts

@@ -338,7 +338,9 @@ export class CustomerService {
         if (!user.verified) {
             user = await this.userService.setVerificationToken(ctx, user);
         }
+
         customer.user = user;
+        await this.connection.getRepository(ctx, User).save(user, { reload: false });
         await this.connection.getRepository(ctx, Customer).save(customer, { reload: false });
         if (!user.verified) {
             this.eventBus.publish(new AccountRegistrationEvent(ctx, user));

packages/core/src/service/services/user.service.ts

@@ -67,6 +67,17 @@ export class UserService {
         identifier: string,
         password?: string,
     ): Promise<User> {
+        const checkUser = user.id != null && (await this.getUserById(ctx, user.id));
+        if (checkUser) {
+            if (
+                !!checkUser.authenticationMethods.find(
+                    (m): m is NativeAuthenticationMethod => m instanceof NativeAuthenticationMethod,
+                )
+            ) {
+                // User already has a NativeAuthenticationMethod registered, so just return.
+                return user;
+            }
+        }
         const authenticationMethod = new NativeAuthenticationMethod();
         if (this.configService.authOptions.requireVerification) {
             authenticationMethod.verificationToken = this.verificationTokenGenerator.generateVerificationToken();