Re: [pve-devel] [PATCH cluster v8 4/4] DataCenterConfig: add tag rights control to the datacenter config

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 18/10/2022 um 16:02 schrieb Dominik Csapak:
> by adding a 'user-tag-privileges' and 'admin-tags' option.
> The first sets the policy by which "normal" users (with
> 'VM.Config.Options' on the respective guest) can create/delete tags
> and the second is a list of tags only settable by 'admins'
> ('Sys.Modify' on '/')
> 
> also add a helper 'get_user_admin_tags' that returns two hashmaps that
> determines the allowed user tags and admin tags that require elevated
> permissions
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  data/PVE/DataCenterConfig.pm | 93 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 93 insertions(+)
> 
> diff --git a/data/PVE/DataCenterConfig.pm b/data/PVE/DataCenterConfig.pm
> index bb29d26..e2140ff 100644
> --- a/data/PVE/DataCenterConfig.pm
> +++ b/data/PVE/DataCenterConfig.pm
> @@ -154,6 +154,26 @@ my $tag_style_format = {
>      },
>  };
>  
> +my $user_tag_privs_format = {
> +    'usable' => {
> +	optional => 1,
> +	type => 'string',
> +	enum => ['none', 'list', 'existing', 'free'],
> +	default => 'free',
> +	dscription => "Determines which tags a user without Sys.Modify on '/' can set and delete. ".
> +	    "'none' means no tags are settable.'list' allows tags from the given list. ".
> +	    "'existing' means only already existing tags or from the given list. ".
> +	    "And 'free' means users can assign any tags."

could be split into a "description" (for CLI usage) and a "verbose_description" (for man page/docs),
something like:

description => "Controls tag usage for users without `Sys.Modify` on `/` by either"
    ." allowing `none`, a `list`, already `existing` (used) or anything (`free`).",
verbose_description => "Controls which tags can be set or deleted on resources an user
    ." controls (such as guests). Users with the `Sys.Modify` privilege on `/` are always unrestricted."
    ."* `none`: ..."
    ."* `list`: ..."
    ."* `existing`: ..."
    ."* `free`: ...",

> +    },
> +    'list' => {
> +	optional => 1,
> +	type => 'string',
> +	pattern => "${PVE::JSONSchema::PVE_TAG_RE}(?:\;${PVE::JSONSchema::PVE_TAG_RE})*",
> +	typetext => "<tag>[;<tag>=...]",
> +	description => "List of tags users are allowd to set and delete (semicolon separated).",
> +    },
> +};
> +
>  my $datacenter_schema = {
>      type => "object",
>      additionalProperties => 0,
> @@ -285,12 +305,60 @@ my $datacenter_schema = {
>  	    description => "Tag style options.",
>  	    format => $tag_style_format,
>  	},
> +	'user-tag-privileges' => {
> +	    optional => 1,
> +	    type => 'string',
> +	    description => "Privilege options for user settable tags",
> +	    format => $user_tag_privs_format,
> +	},
> +	'admin-tags' => {
> +	    optional => 1,
> +	    type => 'string',
> +	    description => "A list of tags only admins (Sys.Modify on '/') are allowed to set/delete",
> +	    pattern => "(?:${PVE::JSONSchema::PVE_TAG_RE};)*${PVE::JSONSchema::PVE_TAG_RE}",
> +	},
>      },
>  };
>  
>  # make schema accessible from outside (for documentation)
>  sub get_datacenter_schema { return $datacenter_schema };
>  
> +# returns two hashmaps of tags, the first is the list of tags that can

returns a tuple of two hash maps with tags as keys, ...

> +# be used by users with 'VM.Config.Options', and the second is a list

be used with just 'VM.Config.Options' on '/vms/{vmid}'

> +# that needs 'Sys.Modify' on '/'
> +#
> +# If the first map is 'undef', it means there is generally no restriction
> +# besides the tags defined in the second map.
> +#
> +# CAUTION: this function may include tags from *all* guest configs,
> +# regardless of the current authuser
> +sub get_user_admin_tags {

hmm, sounds a bit confusing, mabye one of:

* add and: get_user_and_admin_tags

* get_unrestricted_and_registered_tags

* or just get_allowed_tags (with the comment highlighting that it returns two, the
  allowed for all, and the one for "admins" it would be quite clear and also short)