[PATCH access-control] fix #6429: openid: use the updated user config for the login permissions

[PATCH access-control] fix #6429: openid: use the updated user config for the login permissions

[Michal Fox]

When the OpenID login creates a user, or adds a user to groups from the
groups claim, it writes these changes to user.cfg. The permissions it
returns for the web UI however got computed with the copy of user.cfg
the RPC environment loaded at the start of the request, which does not
know about the new user or group memberships yet.

So a user logging in for the first time got no permissions from the
ACLs of its groups in this session, and buttons like "Create VM" stayed
disabled, until the next login. The same happened to existing users on
the login that added them to a new group.

Add a helper to reload the user config of the RPC environment, and use
it after the login changed user.cfg, before computing the permissions,
as suggested in the bug.

Add a test for both cases, which mocks the OpenID provider and keeps
user.cfg in memory.

Signed-off-by: Michal Fox <me@dualfroz.com>
---

Notes:
    tested with 'make test' in src. without the change to OpenId.pm and
    RPCEnvironment.pm, the new test fails for both cases, as the returned
    'cap' has no VM privileges.

 src/PVE/API2/OpenId.pm        |   7 ++
 src/PVE/RPCEnvironment.pm     |   9 +++
 src/test/api-tests.pl         |   2 +-
 src/test/openid-login-test.pl | 137 ++++++++++++++++++++++++++++++++++
 4 files changed, 154 insertions(+), 1 deletion(-)
 create mode 100644 src/test/openid-login-test.pl

diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
index 429cb3a..5b03a8a 100644
--- a/src/PVE/API2/OpenId.pm
+++ b/src/PVE/API2/OpenId.pm
@@ -206,6 +206,8 @@ __PACKAGE__->register_method({
             # first, check if $username respects our naming conventions
             PVE::Auth::Plugin::verify_username($username);
 
+            my $user_cfg_changed;
+
             if ($config->{'autocreate'} && !$rpcenv->check_user_exist($username, 1)) {
                 PVE::AccessControl::lock_user_config(
                     sub {
@@ -231,6 +233,7 @@ __PACKAGE__->register_method({
                     },
                     "autocreate openid user failed",
                 );
+                $user_cfg_changed = 1;
             } else {
                 # test if user exists and is enabled
                 $rpcenv->check_user_enabled($username);
@@ -316,6 +319,7 @@ __PACKAGE__->register_method({
                             },
                             "openid group mapping failed",
                         );
+                        $user_cfg_changed = 1;
                     } else {
                         syslog(
                             'err',
@@ -327,6 +331,9 @@ __PACKAGE__->register_method({
                 }
             }
 
+            # the cached user config of this request does not know about the changes yet
+            $rpcenv->reload_user_config() if $user_cfg_changed;
+
             my $ticket = PVE::AccessControl::assemble_ticket($username);
             my $csrftoken = PVE::AccessControl::assemble_csrf_prevention_token($username);
             my $cap = $rpcenv->compute_api_permission($username);
diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
index 7591aa9..c627ae5 100644
--- a/src/PVE/RPCEnvironment.pm
+++ b/src/PVE/RPCEnvironment.pm
@@ -635,6 +635,15 @@ sub init_request {
     }
 }
 
+# reload the user config, needed if the current request modified it and the permissions should
+# already reflect that
+sub reload_user_config {
+    my ($self) = @_;
+
+    $self->{aclcache} = {};
+    $self->{user_cfg} = PVE::Cluster::cfs_read_file('user.cfg');
+}
+
 # hacks: to provide better backwards compatibility
 
 # old code uses PVE::RPCEnvironment::get();
diff --git a/src/test/api-tests.pl b/src/test/api-tests.pl
index a1a987a..f7e202a 100755
--- a/src/test/api-tests.pl
+++ b/src/test/api-tests.pl
@@ -7,6 +7,6 @@ use TAP::Harness;
 
 my $harness = TAP::Harness->new({ verbosity => -1 });
 
-my $result = $harness->runtests('api-get-permissions-test.pl');
+my $result = $harness->runtests('api-get-permissions-test.pl', 'openid-login-test.pl');
 
 exit -1 if $result->{failed};
diff --git a/src/test/openid-login-test.pl b/src/test/openid-login-test.pl
new file mode 100644
index 0000000..e533088
--- /dev/null
+++ b/src/test/openid-login-test.pl
@@ -0,0 +1,137 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+
+use lib qw(..);
+
+use Test::More;
+use Test::MockModule;
+
+use PVE::AccessControl;
+use PVE::RPCEnvironment;
+use PVE::API2::OpenId;
+
+my $realm_config = {
+    type => 'openid',
+    'issuer-url' => 'https://idp.example.com',
+    'client-id' => 'pve',
+    autocreate => 1,
+    'groups-claim' => 'groups',
+};
+
+my $user_cfg_raw;
+my $user_cfg_version = 1;
+
+my $read_file = sub {
+    my ($filename) = @_;
+
+    return PVE::AccessControl::parse_user_config('/etc/pve/user.cfg', $user_cfg_raw)
+        if $filename eq 'user.cfg';
+    return { ids => { oidc => $realm_config } } if $filename eq 'domains.cfg';
+    return {} if $filename eq 'datacenter.cfg';
+
+    die "unexpected read of '$filename'\n";
+};
+
+my $write_file = sub {
+    my ($filename, $cfg) = @_;
+
+    die "unexpected write of '$filename'\n" if $filename ne 'user.cfg';
+
+    $user_cfg_raw = PVE::AccessControl::write_user_config('/etc/pve/user.cfg', $cfg);
+    $user_cfg_version++;
+};
+
+my $cluster_module = Test::MockModule->new('PVE::Cluster');
+$cluster_module->noop('cfs_update', 'log_msg');
+$cluster_module->mock(
+    cfs_file_version => sub { return ($user_cfg_version, {}) },
+    cfs_read_file => $read_file,
+    get_clinfo => sub { return {} },
+);
+
+my $access_control_module = Test::MockModule->new('PVE::AccessControl');
+$access_control_module->mock(
+    cfs_lock_file => sub {
+        my ($filename, $timeout, $code) = @_;
+        my $res = eval { $code->() };
+        return $res;
+    },
+    assemble_ticket => sub { return "ticket:$_[0]" },
+    assemble_csrf_prevention_token => sub { return "csrf:$_[0]" },
+);
+
+my $openid_api_module = Test::MockModule->new('PVE::API2::OpenId');
+$openid_api_module->mock(
+    cfs_read_file => $read_file,
+    cfs_write_file => $write_file,
+    syslog => sub { },
+);
+
+my $claims;
+my $openid_module = Test::MockModule->new('PVE::RS::OpenId');
+$openid_module->mock(
+    verify_public_auth_state => sub { return ('oidc', 'private-state') },
+    discover => sub { return bless {}, 'PVE::RS::OpenId' },
+    verify_authorization_code => sub { return $claims },
+);
+
+my $rpcenv = PVE::RPCEnvironment->init('cli');
+
+my ($handler, $handler_info) = PVE::API2::OpenId->find_handler('POST', 'login');
+
+# each test is comprised of the following keys:
+# description => what the test is about
+# user_cfg => user.cfg before the login
+# claims => claims returned by the OpenID provider
+# expected_vms_cap => VM privileges the login result must contain
+my $tests = [
+    {
+        description => 'new user added to a group with ACLs',
+        user_cfg => <<"EOF",
+group:tenant-oidc:::
+
+acl:1:/vms:\@tenant-oidc:PVEVMAdmin:
+EOF
+        claims => { sub => 'alice', groups => ['tenant'] },
+        expected_vms_cap => ['VM.Allocate', 'VM.PowerMgmt'],
+    },
+    {
+        description => 'existing user newly added to a group with ACLs',
+        user_cfg => <<"EOF",
+user:bob\@oidc:1:0:::::
+
+group:tenant-oidc:::
+
+acl:1:/vms:\@tenant-oidc:PVEVMAdmin:
+EOF
+        claims => { sub => 'bob', groups => ['tenant'] },
+        expected_vms_cap => ['VM.Allocate', 'VM.PowerMgmt'],
+    },
+];
+
+for my $test (@$tests) {
+    $user_cfg_raw = $test->{user_cfg};
+    $claims = $test->{claims};
+
+    $rpcenv->init_request();
+
+    my $login = eval {
+        $handler->handle(
+            $handler_info,
+            {
+                state => 'public-state',
+                code => 'authorization-code',
+                'redirect-url' => 'https://pve.example.com:8006',
+            },
+        );
+    };
+    is($@, '', "$test->{description}: login succeeds");
+
+    my $vms_cap = $login->{cap}->{vms} // {};
+    my $missing = [grep { !$vms_cap->{$_} } $test->{expected_vms_cap}->@*];
+    is_deeply($missing, [], "$test->{description}: permissions are active right away");
+}
+
+done_testing();
-- 
2.43.0