fix #2302: allow deletion of users when realm enforces TFA

[pve-access-control.git] / src / PVE / AccessControl.pm

diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
index f7d4e7829d31885f321ca508988af4b8ee2ebeea..fcb16bd2dbeceeab5268aa8c9cf7c931d7779ac7 100644 (file)
--- a/src/PVE/AccessControl.pm
+++ b/src/PVE/AccessControl.pm
@@ -24,6 +24,7 @@ use PVE::Auth::AD;
 use PVE::Auth::LDAP;
 use PVE::Auth::PVE;
 use PVE::Auth::PAM;
+use PVE::Auth::OpenId;
 
 # load and initialize all plugins
 
@@ -31,6 +32,7 @@ PVE::Auth::AD->register();
 PVE::Auth::LDAP->register();
 PVE::Auth::PVE->register();
 PVE::Auth::PAM->register();
+PVE::Auth::OpenId->register();
 PVE::Auth::Plugin->init();
 
 # $authdir must be writable by root only!
@@ -53,12 +55,8 @@ my $authkey_lifetime = 3600 * 24; # rotate every 24 hours
 
 Crypt::OpenSSL::RSA->import_random_seed();
 
-cfs_register_file('user.cfg',
-                 \&parse_user_config,
-                 \&write_user_config);
-cfs_register_file('priv/tfa.cfg',
-                 \&parse_priv_tfa_config,
-                 \&write_priv_tfa_config);
+cfs_register_file('user.cfg', \&parse_user_config, \&write_user_config);
+cfs_register_file('priv/tfa.cfg', \&parse_priv_tfa_config, \&write_priv_tfa_config);
 
 sub verify_username {
     PVE::Auth::Plugin::verify_username(@_);
@@ -432,12 +430,10 @@ sub verify_token {
     check_user_enabled($usercfg, $username);
     check_token_exist($usercfg, $username, $token);
 
-    my $ctime = time();
-
     my $user = $usercfg->{users}->{$username};
-    die "account expired\n" if $user->{expire} && ($user->{expire} < $ctime);
-
     my $token_info = $user->{tokens}->{$token};
+
+    my $ctime = time();
     die "token expired\n" if $token_info->{expire} && ($token_info->{expire} < $ctime);
 
     die "invalid token value!\n" if !PVE::Cluster::verify_token($tokenid, $value);
@@ -583,6 +579,11 @@ sub check_user_enabled {
 
     die "user '$username' is disabled\n" if !$noerr;
 
+    my $ctime = time();
+    my $expire = $usercfg->{users}->{$username}->{expire};
+
+    die "account expired\n" if $expire && ($expire < $ctime);
+
     return undef;
 }
 
@@ -633,11 +634,6 @@ sub authenticate_user {
 
     check_user_enabled($usercfg, $username);
 
-    my $ctime = time();
-    my $expire = $usercfg->{users}->{$username}->{expire};
-
-    die "account expired\n" if $expire && ($expire < $ctime);
-
     my $domain_cfg = cfs_read_file('domains.cfg');
 
     my $cfg = $domain_cfg->{ids}->{$realm};
@@ -819,8 +815,12 @@ my $privgroups = {
        admin => [
            'Pool.Allocate', # create/delete pools
        ],
-       user => [],
-       audit => [],
+       user => [
+           'Pool.Audit',
+       ],
+       audit => [
+           'Pool.Audit',
+       ],
     },
 };
 
@@ -935,12 +935,16 @@ sub check_path {
        /
        |/access
        |/access/groups
+       |/access/groups/[[:alnum:]\.\-\_]+
        |/access/realm
+       |/access/realm/[[:alnum:]\.\-\_]+
        |/nodes
        |/nodes/[[:alnum:]\.\-\_]+
        |/pool
        |/pool/[[:alnum:]\.\-\_]+
        |/sdn
+       |/sdn/zones/[[:alnum:]\.\-\_]+
+       |/sdn/vnets/[[:alnum:]\.\-\_]+
        |/storage
        |/storage/[[:alnum:]\.\-\_]+
        |/vms
@@ -1596,8 +1600,7 @@ sub user_set_tfa {
     }
 
     my $user_cfg = $cached_usercfg || cfs_read_file('user.cfg');
-    my $user = $user_cfg->{users}->{$userid}
-       or die "user '$userid' not found\n";
+    my $user = $user_cfg->{users}->{$userid};
 
     my $domain_cfg = $cached_domaincfg || cfs_read_file('domains.cfg');
     my $realm_cfg = $domain_cfg->{ids}->{$realm};
@@ -1608,6 +1611,7 @@ sub user_set_tfa {
        $realm_tfa = PVE::Auth::Plugin::parse_tfa_config($realm_tfa);
        # If the realm has a TFA setting, we're only allowed to use that.
        if (defined($data)) {
+           die "user '$userid' not found\n" if !defined($user);
            my $required_type = $realm_tfa->{type};
            if ($required_type ne $type) {
                die "realm '$realm' only allows TFA of type '$required_type\n";
@@ -1620,9 +1624,11 @@ sub user_set_tfa {
            # realm-configured tfa always uses a simple key list, so use the user.cfg
            $user->{keys} = $data->{keys};
        } else {
-           die "realm '$realm' does not allow removing the 2nd factor\n";
+           # TFA is enforce by realm, only allow deletion if the whole user gets delete
+           die "realm '$realm' does not allow removing the 2nd factor\n" if defined($user);
        }
     } else {
+       die "user '$userid' not found\n" if !defined($user);
        # Without a realm-enforced TFA setting the user can add a u2f or totp entry by themselves.
        # The 'yubico' type requires yubico server settings, which have to be configured on the
        # realm, so this is not supported here:
@@ -1646,10 +1652,10 @@ sub user_set_tfa {
        delete $tfa_cfg->{users}->{$userid};
        cfs_write_file('priv/tfa.cfg', $tfa_cfg);
 
-       delete $user->{keys};
+       delete $user->{keys} if defined($user);
     }
 
-    cfs_write_file('user.cfg', $user_cfg);
+    cfs_write_file('user.cfg', $user_cfg) if defined($user);
 }
 
 sub user_get_tfa {