From 69f46a365711e5fbc5de5610f061e3eb85a279a8 Mon Sep 17 00:00:00 2001 From: Michael Bromley Date: Wed, 16 Jun 2021 14:27:59 +0200 Subject: [PATCH] fix(core): Fix Admin/Customer user conflict with external auth Fixes #926 --- .../e2e/authentication-strategy.e2e-spec.ts | 31 +++++++- .../test-authentication-strategies.ts | 71 +++++++++++++++++++ .../external-authentication.service.ts | 36 ++++++---- 3 files changed, 124 insertions(+), 14 deletions(-) diff --git a/packages/core/e2e/authentication-strategy.e2e-spec.ts b/packages/core/e2e/authentication-strategy.e2e-spec.ts index 0147718e8a..9e69697743 100644 --- a/packages/core/e2e/authentication-strategy.e2e-spec.ts +++ b/packages/core/e2e/authentication-strategy.e2e-spec.ts @@ -12,13 +12,14 @@ import { testConfig, TEST_SETUP_TIMEOUT_MS } from '../../../e2e-common/test-conf import { TestAuthenticationStrategy, TestAuthenticationStrategy2, + TestSSOStrategyAdmin, + TestSSOStrategyShop, VALID_AUTH_TOKEN, } from './fixtures/test-authentication-strategies'; import { CURRENT_USER_FRAGMENT } from './graphql/fragments'; import { Authenticate, CreateCustomer, - CurrentUser, CurrentUserFragment, CustomerFragment, DeleteCustomer, @@ -31,7 +32,6 @@ import { import { Register } from './graphql/generated-e2e-shop-types'; import { CREATE_CUSTOMER, DELETE_CUSTOMER, GET_CUSTOMER_HISTORY, ME } from './graphql/shared-definitions'; import { REGISTER_ACCOUNT } from './graphql/shop-definitions'; -import { assertThrowsWithMessage } from './utils/assert-throws-with-message'; describe('AuthenticationStrategy', () => { const { server, adminClient, shopClient } = createTestEnvironment( @@ -41,7 +41,9 @@ describe('AuthenticationStrategy', () => { new NativeAuthenticationStrategy(), new TestAuthenticationStrategy(), new TestAuthenticationStrategy2(), + new TestSSOStrategyShop(), ], + adminAuthenticationStrategy: [new NativeAuthenticationStrategy(), new TestSSOStrategyAdmin()], }, }), ); @@ -292,6 +294,31 @@ describe('AuthenticationStrategy', () => { }, ]); }); + + // https://github.com/vendure-ecommerce/vendure/issues/926 + it('Customer and Admin external auth does not reuse same User for different strategies', async () => { + const emailAddress = 'hello@test-domain.com'; + await adminClient.asAnonymousUser(); + const { authenticate: adminAuth } = await adminClient.query(AUTHENTICATE, { + input: { + test_sso_strategy_admin: { + email: emailAddress, + }, + }, + }); + currentUserGuard.assertSuccess(adminAuth); + + const { authenticate: shopAuth } = await shopClient.query(AUTHENTICATE, { + input: { + test_sso_strategy_shop: { + email: emailAddress, + }, + }, + }); + currentUserGuard.assertSuccess(shopAuth); + + expect(adminAuth.id).not.toBe(shopAuth.id); + }); }); describe('native auth', () => { diff --git a/packages/core/e2e/fixtures/test-authentication-strategies.ts b/packages/core/e2e/fixtures/test-authentication-strategies.ts index 0ea6b2e01c..eb4d1fb2c3 100644 --- a/packages/core/e2e/fixtures/test-authentication-strategies.ts +++ b/packages/core/e2e/fixtures/test-authentication-strategies.ts @@ -3,6 +3,7 @@ import { ExternalAuthenticationService, Injector, RequestContext, + RoleService, User, } from '@vendure/core'; import { DocumentNode } from 'graphql'; @@ -65,6 +66,76 @@ export class TestAuthenticationStrategy implements AuthenticationStrategy { + readonly name = 'test_sso_strategy_admin'; + private externalAuthenticationService: ExternalAuthenticationService; + private roleService: RoleService; + + init(injector: Injector) { + this.externalAuthenticationService = injector.get(ExternalAuthenticationService); + this.roleService = injector.get(RoleService); + } + + defineInputType(): DocumentNode { + return gql` + input TestSSOInputAdmin { + email: String! + } + `; + } + + async authenticate(ctx: RequestContext, data: { email: string }): Promise { + const { email } = data; + const user = await this.externalAuthenticationService.findUser(ctx, this.name, email); + if (user) { + return user; + } + const superAdminRole = await this.roleService.getSuperAdminRole(); + return this.externalAuthenticationService.createAdministratorAndUser(ctx, { + strategy: this.name, + externalIdentifier: email, + emailAddress: email, + firstName: 'SSO Admin First Name', + lastName: 'SSO Admin Last Name', + identifier: email, + roles: [superAdminRole], + }); + } +} + +export class TestSSOStrategyShop implements AuthenticationStrategy<{ email: string }> { + readonly name = 'test_sso_strategy_shop'; + private externalAuthenticationService: ExternalAuthenticationService; + + init(injector: Injector) { + this.externalAuthenticationService = injector.get(ExternalAuthenticationService); + } + + defineInputType(): DocumentNode { + return gql` + input TestSSOInputShop { + email: String! + } + `; + } + + async authenticate(ctx: RequestContext, data: { email: string }): Promise { + const { email } = data; + const user = await this.externalAuthenticationService.findUser(ctx, this.name, email); + if (user) { + return user; + } + return this.externalAuthenticationService.createCustomerAndUser(ctx, { + strategy: this.name, + externalIdentifier: email, + emailAddress: email, + firstName: 'SSO Customer First Name', + lastName: 'SSO Customer Last Name', + verified: true, + }); + } +} + export class TestAuthenticationStrategy2 implements AuthenticationStrategy<{ token: string; email: string }> { readonly name = 'test_strategy2'; private externalAuthenticationService: ExternalAuthenticationService; diff --git a/packages/core/src/service/helpers/external-authentication/external-authentication.service.ts b/packages/core/src/service/helpers/external-authentication/external-authentication.service.ts index 1ca318ab52..17e46790d0 100644 --- a/packages/core/src/service/helpers/external-authentication/external-authentication.service.ts +++ b/packages/core/src/service/helpers/external-authentication/external-authentication.service.ts @@ -4,7 +4,6 @@ import { HistoryEntryType } from '@vendure/common/lib/generated-types'; import { RequestContext } from '../../../api/common/request-context'; import { Administrator } from '../../../entity/administrator/administrator.entity'; import { ExternalAuthenticationMethod } from '../../../entity/authentication-method/external-authentication-method.entity'; -import { Collection } from '../../../entity/collection/collection.entity'; import { Customer } from '../../../entity/customer/customer.entity'; import { Role } from '../../../entity/role/role.entity'; import { User } from '../../../entity/user/user.entity'; @@ -13,7 +12,6 @@ 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'; /** @@ -32,7 +30,6 @@ export class ExternalAuthenticationService { private customerService: CustomerService, private administratorService: AdministratorService, private channelService: ChannelService, - private userService: UserService, ) {} /** @@ -96,7 +93,8 @@ export class ExternalAuthenticationService { ): Promise { let user: User; - const existingUser = await this.userService.getUserByEmailAddress(ctx, config.emailAddress); + const existingUser = await this.findExistingCustomerUserByEmailAddress(ctx, config.emailAddress); + if (existingUser) { user = existingUser; } else { @@ -207,20 +205,34 @@ export class ExternalAuthenticationService { strategy: string, externalIdentifier: string, ): Promise { - const user = await this.connection + const usersWithMatchingIdentifier = await this.connection .getRepository(ctx, User) .createQueryBuilder('user') .leftJoinAndSelect('user.authenticationMethods', 'authMethod') .andWhere('authMethod.externalIdentifier = :externalIdentifier', { externalIdentifier }) .andWhere('user.deletedAt IS NULL') - .getOne(); + .getMany(); - const userHasMatchingAuthMethod = !!user?.authenticationMethods.find(m => { - return m instanceof ExternalAuthenticationMethod && m.strategy === strategy; - }); + const matchingUser = usersWithMatchingIdentifier.find(user => + user.authenticationMethods.find( + m => m instanceof ExternalAuthenticationMethod && m.strategy === strategy, + ), + ); - if (userHasMatchingAuthMethod) { - return user; - } + return matchingUser; + } + + private async findExistingCustomerUserByEmailAddress(ctx: RequestContext, emailAddress: string) { + const customer = await this.connection + .getRepository(ctx, Customer) + .createQueryBuilder('customer') + .leftJoinAndSelect('customer.user', 'user') + .leftJoin('customer.channels', 'channel') + .leftJoinAndSelect('user.authenticationMethods', 'authMethod') + .andWhere('customer.emailAddress = :emailAddress', { emailAddress }) + .andWhere('user.deletedAt IS NULL') + .getOne(); + + return customer?.user; } }