Merge pull request #100 from django-oscar/fix-checkout-view

The checkout view should not use the wrong mixin to check the basket ownership

oscarapi/tests/testcheckout.py

@@ -349,8 +349,8 @@ def test_anonymous_checkout(self):
             'shipping_method_code': "no-shipping-required",
             'shipping_charge': {
                 'currency': 'EUR',
-                'excl_tax':'0.00',
-                'tax':'0.00'
+                'excl_tax': '0.00',
+                'tax': '0.00'
             },
             "shipping_address": {
                 "country": "http://127.0.0.1:8000/api/countries/NL/",
@@ -396,10 +396,59 @@ def test_post_checkout_signal_send(self, mock):
         self.test_anonymous_checkout()
         self.assertTrue(mock.called)
 
-    @unittest.skip
     def test_checkout_permissions(self):
         "Prove that someone can not check out someone elses cart by mistake"
-        self.fail('Please add implementation')
+
+        # first login as nobody
+        self.login(username='nobody', password='nobody')
+        response = self.get('api-basket')
+
+        # store this basket because somebody is going to checkout with this
+        basket = response.data
+        nobody_basket_url = basket.get('url')
+
+        response = self.post(
+            'api-basket-add-product',
+            url="http://testserver/api/products/1/", quantity=5)
+
+        self.client.logout()
+
+        # now login as smomebody and fill another basket
+        self.login(username='somebody', password='somebody')
+        response = self.post(
+            'api-basket-add-product',
+            url="http://testserver/api/products/1/", quantity=5)
+
+        # so let's checkout with nobody's basket WHAHAAAHAHA!
+        request = {
+            'basket': nobody_basket_url,
+            'total': '50.0',
+            'shipping_method_code': "no-shipping-required",
+            'shipping_charge': {
+                'currency': 'EUR',
+                'excl_tax': '0.00',
+                'tax': '0.00'
+            },
+            "shipping_address": {
+                "country": "http://127.0.0.1:8000/api/countries/NL/",
+                "first_name": "Henk",
+                "last_name": "Van den Heuvel",
+                "line1": "Roemerlaan 44",
+                "line2": "",
+                "line3": "",
+                "line4": "Kroekingen",
+                "notes": "Niet STUK MAKEN OK!!!!",
+                "phone_number": "+31 26 370 4887",
+                "postcode": "7777KK",
+                "state": "Gerendrecht",
+                "title": "Mr"
+            }
+        }
+
+        # Oh, this is indeed not possible
+        response = self.post('api-checkout', **request)
+        self.assertEqual(response.status_code, 401)
+        self.assertEqual(response.data, "Unauthorized")
 
     @unittest.skip
     def test_cart_immutable_after_checkout(self):

oscarapi/views/checkout.py

@@ -1,16 +1,13 @@
-from rest_framework import status, views, response, generics
-
 from oscar.core.loading import get_model
+from oscarapi.basket.operations import request_contains_basket
+from oscarapi.permissions import IsOwner
 from oscarapi.serializers import (
-    OrderSerializer,
-    CheckoutSerializer,
-    OrderLineSerializer,
-    OrderLineAttributeSerializer,
-    UserAddressSerializer,
+    CheckoutSerializer, OrderLineAttributeSerializer, OrderLineSerializer,
+    OrderSerializer, UserAddressSerializer
 )
-from oscarapi.permissions import IsOwner
-from oscarapi.views.utils import BasketPermissionMixin
 from oscarapi.signals import oscarapi_post_checkout
+from oscarapi.views.utils import parse_basket_from_hyperlink
+from rest_framework import generics, response, status, views
 
 Order = get_model('order', 'Order')
 OrderLine = get_model('order', 'Line')
@@ -74,7 +71,7 @@ class OrderLineAttributeDetail(generics.RetrieveAPIView):
     serializer_class = OrderLineAttributeSerializer
 
 
-class CheckoutView(BasketPermissionMixin, views.APIView):
+class CheckoutView(views.APIView):
     """
     Prepare an order for checkout.
 
@@ -115,16 +112,15 @@ def post(self, request, format=None):
         # at the moment, no options are passed to this method, which means they
         # are also not created.
 
-        data_basket = self.get_data_basket(request.data, format)
-        basket = self.check_basket_permission(request,
-                                              basket_pk=data_basket.pk)
+        basket = parse_basket_from_hyperlink(request.data, format)
 
-        # by now an error should have been raised if someone was messing
-        # around with the basket, so asume invariant
-        assert(data_basket == basket)
+        if not request_contains_basket(request, basket):
+            return response.Response(
+                "Unauthorized", status=status.HTTP_401_UNAUTHORIZED)
 
         c_ser = self.serializer_class(
             data=request.data, context={'request': request})
+
         if c_ser.is_valid():
             order = c_ser.save()
             basket.freeze()

oscarapi/views/utils.py

@@ -1,16 +1,33 @@
 from django.core.exceptions import ValidationError
+
 from oscar.core.loading import get_model
-from rest_framework import generics, exceptions
-from rest_framework.relations import HyperlinkedRelatedField
 
 from oscarapi import permissions
 
+from rest_framework import exceptions, generics
+from rest_framework.relations import HyperlinkedRelatedField
 
 __all__ = ('BasketPermissionMixin',)
 
 Basket = get_model('basket', 'Basket')
 
 
+def parse_basket_from_hyperlink(DATA, format):  # noqa
+    "Parse basket from relation hyperlink"
+    basket_parser = HyperlinkedRelatedField(
+        view_name='basket-detail',
+        queryset=Basket.objects,
+        format=format
+    )
+    try:
+        basket_uri = DATA.get('basket')
+        data_basket = basket_parser.to_internal_value(basket_uri)
+    except ValidationError as e:
+        raise exceptions.NotAcceptable(e.messages)
+    else:
+        return data_basket
+
+
 class BasketPermissionMixin(object):
     """
     This mixins adds some methods that can be used to check permissions
@@ -19,20 +36,8 @@ class BasketPermissionMixin(object):
     # The permission class is mainly used to check Basket permission!
     permission_classes = (permissions.IsAdminUserOrRequestContainsBasket,)
 
-    def get_data_basket(self, DATA, format):
-        "Parse basket from relation hyperlink"
-        basket_parser = HyperlinkedRelatedField(
-            view_name='basket-detail',
-            queryset=Basket.objects,
-            format=format
-        )
-        try:
-            basket_uri = DATA.get('basket')
-            data_basket = basket_parser.to_internal_value(basket_uri)
-        except ValidationError as e:
-            raise exceptions.NotAcceptable(e.messages)
-        else:
-            return data_basket
+    def get_data_basket(self, DATA, format):  # noqa
+        return parse_basket_from_hyperlink(DATA, format)
 
     def check_basket_permission(self, request, basket_pk=None, basket=None):
         "Check if the user may access this basket"