[pve-devel] [PATCH manager] fix #474: allow transfer from container/vms

[pve-devel] [PATCH manager] fix #474: allow transfer from container/vms

[Philipp Hufnagl]

When a member of 2 pools wants to transfer a
 vm/container to an other pool they can not do that. The vv/container would
 have first to be removed form the current pool resulting in a loss of
 privileges of the pool member for this vm/contianer. This feature introduces
 a way to transfer a vm between pools, guarded by a checkbox from accidental
 transfers

Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
---
 PVE/API2/Pool.pm                 | 19 +++++++++++++++++--
 www/manager6/grid/PoolMembers.js | 17 ++++++++++++++---
 2 files changed, 31 insertions(+), 5 deletions(-)

diff --git a/PVE/API2/Pool.pm b/PVE/API2/Pool.pm
index 007fc815..2f69911a 100644
--- a/PVE/API2/Pool.pm
+++ b/PVE/API2/Pool.pm
@@ -131,6 +131,11 @@ __PACKAGE__->register_method ({
 		type => 'string',  format => 'pve-storage-id-list',
 		optional => 1,
 	    },
+	    transfer => {
+		description => "Allow transfering vms to another pool.",
+		type => 'boolean',
+		optional => 1,
+	    },
 	    delete => {
 		description => "Remove vms/storage (instead of adding it).",
 		type => 'boolean',
@@ -165,8 +170,18 @@ __PACKAGE__->register_method ({
 		    } else {
 			die "VM $vmid is already a pool member\n" if $pool_config->{vms}->{$vmid};
 			my $existing_pool = $usercfg->{vms}->{$vmid};
-			die "VM $vmid belongs already to pool '$existing_pool'\n" if defined($existing_pool);
-
+			if(defined($existing_pool) )
+			{
+			    if($param->{transfer})
+			    {
+				my $existing_pool_config = $usercfg->{pools}->{$existing_pool};
+				delete $existing_pool_config->{vms}->{$vmid};
+			    }
+			    else
+			    {
+				die "VM $vmid belongs already to pool '$existing_pool' and transfer is not set\n";
+			    }
+			}
 			$pool_config->{vms}->{$vmid} = 1;
 			$usercfg->{vms}->{$vmid} = $pool;
 		    }
diff --git a/www/manager6/grid/PoolMembers.js b/www/manager6/grid/PoolMembers.js
index 6acb622d..224daca3 100644
--- a/www/manager6/grid/PoolMembers.js
+++ b/www/manager6/grid/PoolMembers.js
@@ -1,7 +1,7 @@
 Ext.define('PVE.pool.AddVM', {
     extend: 'Proxmox.window.Edit',
     width: 600,
-    height: 400,
+    height: 420,
     isAdd: true,
     isCreate: true,
     initComponent: function() {
@@ -30,7 +30,7 @@ Ext.define('PVE.pool.AddVM', {
 	    ],
 	    filters: [
 		function(item) {
-		    return (item.data.type === 'lxc' || item.data.type === 'qemu') && item.data.pool === '';
+		    return (item.data.type === 'lxc' || item.data.type === 'qemu') &&item.data.pool !== me.pool;
 		},
 	    ],
 	});
@@ -63,6 +63,10 @@ Ext.define('PVE.pool.AddVM', {
 		    header: gettext('Node'),
 		    dataIndex: 'node',
 		},
+		{
+		    header: gettext('Pool'),
+		    dataIndex: 'pool',
+		},
 		{
 		    header: gettext('Status'),
 		    dataIndex: 'uptime',
@@ -85,9 +89,16 @@ Ext.define('PVE.pool.AddVM', {
 		},
 	    ],
 	});
+
+	let transfer = Ext.create('Ext.form.field.Checkbox', {
+	    name: 'transfer',
+	    boxLabel: gettext('Allow Transfer'),
+	    inputValue: 1,
+	    value: 0,
+	});
 	Ext.apply(me, {
 	    subject: gettext('Virtual Machine'),
-	    items: [vmsField, vmGrid],
+	    items: [vmsField, vmGrid, transfer],
 	});
 
 	me.callParent();
-- 
2.39.2

Re: [pve-devel] [PATCH manager] fix #474: allow transfer from container/vms

[Fiona Ebner]

Am 08.08.23 um 11:13 schrieb Philipp Hufnagl:
> When a member of 2 pools wants to transfer a
>  vm/container to an other pool they can not do that. The vv/container would
>  have first to be removed form the current pool resulting in a loss of
>  privileges of the pool member for this vm/contianer. This feature introduces
>  a way to transfer a vm between pools, guarded by a checkbox from accidental
>  transfers
> 

Nit: there's an additional space at the start of each line, also some
typos, e.g. vv/contianer

> Signed-off-by: Philipp Hufnagl <p.hufnagl@proxmox.com>
> ---
>  PVE/API2/Pool.pm                 | 19 +++++++++++++++++--
>  www/manager6/grid/PoolMembers.js | 17 ++++++++++++++---
>  2 files changed, 31 insertions(+), 5 deletions(-)

The backend and UI changes should be two different patches.

> 
> diff --git a/PVE/API2/Pool.pm b/PVE/API2/Pool.pm
> index 007fc815..2f69911a 100644
> --- a/PVE/API2/Pool.pm
> +++ b/PVE/API2/Pool.pm
> @@ -131,6 +131,11 @@ __PACKAGE__->register_method ({
>  		type => 'string',  format => 'pve-storage-id-list',
>  		optional => 1,
>  	    },
> +	    transfer => {
> +		description => "Allow transfering vms to another pool.",

s/transfering vms/transferring VMs/

> +		type => 'boolean',
> +		optional => 1,
> +	    },
>  	    delete => {
>  		description => "Remove vms/storage (instead of adding it).",
>  		type => 'boolean',
> @@ -165,8 +170,18 @@ __PACKAGE__->register_method ({
>  		    } else {
>  			die "VM $vmid is already a pool member\n" if $pool_config->{vms}->{$vmid};
>  			my $existing_pool = $usercfg->{vms}->{$vmid};
> -			die "VM $vmid belongs already to pool '$existing_pool'\n" if defined($existing_pool);
> -
> +			if(defined($existing_pool) )
> +			{
> +			    if($param->{transfer})

Style nit: both ifs are missing a space after the keyword, the upper one
has a space too much before the parenthesis. The curly braces should be
on the same line

> +			    {
> +				my $existing_pool_config = $usercfg->{pools}->{$existing_pool};

The permission for the original pool should be checked here?! Or is that
already done somewhere?

> +				delete $existing_pool_config->{vms}->{$vmid};
> +			    }
> +			    else
> +			    {

Style nit: should be } else {

> +				die "VM $vmid belongs already to pool '$existing_pool' and transfer is not set\n";
> +			    }
> +			}
>  			$pool_config->{vms}->{$vmid} = 1;
>  			$usercfg->{vms}->{$vmid} = $pool;
>  		    }

Re: [pve-devel] [PATCH manager] fix #474: allow transfer from container/vms

[Philipp Hufnagl]

On 8/9/23 13:32, Fiona Ebner wrote:

> The permission for the original pool should be checked here?! Or is 
> that already done somewhere? 

The permission of the original pool does not matter. The permission of 
the VM is important
(maybe the original pool granting the user permission on the VM). 
Hovever I tested it with granting the
user merely audit permissions on the VM and admin permissions on the 
target pool and still got the
correct permission error so I don't think the permission checks have to 
be modified at all

Re: [pve-devel] [PATCH manager] fix #474: allow transfer from container/vms

[Fiona Ebner]

Am 09.08.23 um 16:20 schrieb Philipp Hufnagl:
> On 8/9/23 13:32, Fiona Ebner wrote:
> 
>> The permission for the original pool should be checked here?! Or is
>> that already done somewhere? 
> 
> The permission of the original pool does not matter.

But it should. After all, the operation is modifying the original pool,
so the user better have an appropriate permission to do so.

> The permission of the VM is important
> (maybe the original pool granting the user permission on the VM).
> Hovever I tested it with granting the
> user merely audit permissions on the VM and admin permissions on the
> target pool and still got the
> correct permission error so I don't think the permission checks have to
> be modified at all
> 

Currently, Permissions.Modify|VM.Allocate on the VM and Pool.Allocate on
the target pool would be enough to "steal" the guest, no permissions
required on the original pool at all. IMHO, the user really should have
a Pool.Allocate on the original pool as well.

Since I noticed it in v3: we usually use "api:" and "ui:" as prefixes
rather than "backend:" and "frontend:". Would be nice if you could use
them too for consistency.

Re: [pve-devel] [PATCH manager] fix #474: allow transfer from container/vms

[Philipp Hufnagl]

On 8/10/23 09:16, Fiona Ebner wrote:
> But it should. After all, the operation is modifying the original pool,
> so the user better have an appropriate permission to do so.

> Currently, Permissions.Modify|VM.Allocate on the VM and Pool.Allocate on
> the target pool would be enough to "steal" the guest, no permissions
> required on the original pool at all. IMHO, the user really should have
> a Pool.Allocate on the original pool as well.

You are right! It would be possible to "steal" a VM in a way that it was 
not before!

Thank you for finding this! Will fix!

> Since I noticed it in v3: we usually use "api:" and "ui:" as prefixes
> rather than "backend:" and "frontend:". Would be nice if you could use
> them too for consistency.

Ok. Good to know. I will do that. Thanks