From cd2dd517b78e21b5e68bb3a834e7abbcb9925fc8 Mon Sep 17 00:00:00 2001 From: catborise Date: Sat, 31 Oct 2026 00:21:06 +0300 Subject: [PATCH] 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 | 14 ++++-- computes/utils.py | 3 +- instances/tests/test_owner_race.py | 78 ++++++++++++++++++++++++++++++ instances/views.py | 31 ++++++------ 4 files changed, 107 insertions(+), 19 deletions(-) create mode 100644 instances/tests/test_owner_race.py diff --git a/accounts/views.py b/accounts/views.py index 80fde617..9ec4f9be 100644 --- a/accounts/views.py +++ b/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, diff --git a/computes/utils.py b/computes/utils.py index 5938018e..ff4b61d0 100644 --- a/computes/utils.py +++ b/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]) diff --git a/instances/tests/test_owner_race.py b/instances/tests/test_owner_race.py new file mode 100644 index 00000000..ac20b02c --- /dev/null +++ b/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"]) diff --git a/instances/views.py b/instances/views.py index b6e91a33..1b28ee6f 100755 --- a/instances/views.py +++ b/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")