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

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

[Gabriel Goller]

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);
 sub verify_username {
     my ($username, $noerr) = @_;
 
     $username = '' if !$username;
     my $len = length($username);
-    if ($len < 3) {
+    if ($len < 1) {
 	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}',
     maxLength => 64,
 });
 
-- 
2.43.0

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

[Stoiko Ivanov]

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) 

>  });
>  

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

[Gabriel Goller]

Thanks for the review!

On Wed Feb 14, 2024 at 12:55 PM CET, Stoiko Ivanov wrote:
> > 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.

Right, yeah. I readded the register_format call...
Hmm how would you unify verify_entry with verify_username though? It 
seems to me that verify_entry just splits the username from the userid 
(if needed) then checks if the username is in the userid (which we 
could also check in verify_username) and then calls verify_username?

> >  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..

Missed this :(
How about I use a min length of 5 here? 
shortest realm (pam/pmg) + @ + shortest username = 5