Обсуждение: No warning for a no-op REVOKE

Right now, if you do a REVOKE that doesn't actually revoke anything, it works silently.  This can be a bit of a
foot-gun. For example: 

    CREATE FUNCTION f() RETURNS int as $$ SELECT 1; $$ LANGUAGE sql;
    REVOKE EXECUTE ON FUNCTION f() FROM lowpriv;

Naively, it might be expected that `lowpriv` can't execute the function, but unless default privileges have been
changed,`lowpriv` still can under the default grant of EXECUTE to PUBLIC.  Since there was no previous grant to
`lowpriv`,nothing actually changes in the ACL.  This bit a client recently. 

Is it worth generating a warning in this case?

> On 25 Mar 2024, at 14:54, Christophe Pettus <xof@thebuild.com> wrote:
>
> Right now, if you do a REVOKE that doesn't actually revoke anything, it works silently.  This can be a bit of a
foot-gun. For example: 
>
>     CREATE FUNCTION f() RETURNS int as $$ SELECT 1; $$ LANGUAGE sql;
>     REVOKE EXECUTE ON FUNCTION f() FROM lowpriv;
>
> Naively, it might be expected that `lowpriv` can't execute the function, but unless default privileges have been
changed,`lowpriv` still can under the default grant of EXECUTE to PUBLIC.  Since there was no previous grant to
`lowpriv`,nothing actually changes in the ACL.  This bit a client recently. 

That's indeed a potential foot-gun.

> Is it worth generating a warning in this case?

Or maybe a NOTICE?

--
Daniel Gustafsson

Christophe Pettus <xof@thebuild.com> writes:
> Right now, if you do a REVOKE that doesn't actually revoke anything, it works silently.  This can be a bit of a
foot-gun. For example: 
>     CREATE FUNCTION f() RETURNS int as $$ SELECT 1; $$ LANGUAGE sql;
>     REVOKE EXECUTE ON FUNCTION f() FROM lowpriv;

> Naively, it might be expected that `lowpriv` can't execute the function, but unless default privileges have been
changed,`lowpriv` still can under the default grant of EXECUTE to PUBLIC.  Since there was no previous grant to
`lowpriv`,nothing actually changes in the ACL.  This bit a client recently. 

> Is it worth generating a warning in this case?

To be clear, even if there had been a grant of EXECUTE to lowpriv,
that role would still be able to call the function thanks to the
public EXECUTE permission.  I'm not sure that warning about whether
anything got revoked is going to help people who fundamentally
misunderstand where the privilege is coming from.  Still, I take
your point that maybe the command should mention that nothing
happened.  To me the best argument for producing a warning is that
we already do so for grants of roles:

regression=# create user alice;
CREATE ROLE
regression=# create role bob;
CREATE ROLE
regression=# revoke bob from alice;
WARNING:  role "alice" has not been granted membership in role "bob" by role "postgres"
REVOKE ROLE

However, ordinary privileges are a bit more complicated since
multiple privilege bits can be specified:

regression=# revoke select,update on table t from alice;
REVOKE

or even

regression=# revoke select(x),update(y) on table t from alice;
REVOKE

Should we generate a warning when only some of the named privileges
exist to be revoked?

My initial reaction is that we should warn only when the command
is a complete no-op, that is none of the mentioned privileges
matched.  But I've not thought about it very hard.

            regards, tom lane

> On 25 Mar 2024, at 15:09, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> My initial reaction is that we should warn only when the command
> is a complete no-op, that is none of the mentioned privileges
> matched.

That's my gut reaction too, 

--
Daniel Gustafsson

> On Mar 25, 2024, at 07:20, Daniel Gustafsson <daniel@yesql.se> wrote:
>
>> On 25 Mar 2024, at 15:09, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
>> My initial reaction is that we should warn only when the command
>> is a complete no-op, that is none of the mentioned privileges
>> matched.
>
> That's my gut reaction too,

I think that's fine.  The all-singing-all-dancing solution would be to warn if the role retains any of the mentioned
privilegesfor some other reason, as in: 

    WARNING: role "lowpriv" still has EXECUTE permission on "f()" via a grant to role "PUBLIC" by role "owner"

... but I suspect the implementation complexity there isn't trivial.