Allow makeaclitem() to accept multiple privileges

Started by Tharakan, Robinsover 3 years ago2 messages
#1Tharakan, Robins
tharar@amazon.com
2 attachment(s)

Hi,

Presently, makeaclitem() allows only a single privilege in a single call.
This
patch allows it to additionally accept multiple comma-separated privileges.

The attached patch reuses the has_foo_privileges() infrastructure and
besides
a minor change to the function documentation, it also adds 3 regression
tests
that increase the function code-coverage to 100%.

Sample usage:

postgres=# SELECT makeaclitem('postgres'::regrole, 'usr1'::regrole,
'SELECT, INSERT, UPDATE, ALTER SYSTEM', FALSE);
makeaclitem
--------------------
postgres=arwA/usr1
(1 row)

The need for this patch came up during a recent customer experience, where
'pg_init_privs.initprivs' had grantees pointing to non-existent roles. This
is
easy to reproduce [5] and given that this issue was blocking the
customer's planned upgrade, the temporary solution was to UPDATE the
initprivs column. From what I could see, there was a fix for similar
issues [1], although that didn't fix this specific issue [2] and thus
manually
modifying initprivs was required. For this manual update though, if the
proposed feature was available, it would have helped with the UPDATE SQLs.

To elaborate the customer issue, in most rows aclitems[]::TEXT was 2000+
characters long and spanned 30+ missing roles and multiple databases. In
trying
to automate the generation of UPDATE SQLs, I tried to use aclexplode() to
filter-out the missing grantee roles, however re-stitching the remaining
items
back into an aclitems[] array was non-trivial, since makeaclitem() doesn't
yet
accept multiple privileges in a single call. In particular, the
unnest() + string-search approach mentioned in this thread [4] didn't scale
with many missing roles where rolenames were alphanumeric.

See [6] for a contrived example where the updated makeaclitem() can be used
to
regenerate the initprivs column, weeding out privileges related to missing
grantees.

Lastly, while researching, I saw a thread [3] questioning whether
makeaclitem() is useful, and think that if it were to allow multiple
privileges,
it could have helped in the above situation, and thus I'd -1 on dropping the
function.

Reference:
1)
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=47088c599
cc6d6473c7b89ac029060525cf086d8

2)
/messages/by-id/29913.1573835973@sss.pgh.pa.us
90b0192c126ea61266e31dbb864c9b08

3)
/messages/by-id/48f9156d-3937-cf47-13ee-ac4e90c83
c43%40postgresfriends.org#7f5c830819bc104c222b440689d2028f

4)
/messages/by-id/1573808483712.96817@Optiver.com

5) Reproduction to get pg_init_privs.initprivs to point to missing roles.

psql -c "CREATE DATABASE w" postgres
export PGDATABASE=w;
psql -c "CREATE USER usr1 PASSWORD 'usr1' SUPERUSER;"
psql -U usr1 -c "CREATE EXTENSION pg_stat_statements" -c "SELECT * FROM
pg_init_privs WHERE privtype = 'e'" -c "REASSIGN OWNED BY usr1 TO postgres;"
psql -c "DROP USER usr1" -c "SELECT * FROM pg_init_privs WHERE privtype =
'e';"
.
.
.
This would end up with something like this:

r=# select * from pg_init_privs where privtype = 'e';
objoid | classoid | objsubid | privtype |
initprivs
--------+----------+----------+----------+----------------------------------
-------------------------------------
16433 | 1255 | 0 | e | {16425=X/16425}
16441 | 1259 | 0 | e |
{16425=arwdDxt/16425,=r/16425,t=r/postgres}
16452 | 1259 | 0 | e |
{16425=arwdDxt/16425,=r/16425,uw22341=arwdDxt/postgres,t=rw/postgres}
(3 rows)

6) This feature can then be used to generate the UPDATE SQLs to fix the
issue:

