From 3d632f159268c9307d57121dd61cd92944adcca0 Mon Sep 17 00:00:00 2001 From: catborise Date: Fri, 30 Oct 2026 17:02:38 +0300 Subject: [PATCH] feat(instances): a VM's owners with their rights, and XML edits kept until saved Settings > Users was a bold title, an unlabelled "+" and a table of names with a trash icon. The owner chose this layout from three rendered options: one list of owners, each with what it may do (change settings, delete the VM, change console settings, or power and console only), Edit rights and Remove, an empty state, and a labelled "Add owner". - Remove and Edit rights come back to the VM's Users tab: the return address carried #users unencoded, so it was lost, and Edit rights went to the user's account page. Both accounts views now go to a ?next page of this site. - The add-owner list offers only users who are not owners yet and not administrators, who can use every VM anyway; a superuser owner also took the only place when a VM may have one owner. - The unused del_owner view and URL are removed: the list uses the accounts page, which asks before removing an owner. XML: - An XML edit not saved yet is kept: the page no longer reloads itself when the VM's state changes, and the browser asks before leaving it. A state change lost the edit. - The button that opens the review dialog reads "Review changes"; it did not save, as "Change" suggested. Closing the dialog gives focus back to the editor. --- accounts/tests/test_views.py | 19 ++++++ accounts/views.py | 22 ++++--- .../templates/add_instance_owner_block.html | 29 +++++---- instances/templates/instance.html | 3 +- .../templates/instances/settings_tab.html | 60 +++++++++++-------- instances/tests/test_audit_rows.py | 2 +- instances/tests/test_error_handling.py | 10 +--- instances/tests/test_pending_edits.py | 18 ++++++ instances/tests/test_views.py | 6 +- instances/urls.py | 5 +- instances/views.py | 16 +---- static/js/xml_editor.js | 20 ++++++- 12 files changed, 125 insertions(+), 85 deletions(-) diff --git a/accounts/tests/test_views.py b/accounts/tests/test_views.py index 41e58e87..46f01ffe 100644 --- a/accounts/tests/test_views.py +++ b/accounts/tests/test_views.py @@ -311,3 +311,22 @@ class SingleOwnerEditTests(TestCase): self.assertRedirects(response, reverse("accounts:account", args=[bob.pk]), fetch_redirect_response=False) ui.refresh_from_db() self.assertTrue(ui.is_vnc) + + +class OwnerRightsReturnTestCase(TestCase): + """Edit rights on a VM's Users tab comes back to that tab; a page of + another site does not count.""" + + def test_an_owner_s_rights_come_back_to_the_vm_page(self): + admin = get_user_model().objects.create_superuser("rights-admin", "rights@example.com", "x") + owner = get_user_model().objects.create_user("rights-owner") + compute = Compute.objects.create(name="rights", hostname="192.0.2.30", login="u", password="p", type=1) + vm = Instance.objects.create(compute=compute, name="rights-vm", uuid="12121212-0000-0000-0000-000000000000") + user_instance = UserInstance.objects.create(user=owner, instance=vm) + self.client.force_login(admin) + update = reverse("accounts:user_instance_update", args=[user_instance.id]) + for next_url, back in ((f"/instances/{vm.id}/%23users", f"/instances/{vm.id}/#users"), + ("https://example.com/", reverse("accounts:account", args=[owner.id]))): + with self.subTest(next_url): + response = self.client.post(f"{update}?next={next_url}", {"is_change": True}) + self.assertRedirects(response, back, fetch_redirect_response=False) diff --git a/accounts/views.py b/accounts/views.py index f8faa260..80fde617 100644 --- a/accounts/views.py +++ b/accounts/views.py @@ -235,6 +235,17 @@ def user_instance_create(request, user_id): ) +def _next_or(request, default): + """To the ?next page, the VM page an owner was changed from, when it is + on this site; else to default.""" + next_url = request.GET.get("next") + if next_url and url_has_allowed_host_and_scheme( + url=next_url, allowed_hosts={request.get_host()}, require_https=request.is_secure() + ): + return redirect(next_url) + return redirect(default) + + @superuser_only def user_instance_update(request, pk): user_instance = get_object_or_404(UserInstance, pk=pk) @@ -246,7 +257,7 @@ def user_instance_update(request, pk): fields = changed_fields(before, snapshot(user_instance)) entry.action = f"change owner {user_instance.user.username}: " + ", ".join(fields) entry.skip = not fields - return redirect(reverse("accounts:account", args=[user_instance.user.id])) + return _next_or(request, reverse("accounts:account", args=[user_instance.user.id])) return render( request, @@ -265,14 +276,7 @@ def user_instance_delete(request, pk): user, instance = user_instance.user, user_instance.instance with audit_vm(request, instance, f"remove owner {user.username}"): user_instance.delete() - next_url = request.GET.get("next", None) - if next_url and url_has_allowed_host_and_scheme( - url=next_url, - allowed_hosts={request.get_host()}, - require_https=request.is_secure(), - ): - return redirect(next_url) - return redirect(reverse("accounts:account", args=[user.id])) + return _next_or(request, reverse("accounts:account", args=[user.id])) return render( request, diff --git a/instances/templates/add_instance_owner_block.html b/instances/templates/add_instance_owner_block.html index b26a4bfc..58d91c0e 100644 --- a/instances/templates/add_instance_owner_block.html +++ b/instances/templates/add_instance_owner_block.html @@ -1,35 +1,34 @@ {% load i18n %} {% load bootstrap_icons %} {% if request.user.is_superuser %} - - - +
{% if instance.status != 5 %} -

