From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [RFC manager] api: replication: allow users to enumerate accessible replication jobs
Date: Mon, 04 Dec 2023 14:39:48 +0100 [thread overview]
Message-ID: <1701696810.6ovldiybh5.astroid@yuna.none> (raw)
In-Reply-To: <20231201132409.153256-1-l.wagner@proxmox.com>
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
>
>
>
next prev parent reply other threads:[~2023-12-04 13:40 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-12-01 13:24 Lukas Wagner
2023-12-04 13:39 ` Fabian Grünbichler [this message]
2023-12-04 15:54 ` [pve-devel] applied: " Thomas Lamprecht
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1701696810.6ovldiybh5.astroid@yuna.none \
--to=f.gruenbichler@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox