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

[Hannes Laimer <h.laimer@proxmox.com>]

On 4/4/25 17:06, Christian Ebner wrote:
> one comment inline
> 
> On 3/20/25 14:57, Hannes Laimer wrote:
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>>   src/api2/access/user.rs | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/src/api2/access/user.rs b/src/api2/access/user.rs
>> index 39cffdba..9bed14a4 100644
>> --- a/src/api2/access/user.rs
>> +++ b/src/api2/access/user.rs
>> @@ -353,6 +353,7 @@ pub async fn update_user(
>>   pub fn delete_user(userid: Userid, digest: Option<String>) -> 
>> Result<(), Error> {
>>       let _lock = pbs_config::user::lock_config()?;
>>       let _tfa_lock = crate::config::tfa::write_lock()?;
>> +    let _acl_lock = pbs_config::acl::lock_config()?;
>>       let (mut config, expected_digest) = pbs_config::user::config()?;
>> @@ -380,6 +381,19 @@ pub fn delete_user(userid: Userid, digest: 
>> Option<String>) -> Result<(), Error>
>>           eprintln!("error updating TFA config after deleting user 
>> {userid:?} {err}",);
>>       }
>> +    let user_tokens: Vec<ApiToken> = config
>> +        .convert_to_typed_array::<ApiToken>("token")?
>> +        .into_iter()
>> +        .filter(|token| token.tokenid.user().eq(&userid))
>> +        .collect();
>> +
>> +    let (mut acl_tree, _digest) = pbs_config::acl::config()?;
>> +    for token in user_tokens {
>> +        if let Some(name) = token.tokenid.tokenname() {
>> +            do_delete_token(name.to_owned(), &userid, &mut config, 
>> &mut acl_tree)?;
> 
> This removes the tokenid from the user_config and acl_config, but that 
> change is never written to the config afterwards? There should be calls 
> to the respective save_config implementations just like for 
> delete_token. Or am I overlooking something?
> 
> The user config and tfa config are already written at this point, so the 
> pbs_config::user::save_config(&config) must be moved below this and the 
> pbs_config::acl::save_config(&acl_tree) added as well.

actually true, I missed that. Not necessarily moving though, having it
in two places is fine, we hold the lock. And given there are some `?`s
I'd rather just add an additional
`pbs_config::user::save_config(&config)`
at the end.

A bit of unlucky timing with my v2, I'll send a v3 fixing this.
Thanks for taking a look!

> 
>> +        }
>> +    }
>> +
>>       Ok(())
>>   }
> 



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel