[pdm-devel] [PATCH proxmox 2/4] access-control: add acl api feature

[Shannon Sterz]

On Wed Apr 9, 2025 at 2:58 PM CEST, Shannon Sterz wrote:
> On Wed Apr 9, 2025 at 1:39 PM CEST, Dominik Csapak wrote:
>> On 4/9/25 13:01, Dietmar Maurer wrote:
>> maybe something like this for the update case (untested, please verify before using this!):
>> (the diff is for pbs, where the code was copied from)
>>
>> this also includes a reformatted check for the token,non-token, same user checks
>> that are IMHO more readable than what we currently have
>> with the match, i think it's much more obvious that all cases are handled
>>
>> ---
>>       let user_info = CachedUserInfo::new()?;
>>
>> -    let top_level_privs = user_info.lookup_privs(&current_auth_id, &["access", "acl"]);
>> -    if top_level_privs & PRIV_PERMISSIONS_MODIFY == 0 {
>> +    let has_modify_permission = user_info
>> +        .check_privs(
>> +            &current_auth_id,
>> +            &["access", "acl"],
>> +            PRIV_PERMISSIONS_MODIFY,
>> +            false,
>> +        )
>> +        .is_ok();
>> +
>> +    if !has_modify_permission {
>>           if group.is_some() {
>>               bail!("Unprivileged users are not allowed to create group ACL item.");
>>           }
>>
>>           match &auth_id {
>>               Some(auth_id) => {
>> -                if current_auth_id.is_token() {
>> -                    bail!("Unprivileged API tokens can't set ACL items.");
>> -                } else if !auth_id.is_token() {
>> -                    bail!("Unprivileged users can only set ACL items for API tokens.");
>> -                } else if auth_id.user() != current_auth_id.user() {
>> -                    bail!("Unprivileged users can only set ACL items for their own API tokens.");
>> +                let same_user = auth_id.user() == current_auth_id.user();
>> +                match (current_auth_id.is_token(), auth_id.is_token(), same_user) {
>> +                    (true, _, _) => bail!("Unprivileged API tokens can't set ACL items."),
>> +                    (false, false, _) => {
>> +                        bail!("Unprivileged users can only set ACL items for API tokens.")
>> +                    }
>> +                    (false, true, true) => {
>> +                        // users are always allowed to modify ACLs for their own tokens
>> +                    }
>> +                    (false, true, false) => {
>> +                        bail!("Unprivileged users can only set ACL items for their own API tokens.")
>> +                    }
>>                   }
>>               }
>>               None => {
>> ---

had another think about this, i'd tend towards something like the below.
the match statement is a nice idea, but it couples together things that
aren't really related. for example, why pull in the
`current_auth_id.is_token()` check, but not the `group.is_some()` check?
having match statements with tuples like this is making the code more
complex. imo, this is simpler:

```rs
    let unprivileged_user = CachedUserInfo::new()?
        .check_privs(
            &current_auth_id,
            &["access", "acl"],
            access_conf.acl_modify_privileges(),
            access_conf.allow_partial_permission_match(),
        )
        .is_err();

    if unprivileged_user {
        if group.is_some() {
            bail!("Unprivileged users are not allowed to create group ACL item.");
        }

        let auth_id = auth_id.as_ref().ok_or_else(|| {
            format_err!("Unprivileged user needs to provide auth_id to update ACL item.")
        })?;

        if current_auth_id.is_token() {
            bail!("Unprivileged API tokens can't set ACL items.");
        }

        if !auth_id.is_token() {
            bail!("Unprivileged users can only set ACL items for API tokens.");
        }

        if current_auth_id != *auth_id {
            bail!("Unprivileged users can only set ACL items for their own API tokens.");
        }
    }
```

what do you think?