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

@@ -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).
 

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.

instances/templates/instances/settings_tab.html

@@ -198,12 +198,12 @@
                                     {% endif %}
                                     </select>
                                     
-                                    {% if instance.media_iso and allow_admin_or_not_template %}
+                                    {% if instance.media_iso %}
                                         <button type="submit" class="btn btn-sm btn-success" name="mount_iso" value="{{ cd.dev }}">{% trans "Mount" %}</button>
                                     {% else %}
                                         <button class="btn btn-sm btn-success disabled">{% trans "Mount" %}</button>
                                     {% endif %}
-                                    {% if instance.status == 5 and allow_admin_or_not_template %}
+                                    {% if instance.status == 5 %}
                                         <button type="submit" class="btn btn-sm btn-danger float-end" formaction="{% url 'instances:detach_cdrom' instance.id cd.dev %}" formmethod="post" title="{% trans "Detach CD-ROM (remove device)" %}">
                                             {% bs_icon 'dash-circle' %}
                                         </button>
@@ -229,11 +229,7 @@
                                     </a>
                                     <input type="text" class="form-control" value="{{ cd.image }}" disabled/>
                                     <input type="hidden" name="path" value="{{ cd.path }}">
-                                    {% if allow_admin_or_not_template %}
-                                        <button type="submit" class="btn btn-sm btn-success" value="{{ cd.dev }}" name="umount_iso">{% trans "Unmount" %}</button>
-                                    {% else %}
-                                        <button class="btn btn-sm btn-success disabled" value="{{ cd.dev }}" name="umount_iso">{% trans "Unmount" %}</button>
-                                    {% endif %}
+                                    <button type="submit" class="btn btn-sm btn-success" value="{{ cd.dev }}" name="umount_iso">{% trans "Unmount" %}</button>
                                 </div>
                             </form>
                         {% endif %}

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})

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):

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()

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", "")

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):