[pve-devel] [PATCH access-control v3 1/2] refactor API by unifying duplicate properties

Stoiko Ivanov s.ivanov at proxmox.com
Tue Jun 12 12:41:48 CEST 2018


* The JSONSchema for most API calls share some properties, which this
 refactoring pulls out in a common_$type_properties hashref in order to
 minimize code duplication (All parameters, which are thus added to the API
 calls were optional)
* Additionally for some fields the title property is added (its not used
  elswhere) - as preparation for unified CLI handling
* PVE/AccessControl::role_is_special now return 0 instead of '' for false
  (Schemavalidation did complain about '')
* For 2 fields a new property (print_width) was added

Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
---
 PVE/API2/ACL.pm      | 40 ++++++++++++----------
 PVE/API2/Group.pm    | 38 +++++++++-----------
 PVE/API2/Role.pm     | 58 ++++++++++++++++---------------
 PVE/API2/User.pm     | 97 +++++++++++++++++++---------------------------------
 PVE/AccessControl.pm |  2 +-
 PVE/Auth/Plugin.pm   |  2 +-
 6 files changed, 107 insertions(+), 130 deletions(-)

diff --git a/PVE/API2/ACL.pm b/PVE/API2/ACL.pm
index d37771b..64d84e3 100644
--- a/PVE/API2/ACL.pm
+++ b/PVE/API2/ACL.pm
@@ -13,6 +13,20 @@ use PVE::RESTHandler;
 
 use base qw(PVE::RESTHandler);
 
