"has_column_privilege()" issue with attnums and non-existent columns

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|

"has_column_privilege()" issue with attnums and non-existent columns

Ian Lawrence Barwick
Greetings

Consider a table set up as follows:

    CREATE TABLE foo (id INT, val TEXT);
    ALTER TABLE foo DROP COLUMN val;

When specifying the name of a dropped or non-existent column, the function
"has_column_privilege()" works as expected:

    postgres=# SELECT has_column_privilege('foo', 'val', 'SELECT');
    ERROR:  column "val" of relation "foo" does not exist

    postgres=# SELECT has_column_privilege('foo', 'bar', 'SELECT');
    ERROR:  column "bar" of relation "foo" does not exist

However when specifying a dropped or non-existent attnum, it returns
"TRUE", which is unexpected:

    postgres=# SELECT has_column_privilege('foo', 2::int2, 'SELECT');
     has_column_privilege
    ----------------------
     t
    (1 row)

    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.

Attached patch resolves this by having "column_privilege_check()" callers
indicate whether they have checked the column. If this is the case, and if the
user as table-level privileges, "column_privilege_check()" can return as before
with no further lookups needed.

If the user has table-level privileges but the column was not checked (i.e
attnum provided), the column lookup is performed.

The regression tests did contain checks for dropped/non-existent attnums,
however one group was acting on a table where the user had no table-level
privilege, giving a fall-through to the column-level check; the other group
contained a check which was just plain wrong.

This issue appears to have been present since "has_column_privilege()" was
introduced in PostreSQL 8.4 (commit 7449427a1e6a099bc7e76164cb99a01d5e87237b),
so falls into the "obscure, extreme corner-case" category, OTOH seems worth
backpatching to supported versions.

The second patch adds a bunch of missing static prototypes to "acl.c",
on general
principles.

I'll add this to the next commitfest.


Regards

Ian Barwick


--
EnterpriseDB: https://www.enterprisedb.com

has_column_privilege-attnum-fix.v1.patch (10K) Download Attachment
acl-missing-prototypes.v1.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: "has_column_privilege()" issue with attnums and non-existent columns

Peter Eisentraut-7
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?


Reply | Threaded
Open this post in threaded view
|

Re: "has_column_privilege()" issue with attnums and non-existent columns

Ian Lawrence Barwick


2021年1月28日(木) 17:18 Peter Eisentraut <[hidden email]>:
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

--
Reply | Threaded
Open this post in threaded view
|

Re: "has_column_privilege()" issue with attnums and non-existent columns

Joe Conway
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 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: "has_column_privilege()" issue with attnums and non-existent columns

David Steele
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
[hidden email]


Reply | Threaded
Open this post in threaded view
|

Re: "has_column_privilege()" issue with attnums and non-existent columns

Joe Conway
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 (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: "has_column_privilege()" issue with attnums and non-existent columns

Joe Conway
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


Reply | Threaded
Open this post in threaded view
|

Re: "has_column_privilege()" issue with attnums and non-existent columns

Zhihong Yu
Joe:
I don't seem to find attachment.

Maybe attach again ?

Thanks

On Sun, Mar 7, 2021 at 11:12 AM Joe Conway <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: "has_column_privilege()" issue with attnums and non-existent columns

Joe Conway
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

has_column_privilege-attnum-fix-jec.00.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: "has_column_privilege()" issue with attnums and non-existent columns

Chengxi Sun
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
Reply | Threaded
Open this post in threaded view
|

Re: "has_column_privilege()" issue with attnums and non-existent columns

Joe Conway
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


Reply | Threaded
Open this post in threaded view
|

Re: "has_column_privilege()" issue with attnums and non-existent columns

Joe Conway
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


Reply | Threaded
Open this post in threaded view
|

Re: "has_column_privilege()" issue with attnums and non-existent columns

Tom Lane-2
Joe Conway <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: "has_column_privilege()" issue with attnums and non-existent columns

Joe Conway
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

has_column_privilege-attnum-fix-jec.01.patch (10K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: "has_column_privilege()" issue with attnums and non-existent columns

Tom Lane-2
Joe Conway <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: "has_column_privilege()" issue with attnums and non-existent columns

Joe Conway
In reply to this post by Joe Conway
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

has_column_privilege-attnum-fix-jec.02.patch (11K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: "has_column_privilege()" issue with attnums and non-existent columns

Tom Lane-2
Joe Conway <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: "has_column_privilege()" issue with attnums and non-existent columns

Joe Conway
On 3/30/21 6:22 PM, Tom Lane wrote:

> Joe Conway <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: "has_column_privilege()" issue with attnums and non-existent columns

Joe Conway
On 3/30/21 8:17 PM, Joe Conway wrote:

> On 3/30/21 6:22 PM, Tom Lane wrote:
>> Joe Conway <[hidden email]> 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