fix(instances): a template is not power-cycled on, and deleted only by superusers and staff owners

Power cycle force-stopped and started a template, which power on refuses.
A non-staff owner with is_delete could delete a template that the
template rule otherwise leaves to superusers and staff owners;
get_instance(perm_type="delete") now refuses it, and the page offers
Destroy by the same rule.

README.md

@@ -521,7 +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.
+- A template VM is changed or deleted only by superusers and staff owners with the matching permission; other owners can still view, open the console and clone it. Power cycle no longer starts a template.
 - The log rotates (10 MB × 5) in `data/webvirtcloud.log`.
 - gstfsd finds the guest's operating system the way libguestfs inspects it, so a root on LVM works, and it always answers (an error when it finds no or several operating systems). Update `/usr/local/bin/gstfsd` on every compute (see [Adding a Compute Node](#adding-a-compute-node), step 3).
 - Docker no longer publishes port 6080; the console goes through nginx on the panel's port (`WS_PUBLIC_PORT`, default 80).

doc/permissions.md

@@ -28,5 +28,5 @@ 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.
-- 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`.
+- A template VM is changed (resize, root password, SSH keys, options, console settings, snapshots) only by superusers and by staff owners with `is_change`, and deleted only by superusers and staff owners with `is_delete`; other owners keep view, console and power (power on and power cycle of a template are refused with a warning). 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/destroy_instance_form.html

@@ -9,7 +9,7 @@
 {% endblock page_heading %}
 
 {% block content %}
-{% if request.user.is_superuser or userinstance.is_delete %}
+{% if can_delete %}
     {% if instance.status == 3 %}
         <div class="alert alert-danger">
             {% trans "Instance is suspended, cannot destroy!" %}

instances/templates/instances/destroy_tab.html

@@ -12,7 +12,7 @@
     <div class="tab-content">
         <div role="tabpanel" class="tab-pane tab-pane-bordered active" id="destroy">
             <p>{% trans 'This action starts remove instance process' %}</p>
-            {% if request.user.is_superuser or userinstance.is_delete %}
+            {% if can_delete %}
                 {% if instance.status == 3 %}
                     <a class="btn btn-lg btn-success disabled float-end">{% trans "Destroy" %}</a>
                 {% else %}

instances/test_page_controls.py

@@ -89,6 +89,8 @@ class PageControls:
             "resize": form("resize_memory"),
             "options": form("change_options"),
             "console settings": form("update_console"),
+            # a link to the destroy confirmation page
+            "destroy": bool(doc.xpath("//a[@href=$a]", a=reverse("instances:destroy", args=[pk]))),
             "vcpu hotplug": form("set_vcpu"),
             "video model": form("set_video_model"),
             "guest agent": form("set_guest_agent"),
@@ -104,7 +106,7 @@ class PageControlsTestCase(PageControls, TestCase):
         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", "console settings"},
+            CONSOLE | POWER | CHANGE | SUPERUSER_ONLY | {"clone", "snapshots", "console settings", "destroy"},
         )
 
     def test_global_viewers_see_nothing_to_act_on(self):
@@ -150,6 +152,12 @@ class TemplatePageControlsTestCase(PageControls, TestCase):
         self.assertNotIn("console settings", self.controls(owner, status=5))
         self.assertIn("console settings", self.controls(staff, status=5))
 
+    def test_only_staff_owners_destroy_a_template(self):
+        owner = self.user("tpl_deleter", owner={"is_delete": True})
+        staff = self.user("tpl_staff_deleter", staff=True, owner={"is_delete": True})
+        self.assertNotIn("destroy", self.controls(owner, status=5))
+        self.assertIn("destroy", 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_rules.py

@@ -0,0 +1,64 @@
+"""
+A template is not started (power on or power cycle, web or API) and is
+changed or deleted only by superusers and staff owners.
+"""
+from unittest.mock import patch
+
+from django.contrib.auth import get_user_model
+from django.test import TestCase
+from django.urls import reverse
+from rest_framework.test import APIClient
+
+from accounts.models import UserInstance
+from computes.models import Compute
+from instances.models import Instance
+
+
+class TemplateRulesTestCase(TestCase):
+    def setUp(self):
+        compute = Compute.objects.create(name="tpl-c", hostname="192.0.2.5", login="root", password="", type=1)
+        self.template = Instance.objects.create(
+            compute=compute, name="tpl-vm", uuid="ffffffff-0000-1111-2222-333333333333", is_template=True
+        )
+        self.owner = get_user_model().objects.create_user("tpl_owner", password="x")
+        UserInstance.objects.create(instance=self.template, user=self.owner, is_delete=True)
+        self.staff = get_user_model().objects.create_user("tpl_staff", password="x", is_staff=True)
+        UserInstance.objects.create(instance=self.template, user=self.staff, is_delete=True)
+
+    def test_power_cycle_does_not_start_a_template(self):
+        api = APIClient()
+        api.force_login(self.owner)
+        self.client.force_login(self.owner)
+        for post in (
+            lambda: self.client.post(reverse("instances:powercycle", args=[self.template.id])),
+            lambda: api.post(reverse("compute-instance-powercycle", args=[self.template.compute_id, self.template.id])),
+        ):
+            with patch("instances.models.wvmInstance") as wvm:
+                post()
+            self.assertFalse(wvm.return_value.start.called)
+            self.assertFalse(wvm.return_value.force_shutdown.called)
+
+    def test_only_staff_owners_delete_a_template(self):
+        self.client.force_login(self.owner)
+        with patch("instances.models.wvmInstance") as wvm:
+            response = self.client.post(reverse("instances:destroy", args=[self.template.id]))
+        self.assertEqual(response.status_code, 403)
+        self.assertFalse(wvm.return_value.delete.called)
+
+        self.client.force_login(self.staff)
+        with patch("instances.models.wvmInstance") as wvm:
+            wvm.return_value.instance.isActive.return_value = False
+            self.client.post(reverse("instances:destroy", args=[self.template.id]))
+        self.assertTrue(wvm.return_value.delete.called)
+
+    def test_the_destroy_confirmation_page_offers_the_form_to_who_may_delete(self):
+        url = reverse("instances:destroy", args=[self.template.id])
+        superuser = get_user_model().objects.create_superuser("tpl_root", "r@example.com", "x")
+        for user, status, form in ((superuser, 200, True), (self.staff, 200, True), (self.owner, 403, False)):
+            with self.subTest(user=user.username):
+                self.client.force_login(user)
+                with patch("instances.models.wvmInstance") as wvm:
+                    wvm.return_value.get_status.return_value = 5
+                    response = self.client.get(url)
+                self.assertEqual(response.status_code, status)
+                self.assertEqual('id="delete_form"' in response.content.decode(), form)

instances/views.py

@@ -137,6 +137,10 @@ 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
+    # get_instance(perm_type="delete"): a template only for superusers and staff owners
+    can_delete = request.user.is_superuser or bool(
+        userinstance and userinstance.is_delete and (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)
@@ -393,9 +397,9 @@ 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); a template
-        only for superusers and staff owners
+      - 'change': superuser or (UserInstance owner and is_change)
       - 'delete': superuser or (UserInstance owner and is_delete)
+      A template is changed or deleted only by superusers and staff owners.
     """
     valid_perms = {"view", "power", "change", "delete"}
     if perm_type not in valid_perms:
@@ -424,7 +428,7 @@ def get_instance(user, pk, perm_type="view"):
         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):
+        if not (user_inst and user_inst.is_delete) or (instance.is_template and not user.is_staff):
             raise PermissionDenied
     else:
         raise PermissionDenied
@@ -573,11 +577,14 @@ def poweron(request, pk):
 @serialize_instance_mutation
 def powercycle(request, pk):
     instance = get_instance(request.user, pk, perm_type="power")
-    instance.proxy.force_shutdown()
-    instance.proxy.start()
-    addlogmsg(
-        request.user.username, instance.compute.name, instance.name, _("Power Cycle")
-    )
+    if instance.is_template:
+        messages.warning(request, _("Templates cannot be started."))
+    else:
+        instance.proxy.force_shutdown()
+        instance.proxy.start()
+        addlogmsg(
+            request.user.username, instance.compute.name, instance.name, _("Power Cycle")
+        )
     return get_safe_redirect(
         request, default=reverse("instances:instance", args=[instance.id])
     )
@@ -686,6 +693,8 @@ def destroy(request, pk):
         {
             "instance": instance,
             "userinstance": userinstance,
+            # get_instance(perm_type="delete") above already refused the rest
+            "can_delete": True,
         },
     )
 

webvirtcloud/test_authorization_matrix.py

@@ -128,12 +128,13 @@ 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.
+# A template VM: only superusers and staff owners change or delete it; other
+# owners keep view, power (poweron and powercycle are no-ops 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),
+    "owner_is_delete": (False, False, {"is_delete": True}, VIEW_VM | POWER_AND_CONSOLE),
 }