Re: Bug / shortcoming in has_*_privilege

test_us(at)workbook=# select has_table_privilege( 'public', 'test', 'SELECT' );
ERROR: role "public" does not exist
test_us(at)workbook=#

So there's no way to see if a particular privilege has been granted to public. ISTM 'public' should be accepted, since you can't use it as a role name anyway...

test_us(at)workbook=# create role public;
ERROR: role name "public" is reserved
test_us(at)workbook=# create role "public";
ERROR: role name "public" is reserved
--
Jim C. Nasby, Database Architect jim(at)nasby(dot)net
512.569.9461 (cell) http://jim.nasby.net

On Thu, Jun 10, 2010 at 5:54 PM, Jim Nasby <jim(at)nasby(dot)net> wrote:
> test_us(at)workbook=# select has_table_privilege( 'public', 'test', 'SELECT' );
> ERROR:  role "public" does not exist
> test_us(at)workbook=#
>
> So there's no way to see if a particular privilege has been granted to public. ISTM 'public' should be accepted, since you can't use it as a role name anyway...
>
> test_us(at)workbook=# create role public;
> ERROR:  role name "public" is reserved
> test_us(at)workbook=# create role "public";
> ERROR:  role name "public" is reserved

It's a bit sticky - you could make that work for
has_table_privilege(name, oid, text) or has_table_privilege(name,
text, text), but what would you do about the versions whose first
argument is an oid? It would seem a bit awkward to have the behavior
by asymmetrical, although I guess we could...

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Jun 10, 2010 at 5:54 PM, Jim Nasby <jim(at)nasby(dot)net> wrote:
>> So there's no way to see if a particular privilege has been granted to public. ISTM 'public' should be accepted, since you can't use it as a role name anyway...

> It's a bit sticky - you could make that work for
> has_table_privilege(name, oid, text) or has_table_privilege(name,
> text, text), but what would you do about the versions whose first
> argument is an oid?

Nothing. The only reason to use those forms is in a join against
pg_authid, and the "public" group doesn't have an entry there.

regards, tom lane

On Jun 11, 2010, at 5:18 AM, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> On Thu, Jun 10, 2010 at 5:54 PM, Jim Nasby <jim(at)nasby(dot)net> wrote:
>>> So there's no way to see if a particular privilege has been granted to public. ISTM 'public' should be accepted, since you can't use it as a role name anyway...
>
>> It's a bit sticky - you could make that work for
>> has_table_privilege(name, oid, text) or has_table_privilege(name,
>> text, text), but what would you do about the versions whose first
>> argument is an oid?
>
> Nothing. The only reason to use those forms is in a join against
> pg_authid, and the "public" group doesn't have an entry there.

Cool, I'll have CMD come up with a patch.
--
Jim "Decibel!" Nasby jnasby(at)EnovaFinancial(dot)com
Primary: 512-579-9024 Backup: 512-569-9461

On Thu, 2010-06-10 at 23:18 -0400, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > On Thu, Jun 10, 2010 at 5:54 PM, Jim Nasby <jim(at)nasby(dot)net> wrote:
> >> So there's no way to see if a particular privilege has been granted to public. ISTM 'public' should be accepted, since you can't use it as a role name anyway...
>
> > It's a bit sticky - you could make that work for
> > has_table_privilege(name, oid, text) or has_table_privilege(name,
> > text, text), but what would you do about the versions whose first
> > argument is an oid?
>
> Nothing. The only reason to use those forms is in a join against
> pg_authid, and the "public" group doesn't have an entry there.

ISTM this bug should be on the open items list...

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services

On Wed, Aug 11, 2010 at 3:57 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Thu, 2010-06-10 at 23:18 -0400, Tom Lane wrote:
>> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> > On Thu, Jun 10, 2010 at 5:54 PM, Jim Nasby <jim(at)nasby(dot)net> wrote:
>> >> So there's no way to see if a particular privilege has been granted to public. ISTM 'public' should be accepted, since you can't use it as a role name anyway...
>>
>> > It's a bit sticky - you could make that work for
>> > has_table_privilege(name, oid, text) or has_table_privilege(name,
>> > text, text), but what would you do about the versions whose first
>> > argument is an oid?
>>
>> Nothing.  The only reason to use those forms is in a join against
>> pg_authid, and the "public" group doesn't have an entry there.
>
> ISTM this bug should be on the open items list...