{% trans "This is the definition for the VM's next start; a saved change applies then." %}

+

{% trans "This is the definition for the VM's next start; a saved change applies then." %}

{% endif %}
{% csrf_token %}
- +
-
-
- {% trans "Instance owners" %} - {% include 'add_instance_owner_block.html' %} -
-
-
- - +
+
+

{% trans "Instance owners" %}

+ {% include 'add_instance_owner_block.html' %} +
+

{% trans "Every owner can power the VM and open its console. A new owner has no other right until it is given one." %}

+ {% if userinstances %} +
    {% for userinstance in userinstances %} -
- - - +
  • +
    +
    + {{ userinstance.user }} +

    + {% if userinstance.is_change or userinstance.is_delete %} + {% if userinstance.is_change %}{% trans "Change settings" %}{% endif %}{% if userinstance.is_change and userinstance.is_delete %} · {% endif %}{% if userinstance.is_delete %}{% trans "Delete the VM" %}{% endif %}{% if userinstance.is_change and userinstance.is_vnc %} · {% trans "Change console settings" %}{% endif %} + {% else %} + {% trans "Power and console only" %} + {% endif %} +

    +
    + {% if request.user.is_superuser %} +
    + {% trans "Edit rights" %} + {% trans "Remove" %} +
    + {% endif %} +
    +
    • {% endfor %} - -
    {{ userinstance.user }} - - {% bs_icon 'trash' %} - -
    -
    -
    + + {% else %} +

    {% trans "The VM has no owner: only administrators can use it." %}

    + {% endif %} +
    {% endif %} {% if can_manage_console %} diff --git a/instances/tests/test_audit_rows.py b/instances/tests/test_audit_rows.py index c64ddcb0..94eebe22 100644 --- a/instances/tests/test_audit_rows.py +++ b/instances/tests/test_audit_rows.py @@ -31,7 +31,7 @@ class VMOwnerRowTests(TestCase): self.client.post(add, {"user_id": self.carol.pk}) self.assertFalse(UserInstance.objects.filter(user=self.carol).exists()) ui = UserInstance.objects.get(user=self.bob) - self.client.post(reverse("instances:del_owner", args=[self.vm.pk]), {"userinstance": ui.pk}) + self.client.post(reverse("accounts:user_instance_delete", args=[ui.pk])) self.assertEqual(rows(), [ ("VM vm: add owner bob", True), ("VM vm: add owner carol: only one owner is allowed", False), diff --git a/instances/tests/test_error_handling.py b/instances/tests/test_error_handling.py index d4ca3511..d1691a0f 100644 --- a/instances/tests/test_error_handling.py +++ b/instances/tests/test_error_handling.py @@ -3,7 +3,6 @@ import inspect from unittest.mock import patch -from accounts.models import UserInstance from computes.models import Compute from django.contrib.auth import get_user_model from django.contrib.messages import get_messages @@ -49,8 +48,6 @@ class ErrorHandlingTestCase(TestCase): # Required fields for the views that 404 without them DATA = { "add_owner": lambda self: {"user_id": get_user_model().objects.get(username="err_admin").id}, - "del_owner": lambda self: {"userinstance": UserInstance.objects.get_or_create( - instance=self.instance, user=get_user_model().objects.get(username="err_admin"))[0].id}, "add_network": lambda self: {"add-net-network": "net:default"}, "set_qos": lambda self: {"qos_direction": "inbound", "net-mac-0": "52:54:00:00:00:01"}, } @@ -68,13 +65,8 @@ class ErrorHandlingTestCase(TestCase): self.assertEqual(response.status_code, 302) self.assertTrue(response.url.startswith(self.page + "#"), response.url) - def test_owner_views_reject_missing_or_foreign_ids(self): - other = Instance.objects.create(compute=self.compute, name="other", uuid="11111111-2222-3333-4444-777777777777") - foreign = UserInstance.objects.create(instance=other, user=get_user_model().objects.get(username="err_admin")) + def test_add_owner_rejects_a_missing_user(self): self.assertEqual(self.client.post(self.url("add_owner"), {}).status_code, 404) - self.assertEqual(self.client.post(self.url("del_owner"), {}).status_code, 404) - self.assertEqual(self.client.post(self.url("del_owner"), {"userinstance": foreign.id}).status_code, 404) - self.assertTrue(UserInstance.objects.filter(pk=foreign.id).exists()) def test_same_site_referer_is_kept(self): with patch("instances.models.wvmInstance"): diff --git a/instances/tests/test_pending_edits.py b/instances/tests/test_pending_edits.py index 99be4186..ce6e0cc5 100644 --- a/instances/tests/test_pending_edits.py +++ b/instances/tests/test_pending_edits.py @@ -3,6 +3,7 @@ until the next start. The edit forms show that definition, so a save does not post the running VM's value back over a pending change.""" from unittest.mock import PropertyMock, patch +from urllib.parse import unquote from computes.models import Compute from django.contrib.auth import get_user_model @@ -150,6 +151,23 @@ class PendingEditsTestCase(TestCase): self.assertEqual(doc.xpath("//input[@name='rng']/@checked"), ["checked"]) self.assertEqual(doc.xpath("//select[@name='on_crash']/option[@selected]/@value"), ["restart"]) + def test_the_users_tab_shows_each_owner_and_offers_only_new_owners(self): + from accounts.models import UserInstance + + User = get_user_model() + owner = User.objects.create_user("pe-owner") + UserInstance.objects.create(instance=self.instance, user=owner, is_change=True, is_vnc=True) + User.objects.create_user("pe-new") + User.objects.create_superuser("pe-admin2", "pe2@example.com", "x") + doc = self.page() + (row,) = doc.xpath("//div[@id='users']//li") + self.assertIn("Change settings · Change console settings", " ".join(row.text_content().split())) + # Remove and Edit rights come back to the Users tab + self.assertEqual([unquote(href.split("?next=")[1]) for href in row.xpath(".//a[contains(@class, 'btn')]/@href")], + [f"/instances/{self.instance.id}/#users"] * 2) + # not an owner yet and not an administrator, who can use every VM anyway + self.assertEqual(doc.xpath("//select[@id='add-owner-user']/option/text()"), ["pe-new"]) + def test_buttons_shown_as_disabled_cannot_submit(self): # running: the CPU resize, vCPU hot plug, template and disk removal forms wait for a shutdown; # the title, description and video model are of the next start, saved at once diff --git a/instances/tests/test_views.py b/instances/tests/test_views.py index 2a920537..e621f1fc 100644 --- a/instances/tests/test_views.py +++ b/instances/tests/test_views.py @@ -643,11 +643,7 @@ class InstancesTestCase(TestCase): self.assertEqual(response.status_code, 302) self.assertEqual(UserInstance.objects.count(), 1) - response = self.client.post( - reverse("instances:del_owner", args=[self.instance.id]), - {"userinstance": user_instance.id}, - HTTP_REFERER=reverse("index"), - ) + response = self.client.post(reverse("accounts:user_instance_delete", args=[user_instance.id])) self.assertEqual(response.status_code, 302) self.assertEqual(UserInstance.objects.count(), 0) diff --git a/instances/urls.py b/instances/urls.py index e7d8e541..b3848d10 100755 --- a/instances/urls.py +++ b/instances/urls.py @@ -68,10 +68,7 @@ urlpatterns = [ path("/delete_pci_hostdev/", views.delete_pci_hostdev, name="delete_pci_hostdev"), path("/set_link_state/", views.set_link_state, name="set_link_state"), path("/set_qos/", views.set_qos, name="set_qos"), - path("/unset_qos/", views.unset_qos, name="unset_qos"), - path( - "/del_owner/", views.del_owner, name="del_owner" - ), # no links to this one??? + path("/unset_qos/", views.unset_qos, name="unset_qos"), # no links to this one??? path("/clone/", views.clone, name="clone"), path("/update_console/", views.update_console, name="update_console"), path("/change_options/", views.change_options, name="change_options"), diff --git a/instances/views.py b/instances/views.py index 46b95b24..dbda8ea6 100755 --- a/instances/views.py +++ b/instances/views.py @@ -239,7 +239,8 @@ def instance(request, pk): def _instance_page(request, pk, instance, compute): computes = Compute.objects.all().order_by("name") computes_count = computes.count() - users = User.objects.all().order_by("username") + # the add-owner choices: users who are not owners yet; a superuser can use every VM + users = User.objects.filter(is_superuser=False).exclude(userinstance__instance=instance).order_by("username") publickeys = UserSSHKey.objects.filter(user_id=request.user.id) keymaps = settings.QEMU_KEYMAPS # The console form renders the VNC password, so only users who may manage @@ -2096,19 +2097,6 @@ def add_owner(request, pk): return _back(request, pk, "users") -@require_POST -@superuser_only -def del_owner(request, pk): - instance = get_instance(request.user, pk) - userinstance_id = request.POST.get("userinstance", "") - if not userinstance_id.isdigit(): - raise Http404 - userinstance = get_object_or_404(UserInstance, pk=userinstance_id, instance=instance) - with audit_vm(request, instance, f"remove owner {userinstance.user.username}"): - userinstance.delete() - return _back(request, pk, "users") - - # Buses for the empty CD-ROM of a new VM: fdc and virtio have no ejectable # media, and libvirt refuses an empty usb disk. CDROM_BUSES = ("ide", "sata", "scsi") diff --git a/static/js/xml_editor.js b/static/js/xml_editor.js index f2527856..b24b8657 100644 --- a/static/js/xml_editor.js +++ b/static/js/xml_editor.js @@ -1,6 +1,8 @@ -// The VM page's XML editor. Change shows the lines the edit changes in the -// VM's definition (instances.views.change_xml), and Save in that dialog +// The VM page's XML editor. Review changes shows the lines the edit changes +// in the VM's definition (instances.views.change_xml), and Save in that dialog // defines it. A refusal shows in the dialog, and the edit stays in the editor. +// An edit not saved yet keeps the page from reloading itself on a VM state +// change (window.xmlEditUnsaved), and the browser asks before leaving it. (function () { var form = document.getElementById("xml-form"); if (!form) { @@ -16,6 +18,19 @@ var diff = document.getElementById("xml-review-diff"); var save = document.getElementById("xml-review-save"); var reviewed = ""; // Save defines the XML the dialog shows, not later edits + var clean = base; // the XML the page has: the opened definition, or the one Save just defined + + window.xmlEditUnsaved = function () { + return editor.getValue() !== clean; + }; + window.addEventListener("beforeunload", function (event) { + if (window.xmlEditUnsaved()) { + event.preventDefault(); + } + }); + dialog.addEventListener("hidden.bs.modal", function () { + editor.focus(); + }); // The answer: diff, saved, or error (detail when the VM is busy or moved). // Any other failure, such as a redirect, keeps the edit and says so. @@ -70,6 +85,7 @@ save.disabled = true; post(reviewed, true).then(function (answer) { if (answer.saved) { + clean = reviewed; location.hash = "xmledit"; location.reload(); } else {