[pbs-devel] [PATCH proxmox-backup 1/2] pull: fix permission checks for local syncs

[pbs-devel] [PATCH proxmox-backup 1/2] pull: fix permission checks for local syncs

[Hannes Laimer]

Reported-by: Dominik Csapak <d.csapak@proxmox.com>
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 src/api2/config/sync.rs | 10 ++++------
 src/server/pull.rs      |  6 +++---
 2 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
index ea0e08f1..8809465c 100644
--- a/src/api2/config/sync.rs
+++ b/src/api2/config/sync.rs
@@ -8,8 +8,8 @@ use proxmox_schema::{api, param_bail};
 
 use pbs_api_types::{
     Authid, SyncJobConfig, SyncJobConfigUpdater, JOB_ID_SCHEMA, PRIV_DATASTORE_AUDIT,
-    PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ,
-    PRIV_REMOTE_AUDIT, PRIV_REMOTE_READ, PROXMOX_CONFIG_DIGEST_SCHEMA,
+    PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_REMOTE_AUDIT,
+    PRIV_REMOTE_READ, PROXMOX_CONFIG_DIGEST_SCHEMA,
 };
 use pbs_config::sync;
 
@@ -70,11 +70,9 @@ pub fn check_sync_job_modify_access(
 
     if let Some(remote) = &job.remote {
         let remote_privs = user_info.lookup_privs(auth_id, &["remote", remote, &job.remote_store]);
-        remote_privs & PRIV_REMOTE_READ != 0
-    } else {
-        let source_ds_privs = user_info.lookup_privs(auth_id, &["datastore", &job.remote_store]);
-        source_ds_privs & PRIV_DATASTORE_READ != 0
+        return remote_privs & PRIV_REMOTE_READ != 0;
     }
+    true
 }
 
 #[api(
diff --git a/src/server/pull.rs b/src/server/pull.rs
index 1403c7a7..66ef333a 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -17,7 +17,7 @@ use serde_json::json;
 use pbs_api_types::{
     print_store_and_ns, Authid, BackupDir, BackupGroup, BackupNamespace, CryptMode, GroupFilter,
     GroupListItem, Operation, RateLimitConfig, Remote, SnapshotListItem, MAX_NAMESPACE_DEPTH,
-    PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
+    PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_READ,
 };
 use pbs_client::{BackupReader, BackupRepository, HttpClient, RemoteChunkReader};
 use pbs_config::CachedUserInfo;
@@ -271,8 +271,8 @@ impl PullSource for LocalSource {
             &self.store,
             namespace.clone(),
             0,
-            None,
-            None,
+            Some(PRIV_DATASTORE_READ),
+            Some(PRIV_DATASTORE_BACKUP),
             Some(owner),
         )?
         .filter_map(Result::ok)
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 2/2] ui: fix changing remote to local sync job

[Hannes Laimer]

Reported-by: Dominik Csapak <d.csapak@proxmox.com>
Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
---
 www/window/SyncJobEdit.js | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
