Attached is a minimal patch to allow missing roles in REVOKE command
This should fix the pg_upgrade issue and also a case where somebody
has dropped a role you are trying to revoke privileges from :
smalltest=# create table revoketest();
CREATE TABLE
smalltest=# revoke select on revoketest from bob;
WARNING: ignoring REVOKE FROM a missing role "bob"
REVOKE
smalltest=# create user bob;
CREATE ROLE
smalltest=# grant select on revoketest to bob;
GRANT
smalltest=# \du
List of roles
Role name | Attributes
-----------+------------------------------------------------------------
bob |
hannuk | Superuser, Create role, Create DB, Replication, Bypass RLS
smalltest=# \dp
Access privileges
Schema | Name | Type | Access privileges | Column
privileges | Policies
--------+------------+-------+------------------------+-------------------+----------
public | revoketest | table | hannuk=arwdDxtm/hannuk+| |
| | | bob=r/hannuk | |
public | vacwatch | table | | |
(2 rows)
smalltest=# revoke select on revoketest from bob, joe;
WARNING: ignoring REVOKE FROM a missing role "joe"
REVOKE
smalltest=# \dp
Access privileges
Schema | Name | Type | Access privileges | Column
privileges | Policies
--------+------------+-------+------------------------+-------------------+----------
public | revoketest | table | hannuk=arwdDxtm/hannuk | |
public | vacwatch | table | | |
(2 rows)
On Sun, May 26, 2024 at 12:05 AM Hannu Krosing <[email protected]> wrote:
>
> On Sat, May 25, 2024 at 4:48 PM Tom Lane <[email protected]> wrote:
> >
> > Hannu Krosing <[email protected]> writes:
> > > Having an pg_init_privs entry referencing a non-existing user is
> > > certainly of no practical use.
> >
> > Sure, that's not up for debate. What I think we're discussing
> > right now is
> >
> > 1. What other cases are badly handled by the pg_init_privs
> > mechanisms.
> >
> > 2. How much of that is practical to fix in v17, seeing that
> > it's all long-standing bugs and we're already past beta1.
> >
> > I kind of doubt that the answer to #2 is "all of it".
> > But perhaps we can do better than "none of it".
>
> Putting the fix either in pg_dump or making REVOKE tolerate
> non-existing users would definitely be most practical / useful fixes,
> as these would actually allow pg_upgrade to v17 to work without
> changing anything in older versions.
>
> Currently one already can revoke a privilege that is not there in the
> first place, with the end state being that the privilege (still) does
> not exist.
> This does not even generate a warning.
>
> Extending this to revoking from users that do not exist does not seem
> any different on conceptual level, though I understand that
> implementation would be very different as it needs catching the user
> lookup error from a very different part of the code.
>
> That said, it would be better if we can have something that would be
> easy to backport something that would make pg_upgrade work for all
> supported versions.
> Making REVOKE silently ignore revoking from non-existing users would
> improve general robustness but could conceivably change behaviour if
> somebody relies on it in their workflows.
>
> Regards,
> Hannu
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index 7abf3c2a74..aff91582d7 100644
--- a/src/backend/catalog/aclchk.c
+++ b/src/backend/catalog/aclchk.c
@@ -444,6 +444,9 @@ ExecuteGrantStmt(GrantStmt *stmt)
* Convert the RoleSpec list into an Oid list. Note that at this point we
* insert an ACL_ID_PUBLIC into the list if appropriate, so downstream
* there shouldn't be any additional work needed to support this case.
+ *
+ * Allow missing grantees in case of REVOKE (!istmt.is_grant)
+ * if now valid roles found return immediately
*/
foreach(cell, stmt->grantees)
{
@@ -456,12 +459,20 @@ ExecuteGrantStmt(GrantStmt *stmt)
grantee_uid = ACL_ID_PUBLIC;
break;
default:
- grantee_uid = get_rolespec_oid(grantee, false);
+ grantee_uid = get_rolespec_oid(grantee, !istmt.is_grant);
break;
}
- istmt.grantees = lappend_oid(istmt.grantees, grantee_uid);
+ if (OidIsValid(grantee_uid))
+ istmt.grantees = lappend_oid(istmt.grantees, grantee_uid);
+ else
+ ereport(WARNING,
+ (errcode(ERRCODE_INVALID_ROLE_SPECIFICATION),
+ errmsg("ignoring REVOKE FROM a missing role \"%s\"", grantee->rolename)));
}
+ if (istmt.grantees == NIL)
+ return;
+
/*
* Convert stmt->privileges, a list of AccessPriv nodes, into an AclMode
* bitmask. Note: objtype can't be OBJECT_COLUMN.
