[pve-devel] [RFC manager] api: replication: allow users to enumerate accessible replication jobs

[pve-devel] [RFC manager] api: replication: allow users to enumerate accessible replication jobs

[Lukas Wagner]

Previously, the /cluster/replication API handler would fail completely
with a HTTP 403 if a user does have VM.Audit permissions for
a single VM/CT. That was due to the 'noerr' parameter not set for
$rpcenv->check()

Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
---
Not sure if this violates our API stability guarantees, so I'm sending
this as an RFC in advance. If this change is problematic, we could 
hide the new behavior behind an optional flag.

This change is necessary for retrieving a list of known job-ids for
enhancements to the notification matching rule edit window.

 PVE/API2/ReplicationConfig.pm | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/PVE/API2/ReplicationConfig.pm b/PVE/API2/ReplicationConfig.pm
index 8af62621..d0e8a49e 100644
--- a/PVE/API2/ReplicationConfig.pm
+++ b/PVE/API2/ReplicationConfig.pm
@@ -20,7 +20,8 @@ __PACKAGE__->register_method ({
     method => 'GET',
     description => "List replication jobs.",
     permissions => {
-	description => "Requires the VM.Audit permission on /vms/<vmid>.",
+	description => "Will only return replication jobs for which the calling user has"
+	    . " VM.Audit permission on /vms/<vmid>.",
 	user => 'all',
     },
     parameters => {
@@ -47,7 +48,7 @@ __PACKAGE__->register_method ({
 	foreach my $id (sort keys %{$cfg->{ids}}) {
 	    my $d = $cfg->{ids}->{$id};
 	    my $vmid = $d->{guest};
-	    next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Audit' ]);
+	    next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Audit' ], 1);
 	    $d->{id} = $id;
 	    push @$res, $d;
 	}
-- 
2.39.2

Re: [pve-devel] [RFC manager] api: replication: allow users to enumerate accessible replication jobs

[Fabian Grünbichler]

On December 1, 2023 2:24 pm, Lukas Wagner wrote:
> Previously, the /cluster/replication API handler would fail completely
> with a HTTP 403 if a user does have VM.Audit permissions for
> a single VM/CT. That was due to the 'noerr' parameter not set for
> $rpcenv->check()
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> Not sure if this violates our API stability guarantees, so I'm sending
> this as an RFC in advance. If this change is problematic, we could 
> hide the new behavior behind an optional flag.
> 
> This change is necessary for retrieving a list of known job-ids for
> enhancements to the notification matching rule edit window.

this seems very much in line with how we treat other, similar list calls
for various entities, and was also likely what was originally intended
(the `next if !` doesn't make any sense otherwise, after all).

going from a likely too strict check that is accidentally erroring out,
to the proper check not erroring out is definitely not an API breaking
change - if somebody was relying on this to error out if the calling
user doesn't have access to all replicated VMs, then they are relying on
undocumented behaviour..

consider this

Reviewed-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

>  PVE/API2/ReplicationConfig.pm | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/API2/ReplicationConfig.pm b/PVE/API2/ReplicationConfig.pm
> index 8af62621..d0e8a49e 100644
> --- a/PVE/API2/ReplicationConfig.pm
> +++ b/PVE/API2/ReplicationConfig.pm
> @@ -20,7 +20,8 @@ __PACKAGE__->register_method ({
>      method => 'GET',
>      description => "List replication jobs.",
>      permissions => {
> -	description => "Requires the VM.Audit permission on /vms/<vmid>.",
> +	description => "Will only return replication jobs for which the calling user has"
> +	    . " VM.Audit permission on /vms/<vmid>.",
>  	user => 'all',
>      },
>      parameters => {
> @@ -47,7 +48,7 @@ __PACKAGE__->register_method ({
>  	foreach my $id (sort keys %{$cfg->{ids}}) {
>  	    my $d = $cfg->{ids}->{$id};
>  	    my $vmid = $d->{guest};
> -	    next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Audit' ]);
> +	    next if !$rpcenv->check($authuser, "/vms/$vmid", [ 'VM.Audit' ], 1);
>  	    $d->{id} = $id;
>  	    push @$res, $d;
>  	}
> -- 
> 2.39.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[pve-devel] applied: [RFC manager] api: replication: allow users to enumerate accessible replication jobs

[Thomas Lamprecht]

Am 01/12/2023 um 14:24 schrieb Lukas Wagner:
> Previously, the /cluster/replication API handler would fail completely
> with a HTTP 403 if a user does have VM.Audit permissions for
> a single VM/CT. That was due to the 'noerr' parameter not set for
> $rpcenv->check()
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
> ---
> Not sure if this violates our API stability guarantees, so I'm sending
> this as an RFC in advance. If this change is problematic, we could 
> hide the new behavior behind an optional flag.
> 
> This change is necessary for retrieving a list of known job-ids for
> enhancements to the notification matching rule edit window.
> 
>  PVE/API2/ReplicationConfig.pm | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
>

applied, with Fabian's R-b tag, thanks!