From 4dff1008cc8792372178e18e5c4477f66eac309b Mon Sep 17 00:00:00 2001 From: catborise Date: Wed, 7 Oct 2026 20:59:15 +0300 Subject: [PATCH] fix(instances): only superusers and staff owners change a template VM The documented rule held only for snapshots: an owner with is_change could still resize a template, set its root password and SSH keys, change its options and console settings. get_instance(perm_type="change") now refuses a template to owners who are not staff, which covers every change view and the API; the per-view snapshot checks it replaces are removed, and the page offers those controls by the same rule. Viewing, the console, power (a no-op on a template) and cloning are unchanged. --- README.md | 1 + doc/permissions.md | 3 +- .../templates/instances/settings_tab.html | 10 +-- instances/test_page_controls.py | 17 ++++- instances/test_template_flag.py | 9 +-- instances/utils.py | 5 +- instances/views.py | 71 ++++--------------- webvirtcloud/test_authorization_matrix.py | 11 +-- 8 files changed, 53 insertions(+), 74 deletions(-) diff --git a/README.md b/README.md index 64d0226d..103fdd99 100644 --- a/README.md +++ b/README.md @@ -521,6 +521,7 @@ A rollback leaves the clone on a detached HEAD at the restored commit; the updat - Changing one's own password is the "Can change password" permission (per user or group; new users have it); `SHOW_PROFILE_EDIT_PASSWORD` is gone. - The admin's first-login password change survives losing `data/admin_password` (migration `accounts.0007`); run `migrate` as a user who can read that file. - "VM Clone Auto Migrate" is removed; a clone stays on its source host. +- A template VM is changed only by superusers and staff owners with change permission; other owners can still view, open the console and clone it. - The log rotates (10 MB × 5) in `data/webvirtcloud.log`. - Docker no longer publishes port 6080; the console goes through nginx on the panel's port (`WS_PUBLIC_PORT`, default 80). diff --git a/doc/permissions.md b/doc/permissions.md index f98f1b73..6fdf3154 100644 --- a/doc/permissions.md +++ b/doc/permissions.md @@ -27,5 +27,6 @@ WebVirtCloud combines three kinds of access rights: Notes: - Only superusers choose disk names and MAC addresses when cloning; for other users the server derives them. -- `is_staff` gives no access to VMs and does not reveal console settings or VNC passwords by itself. On VMs where a staff user already has `is_change`, staff may also change VMs that are marked as templates; other owners cannot. +- `is_staff` gives no access to VMs and does not reveal console settings or VNC passwords by itself. +- A template VM is changed (resize, root password, SSH keys, options, console settings, snapshots) only by superusers and by staff owners with `is_change`; other owners keep view, console and power (starting a template does nothing). Cloning a template needs only view access and `instances.clone_instances`. - On VM pages, users without ownership or `view_instances` get **404** (the VM's existence is not revealed), and users who can see a VM but lack the right for an action get **403**. Superuser-only pages answer **403** to everyone else; the console page answers **403** both for a missing VM and for a VM the user may not open. diff --git a/instances/templates/instances/settings_tab.html b/instances/templates/instances/settings_tab.html index 0130ce74..bb93323a 100644 --- a/instances/templates/instances/settings_tab.html +++ b/instances/templates/instances/settings_tab.html @@ -198,12 +198,12 @@ {% endif %} - {% if instance.media_iso and allow_admin_or_not_template %} + {% if instance.media_iso %} {% else %} {% endif %} - {% if instance.status == 5 and allow_admin_or_not_template %} + {% if instance.status == 5 %} @@ -229,11 +229,7 @@ - {% if allow_admin_or_not_template %} - - {% else %} - - {% endif %} + {% endif %} diff --git a/instances/test_page_controls.py b/instances/test_page_controls.py index ad4be1fb..bfaf6faf 100644 --- a/instances/test_page_controls.py +++ b/instances/test_page_controls.py @@ -88,6 +88,7 @@ class PageControls: # a running VM with hotplug shows per-vCPU buttons instead of the CPU form "resize": form("resize_memory"), "options": form("change_options"), + "console settings": form("update_console"), "vcpu hotplug": form("set_vcpu"), "video model": form("set_video_model"), "guest agent": form("set_guest_agent"), @@ -102,7 +103,8 @@ class PageControlsTestCase(PageControls, TestCase): def test_superuser_sees_every_control(self): superuser = get_user_model().objects.create_superuser("ctl_super", "ctl_super@example.com", "x") self.assertEqual( - self.controls(superuser), CONSOLE | POWER | CHANGE | SUPERUSER_ONLY | {"clone", "snapshots"} + self.controls(superuser), + CONSOLE | POWER | CHANGE | SUPERUSER_ONLY | {"clone", "snapshots", "console settings"}, ) def test_global_viewers_see_nothing_to_act_on(self): @@ -135,6 +137,19 @@ class TemplatePageControlsTestCase(PageControls, TestCase): viewer = self.user("tpl_viewer", perms=("view_instances", "clone_instances")) self.assertEqual(self.controls(viewer, status=5), {"clone"}) + def test_only_staff_owners_change_a_template(self): + owner = self.user("tpl_plain_changer", owner={"is_change": True}) + staff = self.user("tpl_staff_owner", staff=True, owner={"is_change": True}) + self.assertEqual(self.controls(owner, status=5) & CHANGE, set()) + self.assertEqual(self.controls(staff, status=5) & CHANGE, CHANGE) + + def test_console_settings_of_a_template_are_for_staff_owners(self): + flags = {"is_change": True, "is_vnc": True} + owner = self.user("tpl_vnc_owner", owner=flags) + staff = self.user("tpl_vnc_staff", staff=True, owner=flags) + self.assertNotIn("console settings", self.controls(owner, status=5)) + self.assertIn("console settings", self.controls(staff, status=5)) + def test_snapshots_of_a_template_are_for_staff_owners(self): perms = ("clone_instances", "snapshot_instances") owner = self.user("tpl_changer", perms=perms, owner={"is_change": True}) diff --git a/instances/test_template_flag.py b/instances/test_template_flag.py index 82bc2200..6db74e31 100644 --- a/instances/test_template_flag.py +++ b/instances/test_template_flag.py @@ -36,7 +36,7 @@ class TemplateFlagTestCase(TestCase): for user in (self.owner, self.staff_owner): UserInstance.objects.create(instance=self.instance, user=user, is_change=True) - def _change_options(self, user, data): + def _change_options(self, user, data, status=302): self.client.force_login(user) with patch("instances.models.wvmInstance") as wvm: # unmarking a template checks the owner's quota with the VM's size @@ -46,7 +46,7 @@ class TemplateFlagTestCase(TestCase): res = self.client.post( reverse("instances:change_options", args=[self.instance.id]), data ) - self.assertEqual(res.status_code, 302) + self.assertEqual(res.status_code, status) self.instance.refresh_from_db() def _make_template(self): @@ -57,9 +57,10 @@ class TemplateFlagTestCase(TestCase): self._change_options(self.owner, {"title": "t", "is_template": "True"}) self.assertFalse(self.instance.is_template) - def test_owner_editing_title_does_not_unmark_template(self): + def test_owner_cannot_change_a_template(self): + # a template is changed only by superusers and staff owners self._make_template() - self._change_options(self.owner, {"title": "new title", "description": ""}) + self._change_options(self.owner, {"title": "new title", "description": ""}, status=403) self.assertTrue(self.instance.is_template) def test_staff_owner_can_mark_and_unmark_template(self): diff --git a/instances/utils.py b/instances/utils.py index 6dd476f0..454dd70f 100644 --- a/instances/utils.py +++ b/instances/utils.py @@ -361,12 +361,15 @@ def can_manage_console(user, instance): """ Who may see and change a VM's console settings, including its VNC password: an active superuser, or an active owner with both is_change and - is_vnc. is_staff and the global view_instances permission do not count. + is_vnc; on a template only a staff owner (as get_instance(perm_type="change")). + is_staff and the global view_instances permission alone do not count. """ if not user.is_active: return False if user.is_superuser: return True + if instance.is_template and not user.is_staff: + return False return UserInstance.objects.filter( instance=instance, user=user, is_change=True, is_vnc=True ).exists() diff --git a/instances/views.py b/instances/views.py index 5cb196dd..92281284 100755 --- a/instances/views.py +++ b/instances/views.py @@ -124,9 +124,6 @@ def _instance_page(request, pk, instance, compute): ) console_listener_addresses = settings.QEMU_CONSOLE_LISTENER_ADDRESSES bottom_bar = app_settings.VIEW_INSTANCE_DETAIL_BOTTOM_BAR - allow_admin_or_not_template = ( - request.user.is_superuser or request.user.is_staff or not instance.is_template - ) try: userinstance = UserInstance.objects.get( instance__compute_id=compute.id, @@ -140,13 +137,12 @@ def _instance_page(request, pk, instance, compute): # view_instances permission are read-only. can_open_console = utils.can_open_console(request.user, instance) can_power = request.user.is_superuser or userinstance is not None - can_change = request.user.is_superuser or bool(userinstance and userinstance.is_change) - can_clone = request.user.has_perm("instances.clone_instances") and (instance.is_template or can_change) - can_snapshot = ( - can_change - and request.user.has_perm("instances.snapshot_instances") - and (request.user.is_superuser or request.user.is_staff or not instance.is_template) + # get_instance(perm_type="change"): a template only for superusers and staff owners + can_change = request.user.is_superuser or bool( + userinstance and userinstance.is_change and (request.user.is_staff or not instance.is_template) ) + can_clone = request.user.has_perm("instances.clone_instances") and (instance.is_template or can_change) + can_snapshot = can_change and request.user.has_perm("instances.snapshot_instances") show_settings = ( request.user.is_superuser or can_manage_console or can_clone or can_change or instance.guest_agent_ready ) @@ -184,10 +180,6 @@ def _instance_page(request, pk, instance, compute): # userinstances = UserInstance.objects.filter(instance=instance).order_by('user__username') userinstances = instance.userinstance_set.order_by("user__username") - allow_admin_or_not_template = ( - request.user.is_superuser or request.user.is_staff or not instance.is_template - ) - # Host resources vcpu_host = len(instance.vcpu_range) memory_host = instance.proxy.get_max_memory() @@ -401,7 +393,8 @@ def get_instance(user, pk, perm_type="view"): perm_type: - 'view': superuser, has_perm("instances.view_instances"), or UserInstance owner - 'power': superuser or UserInstance owner - - 'change': superuser or (UserInstance owner and is_change) + - 'change': superuser or (UserInstance owner and is_change); a template + only for superusers and staff owners - 'delete': superuser or (UserInstance owner and is_delete) """ valid_perms = {"view", "power", "change", "delete"} @@ -427,7 +420,8 @@ def get_instance(user, pk, perm_type="view"): if not has_owner_rel: raise PermissionDenied elif perm_type == "change": - if not (user_inst and user_inst.is_change): + # A template is changed only by superusers (above) and staff owners + if not (user_inst and user_inst.is_change) or (instance.is_template and not user.is_staff): raise PermissionDenied elif perm_type == "delete": if not (user_inst and user_inst.is_delete): @@ -1280,11 +1274,7 @@ def mount_iso(request, pk): @serialize_instance_mutation def snapshot(request, pk): instance = get_instance(request.user, pk, perm_type="change") - allow_admin_or_not_template = ( - request.user.is_superuser or request.user.is_staff or not instance.is_template - ) - - if allow_admin_or_not_template and request.user.has_perm( + if request.user.has_perm( "instances.snapshot_instances" ): name = request.POST.get("name", "") @@ -1304,10 +1294,7 @@ def snapshot(request, pk): @serialize_instance_mutation def delete_snapshot(request, pk): instance = get_instance(request.user, pk, perm_type="change") - allow_admin_or_not_template = ( - request.user.is_superuser or request.user.is_staff or not instance.is_template - ) - if allow_admin_or_not_template and request.user.has_perm( + if request.user.has_perm( "instances.snapshot_instances" ): snap_name = request.POST.get("name", "") @@ -1321,10 +1308,7 @@ def delete_snapshot(request, pk): @serialize_instance_mutation def revert_snapshot(request, pk): instance = get_instance(request.user, pk, perm_type="change") - allow_admin_or_not_template = ( - request.user.is_superuser or request.user.is_staff or not instance.is_template - ) - if allow_admin_or_not_template and request.user.has_perm( + if request.user.has_perm( "instances.snapshot_instances" ): snap_name = request.POST.get("name", "") @@ -1345,11 +1329,7 @@ def revert_snapshot(request, pk): @serialize_instance_mutation def create_external_snapshot(request, pk): instance = get_instance(request.user, pk, perm_type="change") - allow_admin_or_not_template = ( - request.user.is_superuser or request.user.is_staff or not instance.is_template - ) - - if allow_admin_or_not_template and request.user.has_perm( + if request.user.has_perm( "instances.snapshot_instances" ): name = request.POST.get("name", "") @@ -1360,28 +1340,11 @@ def create_external_snapshot(request, pk): return _back(request, pk, "managesnapshot") -def get_external_snapshots(request, pk): - instance = get_instance(request.user, pk) - allow_admin_or_not_template = ( - request.user.is_superuser or request.user.is_staff or not instance.is_template - ) - - if allow_admin_or_not_template and request.user.has_perm( - "instances.snapshot_instances" - ): - external_snapshots = instance.proxy.get_external_snapshots() - return external_snapshots - - @require_POST @serialize_instance_mutation def revert_external_snapshot(request, pk): instance = get_instance(request.user, pk, perm_type="change") - allow_admin_or_not_template = ( - request.user.is_superuser or request.user.is_staff or not instance.is_template - ) - - if allow_admin_or_not_template and request.user.has_perm( + if request.user.has_perm( "instances.snapshot_instances" ): instance_state = True if instance.proxy.get_status() != 5 else False @@ -1401,11 +1364,7 @@ def revert_external_snapshot(request, pk): def delete_external_snapshot(request, pk): instance = get_instance(request.user, pk, perm_type="change") instance_state = True if instance.proxy.get_status() == 5 else False - allow_admin_or_not_template = ( - request.user.is_superuser or request.user.is_staff or not instance.is_template - ) - - if allow_admin_or_not_template and request.user.has_perm( + if request.user.has_perm( "instances.snapshot_instances" ): name = request.POST.get("name", "") diff --git a/webvirtcloud/test_authorization_matrix.py b/webvirtcloud/test_authorization_matrix.py index c78ec90d..b516b75b 100644 --- a/webvirtcloud/test_authorization_matrix.py +++ b/webvirtcloud/test_authorization_matrix.py @@ -128,10 +128,13 @@ ROLE_EXPECTATIONS = { } -# A template VM is reached the same way: on templates, snapshot requests and -# poweron are no-ops for these roles, and cloning (which none of them may do -# without instances.clone_instances) has its own rule, see the clone test. -TEMPLATE_ROLE_EXPECTATIONS = ROLE_EXPECTATIONS +# A template VM: only superusers and staff owners with is_change change it; +# other owners with is_change keep view, power (poweron is a no-op on a +# template) and console. Cloning has its own rule, see the clone test. +TEMPLATE_ROLE_EXPECTATIONS = { + **ROLE_EXPECTATIONS, + "owner_is_change": (False, False, {"is_change": True}, VIEW_VM | POWER_AND_CONSOLE), +} def iter_patterns(patterns, prefix="", params=None):