Re: [pbs-devel] [PATCH proxmox-backup 1/6] backup: hierarchy: add new can_access_any_namespace_in_range helper

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

On 10/3/25 11:52 AM, Thomas Lamprecht wrote:
> Am 03.10.25 um 10:50 schrieb Dominik Csapak:
>> sometimes we need to check the permissions in a range from a starting
>> namespace with a certain depth.
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>>   src/backup/hierarchy.rs | 27 ++++++++++++++++++++-------
>>   1 file changed, 20 insertions(+), 7 deletions(-)
>>
>> diff --git a/src/backup/hierarchy.rs b/src/backup/hierarchy.rs
>> index 8dd71fcf7..438bc3ee3 100644
>> --- a/src/backup/hierarchy.rs
>> +++ b/src/backup/hierarchy.rs
>> @@ -68,19 +68,23 @@ pub fn check_ns_privs_full(
>>       );
>>   }
>>   
>> -pub fn can_access_any_namespace(
>> +/// Checks if the given user has read/access rights on any namespace on the given datastore,
>> +/// beginning with `start_ns` up to `max_depth` below.
>> +pub fn can_access_any_namespace_in_range(
> 
> I would interpret a range being over a linear list, not a tree, the "below"
> you use in the doccomment is already much better fitting, like:
> 
> can_access_any_namespace_below
> 

actually what i had at first^^
i opted for range because of the max-depth limiting, but you're right
range makes not much sense in a tree

> 
>>       store: Arc<DataStore>,
>>       auth_id: &Authid,
>>       user_info: &CachedUserInfo,
>> +    start_ns: Option<BackupNamespace>,
> 
> nit: start is IMO slightly confusing for the tree-like nature of namespaces, maybe
> parent_ns would be better suited?

sounds good

> 
>> +    max_depth: Option<usize>,
>>   ) -> bool {
>> +    let ns = start_ns.unwrap_or_default();
>>       // NOTE: traversing the datastore could be avoided if we had an "ACL tree: is there any priv
>>       // below /datastore/{store}" helper
>> -    let mut iter =
>> -        if let Ok(iter) = store.recursive_iter_backup_ns_ok(BackupNamespace::root(), None) {
>> -            iter
>> -        } else {
>> -            return false;
>> -        };
>> +    let mut iter = if let Ok(iter) = store.recursive_iter_backup_ns_ok(ns, max_depth) {
>> +        iter
>> +    } else {
>> +        return false;
>> +    };
> 
> This could use let-else, e.g. something like (untested):
> 
> let Ok(mut iter) = store.recursive_iter_backup_ns_ok(ns, max_depth) else {
>      return false;
> };
> 

can do, i wanted to stay as close as the original code as possible so
it's easier to see that the code only moved

should i do that cleanup as an extra commit or should i include it in 
this one?

> 
>>       let wanted =
>>           PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_MODIFY | PRIV_DATASTORE_READ | PRIV_DATASTORE_BACKUP;
>>       let name = store.name();
>> @@ -90,6 +94,15 @@ pub fn can_access_any_namespace(
>>       })
>>   }
>>   
>> +/// Checks if the given user has read/access rights on any namespace on given datastore
>> +pub fn can_access_any_namespace(
>> +    store: Arc<DataStore>,
>> +    auth_id: &Authid,
>> +    user_info: &CachedUserInfo,
>> +) -> bool {
>> +    can_access_any_namespace_in_range(store, auth_id, user_info, None, None)
>> +}
>> +
>>   /// A privilege aware iterator for all backup groups in all Namespaces below an anchor namespace,
>>   /// most often that will be the `BackupNamespace::root()` one.
>>   ///
> 



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