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 <weller@rami.io>

src/pretix/api/views/order.py

@@ -20,6 +20,7 @@
 # <https://www.gnu.org/licenses/>.
 #
 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:

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",
+            ),
+        ),
+    ]

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:

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

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,

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'

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 <https://pretix.eu/about/en/license>.
+#
+# 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
+# <https://www.gnu.org/licenses/>.
+#
+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

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'

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

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)