+my $common_acl_properties = {
+    propagate => {
+	description => "Allow to propagate (inherit) permissions.",
+	type => 'boolean',
+	title => 'Propagate',
+	optional => 1,
+	default => 1,
+    },
+    path => {
+	description => "Access control path",
+	title => 'Path',
+	type => 'string',
+    },
+};
 __PACKAGE__->register_method ({
     name => 'read_acl',
     path => '',
@@ -31,13 +45,11 @@ __PACKAGE__->register_method ({
 	items => {
 	    type => "object",
 	    additionalProperties => 0,
-	    properties => {
-		path => { type => 'string' },
-		type => { type => 'string', enum => ['user', 'group'] },
-		ugid => { type => 'string' },
-		roleid => { type => 'string' },
-		propagate => { type => 'boolean' },
-	    },
+	    properties => { (%$common_acl_properties,
+		type => { type => 'string', title => 'Type', enum => ['user', 'group'] },
+		ugid => { type => 'string', title => 'ID'},
+		roleid => { type => 'string', title => 'Role' },
+	    ) },
 	},
     },
     code => sub {
@@ -89,11 +101,7 @@ __PACKAGE__->register_method ({
     description => "Update Access Control List (add or remove permissions).",
     parameters => {
 	additionalProperties => 0,
-	properties => {
-	    path => {
-		description => "Access control path",
-		type => 'string',
-	    },
+	properties => { (%$common_acl_properties,
 	    users => {
 		description => "List of users.",
 		type => 'string',  format => 'pve-userid-list',
@@ -108,18 +116,12 @@ __PACKAGE__->register_method ({
 		description => "List of roles.",
 		type => 'string', format => 'pve-roleid-list',
 	    },
-	    propagate => {
-		description => "Allow to propagate (inherit) permissions.",
-		type => 'boolean',
-		optional => 1,
-		default => 1,
-	    },
 	    delete => {
 		description => "Remove permissions (instead of adding it).",
 		type => 'boolean',
 		optional => 1,
 	    },
-	},
+	) },
     },
     returns => { type => 'null' },
     code => sub {
diff --git a/PVE/API2/Group.pm b/PVE/API2/Group.pm
index fca8a2a..d3c91c8 100644
--- a/PVE/API2/Group.pm
+++ b/PVE/API2/Group.pm
@@ -9,6 +9,16 @@ use PVE::RESTHandler;
 
 use base qw(PVE::RESTHandler);
 
+my $common_group_properties = {
+    groupid => {
+        type => 'string',
+        format => 'pve-groupid',
+        title => 'Group ID' ,
+        completion => \&PVE::AccessControl::complete_group,
+    },
+    comment => { type => 'string', optional => 1 },
+};
+
 __PACKAGE__->register_method ({
     name => 'index', 
     path => '', 
@@ -26,9 +36,7 @@ __PACKAGE__->register_method ({
 	type => 'array',
 	items => {
 	    type => "object",
-	    properties => {
-		groupid => { type => 'string' },
-	    },
+	    properties => $common_group_properties,
 	},
 	links => [ { rel => 'child', href => "{groupid}" } ],
     },
@@ -65,10 +73,7 @@ __PACKAGE__->register_method ({
     description => "Create new group.",
     parameters => {
    	additionalProperties => 0,
-	properties => {
-	    groupid => { type => 'string', format => 'pve-groupid' },
-	    comment => { type => 'string', optional => 1 },
-	},
+	properties => $common_group_properties,
     },
     returns => { type => 'null' },
     code => sub {
@@ -106,13 +111,7 @@ __PACKAGE__->register_method ({
     description => "Update group data.",
     parameters => {
    	additionalProperties => 0,
-	properties => {
-	    groupid => {
-		type => 'string', format => 'pve-groupid',
-		completion => \&PVE::AccessControl::complete_group,
-	    },
-	    comment => { type => 'string', optional => 1 },
-	},
+	properties => $common_group_properties,
     },
     returns => { type => 'null' },
     code => sub {
@@ -149,18 +148,18 @@ __PACKAGE__->register_method ({
     parameters => {
    	additionalProperties => 0,
 	properties => {
-	    groupid => { type => 'string', format => 'pve-groupid' },
+	    %$common_group_properties{'groupid'},
 	},
     },
     returns => {
 	type => "object",
 	additionalProperties => 0,
 	properties => {
-	    comment => { type => 'string', optional => 1 },
+	    comment => $common_group_properties->{'comment'},
 	    members => {
 		type => 'array',
 		items => {
-		    type => "string",
+		    type => 'string', format => 'pve-userid',
 		},
 	    },
 	},
@@ -198,10 +197,7 @@ __PACKAGE__->register_method ({
     parameters => {
    	additionalProperties => 0,
 	properties => {
-	    groupid => {
-		type => 'string' , format => 'pve-groupid',
-		completion => \&PVE::AccessControl::complete_group,
-	    },
+	    %$common_group_properties{'groupid'},
 	}
     },
     returns => { type => 'null' },
diff --git a/PVE/API2/Role.pm b/PVE/API2/Role.pm
index adbb4db..860c57c 100644
--- a/PVE/API2/Role.pm
+++ b/PVE/API2/Role.pm
@@ -11,6 +11,18 @@ use PVE::RESTHandler;
 
 use base qw(PVE::RESTHandler);
 
+my $common_role_properties = {
+    roleid => { type => 'string',
+	format => 'pve-roleid',
+	title => 'Role ID',
+	print_width => 30
+    },
+    privs => { type => 'string' ,
+	format => 'pve-priv-list',
+	optional => 1, title => 'Privileges',
+    },
+};
+
 __PACKAGE__->register_method ({
     name => 'index',
     path => '',
@@ -27,9 +39,9 @@ __PACKAGE__->register_method ({
 	type => 'array',
 	items => {
 	    type => "object",
-	    properties => {
-		roleid => { type => 'string' },
-	    },
+	    properties => { (%$common_role_properties,
+		special => { type => 'boolean', optional => 1, default => 0, title => 'Built-In' },
+	    ) },
 	},
 	links => [ { rel => 'child', href => "{roleid}" } ],
     },
@@ -63,10 +75,7 @@ __PACKAGE__->register_method ({
     description => "Create new role.",
     parameters => {
 	additionalProperties => 0,
-	properties => {
-	    roleid => { type => 'string', format => 'pve-roleid' },
-	    privs => { type => 'string' , format => 'pve-priv-list', optional => 1 },
-	},
+	properties => $common_role_properties,
     },
     returns => { type => 'null' },
     code => sub {
@@ -100,18 +109,12 @@ __PACKAGE__->register_method ({
     permissions => {
 	check => ['perm', '/access', ['Sys.Modify']],
     },
-    description => "Create new role.",
+    description => "Update an existing role.",
     parameters => {
 	additionalProperties => 0,
-	properties => {
-	    roleid => { type => 'string', format => 'pve-roleid' },
-	    privs => { type => 'string' , format => 'pve-priv-list' },
-	    append => {
-		type => 'boolean',
-		optional => 1,
-		requires => 'privs',
-	    },
-	},
+	properties => { (%$common_role_properties,
+	    append => { type => 'boolean', optional => 1, requires => 'privs' },
+	) },
     },
     returns => { type => 'null' },
     code => sub {
@@ -137,7 +140,6 @@ __PACKAGE__->register_method ({
 	return undef;
 }});
 
-# fixme: return format!
 __PACKAGE__->register_method ({
     name => 'read_role',
     path => '{roleid}',
@@ -148,11 +150,13 @@ __PACKAGE__->register_method ({
     description => "Get role configuration.",
     parameters => {
 	additionalProperties => 0,
-	properties => {
-	    roleid => { type => 'string' , format => 'pve-roleid' },
-	},
+	properties => { %$common_role_properties{'roleid'}, },
+    },
+    returns => {
+	type => "object",
+	additionalProperties => 0,
+	properties => { %$common_role_properties{qw(privs)} },
     },
-    returns => {},
     code => sub {
 	my ($param) = @_;
 
@@ -165,7 +169,8 @@ __PACKAGE__->register_method ({
 	die "role '$role' does not exist\n" if !$data;
 
 	return $data;
-}});
+    }
+});
 
 __PACKAGE__->register_method ({
     name => 'delete_role',
@@ -178,9 +183,7 @@ __PACKAGE__->register_method ({
     description => "Delete role.",
     parameters => {
 	additionalProperties => 0,
-	properties => {
-	    roleid => { type => 'string', format => 'pve-roleid' },
-	}
+	properties => { %$common_role_properties{'roleid'}, },
     },
     returns => { type => 'null' },
     code => sub {
@@ -206,6 +209,7 @@ __PACKAGE__->register_method ({
 	    }, "delete role failed");
 
 	return undef;
-}});
+    }
+});
 
 1;
diff --git a/PVE/API2/User.pm b/PVE/API2/User.pm
index 1dc0293..6449ed0 100644
--- a/PVE/API2/User.pm
+++ b/PVE/API2/User.pm
@@ -14,6 +14,34 @@ use PVE::RESTHandler;
 
 use base qw(PVE::RESTHandler);
 
+my $common_user_properties = {
+    userid => get_standard_option('userid', {
+	completion => \&PVE::AccessControl::complete_username,
+	}),
+    enable => {
+	title => "Enable",
+    description => "Enable the account (default). You can set this to '0' to disable the account",
+	type => 'boolean',
+	optional => 1,
+	default => 1,
+    },
+    expire => {
+	description => "Account expiration date (seconds since epoch). '0' means no expiration date.",
+	type => 'integer',
+	minimum => 0,
+	optional => 1,
+    },
+    firstname => { type => 'string', optional => 1 },
+    lastname => { type => 'string', optional => 1 },
+    email => { type => 'string', optional => 1, format => 'email-opt' },
+    comment => { type => 'string', optional => 1 },
+    keys => {
+	description => "Keys for two factor auth (yubico).",
+	type => 'string',
+	optional => 1,
+    },
+};
+
 my $extract_user_data = sub {
     my ($data, $full) = @_;
 
@@ -53,10 +81,8 @@ __PACKAGE__->register_method ({
 	type => 'array',
 	items => {
 	    type => "object",
-	    properties => {
-		userid => { type => 'string' },
+	    properties => $common_user_properties,
 	    },
-	},
 	links => [ { rel => 'child', href => "{userid}" } ],
     },
     code => sub {
@@ -108,8 +134,7 @@ __PACKAGE__->register_method ({
     description => "Create new user.",
     parameters => {
 	additionalProperties => 0,
-	properties => {
-	    userid => get_standard_option('userid'),
+	properties => { ( %$common_user_properties,
 	    password => {
 		description => "Initial password.",
 		type => 'string',
@@ -122,28 +147,7 @@ __PACKAGE__->register_method ({
 		optional => 1,
 		completion => \&PVE::AccessControl::complete_group,
 	    },
-	    firstname => { type => 'string', optional => 1 },
-	    lastname => { type => 'string', optional => 1 },
-	    email => { type => 'string', optional => 1, format => 'email-opt' },
-	    comment => { type => 'string', optional => 1 },
-	    keys => {
-		description => "Keys for two factor auth (yubico).",
-		type => 'string',
-		optional => 1,
-	    },
-	    expire => {
-		description => "Account expiration date (seconds since epoch). '0' means no expiration date.",
-		type => 'integer',
-		minimum => 0,
-		optional => 1,
-	    },
-	    enable => {
-		description => "Enable the account (default). You can set this to '0' to disable the accout",
-		type => 'boolean',
-		optional => 1,
-		default => 1,
-	    },
-	},
+	)}
     },
     returns => { type => 'null' },
     code => sub {
@@ -204,16 +208,10 @@ __PACKAGE__->register_method ({
     },
     returns => {
 	additionalProperties => 0,
-	properties => {
-	    enable => { type => 'boolean' },
-	    expire => { type => 'integer', optional => 1 },
-	    firstname => { type => 'string', optional => 1 },
-	    lastname => { type => 'string', optional => 1 },
-	    email => { type => 'string', optional => 1 },
-	    comment => { type => 'string', optional => 1 },
-	    keys => { type => 'string', optional => 1 },
+	properties => { ( %$common_user_properties{qw(enable expire firstname lastname email comment keys)},
 	    groups => { type => 'array' },
-	}
+	) },
+	type => "object"
     },
     code => sub {
 	my ($param) = @_;
@@ -239,10 +237,7 @@ __PACKAGE__->register_method ({
     description => "Update user configuration.",
     parameters => {
 	additionalProperties => 0,
-	properties => {
-	    userid => get_standard_option('userid', {
-		completion => \&PVE::AccessControl::complete_username,
-	    }),
+	properties => { (%{$common_user_properties},
 	    groups => {
 		type => 'string', format => 'pve-groupid-list',
 		optional => 1,
@@ -253,27 +248,7 @@ __PACKAGE__->register_method ({
 		optional => 1,
 		requires => 'groups',
 	    },
-	    enable => {
-		description => "Enable/disable the account.",
-		type => 'boolean',
-		optional => 1,
-	    },
-	    firstname => { type => 'string', optional => 1 },
-	    lastname => { type => 'string', optional => 1 },
-	    email => { type => 'string', optional => 1, format => 'email-opt' },
-	    comment => { type => 'string', optional => 1 },
-	    keys => {
-		description => "Keys for two factor auth (yubico).",
-		type => 'string',
-		optional => 1,
-	    },
-	    expire => {
-		description => "Account expiration date (seconds since epoch). '0' means no expiration date.",
-		type => 'integer',
-		minimum => 0,
-		optional => 1
-	    },
-	},
+	) },
     },
     returns => { type => 'null' },
     code => sub {
diff --git a/PVE/AccessControl.pm b/PVE/AccessControl.pm
index f0fb7dc..f847a8b 100644
--- a/PVE/AccessControl.pm
+++ b/PVE/AccessControl.pm
@@ -501,7 +501,7 @@ create_roles();
 
 sub role_is_special {
     my ($role) = @_;
-    return exists $special_roles->{$role};
+    return (exists $special_roles->{$role} ? 1 : 0);
 }
 
 sub add_role_privs {
diff --git a/PVE/Auth/Plugin.pm b/PVE/Auth/Plugin.pm
index d5d2c06..0bffa2b 100755
--- a/PVE/Auth/Plugin.pm
+++ b/PVE/Auth/Plugin.pm
@@ -76,7 +76,7 @@ sub verify_username {
 }
 
 PVE::JSONSchema::register_standard_option('userid', {
-    description => "User ID",
+    description => "User ID", title => "User ID",
     type => 'string', format => 'pve-userid',
     maxLength => 64,
 });
-- 
2.11.0




More information about the pve-devel mailing list