Re: [PATCH proxmox-backup 14/28] sync: pull: allow pulling for user/token with append permission

[Christian Ebner <c.ebner@proxmox.com>]

On 9/9/26 2:31 PM, Robert Obkircher wrote:
>> Extends the access checks for syncs in pull direction to allow
>> appending contents when user has Datastore.Append permissions.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>>
>> diff --git a/src/api2/pull.rs b/src/api2/pull.rs
>> index 17c1e4f34..6b53c55b7 100644
>> --- a/src/api2/pull.rs
>> +++ b/src/api2/pull.rs
>> @@ -8,9 +8,9 @@ use proxmox_schema::api;
>>   
>>   use pbs_api_types::{
>>       Authid, BackupNamespace, CRYPT_KEY_ID_SCHEMA, DATASTORE_SCHEMA, GROUP_FILTER_LIST_SCHEMA,
>> -    GroupFilter, NS_MAX_DEPTH_REDUCED_SCHEMA, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_PRUNE,
>> -    PRIV_REMOTE_READ, REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA, RESYNC_CORRUPT_SCHEMA,
>> -    RateLimitConfig, SYNC_ENCRYPTED_ONLY_SCHEMA, SYNC_VERIFIED_ONLY_SCHEMA,
>> +    GroupFilter, NS_MAX_DEPTH_REDUCED_SCHEMA, PRIV_DATASTORE_APPEND, PRIV_DATASTORE_BACKUP,
>> +    PRIV_DATASTORE_PRUNE, PRIV_REMOTE_READ, REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA,
>> +    RESYNC_CORRUPT_SCHEMA, RateLimitConfig, SYNC_ENCRYPTED_ONLY_SCHEMA, SYNC_VERIFIED_ONLY_SCHEMA,
>>       SYNC_WORKER_THREADS_SCHEMA, SyncJobConfig, TRANSFER_LAST_SCHEMA,
>>   };
>>   use pbs_config::CachedUserInfo;
>> @@ -36,8 +36,8 @@ pub fn check_pull_privs(
>>       user_info.check_privs(
>>           auth_id,
>>           &local_store_ns_acl_path,
>> -        PRIV_DATASTORE_BACKUP,
>> -        false,
>> +        PRIV_DATASTORE_APPEND | PRIV_DATASTORE_BACKUP,
>> +        true,
>>       )?;
>>   
>>       if let Some(remote) = remote {
>> diff --git a/src/server/pull.rs b/src/server/pull.rs
>> index 8c6253604..856e0fc5a 100644
>> --- a/src/server/pull.rs
>> +++ b/src/server/pull.rs
>> @@ -1442,13 +1442,16 @@ fn check_and_create_ns(params: &PullParameters, ns: &BackupNamespace) -> Result<
>>           created = true;
>>       }
>>   
>> -    check_ns_privs(
>> -        params.target.store.name(),
>> -        ns,
>> -        &params.owner,
>> -        PRIV_DATASTORE_BACKUP | PRIV_DATASTORE_APPEND,
>> -    )
>> -    .map_err(|err| format_err!("sync into {store_ns_str} not allowed - {err}"))?;
>> +    let privs = if params.resync_corrupt {
>> +        PRIV_DATASTORE_BACKUP
> Should we really skip the entire namespace if we only have append
> permission? We could still pull the snapshots that aren't corrupt.

Yeah, pulling new snapshots which are not corrupt might be more 
intuitive here. I would however argue that setting the resync corrupt 
while having only append privs is a misconfiguration. Will see to extend 
this and prominently log the lack of privs.