[pbs-devel] [PATCH proxmox-backup 1/2] fix #2847: api: datastore: change backup owner

[pbs-devel] [PATCH proxmox-backup 1/2] fix #2847: api: datastore: change backup owner

[Dylan Whyte]

This adds an api method to change the owner of
a backup-group.

Signed-off-by: Dylan Whyte <d.whyte@proxmox.com>
---
 src/api2/admin/datastore.rs | 56 +++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index c260b62d..f4c4e2de 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1492,6 +1492,57 @@ fn set_notes(
     Ok(())
 }
 
+#[api(
+   input: {
+        properties: {
+            store: {
+                schema: DATASTORE_SCHEMA,
+            },
+            group: {
+                description: "Backup group.",
+            },
+            "new-owner": {
+                description: "Userid of new owner.",
+            },
+        },
+   },
+   access: {
+       permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_MODIFY, true),
+   },
+)]
+/// Change owner of a backup group
+fn set_backup_owner(
+    store: String,
+    group: String,
+    new_owner: String,
+    rpcenv: &mut dyn RpcEnvironment,
+) -> Result<(), Error> {
+
+    let datastore = DataStore::lookup_datastore(&store)?;
+
+    // user requesting change of owner
+    let userid: Userid = rpcenv.get_user().unwrap().parse()?;
+    let user_info = CachedUserInfo::new()?;
+    let user_privs = user_info.lookup_privs(&userid, &["datastore", &store]);
+
+    let backup_group: BackupGroup = group.parse()?;
+
+    let new_owner: Userid = new_owner.parse()?;
+    let new_owner_info = CachedUserInfo::new()?;
+
+    if new_owner_info.is_active_user(&new_owner) {
+        let allowed = (user_privs & PRIV_DATASTORE_MODIFY) != 0;
+        if !allowed { check_backup_owner(&datastore, &backup_group, &userid)?; }
+
+        datastore.set_owner(&backup_group, &new_owner, true)?;
+
+    } else {
+        bail!("user {} is inactive or non-existent", new_owner);
+    }
+
+    Ok(())
+}
+
 #[sortable]
 const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
     (
@@ -1499,6 +1550,11 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
         &Router::new()
             .get(&API_METHOD_CATALOG)
     ),
+    (
+        "change-owner",
+        &Router::new()
+            .post(&API_METHOD_SET_BACKUP_OWNER)
+    ),
     (
         "download",
         &Router::new()
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/2] fix #2847: proxmox-backup-client: add change-owner cmd

[Dylan Whyte]

This adds a change-owner command to proxmox-backup-client,
that allows a caller with datastore modify privileges
to change the owner of a backup-group.

Signed-off-by: Dylan Whyte <d.whyte@proxmox.com>
---
 src/bin/proxmox-backup-client.rs | 43 +++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs
index 97398f49..90cb8378 100644
--- a/src/bin/proxmox-backup-client.rs
+++ b/src/bin/proxmox-backup-client.rs
@@ -412,6 +412,41 @@ async fn list_backup_groups(param: Value) -> Result<Value, Error> {
     Ok(Value::Null)
 }
 
