Re: Proposal: SET ROLE hook

2016-03-05 21:49 GMT+01:00 Joe Conway <mail(at)joeconway(dot)com>:

> On 03/01/2016 08:18 AM, Pavel Stehule wrote:
> > 2016-03-01 16:52 GMT+01:00 Joe Conway:
> > On 03/01/2016 02:09 AM, Pavel Stehule wrote:
> > > > On 01/06/2016 10:36 AM, Tom Lane wrote:
> > > >> I think a design that was actually somewhat robust would
> require two
> > > >> hooks, one at check_role and one at assign_role, wherein
> the first one
> > > >> would do any potentially-failing work and package all
> required info into
> > > >> a blob that could be passed through to the assign hook.
> >
> > > I see following issues:
> > >
> > > 1. Missing the possibility to pass custom data from
> SetRoleCheck_hook to
> > > SetRoleAssign_hook. Tom mentioned it in his comment.
> >
> > You can pass the data between the two plugin hook functions in your
> > extension itself, so I see no need to try to pass custom data through
> > the backend. Do any of the other hooks even do that?
> >
> > I don't know about it, but these hooks are specific. is it ensured a
> > order of calls of these hooks?
>
> > > 2. Missing little bit more comments and an explanation why and
> when to
> > > use these hooks.
> >
> > Doesn't look all that different from existing user hooks to me, but
> > sure, I'll add a bit more to the comments.
>
> > some like "I think the main point was to have two hooks. The
> > potentially-failing
> > work could be done during the check_role() hook and the collected info
> > could be used during the assign_role() hook."
>
> Ok, I added a comment similar to that at the check_role() function hook
> call site. Updated patch attached.
>

the comments are good enough now

>
> I still don't see any point in trying to pass data back from the hooks
> as the extension can maintain that state just fine, although it looks
> like it would be pretty trivial to do using a new void* member added to
> role_auth_extra. Tom (or anyone else), any comment on that?
>

see Tom's comment, I share his opinion.

>
> I do however find myself wishing I could pass the action down from
> set_config_option() to at least the assign_role() hook, but that seems
> more invasive than I'd like.
>

describe this use case, please.

In this case, I don't afraid of some possible API changes in future. This
API is "invisible" for common user. So it should not to handle all possible
use cases.

For me, the possibility to control private data is in category better to
have, the extension can be safer, smaller, but probably 75% of potential
customer will be happy from current state of this patch.

On the other hand, if we can do some simple changes (few or little bit more
than few lines), why don't do it now?

Pavel

>
> Joe
>
> --
> Crunchy Data - http://crunchydata.com
> PostgreSQL Support for Secure Enterprises
> Consulting, Training, & Open Source Development
>