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

[Dietmar Maurer <dietmar@proxmox.com>]

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