+#[api(
+   input: {
+        properties: {
+            repository: {
+                schema: REPO_URL_SCHEMA,
+                optional: true,
+            },
+            group: {
+                type: String,
+                description: "Backup group.",
+            },
+            "new-owner": {
+                type: String,
+                description: "Userid to transfer ownership to",
+            },
+        }
+   }
+)]
+/// Change owner of a backup group
+async fn change_backup_owner(mut param: Value) -> Result<(), Error> {
+
+    let repo = extract_repository_from_value(&param)?;
+
+    let mut client = connect(repo.host(), repo.port(), repo.user())?;
+
+    param.as_object_mut().unwrap().remove("repository");
+
+    let path = format!("api2/json/admin/datastore/{}/change-owner", repo.store());
+    client.post(&path, Some(param)).await?;
+
+    record_repository(&repo);
+
+    Ok(())
+}
+
 #[api(
    input: {
         properties: {
@@ -1967,6 +2002,11 @@ fn main() {
     let version_cmd_def = CliCommand::new(&API_METHOD_API_VERSION)
         .completion_cb("repository", complete_repository);
 
+    let change_owner_cmd_def = CliCommand::new(&API_METHOD_CHANGE_BACKUP_OWNER)
+        .arg_param(&["group", "new-owner"])
+        .completion_cb("group", complete_backup_group)
+        .completion_cb("repository", complete_repository);
+
     let cmd_def = CliCommandMap::new()
         .insert("backup", backup_cmd_def)
         .insert("upload-log", upload_log_cmd_def)
@@ -1987,7 +2027,8 @@ fn main() {
         .insert("catalog", catalog_mgmt_cli())
         .insert("task", task_mgmt_cli())
         .insert("version", version_cmd_def)
-        .insert("benchmark", benchmark_cmd_def);
+        .insert("benchmark", benchmark_cmd_def)
+        .insert("change-owner", change_owner_cmd_def);
 
     let rpcenv = CliEnvironment::new();
     run_cli_command(cmd_def, rpcenv, Some(|future| {
-- 
2.20.1

Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix #2847: api: datastore: change backup owner

[Dietmar Maurer]

applied a modified version, see comments inline:

> On 10/13/2020 10:58 AM Dylan Whyte <d.whyte@proxmox.com> wrote:
> 
>  
> This adds an api method to change the owner of
> a backup-group.
> 
> Signed-off-by: Dylan Whyte <d.whyte@proxmox.com>
> ---
>  src/api2/admin/datastore.rs | 56 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index c260b62d..f4c4e2de 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -1492,6 +1492,57 @@ fn set_notes(
>      Ok(())
>  }
>  
> +#[api(
> +   input: {
> +        properties: {
> +            store: {
> +                schema: DATASTORE_SCHEMA,
> +            },
> +            group: {
> +                description: "Backup group.",
> +            },

All others method in this api path uses "backup-type" and "backup-id", so I prefer to use
that here too.

> +            "new-owner": {
> +                description: "Userid of new owner.",
> +            },

Using "String" as type is much too generic. This should be:

            "new-owner": {
                type: Userid,
            },


> +        },
> +   },
> +   access: {
> +       permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_MODIFY, true),
> +   },
> +)]
> +/// Change owner of a backup group
> +fn set_backup_owner(
> +    store: String,
> +    group: String,
> +    new_owner: String,
> +    rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<(), Error> {
> +
> +    let datastore = DataStore::lookup_datastore(&store)?;
> +
> +    // user requesting change of owner
> +    let userid: Userid = rpcenv.get_user().unwrap().parse()?;
> +    let user_info = CachedUserInfo::new()?;
> +    let user_privs = user_info.lookup_privs(&userid, &["datastore", &store]);
> +
> +    let backup_group: BackupGroup = group.parse()?;
> +
> +    let new_owner: Userid = new_owner.parse()?;
> +    let new_owner_info = CachedUserInfo::new()?;

There is no need to get CachedUserInfo::new() twice!

> +
> +    if new_owner_info.is_active_user(&new_owner) {
> +        let allowed = (user_privs & PRIV_DATASTORE_MODIFY) != 0;
> +        if !allowed { check_backup_owner(&datastore, &backup_group, &userid)?; }

Also, this check is redundant, because the rest server already verifies the "access" permissions.
I removed that for now. Fabian will extend this when he add the api token patches.

> +
> +        datastore.set_owner(&backup_group, &new_owner, true)?;
> +
> +    } else {
> +        bail!("user {} is inactive or non-existent", new_owner);
> +    }
> +
> +    Ok(())
> +}
> +
>  #[sortable]
>  const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
>      (
> @@ -1499,6 +1550,11 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
>          &Router::new()
>              .get(&API_METHOD_CATALOG)
>      ),
> +    (
> +        "change-owner",
> +        &Router::new()
> +            .post(&API_METHOD_SET_BACKUP_OWNER)
> +    ),
>      (
>          "download",
>          &Router::new()
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [PATCH proxmox-backup 2/2] fix #2847: proxmox-backup-client: add change-owner cmd

[Dietmar Maurer]

applied, adopted to previous api changes

> On 10/13/2020 10:58 AM Dylan Whyte <d.whyte@proxmox.com> wrote:
> 
>  
> This adds a change-owner command to proxmox-backup-client,
> that allows a caller with datastore modify privileges
> to change the owner of a backup-group.
> 
> Signed-off-by: Dylan Whyte <d.whyte@proxmox.com>
> ---
>  src/bin/proxmox-backup-client.rs | 43 +++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs
> index 97398f49..90cb8378 100644
> --- a/src/bin/proxmox-backup-client.rs
> +++ b/src/bin/proxmox-backup-client.rs
> @@ -412,6 +412,41 @@ async fn list_backup_groups(param: Value) -> Result<Value, Error> {
>      Ok(Value::Null)
>  }
>  
> +#[api(
> +   input: {
> +        properties: {
> +            repository: {
> +                schema: REPO_URL_SCHEMA,
> +                optional: true,
> +            },
> +            group: {
> +                type: String,
> +                description: "Backup group.",
> +            },
> +            "new-owner": {
> +                type: String,

 type: Userid

> +                description: "Userid to transfer ownership to",
> +            },
> +        }
> +   }
> +)]
> +/// Change owner of a backup group
> +async fn change_backup_owner(mut param: Value) -> Result<(), Error> {
> +
> +    let repo = extract_repository_from_value(&param)?;
> +
> +    let mut client = connect(repo.host(), repo.port(), repo.user())?;
> +
> +    param.as_object_mut().unwrap().remove("repository");
> +
> +    let path = format!("api2/json/admin/datastore/{}/change-owner", repo.store());
> +    client.post(&path, Some(param)).await?;
> +
> +    record_repository(&repo);
> +
> +    Ok(())
> +}
> +
>  #[api(
>     input: {
>          properties: {
> @@ -1967,6 +2002,11 @@ fn main() {
>      let version_cmd_def = CliCommand::new(&API_METHOD_API_VERSION)
>          .completion_cb("repository", complete_repository);
>  
> +    let change_owner_cmd_def = CliCommand::new(&API_METHOD_CHANGE_BACKUP_OWNER)
> +        .arg_param(&["group", "new-owner"])
> +        .completion_cb("group", complete_backup_group)
 
added:  .completion_cb("new-owner",  complete_user_name)
 
> +        .completion_cb("repository", complete_repository);
> +
>      let cmd_def = CliCommandMap::new()
>          .insert("backup", backup_cmd_def)
>          .insert("upload-log", upload_log_cmd_def)
> @@ -1987,7 +2027,8 @@ fn main() {
>          .insert("catalog", catalog_mgmt_cli())
>          .insert("task", task_mgmt_cli())
>          .insert("version", version_cmd_def)
> -        .insert("benchmark", benchmark_cmd_def);
> +        .insert("benchmark", benchmark_cmd_def)
> +        .insert("change-owner", change_owner_cmd_def);
>  
>      let rpcenv = CliEnvironment::new();
>      run_cli_command(cmd_def, rpcenv, Some(|future| {
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel