From: Michael Paquier <michael@paquier.xyz>
To: Joel Jacobson <joel@compiler.org>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Postgres hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: pgsql: Add pg_get_acl() to get the ACL for a database object
Date: Mon, 8 Jul 2024 17:34:18 +0900
Message-ID: <ZoukijRBef4m-VMO@paquier.xyz> (raw)
In-Reply-To: <f2539bff-64be-47f0-9f0b-df85d3cc0432@app.fastmail.com>
References: <E1sPHWu-000GPc-0z@gemulon.postgresql.org>
<3408967.1720107891@sss.pgh.pa.us>
<f2688fb7-add6-43da-b1df-97312a451893@app.fastmail.com>
<ZoctyZzE3UlUSer_@paquier.xyz>
<f2539bff-64be-47f0-9f0b-df85d3cc0432@app.fastmail.com>
On Fri, Jul 05, 2024 at 10:40:39AM +0200, Joel Jacobson wrote:
> OK, I made an attempt to implement this, based on adapting code from
> recordExtObjInitPriv() in aclchk.c, which retrieves ACL based on ATTNUM.
>
> There are now two different code paths for columns and non-columns.
>
> It sounds like a bigger refactoring in this area would be nice,
> which would enable cleaning up code in other functions as well,
> but maybe that's better to do as a separate project.
Thanks for the patch. I have been looking at it for a few hours,
eyeing a bit on the ObjectProperty parts a bit if we were to extend it
for sub-object IDs, and did not like the complexity this introduces,
so I'd be OK to live with the extra handling in pg_get_acl() itself.
+ /* ignore dropped columns */
+ if (atttup->attisdropped)
+ {
+ ReleaseSysCache(tup);
+ PG_RETURN_NULL();
+ }
Hmm. This is an important bit and did not consider it first. That
makes the use of ObjectProperty less flexible because we want to look
at the data in the pg_attribute tuple to be able to return NULL in
this case.
I've tweaked a bit what you are proposing, simplifying the code and
removing the first batch of queries in the tests as these were less
interesting. How does that look?
--
Michael
Attachments:v2-0001-Extend-pg_get_acl-to-handle-sub-object-IDs.patch (text/x-diff, 9.4K) download
From 07b65a78fb660f6a27ef464870a3e27f788c5e28 Mon Sep 17 00:00:00 2001
From: Michael Paquier <michael@paquier.xyz>
Date: Mon, 8 Jul 2024 16:24:42 +0900
Subject: [PATCH v2] Extend pg_get_acl to handle sub-object IDs.
This patch modifies the pg_get_acl function to accept a third objsubid
param. This enables the retrieval of ACLs for columns within a table.
---
src/include/catalog/pg_proc.dat | 2 +-
src/backend/catalog/objectaddress.c | 60 +++++++++++++++++++-----
src/test/regress/expected/privileges.out | 32 +++++++++++--
src/test/regress/sql/privileges.sql | 12 +++--
doc/src/sgml/func.sgml | 6 +--
5 files changed, 87 insertions(+), 25 deletions(-)
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.datindex 0bf413fe05..0a0f6aa198 100644--- a/src/include/catalog/pg_proc.dat+++ b/src/include/catalog/pg_proc.dat@@ -6364,7 +6364,7 @@
{ oid => '8730', descr => 'get ACL for SQL object',
proname => 'pg_get_acl', provolatile => 's', prorettype => '_aclitem',
- proargtypes => 'oid oid', proargnames => '{classid,objid}',+ proargtypes => 'oid oid int4', proargnames => '{classid,objid,objsubid}',
prosrc => 'pg_get_acl' },
{ oid => '3839',
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.cindex 2983b9180f..fac3922964 100644--- a/src/backend/catalog/objectaddress.c+++ b/src/backend/catalog/objectaddress.c@@ -4364,19 +4364,19 @@ pg_identify_object_as_address(PG_FUNCTION_ARGS)
/*
* SQL-level callable function to obtain the ACL of a specified object, given
- * its catalog OID and object OID.+ * its catalog OID, object OID and sub-object ID.
*/
Datum
pg_get_acl(PG_FUNCTION_ARGS)
{
Oid classId = PG_GETARG_OID(0);
Oid objectId = PG_GETARG_OID(1);
+ int32 objsubid = PG_GETARG_INT32(2);
Oid catalogId;
AttrNumber Anum_acl;
- Relation rel;- HeapTuple tup;
Datum datum;
bool isnull;
+ HeapTuple tup;
/* for "pinned" items in pg_depend, return null */
if (!OidIsValid(classId) && !OidIsValid(objectId))
@@ -4391,18 +4391,52 @@ pg_get_acl(PG_FUNCTION_ARGS)
if (Anum_acl == InvalidAttrNumber)
PG_RETURN_NULL();
- rel = table_open(catalogId, AccessShareLock);-- tup = get_catalog_object_by_oid(rel, get_object_attnum_oid(catalogId),- objectId);- if (!HeapTupleIsValid(tup))+ /*+ * If dealing with a relation's attribute (objsubid is set), the ACL is+ * retrieved from pg_attribute.+ */+ if (classId == RelationRelationId && objsubid != 0)
{
- table_close(rel, AccessShareLock);- PG_RETURN_NULL();- }+ AttrNumber attnum = (AttrNumber) objsubid;+ Form_pg_attribute atttup;- datum = heap_getattr(tup, Anum_acl, RelationGetDescr(rel), &isnull);- table_close(rel, AccessShareLock);+ tup = SearchSysCache2(ATTNUM, ObjectIdGetDatum(objectId),+ Int16GetDatum(attnum));++ if (!HeapTupleIsValid(tup))+ PG_RETURN_NULL();++ atttup = (Form_pg_attribute) GETSTRUCT(tup);++ /* ignore dropped columns */+ if (atttup->attisdropped)+ {+ ReleaseSysCache(tup);+ PG_RETURN_NULL();+ }++ datum = SysCacheGetAttr(ATTNUM, tup,+ Anum_pg_attribute_attacl,+ &isnull);+ ReleaseSysCache(tup);+ }+ else+ {+ Relation rel;++ rel = table_open(catalogId, AccessShareLock);++ tup = get_catalog_object_by_oid(rel, get_object_attnum_oid(catalogId),+ objectId);+ if (!HeapTupleIsValid(tup))+ {+ table_close(rel, AccessShareLock);+ PG_RETURN_NULL();+ }++ datum = heap_getattr(tup, Anum_acl, RelationGetDescr(rel), &isnull);+ table_close(rel, AccessShareLock);+ }
if (isnull)
PG_RETURN_NULL();
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.outindex 332bc584eb..fab0cc800f 100644--- a/src/test/regress/expected/privileges.out+++ b/src/test/regress/expected/privileges.out@@ -213,7 +213,7 @@ SELECT * FROM atest1;
(0 rows)
CREATE TABLE atest2 (col1 varchar(10), col2 boolean);
-SELECT pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid);+SELECT pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid, 0);
pg_get_acl
------------
@@ -223,7 +223,7 @@ GRANT SELECT ON atest2 TO regress_priv_user2;
GRANT UPDATE ON atest2 TO regress_priv_user3;
GRANT INSERT ON atest2 TO regress_priv_user4 GRANTED BY CURRENT_USER;
GRANT TRUNCATE ON atest2 TO regress_priv_user5 GRANTED BY CURRENT_ROLE;
-SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid));+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid, 0));
unnest
------------------------------------------------
regress_priv_user1=arwdDxtm/regress_priv_user1
@@ -234,13 +234,13 @@ SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid));
(5 rows)
-- Invalid inputs
-SELECT pg_get_acl('pg_class'::regclass, 0); -- null+SELECT pg_get_acl('pg_class'::regclass, 0, 0); -- null
pg_get_acl
------------
(1 row)
-SELECT pg_get_acl(0, 0); -- null+SELECT pg_get_acl(0, 0, 0); -- null
pg_get_acl
------------
@@ -653,6 +653,30 @@ CREATE TABLE atest5 (one int, two int unique, three int, four int unique);
CREATE TABLE atest6 (one int, two int, blue int);
GRANT SELECT (one), INSERT (two), UPDATE (three) ON atest5 TO regress_priv_user4;
GRANT ALL (one) ON atest5 TO regress_priv_user3;
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 1));+ unnest +--------------------------------------------+ regress_priv_user4=r/regress_priv_user1+ regress_priv_user3=arwx/regress_priv_user1+(2 rows)++SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 2));+ unnest +-----------------------------------------+ regress_priv_user4=a/regress_priv_user1+(1 row)++SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 3));+ unnest +-----------------------------------------+ regress_priv_user4=w/regress_priv_user1+(1 row)++SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 4));+ unnest +--------+(0 rows)+
INSERT INTO atest5 VALUES (1,2,3);
SET SESSION AUTHORIZATION regress_priv_user4;
SELECT * FROM atest5; -- fail
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sqlindex 980d19bde5..ae338e8cc8 100644--- a/src/test/regress/sql/privileges.sql+++ b/src/test/regress/sql/privileges.sql@@ -183,16 +183,16 @@ GRANT SELECT ON atest1 TO regress_priv_user3, regress_priv_user4;
SELECT * FROM atest1;
CREATE TABLE atest2 (col1 varchar(10), col2 boolean);
-SELECT pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid);+SELECT pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid, 0);
GRANT SELECT ON atest2 TO regress_priv_user2;
GRANT UPDATE ON atest2 TO regress_priv_user3;
GRANT INSERT ON atest2 TO regress_priv_user4 GRANTED BY CURRENT_USER;
GRANT TRUNCATE ON atest2 TO regress_priv_user5 GRANTED BY CURRENT_ROLE;
-SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid));+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest2'::regclass::oid, 0));
-- Invalid inputs
-SELECT pg_get_acl('pg_class'::regclass, 0); -- null-SELECT pg_get_acl(0, 0); -- null+SELECT pg_get_acl('pg_class'::regclass, 0, 0); -- null+SELECT pg_get_acl(0, 0, 0); -- null
GRANT TRUNCATE ON atest2 TO regress_priv_user4 GRANTED BY regress_priv_user5; -- error
@@ -439,6 +439,10 @@ CREATE TABLE atest5 (one int, two int unique, three int, four int unique);
CREATE TABLE atest6 (one int, two int, blue int);
GRANT SELECT (one), INSERT (two), UPDATE (three) ON atest5 TO regress_priv_user4;
GRANT ALL (one) ON atest5 TO regress_priv_user3;
+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 1));+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 2));+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 3));+SELECT unnest(pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 4));
INSERT INTO atest5 VALUES (1,2,3);
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgmlindex 93ee3d4b60..aad944979f 100644--- a/doc/src/sgml/func.sgml+++ b/doc/src/sgml/func.sgml@@ -26592,12 +26592,12 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id'));
<indexterm>
<primary>pg_get_acl</primary>
</indexterm>
- <function>pg_get_acl</function> ( <parameter>classid</parameter> <type>oid</type>, <parameter>objid</parameter> <type>oid</type> )+ <function>pg_get_acl</function> ( <parameter>classid</parameter> <type>oid</type>, <parameter>objid</parameter> <type>oid</type>, <parameter>objsubid</parameter> <type>integer</type> )
<returnvalue>aclitem[]</returnvalue>
</para>
<para>
Returns the <acronym>ACL</acronym> for a database object, specified
- by catalog OID and object OID. This function returns+ by catalog OID, object OID and sub-object ID. This function returns
<literal>NULL</literal> values for undefined objects.
</para></entry>
</row>
@@ -26723,7 +26723,7 @@ SELECT currval(pg_get_serial_sequence('sometable', 'id'));
<programlisting>
postgres=# SELECT
(pg_identify_object(s.classid,s.objid,s.objsubid)).*,
- pg_catalog.pg_get_acl(s.classid,s.objid) AS acl+ pg_catalog.pg_get_acl(s.classid,s.objid,s.objsubid) AS acl
FROM pg_catalog.pg_shdepend AS s
JOIN pg_catalog.pg_database AS d
ON d.datname = current_database() AND
--
2.45.2
signature.asc (application/pgp-signature, 832B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-committers@postgresql.org
Cc: michael@paquier.xyz, joel@compiler.org, tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org
Subject: Re: pgsql: Add pg_get_acl() to get the ACL for a database object
In-Reply-To: <ZoukijRBef4m-VMO@paquier.xyz>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox