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 <emamuse86@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