I don't think this is a bug.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

* Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> On Wed, Aug 11, 2010 at 3:57 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > On Thu, 2010-06-10 at 23:18 -0400, Tom Lane wrote:
> >> Nothing.  The only reason to use those forms is in a join against
> >> pg_authid, and the "public" group doesn't have an entry there.
> >
> > ISTM this bug should be on the open items list...
>
> I don't think this is a bug.

Agreed, and it's certainly not something that needs to be dealt with for
9.0..

Thanks,

Stephen

On Wed, 2010-08-11 at 06:48 -0400, Robert Haas wrote:
> On Wed, Aug 11, 2010 at 3:57 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> > On Thu, 2010-06-10 at 23:18 -0400, Tom Lane wrote:
> >> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> >> > On Thu, Jun 10, 2010 at 5:54 PM, Jim Nasby <jim(at)nasby(dot)net> wrote:
> >> >> So there's no way to see if a particular privilege has been granted to public. ISTM 'public' should be accepted, since you can't use it as a role name anyway...
> >>
> >> > It's a bit sticky - you could make that work for
> >> > has_table_privilege(name, oid, text) or has_table_privilege(name,
> >> > text, text), but what would you do about the versions whose first
> >> > argument is an oid?
> >>
> >> Nothing. The only reason to use those forms is in a join against
> >> pg_authid, and the "public" group doesn't have an entry there.
> >
> > ISTM this bug should be on the open items list...
>
> I don't think this is a bug.

It clearly rates higher in importance than most of the things on the
open items list of late...

--
Simon Riggs www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Training and Services

On Wed, Aug 11, 2010 at 8:51 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
> On Wed, 2010-08-11 at 06:48 -0400, Robert Haas wrote:
>> On Wed, Aug 11, 2010 at 3:57 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> > On Thu, 2010-06-10 at 23:18 -0400, Tom Lane wrote:
>> >> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
>> >> > On Thu, Jun 10, 2010 at 5:54 PM, Jim Nasby <jim(at)nasby(dot)net> wrote:
>> >> >> So there's no way to see if a particular privilege has been granted to public. ISTM 'public' should be accepted, since you can't use it as a role name anyway...
>> >>
>> >> > It's a bit sticky - you could make that work for
>> >> > has_table_privilege(name, oid, text) or has_table_privilege(name,
>> >> > text, text), but what would you do about the versions whose first
>> >> > argument is an oid?
>> >>
>> >> Nothing.  The only reason to use those forms is in a join against
>> >> pg_authid, and the "public" group doesn't have an entry there.
>> >
>> > ISTM this bug should be on the open items list...
>>
>> I don't think this is a bug.
>
> It clearly rates higher in importance than most of the things on the
> open items list of late...

First, I don't think that's true. WALreceiver crashing on AIX, the
backup procedure in the manual possibly being wrong, and the
documentation failing to be installed sometimes all seem like they are
clearly more serious issues than this. I am sort of wondering why no
one is working on those issues; apparently, nobody other than me minds
if it takes another three months to get 9.0 out the door. Frankly, I
think the ExplainOnePlan bit is more important, too, although I'm
starting to think we should fix that for 9.1 rather than 9.0.

Second, even if it were true, the fact that something is important
does not make it a bug fix.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise Postgres Company

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Wed, Aug 11, 2010 at 8:51 AM, Simon Riggs <simon(at)2ndquadrant(dot)com> wrote:
>> It clearly rates higher in importance than most of the things on the
>> open items list of late...

> First, I don't think that's true. WALreceiver crashing on AIX, the
> backup procedure in the manual possibly being wrong, and the
> documentation failing to be installed sometimes all seem like they are
> clearly more serious issues than this. I am sort of wondering why no
> one is working on those issues; apparently, nobody other than me minds
> if it takes another three months to get 9.0 out the door.

Quite. At this point, the only things that should be on the open items
list are things that would be release stoppers, which is to say things
that are regressions from prior releases or design errors that we don't
want to ever get into a release. This item is not a bug but a feature
omission, and one of rather long standing.

