[pve-devel] [PATCH v2 access-control 05/23] rpcenv: drop unused roles()

[Fabian Grünbichler]

it was useful for test-cases to verify the behaviour when pools where
introduced, but it is not used anywhere else in the code base and those
tests can also just check on permission-level.

Signed-off-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
---

Notes:
    alternatively, we can give this the same semantics w.r.t. tokens as
    PVE::AccessControl::roles, but with pool roles mixed in via
    $compile_acl_path->()

 PVE/AccessControl.pm  |  2 +-
 PVE/RPCEnvironment.pm | 23 ------------------
 test/perm-test5.pl    |  2 +-
 test/perm-test6.pl    | 54 +++++++++++++++++++++++++++++++------------
 test/perm-test7.pl    | 24 +++++++++++++++++--
 5 files changed, 63 insertions(+), 42 deletions(-)

diff --git a/PVE/AccessControl.pm b/PVE/AccessControl.pm
index ad97a42..eb21f50 100644
--- a/PVE/AccessControl.pm
+++ b/PVE/AccessControl.pm
@@ -1211,7 +1211,7 @@ sub roles {
     my ($cfg, $user, $path) = @_;
 
     # NOTE: we do not consider pools here.
-    # You need to use $rpcenv->roles() instead if you want that.
+    # Use $rpcenv->permission() for any actual permission checks!
 
     return 'Administrator' if $user eq 'root at pam'; # root can do anything
 
diff --git a/PVE/RPCEnvironment.pm b/PVE/RPCEnvironment.pm
index 95d3029..7e0af70 100644
--- a/PVE/RPCEnvironment.pm
+++ b/PVE/RPCEnvironment.pm
@@ -81,29 +81,6 @@ my $compile_acl_path = sub {
     return $privs;
 };
 
-sub roles {
-   my ($self, $user, $path) = @_;
-
-   if ($user eq 'root at pam') { # root can do anything
-       return ('Administrator');
-   }
-
-   $user = PVE::AccessControl::verify_username($user, 1);
-   return () if !$user;
-
-   my $cache = $self->{aclcache};
-   $cache->{$user} = {} if !$cache->{$user};
-
-   my $acl = $cache->{$user};
-
-   my $roles = $acl->{roles}->{$path};
-   return @$roles if $roles;
-
-   &$compile_acl_path($self, $user, $path);
-   $roles = $acl->{roles}->{$path} || [];
-   return @$roles;
-}
-
 sub permissions {
     my ($self, $user, $path) = @_;
 
diff --git a/test/perm-test5.pl b/test/perm-test5.pl
index 697b959..1da9896 100755
--- a/test/perm-test5.pl
+++ b/test/perm-test5.pl
@@ -14,7 +14,7 @@ $rpcenv->init_request(userconfig => $cfgfn);
 sub check_roles {
     my ($user, $path, $expected_result) = @_;
 
-    my @ra = $rpcenv->roles($user, $path);
+    my @ra = PVE::AccessControl::roles($rpcenv->{user_cfg}, $user, $path);
     my $res = join(',', sort @ra);
 
     die "unexpected result\nneed '${expected_result}'\ngot '$res'\n"
diff --git a/test/perm-test6.pl b/test/perm-test6.pl
index 58ced5f..71e2a70 100755
--- a/test/perm-test6.pl
+++ b/test/perm-test6.pl
@@ -14,7 +14,7 @@ $rpcenv->init_request(userconfig => $cfgfn);
 sub check_roles {
     my ($user, $path, $expected_result) = @_;
 
-    my @ra = $rpcenv->roles($user, $path);
+    my @ra = PVE::AccessControl::roles($rpcenv->{user_cfg}, $user, $path);
     my $res = join(',', sort @ra);
 
     die "unexpected result\nneed '${expected_result}'\ngot '$res'\n"
@@ -23,6 +23,23 @@ sub check_roles {
     print "ROLES:$path:$user:$res\n";
 }
 
+sub check_permissions {
+    my ($user, $path, $expected_result) = @_;
+
+    my $perm = $rpcenv->permissions($user, $path);
+    my $res = join(',', sort keys %$perm);
+
+    die "unexpected result\nneed '${expected_result}'\ngot '$res'\n"
+	if $res ne $expected_result;
+
+    $perm = $rpcenv->permissions($user, $path);
+    $res = join(',', sort keys %$perm);
+    die "unexpected result (compiled)\nneed '${expected_result}'\ngot '$res'\n"
+	if $res ne $expected_result;
+
+    print "PERM:$path:$user:$res\n";
+}
+
 check_roles('User1 at pve', '', '');
 check_roles('User2 at pve', '', '');
 check_roles('User3 at pve', '', '');
@@ -43,25 +60,32 @@ check_roles('User2 at pve', '/vms/300', 'RoleTEST1');
 check_roles('User3 at pve', '/vms/300', 'NoAccess');
 check_roles('User4 at pve', '/vms/300', 'Role1');
 
-check_roles('User1 at pve', '/vms/500', 'RoleDEVEL,RoleTEST1');
-check_roles('User2 at pve', '/vms/500', 'RoleDEVEL,RoleTEST1');
+check_permissions('User1 at pve', '/vms/500', 'VM.Console,VM.PowerMgmt');
+check_permissions('User2 at pve', '/vms/500', 'VM.Console,VM.PowerMgmt');
+# without pool
 check_roles('User3 at pve', '/vms/500', 'NoAccess');
+# with pool
+check_permissions('User3 at pve', '/vms/500', '');
+# without pool
 check_roles('User4 at pve', '/vms/500', '');
+# with pool
+check_permissions('User4 at pve', '/vms/500', '');
 
-check_roles('User1 at pve', '/vms/600', 'RoleMARKETING,RoleTEST1');
-check_roles('User2 at pve', '/vms/600', 'RoleTEST1');
-check_roles('User3 at pve', '/vms/600', 'NoAccess');
-check_roles('User4 at pve', '/vms/600', 'RoleMARKETING');
 
-check_roles('User1 at pve', '/storage/store1', 'RoleDEVEL,RoleMARKETING');
-check_roles('User2 at pve', '/storage/store1', 'RoleDEVEL');
-check_roles('User3 at pve', '/storage/store1', 'RoleDEVEL');
-check_roles('User4 at pve', '/storage/store1', 'RoleMARKETING');
+check_permissions('User1 at pve', '/vms/600', 'VM.Console');
+check_permissions('User2 at pve', '/vms/600', 'VM.Console');
+check_permissions('User3 at pve', '/vms/600', '');
+check_permissions('User4 at pve', '/vms/600', 'VM.Console');
 
-check_roles('User1 at pve', '/storage/store2', 'RoleDEVEL');
-check_roles('User2 at pve', '/storage/store2', 'RoleDEVEL');
-check_roles('User3 at pve', '/storage/store2', 'RoleDEVEL');
-check_roles('User4 at pve', '/storage/store2', '');
+check_permissions('User1 at pve', '/storage/store1', 'VM.Console,VM.PowerMgmt');
+check_permissions('User2 at pve', '/storage/store1', 'VM.PowerMgmt');
+check_permissions('User3 at pve', '/storage/store1', 'VM.PowerMgmt');
+check_permissions('User4 at pve', '/storage/store1', 'VM.Console');
+
+check_permissions('User1 at pve', '/storage/store2', 'VM.PowerMgmt');
+check_permissions('User2 at pve', '/storage/store2', 'VM.PowerMgmt');
+check_permissions('User3 at pve', '/storage/store2', 'VM.PowerMgmt');
+check_permissions('User4 at pve', '/storage/store2', '');
 
 print "all tests passed\n";
 
diff --git a/test/perm-test7.pl b/test/perm-test7.pl
index e2b71a3..2add067 100755
--- a/test/perm-test7.pl
+++ b/test/perm-test7.pl
@@ -14,7 +14,7 @@ $rpcenv->init_request(userconfig => $cfgfn);
 sub check_roles {
     my ($user, $path, $expected_result) = @_;
 
-    my @ra = $rpcenv->roles($user, $path);
+    my @ra = PVE::AccessControl::roles($rpcenv->{user_cfg}, $user, $path);
     my $res = join(',', sort @ra);
 
     die "unexpected result\nneed '${expected_result}'\ngot '$res'\n"
@@ -23,10 +23,30 @@ sub check_roles {
     print "ROLES:$path:$user:$res\n";
 }
 
+sub check_permissions {
+    my ($user, $path, $expected_result) = @_;
+
+    my $perm = $rpcenv->permissions($user, $path);
+    my $res = join(',', sort keys %$perm);
+
+    die "unexpected result\nneed '${expected_result}'\ngot '$res'\n"
+	if $res ne $expected_result;
+
+    $perm = $rpcenv->permissions($user, $path);
+    $res = join(',', sort keys %$perm);
+    die "unexpected result (compiled)\nneed '${expected_result}'\ngot '$res'\n"
+	if $res ne $expected_result;
+
+    print "PERM:$path:$user:$res\n";
+}
 
 check_roles('User1 at pve', '/vms', 'Role1');
 check_roles('User1 at pve', '/vms/200', 'Role1');
-check_roles('User1 at pve', '/vms/100', 'NoAccess');
+
+# no pool
+check_roles('User1 at pve', '/vms/100', 'Role1');
+# with pool
+check_permissions('User1 at pve', '/vms/100', '');
 
 print "all tests passed\n";
 
-- 
2.20.1