`cookies_to_delete` is always an empty list

[kevinrenskers]

When BasketMiddleware adds something to request.cookies_to_delete, it is never actually deleted inside of ApiBasketMiddleWare.process_response. This is because ApiBasketMiddleWare.__call__ calls super(ApiBasketMiddleWare, self).__call__(request), which also has the line request.cookies_to_delete = [] (see here).

So the result of this is that cookies that should be getting deleted are not getting deleted. For example, when I am logged in as a user and then log out, the oscar_open_basket cookie still has a basket_id value of the logged in user's basket. And BasketMiddleware doesn't recognize such a basket for anonymous users:

    def get_cookie_basket(self, cookie_key, request, manager):
        """
        Looks for a basket which is referenced by a cookie.

        If a cookie key is found with no matching basket, then we add
        it to the list to be deleted.
        """
        basket = None
        if cookie_key in request.COOKIES:
            basket_hash = request.COOKIES[cookie_key]
            try:
                basket_id = Signer().unsign(basket_hash)
                basket = Basket.objects.get(pk=basket_id, owner=None,
                                            status=Basket.OPEN)
            except (BadSignature, Basket.DoesNotExist):
                request.cookies_to_delete.append(cookie_key)
        return basket

As you can see, it filters on owner=None and since the basket in the cookie still belongs to someone, it doesn't find the basket, creates a brand new basket, and it also want to delete the cookie. But this deletion does not happen.

[kevinrenskers]

The way cookies are handled are actually a huge problem I think. At the moment when somebody is logged in, and then logs out, and I fetch their baskets using the BasketList view, I get the basket back that belongs to the previously logged in user. You can then proceed to add products to the basket, but it is then impossible to checkout, because the basket that is used by Oscar itself (stored in the request object) is a completely blank basket without an id, and nothing in it.

[kevinrenskers]

Actually not sure if it's caused by the cookie or not; when I manually remove the oscar_open_basket from my browser's storage, I still get the previous user's basket back from BasketList. Why is that an anonymous user gets a basket with an owner id? Oscar itself doesn't allow this (see that owner=None filter I mentioned above) so now oscar and oscarapi are using two separate baskets.

[kevinrenskers]

The method get_anonymous_basket doesn't do any check to see if the owner is None, and get_basket_id_from_session relies on request.session.get(settings.OSCAR_BASKET_COOKIE_OPEN), which still returns the basket id from the previously logged in user, after you logged out.

So something is not getting cleared when logging out it seems?

[kevinrenskers]

My workaround for now is to use my own LoginView:

from oscarapi.views.login import LoginView as CoreLoginView

class LoginView(CoreLoginView):
    def delete(self, request, *args, **kwargs):
        """
        Destroy the session.

        For anonymous users that means having their basket destroyed as well,
        because there is no way to reach it otherwise.
        """
        request = request._request
        if request.user.is_anonymous:
            basket = operations.get_anonymous_basket(request)
            if basket:
                operations.flush_and_delete_basket(basket)

        request.session.flush()

        # Create a new basket for the anonymous user
        basket = Basket.objects.create()
        basket.save()
        store_basket_in_session(basket, request.session)

        response = Response("")

        response.set_cookie(
            settings.OSCAR_BASKET_COOKIE_OPEN,
            Signer().sign(basket.id),
            max_age=settings.OSCAR_BASKET_COOKIE_LIFETIME,
            domain=settings.SESSION_COOKIE_DOMAIN,
            secure=settings.SESSION_COOKIE_SECURE,
            httponly=settings.SESSION_COOKIE_HTTPONLY,
            samesite=settings.SESSION_COOKIE_SAMESITE,
        )

        return response

So when the user logs out I manually create a new basket, store that in the session, and set a new cookie.

[specialunderwear]

Hi @kevinrenskers could you write a test that proves your point and create a pull request? I'm working on the basket middleware right now and I think I solve this based on your description, but it would be awesome to be able to verify with a test.

[specialunderwear]

Maybe you can test the pull request and see if it solves your problem @kevinrenskers

[kevinrenskers]

I'll try, but that won't be before the middle of next week at the earliest.

[specialunderwear]

I had to revert the change that was meant to fix this.

[specialunderwear]

@kevinrenskers I can't reproduce this problem at all. I can only confirm that the cookie of the anonymous user does not get deleted after login. However the moment I access the loggedin user via the api, the cookie is deleted and the basket is moved to the session. I can not confirm that basket listings contain baskets from other users. Could it be that you are logged in as superuser? Superusers can see baskets that belong to other users.

[kevinrenskers]

I'm not seeing a basket that belongs to some random other user, but from the user I was previously logged in as.

So let's say I am logged in as User A, then I log out. This should result in a new basket getting created for the anonymous user, but instead the basket for User A is still being used.

This is the root cause:

When BasketMiddleware adds something to request.cookies_to_delete, it is never actually deleted inside of ApiBasketMiddleWare.process_response. This is because ApiBasketMiddleWare.__call__ calls super(ApiBasketMiddleWare, self).__call__(request), which also has the line request.cookies_to_delete = [] (see here).

The result is that the person who was logged in and then logged out, can not add anything to the basket:

The way cookies are handled are actually a huge problem I think. At the moment when somebody is logged in, and then logs out, and I fetch their baskets using the BasketList view, I get the basket back that belongs to the previously logged in user. You can then proceed to add products to the basket, but it is then impossible to checkout, because the basket that is used by Oscar itself (stored in the request object) is a completely blank basket without an id, and nothing in it.

Basically Oscar and OscarAPI end up with two separate baskets and the user can not checkout at all.

[specialunderwear]

@kevinrenskers could you modify the test in #319 that I added to make it reproduce what you describe? I tried to reproduce it, but I can't get it to show the problem you are describing, as you can see in the test ...

I never claim I couldn't reproduce seeing random users basket, I can't see any other baskets at all, not from the previously loggedin user, none, only the basket that belongs to the current user. There might be some specific sequence of actions that need to be performed in the api and the normal website to trigger this. I can't seem to find it though.

[kevinrenskers]

I'm not sure how to add unit tests for this situation. I've encountered it in real world use of OscarAPI but it'll take quite a lot of time to replicate that in a unit test, I'd first have to get much more familiar with your code. Time I sadly don't have, especially since we as a company have decided to move away from Oscar / OscarAPI, so for us it doesn't make a lot of sense to put more time into this issue. Sorry :/

I did notice there's no unit tests for ApiBasketMiddleWare, maybe that would be a good start?

Otherwise feel free to close this issue if nobody else is running into it and it can't be reproduced.