[pve-devel] [PATCH access-control v2 1/3] fix #1501: pveum: die when deleting special role

[Thomas Lamprecht]

On 09/20/2017 05:10 PM, Philip Abernethy wrote:
> Die with a helpful error message instead of silently ignoring the user
> when trying to delete a special role.
> ---
> v2: Fixed 'special' appearing in WebUI roles list
>   PVE/API2/Role.pm     | 7 ++++++-
>   PVE/AccessControl.pm | 1 +
>   2 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Role.pm b/PVE/API2/Role.pm
> index 6392e13..d6d17db 100644
> --- a/PVE/API2/Role.pm
> +++ b/PVE/API2/Role.pm
> @@ -43,8 +43,10 @@ __PACKAGE__->register_method ({
>   	my $usercfg = cfs_read_file("user.cfg");
>    
>   	foreach my $role (keys %{$usercfg->{roles}}) {
> +	    my $special = $usercfg->{roles}->{$role}->{special};
> +	    delete $usercfg->{roles}->{$role}->{special};

Just FYI, the following one liner would have the same effect:

my $special = delete $usercfg->{roles}->{$role}->{special};

>   	    my $privs = join(',', sort keys %{$usercfg->{roles}->{$role}});
> -	    push @$res, { roleid => $role, privs => $privs };
> +	    push @$res, { roleid => $role, privs => $privs, special => $special };
>   	}
>   
>   	return $res;
> @@ -195,6 +197,9 @@ __PACKAGE__->register_method ({
>   		die "role '$role' does not exist\n"
>   		    if !$usercfg->{roles}->{$role};
>   	
> +		die "role '$role' can not be deleted\n"

Telling the user why this is the case would be great, IMO.
E.g. something like:

die "auto-generated role '$role' can not be deleted\n";


> +		    if ($usercfg->{roles}->{$role}->{special});
> +
>   		delete ($usercfg->{roles}->{$role});
>   
>   		# fixme: delete role from acl?
> diff --git a/PVE/AccessControl.pm b/PVE/AccessControl.pm
> index 7d02cdf..b6be95d 100644
> --- a/PVE/AccessControl.pm
> +++ b/PVE/AccessControl.pm
> @@ -595,6 +595,7 @@ sub userconfig_force_defaults {
>   
>       foreach my $r (keys %$special_roles) {
>   	$cfg->{roles}->{$r} = $special_roles->{$r};
> +	$cfg->{roles}->{$r}->{special} = 1;

Hmm, here a internal marker boolean gets saved together with privileges
on the same level. As our privileges normally follow the OBJ.RIGHT it may
be free of conflict for now, but this could cause possible conflict or pain
in the future (I made the same mistake once...). '_special_' could be better
but still not ideal, IMO.

Also here is not the special role creation point, so there could be code
paths where this information does not gets set?

Alternative proposal: How about not saving anything at all here?
Instead we could add a 'is_special_role' helper method here which takes a
role name as parameter and if the special roles where already created returns
if said param is in the special role hash?
The 'special' property from the `GET /access/roles' API call above could be set
in a similar same manner.

cheers,
Thomas

>       }
>   
>       # add root user if not exists
>