From: Joel Jacobson <joel@compiler.org>
To: Michael Paquier <michael@paquier.xyz>
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: Fri, 05 Jul 2024 10:40:39 +0200
Message-ID: <f2539bff-64be-47f0-9f0b-df85d3cc0432@app.fastmail.com> (raw)
In-Reply-To: <ZoctyZzE3UlUSer_@paquier.xyz>
References: <E1sPHWu-000GPc-0z@gemulon.postgresql.org>
<3408967.1720107891@sss.pgh.pa.us>
<f2688fb7-add6-43da-b1df-97312a451893@app.fastmail.com>
<ZoctyZzE3UlUSer_@paquier.xyz>
On Fri, Jul 5, 2024, at 01:18, Michael Paquier wrote:
> I would still stick to only one function, with arguments coming from
> scanning pg_[sh]depend.
>
>> I found some code in aclchk.c on line 4452-4468 that seems useful,
>> but not sure. Maybe there is some other existing code that is better
>> as an inspiration?
>
> My first feelings about that was that subobjids are only used for
> pg_attribute, so if we use them as a base, it would look like:
> - Adding a new AttrNumber in ObjectProperty to track to which column
> the subobjid should apply and its catcache number (ATTNUM for the
> attribute number for fast lookup).
> - Extend get_catalog_object_by_oid() with more data: the attribute
> column for the subobjid scan stored in ObjectProperty, the subobjid
> value given by the caller. If get_catalog_object_by_oid()'s caller
> defines a subobjid, use get_object_catcache_oid() on it over the main
> OID one. If it cannot be found in the cache, use a scan key based on
> the class OID and the subobjid.
>
>> I guess we need to handle the RelationRelationId separately,
>> and handle all other classes using the current code in pg_get_acl()?
>
> But most of that simply blows away because we track the dependency to
> the attribute ACLs in pg_shdepend based on pg_class, not pg_attribute.
> get_attname() itself relies on ATTNUM, so perhaps that's OK to just
> add an exception based on RelationRelationId in the code path of
> pg_get_acl(). That makes me a bit uncomfortable to derive more from
> the other routines for the object descriptions, but we've been living
> with that in getObjectDescription() for years now, as well.
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.
I've also updated func.sgml, adjusted pg_proc.dat, and added
regression tests for column privileges.
Regards,
Joel
Attachments:0001-Extend-pg_get_acl-to-handle-sub-object-IDs.patch (application/octet-stream, 10.8K) download
From 6245502f9fc981d613c70016b45d6e2674d7d0cb Mon Sep 17 00:00:00 2001
From: Joel Jakobsson <github@compiler.org>
Date: Thu, 4 Jul 2024 22:56:37 +0200
Subject: [PATCH] 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.
Detailed changes:
- Updated func.sgml to document the new parameter and its usage.- Modified pg_get_acl function in objectaddress.c to handle sub-object IDs.- Adjusted the catalog entry in pg_proc.dat to include the new parameter.- Enhanced regression tests in privileges.sql to test the new functionality.---
doc/src/sgml/func.sgml | 6 +--
src/backend/catalog/objectaddress.c | 67 +++++++++++++++++++-----
src/include/catalog/pg_proc.dat | 2 +-
src/test/regress/expected/privileges.out | 56 ++++++++++++++++++--
src/test/regress/sql/privileges.sql | 16 ++++--
5 files changed, 123 insertions(+), 24 deletions(-)
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
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.cindex 2983b9180f..659512b254 100644--- a/src/backend/catalog/objectaddress.c+++ b/src/backend/catalog/objectaddress.c@@ -4364,13 +4364,14 @@ 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;
@@ -4391,21 +4392,63 @@ 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))+ /*+ * The procedure to retrive the ACL is different if the object is a table+ * and the sub-object is non-zero, which means we're dealing with a column+ * of a table. In this case, the objsubid is the attnum for the column.+ */+ if (classId == RelationRelationId && objsubid != 0)
{
- table_close(rel, AccessShareLock);- PG_RETURN_NULL();+ AttrNumber attnum = (AttrNumber) objsubid;++ HeapTuple attTuple;+ bool isNull;++ attTuple = SearchSysCache2(ATTNUM,+ ObjectIdGetDatum(objectId),+ Int16GetDatum(attnum));++ if (!HeapTupleIsValid(attTuple))+ PG_RETURN_NULL();++ /* ignore dropped columns */+ if (((Form_pg_attribute) GETSTRUCT(attTuple))->attisdropped)+ {+ ReleaseSysCache(attTuple);+ PG_RETURN_NULL();+ }++ datum = SysCacheGetAttr(ATTNUM, attTuple,+ Anum_pg_attribute_attacl,+ &isNull);++ /* no need to do anything for a NULL ACL */+ if (isNull)+ {+ ReleaseSysCache(attTuple);+ PG_RETURN_NULL();+ }++ ReleaseSysCache(attTuple);
}
+ else+ {+ rel = table_open(catalogId, AccessShareLock);- datum = heap_getattr(tup, Anum_acl, RelationGetDescr(rel), &isnull);- table_close(rel, AccessShareLock);+ tup = get_catalog_object_by_oid(rel, get_object_attnum_oid(catalogId),+ objectId);+ if (!HeapTupleIsValid(tup))+ {+ table_close(rel, AccessShareLock);+ PG_RETURN_NULL();+ }- if (isnull)- PG_RETURN_NULL();+ datum = heap_getattr(tup, Anum_acl, RelationGetDescr(rel), &isnull);+ table_close(rel, AccessShareLock);++ if (isnull)+ PG_RETURN_NULL();+ }
PG_RETURN_DATUM(datum);
}
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.datindex e1001a4822..9654b3ba4e 100644--- a/src/include/catalog/pg_proc.dat+++ b/src/include/catalog/pg_proc.dat@@ -6364,7 +6364,7 @@
{ oid => '6347', 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/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.outindex 332bc584eb..c904cbef84 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
------------
@@ -651,8 +651,56 @@ ERROR: permission denied for table atest2
SET SESSION AUTHORIZATION regress_priv_user1;
CREATE TABLE atest5 (one int, two int unique, three int, four int unique);
CREATE TABLE atest6 (one int, two int, blue int);
+SELECT pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 1);+ pg_get_acl +------------+ +(1 row)++SELECT pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 2);+ pg_get_acl +------------+ +(1 row)++SELECT pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 3);+ pg_get_acl +------------+ +(1 row)++SELECT pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 4);+ pg_get_acl +------------+ +(1 row)+
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..ea5c93cc57 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
@@ -437,8 +437,16 @@ SELECT * FROM atestv2; -- fail (even though regress_priv_user2 can access underl
SET SESSION AUTHORIZATION regress_priv_user1;
CREATE TABLE atest5 (one int, two int unique, three int, four int unique);
CREATE TABLE atest6 (one int, two int, blue int);
+SELECT pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 1);+SELECT pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 2);+SELECT pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 3);+SELECT pg_get_acl('pg_class'::regclass, 'atest5'::regclass::oid, 4);
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);
--
2.45.1
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: joel@compiler.org, michael@paquier.xyz, 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: <f2539bff-64be-47f0-9f0b-df85d3cc0432@app.fastmail.com>
* 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