From 3230be1bc0924a7920f040148aa7800837e59c7b Mon Sep 17 00:00:00 2001 From: Michal Fox Date: Fri, 2 Oct 2026 16:47:52 +0200 Subject: [PATCH] fix #6429: openid: use the updated user config for the login permissions 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 --- 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 100644 --- 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();