[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