Re: [pve-devel] [PATCH access-control v4 2/6] added acls for Shared Files Directories

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On April 25, 2023 12:21 pm, Markus Frank wrote:
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
>  src/PVE/API2/Directory.pm | 68 +++++++++++++++++++++++++++++++++++++++

this parts seems to be included by accident? ;)

>  src/PVE/AccessControl.pm  | 16 +++++++++
>  src/PVE/RPCEnvironment.pm | 12 ++++++-
>  3 files changed, 95 insertions(+), 1 deletion(-)
>  create mode 100644 src/PVE/API2/Directory.pm
> 
> diff --git a/src/PVE/API2/Directory.pm b/src/PVE/API2/Directory.pm
> new file mode 100644
> index 0000000..b44ba9d
> --- /dev/null
> +++ b/src/PVE/API2/Directory.pm
> @@ -0,0 +1,68 @@
> +package PVE::API2::Directory;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::Exception qw(raise raise_perm_exc raise_param_exc);
> +use PVE::Cluster qw (cfs_read_file cfs_write_file);
> +use PVE::Tools qw(split_list extract_param);
> +use PVE::JSONSchema qw(get_standard_option register_standard_option);
> +use PVE::SafeSyslog;
> +
> +use PVE::AccessControl;
> +use PVE::Auth::Plugin;
> +use PVE::TokenConfig;
> +
> +use PVE::RESTHandler;
> +use PVE::DirConfig;
> +
> +use base qw(PVE::RESTHandler);
> +
> +__PACKAGE__->register_method ({
> +    name => 'index',
> +    path => '',
> +    method => 'GET',
> +    description => "simple return value of parameter 'text'",
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    node => {
> +		type => 'string',
> +	    }
> +	},
> +    },
> +    returns => {
> +	type => 'string',
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +	return $param->{node};
> +    }
> +});
> +
> +
> +__PACKAGE__->register_method ({
> +    name => 'add_dir',
> +    path => 'add',
> +    method => 'POST',
> +    description => "simple return value of parameter 'text'",
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => {
> +	    directory => {
> +		type => 'string',
> +	    },
> +	    node => {
> +		type => 'string',
> +	    }
> +	},
> +    },
> +    returns => {
> +	type => 'string',
> +    },
> +    code => sub {
> +	my ($param) = @_;
> +
> +	return $param->{node};
> +    }
> +});
> diff --git a/src/PVE/AccessControl.pm b/src/PVE/AccessControl.pm
> index 5690a1f..6530753 100644
> --- a/src/PVE/AccessControl.pm
> +++ b/src/PVE/AccessControl.pm
> @@ -1133,6 +1133,18 @@ my $privgroups = {
>  	    'Pool.Audit',
>  	],
>      },
> +    Map => {
> +	root => [],
> +	admin => [
> +	    'Map.Modify',
> +	],
> +	user => [
> +	    'Map.Use',
> +	],
> +	audit => [
> +	    'Map.Audit',
> +	],
> +    },

the priv names need coordination with Dominik, we definitely don't want
two sets!

>  };
>  
>  my $valid_privs = {};
> @@ -1166,6 +1178,8 @@ sub create_roles {
>      }
>  
>      $special_roles->{"PVETemplateUser"} = { 'VM.Clone' => 1, 'VM.Audit' => 1 };
> +
> +    delete($special_roles->{"PVEAdmin"}->{'Map.Modify'});

this is something were Dominik had some ideas as well IIRC ;)

>  };
>  
>  create_roles();
> @@ -1262,6 +1276,8 @@ sub check_path {
>  	|/storage/[[:alnum:]\.\-\_]+
>  	|/vms
>  	|/vms/[1-9][0-9]{2,}
> +	|/map/dirs
> +	|/map/dirs/[[:alnum:]\.\-\_]+
>      )$!xs;
>  }
>  
> diff --git a/src/PVE/RPCEnvironment.pm b/src/PVE/RPCEnvironment.pm
> index 8586938..42ff287 100644
> --- a/src/PVE/RPCEnvironment.pm
> +++ b/src/PVE/RPCEnvironment.pm
> @@ -187,10 +187,11 @@ sub compute_api_permission {
>  	nodes => qr/Sys\.|Permissions\.Modify/,
>  	sdn => qr/SDN\.|Permissions\.Modify/,
>  	dc => qr/Sys\.Audit|SDN\./,
> +	map => qr/Map\.Modify/

why Modify? I think Map. and Permissions.Modify would make more sense?

>      };
>      map { $res->{$_} = {} } keys %$priv_re_map;
>  
> -    my $required_paths = ['/', '/nodes', '/access/groups', '/vms', '/storage', '/sdn'];
> +    my $required_paths = ['/', '/nodes', '/access/groups', '/vms', '/storage', '/sdn', '/map'];
>      my $defined_paths = [];
>      PVE::AccessControl::iterate_acl_tree("/", $usercfg->{acl_root}, sub {
>  	my ($path, $node) = @_;
> @@ -245,6 +246,7 @@ sub get_effective_permissions {
>  	'/sdn' => 1,
>  	'/storage' => 1,
>  	'/vms' => 1,
> +	'/map' => 1,
>      };
>  
>      my $cfg = $self->{user_cfg};
> @@ -361,6 +363,14 @@ sub check_vm_perm {
>      return $self->check_full($user, "/vms/$vmid", $privs, $any, $noerr);
>  };
>  
> +sub check_dir_perm {
> +    my ($self, $user, $dirid, $privs, $any, $noerr) = @_;
> +
> +    my $cfg = $self->{user_cfg};
> +
> +    return $self->check_full($user, "/map/dirs/$dirid", $privs, $any, $noerr);
> +};

I don't think this helper brings anything to the table? check_vm_perm is
special in that it handles pools, and we want that in a single place,
but this is just hardcoding the ACL prefix?

> +
>  sub is_group_member {
>      my ($self, $group, $user) = @_;
>  
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
>