[pdm-devel] [PATCH datacenter-manager 3/3] server: clean up acl tree entries and api tokens when deleting users

[Fabian Grünbichler]

On October 1, 2025 5:29 pm, Shannon Sterz wrote:
> On Fri Sep 26, 2025 at 11:18 AM CEST, Fabian Grünbichler wrote:
>> On September 24, 2025 4:51 pm, Shannon Sterz wrote:
>>> Signed-off-by: Shannon Sterz <s.sterz at proxmox.com>
>>> ---
>>>  server/src/api/access/users.rs | 39 +++++++++++++++++++++++++++++-----
>>>  1 file changed, 34 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/server/src/api/access/users.rs b/server/src/api/access/users.rs
>>> index da598d8..1d1accb 100644
>>> --- a/server/src/api/access/users.rs
>>> +++ b/server/src/api/access/users.rs
>>> @@ -334,20 +334,19 @@ pub fn update_user(
>>>  /// Remove a user from the configuration file.
>>>  pub fn delete_user(userid: Userid, digest: Option<ConfigDigest>) -> Result<(), Error> {
>>>      let _lock = proxmox_access_control::user::lock_config()?;
>>> +    let _acl_lock = proxmox_access_control::acl::lock_config()?;
>>>      let _tfa_lock = crate::auth::tfa::write_lock()?;
>>>
>>> -    let (mut config, config_digest) = proxmox_access_control::user::config()?;
>>> +    let (mut user_config, config_digest) = proxmox_access_control::user::config()?;
>>>      config_digest.detect_modification(digest.as_ref())?;
>>>
>>> -    match config.sections.get(userid.as_str()) {
>>> +    match user_config.sections.get(userid.as_str()) {
>>>          Some(_) => {
>>> -            config.sections.remove(userid.as_str());
>>> +            user_config.sections.remove(userid.as_str());
>>>          }
>>>          None => bail!("user '{}' does not exist.", userid),
>>>      }
>>>
>>> -    proxmox_access_control::user::save_config(&config)?;
>>> -
>>>      let authenticator = crate::auth::lookup_authenticator(userid.realm())?;
>>>      match authenticator.remove_password(userid.name()) {
>>>          Ok(()) => {}
>>> @@ -375,6 +374,36 @@ pub fn delete_user(userid: Userid, digest: Option<ConfigDigest>) -> Result<(), E
>>>          }
>>>      }
>>>
>>> +    let user_tokens: Vec<ApiToken> = user_config
>>> +        .convert_to_typed_array::<ApiToken>("token")?
>>> +        .into_iter()
>>> +        .filter(|token| token.tokenid.user().eq(&userid))
>>> +        .collect();
>>
>> do we have any consistency checks between ACLs and users/tokens? if not,
>> then..
>>
>>> +
>>> +    let (mut acl_config, _digest) = proxmox_access_control::acl::config()?;
>>> +
>>> +    let auth_id = userid.clone().into();
>>> +    acl_config.delete_authid(&auth_id);
>>> +
>>> +    for token in user_tokens {
>>> +        if let Some(token_name) = token.tokenid.tokenname() {
>>> +            let tokenid = Authid::from((userid.clone(), Some(token_name.to_owned())));
>>> +            let tokenid_string = tokenid.to_string();
>>> +            if user_config.sections.remove(&tokenid_string).is_none() {
>>> +                bail!(
>>> +                    "token '{}' of user '{userid}' does not exist.",
>>> +                    token_name.as_str()
>>> +                );
>>> +            }
>>> +
>>> +            proxmox_access_control::token_shadow::delete_secret(&tokenid)?;
>>> +            acl_config.delete_authid(&tokenid);
>>
>> this is not enough to remove all ACLs, since removing a token via the
>> token API currently does not clean up its ACL entries..
> 
> i added the clean up for tokens in a v2.
> 
> but im not entirely sure if i understand you here correctly. are you
> worried about token acls not getting cleaned up if only the token is
> deleted? because then yeah, that part was missing as you pointed out
> there correctly. or is there another scenario you have in mind that i
> currently don't see?

no, that was exactly the issue I was worried about!

delete token => ACL remains
delete user => token doesn't exist, so ACL is not removed either here

in PVE we just drop ACLs on parsing that reference invalid authids..