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

[Thomas Lamprecht]

On 09/21/2017 11:09 AM, Philip Abernethy wrote:
> Die with a helpful error message instead of silently ignoring the user
> when trying to delete a special role.
> Also add a property to the API answer for possible later use by the
> WebUI.
> ---
>   PVE/API2/Role.pm     | 6 +++++-
>   PVE/AccessControl.pm | 5 +++++
>   2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Role.pm b/PVE/API2/Role.pm
> index 6392e13..0216c8d 100644
> --- a/PVE/API2/Role.pm
> +++ b/PVE/API2/Role.pm
> @@ -44,7 +44,8 @@ __PACKAGE__->register_method ({
>    
>   	foreach my $role (keys %{$usercfg->{roles}}) {
>   	    my $privs = join(',', sort keys %{$usercfg->{roles}->{$role}});
> -	    push @$res, { roleid => $role, privs => $privs };
> +	    push @$res, { roleid => $role, privs => $privs,
> +		special => PVE::AccessControl::role_is_special($role) };
>   	}
>   
>   	return $res;
> @@ -195,6 +196,9 @@ __PACKAGE__->register_method ({
>   		die "role '$role' does not exist\n"
>   		    if !$usercfg->{roles}->{$role};
>   	
> +		die "auto-generated role '$role' can not be deleted\n"
> +		    if PVE::AccessControl::role_is_special($role);
> +
>   		delete ($usercfg->{roles}->{$role});
>   
>   		# fixme: delete role from acl?
> diff --git a/PVE/AccessControl.pm b/PVE/AccessControl.pm
> index 7d02cdf..98e2fd6 100644
> --- a/PVE/AccessControl.pm
> +++ b/PVE/AccessControl.pm
> @@ -502,6 +502,11 @@ sub create_roles {
>   
>   create_roles();
>   
> +sub role_is_special {
> +    my ($role) = @_;
> +    return exists $special_roles->{$role};

nit: we normally use defined for this, it has stronger guarantees as the
value the hash key points to must not be undef for defined() to evaluate
to true. But besides code consistence this is not an issue, at least for me.

So, whole series:
Reviewed-by: Thomas Lamprecht <t.lamprecht at proxmox.com>
(and tested for that matter)

I minimally played around in pve-manager adding a (default hidden) column
for this and marking the rows in a disabled style (as said, minimally played
around could be done better):

----8<----
diff --git a/www/css/ext6-pve.css b/www/css/ext6-pve.css
index 722a4d94..fd544dd3 100644
--- a/www/css/ext6-pve.css
+++ b/www/css/ext6-pve.css
@@ -533,6 +533,11 @@ table.osds td:first-of-type {
      background-color: #f3d6d7;
  }

+.pve-noteditable-row {
+    background-color: #EEE;
+}
+
+
  .pve-static-mask div.x-mask-msg-text {
      padding: 10px;
      background-image: none;
diff --git a/www/manager6/dc/RoleView.js b/www/manager6/dc/RoleView.js
index 63a599f4..cf060b17 100644
--- a/www/manager6/dc/RoleView.js
+++ b/www/manager6/dc/RoleView.js
@@ -37,7 +37,10 @@ Ext.define('PVE.dc.RoleView', {
             store: store,

             viewConfig: {
-               trackOver: false
+               trackOver: false,
+               getRowClass: function(record, rowIndex, rowParams, store) {
+                   return record.get("special") ? 'pve-noteditable-row' :'';
+               }
             },
             columns: [
                 {
@@ -53,6 +56,13 @@ Ext.define('PVE.dc.RoleView', {
                     renderer: render_privs,
                     dataIndex: 'privs',
                     flex: 1
+               },
+               {
+                   header: gettext('Special'),
+                   sortable: true,
+                   renderer: PVE.Utils.format_boolean,
+                   hidden: true,
+                   dataIndex: 'special'
                 }
             ],
             listeners: {

---->8----

IMO, we could always add the hidden column.
How we differ between special and user configured gets only important once
we add role add/delete/modify functionality in the webUI anyway.

> +}
> +
>   sub add_role_privs {
>       my ($role, $usercfg, $privs) = @_;
>   
>