Hello Bertrand, thank you for your feedbacks.
We have done every fixes required.
In attach the last version.
Best regards
Il giorno lun 18 nov 2024 alle ore 11:39 Bertrand Drouvot <
bertranddrouvot...@gmail.com> ha scritto:
> Hi,
>
> On Mon, Nov 18, 2024 at 10:01:41AM +0100, Emanuele Musella wrote:
> > Hi all,
> >
> > we have fixed the following warning from the CFbot:
> >
> > [07:14:29.637] passwordcheck.c:37:5: error: no previous extern
> declaration
> > for non-static variable 'min_password_length'
> > [-Werror,-Wmissing-variable-declarations]
> >
> >
> > In attach the fixed patch.
>
> Thanks for the patch, that looks like a useful feature!
>
> A few random comments:
>
> === 1
>
> + * Author: Maurizio Boriani <maurizio@boriani.cloud>
> + * Author: Emanuele Musella <emamus...@gmail.com>
>
> I'm not sure we do that usually. For example, in cb3384a0cb, Fabien Coelho
> has
> not been mentioned as an author in contrib/isn/isn.c.
>
> === 2
>
> #include "commands/user.h"
> #include "fmgr.h"
> #include "libpq/crypt.h"
> +#include "commands/explain.h"
> +#include "utils/guc.h"
>
> The "includes" should be in the order based on their ASCII value (see
> 7e735035f2).
>
> === 3
>
> +/* min_password_length minimum password length */
>
> I think "/* Minimum password length */ is "enough" for the comment.
>
> Also what about adding "/* GUC variables */" on top of it?
>
> === 4 (Nit)
>
> +static int min_password_length;
>
> What about "min_pwd_len" to make it consistent with pwd_has_letter and
> pwd_has_nonletter
> for example?
>
> === 5
>
> prev_check_password_hook = check_password_hook;
> check_password_hook = check_password;
> +
> + /* Define custom GUC variables. */
> + DefineCustomIntVariable("passwordcheck.min_password_length",
>
> What about moving the DefineCustomIntVariable before the hooks? (that
> would be
> consistent with auto_explain.c, auth_delay.c, autoprewarm.c and
> pg_stat_statements.c
> to name a few).
>
> === 6
>
> + "8 is default.",
>
> I don't think that's needed (that would be visible from
> pg_settings.extra_desc
> while it already provides the information through the boot_val field).
>
> === 7
>
> + GUC_UNIT_MS,
>
> Not the right unit, that should be GUC_UNIT_BYTE because...
>
> === 8
>
> postgres=# SET passwordcheck.min_password_length = 4;
> SET
> postgres=# create user test with password 'ab1';
> ERROR: password is too short
>
> But with multibyte in play:
>
> postgres=# create user test with password 'äb1';
> CREATE ROLE.
>
> That's because "strlen(password);" returns the number of bytes.
>
> We could make it based on the characters instead by using "pg_mbstrlen()"
> but that would break the behavior as compared to now. So, I think we
> just need to make it clear that's bytes instead.
>
> === 9
>
> I think that a call to MarkGUCPrefixReserved() is missing (see
> auto_explain.c
> for example).
>
> === 10
>
> + <para>
> + In <filename>postgresql.conf</filename> you may set the minimum
> password length
> + by setting passwordcheck.min_password_length = INT.
>
> I think that would make sense to add a "Configuration Parameters" section
> and
> follow the format done in auto-explain or pg_prewarm for example.
>
> === 11
>
> + The default minimum password length if not setted
> passwordcheck.min_password_length is 8 chars.
>
> s/chars/bytes/ (as per === 8 above)
>
> Regards,
>
> --
> Bertrand Drouvot
> PostgreSQL Contributors Team
> RDS Open Source Databases
> Amazon Web Services: https://aws.amazon.com
>
min_password_length_v6.patch
Description: Binary data
