From 792487e65e179f877bf0f1199fbc971dad990196 Mon Sep 17 00:00:00 2001 From: catborise Date: Sun, 4 Oct 2026 12:23:54 +0300 Subject: [PATCH] fix(instances): failures reach the user; no redirect to other sites Web views: - 34 views returned to request.META["HTTP_REFERER"] + "#tab": without a Referer they failed with a TypeError (500), and a Referer from another site was followed (open redirect). They return through _back(), which keeps a same-site Referer and otherwise opens the VM page at the tab. get_safe_redirect uses the same check. Rejected disk options also return to the VM's disks tab instead of the VM list. - migrate_instance returned silently when the target was the current compute or unreachable, so the web view logged "Instance is migrated" and the API answered "instance migrate is started". It now raises; the view reports the error and logs only a completed migration, and an unknown compute id is a 404 instead of a 500. The automatic migration after a clone skips a target that is the current compute. - add_owner and del_owner failed with a 500 without their ids, add_owner also when the user was already an owner, and del_owner deleted the given ownership record even when it belonged to another VM. Ids are validated, a record of another VM is a 404, and a repeated owner is reported. - Broad handlers that hid programming errors are narrowed: clone catches libvirt, value and OS errors (a lock timeout included), ownership lookups catch DoesNotExist. The DRBD status no longer needs its bare except: the VM name is escaped in the regex, which was the only thing that could fail there. API: - A libvirt error under /api/ is JSON with status 400, and a lock timeout 409, instead of the HTML error page. This lives in the existing ExceptionMiddleware, so existing settings need no change. - Volume create/delete on an inactive pool answer 409, and invalid volume data 400 with the serializer errors, instead of 200. The new tests cover every view that returns with _back() without a Referer and with a foreign one, the migrate failures on web and API, the owner ids, the API error format and the volume statuses; nine of the ten fail on the previous code. --- instances/test_error_handling.py | 146 ++++++++++++++++++++++ instances/utils.py | 8 +- instances/views.py | 201 ++++++++++++++++--------------- storages/api/viewsets.py | 12 +- webvirtcloud/middleware.py | 10 ++ 5 files changed, 267 insertions(+), 110 deletions(-) create mode 100644 instances/test_error_handling.py diff --git a/instances/test_error_handling.py b/instances/test_error_handling.py new file mode 100644 index 00000000..290abaf5 --- /dev/null +++ b/instances/test_error_handling.py @@ -0,0 +1,146 @@ +"""Failures reach the user: safe redirects, failure statuses, no silent success.""" + +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 +from django.test import TestCase +from django.urls import URLPattern, get_resolver, reverse +from logs.models import Logs +from vrtManager.util import OperationError + +from instances.models import Instance + +EXTERNAL = "https://evil.example/instances/1/" + + +def back_views(): + """URL names of the instance views that return with _back().""" + names = [] + for pattern in get_resolver().url_patterns: + if getattr(pattern, "namespace", None) != "instances": + continue + for sub in pattern.url_patterns: + if isinstance(sub, URLPattern) and "_back(request, pk" in inspect.getsource(inspect.unwrap(sub.callback)): + names.append(sub.name) + return names + + +class ErrorHandlingTestCase(TestCase): + def setUp(self): + admin = get_user_model().objects.create_superuser("err_admin", "e@example.com", "pw") + self.client.force_login(admin) + self.compute = Compute.objects.create(name="err-compute", hostname="127.0.0.1", login="", password="", type=4) + self.instance = Instance.objects.create( + compute=self.compute, name="err-vm", uuid="11111111-2222-3333-4444-555555555555" + ) + self.page = f"/instances/{self.instance.id}/" + + def url(self, name): + args = [self.instance.id, "sda"] if name == "detach_cdrom" else [self.instance.id] + return reverse(f"instances:{name}", args=args) + + # 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"}, + } + + def test_redirects_without_or_with_a_foreign_referer_go_to_the_vm_page(self): + names = back_views() + self.assertGreaterEqual(len(names), 30) + for name in names: + for referer in (None, EXTERNAL): + with self.subTest(view=name, referer=referer), patch("instances.models.wvmInstance"): + headers = {"HTTP_REFERER": referer} if referer else {} + data = self.DATA[name](self) if name in self.DATA else {} + response = self.client.post(self.url(name), data, **headers) + 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")) + 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"): + response = self.client.post(self.url("set_autostart"), {}, HTTP_REFERER="http://testserver/instances/?x=1") + self.assertEqual(response.url, "http://testserver/instances/?x=1#boot_opt") + + def migrate(self, target_id, up=True): + with patch("instances.models.wvmInstance"), patch( + "instances.utils.connection_manager.host_is_up", return_value=up + ): + return self.client.post(self.url("migrate"), {"compute_id": target_id}) + + def test_migrate_to_a_down_host_is_reported_and_not_logged_as_done(self): + target = Compute.objects.create(name="down", hostname="10.0.0.9", login="", password="", type=4) + response = self.migrate(str(target.id), up=False) + self.assertEqual(response.status_code, 302) + self.assertIn("not reachable", " ".join(str(m) for m in get_messages(response.wsgi_request))) + self.assertFalse(Logs.objects.filter(message__contains="migrated").exists()) + + def test_migrate_to_the_same_host_is_reported(self): + response = self.migrate(str(self.compute.id)) + self.assertIn("already on", " ".join(str(m) for m in get_messages(response.wsgi_request))) + + def test_migrate_to_an_invalid_compute_is_404(self): + self.assertEqual(self.migrate("").status_code, 404) + self.assertEqual(self.migrate("999").status_code, 404) + + +class ApiErrorTestCase(TestCase): + def setUp(self): + admin = get_user_model().objects.create_superuser("api_admin", "a@example.com", "pw") + self.client.force_login(admin) + self.compute = Compute.objects.create(name="api-compute", hostname="127.0.0.1", login="", password="", type=4) + self.instance = Instance.objects.create( + compute=self.compute, name="api-vm", uuid="11111111-2222-3333-4444-666666666666" + ) + + def test_libvirt_error_is_json_with_a_failure_status(self): + url = reverse("compute-instance-poweron", kwargs={"compute_pk": self.compute.id, "pk": self.instance.id}) + with patch("instances.models.wvmInstance") as wvm: + wvm.return_value.start.side_effect = OperationError("cannot start") + response = self.client.post(url, {}) + self.assertEqual(response.status_code, 400) + self.assertEqual(response.json(), {"detail": "cannot start"}) + + def test_migrate_to_a_down_host_is_not_started(self): + target = Compute.objects.create(name="api-down", hostname="10.0.0.9", login="", password="", type=4) + with patch("instances.models.wvmInstance"), patch( + "instances.utils.connection_manager.host_is_up", return_value=False + ): + response = self.client.post( + reverse("instance-migrate-list"), {"instance": self.instance.id, "target_compute": target.id} + ) + self.assertEqual(response.status_code, 400) + self.assertIn("not reachable", response.json()["detail"]) + + def volume(self, method, active=True, data=None): + kwargs = {"compute_pk": self.compute.id, "storage_pk": "default"} + with patch("storages.api.viewsets.wvmStorage") as storage: + storage.return_value.is_active.return_value = active + if method == "post": + return self.client.post(reverse("compute-storage-volumes-list", kwargs=kwargs), data or {}) + return self.client.delete(reverse("compute-storage-volumes-detail", kwargs={**kwargs, "pk": "disk.qcow2"})) + + def test_volume_calls_on_an_inactive_pool_fail(self): + data = {"name": "v", "size": 1, "type": "qcow2", "meta_prealloc": False} + self.assertEqual(self.volume("post", active=False, data=data).status_code, 409) + self.assertEqual(self.volume("delete", active=False).status_code, 409) + + def test_invalid_volume_data_is_400_with_the_errors(self): + response = self.volume("post", data={}) + self.assertEqual(response.status_code, 400) + self.assertIn("name", response.json()) diff --git a/instances/utils.py b/instances/utils.py index 6224a307..c09cb31b 100644 --- a/instances/utils.py +++ b/instances/utils.py @@ -9,6 +9,7 @@ from computes.utils import libvirt_compute_lock from django.conf import settings from django.db import transaction from django.utils.translation import gettext_lazy as _ +from vrtManager import util from vrtManager.connection import connection_manager from vrtManager.instance import wvmInstance, wvmInstances @@ -137,11 +138,10 @@ def migrate_instance( compress=False, postcopy=False, ): - status = connection_manager.host_is_up(new_compute.type, new_compute.hostname) - if not status: - return if new_compute == instance.compute: - return + raise util.OperationError(_("The instance is already on %(compute)s") % {"compute": new_compute.name}) + if not connection_manager.host_is_up(new_compute.type, new_compute.hostname): + raise util.OperationError(_("%(compute)s is not reachable") % {"compute": new_compute.name}) c1, c2 = ( (instance.compute, new_compute) if instance.compute.id < new_compute.id diff --git a/instances/views.py b/instances/views.py index 29662df6..53ce3e21 100755 --- a/instances/views.py +++ b/instances/views.py @@ -177,37 +177,33 @@ def drbd_status(request, pk): ) if remoteDrbdStatus.stdout: - try: - instanceFindDrbd = re.compile( - instance.name + "[_]*[A-Z]* role:(.+?)\n disk:(.+?)\n", - re.IGNORECASE, - ) - instanceDrbd = instanceFindDrbd.findall(remoteDrbdStatus.stdout) + instanceFindDrbd = re.compile( + re.escape(instance.name) + "[_]*[A-Z]* role:(.+?)\n disk:(.+?)\n", + re.IGNORECASE, + ) + instanceDrbd = instanceFindDrbd.findall(remoteDrbdStatus.stdout) - primaryCount = 0 - secondaryCount = 0 - statusDisk = "OK" + primaryCount = 0 + secondaryCount = 0 + statusDisk = "OK" - for disk in instanceDrbd: - if disk[0] == "Primary": - primaryCount = primaryCount + 1 - elif disk[0] == "Secondary": - secondaryCount = secondaryCount + 1 - if disk[1] != "UpToDate": - statusDisk = "NOK" + for disk in instanceDrbd: + if disk[0] == "Primary": + primaryCount = primaryCount + 1 + elif disk[0] == "Secondary": + secondaryCount = secondaryCount + 1 + if disk[1] != "UpToDate": + statusDisk = "NOK" - if primaryCount > 0 and secondaryCount > 0: - statusRole = "NOK" + if primaryCount > 0 and secondaryCount > 0: + statusRole = "NOK" + else: + if primaryCount > secondaryCount: + statusRole = "Primary" else: - if primaryCount > secondaryCount: - statusRole = "Primary" - else: - statusRole = "Secondary" + statusRole = "Secondary" - result = statusRole + "/" + statusDisk - - except: - print("Error to get drbd role and status") + result = statusRole + "/" + statusDisk return result @@ -326,16 +322,13 @@ def _same_origin_referer(request, default): def get_safe_redirect(request, default=None): - referer = request.META.get("HTTP_REFERER") - if referer and url_has_allowed_host_and_scheme( - url=referer, - allowed_hosts={request.get_host()}, - require_https=request.is_secure(), - ): - return redirect(referer) - if default: - return redirect(default) - return redirect("instances:index") + return redirect(_same_origin_referer(request, default or reverse("instances:index"))) + + +def _back(request, pk, tab): + """Back to the page the form was on, at the given tab; the VM page if + the Referer is missing or from another site.""" + return redirect(_same_origin_referer(request, reverse("instances:instance", args=[pk])) + "#" + tab) def get_instance(user, pk, perm_type="view"): @@ -474,12 +467,12 @@ def invalid_disk_options( ] -def reject_disk_options(request, invalid): +def reject_disk_options(request, pk, invalid): messages.error( request, _("Invalid disk options: %(options)s") % {"options": ", ".join(invalid)}, ) - return get_safe_redirect(request) + return _back(request, pk, "disks") @require_POST @@ -605,7 +598,7 @@ def destroy(request, pk): instance = get_instance(request.user, pk, perm_type="delete") try: userinstance = instance.userinstance_set.get(user=request.user) - except Exception: + except UserInstance.DoesNotExist: userinstance = UserInstance( is_delete=request.user.is_superuser ) @@ -635,7 +628,9 @@ def migrate(request, pk): postcopy = request.POST.get("postcopy", False) current_host = instance.compute.hostname - target_host = Compute.objects.get(id=compute_id) + if not compute_id.isdigit(): + raise Http404 + target_host = get_object_or_404(Compute, pk=compute_id) try: utils.migrate_instance( @@ -650,15 +645,15 @@ def migrate(request, pk): compress, postcopy, ) - except libvirtError as err: + except (libvirtError, OSError) as err: # OSError includes a lock timeout messages.error(request, err) - - migration_method = "live" if live is True else "offline" - msg = _("Instance is migrated(%(method)s) to %(hostname)s") % { - "hostname": target_host.hostname, - "method": migration_method, - } - addlogmsg(request.user.username, current_host, instance.name, msg) + else: + migration_method = "live" if live else "offline" + msg = _("Instance is migrated(%(method)s) to %(hostname)s") % { + "hostname": target_host.hostname, + "method": migration_method, + } + addlogmsg(request.user.username, current_host, instance.name, msg) return get_safe_redirect( request, default=reverse("instances:instance", args=[instance.id]) @@ -894,7 +889,7 @@ def add_new_vol(request, pk): if not VOLUME_NAME_RE.fullmatch(name): invalid.append("name") if invalid: - return reject_disk_options(request, invalid) + return reject_disk_options(request, pk, invalid) conn_create = wvmCreate( instance.compute.hostname, @@ -945,7 +940,7 @@ def add_new_vol(request, pk): "format": format, } addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#disks") + return _back(request, pk, "disks") @require_POST @@ -960,7 +955,7 @@ def add_existing_vol(request, pk): invalid = invalid_disk_options(instance, bus=bus, cache=cache) if invalid: - return reject_disk_options(request, invalid) + return reject_disk_options(request, pk, invalid) media = instance.proxy.get_media_devices() disks = instance.proxy.get_disk_devices() @@ -976,7 +971,7 @@ def add_existing_vol(request, pk): # Only volumes that really exist in the selected pool; this also rules # out path traversal through the volume name. if name not in conn_create.get_volumes(): - return reject_disk_options(request, ["vols"]) + return reject_disk_options(request, pk, ["vols"]) format_type = conn_create.get_volume_format_type(name) disk_type = conn_create.get_volume_type(name) @@ -1003,7 +998,7 @@ def add_existing_vol(request, pk): ) msg = _("Attach Existing disk: %(target_dev)s") % {"target_dev": target_dev} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#disks") + return _back(request, pk, "disks") @require_POST @@ -1050,7 +1045,7 @@ def edit_volume(request, pk): if not current: invalid.append("dev") if invalid: - return reject_disk_options(request, invalid) + return reject_disk_options(request, pk, invalid) instance.proxy.edit_disk( target_dev, @@ -1079,7 +1074,7 @@ def edit_volume(request, pk): msg = _("Edit disk: %(target_dev)s") % {"target_dev": target_dev} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#disks") + return _back(request, pk, "disks") @require_POST @@ -1097,7 +1092,7 @@ def delete_vol(request, pk): request, _("Disk %(dev)s is not a storage volume of this instance") % {"dev": dev}, ) - return redirect(request.META.get("HTTP_REFERER") + "#disks") + return _back(request, pk, "disks") conn_delete = wvmStorage( instance.compute.hostname, @@ -1112,7 +1107,7 @@ def delete_vol(request, pk): request, _("Volume %(vol)s is used by another VM; detach it instead") % {"vol": disk["image"]}, ) - return redirect(request.META.get("HTTP_REFERER") + "#disks") + return _back(request, pk, "disks") msg = _("Delete disk: %(dev)s") % {"dev": dev} instance.proxy.detach_disk(dev) @@ -1124,11 +1119,11 @@ def delete_vol(request, pk): "Delete it from the storage pool once the VM is shut down.") % {"dev": dev, "vol": disk["image"]}, ) - return redirect(request.META.get("HTTP_REFERER") + "#disks") + return _back(request, pk, "disks") conn_delete.del_volume(disk["image"]) addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#disks") + return _back(request, pk, "disks") @require_POST @@ -1141,7 +1136,7 @@ def detach_vol(request, pk): msg = _("Detach disk: %(dev)s") % {"dev": dev} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#disks") + return _back(request, pk, "disks") @require_POST @@ -1152,7 +1147,7 @@ def add_cdrom(request, pk): bus = request.POST.get("bus", "ide" if instance.machine == "pc" else "sata") invalid = invalid_disk_options(instance, bus=bus) if invalid: - return reject_disk_options(request, invalid) + return reject_disk_options(request, pk, invalid) target = utils.get_new_disk_dev(instance.media, instance.disks, bus) instance.proxy.attach_disk( @@ -1166,7 +1161,7 @@ def add_cdrom(request, pk): msg = _("Add CD-ROM: %(target)s") % {"target": target} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#disks") + return _back(request, pk, "disks") @require_POST @@ -1179,7 +1174,7 @@ def detach_cdrom(request, pk, dev): msg = _("Detach CD-ROM: %(dev)s") % {"dev": dev} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#disks") + return _back(request, pk, "disks") @require_POST @@ -1236,7 +1231,7 @@ def snapshot(request, pk): else: msg = _("Create snapshot: %(snap)s") % {"snap": name} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#managesnapshot") + return _back(request, pk, "managesnapshot") @require_POST @@ -1253,7 +1248,7 @@ def delete_snapshot(request, pk): instance.proxy.snapshot_delete(snap_name) msg = _("Delete snapshot: %(snap)s") % {"snap": snap_name} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#managesnapshot") + return _back(request, pk, "managesnapshot") @require_POST @@ -1271,13 +1266,13 @@ def revert_snapshot(request, pk): instance.proxy.snapshot_revert(snap_name) except libvirtError as err: messages.error(request, _("Snapshot was not reverted: %(err)s") % {"err": err}) - return redirect(request.META.get("HTTP_REFERER") + "#managesnapshot") + return _back(request, pk, "managesnapshot") msg = _("Successful revert snapshot: ") msg += snap_name messages.success(request, msg) msg = _("Revert snapshot: %(snap)s") % {"snap": snap_name} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#managesnapshot") + return _back(request, pk, "managesnapshot") @require_POST @@ -1296,7 +1291,7 @@ def create_external_snapshot(request, pk): instance.proxy.create_external_snapshot("s1." + name, instance, desc=desc) msg = _("Create external snapshot: %(snap)s") % {"snap": name} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#managesnapshot") + return _back(request, pk, "managesnapshot") def get_external_snapshots(request, pk): @@ -1332,7 +1327,7 @@ def revert_external_snapshot(request, pk): instance.proxy.start() if instance_state else None msg = _("Revert external snapshot: %(snap)s") % {"snap": name} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#managesnapshot") + return _back(request, pk, "managesnapshot") @require_POST @@ -1358,7 +1353,7 @@ def delete_external_snapshot(request, pk): finally: instance.proxy.force_shutdown() if instance_state else None - return redirect(request.META.get("HTTP_REFERER") + "#managesnapshot") + return _back(request, pk, "managesnapshot") @require_POST @@ -1374,7 +1369,7 @@ def set_vcpu(request, pk): instance.proxy.set_vcpu(id, 0) msg = _("VCPU %(id)s is enabled=%(enabled)s") % {"id": id, "enabled": enabled} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#resize") + return _back(request, pk, "resize") @require_POST @@ -1386,7 +1381,7 @@ def set_vcpu_hotplug(request, pk): msg = _("VCPU Hot-plug is enabled=%(status)s") % {"status": status} instance.proxy.set_vcpu_hotplug(status) addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#resize") + return _back(request, pk, "resize") @require_POST @@ -1397,7 +1392,7 @@ def set_autostart(request, pk): instance.proxy.set_autostart(1) msg = _("Set autostart") addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#boot_opt") + return _back(request, pk, "boot_opt") @require_POST @@ -1408,7 +1403,7 @@ def unset_autostart(request, pk): instance.proxy.set_autostart(0) msg = _("Unset autostart") addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#boot_opt") + return _back(request, pk, "boot_opt") @require_POST @@ -1419,7 +1414,7 @@ def set_bootmenu(request, pk): instance.proxy.set_bootmenu(1) msg = _("Enable boot menu") addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#boot_opt") + return _back(request, pk, "boot_opt") @require_POST @@ -1430,7 +1425,7 @@ def unset_bootmenu(request, pk): instance.proxy.set_bootmenu(0) msg = _("Disable boot menu") addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#boot_opt") + return _back(request, pk, "boot_opt") @require_POST @@ -1458,7 +1453,7 @@ def set_bootorder(request, pk): else: messages.success(request, _("Boot order changed successfully.")) addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#boot_opt") + return _back(request, pk, "boot_opt") @require_POST @@ -1471,7 +1466,7 @@ def change_xml(request, pk): instance.proxy._defineXML(new_xml) msg = _("Change instance XML") addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#xmledit") + return _back(request, pk, "xmledit") @require_POST @@ -1487,7 +1482,7 @@ def set_guest_agent(request, pk): msg = _("Set Guest Agent: %(status)s") % {"status": status} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#options") + return _back(request, pk, "options") @require_POST @@ -1499,7 +1494,7 @@ def set_video_model(request, pk): instance.proxy.set_video_model(video_model) msg = _("Set Video Model: %(model)s") % {"model": video_model} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#options") + return _back(request, pk, "options") @require_POST @@ -1579,7 +1574,7 @@ def add_network(request, pk): instance.proxy.add_network(mac, source, source_type, model=model, nwfilter=nwfilter) msg = _("Add network: %(mac)s") % {"mac": mac} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#network") + return _back(request, pk, "network") @require_POST @@ -1592,7 +1587,7 @@ def delete_network(request, pk): instance.proxy.delete_network(mac_address) msg = _("Delete Network: %(mac)s") % {"mac": mac_address} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#network") + return _back(request, pk, "network") @require_POST @@ -1607,7 +1602,7 @@ def set_link_state(request, pk): instance.proxy.set_link_state(mac_address, state) msg = _("Set Link State: %(state)s") % {"state": state} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#network") + return _back(request, pk, "network") @require_POST @@ -1640,7 +1635,7 @@ def set_qos(request, pk): % {"qos_dir": qos_dir.capitalize()}, ) - return redirect(request.META.get("HTTP_REFERER") + "#network") + return _back(request, pk, "network") @require_POST @@ -1665,14 +1660,17 @@ def unset_qos(request, pk): ) % {"qos_dir": qos_dir.capitalize()}, ) - return redirect(request.META.get("HTTP_REFERER") + "#network") + return _back(request, pk, "network") @require_POST @superuser_only def add_owner(request, pk): instance = get_instance(request.user, pk) - user_id = request.POST.get("user_id") + user_id = request.POST.get("user_id", "") + if not user_id.isdigit(): + raise Http404 + user = get_object_or_404(User, pk=user_id) check_inst = 0 @@ -1684,24 +1682,26 @@ def add_owner(request, pk): request, _("Only one owner is allowed and the one already added") ) else: - add_user_inst = UserInstance(instance=instance, user_id=user_id) - add_user_inst.save() - user = User.objects.get(id=user_id) - msg = _("Add owner: %(user)s") % {"user": user} - addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#users") + if UserInstance.objects.get_or_create(instance=instance, user=user)[1]: + msg = _("Add owner: %(user)s") % {"user": user} + addlogmsg(request.user.username, instance.compute.name, instance.name, msg) + else: + messages.warning(request, _("%(user)s is already an owner") % {"user": user}) + return _back(request, pk, "users") @require_POST @superuser_only def del_owner(request, pk): instance = get_instance(request.user, pk) - userinstance_id = int(request.POST.get("userinstance", "")) - userinstance = UserInstance.objects.get(pk=userinstance_id) + userinstance_id = request.POST.get("userinstance", "") + if not userinstance_id.isdigit(): + raise Http404 + userinstance = get_object_or_404(UserInstance, pk=userinstance_id, instance=instance) userinstance.delete() msg = _("Delete owner: %(userinstance_id)s ") % {"userinstance_id": userinstance_id} addlogmsg(request.user.username, instance.compute.name, instance.name, msg) - return redirect(request.META.get("HTTP_REFERER") + "#users") + return _back(request, pk, "users") MAC_ADDRESS_RE = re.compile(r"^([0-9A-F]{2})(:?[0-9A-F]{2}){5}$", re.IGNORECASE) @@ -1831,12 +1831,13 @@ def clone(request, pk): if app_settings.CLONE_INSTANCE_AUTO_MIGRATE == "True": new_compute = Compute.objects.order_by("?").first() - utils.migrate_instance( - new_compute, new_instance, request.user, xml_del=True, offline=True - ) + if new_compute != instance.compute: + utils.migrate_instance( + new_compute, new_instance, request.user, xml_del=True, offline=True + ) return redirect(reverse("instances:instance", args=[new_instance.id])) - except Exception as e: + except (libvirtError, ValueError, OSError) as e: # OSError includes a lock timeout messages.error(request, e) return get_safe_redirect(request, default=reverse("instances:instance", args=[pk])) @@ -1905,7 +1906,7 @@ def change_options(request, pk): instance = get_instance(request.user, pk, perm_type="change") try: userinstance = instance.userinstance_set.get(user=request.user) - except Exception: + except UserInstance.DoesNotExist: userinstance = UserInstance(is_change=False) if request.user.is_superuser or userinstance.is_change: diff --git a/storages/api/viewsets.py b/storages/api/viewsets.py index f55f1d48..48c407ea 100644 --- a/storages/api/viewsets.py +++ b/storages/api/viewsets.py @@ -3,7 +3,7 @@ from appsettings.settings import app_settings from computes.models import Compute from django.shortcuts import get_object_or_404 from drf_spectacular.utils import extend_schema, inline_serializer -from rest_framework import serializers, viewsets +from rest_framework import serializers, status, viewsets from rest_framework.decorators import action from rest_framework.response import Response from vrtManager.storage import wvmStorage, wvmStorages @@ -159,7 +159,7 @@ class VolumeViewSet(viewsets.ViewSet): return Response(serializer.data) - @extend_schema(responses=StatusSerializer) + @extend_schema(responses={200: StatusSerializer, 400: dict, 409: StatusSerializer}) def create(self, request, storage_pk=None, compute_pk=None): compute = get_object_or_404(Compute, pk=compute_pk) @@ -183,11 +183,11 @@ class VolumeViewSet(viewsets.ViewSet): conn.close() return Response({"status": "Volume: " + ret + " is created"}) else: - return Response({"status": "Pool is not active"}) + return Response({"status": "Pool is not active"}, status=status.HTTP_409_CONFLICT) else: - return Response({"status": "Data is not right for create volume"}) + return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) - @extend_schema(responses=StatusSerializer) + @extend_schema(responses={200: StatusSerializer, 409: StatusSerializer}) def destroy(self, request, storage_pk=None, compute_pk=None, pk=None): compute = get_object_or_404(Compute, pk=compute_pk) @@ -200,4 +200,4 @@ class VolumeViewSet(viewsets.ViewSet): conn.close() return Response({"status": "Volume: " + pk + " is deleted"}) else: - return Response({"status": "Pool is not active"}) + return Response({"status": "Pool is not active"}, status=status.HTTP_409_CONFLICT) diff --git a/webvirtcloud/middleware.py b/webvirtcloud/middleware.py index f0900762..8064725b 100644 --- a/webvirtcloud/middleware.py +++ b/webvirtcloud/middleware.py @@ -1,8 +1,11 @@ from django.contrib import messages +from django.http import JsonResponse from django.shortcuts import render from django.utils.translation import gettext_lazy as _ from libvirt import libvirtError +API_PREFIX = "/api/" + class ExceptionMiddleware: def __init__(self, get_response): @@ -12,6 +15,13 @@ class ExceptionMiddleware: return self.get_response(request) def process_exception(self, request, exception): + # API clients get JSON with a failure status, not the HTML error page. + if request.path_info.startswith(API_PREFIX): + if isinstance(exception, libvirtError): + return JsonResponse({"detail": str(exception)}, status=400) + if isinstance(exception, TimeoutError): + return JsonResponse({"detail": "Operation is busy. Please retry."}, status=409) + return None if isinstance(exception, libvirtError): messages.error( request,