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

External Authentication Conflicts for Admins and Customers #926

Closed
mattgills opened this issue Jun 10, 2021 · 0 comments
Closed

External Authentication Conflicts for Admins and Customers #926

mattgills opened this issue Jun 10, 2021 · 0 comments
Assignees
Labels
type: bug 🐛 Something isn't working

Comments

@mattgills
Copy link
Contributor

Describe the bug
If a Vendure instance uses SSO for both shop and admin login, then someone who logs into to both with the same email will have permissions issues for certain API requests. For example, if an admin exists with external auth strategy ssoAdmin, SuperAdmin privileges, and is related to a user record with id 1, then the function ExternalAuthenticationService.findCustomerUser will return undefined because user id 1 does not have any customers associated with it. As a result, one would create a new customer via ExternalAuthenticationService.createCustomerAndUser. However since this function will match user id 1 via email address query, a new customer is created with respect to user id 1 (even though it is a SuperAdmin). This customer is able to login to the storefront, and complete most actions, such as setOrderBillingAddress. One failing action, however, is the addPaymentToOrder mutation, which prevents checkout from completing altogether. The key difference between the two is that setOrderBillingAddress has permission of ['Owner'], while addPaymentToOrder has permissions ['UpdateOrder', 'Owner']. This will cause the following differences in the request-context.service.ts:

// setBillingAddress
// requiredPermissions = ['Owner']
// hasOwnerPermission = true
// isAuthorized = false
// authorizedAsOwnerOnly = !false && true = true
// Successful request because authorizedAsOwnerOnly is true

// addPaymentToOrder
// requiredPermissions = ['UpdateOrder', 'Owner']
// hasOwnerPermission = true
// isAuthorized = true => SuperAdmin has 'UpdateOrder'
// authorizedAsOwnerOnly = !true && true = false
// Fails the request because authorizedAsOwnerOnly is false and throws no active order found

To Reproduce
Steps to reproduce the behavior:

  1. Manually create an admin with external auth strategy ssoAdmin and SuperAdmin permissions in the DB. This can be done by creating an admin through the Admin UI and modifying the authentication_method table. This also can be done programmatically through the external auth service.
  2. Create a class that implements AuthenticationStrategy that has the strategy name ssoShop
    • This class should inject the ExternalAuthenticationService and implement the authenticate function
    • The authenticate function should first check if the customer exists via this.externalAuthenticationService.findCustomerUser. If the customer doesn't exist it should return this.externalAuthenticationService.createCustomerAndUser(...) (i.e. this should be very similar to the documentation for the AuthenticationStrategy class).
  3. Login to the storefront with the same email from step 1 via the ssoShop authentication strategy
  4. Build a cart
  5. Checkout should allow you to proceed up to the payment step, successfully adding shipping and billing addresses
  6. Payment should fail, digging into the network tab will reveal that the mutation failed with error NO_ACTIVE_ORDER, indicating that ctx.authorizedAsOwnerOnly = false
  7. If you view the database you will see that the the customer created with ssoShop will be related to the same user as the admin from step 1. The user_roles_role table will show that the user has SuperAdmin privileges leading to the issue described in the first section of this bug report.

Expected behavior
I would expect the admin and customer to be associated with two different users, so there is no overlapping of SuperAdmin and Customer roles.

Environment (please complete the following information):

  • @vendure/core version: 1.0.1
  • Nodejs version: 14.17.0
  • Database (mysql/postgres etc): postgres

Additional context
One area of investigation could be the following function in the ExternalAuthenticationService:

  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')
          .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;
      }
  }

Since the query is getOne() the earliest user entry in the DB will be returned. Since we created the admin first, the customer ends up associated with the admin role. The reverse should be true if we created the customer first. It may be worth using getMany() and checking whether each user has an admin or customer relationship (in addition to the existing checks).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🐛 Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants