Re: Parametrization minimum password lenght

We notice some errors on CFBot results.

In attached the errors fixed

Il giorno lun 18 nov 2024 alle ore 16:59 Emanuele Musella <
emamuse86(at)gmail(dot)com> ha scritto:

> 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(dot)pg(at)gmail(dot)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(at)boriani(dot)cloud>
>> + * Author: Emanuele Musella <emamuse86(at)gmail(dot)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
>>
>