[pve-devel] [PATCH v2 manager 1/4] ui: qemu/HardwareView: eslint: enforce "max-len" rule

[pve-devel] [PATCH v2 manager 1/4] ui: qemu/HardwareView: eslint: enforce "max-len" rule

[Aaron Lauterer]

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
v2: moved all lines of the parameter call to their separate lines after
the call

 www/manager6/qemu/HardwareView.js | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 77640e53..3c4ce821 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -594,7 +594,11 @@ Ext.define('PVE.qemu.HardwareView', {
 
 	    var isEfi = key === 'efidisk0';
 
-	    remove_btn.setDisabled(rec.data.delete || rowdef.never_delete === true || (isUnusedDisk && !diskCap));
+	    remove_btn.setDisabled(
+		rec.data.delete ||
+		rowdef.never_delete === true ||
+		(isUnusedDisk && !diskCap),
+	    );
 	    remove_btn.setText(isUsedDisk && !isCloudInit ? remove_btn.altText : remove_btn.defaultText);
 	    remove_btn.RESTMethod = isUnusedDisk ? 'POST':'PUT';
 
-- 
2.20.1

[pve-devel] [PATCH v2 manager 2/4] ui: qemu/HardwareView: eslint: enforce "no-useless-concat" rule

[Aaron Lauterer]

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---

v2: use template strings

 www/manager6/qemu/HardwareView.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 3c4ce821..730a44d9 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -612,7 +612,7 @@ Ext.define('PVE.qemu.HardwareView', {
 	};
 
 	Ext.apply(me, {
-	    url: '/api2/json/' + 'nodes/' + nodename + '/qemu/' + vmid + '/pending',
+	    url: `/api2/json/nodes/${nodename}/qemu/${vmid}/pending`,
 	    interval: 5000,
 	    selModel: sm,
 	    run_editor: run_editor,
-- 
2.20.1

[pve-devel] [PATCH v2 manager 3/4] ui: qemu/HardwareView: eslint: enforce "no-shadow" rule

[Aaron Lauterer]

By removing global vars 'i' and 'confid' and declaring them with let in the needed
context.

'i' wasn't necessary but had to be touched anyway.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---

v2: removed the global definitions for i and confid and declared them
locally where needed.

 www/manager6/qemu/HardwareView.js | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 730a44d9..036bfa26 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -45,7 +45,6 @@ Ext.define('PVE.qemu.HardwareView', {
 
     initComponent: function() {
 	var me = this;
-	var i, confid;
 
 	var nodename = me.pveSelNode.data.node;
 	if (!nodename) {
@@ -220,7 +219,7 @@ Ext.define('PVE.qemu.HardwareView', {
 	};
 
 	PVE.Utils.forEachBus(undefined, function(type, id) {
-	    var confid = type + id;
+	    let confid = type + id;
 	    rows[confid] = {
 		group: 10,
 		iconCls: 'hdd-o',
@@ -232,8 +231,8 @@ Ext.define('PVE.qemu.HardwareView', {
 		cloudheader: gettext('CloudInit Drive') + ' (' + confid + ')',
 	    };
 	});
-	for (i = 0; i < PVE.Utils.hardware_counts.net; i++) {
-	    confid = "net" + i.toString();
+	for (let i = 0; i < PVE.Utils.hardware_counts.net; i++) {
+	    let confid = "net" + i.toString();
 	    rows[confid] = {
 		group: 15,
 		order: i,
@@ -250,8 +249,8 @@ Ext.define('PVE.qemu.HardwareView', {
 	    never_delete: !caps.vms['VM.Config.Disk'],
 	    header: gettext('EFI Disk'),
 	};
-	for (i = 0; i < PVE.Utils.hardware_counts.usb; i++) {
-	    confid = "usb" + i.toString();
+	for (let i = 0; i < PVE.Utils.hardware_counts.usb; i++) {
+	    let confid = "usb" + i.toString();
 	    rows[confid] = {
 		group: 25,
 		order: i,
@@ -261,8 +260,8 @@ Ext.define('PVE.qemu.HardwareView', {
 		header: gettext('USB Device') + ' (' + confid + ')',
 	    };
 	}
-	for (i = 0; i < PVE.Utils.hardware_counts.hostpci; i++) {
-	    confid = "hostpci" + i.toString();
+	for (let i = 0; i < PVE.Utils.hardware_counts.hostpci; i++) {
+	    let confid = "hostpci" + i.toString();
 	    rows[confid] = {
 		group: 30,
 		order: i,
@@ -272,8 +271,8 @@ Ext.define('PVE.qemu.HardwareView', {
 		header: gettext('PCI Device') + ' (' + confid + ')',
 	    };
 	}
-	for (i = 0; i < PVE.Utils.hardware_counts.serial; i++) {
-	    confid = "serial" + i.toString();
+	for (let i = 0; i < PVE.Utils.hardware_counts.serial; i++) {
+	    let confid = "serial" + i.toString();
 	    rows[confid] = {
 		group: 35,
 		order: i,
@@ -289,7 +288,7 @@ Ext.define('PVE.qemu.HardwareView', {
 	    never_delete: !caps.vms['VM.Config.HWType'],
 	    header: gettext('Audio Device'),
 	};
-	for (i = 0; i < 256; i++) {
+	for (let i = 0; i < 256; i++) {
 	    rows["unused" + i.toString()] = {
 		group: 99,
 		order: i,
@@ -531,8 +530,8 @@ Ext.define('PVE.qemu.HardwareView', {
 	let isAtLimit = (type) => counts[type] >= PVE.Utils.hardware_counts[type];
 
 	var set_button_status = function() {
-	    var sm = me.getSelectionModel();
-	    var rec = sm.getSelection()[0];
+	    var selection_model = me.getSelectionModel();
+	    var rec = selection_model.getSelection()[0];
 
 	    // en/disable hardwarebuttons
 	    counts = {};
-- 
2.20.1

[pve-devel] [PATCH v2 qemu-server] API: update_vm_api: check for CDROM on disk delete

[Aaron Lauterer]

Since CDRoms and disks share the same config keys, we need to check if
it actually is a CDRom and then check the permissions accordingly.

Otherwise it is possible for someone without VM.Config.CDROM
permissions, but with VM.Config.Disk permissions to remove a CD drive
while being unable to create a CDRom drive.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---

Since it is possible to delete a CDRom while not having the permissions
to create them, I consider this a bug.

With this patch it is also possible to now remove a CDRom drive with
only the VM.Config.CDROM permissions which needed VM.Config.Disk
permissions before. Creating them with the CDRom permissions has already
been possible before.

 PVE/API2/Qemu.pm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 3571f5e..feb9ea8 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1235,7 +1235,12 @@ my $update_vm_api  = sub {
 		    }
 		} elsif (PVE::QemuServer::is_valid_drivename($opt)) {
 		    PVE::QemuConfig->check_protection($conf, "can't remove drive '$opt'");
-		    $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk']);
+		    my $drive = PVE::QemuServer::parse_drive($opt, $val);
+		    if (PVE::QemuServer::drive_is_cdrom($drive)) {
+			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM']);
+		    } else {
+			$rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk']);
+		    }
 		    PVE::QemuServer::vmconfig_register_unused_drive($storecfg, $vmid, $conf, PVE::QemuServer::parse_drive($opt, $val))
 			if $is_pending_val;
 		    PVE::QemuConfig->add_to_pending_delete($conf, $opt, $force);
-- 
2.20.1

[pve-devel] [PATCH v2 manager 4/4] ui: qemu/HardwareView: fix CDRom permission checkss

[Aaron Lauterer]

There are several types of drives that use the same config keys. Most
notably CDRom and regular VM disks (EFI and cloudinit exist as well).

Since there is a dedicated permission for CDRom drives we need to check
permissions in more detail, depending on what type of drive it actually
is for things like the edit, remove and Add -> CDRom buttons.

The permission check in the row definition itself which only checked for
'VM.Config.Disk' permissions (never_delete) had to be removed and finer
grained checks added for the individual buttons. This also meant a bit
of reshuffling in the checks what kind of disk the current one is.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---

v2: improved permission checks in the GUI to make sure that CDRom things
will behave as expected and the other things are kept as is. Meaning all
other disks (cloudinit, efi, used, unused) still need VM.Config.Disk
permissions.

Tested by giving a user only VMAudit and Config.CDRom permissions in the
one case, and all VM.Config.* permissions except CDRom.

 www/manager6/qemu/HardwareView.js | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 036bfa26..5de3bf95 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -224,7 +224,6 @@ Ext.define('PVE.qemu.HardwareView', {
 		group: 10,
 		iconCls: 'hdd-o',
 		editor: 'PVE.qemu.HDEdit',
-		never_delete: !caps.vms['VM.Config.Disk'],
 		isOnStorageBus: true,
 		header: gettext('Hard Disk') + ' (' + confid +')',
 		cdheader: gettext('CD/DVD Drive') + ' (' + confid +')',
@@ -584,24 +583,33 @@ Ext.define('PVE.qemu.HardwareView', {
 	    var value = rec.data.value;
 	    var rowdef = rows[key];
 
+	    let isCloudInit = value && value.toString().match(/vm-.*-cloudinit/);
+	    let isCDRom = value && !!value.toString().match(/media=cdrom/) && !isCloudInit;
+
 	    var pending = rec.data.delete || me.hasPendingChanges(key);
-	    var isCDRom = value && !!value.toString().match(/media=cdrom/);
 	    var isUnusedDisk = key.match(/^unused\d+/);
-	    var isUsedDisk = !isUnusedDisk && rowdef.isOnStorageBus && !isCDRom;
-
-	    var isCloudInit = value && value.toString().match(/vm-.*-cloudinit/);
+	    let isUsedDisk = !isUnusedDisk && rowdef.isOnStorageBus && !isCDRom && !isCloudInit;
 
 	    var isEfi = key === 'efidisk0';
 
+	    let isDisk = isCloudInit || isUnusedDisk || isUsedDisk;
+
 	    remove_btn.setDisabled(
 		rec.data.delete ||
 		rowdef.never_delete === true ||
-		(isUnusedDisk && !diskCap),
+		(isCDRom && !caps.vms['VM.Config.CDROM']) ||
+		(isDisk && !diskCap),
 	    );
 	    remove_btn.setText(isUsedDisk && !isCloudInit ? remove_btn.altText : remove_btn.defaultText);
 	    remove_btn.RESTMethod = isUnusedDisk ? 'POST':'PUT';
 
-	    edit_btn.setDisabled(rec.data.delete || !rowdef.editor || isCloudInit || (!isCDRom && !diskCap));
+	    edit_btn.setDisabled(
+		rec.data.delete ||
+		!rowdef.editor ||
+		isCloudInit ||
+		(isCDRom && !caps.vms['VM.Config.CDROM']) ||
+		(isDisk && !diskCap),
+	    );
 
 	    resize_btn.setDisabled(pending || !isUsedDisk || !diskCap);
 
@@ -637,7 +645,7 @@ Ext.define('PVE.qemu.HardwareView', {
 			    {
 				text: gettext('CD/DVD Drive'),
 				iconCls: 'pve-itype-icon-cdrom',
-				disabled: !caps.vms['VM.Config.Disk'],
+				disabled: !caps.vms['VM.Config.CDROM'],
 				handler: function() {
 				    var win = Ext.create('PVE.qemu.CDEdit', {
 					url: '/api2/extjs/' + baseurl,
-- 
2.20.1

[pve-devel] applied: [PATCH v2 manager 1/4] ui: qemu/HardwareView: eslint: enforce "max-len" rule

[Thomas Lamprecht]

On 12.02.21 16:57, Aaron Lauterer wrote:
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> v2: moved all lines of the parameter call to their separate lines after
> the call
> 
>  www/manager6/qemu/HardwareView.js | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH v2 manager 2/4] ui: qemu/HardwareView: eslint: enforce "no-useless-concat" rule

[Thomas Lamprecht]

On 12.02.21 16:57, Aaron Lauterer wrote:
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> 
> v2: use template strings
> 
>  www/manager6/qemu/HardwareView.js | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH v2 manager 3/4] ui: qemu/HardwareView: eslint: enforce "no-shadow" rule

[Thomas Lamprecht]

On 12.02.21 16:57, Aaron Lauterer wrote:
> By removing global vars 'i' and 'confid' and declaring them with let in the needed
> context.
> 
> 'i' wasn't necessary but had to be touched anyway.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> 
> v2: removed the global definitions for i and confid and declared them
> locally where needed.
> 
>  www/manager6/qemu/HardwareView.js | 25 ++++++++++++-------------
>  1 file changed, 12 insertions(+), 13 deletions(-)
> 
>

applied, thanks!

[pve-devel] applied: [PATCH v2 manager 4/4] ui: qemu/HardwareView: fix CDRom permission checkss

[Thomas Lamprecht]

On 12.02.21 16:57, Aaron Lauterer wrote:
> There are several types of drives that use the same config keys. Most
> notably CDRom and regular VM disks (EFI and cloudinit exist as well).
> 
> Since there is a dedicated permission for CDRom drives we need to check
> permissions in more detail, depending on what type of drive it actually
> is for things like the edit, remove and Add -> CDRom buttons.
> 
> The permission check in the row definition itself which only checked for
> 'VM.Config.Disk' permissions (never_delete) had to be removed and finer
> grained checks added for the individual buttons. This also meant a bit
> of reshuffling in the checks what kind of disk the current one is.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> 
> v2: improved permission checks in the GUI to make sure that CDRom things
> will behave as expected and the other things are kept as is. Meaning all
> other disks (cloudinit, efi, used, unused) still need VM.Config.Disk
> permissions.
> 
> Tested by giving a user only VMAudit and Config.CDRom permissions in the
> one case, and all VM.Config.* permissions except CDRom.
> 
>  www/manager6/qemu/HardwareView.js | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
>

applied, but prefixed with some further cleanups which result was merged
into this one, so maybe a re-check would be good.
much thanks!

[pve-devel] applied: [PATCH v2 qemu-server] API: update_vm_api: check for CDROM on disk delete

[Thomas Lamprecht]

On 12.02.21 16:57, Aaron Lauterer wrote:
> Since CDRoms and disks share the same config keys, we need to check if
> it actually is a CDRom and then check the permissions accordingly.
> 
> Otherwise it is possible for someone without VM.Config.CDROM
> permissions, but with VM.Config.Disk permissions to remove a CD drive
> while being unable to create a CDRom drive.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> 
> Since it is possible to delete a CDRom while not having the permissions
> to create them, I consider this a bug.
> 
> With this patch it is also possible to now remove a CDRom drive with
> only the VM.Config.CDROM permissions which needed VM.Config.Disk
> permissions before. Creating them with the CDRom permissions has already
> been possible before.
> 
>  PVE/API2/Qemu.pm | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
>

applied, thanks!

For the record: technically this is, in the widest sense, a backward incompatible
change and I even had a commit here prepared which would allow any of those
permissions (see below), but I dropped it, this is such a niche use case that I just
cannot believe that anybody will be affected by it - one needs to have CDROM for
adding, so basically a CDROM dev would need to be there, then a user with a role
containing 'VM.Config.Disk' but *not* 'VM.Config.CDROM', whom only needs to remove
CDROM devices but not add them, cannot do that anymore, yeah, no, really not worth
the hassle.


obsoleted dropped diff for the record only:

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index feb9ea8..c932571 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -1237,7 +1237,8 @@ my $update_vm_api  = sub {
                    PVE::QemuConfig->check_protection($conf, "can't remove drive '$opt'");
                    my $drive = PVE::QemuServer::parse_drive($opt, $val);
                    if (PVE::QemuServer::drive_is_cdrom($drive)) {
-                       $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM']);
+                       # FIXME: remove 'VM.Config.Disk' and $any flag for PVE 7
+                       $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM', 'VM.Config.Disk'], 1);
                    } else {
                        $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.Disk']);
                    }