Thank you so much for the review and i will update it accordingly.
Had some doubts which i added as inline comments.
On Tue Jul 29, 2025 at 11:45 AM CEST, Fabian Grünbichler wrote:
> On July 29, 2025 10:30 am, Shan Shaji wrote:
> > When removing TFA from a user via the command line, the change was not
> > reflected in the GUI or in the output of `pveum user list`. Both
> > continued to show that TFA was enabled for the user. Fixed the issue
> > by updating the user configuration file.
> >
> > Signed-off-by: Shan Shaji <s.sh...@proxmox.com>
> > ---
> >
> > Tested Cases:
> > - Case 1:
> > - Added one TFA for a user.
> > - Delete with CLI `pveum user tfa delete <user>`
> > - Verified if the UI and CLI outputs are updated.
> > - Case 2:
> > - Add two TFA for a user.
> > - Delete with CLI `pveum user tfa delete <user>`
> > - Verified if the UI and CLI outputs are updated.
> > - Case 3:
> > - Add two TFA for a user.
> > - Delete with CLI `pveum user tfa delete <user> --id <id>`
> > - Verified if the UI and CLI outputs are updated.
> >
> > src/PVE/CLI/pveum.pm | 16 ++++++++++++++--
> > 1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/PVE/CLI/pveum.pm b/src/PVE/CLI/pveum.pm
> > index 0645a6d..87e68df 100755
> > --- a/src/PVE/CLI/pveum.pm
> > +++ b/src/PVE/CLI/pveum.pm
> > @@ -141,16 +141,28 @@ __PACKAGE__->register_method({
> >
> > my $userid = extract_param($param, "userid");
> > my $tfa_id = extract_param($param, "id");
> > + my $update_user_config;
> >
> > PVE::AccessControl::lock_tfa_config(sub {
> > my $tfa_cfg = cfs_read_file('priv/tfa.cfg');
> > if (defined($tfa_id)) {
> > - $tfa_cfg->api_delete_tfa($userid, $tfa_id);
> > + my $has_entries_left = $tfa_cfg->api_delete_tfa($userid,
> > $tfa_id);
> > + $update_user_config = !$has_entries_left;
>
> this one is nice split like that, but
>
> > } else {
> > - $tfa_cfg->remove_user($userid);
> > + my $needs_user_saving = $tfa_cfg->remove_user($userid);
> > + $update_user_config = $needs_user_saving;
>
> these two lines could be combined IMHO, or we could even unconditionally
> set $update_user_config here - if we (are asked to) remove all TFA
> entries for a particular user, we want to clear the 'x' entry even if no
> TFA entries existed in the first place (e.g., because the configs ran
> out of sync).
So it's okay if i set `$update_user_config = 1;`
> > }
> > cfs_write_file('priv/tfa.cfg', $tfa_cfg);
> > });
> > +
> > + if ($update_user_config) {
> > + PVE::AccessControl::lock_user_config(sub {
> > + my $user_cfg = cfs_read_file('user.cfg');
> > + my $user = $user_cfg->{users}->{$userid};
> > + $user->{keys} = undef;
> > + cfs_write_file('user.cfg', $user_cfg);
> > + });
> > + }
>
> the locking here is incomplete - in the meantime, another invocation
> could have added a TFA entry again, and potentially the 'x' marker is
> dropped afterwards making user.cfg wrong/out of sync..
>
> PVE::API2::TFA::delete_tfa() has the same issue, but in
> PVE::API2::TFA::add_tfa_entry we have a nested call:
So the order will the lock TFA -> lock user cfg -> update user cfg ->
update tfa cfg.
> lock_tfa_config() {
> set_user_tfa_enabled() {
> lock_user_config() {
> add 'x' entry to user.cfg
> }
> }
> add tfa entry to tfa.cfg
> }
>
> if that lock order is used everywhere else as well, the two deletion
> paths (API and CLI) should also be done like that, to avoid races. there
> might be more code paths missing proper nested locking, I haven't done a
> complete analysis.
>
> > return;
> > },
> > });
> > --
> > 2.39.5
> >
> >
> >
> > _______________________________________________
> > pve-devel mailing list
> > pve-devel@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> >
> >
> >
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
