Re: [pbs-devel] [PATCH proxmox-backup 3/5] fix #4382: api2: remove permissions and tokens of user on deletion

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 20/12/2022 15:57, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  src/api2/access/user.rs | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
> index ce676252..40177c8d 100644
> --- a/src/api2/access/user.rs
> +++ b/src/api2/access/user.rs
> @@ -346,7 +346,7 @@ pub fn update_user(
>  )]
>  /// Remove a user from the configuration file.
>  pub fn delete_user(userid: Userid, digest: Option<String>) -> Result<(), Error> {
> -    let _lock = pbs_config::user::lock_config()?;
> +    let _user_lock = pbs_config::user::lock_config()?;
>      let _tfa_lock = crate::config::tfa::write_lock()?;
>  
>      let (mut config, expected_digest) = pbs_config::user::config()?;
> @@ -390,6 +390,28 @@ pub fn delete_user(userid: Userid, digest: Option<String>) -> Result<(), Error>
>          }
>      }
>  
> +    // delete_token needs the user config lock, therefore we have to drop it here
> +    drop(_user_lock);

opens us up for a data race though. Why not have a private (or marked unsafe)
delete_token_no_lock fn that we can call here and in delete_token?

Alternative but similar: a private do_delete_token that takes the locks as
parameters, probably nicer.

Did not looked at the whole code closely in recent times, so there may even
better ways - possible encapsulating this in a impl on a User/AuthId type
(especially helpful if we want to re-use this for other projects); but for now
I think the do_delete_token taking the locks as parameters would be the
simplest and nicest to avoid races and other issues.

> +
> +    let user_tokens: Vec<ApiToken> = config
> +        .convert_to_typed_array::<ApiToken>("token")?
> +        .into_iter()
> +        .filter(|token| token.tokenid.user().eq(&userid))
> +        .collect();
> +    for token in user_tokens {
> +        if let Some(name) = token.tokenid.tokenname() {
> +            let user = token.tokenid.user();
> +            delete_token(user.clone(), name.to_owned(), None)?
> +        }
> +    }
> +
> +    // delete_token needs the acl config lock, that's why it is not aquired at the beginning

s/aquired/acquired/

> +    let _acl_lock = pbs_config::acl::lock_config()?;
> +
> +    let (mut tree, _digest) = pbs_config::acl::config()?;
> +    tree.delete_authid(&Authid::from(userid));
> +    pbs_config::acl::save_config(&tree)?;
> +
>      Ok(())
>  }
>