CVE-2020-7964

@@ -54,6 +54,7 @@ All notable, unreleased changes to this project will be documented in this file.
 - Drop gettext occurrences - #5189 by @IKarbowiak
 - Fix `product_created` webhook - #5187 by @dzkb
 - Drop unused resolver `resolve_availability` - #5190 by @maarcingebala
+- Fix permission for `checkoutCustomerAttach` mutation - #5192 by @maarcingebala
 
 ## 2.9.0
 

@@ -5,6 +5,7 @@
 from django.core.exceptions import ObjectDoesNotExist, ValidationError
 from django.db import transaction
 from django.db.models import Prefetch
+from graphql_jwt.exceptions import PermissionDenied
 
 from ...account.error_codes import AccountErrorCode
 from ...checkout import models
@@ -35,7 +36,7 @@
 from ...product import models as product_models
 from ...warehouse.availability import check_stock_quantity, get_available_quantity
 from ..account.i18n import I18nMixin
-from ..account.types import AddressInput, User
+from ..account.types import AddressInput
 from ..core.mutations import (
     BaseMutation,
     ClearMetaBaseMutation,
@@ -404,22 +405,39 @@ class CheckoutCustomerAttach(BaseMutation):
 
     class Arguments:
         checkout_id = graphene.ID(required=True, description="ID of the checkout.")
-        customer_id = graphene.ID(required=True, description="The ID of the customer.")
+        customer_id = graphene.ID(
+            required=False,
+            description=(
+                "The ID of the customer. DEPRECATED: This field is deprecated and will "
+                "be removed in Saleor 2.11. To identify a customer you should "
+                "authenticate with JWT token."
+            ),
+        )
 
     class Meta:
         description = "Sets the customer as the owner of the checkout."
         error_type_class = CheckoutError
         error_type_field = "checkout_errors"
 
     @classmethod
-    def perform_mutation(cls, _root, info, checkout_id, customer_id):
+    def check_permissions(cls, context):
+        return context.user.is_authenticated
+
+    @classmethod
+    def perform_mutation(cls, _root, info, checkout_id, customer_id=None):
         checkout = cls.get_node_or_error(
             info, checkout_id, only_type=Checkout, field="checkout_id"
         )
-        customer = cls.get_node_or_error(
-            info, customer_id, only_type=User, field="customer_id"
-        )
-        checkout.user = customer
+
+        # Check if provided customer_id matches with the authenticated user and raise
+        # error if it doesn't. This part can be removed when `customer_id` field is
+        # removed.
+        if customer_id:
+            current_user_id = graphene.Node.to_global_id("User", info.context.user.id)
+            if current_user_id != customer_id:
+                raise PermissionDenied()
+
+        checkout.user = info.context.user
         checkout.save(update_fields=["user", "last_change"])
         return CheckoutCustomerAttach(checkout=checkout)
 
@@ -435,11 +453,20 @@ class Meta:
         error_type_class = CheckoutError
         error_type_field = "checkout_errors"
 
+    @classmethod
+    def check_permissions(cls, context):
+        return context.user.is_authenticated
+
     @classmethod
     def perform_mutation(cls, _root, info, checkout_id):
         checkout = cls.get_node_or_error(
             info, checkout_id, only_type=Checkout, field="checkout_id"
         )
+
+        # Raise error if the current user doesn't own the checkout of the given ID.
+        if checkout.user and checkout.user != info.context.user:
+            raise PermissionDenied()
+
         checkout.user = None
         checkout.save(update_fields=["user", "last_change"])
         return CheckoutCustomerDetach(checkout=checkout)

@@ -2288,7 +2288,7 @@ type Mutation {
   checkoutBillingAddressUpdate(billingAddress: AddressInput!, checkoutId: ID!): CheckoutBillingAddressUpdate
   checkoutComplete(checkoutId: ID!, redirectUrl: String, storeSource: Boolean = false): CheckoutComplete
   checkoutCreate(input: CheckoutCreateInput!): CheckoutCreate
-  checkoutCustomerAttach(checkoutId: ID!, customerId: ID!): CheckoutCustomerAttach
+  checkoutCustomerAttach(checkoutId: ID!, customerId: ID): CheckoutCustomerAttach
   checkoutCustomerDetach(checkoutId: ID!): CheckoutCustomerDetach
   checkoutEmailUpdate(checkoutId: ID, email: String!): CheckoutEmailUpdate
   checkoutLineDelete(checkoutId: ID!, lineId: ID): CheckoutLineDelete

@@ -8,6 +8,7 @@
 from django.core.exceptions import ValidationError
 from prices import Money, TaxedMoney
 
+from saleor.account.models import User
 from saleor.checkout import calculations
 from saleor.checkout.error_codes import CheckoutErrorCode
 from saleor.checkout.models import Checkout
@@ -25,7 +26,7 @@
 from saleor.shipping import ShippingMethodType
 from saleor.shipping.models import ShippingMethod
 from saleor.warehouse.models import Stock
-from tests.api.utils import get_graphql_content
+from tests.api.utils import assert_no_permission, get_graphql_content
 
 
 @pytest.fixture
@@ -851,7 +852,9 @@ def test_checkout_line_delete_by_zero_quantity(
     mocked_update_shipping_method.assert_called_once_with(checkout, mock.ANY)
 
 
-def test_checkout_customer_attach(user_api_client, checkout_with_item, customer_user):
+def test_checkout_customer_attach(
+    api_client, user_api_client, checkout_with_item, customer_user
+):
     checkout = checkout_with_item
     assert checkout.user is None
 
@@ -871,16 +874,26 @@ def test_checkout_customer_attach(user_api_client, checkout_with_item, customer_
     """
     checkout_id = graphene.Node.to_global_id("Checkout", checkout.pk)
     customer_id = graphene.Node.to_global_id("User", customer_user.pk)
-
     variables = {"checkoutId": checkout_id, "customerId": customer_id}
+
+    # Mutation should fail for unauthenticated customers
+    response = api_client.post_graphql(query, variables)
+    assert_no_permission(response)
+
+    # Mutation should succeed for authenticated customer
     response = user_api_client.post_graphql(query, variables)
     content = get_graphql_content(response)
-
     data = content["data"]["checkoutCustomerAttach"]
     assert not data["errors"]
     checkout.refresh_from_db()
     assert checkout.user == customer_user
 
+    # Mutation with ID of a different user should fail as well
+    other_customer = User.objects.create_user("othercustomer@example.com", "password")
+    variables["customerId"] = graphene.Node.to_global_id("User", other_customer.pk)
+    response = user_api_client.post_graphql(query, variables)
+    assert_no_permission(response)
+
 
 MUTATION_CHECKOUT_CUSTOMER_DETACH = """
     mutation checkoutCustomerDetach($checkoutId: ID!) {
@@ -904,16 +917,26 @@ def test_checkout_customer_detach(user_api_client, checkout_with_item, customer_
 
     checkout_id = graphene.Node.to_global_id("Checkout", checkout.pk)
     variables = {"checkoutId": checkout_id}
+
+    # Mutation should succeed if the user owns this checkout.
     response = user_api_client.post_graphql(
         MUTATION_CHECKOUT_CUSTOMER_DETACH, variables
     )
     content = get_graphql_content(response)
-
     data = content["data"]["checkoutCustomerDetach"]
     assert not data["errors"]
     checkout.refresh_from_db()
     assert checkout.user is None
 
+    # Mutation should fail when user calling it doesn't own the checkout.
+    other_user = User.objects.create_user("othercustomer@example.com", "password")
+    checkout.user = other_user
+    checkout.save()
+    response = user_api_client.post_graphql(
+        MUTATION_CHECKOUT_CUSTOMER_DETACH, variables
+    )
+    assert_no_permission(response)
+
 
 MUTATION_CHECKOUT_SHIPPING_ADDRESS_UPDATE = """
     mutation checkoutShippingAddressUpdate(