r=# BEGIN;
BEGIN
r=*# WITH a AS (SELECT pg_init_privs.* ,(aclexplode(initprivs)).* FROM
pg_init_privs WHERE privtype = 'e')
r-*# SELECT DISTINCT 'UPDATE pg_init_privs SET initprivs = ''{' || (
r(*# WITH x AS (
r(*# SELECT (aclexplode(initprivs)).*
r(*# FROM pg_init_privs
r(*# WHERE objoid = a.objoid AND classoid = a.classoid AND objsubid
= a.objsubid
r(*# )
r(*# ,y AS (
r(*# SELECT makeaclitem(grantee, grantor, string_agg(privilege_type,
','), is_grantable) p
r(*# FROM x
r(*# WHERE grantee IN (SELECT oid FROM pg_roles)
r(*# GROUP BY grantee,grantor,is_grantable
r(*# )
r(*# SELECT string_agg(p::TEXT, ',') FROM y
r(*# ) || '}'' WHERE objoid=' || a.objoid || ' AND classoid=' ||
a.classoid || ' AND objsubid=' || a.objsubid || ';'
r-*# FROM a;
?column?
----------------------------------------------------------------------------
----------------------------------------------------------
UPDATE pg_init_privs SET initprivs = '{t=r/postgres}' WHERE objoid=16441
AND classoid=1259 AND objsubid=0;
UPDATE pg_init_privs SET initprivs =
'{uw22341=arwdDxt/postgres,t=rw/postgres}' WHERE objoid=16452 AND
classoid=1259 AND objsubid=0;

(3 rows)

r=*# \gexec
UPDATE 1
UPDATE 1
r=*# select * from pg_init_privs where privtype = 'e';
objoid | classoid | objsubid | privtype | initprivs
--------+----------+----------+----------+----------------------------------
--------
16433 | 1255 | 0 | e | {16425=X/16425}
16441 | 1259 | 0 | e | {t=r/postgres}
16452 | 1259 | 0 | e |
{uw22341=arwdDxt/postgres,t=rw/postgres}
(3 rows)

(Similarly, it should be possible to generate DELETE SQLs for rows with
*all*
ACL objects pointing to missing roles - for e.g. row 1 above).

-
Robins Tharakan
Amazon Web Services

Attachments:

v1_makeaclitem_multiple_privileges.patchapplication/octet-stream; name=v1_makeaclitem_multiple_privileges.patchDownload
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index e52e2d953b..d2ed0cecc1 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -24256,6 +24256,11 @@ SELECT has_function_privilege('joeuser', 'myfunc(int, text)', 'execute');
        </para>
        <para>
         Constructs an <type>aclitem</type> with the given properties.
+        Multiple privilege types can be listed separated by commas, in which
+        case all privileges would be set in the <type>aclitem</type>.
+        (As with <type>has_foo_privilege()</type> set of functions,
+        case of the privilege string is not significant, and extra whitespace
+        is allowed between but not within privilege names)
        </para></entry>
       </row>
      </tbody>
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index 772c04155c..f127cb92d4 100644
--- a/src/backend/utils/adt/acl.c
+++ b/src/backend/utils/adt/acl.c
@@ -86,7 +86,6 @@ static void check_circularity(const Acl *old_acl, const AclItem *mod_aip,
 static Acl *recursive_revoke(Acl *acl, Oid grantee, AclMode revoke_privs,
 							 Oid ownerId, DropBehavior behavior);
 
-static AclMode convert_priv_string(text *priv_type_text);
 static AclMode convert_any_priv_string(text *priv_type_text,
 									   const priv_map *privileges);
 
@@ -1573,8 +1572,27 @@ makeaclitem(PG_FUNCTION_ARGS)
 	bool		goption = PG_GETARG_BOOL(3);
 	AclItem    *result;
 	AclMode		priv;
+	static const priv_map table_priv_map[] = {
+		{"SELECT", ACL_SELECT},
+		{"INSERT", ACL_INSERT},
+		{"UPDATE", ACL_UPDATE},
+		{"DELETE", ACL_DELETE},
+		{"TRUNCATE", ACL_TRUNCATE},
+		{"REFERENCES", ACL_REFERENCES},
+		{"TRIGGER", ACL_TRIGGER},
+		{"EXECUTE", ACL_EXECUTE},
+		{"USAGE", ACL_USAGE},
+		{"CREATE", ACL_CREATE},
+		{"TEMP", ACL_CREATE_TEMP},
+		{"TEMPORARY", ACL_CREATE_TEMP},
+		{"CONNECT", ACL_CONNECT},
+		{"SET", ACL_SET},
+		{"ALTER SYSTEM", ACL_ALTER_SYSTEM},
+		{"RULE", 0},			/* ignore old RULE privileges */
+		{NULL, 0}
+	};
 
-	priv = convert_priv_string(privtext);
+	priv = convert_any_priv_string(privtext, table_priv_map);
 
 	result = (AclItem *) palloc(sizeof(AclItem));
 
@@ -1587,51 +1605,6 @@ makeaclitem(PG_FUNCTION_ARGS)
 	PG_RETURN_ACLITEM_P(result);
 }
 
-static AclMode
-convert_priv_string(text *priv_type_text)
-{
-	char	   *priv_type = text_to_cstring(priv_type_text);
-
-	if (pg_strcasecmp(priv_type, "SELECT") == 0)
-		return ACL_SELECT;
-	if (pg_strcasecmp(priv_type, "INSERT") == 0)
-		return ACL_INSERT;
-	if (pg_strcasecmp(priv_type, "UPDATE") == 0)
-		return ACL_UPDATE;
-	if (pg_strcasecmp(priv_type, "DELETE") == 0)
-		return ACL_DELETE;
-	if (pg_strcasecmp(priv_type, "TRUNCATE") == 0)
-		return ACL_TRUNCATE;
-	if (pg_strcasecmp(priv_type, "REFERENCES") == 0)
-		return ACL_REFERENCES;
-	if (pg_strcasecmp(priv_type, "TRIGGER") == 0)
-		return ACL_TRIGGER;
-	if (pg_strcasecmp(priv_type, "EXECUTE") == 0)
-		return ACL_EXECUTE;
-	if (pg_strcasecmp(priv_type, "USAGE") == 0)
-		return ACL_USAGE;
-	if (pg_strcasecmp(priv_type, "CREATE") == 0)
-		return ACL_CREATE;
-	if (pg_strcasecmp(priv_type, "TEMP") == 0)
-		return ACL_CREATE_TEMP;
-	if (pg_strcasecmp(priv_type, "TEMPORARY") == 0)
-		return ACL_CREATE_TEMP;
-	if (pg_strcasecmp(priv_type, "CONNECT") == 0)
-		return ACL_CONNECT;
-	if (pg_strcasecmp(priv_type, "SET") == 0)
-		return ACL_SET;
-	if (pg_strcasecmp(priv_type, "ALTER SYSTEM") == 0)
-		return ACL_ALTER_SYSTEM;
-	if (pg_strcasecmp(priv_type, "RULE") == 0)
-		return 0;				/* ignore old RULE privileges */
-
-	ereport(ERROR,
-			(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
-			 errmsg("unrecognized privilege type: \"%s\"", priv_type)));
-	return ACL_NO_RIGHTS;		/* keep compiler quiet */
-}
-
-
 /*
  * convert_any_priv_string: recognize privilege strings for has_foo_privilege
  *
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 03df567d50..003bf28468 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -2099,6 +2099,24 @@ SELECT has_table_privilege('regress_priv_user1', 'testns.acltest1', 'INSERT'); -
 ALTER DEFAULT PRIVILEGES FOR ROLE regress_priv_user1 REVOKE EXECUTE ON FUNCTIONS FROM public;
 ALTER DEFAULT PRIVILEGES IN SCHEMA testns GRANT USAGE ON SCHEMAS TO regress_priv_user2; -- error
 ERROR:  cannot use IN SCHEMA clause when using GRANT/REVOKE ON SCHEMAS
+-- Test makeaclitem()
+SELECT makeaclitem('regress_priv_user1'::regrole, 'regress_priv_user2'::regrole, 'SELECT', TRUE); -- single privilege
+               makeaclitem                
+------------------------------------------
+ regress_priv_user1=r*/regress_priv_user2
+(1 row)
+
+SELECT makeaclitem('regress_priv_user1'::regrole, 'regress_priv_user2'::regrole,
+	'SELECT, INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES, TRIGGER, EXECUTE, USAGE, CREATE,
+	TEMP, TEMPORARY, CONNECT, SET, ALTER SYSTEM, RULE ', FALSE); -- multiple privileges
+                     makeaclitem                      
+------------------------------------------------------
+ regress_priv_user1=arwdDxtXUCTcsA/regress_priv_user2
+(1 row)
+
+SELECT makeaclitem('regress_priv_user1'::regrole, 'regress_priv_user2'::regrole,
+	'SELECT, fake_privilege ', FALSE); -- error
+ERROR:  unrecognized privilege type: "fake_privilege"
 --
 -- Testing blanket default grants is very hazardous since it might change
 -- the privileges attached to objects created by concurrent regression tests.
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 2a6ba38e52..65c32211e2 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -1339,6 +1339,15 @@ ALTER DEFAULT PRIVILEGES FOR ROLE regress_priv_user1 REVOKE EXECUTE ON FUNCTIONS
 
 ALTER DEFAULT PRIVILEGES IN SCHEMA testns GRANT USAGE ON SCHEMAS TO regress_priv_user2; -- error
 
+
+-- Test makeaclitem()
+SELECT makeaclitem('regress_priv_user1'::regrole, 'regress_priv_user2'::regrole, 'SELECT', TRUE); -- single privilege
+SELECT makeaclitem('regress_priv_user1'::regrole, 'regress_priv_user2'::regrole,
+	'SELECT, INSERT, UPDATE, DELETE, TRUNCATE, REFERENCES, TRIGGER, EXECUTE, USAGE, CREATE,
+	TEMP, TEMPORARY, CONNECT, SET, ALTER SYSTEM, RULE ', FALSE); -- multiple privileges
+SELECT makeaclitem('regress_priv_user1'::regrole, 'regress_priv_user2'::regrole,
+	'SELECT, fake_privilege ', FALSE); -- error
+
 --
 -- Testing blanket default grants is very hazardous since it might change
 -- the privileges attached to objects created by concurrent regression tests.
smime.p7sapplication/pkcs7-signature; name=smime.p7sDownload
#2Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tharakan, Robins (#1)
Re: Allow makeaclitem() to accept multiple privileges

"Tharakan, Robins" <tharar@amazon.com> writes:

Presently, makeaclitem() allows only a single privilege in a single call.
This
patch allows it to additionally accept multiple comma-separated privileges.

Seems reasonable. Pushed with minor cosmetic adjustments (mostly
docs changes).

regards, tom lane