Re: [pmg-devel] [PATCH] utils: cleanup username/userid regex and verify

[Stoiko Ivanov <s.ivanov@proxmox.com>]

Thanks for addressing this so promptly

a few notes inline:
On Wed, 14 Feb 2024 10:15:01 +0100
Gabriel Goller <g.goller@proxmox.com> wrote:

> Cleaned up the verify_username function and userid regex after the
> recent changes to minLength have been applied [0].
> 
> [0]: https://lists.proxmox.com/pipermail/pmg-devel/2023-September/002521.html
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  src/PMG/Utils.pm | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PMG/Utils.pm b/src/PMG/Utils.pm
> index 12b3ed5..8f7d438 100644
> --- a/src/PMG/Utils.pm
> +++ b/src/PMG/Utils.pm
> @@ -72,13 +72,12 @@ PVE::JSONSchema::register_standard_option('pmg-endtime', {
>      optional => 1,
>  });
>  
> -PVE::JSONSchema::register_format('pmg-userid', \&verify_username);
why deregister the format here? (verify_username does a bit more than a
regex match - and reusing the same verification we use in the auth-code
also in the parts where the API comes in helps in not getting even more
matches-almost-the-same-regexes matching auth-data) - Currently I'd rather
aim to reduce those and if possible unify PMG::UserConfig::verify_entry
with verify_username here as far as possible - see also:
https://lists.proxmox.com/pipermail/pmg-devel/2023-March/002381.html
and Fabian's follow-up to it.


>  sub verify_username {
>      my ($username, $noerr) = @_;
>  
>      $username = '' if !$username;
>      my $len = length($username);
> -    if ($len < 3) {
> +    if ($len < 1) {
this "username" here is actually the one with the realm...
e.g. root@pam vs. root - so limiting the length to 1 is too little
restrictive - probably at least renaming the variable name to user_id
might help in reducing confusion..


>  	die "user name '$username' is too short\n" if !$noerr;
>  	return undef;
>      }
> @@ -102,8 +101,8 @@ sub verify_username {
>  
>  PVE::JSONSchema::register_standard_option('userid', {
>      description => "User ID",
> -    type => 'string', format => 'pmg-userid',
> -    minLength => 4,
> +    type => 'string',
> +    pattern => '[^\s:\/]{1,60}',
the pattern you add here.. 
>      maxLength => 64,
effectively sets the maxLength to 60 here (you get a different
error-message if you're over 64, but still cannot enter anything over 60..)

some thorough testing (especially with corner-cases) would be appreciated
(not only for your direct patch) 

>  });
>