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")