Re: pg_auth_members.grantor is bunk

Greetings,

On Sun, Jul 31, 2022 at 11:44 David G. Johnston <david(dot)g(dot)johnston(at)gmail(dot)com>
wrote:

> On Sun, Jul 31, 2022 at 11:18 AM Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>
>> Greetings,
>>
>> * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
>> > On Tue, Jul 26, 2022 at 12:46 PM Robert Haas <robertmhaas(at)gmail(dot)com>
>> wrote:
>>
>> + }
>> +
>> + /*
>> + * Disallow attempts to grant ADMIN OPTION back to a user who
>> granted it
>> + * to you, similar to what check_circularity does for ACLs. We
>> want the
>> + * chains of grants to remain acyclic, so that it's always
>> possible to use
>> + * REVOKE .. CASCADE to clean up all grants that depend on the
>> one being
>> + * revoked.
>> + *
>> + * NB: This check might look redundant with the check for
>> membership
>> + * loops above, but it isn't. That's checking for role-member
>> loop (e.g.
>> + * A is a member of B and B is a member of A) while this is
>> checking for
>> + * a member-grantor loop (e.g. A gave ADMIN OPTION to X to B and
>> now B, who
>> + * has no other source of ADMIN OPTION on X, tries to give ADMIN
>> OPTION
>> + * on X back to A).
>> + */
>>
>> With this exact scenario, wouldn't it just be a no-op as A must have
>> ADMIN OPTION already on X? The spec says that no cycles of role
>> authorizations are allowed.
>
>
I’ve realized that what I hadn’t been contemplating here is actually that
the GRANT from B to A for X wouldn’t be redundant because grantor is part
of the key (A got the right from someone else, but this would be giving it
to A from B and therefore would be distinct and would also create a loop
which is no good). Haven’t got a good idea on how to improve on the
comment based off of that though it still feels like it could be clearer.
If I think of something, I’ll share it.

Role A must have admin option for X to grant membership in X (with or
> without admin option) to B. But that doesn't preclude A from getting
> another admin option from someone else. That someone else cannot be
> someone to whom they gave admin option to however. So B cannot grant admin
> option back to A but role P could if it was basically a sibling of A (i.e.,
> both getting their initial admin option from someone else).
>

Right but that wasn’t what I had been trying to get at above.

If they do have admin option twice it should be possible to drop one of
> them, the prohibition should be on dropping the only admin option
> permission a role has for some other role. The commit message for 2
> contemplates this though I haven't gone through the revocation code in
> detail.
>

Yes, think I agree with this also- if A has been given the WITH ADMIN right
from Q and P to GRANT X to other roles, and uses that to GRANT X to B, then
the GRANT of X to B should be retained even if Q decides to revoke their
GRANT as A still has the right from P. If both remove the right, however,
either B should lose the right (if CASCADE was passed in) or an error
should be returned saying that there’s a dependent GRANT and CASCADE wasn’t
given.

I'm trying to get this review out sooner than later and so I might be
>> missing something, but looking at the regression test for this and these
>> error messages, feels like the 'circular' error message makes more sense
>> than the 'your own grantor' message that actually ends up being
>> returned in that regression test.
>>
>
> Having a more specific error seems reasonable, faster to track down what
> the problem is.
>

Yeah, but also making sure that all the error messages we have in this area
are in the regression test output would be good.

Makes me wonder if we might try to figure out a way to globally check for
that. I suppose one could review coverage.p.o for any ereport() calls that
aren’t ever called. I wonder what that would turn up.

I think that the whole graph dynamic of this might need some presentation
> work (messages and/or psql and/or functions) ; but assuming the errors are
> handled improved messages and/or presentation of graphs can be a separate
> enhancement.
>

Yes, we can further improve this later too but that doesn’t mean we should
just commit this as-is when some deficiencies have been pointed out. If the
only comments were “would be good to improve this error message but I
haven’t got a great idea how”, then sure, but there were other items
pointed out which were clear corrections and we should make sure to cover
in the regression tests all these scenarios that we are checking for and
erroring on, lest we end up breaking them unintentionally later.

Thanks,

Stephen

>