Re: [pve-devel] [PATCH access-control 1/1] fix #4411: openid: add logic for openid groups support

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

a few nits, mostly style related below

On September 1, 2024 6:55 pm, Thomas Skinner wrote:
> Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> ---
>  src/PVE/API2/OpenId.pm | 32 ++++++++++++++++++++++++++++++++
>  src/PVE/Auth/OpenId.pm | 21 +++++++++++++++++++++
>  2 files changed, 53 insertions(+)
> 
> diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm
> index 77410e6..22a2188 100644
> --- a/src/PVE/API2/OpenId.pm
> +++ b/src/PVE/API2/OpenId.pm
> @@ -220,6 +220,38 @@ __PACKAGE__->register_method ({
>  		$rpcenv->check_user_enabled($username);
>  	    }
>  
> +		if (defined(my $groups_claim = $config->{'groups-claim'})) {
> +			if (defined(my $groups_list = $info->{$groups_claim})) {
> +				if (UNIVERSAL::isa($groups_list, 'ARRAY')) {

we normally use `ref($groups_list) eq 'ARRAY'`

> +					PVE::AccessControl::lock_user_config(sub {
> +						my $usercfg = cfs_read_file("user.cfg");
> +						
> +						# if groups should be overwritten, delete them first
> +						if ( $config->{'groups-overwrite'}) {
> +							PVE::AccessControl::delete_user_group($username, $usercfg);
> +						}
> +						
> +						# replace any invalid characters with
> +						my $replace_character = $config->{'groups-replace-character'};
> +						my @oidc_groups_list = map { $_ =~ s/[^A-Za-z0-9\.\-_]/$replace_character/gr } @{ $groups_list };

we normally use array references, and (for new code) dereferencing via
->

this RE (continued below)

> +						
> +						# only populate groups that are in the oidc list and exist in pve
> +						my @existing_groups_list = keys %{$usercfg->{groups}};
> +						my @groups_intersect = grep { my $g = $_; grep $_ eq $g, @oidc_groups_list } @existing_groups_list;

we do have PVE::Tools:array_intersect which does this for N array
references..

> +
> +						# ensure user is a member of these groups
> +						map { PVE::AccessControl::add_user_group($username, $usercfg, $_) } @groups_intersect;

this could be a `for` loop, since the map result is not used at all..

> +
> +						cfs_write_file("user.cfg", $usercfg);
> +					}, "openid group mapping failed");
> +				} else {
> +					syslog('err', "groups list is not an array; groups will not be updated");
> +				}
> +			} else {
> +				syslog('err', "groups claim '$groups_claim' is not found in claims");
> +			}
> +		}
> +
>  	    my $ticket = PVE::AccessControl::assemble_ticket($username);
>  	    my $csrftoken = PVE::AccessControl::assemble_csrf_prevention_token($username);
>  	    my $cap = $rpcenv->compute_api_permission($username);
> diff --git a/src/PVE/Auth/OpenId.pm b/src/PVE/Auth/OpenId.pm
> index c8e4db9..0e3fdc4 100755
> --- a/src/PVE/Auth/OpenId.pm
> +++ b/src/PVE/Auth/OpenId.pm
> @@ -42,6 +42,24 @@ sub properties {
>  	    type => 'string',
>  	    optional => 1,
>  	},
> +	"groups-claim" => {
> +	    description => "OpenID claim used to retrieve groups with.",
> +	    type => 'string',
> +	    optional => 1,
> +	},
> +	"groups-overwrite" => {
> +		description => "All groups will be overwritten for the user on login.",
> +	    type => 'boolean',
> +		default => 0,
> +	    optional => 1,
> +	},
> +	"groups-replace-character" => {
> +	    description => "Character used to replace any invalid characters in groups from provider.",
> +	    type => 'string',
> +		pattern => '^[A-Za-z0-9\.\-_]$',

and this RE are inverses of eachother - should we define them in one
place in case we ever need to update it?

> +		default => '_',
> +	    optional => 1,
> +	},
>  	prompt => {
>  	    description => "Specifies whether the Authorization Server prompts the End-User for"
>  	        ." reauthentication and consent.",
> @@ -73,6 +91,9 @@ sub options {
>  	"client-key" => { optional => 1 },
>  	autocreate => { optional => 1 },
>  	"username-claim" => { optional => 1, fixed => 1 },
> +	"groups-claim" => { optional => 1 },
> +	"groups-overwrite" => { optional => 1 },
> +	"groups-replace-character" => { optional => 1},
>  	prompt => { optional => 1 },
>  	scopes => { optional => 1 },
>  	"acr-values" => { optional => 1 },
> -- 
> 2.39.2
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel