Re: pg_auth_members.grantor is bunk

On Sun, Jul 31, 2022 at 11:18 AM Stephen Frost <sfrost@snowman.net> wrote:

> Greetings,
>
> * Robert Haas (robertmhaas@gmail.com) wrote:
> > On Tue, Jul 26, 2022 at 12:46 PM Robert Haas <robertmhaas@gmail.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.


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).

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.


> 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.

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.

David J.