On 2/6/25 13:20, Alexander Abraham wrote:
Added a field for OpenID audiences to the JSON schema and retrieved the data for the audiences the user configured on the frontend.
Hi, same comment as for the last patch: please don't write the 'what'/'how' here, but rather the 'why' e.g. 'We want to let users specify the audiences to verify [...]' some comments inline
Signed-off-by: Alexander Abraham <a.abra...@proxmox.com> --- src/PVE/API2/OpenId.pm | 5 ++++- src/PVE/Auth/OpenId.pm | 6 ++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/PVE/API2/OpenId.pm b/src/PVE/API2/OpenId.pm index 77410e6..ad92fcb 100644 --- a/src/PVE/API2/OpenId.pm +++ b/src/PVE/API2/OpenId.pm @@ -45,6 +45,10 @@ my $lookup_openid_auth = sub { $openid_config->{acr_values} = [ PVE::Tools::split_list($acr) ]; }+ if (defined(my $aud = $config->{'aud'})) {+ $openid_config->{aud} = [ PVE::Tools::split_list($aud) ]; + } +
you do a split list here
my $openid = PVE::RS::OpenId->discover($openid_config, $redirect_url); return ($config, $openid); }; @@ -169,7 +173,6 @@ __PACKAGE__->register_method ({ my $redirect_url = extract_param($param, 'redirect-url');my ($config, $openid) = $lookup_openid_auth->($realm, $redirect_url);- my $info = $openid->verify_authorization_code($param->{code}, $private_auth_state); my $subject = $info->{'sub'};diff --git a/src/PVE/Auth/OpenId.pm b/src/PVE/Auth/OpenId.pmindex c8e4db9..a7af1bc 100755 --- a/src/PVE/Auth/OpenId.pm +++ b/src/PVE/Auth/OpenId.pm @@ -63,6 +63,11 @@ sub properties { pattern => '^[^\x00-\x1F\x7F <>#"]*$', # Prohibit characters not allowed in URI RFC 2396. optional => 1, }, + 'aud' => { + description => "Specifies the Authentication claims neccessary for checking the privileges the requesting user has.", + type => 'string', + optional => 1, + },
but here you don't mention that it's actually a list also there is no limit on what users can enter, normally we are rather strict with that (e.g. just allow [a-zA-Z0-9_+.] etc.) it's better to be strict first since we can always loosen the requirements later. when we accept everything it's much harder to get more strict later also, i don't think 'aud' is the best way to name this field. what about 'audiences' ?
}; }@@ -76,6 +81,7 @@ sub options {prompt => { optional => 1 }, scopes => { optional => 1 }, "acr-values" => { optional => 1 }, + "aud" => { optional => 1 }, default => { optional => 1 }, comment => { optional => 1 }, };
_______________________________________________ pve-devel mailing list pve-devel@lists.proxmox.com https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
