[pve-devel] [PATCH access-control v4 1/2] refactor API by unifying duplicate properties
Thomas Lamprecht
t.lamprecht at proxmox.com
Mon Jun 18 18:23:52 CEST 2018
On 6/18/18 10:18 AM, 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)
OK, but...
> * Additionally for some fields the title property is added (its not used
> elswhere) - as preparation for unified CLI handling
...this is not refactoring...
> * PVE/AccessControl::role_is_special now return 0 instead of '' for false
> (Schemavalidation did complain about '')
...neither this...
> * For 2 fields a new property (print_width) was added
...and neither this, please do this in separate patches.
In general, if you can make separate list points in the commit message,
like here, you may consider if splitting the patch up makes it
a) easier to digest
b) less likely to introduce regressions
It's not always straight forward and there's room for opinions but pure
refactoring should not be mixed with changes outside of the refactoring
scope (in your case, this means all changes which do not directly relate
to unifying duplicate properties).
The end result looks mostly good... another comment inline, though.
>> Signed-off-by: Stoiko Ivanov <s.ivanov at proxmox.com>
> ---
> PVE/API2/ACL.pm | 36 +++++++------
> PVE/API2/AccessControl.pm | 12 ++---
> PVE/API2/Group.pm | 35 ++++++------
> PVE/API2/Role.pm | 54 ++++++++++++-------
> PVE/API2/User.pm | 132 +++++++++++++++++++++++-----------------------
> PVE/AccessControl.pm | 9 +---
> PVE/Auth/Plugin.pm | 11 +++-
> 7 files changed, 155 insertions(+), 134 deletions(-)
>
> diff --git a/PVE/API2/ACL.pm b/PVE/API2/ACL.pm
> index d37771b..298f8f4 100644
> --- a/PVE/API2/ACL.pm
> +++ b/PVE/API2/ACL.pm
> @@ -6,6 +6,7 @@ use PVE::Cluster qw (cfs_read_file cfs_write_file);
> use PVE::Tools qw(split_list);
> use PVE::AccessControl;
> use PVE::Exception qw(raise_param_exc);
> +use PVE::JSONSchema qw(get_standard_option register_standard_option);
>
> use PVE::SafeSyslog;
>
> @@ -13,6 +14,19 @@ use PVE::RESTHandler;
>
> use base qw(PVE::RESTHandler);
>
> +register_standard_option('acl-propagate', {
> + description => "Allow to propagate (inherit) permissions.",
> + type => 'boolean',
> + title => 'Propagate',
> + optional => 1,
> + default => 1,
> +});
> +register_standard_option('acl-path', {
> + description => "Access control path",
> + title => 'Path',
> + type => 'string',
> +});
> +
> __PACKAGE__->register_method ({
> name => 'read_acl',
> path => '',
> @@ -32,11 +46,11 @@ __PACKAGE__->register_method ({
> type => "object",
> additionalProperties => 0,
> properties => {
> - path => { type => 'string' },
> - type => { type => 'string', enum => ['user', 'group'] },
> - ugid => { type => 'string' },
> - roleid => { type => 'string' },
> - propagate => { type => 'boolean' },
> + propagate => get_standard_option('acl-propagate'),
> + path => get_standard_option('acl-path'),
> + type => { type => 'string', title => 'Type', enum => ['user', 'group'] },
> + ugid => { type => 'string', title => 'ID'},
> + roleid => { type => 'string', title => 'Role' },
> },
> },
> },
> @@ -90,10 +104,8 @@ __PACKAGE__->register_method ({
> parameters => {
> additionalProperties => 0,
> properties => {
> - path => {
> - description => "Access control path",
> - type => 'string',
> - },
> + propagate => get_standard_option('acl-propagate'),
> + path => get_standard_option('acl-path'),
> users => {
> description => "List of users.",
> type => 'string', format => 'pve-userid-list',
> @@ -108,12 +120,6 @@ __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',
> diff --git a/PVE/API2/AccessControl.pm b/PVE/API2/AccessControl.pm
> index afcd2fa..37efd96 100644
> --- a/PVE/API2/AccessControl.pm
> +++ b/PVE/API2/AccessControl.pm
> @@ -15,6 +15,7 @@ use PVE::API2::User;
> use PVE::API2::Group;
> use PVE::API2::Role;
> use PVE::API2::ACL;
> +use PVE::Auth::Plugin;;
>
> use base qw(PVE::RESTHandler);
>
> @@ -204,12 +205,7 @@ __PACKAGE__->register_method ({
> parameters => {
> additionalProperties => 0,
> properties => {
> - username => {
> - description => "User name",
> - type => 'string',
> - maxLength => 64,
> - completion => \&PVE::AccessControl::complete_username,
> - },
> + username => get_standard_option('userid', { description => "User name"}),
> realm => get_standard_option('realm', {
> description => "You can optionally pass the realm using this parameter. Normally the realm is simply added to the username <username>\@<relam>.",
> optional => 1,
> @@ -301,9 +297,7 @@ __PACKAGE__->register_method ({
> parameters => {
> additionalProperties => 0,
> properties => {
> - userid => get_standard_option('userid', {
> - completion => \&PVE::AccessControl::complete_username,
> - }),
> + userid => get_standard_option('userid'),
> password => {
> description => "The new password.",
> type => 'string',
> diff --git a/PVE/API2/Group.pm b/PVE/API2/Group.pm
> index fca8a2a..4503597 100644
> --- a/PVE/API2/Group.pm
> +++ b/PVE/API2/Group.pm
> @@ -6,9 +6,19 @@ use PVE::Cluster qw (cfs_read_file cfs_write_file);
> use PVE::AccessControl;
> use PVE::SafeSyslog;
> use PVE::RESTHandler;
> +use PVE::JSONSchema qw(get_standard_option register_standard_option);
>
> use base qw(PVE::RESTHandler);
>
> +register_standard_option('group-id', {
> + type => 'string',
> + format => 'pve-groupid',
> + title => 'Group ID' ,
> + completion => \&PVE::AccessControl::complete_group,
> +});
> +
> +register_standard_option('group-comment', { type => 'string', optional => 1 });
> +
> __PACKAGE__->register_method ({
> name => 'index',
> path => '',
> @@ -27,7 +37,8 @@ __PACKAGE__->register_method ({
> items => {
> type => "object",
> properties => {
> - groupid => { type => 'string' },
> + groupid => get_standard_option('group-id'),
> + comment => get_standard_option('group-comment'),
> },
> },
> links => [ { rel => 'child', href => "{groupid}" } ],
> @@ -66,8 +77,8 @@ __PACKAGE__->register_method ({
> parameters => {
> additionalProperties => 0,
> properties => {
> - groupid => { type => 'string', format => 'pve-groupid' },
> - comment => { type => 'string', optional => 1 },
> + groupid => get_standard_option('group-id'),
> + comment => get_standard_option('group-comment'),
> },
> },
> returns => { type => 'null' },
> @@ -107,11 +118,8 @@ __PACKAGE__->register_method ({
> parameters => {
> additionalProperties => 0,
> properties => {
> - groupid => {
> - type => 'string', format => 'pve-groupid',
> - completion => \&PVE::AccessControl::complete_group,
> - },
> - comment => { type => 'string', optional => 1 },
> + groupid => get_standard_option('group-id'),
> + comment => get_standard_option('group-comment'),
> },
> },
> returns => { type => 'null' },
> @@ -149,18 +157,18 @@ __PACKAGE__->register_method ({
> parameters => {
> additionalProperties => 0,
> properties => {
> - groupid => { type => 'string', format => 'pve-groupid' },
> + groupid => get_standard_option('group-id'),
> },
> },
> returns => {
> type => "object",
> additionalProperties => 0,
> properties => {
> - comment => { type => 'string', optional => 1 },
> + comment => get_standard_option('group-comment'),
> members => {
> type => 'array',
> items => {
> - type => "string",
> + type => 'string', format => 'pve-userid',
> },
> },
> },
> @@ -198,10 +206,7 @@ __PACKAGE__->register_method ({
> parameters => {
> additionalProperties => 0,
> properties => {
> - groupid => {
> - type => 'string' , format => 'pve-groupid',
> - completion => \&PVE::AccessControl::complete_group,
> - },
> + groupid => get_standard_option('group-id'),
> }
> },
> returns => { type => 'null' },
> diff --git a/PVE/API2/Role.pm b/PVE/API2/Role.pm
> index adbb4db..156d3b8 100644
> --- a/PVE/API2/Role.pm
> +++ b/PVE/API2/Role.pm
> @@ -4,6 +4,7 @@ use strict;
> use warnings;
> use PVE::Cluster qw (cfs_read_file cfs_write_file);
> use PVE::AccessControl;
> +use PVE::JSONSchema qw(get_standard_option register_standard_option);
>
> use PVE::SafeSyslog;
>
> @@ -11,6 +12,18 @@ use PVE::RESTHandler;
>
> use base qw(PVE::RESTHandler);
>
> +register_standard_option('role-id', {
> + type => 'string',
> + format => 'pve-roleid',
> + title => 'Role ID',
> + print_width => 30
> +});
> +register_standard_option('role-privs', {
> + type => 'string' ,
> + format => 'pve-priv-list',
> + optional => 1, title => 'Privileges',
> +});
> +
> __PACKAGE__->register_method ({
> name => 'index',
> path => '',
> @@ -28,7 +41,9 @@ __PACKAGE__->register_method ({
> items => {
> type => "object",
> properties => {
> - roleid => { type => 'string' },
> + roleid => get_standard_option('role-id'),
> + privs => get_standard_option('role-privs'),
> + special => { type => 'boolean', optional => 1, default => 0, title => 'Built-In' },
> },
> },
> links => [ { rel => 'child', href => "{roleid}" } ],
> @@ -64,8 +79,8 @@ __PACKAGE__->register_method ({
> parameters => {
> additionalProperties => 0,
> properties => {
> - roleid => { type => 'string', format => 'pve-roleid' },
> - privs => { type => 'string' , format => 'pve-priv-list', optional => 1 },
> + roleid => get_standard_option('role-id'),
> + privs => get_standard_option('role-privs'),
> },
> },
> returns => { type => 'null' },
> @@ -100,17 +115,13 @@ __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',
> - },
> + roleid => get_standard_option('role-id'),
> + privs => get_standard_option('role-privs'),
> + append => { type => 'boolean', optional => 1, requires => 'privs' },
> },
> },
> returns => { type => 'null' },
> @@ -137,7 +148,6 @@ __PACKAGE__->register_method ({
> return undef;
> }});
>
> -# fixme: return format!
> __PACKAGE__->register_method ({
> name => 'read_role',
> path => '{roleid}',
> @@ -149,10 +159,16 @@ __PACKAGE__->register_method ({
> parameters => {
> additionalProperties => 0,
> properties => {
> - roleid => { type => 'string' , format => 'pve-roleid' },
> + roleid => get_standard_option('role-id'),
> + },
> + },
> + returns => {
> + type => "object",
> + additionalProperties => 0,
> + properties => {
> + privs => get_standard_option('role-privs'),
> },
> },
> - returns => {},
> code => sub {
> my ($param) = @_;
>
> @@ -165,7 +181,8 @@ __PACKAGE__->register_method ({
> die "role '$role' does not exist\n" if !$data;
>
> return $data;
> -}});
> + }
> +});
>
> __PACKAGE__->register_method ({
> name => 'delete_role',
> @@ -179,8 +196,8 @@ __PACKAGE__->register_method ({
> parameters => {
> additionalProperties => 0,
> properties => {
> - roleid => { type => 'string', format => 'pve-roleid' },
> - }
> + roleid => get_standard_option('role-id'),
> + },
> },
> returns => { type => 'null' },
> code => sub {
> @@ -206,6 +223,7 @@ __PACKAGE__->register_method ({
> }, "delete role failed");
>
> return undef;
> -}});
> + }
> +});
>
> 1;
> diff --git a/PVE/API2/User.pm b/PVE/API2/User.pm
> index 1dc0293..70760f9 100644
> --- a/PVE/API2/User.pm
> +++ b/PVE/API2/User.pm
> @@ -6,7 +6,8 @@ use PVE::Exception qw(raise raise_perm_exc);
> use PVE::Cluster qw (cfs_read_file cfs_write_file);
> use PVE::Tools qw(split_list);
> use PVE::AccessControl;
> -use PVE::JSONSchema qw(get_standard_option);
> +use PVE::JSONSchema qw(get_standard_option register_standard_option);
> +use PVE::Auth::Plugin;
>
> use PVE::SafeSyslog;
>
> @@ -14,6 +15,34 @@ use PVE::RESTHandler;
>
> use base qw(PVE::RESTHandler);
>
> +register_standard_option('user-enable', {
> + title => "Enable",
> + description => "Enable the account (default). You can set this to '0' to disable the account",
> + type => 'boolean',
> + optional => 1,
> + default => 1,
> +});
> +register_standard_option('user-expire', {
> + description => "Account expiration date (seconds since epoch). '0' means no expiration date.",
> + type => 'integer',
> + minimum => 0,
> + optional => 1,
> +});
> +register_standard_option('user-firstname', { type => 'string', optional => 1 });
> +register_standard_option('user-lastname', { type => 'string', optional => 1 });
> +register_standard_option('user-email', { type => 'string', optional => 1, format => 'email-opt' });
> +register_standard_option('user-comment', { type => 'string', optional => 1 });
> +register_standard_option('user-keys', {
> + description => "Keys for two factor auth (yubico).",
> + type => 'string',
> + optional => 1,
> +});
> +register_standard_option('group-list', {
> + type => 'string', format => 'pve-groupid-list',
> + optional => 1,
> + completion => \&PVE::AccessControl::complete_group,
> +});
> +
> my $extract_user_data = sub {
> my ($data, $full) = @_;
>
> @@ -54,7 +83,14 @@ __PACKAGE__->register_method ({
> items => {
> type => "object",
> properties => {
> - userid => { type => 'string' },
> + userid => get_standard_option('userid'),
> + enable => get_standard_option('user-enable'),
> + expire => get_standard_option('user-expire'),
> + firstname => get_standard_option('user-firstname'),
> + lastname => get_standard_option('user-lastname'),
> + email => get_standard_option('user-email'),
> + comment => get_standard_option('user-comment'),
> + keys => get_standard_option('user-keys'),
> },
> },
> links => [ { rel => 'child', href => "{userid}" } ],
> @@ -110,6 +146,13 @@ __PACKAGE__->register_method ({
> additionalProperties => 0,
> properties => {
> userid => get_standard_option('userid'),
> + enable => get_standard_option('user-enable'),
> + expire => get_standard_option('user-expire'),
> + firstname => get_standard_option('user-firstname'),
> + lastname => get_standard_option('user-lastname'),
> + email => get_standard_option('user-email'),
> + comment => get_standard_option('user-comment'),
> + keys => get_standard_option('user-keys'),
> password => {
> description => "Initial password.",
> type => 'string',
> @@ -117,32 +160,7 @@ __PACKAGE__->register_method ({
> minLength => 5,
> maxLength => 64
> },
> - groups => {
> - type => 'string', format => 'pve-groupid-list',
> - 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,
> - },
> + groups => get_standard_option('group-list'),
> },
> },
> returns => { type => 'null' },
> @@ -205,15 +223,16 @@ __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 },
> + enable => get_standard_option('user-enable'),
> + expire => get_standard_option('user-expire'),
> + firstname => get_standard_option('user-firstname'),
> + lastname => get_standard_option('user-lastname'),
> + email => get_standard_option('user-email'),
> + comment => get_standard_option('user-comment'),
> + keys => get_standard_option('user-keys'),
> groups => { type => 'array' },
> - }
> + },
> + type => "object"
> },
> code => sub {
> my ($param) = @_;
> @@ -240,39 +259,20 @@ __PACKAGE__->register_method ({
> parameters => {
> additionalProperties => 0,
> properties => {
> - userid => get_standard_option('userid', {
> - completion => \&PVE::AccessControl::complete_username,
> - }),
> - groups => {
> - type => 'string', format => 'pve-groupid-list',
> - optional => 1,
> - completion => \&PVE::AccessControl::complete_group,
> - },
> + userid => get_standard_option('userid'),
> + enable => get_standard_option('user-enable'),
> + expire => get_standard_option('user-expire'),
> + firstname => get_standard_option('user-firstname'),
> + lastname => get_standard_option('user-lastname'),
> + email => get_standard_option('user-email'),
> + comment => get_standard_option('user-comment'),
> + keys => get_standard_option('user-keys'),
> + groups => get_standard_option('group-list'),
> append => {
> type => 'boolean',
> 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' },
> @@ -333,9 +333,7 @@ __PACKAGE__->register_method ({
> parameters => {
> additionalProperties => 0,
> properties => {
> - userid => get_standard_option('userid', {
> - completion => \&PVE::AccessControl::complete_username,
> - }),
> + userid => get_standard_option('userid'),
> }
> },
> returns => { type => 'null' },
> diff --git a/PVE/AccessControl.pm b/PVE/AccessControl.pm
> index f0fb7dc..9184077 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 {
> @@ -1078,13 +1078,6 @@ sub remove_vm_from_pool {
>
> # bash completion helpers
>
> -sub complete_username {
> -
> - my $user_cfg = cfs_read_file('user.cfg');
> -
> - return [ keys %{$user_cfg->{users}} ];
> -}
> -
> sub complete_group {
>
> my $user_cfg = cfs_read_file('user.cfg');
> diff --git a/PVE/Auth/Plugin.pm b/PVE/Auth/Plugin.pm
> index d5d2c06..7701eed 100755
> --- a/PVE/Auth/Plugin.pm
> +++ b/PVE/Auth/Plugin.pm
> @@ -76,11 +76,18 @@ 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,
> + maxLength => 64, completion => \&complete_username,
> });
>
> +sub complete_username {
> +
> + my $user_cfg = cfs_read_file('user.cfg');
Hmm, really not sure about this...
user.cfg gets registered in PVE::AccessControl so this only works
by chance. Also the base Plugin is used (and registered) by
PVE::AccessControl, which makes this a somewhat strange dependency
cycle...
> +
> + return [ keys %{$user_cfg->{users}} ];
> +}
> +
> PVE::JSONSchema::register_format('pve-tfa-config', \&verify_tfa_config);
> sub verify_tfa_config {
> my ($value, $noerr) = @_;
>
More information about the pve-devel
mailing list