From cda8144ff0cb84dec9f7c0cbcb2012bab345f2e0 Mon Sep 17 00:00:00 2001 From: Raphael Michel Date: Tue, 2 Apr 2024 11:07:40 +0200 Subject: [PATCH] Enforce uniqueness of order codes and ticket secrets (#3988) * Enforce uniqueness of order codes and ticket secrets * Fix test cases which created orders with identical codes --------- Co-authored-by: Mira Weller --- src/pretix/api/views/order.py | 11 +- src/pretix/base/migrations/0258_uniq_indx.py | 48 ++++++++ src/pretix/base/models/orders.py | 30 +++++ src/tests/api/test_order_create.py | 22 ++++ src/tests/base/test_invoices.py | 2 +- src/tests/base/test_models.py | 4 +- .../test_api_order_creation_code.py | 114 ++++++++++++++++++ src/tests/control/test_checkins.py | 2 +- .../banktransfer/test_refund_export.py | 2 +- src/tests/plugins/sendmail/test_rules.py | 4 +- 10 files changed, 230 insertions(+), 9 deletions(-) create mode 100644 src/pretix/base/migrations/0258_uniq_indx.py create mode 100644 src/tests/concurrency_tests/test_api_order_creation_code.py diff --git a/src/pretix/api/views/order.py b/src/pretix/api/views/order.py index de5b5ae892..b2566db364 100644 --- a/src/pretix/api/views/order.py +++ b/src/pretix/api/views/order.py @@ -20,6 +20,7 @@ # . # import datetime +import logging import mimetypes import os from decimal import Decimal @@ -27,7 +28,7 @@ from zoneinfo import ZoneInfo import django_filters from django.conf import settings -from django.db import transaction +from django.db import IntegrityError, transaction from django.db.models import ( Exists, F, OuterRef, Prefetch, Q, Subquery, prefetch_related_objects, ) @@ -97,6 +98,8 @@ from pretix.base.signals import ( from pretix.base.templatetags.money import money_filter from pretix.control.signals import order_search_filter_q +logger = logging.getLogger(__name__) + with scopes_disabled(): class OrderFilter(FilterSet): email = django_filters.CharFilter(field_name='email', lookup_expr='iexact') @@ -900,7 +903,11 @@ class EventOrderViewSet(OrderViewSetMixin, viewsets.ModelViewSet): order_modified.send(sender=serializer.instance.event, order=serializer.instance) def perform_create(self, serializer): - serializer.save() + try: + serializer.save() + except IntegrityError: + logger.exception("Integrity error while saving order") + raise ValidationError("Integrity error, possibly duplicate submission of same order.") def perform_destroy(self, instance): if not instance.testmode: diff --git a/src/pretix/base/migrations/0258_uniq_indx.py b/src/pretix/base/migrations/0258_uniq_indx.py new file mode 100644 index 0000000000..137542bca2 --- /dev/null +++ b/src/pretix/base/migrations/0258_uniq_indx.py @@ -0,0 +1,48 @@ +# Generated by Django 4.2.10 on 2024-03-15 09:59 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("pretixbase", "0257_item_default_price_not_null"), + ] + + operations = [ + migrations.AddField( + model_name="order", + name="organizer", + field=models.ForeignKey( + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="orders", + to="pretixbase.organizer", + ), + ), + migrations.AddField( + model_name="orderposition", + name="organizer", + field=models.ForeignKey( + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="order_positions", + to="pretixbase.organizer", + ), + ), + migrations.AddConstraint( + model_name="order", + constraint=models.UniqueConstraint( + fields=("organizer", "code"), name="order_organizer_code_uniq" + ), + ), + migrations.AddConstraint( + model_name="orderposition", + constraint=models.UniqueConstraint( + models.F("organizer"), + models.F("secret"), + name="orderposition_organizer_secret_uniq", + ), + ), + ] diff --git a/src/pretix/base/models/orders.py b/src/pretix/base/models/orders.py index 6f3208ddb5..0a7df23162 100644 --- a/src/pretix/base/models/orders.py +++ b/src/pretix/base/models/orders.py @@ -188,6 +188,14 @@ class Order(LockModel, LoggedModel): default=False, ) testmode = models.BooleanField(default=False) + organizer = models.ForeignKey( + # Redundant foreign key, but is required for a uniqueness constraint + "Organizer", + related_name="orders", + on_delete=models.CASCADE, + null=True, + blank=True, + ) event = models.ForeignKey( Event, verbose_name=_("Event"), @@ -286,6 +294,9 @@ class Order(LockModel, LoggedModel): models.Index(fields=["datetime", "id"]), models.Index(fields=["last_modified", "id"]), ] + constraints = [ + models.UniqueConstraint(fields=["organizer", "code"], name="order_organizer_code_uniq"), + ] def __str__(self): return self.full_code @@ -499,6 +510,10 @@ class Order(LockModel, LoggedModel): self.set_expires() if 'update_fields' in kwargs: kwargs['update_fields'] = {'expires'}.union(kwargs['update_fields']) + if not self.organizer_id: + self.organizer_id = self.event.organizer_id + if 'update_fields' in kwargs: + kwargs['update_fields'] = {'organizer'}.union(kwargs['update_fields']) is_new = not self.pk update_fields = kwargs.get('update_fields', []) @@ -2356,6 +2371,14 @@ class OrderPosition(AbstractPosition): """ positionid = models.PositiveIntegerField(default=1) + organizer = models.ForeignKey( + # Redundant foreign key, but is required for a uniqueness constraint + "Organizer", + related_name="order_positions", + on_delete=models.CASCADE, + null=True, + blank=True, + ) order = models.ForeignKey( Order, verbose_name=_("Order"), @@ -2429,6 +2452,9 @@ class OrderPosition(AbstractPosition): verbose_name = _("Order position") verbose_name_plural = _("Order positions") ordering = ("positionid", "id") + constraints = [ + models.UniqueConstraint("organizer", "secret", name="orderposition_organizer_secret_uniq") + ] @cached_property def sort_key(self): @@ -2585,6 +2611,10 @@ class OrderPosition(AbstractPosition): if 'update_fields' in kwargs: kwargs['update_fields'] = {'secret'}.union(kwargs['update_fields']) + if not self.organizer_id: + self.organizer_id = self.order.event.organizer_id + if 'update_fields' in kwargs: + kwargs['update_fields'] = {'organizer'}.union(kwargs['update_fields']) if not self.blocked and self.blocked is not None: self.blocked = None if 'update_fields' in kwargs: diff --git a/src/tests/api/test_order_create.py b/src/tests/api/test_order_create.py index 00c0318db5..6b752f72d3 100644 --- a/src/tests/api/test_order_create.py +++ b/src/tests/api/test_order_create.py @@ -279,6 +279,28 @@ def test_order_create(token_client, organizer, event, item, quota, question): assert o.transactions.count() == 2 +@pytest.mark.django_db +def test_order_create_duplicate_code(token_client, organizer, event, item, quota, question): + res = copy.deepcopy(ORDER_CREATE_PAYLOAD) + res['code'] = 'ABC12' + res['positions'][0]['item'] = item.pk + res['positions'][0]['answers'][0]['question'] = question.pk + resp = token_client.post( + '/api/v1/organizers/{}/events/{}/orders/'.format( + organizer.slug, event.slug + ), format='json', data=res + ) + assert resp.status_code == 201 + assert resp.data['code'] == 'ABC12' + resp = token_client.post( + '/api/v1/organizers/{}/events/{}/orders/'.format( + organizer.slug, event.slug + ), format='json', data=res + ) + assert resp.status_code == 400 + assert resp.data == {"code": ["This order code is already in use."]} + + @pytest.mark.django_db def test_order_create_max_size(token_client, organizer, event, item, quota, question): quota.size = settings.PRETIX_MAX_ORDER_SIZE * 2 diff --git a/src/tests/base/test_invoices.py b/src/tests/base/test_invoices.py index c6d47410c5..86f0016551 100644 --- a/src/tests/base/test_invoices.py +++ b/src/tests/base/test_invoices.py @@ -447,7 +447,7 @@ def test_invoice_numbers(env): order2.fees.create(fee_type=OrderFee.FEE_TYPE_PAYMENT, value=Decimal('0.25'), tax_rate=Decimal('0.00'), tax_value=Decimal('0.00')) testorder = Order.objects.create( - code='BAR', event=event, email='dummy2@dummy.test', + code='TESTBAR', event=event, email='dummy2@dummy.test', status=Order.STATUS_PENDING, datetime=now(), expires=now() + timedelta(days=10), total=0, testmode=True, diff --git a/src/tests/base/test_models.py b/src/tests/base/test_models.py index 33afa7ec79..8ee9d6c6ad 100644 --- a/src/tests/base/test_models.py +++ b/src/tests/base/test_models.py @@ -2581,7 +2581,7 @@ class CheckinListTestCase(TestCase): ) self.cl_tickets.limit_products.add(self.item1) o = Order.objects.create( - code='FOO', event=self.event, email='dummy@dummy.test', + code='FOO1', event=self.event, email='dummy@dummy.test', status=Order.STATUS_PAID, datetime=now(), expires=now() + timedelta(days=10), total=Decimal("30"), locale='en' @@ -2608,7 +2608,7 @@ class CheckinListTestCase(TestCase): op3.checkins.create(list=self.cl_both) o = Order.objects.create( - code='FOO', event=self.event, email='dummy@dummy.test', + code='FOO2', event=self.event, email='dummy@dummy.test', status=Order.STATUS_PENDING, datetime=now(), expires=now() + timedelta(days=10), total=Decimal("30"), locale='en' diff --git a/src/tests/concurrency_tests/test_api_order_creation_code.py b/src/tests/concurrency_tests/test_api_order_creation_code.py new file mode 100644 index 0000000000..fcb2a1aa3c --- /dev/null +++ b/src/tests/concurrency_tests/test_api_order_creation_code.py @@ -0,0 +1,114 @@ +# +# This file is part of pretix (Community Edition). +# +# Copyright (C) 2014-2020 Raphael Michel and contributors +# Copyright (C) 2020-2021 rami.io GmbH and contributors +# +# This program is free software: you can redistribute it and/or modify it under the terms of the GNU Affero General +# Public License as published by the Free Software Foundation in version 3 of the License. +# +# ADDITIONAL TERMS APPLY: Pursuant to Section 7 of the GNU Affero General Public License, additional terms are +# applicable granting you additional permissions and placing additional restrictions on your usage of this software. +# Please refer to the pretix LICENSE file to obtain the full terms applicable to this work. If you did not receive +# this file, see . +# +# This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied +# warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more +# details. +# +# You should have received a copy of the GNU Affero General Public License along with this program. If not, see +# . +# +import asyncio + +import pytest +from asgiref.sync import sync_to_async +from django_scopes import scopes_disabled + +from pretix.base.models import Order, OrderPosition + + +async def post_code(session, url, data, **kwargs): + async with session.post(url, json=data, **kwargs) as response: + return response.status + + +@pytest.mark.asyncio +async def test_quota_race_condition_same_order_code(live_server, session, device, event, item, quota): + url = f"/api/v1/organizers/{event.organizer.slug}/events/{event.slug}/orders/?_debug_flag=sleep-after-quota-check" + payload = { + "code": "ABC12", + "email": "dummy@dummy.test", + "phone": "+49622112345", + "locale": "en", + "sales_channel": "web", + "valid_if_pending": True, + "fees": [], + "payment_provider": "banktransfer", + "positions": [ + { + "positionid": 1, + "item": item.pk, + "variation": None, + "price": "23.00", + "attendee_name_parts": {"full_name": "Peter"}, + "attendee_email": None, + "addon_to": None, + "company": "FOOCORP", + "answers": [], + "subevent": None + } + ], + } + + r1, r2 = await asyncio.gather( + post_code(session, f"{live_server}{url}", data=payload, + headers={"Authorization": f"Device {device.api_token}"}), + post_code(session, f"{live_server}{url}", data=payload, + headers={"Authorization": f"Device {device.api_token}"}), + ) + with scopes_disabled(): + assert await sync_to_async(Order.objects.filter(code="ABC12").count)() == 1 + assert [r1, r2].count(201) == 1 + assert [r1, r2].count(400) == 1 + + +@pytest.mark.asyncio +async def test_quota_race_condition_same_ticket_secret(live_server, session, device, event, item, quota): + url = f"/api/v1/organizers/{event.organizer.slug}/events/{event.slug}/orders/?_debug_flag=sleep-after-quota-check" + payload = { + "email": "dummy@dummy.test", + "phone": "+49622112345", + "locale": "en", + "sales_channel": "web", + "valid_if_pending": True, + "fees": [], + "payment_provider": "banktransfer", + "positions": [ + { + "positionid": 1, + "item": item.pk, + "variation": None, + "price": "23.00", + "attendee_name_parts": {"full_name": "Peter"}, + "attendee_email": None, + "addon_to": None, + "company": "FOOCORP", + "answers": [], + "secret": "foobarbaz", + "subevent": None + } + ], + } + + # Other one is just assigned differently + r1, r2 = await asyncio.gather( + post_code(session, f"{live_server}{url}", data=payload, + headers={"Authorization": f"Device {device.api_token}"}), + post_code(session, f"{live_server}{url}", data=payload, + headers={"Authorization": f"Device {device.api_token}"}), + ) + with scopes_disabled(): + assert await sync_to_async(OrderPosition.objects.filter(secret="foobarbaz").count)() == 1 + assert await sync_to_async(OrderPosition.objects.exclude(secret="foobarbaz").count)() == 1 + assert [r1, r2].count(201) == 2 diff --git a/src/tests/control/test_checkins.py b/src/tests/control/test_checkins.py index 4640ef89ec..61151341c0 100644 --- a/src/tests/control/test_checkins.py +++ b/src/tests/control/test_checkins.py @@ -105,7 +105,7 @@ def test_dashboard(dashboard_env): def test_dashboard_pending_not_count(dashboard_env): c = checkin_widget(dashboard_env[0]) order_pending = Order.objects.create( - code='FOO', event=dashboard_env[0], email='dummy@dummy.test', + code='BAR', event=dashboard_env[0], email='dummy@dummy.test', status=Order.STATUS_PENDING, datetime=now(), expires=now() + timedelta(days=10), total=23, locale='en' diff --git a/src/tests/plugins/banktransfer/test_refund_export.py b/src/tests/plugins/banktransfer/test_refund_export.py index d450fbec91..5895ecea60 100644 --- a/src/tests/plugins/banktransfer/test_refund_export.py +++ b/src/tests/plugins/banktransfer/test_refund_export.py @@ -71,7 +71,7 @@ def refund_huf(env): date_from=now(), plugins='pretix.plugins.banktransfer,pretix.plugins.paypal' ) order = Order.objects.create( - code='1Z3AS', event=event, email='admin@localhost', + code='HUFFY', event=event, email='admin@localhost', status=Order.STATUS_PAID, datetime=now(), expires=now() + timedelta(days=10), total=42 diff --git a/src/tests/plugins/sendmail/test_rules.py b/src/tests/plugins/sendmail/test_rules.py index 0314d8c85d..45815e263e 100644 --- a/src/tests/plugins/sendmail/test_rules.py +++ b/src/tests/plugins/sendmail/test_rules.py @@ -283,13 +283,13 @@ def test_sendmail_rule_all_subevents(event_series, subevent1, subevent2, item): o1 = Order.objects.create(event=item.event, status=Order.STATUS_PAID, expires=now() + datetime.timedelta(hours=1), - total=13, code='DUMMY', email='dummy1@dummy.test', + total=13, code='DUMMY1', email='dummy1@dummy.test', datetime=now(), locale='en') o1.all_positions.create(item=item, price=13, subevent=subevent1) o1.all_positions.create(item=item, price=13, subevent=subevent2) o2 = Order.objects.create(event=item.event, status=Order.STATUS_PAID, expires=now() + datetime.timedelta(hours=1), - total=13, code='DUMMY', email='dummy2@dummy.test', + total=13, code='DUMMY2', email='dummy2@dummy.test', datetime=now(), locale='en') o2.all_positions.create(item=item, price=23, subevent=subevent1) o2.all_positions.create(item=item, price=23, subevent=subevent2)