mirror of
https://github.com/retspen/webvirtcloud
synced 2026-11-09 03:36:48 +00:00
feat(instances): a VM's owners with their rights, and XML edits kept until saved
Settings > Users was a bold title, an unlabelled "+" and a table of names with a trash icon. The owner chose this layout from three rendered options: one list of owners, each with what it may do (change settings, delete the VM, change console settings, or power and console only), Edit rights and Remove, an empty state, and a labelled "Add owner". - Remove and Edit rights come back to the VM's Users tab: the return address carried #users unencoded, so it was lost, and Edit rights went to the user's account page. Both accounts views now go to a ?next page of this site. - The add-owner list offers only users who are not owners yet and not administrators, who can use every VM anyway; a superuser owner also took the only place when a VM may have one owner. - The unused del_owner view and URL are removed: the list uses the accounts page, which asks before removing an owner. XML: - An XML edit not saved yet is kept: the page no longer reloads itself when the VM's state changes, and the browser asks before leaving it. A state change lost the edit. - The button that opens the review dialog reads "Review changes"; it did not save, as "Change" suggested. Closing the dialog gives focus back to the editor.
This commit is contained in:
catborise
parent
1150b941be
commit
3d632f1592
12 changed files with 125 additions and 85 deletions
|
|
@ -311,3 +311,22 @@ class SingleOwnerEditTests(TestCase):
|
|||
self.assertRedirects(response, reverse("accounts:account", args=[bob.pk]), fetch_redirect_response=False)
|
||||
ui.refresh_from_db()
|
||||
self.assertTrue(ui.is_vnc)
|
||||
|
||||
|
||||
class OwnerRightsReturnTestCase(TestCase):
|
||||
"""Edit rights on a VM's Users tab comes back to that tab; a page of
|
||||
another site does not count."""
|
||||
|
||||
def test_an_owner_s_rights_come_back_to_the_vm_page(self):
|
||||
admin = get_user_model().objects.create_superuser("rights-admin", "rights@example.com", "x")
|
||||
owner = get_user_model().objects.create_user("rights-owner")
|
||||
compute = Compute.objects.create(name="rights", hostname="192.0.2.30", login="u", password="p", type=1)
|
||||
vm = Instance.objects.create(compute=compute, name="rights-vm", uuid="12121212-0000-0000-0000-000000000000")
|
||||
user_instance = UserInstance.objects.create(user=owner, instance=vm)
|
||||
self.client.force_login(admin)
|
||||
update = reverse("accounts:user_instance_update", args=[user_instance.id])
|
||||
for next_url, back in ((f"/instances/{vm.id}/%23users", f"/instances/{vm.id}/#users"),
|
||||
("https://example.com/", reverse("accounts:account", args=[owner.id]))):
|
||||
with self.subTest(next_url):
|
||||
response = self.client.post(f"{update}?next={next_url}", {"is_change": True})
|
||||
self.assertRedirects(response, back, fetch_redirect_response=False)
|
||||
|
|
|
|||
|
|
@ -235,6 +235,17 @@ def user_instance_create(request, user_id):
|
|||
)
|
||||
|
||||
|
||||
def _next_or(request, default):
|
||||
"""To the ?next page, the VM page an owner was changed from, when it is
|
||||
on this site; else to default."""
|
||||
next_url = request.GET.get("next")
|
||||
if next_url and url_has_allowed_host_and_scheme(
|
||||
url=next_url, allowed_hosts={request.get_host()}, require_https=request.is_secure()
|
||||
):
|
||||
return redirect(next_url)
|
||||
return redirect(default)
|
||||
|
||||
|
||||
@superuser_only
|
||||
def user_instance_update(request, pk):
|
||||
user_instance = get_object_or_404(UserInstance, pk=pk)
|
||||
|
|
@ -246,7 +257,7 @@ def user_instance_update(request, pk):
|
|||
fields = changed_fields(before, snapshot(user_instance))
|
||||
entry.action = f"change owner {user_instance.user.username}: " + ", ".join(fields)
|
||||
entry.skip = not fields
|
||||
return redirect(reverse("accounts:account", args=[user_instance.user.id]))
|
||||
return _next_or(request, reverse("accounts:account", args=[user_instance.user.id]))
|
||||
|
||||
return render(
|
||||
request,
|
||||
|
|
@ -265,14 +276,7 @@ def user_instance_delete(request, pk):
|
|||
user, instance = user_instance.user, user_instance.instance
|
||||
with audit_vm(request, instance, f"remove owner {user.username}"):
|
||||
user_instance.delete()
|
||||
next_url = request.GET.get("next", None)
|
||||
if next_url and url_has_allowed_host_and_scheme(
|
||||
url=next_url,
|
||||
allowed_hosts={request.get_host()},
|
||||
require_https=request.is_secure(),
|
||||
):
|
||||
return redirect(next_url)
|
||||
return redirect(reverse("accounts:account", args=[user.id]))
|
||||
return _next_or(request, reverse("accounts:account", args=[user.id]))
|
||||
|
||||
return render(
|
||||
request,
|
||||
|
|
|
|||
|
|
@ -1,35 +1,34 @@
|
|||
{% load i18n %}
|
||||
{% load bootstrap_icons %}
|
||||
{% if request.user.is_superuser %}
|
||||
<a href="#addInstanceOwner" type="button" class="btn btn-primary float-end" data-bs-toggle="modal">
|
||||
<span aria-hidden="true">{% bs_icon 'plus'%} </span>
|
||||
</a>
|
||||
<button type="button" class="btn btn-sm btn-primary" data-bs-toggle="modal" data-bs-target="#addInstanceOwner">{% bs_icon 'plus' %} {% trans "Add owner" %}</button>
|
||||
|
||||
<!-- Modal pool -->
|
||||
<div class="modal fade" id="addInstanceOwner" tabindex="-1" role="dialog" aria-labelledby="addInstanceOwnerLabel" aria-hidden="true">
|
||||
<div class="modal-dialog">
|
||||
<div class="modal-content">
|
||||
<div class="modal-header">
|
||||
<h5 class="modal-title">{% trans "Add Instance Owner" %}</h5>
|
||||
<h5 class="modal-title" id="addInstanceOwnerLabel">{% trans "Add owner" %}</h5>
|
||||
<button type="button" class="btn-close" data-bs-dismiss="modal" aria-label="Close"></button>
|
||||
</div>
|
||||
<div class="modal-body">
|
||||
<form id="add-owner-form" method="post" action="{% url 'instances:add_owner' instance.id %}" role="form" aria-label="Add instance owner form">{% csrf_token %}
|
||||
<div class="row">
|
||||
<label class="col-sm-4 col-form-label">{% trans "User" %}</label>
|
||||
<div class="col-sm-6">
|
||||
<select class="form-select" name="user_id">
|
||||
{% for user in users %}
|
||||
<option value="{{ user.id }}">{{ user.username }}</option>
|
||||
{% endfor %}
|
||||
</select>
|
||||
</div>
|
||||
</div>
|
||||
{# users who are not owners yet; a superuser can use every VM #}
|
||||
{% if users %}
|
||||
<label class="form-label" for="add-owner-user">{% trans "User" %}</label>
|
||||
<select class="form-select" name="user_id" id="add-owner-user">
|
||||
{% for user in users %}
|
||||
<option value="{{ user.id }}">{{ user.username }}</option>
|
||||
{% endfor %}
|
||||
</select>
|
||||
{% else %}
|
||||
<p class="mb-0">{% trans "Every user who is not an administrator is already an owner." %}</p>
|
||||
{% endif %}
|
||||
</form>
|
||||
</div>
|
||||
<div class="modal-footer">
|
||||
<button type="button" class="btn btn-secondary" data-bs-dismiss="modal">{% trans "Close" %}</button>
|
||||
<button type="submit" class="btn btn-primary" form="add-owner-form">{% trans "Add" %}</button>
|
||||
<button type="submit" class="btn btn-primary" form="add-owner-form"{% if not users %} disabled{% endif %}>{% trans "Add" %}</button>
|
||||
</div>
|
||||
</div> <!-- /.modal-content -->
|
||||
</div> <!-- /.modal-dialog -->
|
||||
|
|
|
|||
|
|
@ -344,7 +344,8 @@
|
|||
var status_poll = pollWhileVisible(5000, function () {
|
||||
return $.getJSON("{% url 'instances:status' instance.id %}");
|
||||
}, function get_status(data) {
|
||||
if (data['status'] != vm_status && !backgroundJobRunning) {
|
||||
// an XML edit not saved yet stays: the reload waits until it is saved or undone
|
||||
if (data['status'] != vm_status && !backgroundJobRunning && !(window.xmlEditUnsaved && window.xmlEditUnsaved())) {
|
||||
window.location.reload()
|
||||
}
|
||||
});
|
||||
|
|
|
|||
|
|
@ -581,16 +581,14 @@
|
|||
</div>
|
||||
<div role="tabpanel" class="tab-pane" id="xmledit">
|
||||
{% if instance.status != 5 %}
|
||||
<p id="xml-next-start">{% trans "This is the definition for the VM's next start; a saved change applies then." %}</p>
|
||||
<p class="small text-body-secondary" id="xml-next-start">{% trans "This is the definition for the VM's next start; a saved change applies then." %}</p>
|
||||
{% endif %}
|
||||
<form action="{% url 'instances:change_xml' instance.id %}" method="post" role="form" aria-label="Edit instance XML form" id="xml-form">
|
||||
{% csrf_token %}
|
||||
<div class="col-sm-12 xmlheight">
|
||||
<textarea id="editor">{{ instance.inst_xml }}</textarea>
|
||||
</div>
|
||||
<button type="submit" class="btn btn-primary float-end" name="change_xml">
|
||||
{% trans "Change" %}
|
||||
</button>
|
||||
<button type="submit" class="btn btn-primary float-end" name="change_xml">{% trans "Review changes" %}</button>
|
||||
</form>
|
||||
<div class="clearfix"></div>
|
||||
<div class="modal fade" id="xml-review" tabindex="-1" aria-labelledby="xml-review-title" aria-hidden="true"
|
||||
|
|
@ -615,29 +613,41 @@
|
|||
</div>
|
||||
</div>
|
||||
<div role="tabpanel" class="tab-pane" id="users">
|
||||
<div>
|
||||
<div class="mb-3">
|
||||
<span class="fw-bold">{% trans "Instance owners" %}</span>
|
||||
{% include 'add_instance_owner_block.html' %}
|
||||
</div>
|
||||
</div>
|
||||
<div class="table-responsive">
|
||||
<table class="table table-striped mt-3 align-middle">
|
||||
<tbody class="searchable">
|
||||
<section aria-labelledby="owners-heading">
|
||||
<div class="d-flex flex-wrap justify-content-between align-items-center gap-2 mb-2">
|
||||
<h2 class="h6 mb-0" id="owners-heading">{% trans "Instance owners" %}</h2>
|
||||
{% include 'add_instance_owner_block.html' %}
|
||||
</div>
|
||||
<p class="small text-body-secondary">{% trans "Every owner can power the VM and open its console. A new owner has no other right until it is given one." %}</p>
|
||||
{% if userinstances %}
|
||||
<ul class="list-group">
|
||||
{% for userinstance in userinstances %}
|
||||
<tr>
|
||||
<td><a href="{% url 'accounts:account' userinstance.user.id %}">{{ userinstance.user }}</a></td>
|
||||
<td style="width:30px;">
|
||||
<a href="{% url 'accounts:user_instance_delete' userinstance.id %}?next={% url 'instances:instance' instance.id %}#users">
|
||||
{% bs_icon 'trash' %}
|
||||
</a>
|
||||
</td>
|
||||
</tr>
|
||||
<li class="list-group-item p-3">
|
||||
<div class="row g-3 align-items-center">
|
||||
<div class="col-12 col-md">
|
||||
<a class="fw-semibold text-break" href="{% url 'accounts:account' userinstance.user.id %}">{{ userinstance.user }}</a>
|
||||
<p class="small mb-0 mt-1">
|
||||
{% if userinstance.is_change or userinstance.is_delete %}
|
||||
{% if userinstance.is_change %}{% trans "Change settings" %}{% endif %}{% if userinstance.is_change and userinstance.is_delete %} · {% endif %}{% if userinstance.is_delete %}{% trans "Delete the VM" %}{% endif %}{% if userinstance.is_change and userinstance.is_vnc %} · {% trans "Change console settings" %}{% endif %}
|
||||
{% else %}
|
||||
<span class="text-body-secondary">{% trans "Power and console only" %}</span>
|
||||
{% endif %}
|
||||
</p>
|
||||
</div>
|
||||
{% if request.user.is_superuser %}
|
||||
<div class="col-12 col-md-auto d-flex flex-wrap gap-2">
|
||||
<a class="btn btn-sm btn-secondary" href="{% url 'accounts:user_instance_update' userinstance.id %}?next={% filter urlencode:'' %}{% url 'instances:instance' instance.id %}#users{% endfilter %}">{% trans "Edit rights" %}<span class="visually-hidden"> {{ userinstance.user }}</span></a>
|
||||
<a class="btn btn-sm btn-danger" href="{% url 'accounts:user_instance_delete' userinstance.id %}?next={% filter urlencode:'' %}{% url 'instances:instance' instance.id %}#users{% endfilter %}">{% trans "Remove" %}<span class="visually-hidden"> {{ userinstance.user }}</span></a>
|
||||
</div>
|
||||
{% endif %}
|
||||
</div>
|
||||
</li>
|
||||
{% endfor %}
|
||||
</tbody>
|
||||
</table>
|
||||
</div>
|
||||
<div class="clearfix"></div>
|
||||
</ul>
|
||||
{% else %}
|
||||
<p class="mb-0">{% trans "The VM has no owner: only administrators can use it." %}</p>
|
||||
{% endif %}
|
||||
</section>
|
||||
</div>
|
||||
{% endif %}
|
||||
{% if can_manage_console %}
|
||||
|
|
|
|||
|
|
@ -31,7 +31,7 @@ class VMOwnerRowTests(TestCase):
|
|||
self.client.post(add, {"user_id": self.carol.pk})
|
||||
self.assertFalse(UserInstance.objects.filter(user=self.carol).exists())
|
||||
ui = UserInstance.objects.get(user=self.bob)
|
||||
self.client.post(reverse("instances:del_owner", args=[self.vm.pk]), {"userinstance": ui.pk})
|
||||
self.client.post(reverse("accounts:user_instance_delete", args=[ui.pk]))
|
||||
self.assertEqual(rows(), [
|
||||
("VM vm: add owner bob", True),
|
||||
("VM vm: add owner carol: only one owner is allowed", False),
|
||||
|
|
|
|||
|
|
@ -3,7 +3,6 @@
|
|||
import inspect
|
||||
from unittest.mock import patch
|
||||
|
||||
from accounts.models import UserInstance
|
||||
from computes.models import Compute
|
||||
from django.contrib.auth import get_user_model
|
||||
from django.contrib.messages import get_messages
|
||||
|
|
@ -49,8 +48,6 @@ class ErrorHandlingTestCase(TestCase):
|
|||
# Required fields for the views that 404 without them
|
||||
DATA = {
|
||||
"add_owner": lambda self: {"user_id": get_user_model().objects.get(username="err_admin").id},
|
||||
"del_owner": lambda self: {"userinstance": UserInstance.objects.get_or_create(
|
||||
instance=self.instance, user=get_user_model().objects.get(username="err_admin"))[0].id},
|
||||
"add_network": lambda self: {"add-net-network": "net:default"},
|
||||
"set_qos": lambda self: {"qos_direction": "inbound", "net-mac-0": "52:54:00:00:00:01"},
|
||||
}
|
||||
|
|
@ -68,13 +65,8 @@ class ErrorHandlingTestCase(TestCase):
|
|||
self.assertEqual(response.status_code, 302)
|
||||
self.assertTrue(response.url.startswith(self.page + "#"), response.url)
|
||||
|
||||
def test_owner_views_reject_missing_or_foreign_ids(self):
|
||||
other = Instance.objects.create(compute=self.compute, name="other", uuid="11111111-2222-3333-4444-777777777777")
|
||||
foreign = UserInstance.objects.create(instance=other, user=get_user_model().objects.get(username="err_admin"))
|
||||
def test_add_owner_rejects_a_missing_user(self):
|
||||
self.assertEqual(self.client.post(self.url("add_owner"), {}).status_code, 404)
|
||||
self.assertEqual(self.client.post(self.url("del_owner"), {}).status_code, 404)
|
||||
self.assertEqual(self.client.post(self.url("del_owner"), {"userinstance": foreign.id}).status_code, 404)
|
||||
self.assertTrue(UserInstance.objects.filter(pk=foreign.id).exists())
|
||||
|
||||
def test_same_site_referer_is_kept(self):
|
||||
with patch("instances.models.wvmInstance"):
|
||||
|
|
|
|||
|
|
@ -3,6 +3,7 @@ until the next start. The edit forms show that definition, so a save does
|
|||
not post the running VM's value back over a pending change."""
|
||||
|
||||
from unittest.mock import PropertyMock, patch
|
||||
from urllib.parse import unquote
|
||||
|
||||
from computes.models import Compute
|
||||
from django.contrib.auth import get_user_model
|
||||
|
|
@ -150,6 +151,23 @@ class PendingEditsTestCase(TestCase):
|
|||
self.assertEqual(doc.xpath("//input[@name='rng']/@checked"), ["checked"])
|
||||
self.assertEqual(doc.xpath("//select[@name='on_crash']/option[@selected]/@value"), ["restart"])
|
||||
|
||||
def test_the_users_tab_shows_each_owner_and_offers_only_new_owners(self):
|
||||
from accounts.models import UserInstance
|
||||
|
||||
User = get_user_model()
|
||||
owner = User.objects.create_user("pe-owner")
|
||||
UserInstance.objects.create(instance=self.instance, user=owner, is_change=True, is_vnc=True)
|
||||
User.objects.create_user("pe-new")
|
||||
User.objects.create_superuser("pe-admin2", "pe2@example.com", "x")
|
||||
doc = self.page()
|
||||
(row,) = doc.xpath("//div[@id='users']//li")
|
||||
self.assertIn("Change settings · Change console settings", " ".join(row.text_content().split()))
|
||||
# Remove and Edit rights come back to the Users tab
|
||||
self.assertEqual([unquote(href.split("?next=")[1]) for href in row.xpath(".//a[contains(@class, 'btn')]/@href")],
|
||||
[f"/instances/{self.instance.id}/#users"] * 2)
|
||||
# not an owner yet and not an administrator, who can use every VM anyway
|
||||
self.assertEqual(doc.xpath("//select[@id='add-owner-user']/option/text()"), ["pe-new"])
|
||||
|
||||
def test_buttons_shown_as_disabled_cannot_submit(self):
|
||||
# running: the CPU resize, vCPU hot plug, template and disk removal forms wait for a shutdown;
|
||||
# the title, description and video model are of the next start, saved at once
|
||||
|
|
|
|||
|
|
@ -643,11 +643,7 @@ class InstancesTestCase(TestCase):
|
|||
self.assertEqual(response.status_code, 302)
|
||||
self.assertEqual(UserInstance.objects.count(), 1)
|
||||
|
||||
response = self.client.post(
|
||||
reverse("instances:del_owner", args=[self.instance.id]),
|
||||
{"userinstance": user_instance.id},
|
||||
HTTP_REFERER=reverse("index"),
|
||||
)
|
||||
response = self.client.post(reverse("accounts:user_instance_delete", args=[user_instance.id]))
|
||||
self.assertEqual(response.status_code, 302)
|
||||
self.assertEqual(UserInstance.objects.count(), 0)
|
||||
|
||||
|
|
|
|||
|
|
@ -68,10 +68,7 @@ urlpatterns = [
|
|||
path("<int:pk>/delete_pci_hostdev/", views.delete_pci_hostdev, name="delete_pci_hostdev"),
|
||||
path("<int:pk>/set_link_state/", views.set_link_state, name="set_link_state"),
|
||||
path("<int:pk>/set_qos/", views.set_qos, name="set_qos"),
|
||||
path("<int:pk>/unset_qos/", views.unset_qos, name="unset_qos"),
|
||||
path(
|
||||
"<int:pk>/del_owner/", views.del_owner, name="del_owner"
|
||||
), # no links to this one???
|
||||
path("<int:pk>/unset_qos/", views.unset_qos, name="unset_qos"), # no links to this one???
|
||||
path("<int:pk>/clone/", views.clone, name="clone"),
|
||||
path("<int:pk>/update_console/", views.update_console, name="update_console"),
|
||||
path("<int:pk>/change_options/", views.change_options, name="change_options"),
|
||||
|
|
|
|||
|
|
@ -239,7 +239,8 @@ def instance(request, pk):
|
|||
def _instance_page(request, pk, instance, compute):
|
||||
computes = Compute.objects.all().order_by("name")
|
||||
computes_count = computes.count()
|
||||
users = User.objects.all().order_by("username")
|
||||
# the add-owner choices: users who are not owners yet; a superuser can use every VM
|
||||
users = User.objects.filter(is_superuser=False).exclude(userinstance__instance=instance).order_by("username")
|
||||
publickeys = UserSSHKey.objects.filter(user_id=request.user.id)
|
||||
keymaps = settings.QEMU_KEYMAPS
|
||||
# The console form renders the VNC password, so only users who may manage
|
||||
|
|
@ -2096,19 +2097,6 @@ def add_owner(request, pk):
|
|||
return _back(request, pk, "users")
|
||||
|
||||
|
||||
@require_POST
|
||||
@superuser_only
|
||||
def del_owner(request, pk):
|
||||
instance = get_instance(request.user, pk)
|
||||
userinstance_id = request.POST.get("userinstance", "")
|
||||
if not userinstance_id.isdigit():
|
||||
raise Http404
|
||||
userinstance = get_object_or_404(UserInstance, pk=userinstance_id, instance=instance)
|
||||
with audit_vm(request, instance, f"remove owner {userinstance.user.username}"):
|
||||
userinstance.delete()
|
||||
return _back(request, pk, "users")
|
||||
|
||||
|
||||
# Buses for the empty CD-ROM of a new VM: fdc and virtio have no ejectable
|
||||
# media, and libvirt refuses an empty usb disk.
|
||||
CDROM_BUSES = ("ide", "sata", "scsi")
|
||||
|
|
|
|||
|
|
@ -1,6 +1,8 @@
|
|||
// The VM page's XML editor. Change shows the lines the edit changes in the
|
||||
// VM's definition (instances.views.change_xml), and Save in that dialog
|
||||
// The VM page's XML editor. Review changes shows the lines the edit changes
|
||||
// in the VM's definition (instances.views.change_xml), and Save in that dialog
|
||||
// defines it. A refusal shows in the dialog, and the edit stays in the editor.
|
||||
// An edit not saved yet keeps the page from reloading itself on a VM state
|
||||
// change (window.xmlEditUnsaved), and the browser asks before leaving it.
|
||||
(function () {
|
||||
var form = document.getElementById("xml-form");
|
||||
if (!form) {
|
||||
|
|
@ -16,6 +18,19 @@
|
|||
var diff = document.getElementById("xml-review-diff");
|
||||
var save = document.getElementById("xml-review-save");
|
||||
var reviewed = ""; // Save defines the XML the dialog shows, not later edits
|
||||
var clean = base; // the XML the page has: the opened definition, or the one Save just defined
|
||||
|
||||
window.xmlEditUnsaved = function () {
|
||||
return editor.getValue() !== clean;
|
||||
};
|
||||
window.addEventListener("beforeunload", function (event) {
|
||||
if (window.xmlEditUnsaved()) {
|
||||
event.preventDefault();
|
||||
}
|
||||
});
|
||||
dialog.addEventListener("hidden.bs.modal", function () {
|
||||
editor.focus();
|
||||
});
|
||||
|
||||
// The answer: diff, saved, or error (detail when the VM is busy or moved).
|
||||
// Any other failure, such as a redirect, keeps the edit and says so.
|
||||
|
|
@ -70,6 +85,7 @@
|
|||
save.disabled = true;
|
||||
post(reviewed, true).then(function (answer) {
|
||||
if (answer.saved) {
|
||||
clean = reviewed;
|
||||
location.hash = "xmledit";
|
||||
location.reload();
|
||||
} else {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue