From: "Gabriel Goller" <g.goller@proxmox.com>
To: "Stoiko Ivanov" <s.ivanov@proxmox.com>
Cc: <pmg-devel@lists.proxmox.com>
Subject: Re: [pmg-devel] [PATCH] utils: cleanup username/userid regex and verify
Date: Wed, 14 Feb 2024 14:54:57 +0100 [thread overview]
Message-ID: <CZ4UPXW7I2I8.1EDVDG9R7VOQT@proxmox.com> (raw)
In-Reply-To: <20240214125537.5af34979@rosa.proxmox.com>
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
prev parent reply other threads:[~2024-02-14 13:55 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-14 9:15 Gabriel Goller
2024-02-14 11:55 ` Stoiko Ivanov
2024-02-14 13:54 ` Gabriel Goller [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CZ4UPXW7I2I8.1EDVDG9R7VOQT@proxmox.com \
--to=g.goller@proxmox.com \
--cc=pmg-devel@lists.proxmox.com \
--cc=s.ivanov@proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox