Fix pg_dump for GRANT OPTION among initial privileges.

The context is an object that no longer bears some aclitem that it bore
initially.  (A user issued REVOKE or GRANT statements upon the object.)
pg_dump is forming SQL to reproduce the object ACL.  Since initdb
creates no ACL bearing GRANT OPTION, reaching this bug requires an
extension where the creation script establishes such an ACL.  No PGXN
extension does that.  If an installation did reach the bug, pg_dump
would have omitted a semicolon, causing a REVOKE and the next SQL
statement to fail.  Separately, since the affected code exists to
eliminate an entire aclitem, it wants plain REVOKE, not REVOKE GRANT
OPTION FOR.  Back-patch to 9.6, where commit
23f34fa4ba358671adab16773e79c17c92cbc870 first appeared.

Discussion: https://postgr.es/m/20210109102423[email protected]

diff --git a/src/bin/pg_dump/dumputils.c b/src/bin/pg_dump/dumputils.c
index 287d485d5f7124f46434df07f5cba03ed209a780..2de0cef40aa5e636b4ce42af54baced1532a570b 100644 (file)
--- a/src/bin/pg_dump/dumputils.c
+++ b/src/bin/pg_dump/dumputils.c
@@ -168,48 +168,28 @@ buildACLCommands(const char *name, const char *subname, const char *nspname,
        for (i = 0; i < nraclitems; i++)
        {
            if (!parseAclItem(raclitems[i], type, name, subname, remoteVersion,
-                             grantee, grantor, privs, privswgo))
+                             grantee, grantor, privs, NULL))
            {
                ok = false;
                break;
            }
 
-           if (privs->len > 0 || privswgo->len > 0)
+           if (privs->len > 0)
            {
-               if (privs->len > 0)
-               {
-                   appendPQExpBuffer(firstsql, "%sREVOKE %s ON %s ",
-                                     prefix, privs->data, type);
-                   if (nspname && *nspname)
-                       appendPQExpBuffer(firstsql, "%s.", fmtId(nspname));
-                   appendPQExpBuffer(firstsql, "%s FROM ", name);
-                   if (grantee->len == 0)
-                       appendPQExpBufferStr(firstsql, "PUBLIC;\n");
-                   else if (strncmp(grantee->data, "group ",
-                                    strlen("group ")) == 0)
-                       appendPQExpBuffer(firstsql, "GROUP %s;\n",
-                                         fmtId(grantee->data + strlen("group ")));
-                   else
-                       appendPQExpBuffer(firstsql, "%s;\n",
-                                         fmtId(grantee->data));
-               }
-               if (privswgo->len > 0)
-               {
-                   appendPQExpBuffer(firstsql,
-                                     "%sREVOKE GRANT OPTION FOR %s ON %s ",
-                                     prefix, privswgo->data, type);
-                   if (nspname && *nspname)
-                       appendPQExpBuffer(firstsql, "%s.", fmtId(nspname));
-                   appendPQExpBuffer(firstsql, "%s FROM ", name);
-                   if (grantee->len == 0)
-                       appendPQExpBufferStr(firstsql, "PUBLIC");
-                   else if (strncmp(grantee->data, "group ",
-                                    strlen("group ")) == 0)
-                       appendPQExpBuffer(firstsql, "GROUP %s",
-                                         fmtId(grantee->data + strlen("group ")));
-                   else
-                       appendPQExpBufferStr(firstsql, fmtId(grantee->data));
-               }
+               appendPQExpBuffer(firstsql, "%sREVOKE %s ON %s ",
+                                 prefix, privs->data, type);
+               if (nspname && *nspname)
+                   appendPQExpBuffer(firstsql, "%s.", fmtId(nspname));
+               appendPQExpBuffer(firstsql, "%s FROM ", name);
+               if (grantee->len == 0)
+                   appendPQExpBufferStr(firstsql, "PUBLIC;\n");
+               else if (strncmp(grantee->data, "group ",
+                                strlen("group ")) == 0)
+                   appendPQExpBuffer(firstsql, "GROUP %s;\n",
+                                     fmtId(grantee->data + strlen("group ")));
+               else
+                   appendPQExpBuffer(firstsql, "%s;\n",
+                                     fmtId(grantee->data));
            }
        }
    }
