On 3/30/21 3:37 PM, Joe Conway wrote:
On 3/21/21 12:27 PM, Tom Lane wrote:
I think we may have to adjust the acl.c APIs, or maybe better provide new
entry points, so that we can have variants of pg_xxx_aclcheck that won't
throw a hard error upon not finding the row. We cheesily tried to avoid
adjusting those APIs to support the semantics we need here, and we now see
that it didn't really work.
Ok, I took a shot at that; see attached.
Heh, I missed the forest for the trees it seems.
That version undid the changes fixing what Ian was originally complaining about.
New version attached that preserves the fixed behavior.
Questions:
1. I confined the changes to just pg_class_aclcheck/mask
and pg_attribute_aclcheck/mask -- did you intend
that we do this same change across the board? Or
perhaps do the rest of them once we open up pg15
development?
2. This seems more invasive than something we would want
to back patch -- agreed?
The questions remain...
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index add3d14..b5a6b3a 100644
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
*************** AclMode
*** 3706,3711 ****
--- 3706,3719 ----
pg_attribute_aclmask(Oid table_oid, AttrNumber attnum, Oid roleid,
AclMode mask, AclMaskHow how)
{
+ return pg_attribute_aclmask_ext(table_oid, attnum, roleid,
+ mask, how, NULL);
+ }
+
+ AclMode
+ pg_attribute_aclmask_ext(Oid table_oid, AttrNumber attnum, Oid roleid,
+ AclMode mask, AclMaskHow how, bool *is_missing)
+ {
AclMode result;
HeapTuple classTuple;
HeapTuple attTuple;
*************** pg_attribute_aclmask(Oid table_oid, Attr
*** 3723,3740 ****
ObjectIdGetDatum(table_oid),
Int16GetDatum(attnum));
if (!HeapTupleIsValid(attTuple))
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_COLUMN),
! errmsg("attribute %d of relation with OID %u does not exist",
! attnum, table_oid)));
attributeForm = (Form_pg_attribute) GETSTRUCT(attTuple);
! /* Throw error on dropped columns, too */
if (attributeForm->attisdropped)
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_COLUMN),
! errmsg("attribute %d of relation with OID %u does not exist",
! attnum, table_oid)));
aclDatum = SysCacheGetAttr(ATTNUM, attTuple, Anum_pg_attribute_attacl,
&isNull);
--- 3731,3768 ----
ObjectIdGetDatum(table_oid),
Int16GetDatum(attnum));
if (!HeapTupleIsValid(attTuple))
! {
! if (is_missing != NULL)
! {
! /* return "no privileges" instead of throwing an error */
! *is_missing = true;
! return 0;
! }
! else
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_COLUMN),
! errmsg("attribute %d of relation with OID %u does not exist",
! attnum, table_oid)));
! }
!
attributeForm = (Form_pg_attribute) GETSTRUCT(attTuple);
! /* Check dropped columns, too */
if (attributeForm->attisdropped)
! {
! if (is_missing != NULL)
! {
! /* return "no privileges" instead of throwing an error */
! *is_missing = true;
! ReleaseSysCache(attTuple);
! return 0;
! }
! else
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_COLUMN),
! errmsg("attribute %d of relation with OID %u does not exist",
! attnum, table_oid)));
! }
aclDatum = SysCacheGetAttr(ATTNUM, attTuple, Anum_pg_attribute_attacl,
&isNull);
*************** AclMode
*** 3791,3796 ****
--- 3819,3831 ----
pg_class_aclmask(Oid table_oid, Oid roleid,
AclMode mask, AclMaskHow how)
{
+ return pg_class_aclmask_ext(table_oid, roleid, mask, how, NULL);
+ }
+
+ AclMode
+ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode mask,
+ AclMaskHow how, bool *is_missing)
+ {
AclMode result;
HeapTuple tuple;
Form_pg_class classForm;
*************** pg_class_aclmask(Oid table_oid, Oid role
*** 3804,3813 ****
*/
tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(table_oid));
if (!HeapTupleIsValid(tuple))
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_TABLE),
! errmsg("relation with OID %u does not exist",
! table_oid)));
classForm = (Form_pg_class) GETSTRUCT(tuple);
/*
--- 3839,3858 ----
*/
tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(table_oid));
if (!HeapTupleIsValid(tuple))
! {
! if (is_missing != NULL)
! {
! /* return "no privileges" instead of throwing an error */
! *is_missing = true;
! return 0;
! }
! else
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_TABLE),
! errmsg("relation with OID %u does not exist",
! table_oid)));
! }
!
classForm = (Form_pg_class) GETSTRUCT(tuple);
/*
*************** AclResult
*** 4468,4474 ****
pg_attribute_aclcheck(Oid table_oid, AttrNumber attnum,
Oid roleid, AclMode mode)
{
! if (pg_attribute_aclmask(table_oid, attnum, roleid, mode, ACLMASK_ANY) != 0)
return ACLCHECK_OK;
else
return ACLCHECK_NO_PRIV;
--- 4513,4527 ----
pg_attribute_aclcheck(Oid table_oid, AttrNumber attnum,
Oid roleid, AclMode mode)
{
! return pg_attribute_aclcheck_ext(table_oid, attnum, roleid, mode, NULL);
! }
!
! AclResult
! pg_attribute_aclcheck_ext(Oid table_oid, AttrNumber attnum,
! Oid roleid, AclMode mode, bool *is_missing)
! {
! if (pg_attribute_aclmask_ext(table_oid, attnum, roleid, mode,
! ACLMASK_ANY, is_missing) != 0)
return ACLCHECK_OK;
else
return ACLCHECK_NO_PRIV;
*************** pg_attribute_aclcheck_all(Oid table_oid,
*** 4581,4587 ****
AclResult
pg_class_aclcheck(Oid table_oid, Oid roleid, AclMode mode)
{
! if (pg_class_aclmask(table_oid, roleid, mode, ACLMASK_ANY) != 0)
return ACLCHECK_OK;
else
return ACLCHECK_NO_PRIV;
--- 4634,4648 ----
AclResult
pg_class_aclcheck(Oid table_oid, Oid roleid, AclMode mode)
{
! return pg_class_aclcheck_ext(table_oid, roleid, mode, NULL);
! }
!
! AclResult
! pg_class_aclcheck_ext(Oid table_oid, Oid roleid,
! AclMode mode, bool *is_missing)
! {
! if (pg_class_aclmask_ext(table_oid, roleid, mode,
! ACLMASK_ANY, is_missing) != 0)
return ACLCHECK_OK;
else
return ACLCHECK_NO_PRIV;
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index c7f029e..cca543e 100644
*** a/src/backend/utils/adt/acl.c
--- b/src/backend/utils/adt/acl.c
*************** column_privilege_check(Oid tableoid, Att
*** 2450,2457 ****
Oid roleid, AclMode mode)
{
AclResult aclresult;
! HeapTuple attTuple;
! Form_pg_attribute attributeForm;
/*
* If convert_column_name failed, we can just return -1 immediately.
--- 2450,2456 ----
Oid roleid, AclMode mode)
{
AclResult aclresult;
! bool is_missing = false;
/*
* If convert_column_name failed, we can just return -1 immediately.
*************** column_privilege_check(Oid tableoid, Att
*** 2459,2501 ****
if (attnum == InvalidAttrNumber)
return -1;
! /*
! * First check if we have the privilege at the table level. We check
! * existence of the pg_class row before risking calling pg_class_aclcheck.
! * Note: it might seem there's a race condition against concurrent DROP,
! * but really it's safe because there will be no syscache flush between
! * here and there. So if we see the row in the syscache, so will
! * pg_class_aclcheck.
! */
! if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(tableoid)))
return -1;
! aclresult = pg_class_aclcheck(tableoid, roleid, mode);
!
if (aclresult == ACLCHECK_OK)
! return true;
!
! /*
! * No table privilege, so try per-column privileges. Again, we have to
! * check for dropped attribute first, and we rely on the syscache not to
! * notice a concurrent drop before pg_attribute_aclcheck fetches the row.
! */
! attTuple = SearchSysCache2(ATTNUM,
! ObjectIdGetDatum(tableoid),
! Int16GetDatum(attnum));
! if (!HeapTupleIsValid(attTuple))
! return -1;
! attributeForm = (Form_pg_attribute) GETSTRUCT(attTuple);
! if (attributeForm->attisdropped)
! {
! ReleaseSysCache(attTuple);
return -1;
! }
! ReleaseSysCache(attTuple);
!
! aclresult = pg_attribute_aclcheck(tableoid, attnum, roleid, mode);
!
! return (aclresult == ACLCHECK_OK);
}
/*
--- 2458,2479 ----
if (attnum == InvalidAttrNumber)
return -1;
! /* First try per-column privileges */
! aclresult = pg_attribute_aclcheck_ext(tableoid, attnum, roleid,
! mode, &is_missing);
! if (aclresult == ACLCHECK_OK)
! return 1;
! else if (is_missing)
return -1;
! /* Next check if we have the privilege at the table level */
! aclresult = pg_class_aclcheck_ext(tableoid, roleid, mode, &is_missing);
if (aclresult == ACLCHECK_OK)
! return 1;
! else if (is_missing)
return -1;
! else
! return 0;
}
/*
diff --git a/src/include/utils/acl.h b/src/include/utils/acl.h
index 541a438..af771c9 100644
*** a/src/include/utils/acl.h
--- b/src/include/utils/acl.h
*************** extern void RemoveRoleFromObjectACL(Oid
*** 233,240 ****
--- 233,246 ----
extern AclMode pg_attribute_aclmask(Oid table_oid, AttrNumber attnum,
Oid roleid, AclMode mask, AclMaskHow how);
+ extern AclMode pg_attribute_aclmask_ext(Oid table_oid, AttrNumber attnum,
+ Oid roleid, AclMode mask,
+ AclMaskHow how, bool *is_missing);
extern AclMode pg_class_aclmask(Oid table_oid, Oid roleid,
AclMode mask, AclMaskHow how);
+ extern AclMode pg_class_aclmask_ext(Oid table_oid, Oid roleid,
+ AclMode mask, AclMaskHow how,
+ bool *is_missing);
extern AclMode pg_database_aclmask(Oid db_oid, Oid roleid,
AclMode mask, AclMaskHow how);
extern AclMode pg_proc_aclmask(Oid proc_oid, Oid roleid,
*************** extern AclMode pg_type_aclmask(Oid type_
*** 256,264 ****
--- 262,275 ----
extern AclResult pg_attribute_aclcheck(Oid table_oid, AttrNumber attnum,
Oid roleid, AclMode mode);
+ extern AclResult pg_attribute_aclcheck_ext(Oid table_oid, AttrNumber attnum,
+ Oid roleid, AclMode mode,
+ bool *is_missing);
extern AclResult pg_attribute_aclcheck_all(Oid table_oid, Oid roleid,
AclMode mode, AclMaskHow how);
extern AclResult pg_class_aclcheck(Oid table_oid, Oid roleid, AclMode mode);
+ extern AclResult pg_class_aclcheck_ext(Oid table_oid, Oid roleid,
+ AclMode mode, bool *is_missing);
extern AclResult pg_database_aclcheck(Oid db_oid, Oid roleid, AclMode mode);
extern AclResult pg_proc_aclcheck(Oid proc_oid, Oid roleid, AclMode mode);
extern AclResult pg_language_aclcheck(Oid lang_oid, Oid roleid, AclMode mode);
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 46a69fc..9857f6c 100644
*** a/src/test/regress/expected/privileges.out
--- b/src/test/regress/expected/privileges.out
*************** select has_column_privilege('mytable','.
*** 1362,1368 ****
select has_column_privilege('mytable',2::int2,'select');
has_column_privilege
----------------------
! t
(1 row)
revoke select on table mytable from regress_priv_user3;
--- 1362,1374 ----
select has_column_privilege('mytable',2::int2,'select');
has_column_privilege
----------------------
!
! (1 row)
!
! select has_column_privilege('mytable',99::int2,'select');
! has_column_privilege
! ----------------------
!
(1 row)
revoke select on table mytable from regress_priv_user3;
*************** select has_column_privilege('mytable',2:
*** 1370,1375 ****
--- 1376,1387 ----
has_column_privilege
----------------------
+ (1 row)
+
+ select has_column_privilege('mytable',99::int2,'select');
+ has_column_privilege
+ ----------------------
+
(1 row)
drop table mytable;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 6277140..93f37fa 100644
*** a/src/test/regress/sql/privileges.sql
--- b/src/test/regress/sql/privileges.sql
*************** alter table mytable drop column f2;
*** 836,843 ****
--- 836,845 ----
select has_column_privilege('mytable','f2','select');
select has_column_privilege('mytable','........pg.dropped.2........','select');
select has_column_privilege('mytable',2::int2,'select');
+ select has_column_privilege('mytable',99::int2,'select');
revoke select on table mytable from regress_priv_user3;
select has_column_privilege('mytable',2::int2,'select');
+ select has_column_privilege('mytable',99::int2,'select');
drop table mytable;
-- Grant options
