[pve-devel] [PATCH access-control 0/2] improve permission self-service

[pve-devel] [PATCH access-control 0/2] improve permission self-service

[Fabian Grünbichler]

noticed this while testing https://lore.proxmox.com/pve-devel/20241031134629.144893-1-d.kral@proxmox.com

the first patch fixes the already allowed "permission self-service" for
users as the web UI implements it (it always passes the $userid
parameter).

the second patch extends that self-service to allow users without
Sys.Audit on /access to evaluate their own tokens' ACLs/permissions,
which seems sensible to me ;)

Fabian Grünbichler (2):
  api: permissions: allow users to view their own permissions
  api: permissions: allow users to check their own tokens

 src/PVE/API2/AccessControl.pm | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH access-control 1/2] api: permissions: allow users to view their own permissions

[Fabian Grünbichler]

even when specifying an explicit userid matching their own.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/PVE/API2/AccessControl.pm | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/PVE/API2/AccessControl.pm b/src/PVE/API2/AccessControl.pm
index c55a7b3..157a5ee 100644
--- a/src/PVE/API2/AccessControl.pm
+++ b/src/PVE/API2/AccessControl.pm
@@ -486,14 +486,14 @@ __PACKAGE__->register_method({
 	my ($param) = @_;
 
 	my $rpcenv = PVE::RPCEnvironment::get();
+	my $authid = $rpcenv->get_user();
 
 	my $userid = $param->{userid};
-	if (defined($userid)) {
+	$userid = $authid if !defined($userid);
+
+	if ($userid ne $authid) {
 	    $rpcenv->check($rpcenv->get_user(), '/access', ['Sys.Audit']);
-	} else {
-	    $userid = $rpcenv->get_user();
 	}
-
 	my $res;
 
 	if (my $path = $param->{path}) {
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH access-control 2/2] api: permissions: allow users to check their own tokens

[Fabian Grünbichler]

even if they lack Sys.Audit on /access - since tokens are self-service,
checking whether the ACLs work as expected should also be doable for every
user.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/PVE/API2/AccessControl.pm | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/src/PVE/API2/AccessControl.pm b/src/PVE/API2/AccessControl.pm
index 157a5ee..7fcf6fb 100644
--- a/src/PVE/API2/AccessControl.pm
+++ b/src/PVE/API2/AccessControl.pm
@@ -460,7 +460,11 @@ __PACKAGE__->register_method({
     method => 'GET',
     description => 'Retrieve effective permissions of given user/token.',
     permissions => {
-	description => "Each user/token is allowed to dump their own permissions. A user can dump the permissions of another user if they have 'Sys.Audit' permission on /access.",
+	description => "Each user/token is allowed to dump their own ".
+	               "permissions (or that of owned tokens). A user  ".
+	               "can dump the permissions of another user or ".
+	               "their tokens if they have 'Sys.Audit' permission ".
+	               "on /access.",
 	user => 'all',
     },
     parameters => {
@@ -491,7 +495,11 @@ __PACKAGE__->register_method({
 	my $userid = $param->{userid};
 	$userid = $authid if !defined($userid);
 
-	if ($userid ne $authid) {
+	my ($user, $token) = PVE::AccessControl::split_tokenid($userid, 1);
+	my $check_self = $userid eq $authid;
+	my $check_owned_token = defined($user) && $user eq $authid;
+
+	if (!($check_self || $check_owned_token)) {
 	    $rpcenv->check($rpcenv->get_user(), '/access', ['Sys.Audit']);
 	}
 	my $res;
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH access-control 0/2] improve permission self-service

[Daniel Kral]

On 11/5/24 09:30, Fabian Grünbichler wrote:
> noticed this while testing https://lore.proxmox.com/pve-devel/20241031134629.144893-1-d.kral@proxmox.com
> 
> the first patch fixes the already allowed "permission self-service" for
> users as the web UI implements it (it always passes the $userid
> parameter).
> 
> the second patch extends that self-service to allow users without
> Sys.Audit on /access to evaluate their own tokens' ACLs/permissions,
> which seems sensible to me ;)
> 
> Fabian Grünbichler (2):
>    api: permissions: allow users to view their own permissions
>    api: permissions: allow users to check their own tokens
> 
>   src/PVE/API2/AccessControl.pm | 18 +++++++++++++-----
>   1 file changed, 13 insertions(+), 5 deletions(-)
> 

I have written some test cases for this and sent the patch to the 
mailing list [0].

[0] 
https://lore.proxmox.com/pve-devel/20241106144813.189056-1-d.kral@proxmox.com/

With the test cases included there, consider the whole series as:

Tested-by: Daniel Kral <d.kral@proxmox.com>


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] applied-series: [PATCH access-control 0/2] improve permission self-service

[Thomas Lamprecht]

Am 05.11.24 um 09:30 schrieb Fabian Grünbichler:
> noticed this while testing https://lore.proxmox.com/pve-devel/20241031134629.144893-1-d.kral@proxmox.com
> 
> the first patch fixes the already allowed "permission self-service" for
> users as the web UI implements it (it always passes the $userid
> parameter).
> 
> the second patch extends that self-service to allow users without
> Sys.Audit on /access to evaluate their own tokens' ACLs/permissions,
> which seems sensible to me ;)
> 
> Fabian Grünbichler (2):
>   api: permissions: allow users to view their own permissions
>   api: permissions: allow users to check their own tokens
> 
>  src/PVE/API2/AccessControl.pm | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 


applied with Daniel's T-b, thanks!

ps. I made a small code-style follow-up for breaking up the schema description
over multiple lines so that it follows our style guide [0], there were some other
bad instances in that file  anyway, so I did it as separate patch.

[0]: https://pve.proxmox.com/wiki/Perl_Style_Guide#Wrapping_Strings


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel