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 {