@@ -462,8 +442,11 @@ buildDefaultACLCommands(const char *type, const char *nspname,
  * The returned grantee string will be the dequoted username or groupname
  * (preceded with "group " in the latter case).  Note that a grant to PUBLIC
  * is represented by an empty grantee string.  The returned grantor is the
- * dequoted grantor name.  Privilege characters are decoded and split between
- * privileges with grant option (privswgo) and without (privs).
+ * dequoted grantor name.  Privilege characters are translated to GRANT/REVOKE
+ * comma-separated privileges lists.  If "privswgo" is non-NULL, the result is
+ * separate lists for privileges with grant option ("privswgo") and without
+ * ("privs").  Otherwise, "privs" bears every relevant privilege, ignoring the
+ * grant option distinction.
  *
  * Note: for cross-version compatibility, it's important to use ALL to
  * represent the privilege sets whenever appropriate.
@@ -514,7 +497,7 @@ parseAclItem(const char *item, const char *type,
 do { \
    if ((pos = strchr(eqpos + 1, code))) \
    { \
-       if (*(pos + 1) == '*') \
+       if (*(pos + 1) == '*' && privswgo != NULL) \
        { \
            AddAcl(privswgo, keywd, subname); \
            all_without_go = false; \

diff --git a/src/test/modules/test_pg_dump/t/001_base.pl b/src/test/modules/test_pg_dump/t/001_base.pl
index b3227b855c0a90687a86d47e92e3d13111671392..501aff09201a4ea00d5ce7f543b047c89214a127 100644 (file)
--- a/src/test/modules/test_pg_dump/t/001_base.pl
+++ b/src/test/modules/test_pg_dump/t/001_base.pl
@@ -348,6 +348,45 @@ my %tests = (
        },
    },
 
+   'REVOKE ALL ON FUNCTION wgo_then_no_access' => {
+       create_order => 3,
+       create_sql   => q{
+           DO $$BEGIN EXECUTE format(
+               'REVOKE ALL ON FUNCTION wgo_then_no_access()
+                FROM pg_signal_backend, public, %I',
+               (SELECT usename
+                FROM pg_user JOIN pg_proc ON proowner = usesysid
+                WHERE proname = 'wgo_then_no_access')); END$$;},
+       regexp => qr/^
+           \QREVOKE ALL ON FUNCTION public.wgo_then_no_access() FROM PUBLIC;\E
+           \n\QREVOKE ALL ON FUNCTION public.wgo_then_no_access() FROM \E.*;
+           \n\QREVOKE ALL ON FUNCTION public.wgo_then_no_access() FROM pg_signal_backend;\E
+           /xm,
+       like => {
+           %full_runs,
+           schema_only      => 1,
+           section_pre_data => 1,
+       },
+       unlike => { no_privs => 1, },
+   },
+
+   'REVOKE GRANT OPTION FOR UPDATE ON SEQUENCE wgo_then_regular' => {
+       create_order => 3,
+       create_sql   => 'REVOKE GRANT OPTION FOR UPDATE ON SEQUENCE
+                           wgo_then_regular FROM pg_signal_backend;',
+       regexp => qr/^
+           \QREVOKE ALL ON SEQUENCE public.wgo_then_regular FROM pg_signal_backend;\E
+           \n\QGRANT SELECT,UPDATE ON SEQUENCE public.wgo_then_regular TO pg_signal_backend;\E
+           \n\QGRANT USAGE ON SEQUENCE public.wgo_then_regular TO pg_signal_backend WITH GRANT OPTION;\E
+           /xm,
+       like => {
+           %full_runs,
+           schema_only      => 1,
+           section_pre_data => 1,
+       },
+       unlike => { no_privs => 1, },
+   },
+
    'CREATE ACCESS METHOD regress_test_am' => {
        regexp => qr/^
            \QCREATE ACCESS METHOD regress_test_am TYPE INDEX HANDLER bthandler;\E

diff --git a/src/test/modules/test_pg_dump/test_pg_dump--1.0.sql b/src/test/modules/test_pg_dump/test_pg_dump--1.0.sql
index c7a35c3afa0f9534d920a3b19010116910eded27..110f7eef66c56ba8855edd7fdd457b5a8f3599f2 100644 (file)
--- a/src/test/modules/test_pg_dump/test_pg_dump--1.0.sql
+++ b/src/test/modules/test_pg_dump/test_pg_dump--1.0.sql
@@ -28,6 +28,15 @@ GRANT SELECT(col1) ON regress_pg_dump_table TO public;
 GRANT SELECT(col2) ON regress_pg_dump_table TO regress_dump_test_role;
 REVOKE SELECT(col2) ON regress_pg_dump_table FROM regress_dump_test_role;
 
+CREATE FUNCTION wgo_then_no_access() RETURNS int LANGUAGE SQL AS 'SELECT 1';
+GRANT ALL ON FUNCTION wgo_then_no_access()
+   TO pg_signal_backend WITH GRANT OPTION;
+
+CREATE SEQUENCE wgo_then_regular;
+GRANT ALL ON SEQUENCE wgo_then_regular TO pg_signal_backend WITH GRANT OPTION;
+REVOKE GRANT OPTION FOR SELECT ON SEQUENCE wgo_then_regular
+   FROM pg_signal_backend;
+
 CREATE ACCESS METHOD regress_test_am TYPE INDEX HANDLER bthandler;
 
 -- Create a set of objects that are part of the schema created by