From b24746c1cb24f474dcbecffb2af8ec5f9e3c1807 Mon Sep 17 00:00:00 2001 From: catborise Date: Fri, 23 Oct 2026 14:09:26 +0300 Subject: [PATCH] fix(logs): blocking a blocked user writes no row; one upload submit path Follow-ups from the review of the audit log layers: - Blocking a user who is already blocked, or unblocking one who is not, changes nothing and now writes no row, like any other edit that changes nothing. - write_chunk takes the request instead of the user and an optional request; the view always had one, so the branch only tests used is gone and every chunk 0 goes through the same audited submit. - Tests: a busy clone creates no job and its row carries the page's message; a later chunk that fails writes no submit row; libvirt refusing a network or pool delete is a failed row. --- admin/tests/test_audit_rows.py | 2 ++ admin/views.py | 6 +++-- networks/tests/test_audit_rows.py | 7 ++++++ storages/tests/test_audit_rows.py | 38 ++++++++++++++++++------------- storages/tests/test_upload_job.py | 14 +++++++++--- storages/upload.py | 10 ++++---- storages/views.py | 4 ++-- 7 files changed, 52 insertions(+), 29 deletions(-) diff --git a/admin/tests/test_audit_rows.py b/admin/tests/test_audit_rows.py index 48ae9d2b..4984b7c8 100644 --- a/admin/tests/test_audit_rows.py +++ b/admin/tests/test_audit_rows.py @@ -65,6 +65,8 @@ class UserRowTests(TestCase): carol = User.objects.get(username="carol") Logs.objects.all().delete() self.client.post(reverse("admin:user_block", args=[carol.pk])) + self.client.post(reverse("admin:user_block", args=[carol.pk])) # already blocked: nothing changes + self.client.post(reverse("admin:user_unblock", args=[carol.pk])) self.client.post(reverse("admin:user_unblock", args=[carol.pk])) self.client.post(reverse("admin:user_update_password", args=[carol.pk]), {"password1": "New-carol-pass-1", "password2": "New-carol-pass-1"}) diff --git a/admin/views.py b/admin/views.py index d37ccda0..6d4196e8 100644 --- a/admin/views.py +++ b/admin/views.py @@ -215,7 +215,8 @@ def user_delete(request, pk): @superuser_only def user_block(request, pk): user: User = get_object_or_404(User, pk=pk) - with audit(request, None, f"User {user.username}", "block"): + with audit(request, None, f"User {user.username}", "block") as entry: + entry.skip = not user.is_active # already blocked user.is_active = False user.save() return redirect("admin:user_list") @@ -225,7 +226,8 @@ def user_block(request, pk): @superuser_only def user_unblock(request, pk): user: User = get_object_or_404(User, pk=pk) - with audit(request, None, f"User {user.username}", "unblock"): + with audit(request, None, f"User {user.username}", "unblock") as entry: + entry.skip = user.is_active # not blocked user.is_active = True user.save() return redirect("admin:user_list") diff --git a/networks/tests/test_audit_rows.py b/networks/tests/test_audit_rows.py index cc2cbe77..0c3dca6f 100644 --- a/networks/tests/test_audit_rows.py +++ b/networks/tests/test_audit_rows.py @@ -81,6 +81,13 @@ class NetworkRowTests(TestCase): self.assertIn(error, [str(m) for m in get_messages(response.wsgi_request)]) self.assertEqual(rows(), [(f"Network lab: stop: {error}", False)]) + def test_a_refused_network_delete(self): + error = "Requested operation is not valid: network is in use" + with patch("vrtManager.network.wvmNetwork.delete", side_effect=libvirt.libvirtError(error)): + response = self.post({"delete": ""}) + self.assertIn(error, [str(m) for m in get_messages(response.wsgi_request)]) + self.assertEqual(rows(), [(f"Network lab: delete: {error}", False)]) + def test_network_create(self): url = reverse("networks", args=[self.compute.id]) self.client.post(url, {"create": "", "name": "audit-net", "forward": "nat", "subnet": "198.51.100.0/24", diff --git a/storages/tests/test_audit_rows.py b/storages/tests/test_audit_rows.py index f834b30e..99a2b4ef 100644 --- a/storages/tests/test_audit_rows.py +++ b/storages/tests/test_audit_rows.py @@ -4,9 +4,8 @@ from types import SimpleNamespace from unittest.mock import patch from django.contrib.auth import get_user_model -from django.contrib.sessions.middleware import SessionMiddleware from django.core.files.uploadedfile import SimpleUploadedFile -from django.test import RequestFactory, TransactionTestCase +from django.test import TransactionTestCase from django.urls import reverse from computes.models import Compute @@ -79,24 +78,17 @@ class CloneRowTests(TransactionTestCase): def test_a_busy_pool_refuses_the_clone(self): self.stg.get_volumes.return_value = ["base.qcow2"] claims.admit({claims.pool_key(self.compute.pk, "default"): Claim.EXCLUSIVE}, holder="192.0.2.30:1:1") - self.client.post(self.url, {"cln_volume": "1", "image": "base.qcow2", "name": "copy", - "submission_key": "key-vol00005"}) - [(message, success)] = rows() - self.assertFalse(success) - self.assertTrue(message.startswith("Storage pool default: clone volume base.qcow2 as copy: "), message) + response = self.client.post(self.url, {"cln_volume": "1", "image": "base.qcow2", "name": "copy", + "submission_key": "key-vol00005"}, follow=True) + [shown] = [str(m) for m in response.context["messages"]] + self.assertFalse(Operation.objects.exists()) + self.assertEqual(rows(), [(f"Storage pool default: clone volume base.qcow2 as copy: {shown}", False)]) class UploadRowTests(upload_tests.LocalPool, upload_tests.UploadTestCase): - def request(self): - request = RequestFactory().post("/") - request.user = self.admin - SessionMiddleware(lambda r: None).process_request(request) - return request - def chunk(self, index, data, name=" disk.iso", total=2, size=6): - return write_chunk(self.admin, self.conn, self.compute, "default", upload_tests.KEY, - {"name": name, "size": size, "chunks": total}, index, SimpleUploadedFile("chunk", data), - request=self.request()) + return write_chunk(self.request(), self.conn, self.compute, "default", upload_tests.KEY, + {"name": name, "size": size, "chunks": total}, index, SimpleUploadedFile("chunk", data)) def test_an_upload_writes_one_row(self): self.chunk(0, b"abc") @@ -107,6 +99,12 @@ class UploadRowTests(upload_tests.LocalPool, upload_tests.UploadTestCase): self.assertEqual(rows(), [("Storage pool default: upload volume disk.iso", True)]) self.assertEqual(Logs.objects.get().host, "up-c") + def test_a_failing_later_chunk_writes_no_submit_row(self): + self.chunk(0, b"abc") + with self.assertRaises(ValueError): + self.chunk(1, b"def", name="other.iso") # the session's parameters changed + self.assertEqual(rows(), []) + def test_a_refused_upload_is_a_failed_row(self): open(self.path(), "wb").close() with self.assertRaises(FileExistsError): @@ -157,6 +155,14 @@ class PoolAndVolumeRowTests(TransactionTestCase): self.assertIn(reason, [str(m) for m in response.context["messages"]]) self.assertEqual(rows(), [(f"Storage pool default: stop: {reason}", False)]) + def test_a_refused_pool_delete(self): + from vrtManager.util import OperationError + + reason = "Pool default holds disks of the VMs web-1" + self.pool.delete.side_effect = OperationError(reason) + self.client.post(self.url, {"delete": "1"}) + self.assertEqual(rows(), [(f"Storage pool default: delete: {reason}", False)]) + def test_pool_create_and_a_duplicate_name(self): url = reverse("storages", args=[self.compute.pk]) self.client.post(url, {"create": "1", "name": "audit-pool", "stg_type": "dir", "target": "/srv/audit-pool"}) diff --git a/storages/tests/test_upload_job.py b/storages/tests/test_upload_job.py index 682de786..fa231cff 100644 --- a/storages/tests/test_upload_job.py +++ b/storages/tests/test_upload_job.py @@ -11,7 +11,8 @@ from unittest.mock import MagicMock, patch from django.contrib.auth import get_user_model from django.core.exceptions import PermissionDenied from django.core.files.uploadedfile import SimpleUploadedFile -from django.test import TestCase +from django.contrib.sessions.middleware import SessionMiddleware +from django.test import RequestFactory, TestCase from django.urls import reverse from django.utils import timezone from libvirt import VIR_ERR_NO_STORAGE_VOL, VIR_ERR_SYSTEM_ERROR, libvirtError @@ -50,8 +51,15 @@ class UploadTestCase(TestCase): return patch.object(target_class, "write", cancelled) + def request(self, user=None): + """A chunk request of user (the admin), with the session remember_job writes to.""" + request = RequestFactory().post("/") + request.user = user or self.admin + SessionMiddleware(lambda r: None).process_request(request) + return request + def send(self, index, data, total=3, size=9, name="disk.iso", key=KEY, user=None): - return write_chunk(user or self.admin, self.conn, self.compute, "default", key, + return write_chunk(self.request(user), self.conn, self.compute, "default", key, {"name": name, "size": size, "chunks": total}, index, SimpleUploadedFile("chunk", data)) def job(self): @@ -170,7 +178,7 @@ class LocalUploadTests(LocalPool, UploadTestCase): chunk.chunks = interrupted with self.assertRaises(OSError): - write_chunk(self.admin, self.conn, self.compute, "default", KEY, + write_chunk(self.request(), self.conn, self.compute, "default", KEY, {"name": "disk.iso", "size": 9, "chunks": 3}, 0, chunk) op = self.job() self.assertEqual((op.intent, op.progress_done), ({"chunk": 0, "created": True}, None)) diff --git a/storages/upload.py b/storages/upload.py index 71695324..dbd8b8d8 100644 --- a/storages/upload.py +++ b/storages/upload.py @@ -498,21 +498,19 @@ def end(op, state, error=""): pass -def write_chunk(user, conn, compute, pool, upload_id, raw, chunk_index, chunk, request=None): +def write_chunk(request, conn, compute, pool, upload_id, raw, chunk_index, chunk): """Write chunk chunk_index of the upload upload_id (the job's submission key) and commit it on the job; the last chunk renames the image.""" kind = jobs.get_kind("volume.upload") raw = {**raw, "compute": compute.pk, "pool": pool} - if chunk_index == 0 and request is not None: + if chunk_index == 0: name = os.path.basename(str(raw["name"])).strip() with audit(request, compute, f"Storage pool {pool}", f"upload volume {name}") as entry: - op = jobs.submit(kind.name, user, raw, upload_id) + op = jobs.submit(kind.name, request.user, raw, upload_id) entry.skip = True # the job writes its row when it ends jobs.remember_job(request, op) - elif chunk_index == 0: - op = jobs.submit(kind.name, user, raw, upload_id) else: - op = Operation.objects.filter(user=user, submission_key=upload_id).first() + op = Operation.objects.filter(user=request.user, submission_key=upload_id).first() if op is None: raise ValueError(_("Upload session not found or expired")) if op.kind != kind.name or op.params != kind.normalize(raw): diff --git a/storages/views.py b/storages/views.py index 57acc98c..62962111 100644 --- a/storages/views.py +++ b/storages/views.py @@ -40,9 +40,9 @@ def _handle_iso_upload(request, conn, compute, pool): return JsonResponse({"error": _("Invalid file size.")}, status=400) try: - op = write_chunk(request.user, conn, compute, pool, request.POST.get("upload_id", ""), + op = write_chunk(request, conn, compute, pool, request.POST.get("upload_id", ""), {"name": request.POST.get("file_name", ""), "size": file_size, "chunks": total_chunks}, - chunk_index, file_chunk, request=request) + chunk_index, file_chunk) except claims.Busy as busy: if busy.blocker is not None: # another job holds the pool or the name return JsonResponse({"error": busy_message(busy, request.user)}, status=400)