From addff12652d9a9273e3f75362f8ca042ff1ba232 Mon Sep 17 00:00:00 2001 From: catborise Date: Mon, 26 Oct 2026 10:51:05 +0300 Subject: [PATCH] fix(admin): a default VM that is not exactly one VM is refused on the form NEW_USER_DEFAULT_INSTANCES names VMs, and a name may repeat across hosts. A name with no VM, or with one on two hosts, made creating any user a 500. The create form now looks the names up and says which one is not exactly one VM; the user and its default VMs are saved together. --- admin/forms.py | 13 +++++++++++++ admin/tests/test_audit_rows.py | 10 +--------- admin/tests/test_user_create.py | 31 +++++++++++++++++++++++++++++++ admin/views.py | 14 ++------------ 4 files changed, 47 insertions(+), 21 deletions(-) diff --git a/admin/forms.py b/admin/forms.py index 8d4f3639..f31149f2 100644 --- a/admin/forms.py +++ b/admin/forms.py @@ -1,4 +1,5 @@ from django import forms +from django.conf import settings from django.contrib.auth import password_validation from django.contrib.auth.forms import AdminPasswordChangeForm from django.contrib.auth.models import Group, User @@ -7,6 +8,7 @@ from django.utils.translation import gettext_lazy as _ from accounts.forms import UniqueEmailMixin from accounts.models import UserAttributes +from instances.models import Instance from .models import Permission @@ -85,6 +87,17 @@ class UserCreateForm(UserForm): content_type__app_label="accounts", codename="change_password" ) + def clean(self): + # the VMs a new user owns, by name: a name may repeat across hosts + cleaned = super().clean() + self.default_instances = [] + for name in settings.NEW_USER_DEFAULT_INSTANCES: + found = list(Instance.objects.filter(name=name)[:2]) + if len(found) != 1: + raise ValidationError(_("NEW_USER_DEFAULT_INSTANCES: %(name)s is not exactly one VM") % {"name": name}) + self.default_instances += found + return cleaned + def _post_clean(self): # The validators in AUTH_PASSWORD_VALIDATORS, as on the password change # forms; after the instance has the form's values (username, email). diff --git a/admin/tests/test_audit_rows.py b/admin/tests/test_audit_rows.py index 4984b7c8..7c67df07 100644 --- a/admin/tests/test_audit_rows.py +++ b/admin/tests/test_audit_rows.py @@ -1,7 +1,7 @@ """Changes to users and groups write rows on the Logs page.""" from django.contrib.auth.models import Group, Permission, User -from django.test import TestCase, override_settings +from django.test import TestCase from django.urls import reverse from logs.models import Logs @@ -77,14 +77,6 @@ class UserRowTests(TestCase): ]) self.assertFalse(Logs.objects.filter(message__contains="New-carol-pass-1").exists()) - @override_settings(NEW_USER_DEFAULT_INSTANCES=["no-such-vm"]) - def test_a_failing_default_vm_leaves_no_user(self): - from instances.models import Instance - with self.assertRaises(Instance.DoesNotExist): - self.create() - self.assertFalse(User.objects.filter(username="carol").exists()) - self.assertEqual(rows(), [("User carol: add: DoesNotExist", False)]) - def test_the_row_has_actor_and_ip(self): self.client.post( reverse("admin:user_create"), {"username": "carol", "password": "Carol-pass-2026", **ATTRIBUTES}, diff --git a/admin/tests/test_user_create.py b/admin/tests/test_user_create.py index cdb0b5c7..36837479 100644 --- a/admin/tests/test_user_create.py +++ b/admin/tests/test_user_create.py @@ -6,6 +6,10 @@ from django.db import connection from django.test import TestCase, override_settings from django.urls import reverse +from accounts.models import UserInstance +from computes.models import Compute +from instances.models import Instance + RAW = "Raw-Passw0rd-in-the-form" LIMITS = {"max_instances": 1, "max_cpus": 1, "max_memory": 1024, "max_disk_size": 4} @@ -63,3 +67,30 @@ class UserCreateTests(TestCase): ) self.assertEqual(response.status_code, 200) self.assertContains(response, "too similar to the username") + + +class DefaultInstancesTests(TestCase): + """NEW_USER_DEFAULT_INSTANCES names VMs, and a name may repeat across hosts.""" + + def setUp(self): + self.client.force_login(User.objects.create_superuser("uc-admin", "uc@example.com", "pw")) + one, two = (Compute.objects.create(name=name, hostname="localhost", login="", password="", details="", type=4) + for name in ("uc-one", "uc-two")) + self.web = Instance.objects.create(compute=one, name="web", uuid="00000000-0000-0000-0000-000000000001") + for compute, uuid in ((one, "00000000-0000-0000-0000-000000000002"), (two, "00000000-0000-0000-0000-000000000003")): + Instance.objects.create(compute=compute, name="db", uuid=uuid) + + def create(self): + return self.client.post(reverse("admin:user_create"), {"username": "newbie", "password": RAW, **LIMITS}) + + @override_settings(NEW_USER_DEFAULT_INSTANCES=["web"]) + def test_the_new_user_owns_the_named_vm(self): + self.assertRedirects(self.create(), reverse("admin:user_list")) + self.assertEqual([o.instance for o in UserInstance.objects.filter(user__username="newbie")], [self.web]) + + def test_a_name_that_is_not_one_vm_is_refused_on_the_form(self): + for name in ("no-such-vm", "db"): + with self.subTest(name), override_settings(NEW_USER_DEFAULT_INSTANCES=["web", name]): + response = self.create() + self.assertContains(response, f"NEW_USER_DEFAULT_INSTANCES: {name} is not exactly one VM") + self.assertFalse(User.objects.filter(username="newbie").exists()) diff --git a/admin/views.py b/admin/views.py index cf744e26..bc244100 100644 --- a/admin/views.py +++ b/admin/views.py @@ -127,7 +127,8 @@ def user_create(request): attributes = attributes_form.save(commit=False) attributes.user = user attributes.save() - add_default_instances(user) + UserInstance.objects.bulk_create(UserInstance(user=user, instance=instance) + for instance in user_form.default_instances) return redirect("admin:user_list") return render( @@ -346,14 +347,3 @@ def tombstone_delete(request, pk): "consequence": _("Its owners no longer get the VM back if its UUID shows up again.")}, ) - -def add_default_instances(user): - """ - Adds instances listed in NEW_USER_DEFAULT_INSTANCES to user - """ - existing_instances = UserInstance.objects.filter(user=user) - if not existing_instances: - for instance_name in settings.NEW_USER_DEFAULT_INSTANCES: - instance = Instance.objects.get(name=instance_name) - user_instance = UserInstance(user=user, instance=instance) - user_instance.save()