Re: "has_column_privilege()" issue with attnums and non-existent columns
On 1/29/21 4:56 AM, Joe Conway wrote: On 1/29/21 12:13 AM, Ian Lawrence Barwick wrote: 2021年1月28日(木) 17:18 Peter Eisentraut: I'm not convinced the current behavior is wrong. Is there some practical use case that is affected by this behavior? I was poking around at the function with a view to using it for something and was curious what it did with bad input. Providing the column name of a dropped column: Me: "Hey Postgres, do I have privileges on the dropped column 'bar' of my table 'foo'?" Pg: "That column doesn't even exist - here, have an error instead." Me: "Hey Postgres, does some other less-privileged user have privileges on the dropped column 'bar' of my table 'foo'? Pg: "That column doesn't even exist - here, have an error instead." Providing the attnum of a dropped column: Me: "Hey Postgres, here's the attnum of the dropped column 'bar', does some other less-privileged user have privileges on that column?" Pg: "That column doesn't even exist - here, have a NULL". Me: "Hey Postgres, here's the attnum of the dropped column 'bar' on this table I own, do I have privileges on that column?" Pg: "Yup. And feel free to throw any other smallint at me, I'll pretend that represents a column too even if it never existed.". Looking at the code, particularly the cited comment, it does seems the intent was to return NULL in all cases where an invalid attnum was provided, but that gets short-circuited by the assumption table owner = has privilege on any column. Nicely illustrated :-) Not the most urgent or exciting of issues, but seems inconsistent to me. +1 Peter, did Ian's explanation answer your concerns? Joe (or Peter), any interest in reviewing/committing this patch? Regards, -- -David da...@pgmasters.net
Re: "has_column_privilege()" issue with attnums and non-existent columns
On 3/3/21 8:50 AM, David Steele wrote: > On 1/29/21 4:56 AM, Joe Conway wrote: >> On 1/29/21 12:13 AM, Ian Lawrence Barwick wrote: >>> 2021年1月28日(木) 17:18 Peter Eisentraut: >>> I'm not convinced the current behavior is wrong. Is there some >>> practical use case that is affected by this behavior? >>> >>> I was poking around at the function with a view to using it for something >>> and was >>> curious what it did with bad input. >>> >>> Providing the column name of a dropped column: >>> >>> Me: "Hey Postgres, do I have privileges on the dropped column 'bar' of >>> my >>> table 'foo'?" >>> Pg: "That column doesn't even exist - here, have an error instead." >>> Me: "Hey Postgres, does some other less-privileged user have >>> privileges on the >>> dropped column 'bar' of my table 'foo'? >>> Pg: "That column doesn't even exist - here, have an error instead." >>> >>> Providing the attnum of a dropped column: >>> >>> Me: "Hey Postgres, here's the attnum of the dropped column 'bar', does >>> some >>> other less-privileged user have privileges on that column?" >>> Pg: "That column doesn't even exist - here, have a NULL". >>> Me: "Hey Postgres, here's the attnum of the dropped column 'bar' on >>> this table >>> I own, do I have privileges on that column?" >>> Pg: "Yup. And feel free to throw any other smallint at me, I'll >>> pretend that >>> represents a column too even if it never existed.". >>> >>> Looking at the code, particularly the cited comment, it does seems the >>> intent was >>> to return NULL in all cases where an invalid attnum was provided, but that >>> gets >>> short-circuited by the assumption table owner = has privilege on any column. >> >> Nicely illustrated :-) >> >>> Not the most urgent or exciting of issues, but seems inconsistent to me. >> >> +1 > > Peter, did Ian's explanation answer your concerns? > > Joe (or Peter), any interest in reviewing/committing this patch? Sure, I'll take a look Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development signature.asc Description: OpenPGP digital signature
Re: "has_column_privilege()" issue with attnums and non-existent columns
On 3/3/21 9:43 AM, Joe Conway wrote: > On 3/3/21 8:50 AM, David Steele wrote: >> On 1/29/21 4:56 AM, Joe Conway wrote: >>> On 1/29/21 12:13 AM, Ian Lawrence Barwick wrote: 2021年1月28日(木) 17:18 Peter Eisentraut: I'm not convinced the current behavior is wrong. Is there some practical use case that is affected by this behavior? I was poking around at the function with a view to using it for something and was curious what it did with bad input. Providing the column name of a dropped column: Me: "Hey Postgres, do I have privileges on the dropped column 'bar' of my table 'foo'?" Pg: "That column doesn't even exist - here, have an error instead." Me: "Hey Postgres, does some other less-privileged user have privileges on the dropped column 'bar' of my table 'foo'? Pg: "That column doesn't even exist - here, have an error instead." Providing the attnum of a dropped column: Me: "Hey Postgres, here's the attnum of the dropped column 'bar', does some other less-privileged user have privileges on that column?" Pg: "That column doesn't even exist - here, have a NULL". Me: "Hey Postgres, here's the attnum of the dropped column 'bar' on this table I own, do I have privileges on that column?" Pg: "Yup. And feel free to throw any other smallint at me, I'll pretend that represents a column too even if it never existed.". Looking at the code, particularly the cited comment, it does seems the intent was to return NULL in all cases where an invalid attnum was provided, but that gets short-circuited by the assumption table owner = has privilege on any column. >>> >>> Nicely illustrated :-) >>> Not the most urgent or exciting of issues, but seems inconsistent to me. >>> >>> +1 >> >> Peter, did Ian's explanation answer your concerns? >> >> Joe (or Peter), any interest in reviewing/committing this patch? > > Sure, I'll take a look Based on Tom's commit here: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=3d0f68dd3 it appears to me that the intent is to return NULL. However I was not to happy with the way the submitted patch added an argument to column_privilege_check(). It seems to me that it is better to just reorder the checks in column_privilege_check() a bit, as in the attached. I wasn't entirely sure it was ok to split up the check for dropped attribute and pg_attribute_aclcheck like I did, but when I tested the race condition (by pausing at pg_attribute_aclcheck and dropping the column in another session) everything seemed to work fine. Any comments? Thanks, Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: "has_column_privilege()" issue with attnums and non-existent columns
Joe: I don't seem to find attachment. Maybe attach again ? Thanks On Sun, Mar 7, 2021 at 11:12 AM Joe Conway wrote: > On 3/3/21 9:43 AM, Joe Conway wrote: > > On 3/3/21 8:50 AM, David Steele wrote: > >> On 1/29/21 4:56 AM, Joe Conway wrote: > >>> On 1/29/21 12:13 AM, Ian Lawrence Barwick wrote: > 2021年1月28日(木) 17:18 Peter Eisentraut: > I'm not convinced the current behavior is wrong. Is there some > practical use case that is affected by this behavior? > > I was poking around at the function with a view to using it for > something and was > curious what it did with bad input. > > Providing the column name of a dropped column: > > Me: "Hey Postgres, do I have privileges on the dropped column > 'bar' of my > table 'foo'?" > Pg: "That column doesn't even exist - here, have an error > instead." > Me: "Hey Postgres, does some other less-privileged user have > privileges on the > dropped column 'bar' of my table 'foo'? > Pg: "That column doesn't even exist - here, have an error > instead." > > Providing the attnum of a dropped column: > > Me: "Hey Postgres, here's the attnum of the dropped column > 'bar', does some > other less-privileged user have privileges on that column?" > Pg: "That column doesn't even exist - here, have a NULL". > Me: "Hey Postgres, here's the attnum of the dropped column 'bar' > on this table > I own, do I have privileges on that column?" > Pg: "Yup. And feel free to throw any other smallint at me, I'll > pretend that > represents a column too even if it never existed.". > > Looking at the code, particularly the cited comment, it does seems > the intent was > to return NULL in all cases where an invalid attnum was provided, but > that gets > short-circuited by the assumption table owner = has privilege on any > column. > >>> > >>> Nicely illustrated :-) > >>> > Not the most urgent or exciting of issues, but seems inconsistent to > me. > >>> > >>> +1 > >> > >> Peter, did Ian's explanation answer your concerns? > >> > >> Joe (or Peter), any interest in reviewing/committing this patch? > > > > Sure, I'll take a look > > Based on Tom's commit here: > > https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=3d0f68dd3 > > it appears to me that the intent is to return NULL. > > However I was not to happy with the way the submitted patch added an > argument to > column_privilege_check(). It seems to me that it is better to just reorder > the > checks in column_privilege_check() a bit, as in the attached. > > I wasn't entirely sure it was ok to split up the check for dropped > attribute and > pg_attribute_aclcheck like I did, but when I tested the race condition (by > pausing at pg_attribute_aclcheck and dropping the column in another > session) > everything seemed to work fine. > > Any comments? > > Thanks, > > Joe > > -- > Crunchy Data - http://crunchydata.com > PostgreSQL Support for Secure Enterprises > Consulting, Training, & Open Source Development > > >
Re: "has_column_privilege()" issue with attnums and non-existent columns
On 3/7/21 2:35 PM, Zhihong Yu wrote:
> Joe:
> I don't seem to find attachment.
>
> Maybe attach again ?
Oops -- I did forget that, didn't I. This time patch is attached :-)
Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index c7f029e..152d1da 100644
*** a/src/backend/utils/adt/acl.c
--- b/src/backend/utils/adt/acl.c
*** column_privilege_check(Oid tableoid, Att
*** 2460,2484
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),
--- 2460,2468
return -1;
/*
! * 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),
*** column_privilege_check(Oid tableoid, Att
*** 2493,2498
--- 2477,2499
}
ReleaseSysCache(attTuple);
+ /*
+ * Now 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 privilege */
aclresult = pg_attribute_aclcheck(tableoid, attnum, roleid, mode);
return (aclresult == ACLCHECK_OK);
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 46a69fc..d60ea53 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;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 6277140..766eeae 100644
*** a/src/test/regress/sql/privileges.sql
--- b/src/test/regress/sql/privileges.sql
*** alter table mytable drop column f2;
*** 836,841
--- 836,842
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');
drop table mytable;
Re: "has_column_privilege()" issue with attnums and non-existent columns
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: not tested Documentation:not tested I tested the patch and it works well. And according to the comment, checking existence of relation and pg_class_aclcheck() won't influenced by concurrent DROP. So I think it's safe to just reorder the checking existence of column and pg_attribute_aclcheck(). Thanks
Re: "has_column_privilege()" issue with attnums and non-existent columns
On 3/16/21 1:42 AM, Chengxi Sun wrote: The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: not tested Documentation:not tested I tested the patch and it works well. Was that my patch (i.e. the latest on this thread) or Ian's? It is not clear since you did not CC me... And according to the comment, checking existence of relation and pg_class_aclcheck() won't influenced by concurrent DROP. So I think it's safe to just reorder the checking existence of column and pg_attribute_aclcheck(). "Code never lies, comments sometimes do" That said, I did at least a basic test of that assumption and it seems to be true. Ian, or anyone else, any comments/complaints on my changes? If not I will commit and push that version sooner rather than later. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: "has_column_privilege()" issue with attnums and non-existent columns
On 3/16/21 2:45 PM, Joe Conway wrote: Ian, or anyone else, any comments/complaints on my changes? If not I will commit and push that version sooner rather than later. Any thoughts on back-patching this? On one hand, in my view it is clearly a bug. On the other hand, no one has complained before and this does change user visible behavior. I guess I am leaning toward not back-patching... Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: "has_column_privilege()" issue with attnums and non-existent columns
Joe Conway writes: > On 3/16/21 2:45 PM, Joe Conway wrote: >> Ian, or anyone else, any comments/complaints on my changes? If not I will >> commit >> and push that version sooner rather than later. I took a quick look, and I'm afraid I don't believe this: !* 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. What the existing code is assuming is that if we do a successful SearchSysCache check, then an immediately following pg_xxx_aclcheck call that needs to examine that same row will also find that row in the cache, because there will be no need for any actual database traffic to do that. What you've done here is changed that pattern to be SearchSysCache(row1) SearchSysCache(row2) aclcheck on row1 aclcheck on row2 But that does NOT offer any such guarantee, because the row2 cache lookup could incur a database access, which might notice that row1 has been deleted. 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. regards, tom lane
Re: "has_column_privilege()" issue with attnums and non-existent columns
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.
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?
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)
Re: "has_column_privilege()" issue with attnums and non-existent columns
Joe Conway writes: > 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. Looks generally reasonable by eyeball. The lack of any documentation comment for the new functions makes me itch, although the ones that are there are hardly better. > 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? In principle, it might be nice to fix all of those functions in acl.c to be implemented similarly --- you could get rid of the initial SearchSysCacheExists calls in the ones that are trying not to throw error for is-missing cases. In practice, as long as there's no reachable bug for the other cases, there are probably better things to spend time on. > 2. This seems more invasive than something we would want > to back patch -- agreed? You could make an argument either way, but given the limited number of complaints about this, I'd lean to no back-patch. regards, tom lane
Re: "has_column_privilege()" issue with attnums and non-existent columns
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
Re: "has_column_privilege()" issue with attnums and non-existent columns
Joe Conway writes: > Heh, I missed the forest for the trees it seems. > That version undid the changes fixing what Ian was originally complaining > about. Duh, right. It would be a good idea for there to be a code comment explaining this, because it's *far* from obvious. Say like * Check for column-level privileges first. This serves in * part as a check on whether the column even exists, so we * need to do it before checking table-level privilege. My gripe about providing API-spec comments for the new aclchk.c entry points still stands. Other than that, I think it's good to go. regards, tom lane
Re: "has_column_privilege()" issue with attnums and non-existent columns
On 3/30/21 6:22 PM, Tom Lane wrote: Joe Conway writes: Heh, I missed the forest for the trees it seems. That version undid the changes fixing what Ian was originally complaining about. Duh, right. It would be a good idea for there to be a code comment explaining this, because it's *far* from obvious. Say like * Check for column-level privileges first. This serves in * part as a check on whether the column even exists, so we * need to do it before checking table-level privilege. Will do. My gripe about providing API-spec comments for the new aclchk.c entry points still stands. Other than that, I think it's good to go. Yeah, I was planning to put something akin to this in all four spots: 8<--- /* * Exported routine for checking a user's access privileges to a table * * Does the bulk of the work for pg_class_aclcheck(), and allows other * callers to avoid the missing relation ERROR when is_missing is non-NULL. */ AclResult pg_class_aclcheck_ext(Oid table_oid, Oid roleid, AclMode mode, bool *is_missing) ... 8<--- Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: "has_column_privilege()" issue with attnums and non-existent columns
On 3/30/21 8:17 PM, Joe Conway wrote: On 3/30/21 6:22 PM, Tom Lane wrote: Joe Conway writes: Heh, I missed the forest for the trees it seems. That version undid the changes fixing what Ian was originally complaining about. Duh, right. It would be a good idea for there to be a code comment explaining this, because it's *far* from obvious. Say like * Check for column-level privileges first. This serves in * part as a check on whether the column even exists, so we * need to do it before checking table-level privilege. Will do. My gripe about providing API-spec comments for the new aclchk.c entry points still stands. Other than that, I think it's good to go. Yeah, I was planning to put something akin to this in all four spots: 8<--- /* * Exported routine for checking a user's access privileges to a table * * Does the bulk of the work for pg_class_aclcheck(), and allows other * callers to avoid the missing relation ERROR when is_missing is non-NULL. */ AclResult pg_class_aclcheck_ext(Oid table_oid, Oid roleid, AclMode mode, bool *is_missing) ... 8<--- Pushed that way. Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development
Re: "has_column_privilege()" issue with attnums and non-existent columns
On 2021-01-12 06:53, Ian Lawrence Barwick wrote:
postgres=# SELECT has_column_privilege('foo', 999::int2, 'SELECT');
has_column_privilege
--
t
(1 row)
The comment on the relevant code section in "src/backend/utils/adt/acl.c"
(related to "column_privilege_check()") indicate that NULL is the intended
return value in these cases:
Likewise, the variants that take an integer attnum
return NULL (rather than throwing an error) if there is no such
pg_attribute entry. All variants return NULL if an attisdropped
column is selected.
The unexpected "TRUE" value is a result of "column_privilege_check()" returning
TRUE if the user has table-level privileges. This returns a valid result with
the function variants where the column name is specified, as the calling
function will have already performed a check of the column through its call to
"convert_column_name()". However when the attnum is specified, the status of
the column never gets checked.
I'm not convinced the current behavior is wrong. Is there some
practical use case that is affected by this behavior?
The second patch adds a bunch of missing static prototypes to "acl.c",
on general
principles.
Why is this necessary?
Re: "has_column_privilege()" issue with attnums and non-existent columns
2021年1月28日(木) 17:18 Peter Eisentraut :
> On 2021-01-12 06:53, Ian Lawrence Barwick wrote:
> > postgres=# SELECT has_column_privilege('foo', 999::int2, 'SELECT');
> > has_column_privilege
> > --
> > t
> > (1 row)
> >
> > The comment on the relevant code section in "src/backend/utils/adt/acl.c"
> > (related to "column_privilege_check()") indicate that NULL is the
> intended
> > return value in these cases:
> >
> > Likewise, the variants that take an integer attnum
> > return NULL (rather than throwing an error) if there is no such
> > pg_attribute entry. All variants return NULL if an attisdropped
> > column is selected.
> >
> > The unexpected "TRUE" value is a result of "column_privilege_check()"
> returning
> > TRUE if the user has table-level privileges. This returns a valid
> result with
> > the function variants where the column name is specified, as the calling
> > function will have already performed a check of the column through its
> call to
> > "convert_column_name()". However when the attnum is specified, the
> status of
> > the column never gets checked.
>
> I'm not convinced the current behavior is wrong. Is there some
> practical use case that is affected by this behavior?
>
I was poking around at the function with a view to using it for something
and was
curious what it did with bad input.
Providing the column name of a dropped column:
Me: "Hey Postgres, do I have privileges on the dropped column 'bar' of
my table 'foo'?"
Pg: "That column doesn't even exist - here, have an error instead."
Me: "Hey Postgres, does some other less-privileged user have privileges
on the
dropped column 'bar' of my table 'foo'?
Pg: "That column doesn't even exist - here, have an error instead."
Providing the attnum of a dropped column:
Me: "Hey Postgres, here's the attnum of the dropped column 'bar', does
some
other less-privileged user have privileges on that column?"
Pg: "That column doesn't even exist - here, have a NULL".
Me: "Hey Postgres, here's the attnum of the dropped column 'bar' on
this table
I own, do I have privileges on that column?"
Pg: "Yup. And feel free to throw any other smallint at me, I'll pretend
that
represents a column too even if it never existed.".
Looking at the code, particularly the cited comment, it does seems the
intent was
to return NULL in all cases where an invalid attnum was provided, but that
gets
short-circuited by the assumption table owner = has privilege on any column.
Not the most urgent or exciting of issues, but seems inconsistent to me.
> > The second patch adds a bunch of missing static prototypes to "acl.c",
> > on general
> > principles.
>
> Why is this necessary?
>
Not exactly necessary, but seemed odd some functions had prototypes, others
not.
I have no idea what the policy is, if any, and not something I would lose
sleep over,
just thought I'd note it in passing.
Regards
Ian Barwick
--
EnterpriseDB: https://www.enterprisedb.com
Re: "has_column_privilege()" issue with attnums and non-existent columns
On 1/29/21 12:13 AM, Ian Lawrence Barwick wrote: > 2021年1月28日(木) 17:18 Peter Eisentraut: > I'm not convinced the current behavior is wrong. Is there some > practical use case that is affected by this behavior? > > > I was poking around at the function with a view to using it for something and > was > curious what it did with bad input. > > Providing the column name of a dropped column: > > Me: "Hey Postgres, do I have privileges on the dropped column 'bar' of my > table 'foo'?" > Pg: "That column doesn't even exist - here, have an error instead." > Me: "Hey Postgres, does some other less-privileged user have privileges > on the > dropped column 'bar' of my table 'foo'? > Pg: "That column doesn't even exist - here, have an error instead." > > Providing the attnum of a dropped column: > > Me: "Hey Postgres, here's the attnum of the dropped column 'bar', does > some > other less-privileged user have privileges on that column?" > Pg: "That column doesn't even exist - here, have a NULL". > Me: "Hey Postgres, here's the attnum of the dropped column 'bar' on this > table > I own, do I have privileges on that column?" > Pg: "Yup. And feel free to throw any other smallint at me, I'll pretend > that > represents a column too even if it never existed.". > > Looking at the code, particularly the cited comment, it does seems the intent > was > to return NULL in all cases where an invalid attnum was provided, but that > gets > short-circuited by the assumption table owner = has privilege on any column. Nicely illustrated :-) > Not the most urgent or exciting of issues, but seems inconsistent to me. +1 Joe -- Crunchy Data - http://crunchydata.com PostgreSQL Support for Secure Enterprises Consulting, Training, & Open Source Development signature.asc Description: OpenPGP digital signature
