diff --git a/README.md b/README.md index 1cb896a0..2554cc53 100644 --- a/README.md +++ b/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). diff --git a/doc/permissions.md b/doc/permissions.md index 6fdf3154..f27f588d 100644 --- a/doc/permissions.md +++ b/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. diff --git a/instances/templates/instances/destroy_instance_form.html b/instances/templates/instances/destroy_instance_form.html index 430aab41..5420fd17 100644 --- a/instances/templates/instances/destroy_instance_form.html +++ b/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 %}
{% trans "Instance is suspended, cannot destroy!" %} diff --git a/instances/templates/instances/destroy_tab.html b/instances/templates/instances/destroy_tab.html index fc80215e..a788c6b3 100644 --- a/instances/templates/instances/destroy_tab.html +++ b/instances/templates/instances/destroy_tab.html @@ -12,7 +12,7 @@

{% trans 'This action starts remove instance process' %}

- {% if request.user.is_superuser or userinstance.is_delete %} + {% if can_delete %} {% if instance.status == 3 %} {% trans "Destroy" %} {% else %} diff --git a/instances/test_page_controls.py b/instances/test_page_controls.py index bfaf6faf..ed7ed8b0 100644 --- a/instances/test_page_controls.py +++ b/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}) diff --git a/instances/test_template_rules.py b/instances/test_template_rules.py new file mode 100644 index 00000000..8b589a0d --- /dev/null +++ b/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) diff --git a/instances/views.py b/instances/views.py index db94a7ac..29ffa5c3 100755 --- a/instances/views.py +++ b/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, }, ) diff --git a/webvirtcloud/test_authorization_matrix.py b/webvirtcloud/test_authorization_matrix.py index b516b75b..15e3a6fa 100644 --- a/webvirtcloud/test_authorization_matrix.py +++ b/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), }