Re: BUG #17062: Assert failed in RemoveRoleFromObjectPolicy() on DROP OWNED policy applied to duplicate role

Greetings,

On Fri, Jun 18, 2021 at 14:18 Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Michael Paquier <michael@paquier.xyz> writes:
> > On Thu, Jun 17, 2021 at 05:51:18PM -0400, Tom Lane wrote:
> >> While I'm whining ... that function's permissions checks seem
> >> completely out of line too.  How is it that, if I have the right
> >> to drop some role, I lose that right if the role is mentioned in
> >> a policy of some relation I don't own?  It feels like this function
> >> was written by copy-and-pasting a whole bunch of irrelevant logic.
>
> > Hm.  Wouldn't it be better to do something similar to 21378e1f where
> > we ensure that there are no duplicated role OIDs in the catalogs to
> > begin with, letting the drop code as it is now?
>
> Well, we'd have to rewrite RemoveRoleFromObjectPolicy in the back
> branches in any case.  Furthermore, I don't think it's a great
> idea for this code to just die with an Assert failure if someone
> has modified the polroles array by hand.  I think we should just
> make it clean out however many matches there are.
>
> Stepping back a bit, I suspect that the weird permission rules
> here stem from not wanting to allow a policy to just disappear
> without the table owner's involvement.  There might be a fair
> amount of intellectual investment in the USING and WITH CHECK
> expressions, so I can sympathize with that point; but I don't
> sympathize with allowing it to block an otherwise-allowed role
> drop.  It seems like we could resolve this tension if we allowed
> the polroles array to go to empty rather than requiring the policy
> to be dropped when that would happen.  The main thing blocking
> that is the need to be able to represent that situation in
> CREATE/ALTER POLICY.  Maybe it'd work to allow "TO NONE" for
> that case?  (I hasten to add that I'm envisioning that as a future
> feature, not something to back-patch.  I think in the back branches
> we should just silently drop the policy.)


I haven’t had a chance to delve into this but as far as the question above
goes- short answer is yes, there was generally an idea that we don’t want
policies just disappearing.  Also- we don’t allow a role to be dropped when
there are GRANT’d privileges, users have to go REVOKE any privileges that
reference the role.

As far as the locking concerns go, I think if we get rid of the
> unnecessary reprocessing of the expression dependencies, we don't
> have to open or lock the associated table at all.  The operation
> would reduce to one UPDATE on the pg_policy row (plus maybe some
> deletions of pg_shdepend rows), and I think the existing handling
> of tuple update conflicts would then be good enough to cope with
> concurrent alter/drop of the policy.


This does sound appealing.

Thanks,

Stephen

>