Thread: BUG #15932: Module passwordcheck doesn't reference previous hooks

The following bug has been logged on the website:

Bug reference:      15932
Logged by:          Rafael Castro
Email address:      rafaelthca@gmail.com
PostgreSQL version: 11.4
Operating system:   Any
Description:

Module passwordcheck doesn't reference (thus, doesn't call) previous
hooks:

https://github.com/postgres/postgres/blob/master/contrib/passwordcheck/passwordcheck.c#L133

As a result, if passwordcheck is used hook check_password_hook becomes
useless.

On Mon, Jul 29, 2019 at 03:56:35PM +0000, PG Bug reporting form wrote:
> Module passwordcheck doesn't reference (thus, doesn't call) previous
> hooks:
>
> https://github.com/postgres/postgres/blob/master/contrib/passwordcheck/passwordcheck.c#L133
>
> As a result, if passwordcheck is used hook check_password_hook becomes
> useless.

So, it would erase the trace of previous hooks if loaded last.
passwordcheck holds value as an example of use for some hooks, and
examples should be a good students, so I agree to do better and fix
that.  The attached patch does the work, what do you think about it?
--
Michael

On Tue, Jul 30, 2019 at 11:24:20AM +0900, Michael Paquier wrote:
> So, it would erase the trace of previous hooks if loaded last.
> passwordcheck holds value as an example of use for some hooks, and
> examples should be a good students, so I agree to do better and fix
> that.  The attached patch does the work, what do you think about it?

Are there any objections about that stuff?  I would not touch anything
else than HEAD for that issue.
--
Michael

> On 31 Jul 2019, at 09:15, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Tue, Jul 30, 2019 at 11:24:20AM +0900, Michael Paquier wrote:
>> So, it would erase the trace of previous hooks if loaded last.
>> passwordcheck holds value as an example of use for some hooks, and
>> examples should be a good students, so I agree to do better and fix
>> that.  The attached patch does the work, what do you think about it?
>
> Are there any objections about that stuff?  I would not touch anything
> else than HEAD for that issue.

LGTM.  A small nitpick is that the below comment isn’t really giving the full
picture, prev_check_password_hook is not just used for unload but for the hook
chaining.

+/* Saved hook value in case of unload */

cheers ./daniel

On Wed, Jul 31, 2019 at 09:23:54AM +0200, Daniel Gustafsson wrote:
> LGTM.  A small nitpick is that the below comment isn’t really giving the full
> picture, prev_check_password_hook is not just used for unload but for the hook
> chaining.

My understanding is that it's the same meaning, and also:
$ git grep "in case of unload"
contrib/auto_explain/auto_explain.c:/* Saved hook values in case of
unload */
contrib/pg_stat_statements/pg_stat_statements.c:/* Saved hook values
in case of unload */
src/test/modules/test_rls_hooks/test_rls_hooks.c:/* Saved hook values
in case of unload */
--
Michael

> On 31 Jul 2019, at 09:34, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Jul 31, 2019 at 09:23:54AM +0200, Daniel Gustafsson wrote:
>> LGTM.  A small nitpick is that the below comment isn’t really giving the full
>> picture, prev_check_password_hook is not just used for unload but for the hook
>> chaining.
>
> My understanding is that it's the same meaning, and also:
> $ git grep "in case of unload"
> contrib/auto_explain/auto_explain.c:/* Saved hook values in case of
> unload */
> contrib/pg_stat_statements/pg_stat_statements.c:/* Saved hook values
> in case of unload */
> src/test/modules/test_rls_hooks/test_rls_hooks.c:/* Saved hook values
> in case of unload */

Hmm, ok, IMO my comment applies to those as well, but it may very well be me
just reading it wrong.  Either way, +1 for pushing your patch.

cheers ./daniel

On Wed, Jul 31, 2019 at 09:54:17AM +0200, Daniel Gustafsson wrote:
> Hmm, ok, IMO my comment applies to those as well, but it may very well be me
> just reading it wrong.  Either way, +1 for pushing your patch.

I am not saying that this cannot be improved either, still I am not
sure how it could be reworded.  And if we do that, it should be
applied to all places where we use it.  I have fixed the issue and
back-patched down to 9.4.  Perhaps that was not worth caring about,
but the versions for the back-branches were not really an issue.

Thanks for the report, Rafael.  And thanks for the review, Daniel.
--
Michael