Re: role self-revocation

Greetings,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > 1. What should be the exact rule for whether A can remove a grant made
> > by B? Is it has_privs_of_role()? is_member_of_role()? Something else?
>
> No strong opinion here, but I'd lean slightly to the more restrictive
> option.
>
> > 2. What happens if the same GRANT is enacted by multiple users? For
> > example, suppose peon does "GRANT peon to boss" and then the superuser
> > does the same thing afterwards, or vice versa? One design would be to
> > try to track those as two separate grants, but I'm not sure if we want
> > to add that much complexity, since that's not how we do it now and it
> > would, for example, implicate the choice of PK on the pg_auth_members
> > table.
>
> As you note later, we *do* track such grants separately in ordinary
> ACLs, and I believe this is clearly required by the SQL spec.

Agreed.

> It says (for privileges on objects):
>
> Each privilege is represented by a privilege descriptor.
> A privilege descriptor contains:
> — The identification of the object on which the privilege is granted.
> — The <authorization identifier> of the grantor of the privilege.
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> — The <authorization identifier> of the grantee of the privilege.
> — Identification of the action that the privilege allows.
> — An indication of whether or not the privilege is grantable.
> — An indication of whether or not the privilege has the WITH HIERARCHY OPTION specified.
>
> Further down (4.42.3 in SQL:2021), the granting of roles is described,
> and that says:
>
> Each role authorization is described by a role authorization descriptor.
> A role authorization descriptor includes:
> — The role name of the role.
> — The authorization identifier of the grantor.
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> — The authorization identifier of the grantee.
> — An indication of whether or not the role authorization is grantable.
>
> If we are not tracking the grantors of role authorizations,
> then we are doing it wrong and we ought to fix that.

Yup, and as noted elsewhere, we are tracking it but not properly dealing
with dependencies nor are we considering the grantor when REVOKE is run.

Looking at the spec for REVOKE is quite useful when trying to understand
how this is all supposed to work (and, admittedly, isn't something I did
enough of when I did the original work on roles... sorry about that, was
early on). In particular, a REVOKE only works when it finds something
to revoke/remove, and part of that search includes basically "was it the
current role who was the grantor?"

The specific language here being: A role authorization descriptor is
said to be identified if it defines the grant of any of the specified
roles revoked to grantee with grantor A.

Basically, a role authorization descriptor isn't identified unless it's
one that this user/role had previously granted.

> > 3. What happens if a user is dropped after being recorded as a
> > grantor?
>
> Should work the same as it does now for ordinary ACLs, ie, you
> gotta drop the grant first.

Agreed.

> > 4. Should we apply this rule to other types of grants, rather than
> > just to role membership?
>
> I am not sure about the reasoning behind the existing rule that
> superuser-granted privileges are recorded as being granted by the
> object owner. It does feel more like a wart than something we want.
> It might have been a hack to deal with the lack of GRANTED BY
> options in GRANT/REVOKE back in the day.

Yeah, that doesn't seem right and isn't great.

> Changing it could have some bad compatibility consequences though.
> In particular, I believe it would break existing pg_dump files,
> in that after restore all privileges would be attributed to the
> restoring superuser, and there'd be no very easy way to clean that
> up.

Ugh, that's pretty grotty, certainly.

> > Please note that it is not really my intention to try to shove
> > anything into v15 here.
>
> Agreed, this is not something to move on quickly. We might want
> to think about adjusting pg_dump to use explicit GRANTED BY
> options in GRANT/REVOKE a release or two before making incompatible
> changes.

I'm with Robert on this though- folks should know already that they need
to use the pg_dump of the version of PG that they want to move to and
not try to re-use older pg_dump output with newer versions, for a number
of reasons and this is just another.

Thanks,

Stephen