index d20def74..58c8fb5c 100644
--- a/www/window/SyncJobEdit.js
+++ b/www/window/SyncJobEdit.js
@@ -73,6 +73,7 @@ Ext.define('PBS.window.SyncJobEdit', {
 		    }
 		    if (!me.isCreate) {
 			PBS.Utils.delete_if_default(values, 'rate-in');
+			PBS.Utils.delete_if_default(values, 'remote');
 			if (typeof values.delete === 'string') {
 			    values.delete = values.delete.split(',');
 			}
@@ -200,9 +201,6 @@ Ext.define('PBS.window.SyncJobEdit', {
 			xtype: 'pbsRemoteSelector',
 			allowBlank: false,
 			name: 'remote',
-			cbind: {
-			    deleteEmpty: '{!isCreate}',
-			},
 			skipEmptyText: true,
 			listeners: {
 			    change: function(f, value) {
-- 
2.39.2

Re: [pbs-devel] [PATCH proxmox-backup 1/2] pull: fix permission checks for local syncs

[Dominik Csapak]

with that i can now sync snapshots that can be read from the local user
code seems fine to me but i'm not sure about the PRIV_DATASTORE_READ and
PRIV_DATASTORE_BACKUP use in the last hunk (i don't have a deep understanding
of the code but i couldn't find an issue by testing), so it's only:

Tested-by: Dominik Csapak <d.csapak@proxmox.com>

On 11/28/23 15:16, Hannes Laimer wrote:
> Reported-by: Dominik Csapak <d.csapak@proxmox.com>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   src/api2/config/sync.rs | 10 ++++------
>   src/server/pull.rs      |  6 +++---
>   2 files changed, 7 insertions(+), 9 deletions(-)
> 
> diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
> index ea0e08f1..8809465c 100644
> --- a/src/api2/config/sync.rs
> +++ b/src/api2/config/sync.rs
> @@ -8,8 +8,8 @@ use proxmox_schema::{api, param_bail};
>   
>   use pbs_api_types::{
>       Authid, SyncJobConfig, SyncJobConfigUpdater, JOB_ID_SCHEMA, PRIV_DATASTORE_AUDIT,
> -    PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_DATASTORE_READ,
> -    PRIV_REMOTE_AUDIT, PRIV_REMOTE_READ, PROXMOX_CONFIG_DIGEST_SCHEMA,
> +    PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE, PRIV_REMOTE_AUDIT,
> +    PRIV_REMOTE_READ, PROXMOX_CONFIG_DIGEST_SCHEMA,
>   };
>   use pbs_config::sync;
>   
> @@ -70,11 +70,9 @@ pub fn check_sync_job_modify_access(
>   
>       if let Some(remote) = &job.remote {
>           let remote_privs = user_info.lookup_privs(auth_id, &["remote", remote, &job.remote_store]);
> -        remote_privs & PRIV_REMOTE_READ != 0
> -    } else {
> -        let source_ds_privs = user_info.lookup_privs(auth_id, &["datastore", &job.remote_store]);
> -        source_ds_privs & PRIV_DATASTORE_READ != 0
> +        return remote_privs & PRIV_REMOTE_READ != 0;
>       }
> +    true
>   }
>   
>   #[api(
> diff --git a/src/server/pull.rs b/src/server/pull.rs
> index 1403c7a7..66ef333a 100644
> --- a/src/server/pull.rs
> +++ b/src/server/pull.rs
> @@ -17,7 +17,7 @@ use serde_json::json;
>   use pbs_api_types::{
>       print_store_and_ns, Authid, BackupDir, BackupGroup, BackupNamespace, CryptMode, GroupFilter,
>       GroupListItem, Operation, RateLimitConfig, Remote, SnapshotListItem, MAX_NAMESPACE_DEPTH,
> -    PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
> +    PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP, PRIV_DATASTORE_READ,
>   };
>   use pbs_client::{BackupReader, BackupRepository, HttpClient, RemoteChunkReader};
>   use pbs_config::CachedUserInfo;
> @@ -271,8 +271,8 @@ impl PullSource for LocalSource {
>               &self.store,
>               namespace.clone(),
>               0,
> -            None,
> -            None,
> +            Some(PRIV_DATASTORE_READ),
> +            Some(PRIV_DATASTORE_BACKUP),
>               Some(owner),
>           )?
>           .filter_map(Result::ok)

Re: [pbs-devel] [PATCH proxmox-backup 2/2] ui: fix changing remote to local sync job

[Dominik Csapak]

Can now convert a remote sync job to a local sync job

Tested-by: Dominik Csapak <d.csapak@proxmox.com>
Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>

On 11/28/23 15:16, Hannes Laimer wrote:
> Reported-by: Dominik Csapak <d.csapak@proxmox.com>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   www/window/SyncJobEdit.js | 4 +---
>   1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
> index d20def74..58c8fb5c 100644
> --- a/www/window/SyncJobEdit.js
> +++ b/www/window/SyncJobEdit.js
> @@ -73,6 +73,7 @@ Ext.define('PBS.window.SyncJobEdit', {
>   		    }
>   		    if (!me.isCreate) {
>   			PBS.Utils.delete_if_default(values, 'rate-in');
> +			PBS.Utils.delete_if_default(values, 'remote');
>   			if (typeof values.delete === 'string') {
>   			    values.delete = values.delete.split(',');
>   			}
> @@ -200,9 +201,6 @@ Ext.define('PBS.window.SyncJobEdit', {
>   			xtype: 'pbsRemoteSelector',
>   			allowBlank: false,
>   			name: 'remote',
> -			cbind: {
> -			    deleteEmpty: '{!isCreate}',
> -			},
>   			skipEmptyText: true,
>   			listeners: {
>   			    change: function(f, value) {

[pbs-devel] applied: [PATCH proxmox-backup 1/2] pull: fix permission checks for local syncs

[Thomas Lamprecht]

Am 28/11/2023 um 15:16 schrieb Hannes Laimer:
> Reported-by: Dominik Csapak <d.csapak@proxmox.com>
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>  src/api2/config/sync.rs | 10 ++++------
>  src/server/pull.rs      |  6 +++---
>  2 files changed, 7 insertions(+), 9 deletions(-)
> 
>

applied series, with Dominik's R-b and T-b, thanks!