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

[Dominik Csapak <d.csapak@proxmox.com>]

On 4/11/25 12:29, Shannon Sterz wrote:
> 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?

I see what you mean, and yes i think it's more readable, but what I really wanted to convey with my
approach was to clarify which condition is ok

we currently try to filter out all invalid states, and it is not really obvious what
condition makes the code continue at first glance

Maybe we have to approach it completely different, for example check only the valid cases first
and let that pass through, and then fail with the specific errors and have a fallback error
for all other cases. that way we can't come into a situation where we forget/overlook some edge
case.


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