> Frankly, I
> think the ExplainOnePlan bit is more important, too, although I'm
> starting to think we should fix that for 9.1 rather than 9.0.

See above. We are not changing that in 9.0 anymore.

regards, tom lane

Excerpts from Jim Nasby's message of jue jun 10 17:54:43 -0400 2010:
> test_us(at)workbook=# select has_table_privilege( 'public', 'test', 'SELECT' );
> ERROR: role "public" does not exist

Here's a patch implementing this idea.

I'm not too sure about the wording in the doc changes. If somebody
wants to propose something better, I'm all ears. To facilitate
bikeshedding, here's a relevant extract:

has_table_privilege checks whether a user can access a table in
a particular way. The user can be specified by name; as public,
to indicate the PUBLIC pseudo-role; by OID (pg_authid.oid), or,
if the argument is omitted, current_user is assumed.

(the first appearance of public is <literal>public</>. I had first made
it <quote> but that didn't feel right.)

Another thing that could raise eyebrows is that I chose to remove the
"missing_ok" argument from get_role_oid_or_public, so it's not a perfect
mirror of it. None of the current callers need it, but perhaps people
would like these functions to be consistent.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Another thing that could raise eyebrows is that I chose to remove the
> "missing_ok" argument from get_role_oid_or_public, so it's not a perfect
> mirror of it. None of the current callers need it, but perhaps people
> would like these functions to be consistent.

Well, it can't be really consistent anyway: if you did have a missing_ok
argument then you'd need an unusual return convention so you could
distinguish "missing" from "public". As long as this is a static
function I don't see a strong need for it to mimic the API of the
general get_whatever_oid functions.

regards, tom lane

(2010/09/07 6:16), Alvaro Herrera wrote:
> Excerpts from Jim Nasby's message of jue jun 10 17:54:43 -0400 2010:
>> test_us(at)workbook=# select has_table_privilege( 'public', 'test', 'SELECT' );
>> ERROR: role "public" does not exist
>
> Here's a patch implementing this idea.
>
I checked this patch.

It seems to me it replaces whole of get_role_oid() in has_*_privilege
functions by the new get_role_oid_or_public(), so this patch allows
to accept the pseudo "public" user in consistent way.

The pg_has_role_*() functions are exception. It will raise an error
with error message of "role "public" does not exist".
Is it an expected bahavior, isn't it?

> I'm not too sure about the wording in the doc changes. If somebody
> wants to propose something better, I'm all ears. To facilitate
> bikeshedding, here's a relevant extract:
>
> has_table_privilege checks whether a user can access a table in
> a particular way. The user can be specified by name; as public,
> to indicate the PUBLIC pseudo-role; by OID (pg_authid.oid), or,
> if the argument is omitted, current_user is assumed.
>
> (the first appearance of public is<literal>public</>. I had first made
> it<quote> but that didn't feel right.)
>
It seems to me fair enough, but I'm not a native in English.

> Another thing that could raise eyebrows is that I chose to remove the
> "missing_ok" argument from get_role_oid_or_public, so it's not a perfect
> mirror of it. None of the current callers need it, but perhaps people
> would like these functions to be consistent.
>
Tom Lane suggested to add missing_ok argument, although it is not a must-
requirement.

Thanks,
--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

Excerpts from KaiGai Kohei's message of mar oct 05 00:06:05 -0400 2010:
> (2010/09/07 6:16), Alvaro Herrera wrote:
> > Excerpts from Jim Nasby's message of jue jun 10 17:54:43 -0400 2010:
> >> test_us(at)workbook=# select has_table_privilege( 'public', 'test', 'SELECT' );
> >> ERROR: role "public" does not exist
> >
> > Here's a patch implementing this idea.
> >
> I checked this patch.

Thanks.

> It seems to me it replaces whole of get_role_oid() in has_*_privilege
> functions by the new get_role_oid_or_public(), so this patch allows
> to accept the pseudo "public" user in consistent way.

Yes.

> The pg_has_role_*() functions are exception. It will raise an error
> with error message of "role "public" does not exist".
> Is it an expected bahavior, isn't it?

Yes. You cannot grant "public" to roles; according to the definition of
public, this doesn't make sense. Accordingly, I chose to reject
"public" as an input for pg_has_role and friends.

> > Another thing that could raise eyebrows is that I chose to remove the
> > "missing_ok" argument from get_role_oid_or_public, so it's not a perfect
> > mirror of it. None of the current callers need it, but perhaps people
> > would like these functions to be consistent.
> >
> Tom Lane suggested to add missing_ok argument, although it is not a must-
> requirement.

Actually I think he suggested the opposite.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

(2010/10/07 2:05), Alvaro Herrera wrote:
>>> Another thing that could raise eyebrows is that I chose to remove the
>>> "missing_ok" argument from get_role_oid_or_public, so it's not a perfect
>>> mirror of it. None of the current callers need it, but perhaps people
>>> would like these functions to be consistent.
>>>
>> Tom Lane suggested to add missing_ok argument, although it is not a must-
>> requirement.
>
> Actually I think he suggested the opposite.
>
Ahh, I understood his suggestion as literal.

Well, I'd like to mark this patch as 'ready for committer'.

Thanks,
--
KaiGai Kohei <kaigai(at)ak(dot)jp(dot)nec(dot)com>

Hi,

On Tue, Sep 7, 2010 at 6:16 AM, Alvaro Herrera
<alvherre(at)commandprompt(dot)com> wrote:
> Excerpts from Jim Nasby's message of jue jun 10 17:54:43 -0400 2010:
>> test_us(at)workbook=# select has_table_privilege( 'public', 'test', 'SELECT' );
>> ERROR:  role "public" does not exist
>
> Here's a patch implementing this idea.

It specially treats only "public" in all lower cases, right?
The pseudo-role name is described as "PUBLIC" (upper) in docs,
but we accept only "public" (lower) as the pseudo-name.

BTW, does the patch need to be back-patched to older versions?
Since they use get_roleid_checked() instead of get_role_oid(), the fix
cannot be applied cleanly to them, though it will be similar codes.

--
Itagaki Takahiro

On Tue, Oct 12, 2010 at 10:05 PM, Itagaki Takahiro
<itagaki(dot)takahiro(at)gmail(dot)com> wrote:
> BTW, does the patch need to be back-patched to older versions?
> Since they use get_roleid_checked() instead of get_role_oid(), the fix
> cannot be applied cleanly to them, though it will be similar codes.

I would interpret this a a feature, not a bug fix, so no back-patch.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Excerpts from Itagaki Takahiro's message of mar oct 12 23:05:36 -0300 2010:
> Hi,
>
> On Tue, Sep 7, 2010 at 6:16 AM, Alvaro Herrera
> <alvherre(at)commandprompt(dot)com> wrote:
> > Excerpts from Jim Nasby's message of jue jun 10 17:54:43 -0400 2010:
> >> test_us(at)workbook=# select has_table_privilege( 'public', 'test', 'SELECT' );
> >> ERROR:  role "public" does not exist
> >
> > Here's a patch implementing this idea.
>
> It specially treats only "public" in all lower cases, right?
> The pseudo-role name is described as "PUBLIC" (upper) in docs,
> but we accept only "public" (lower) as the pseudo-name.

Yeah, only lowercase. Identifiers other than "public" (all lowercase)
are allowed as role names, so we cannot use them for this purpose. Keep
in mind that the docs say PUBLIC without the quotes, which is lowercased.

> BTW, does the patch need to be back-patched to older versions?

There's no intention to do so.

--
Álvaro Herrera <alvherre(at)commandprompt(dot)com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

>> <alvherre(at)commandprompt(dot)com> wrote:
>> > Excerpts from Jim Nasby's message of jue jun 10 17:54:43 -0400 2010:
>> >> test_us(at)workbook=# select has_table_privilege( 'public', 'test', 'SELECT' );
>> >> ERROR:  role "public" does not exist
>> >
>> > Here's a patch implementing this idea.

I applied it almost as-is, except an unused variable in
get_role_oid_or_public().

>> BTW, does the patch need to be back-patched to older versions?
> There's no intention to do so.

OK. Applied only to HEAD. The issue was reported as a bug,
but we will consider the change as an improvement.

--
Itagaki Takahiro