fix(instances): two owners added at once keep a VM to one owner

With one owner per VM, the VM page and a user's page counted the VM's
owners and then added one in separate steps, so two owners added at the
same moment both found the VM without one, and it got two. The check
and the insert now run in one transaction that holds the ownership lock
(PostgreSQL's advisory lock; SQLite's IMMEDIATE transactions already
serialize them): the second waits for the first and is refused. The
audit row is written after the transaction, so a failure keeps its row.

accounts/views.py

@@ -21,6 +21,7 @@ from django.views.decorators.cache import never_cache
 from django.views.decorators.http import require_POST
 
 from accounts.apps import admin_password_path
+from computes.utils import lock_ownership
 from logs.audit import audit, audit_vm, changed_fields, failed, snapshot
 
 from accounts.forms import ApiTokenForm, EmailOTPForm, ProfileForm, UserSSHKeyForm
@@ -221,9 +222,16 @@ def user_instance_create(request, user_id):
     form = forms.UserInstanceForm(request.POST or None, initial={"user": user})
     if form.is_valid():
         instance = form.cleaned_data["instance"]
-        with audit_vm(request, instance, f"add owner {form.cleaned_data['user'].username}"):
-            form.save()
-        return redirect(reverse("accounts:account", args=[user.id]))
+        # the form's one-owner check again, with the insert in one step, as on the VM page
+        with audit_vm(request, instance, f"add owner {form.cleaned_data['user'].username}") as entry, \
+                transaction.atomic():
+            lock_ownership()
+            form.full_clean()
+            entry.skip = not form.is_valid()
+            if form.is_valid():
+                form.save()
+        if form.is_valid():
+            return redirect(reverse("accounts:account", args=[user.id]))
 
     return render(
         request,

computes/utils.py

@@ -53,7 +53,8 @@ OWNERSHIP_LOCK = 0x7776635F6F776E
 def lock_ownership():
     """Hold the ownership step of every compute until this transaction ends:
     a bury and a restore of the same UUID on two computes must not see each
-    other's work half done. SQLite's IMMEDIATE transactions already do this."""
+    other's work half done, nor two owners added at once each other's check.
+    SQLite's IMMEDIATE transactions already do this."""
     if connection.vendor == "postgresql":
         with connection.cursor() as cursor:
             cursor.execute("SELECT pg_advisory_xact_lock(%s)", [OWNERSHIP_LOCK])

instances/tests/test_owner_race.py

@@ -0,0 +1,78 @@
+"""With one owner per VM, two owners added at the same moment, from the VM
+page or a user's page: the second waits for the first and is refused. SQLite's
+IMMEDIATE transactions serialize them; PostgreSQL takes the ownership lock."""
+
+import threading
+import time
+from unittest.mock import patch
+
+from accounts.models import UserInstance
+from appsettings.models import AppSettings
+from computes.models import Compute
+from django.contrib.auth import get_user_model
+from django.db import connection
+from django.test import Client, TransactionTestCase
+from django.urls import reverse
+from instances.models import Instance
+
+UUID = "55555555-5555-5555-5555-555555555555"
+
+
+class OwnerRaceTests(TransactionTestCase):
+    def setUp(self):
+        AppSettings.objects.update_or_create(
+            key="ALLOW_INSTANCE_MULTIPLE_OWNER",
+            defaults={"name": "Multiple owners", "value": "False", "choices": "True,False"},
+        )
+        users = get_user_model().objects
+        self.admin = users.create_superuser("race-admin", "race@example.com", "pw")
+        self.bob, self.carol = users.create_user("bob"), users.create_user("carol")
+        compute = Compute.objects.create(name="c1", hostname="127.0.0.1", login="", password="", type=4)
+        self.vm = Instance.objects.create(compute=compute, name="vm", uuid=UUID)
+
+    def post(self, url, data):
+        client = Client()
+        client.force_login(self.admin)
+        try:
+            self.answers.append(client.post(url, data).status_code)
+        finally:
+            connection.close()
+
+    def test_from_the_vm_page(self):
+        add = reverse("instances:add_owner", args=[self.vm.pk])
+        self.race(lambda: self.post(add, {"user_id": self.bob.pk}),
+                  lambda: self.post(add, {"user_id": self.carol.pk}))
+
+    def test_from_the_vm_page_and_a_user_s_page(self):
+        add = reverse("instances:add_owner", args=[self.vm.pk])
+        create = reverse("accounts:user_instance_create", args=[self.carol.pk])
+        self.race(lambda: self.post(add, {"user_id": self.bob.pk}),
+                  lambda: self.post(create, {"user": self.carol.pk, "instance": self.vm.pk}))
+
+    def race(self, first, second):
+        """first stops after its check, with its transaction open, while
+        second runs; second must then find first's owner. Both answer: the
+        VM page redirects, a refused user's page shows the form again."""
+        self.answers = []
+        checked, go = threading.Event(), threading.Event()
+        real = UserInstance.objects.get_or_create
+
+        def slow_get_or_create(**kwargs):
+            checked.set()
+            go.wait(10)
+            return real(**kwargs)
+
+        with patch.object(UserInstance.objects, "get_or_create", slow_get_or_create):
+            a = threading.Thread(target=first)
+            a.start()
+            self.assertTrue(checked.wait(10))
+            b = threading.Thread(target=second)
+            b.start()
+            time.sleep(1)  # second would add its owner now if nothing held it
+            go.set()
+            a.join(30)
+            b.join(30)
+
+        self.assertEqual(len(self.answers), 2)
+        self.assertTrue(set(self.answers) <= {200, 302}, self.answers)
+        self.assertEqual(list(UserInstance.objects.values_list("user__username", flat=True)), ["bob"])

instances/views.py

@@ -18,7 +18,7 @@ from appsettings.models import AppSettings
 from appsettings.settings import app_settings
 from computes.models import Compute
 from computes.validators import validate_hostname, validate_login
-from computes.utils import libvirt_compute_lock, libvirt_instance_lock, user_quota_lock
+from computes.utils import libvirt_compute_lock, libvirt_instance_lock, lock_ownership, user_quota_lock
 from django.conf import settings
 from django.contrib import messages
 from django.contrib.auth.decorators import permission_required
@@ -26,6 +26,7 @@ from django.contrib.auth.models import User
 from django.core import signing
 from django.core.exceptions import PermissionDenied, ValidationError
 from django.http import Http404, HttpResponse, JsonResponse
+from django.db import transaction
 from django.db.models import Prefetch, prefetch_related_objects
 from django.shortcuts import get_object_or_404, redirect, render
 from django.urls import reverse
@@ -2077,22 +2078,22 @@ def add_owner(request, pk):
         raise Http404
     user = get_object_or_404(User, pk=user_id)
 
-    check_inst = 0
-
-    if app_settings.ALLOW_INSTANCE_MULTIPLE_OWNER == "False":
-        check_inst = UserInstance.objects.filter(instance=instance).count()
-
     action = f"add owner {user.username}"
-    if check_inst > 0:
-        failed_vm(request, instance, action, "only one owner is allowed")
-        messages.error(
-            request, _("Only one owner is allowed and the one already added")
-        )
-    else:
-        with audit_vm(request, instance, action) as entry:
+    # the check and the insert in one step: two owners added at once must not
+    # both find the VM without one; the row is written after the transaction
+    with audit_vm(request, instance, action) as entry, transaction.atomic():
+        lock_ownership()
+        taken = (app_settings.ALLOW_INSTANCE_MULTIPLE_OWNER == "False"
+                 and UserInstance.objects.filter(instance=instance).exists())
+        if taken:
+            entry.skip = True
+        else:
             entry.skip = not UserInstance.objects.get_or_create(instance=instance, user=user)[1]
-        if entry.skip:
-            messages.warning(request, _("%(user)s is already an owner") % {"user": user})
+    if taken:
+        failed_vm(request, instance, action, "only one owner is allowed")
+        messages.error(request, _("Only one owner is allowed and the one already added"))
+    elif entry.skip:
+        messages.warning(request, _("%(user)s is already an owner") % {"user": user})
     return _back(request, pk, "users")