[pve-devel] [PATCH access-control v3 1/2] refactor API by unifying duplicate properties
Thomas Lamprecht
t.lamprecht at proxmox.com
Thu Jun 14 08:42:09 CEST 2018
On 6/12/18 12:41 PM, Stoiko Ivanov wrote:
> * 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',
> + },
> +};
maybe it makes sense to register standard options for these
here locally and use get_standard_option in the API calls,
would keep the info of what parameters an API call has more
local to its definition. What do you think?
> __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,
> });
>
More information about the pve-devel
mailing list