Re: Grantor name gets lost when grantor role dropped

I am sending this email on behalf of Russel Smith. He discovered this
bug and his description follows:

Verified on 8.2.3 on Fedora Core 6
Verified on 8.1.3 on RHEL4, custom compile. (I can't control the update to 8.1.8)

The output of an empty role name would possibly not be a problem, but when you are
doing a dump and restore, pg_dumpall dumps invalid syntax as below;

GRANT "postgres" TO "test_role" GRANTED BY "";

We either need to rethink the way we handle grantor information and when it's valid.
Or we need to at least allow dump/restore to work as expected when a dropped role
granted privileges to other users.

To add to my woes when investigating this, GRANTED BY syntax is not included in the
8.2 documentation at all. It's not listed as valid syntax, and there are no
comments saying what it does.

The self contained test case to produce this is below;

Regards

Russell Smith

psql postgres < bug.sql 2>&1 > output.txt

CREATE ROLE test_role
NOSUPERUSER INHERIT NOCREATEDB NOCREATEROLE;

CREATE ROLE invalid_grantor
SUPERUSER INHERIT NOCREATEDB NOCREATEROLE;

SET ROLE invalid_grantor;
GRANT "postgres" TO "test_role";
SET ROLE postgres;

select * from pg_roles;

select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members LEFT JOIN pg_roles ur ON roleid = oid
LEFT JOIN pg_roles gr ON gr.oid = grantor;

DROP ROLE invalid_grantor;

select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members LEFT JOIN pg_roles ur ON roleid = oid
LEFT JOIN pg_roles gr ON gr.oid = grantor;

DROP ROLE test_role;

Jeff Davis wrote:

> CREATE ROLE test_role
> NOSUPERUSER INHERIT NOCREATEDB NOCREATEROLE;
>
> CREATE ROLE invalid_grantor
> SUPERUSER INHERIT NOCREATEDB NOCREATEROLE;
>
> SET ROLE invalid_grantor;
> GRANT "postgres" TO "test_role";
> SET ROLE postgres;
>
> select * from pg_roles;
>
> select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members LEFT JOIN pg_roles ur ON roleid = oid
> LEFT JOIN pg_roles gr ON gr.oid = grantor;
>
> DROP ROLE invalid_grantor;
>
> select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members LEFT JOIN pg_roles ur ON roleid = oid
> LEFT JOIN pg_roles gr ON gr.oid = grantor;
>
> DROP ROLE test_role;

The problem here is that we allowed the drop of invalid_grantor. We are
missing a shared dependency on it.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Jeff Davis wrote:
>
>
>> CREATE ROLE test_role
>> NOSUPERUSER INHERIT NOCREATEDB NOCREATEROLE;
>>
>> CREATE ROLE invalid_grantor
>> SUPERUSER INHERIT NOCREATEDB NOCREATEROLE;
>>
>> SET ROLE invalid_grantor;
>> GRANT "postgres" TO "test_role";
>> SET ROLE postgres;
>>
>> select * from pg_roles;
>>
>> select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members LEFT JOIN pg_roles ur ON roleid = oid
>> LEFT JOIN pg_roles gr ON gr.oid = grantor;
>>
>> DROP ROLE invalid_grantor;
>>
>> select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members LEFT JOIN pg_roles ur ON roleid = oid
>> LEFT JOIN pg_roles gr ON gr.oid = grantor;
>>
>> DROP ROLE test_role;
>>
>
> The problem here is that we allowed the drop of invalid_grantor. We are
> missing a shared dependency on it.
>
So does this make a todo item?

But this still leaves the concerns about you can currently get the
database into an invalid state that can't be dumped and restored.

Russell Smith wrote:
> Alvaro Herrera wrote:
> >Jeff Davis wrote:
> >
> >
> >>CREATE ROLE test_role
> >> NOSUPERUSER INHERIT NOCREATEDB NOCREATEROLE;
> >>
> >>CREATE ROLE invalid_grantor
> >> SUPERUSER INHERIT NOCREATEDB NOCREATEROLE;
> >>
> >>SET ROLE invalid_grantor;
> >>GRANT "postgres" TO "test_role";
> >>SET ROLE postgres;
> >>
> >>select * from pg_roles;
> >>
> >>select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members
> >>LEFT JOIN pg_roles ur ON roleid = oid
> >>LEFT JOIN pg_roles gr ON gr.oid = grantor;
> >>
> >>DROP ROLE invalid_grantor;
> >>
> >>select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members
> >>LEFT JOIN pg_roles ur ON roleid = oid
> >>LEFT JOIN pg_roles gr ON gr.oid = grantor;
> >>
> >>DROP ROLE test_role;
> >>
> >
> >The problem here is that we allowed the drop of invalid_grantor. We are
> >missing a shared dependency on it.
> >
> So does this make a todo item?
>
> But this still leaves the concerns about you can currently get the
> database into an invalid state that can't be dumped and restored.

Correct, which makes it a bug (==> needs fixed) rather than a todo item.

We now have a problem because there may already be databases that are
undumpable. We might need to provide a workaround for people with such
a database.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Russell Smith wrote:
>
>> Alvaro Herrera wrote:
>>
>>> Jeff Davis wrote:
>>>
>>>
>>>
>>>> CREATE ROLE test_role
>>>> NOSUPERUSER INHERIT NOCREATEDB NOCREATEROLE;
>>>>
>>>> CREATE ROLE invalid_grantor
>>>> SUPERUSER INHERIT NOCREATEDB NOCREATEROLE;
>>>>
>>>> SET ROLE invalid_grantor;
>>>> GRANT "postgres" TO "test_role";
>>>> SET ROLE postgres;
>>>>
>>>> select * from pg_roles;
>>>>
>>>> select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members
>>>> LEFT JOIN pg_roles ur ON roleid = oid
>>>> LEFT JOIN pg_roles gr ON gr.oid = grantor;
>>>>
>>>> DROP ROLE invalid_grantor;
>>>>
>>>> select pg_auth_members.*, ur.rolname, gr.rolname from pg_auth_members
>>>> LEFT JOIN pg_roles ur ON roleid = oid
>>>> LEFT JOIN pg_roles gr ON gr.oid = grantor;
>>>>
>>>> DROP ROLE test_role;
>>>>
>>>>
>>> The problem here is that we allowed the drop of invalid_grantor. We are
>>> missing a shared dependency on it.
>>>
>>>
>> So does this make a todo item?
>>
>> But this still leaves the concerns about you can currently get the
>> database into an invalid state that can't be dumped and restored.
>>
>
> Correct, which makes it a bug (==> needs fixed) rather than a todo item.
>
> We now have a problem because there may already be databases that are
> undumpable. We might need to provide a workaround for people with such
> a database.
>
Well, given GRANTED BY is not documented, it should be reasonable to
alter pg_dumpall to remove GRANTED BY in cases where the role doesn't
resolve.

That is not an unsafe change as it should never happen if the depend
data is in place. It will also allow any currently undumpable databases
to be dumpable again.

A simple and totally untested or compiled patch is below;

Index: pg_dumpall.c
===================================================================
RCS file: /projects/cvsroot/pgsql/src/bin/pg_dump/pg_dumpall.c,v
retrieving revision 1.90
diff -c -r1.90 pg_dumpall.c
*** pg_dumpall.c 10 Feb 2007 14:58:55 -0000 1.90
--- pg_dumpall.c 18 Apr 2007 07:41:44 -0000
***************
*** 724,730 ****
fprintf(OPF, " TO %s", fmtId(member));
if (*option == 't')
fprintf(OPF, " WITH ADMIN OPTION");
! fprintf(OPF, " GRANTED BY %s;\n", fmtId(grantor));
}

PQclear(res);
--- 724,739 ----
fprintf(OPF, " TO %s", fmtId(member));
if (*option == 't')
fprintf(OPF, " WITH ADMIN OPTION");
! /*
! * It's possible due to a lack of shared dependency tracking
! * of grantor that this parameter and be an empty string.
! * In that case, we don't dump the grantor and the grant
! * will be granted by the user who imports the roles.
! */
! if (strlen(grantor) != 0)
! fprintf(OPF, " GRANTED BY %s", fmtId(grantor));
!
! fprintf(OPF, ";\n");
}

PQclear(res);

Russell Smith wrote:
>>>>> CREATE ROLE test_role
>>>>> NOSUPERUSER INHERIT NOCREATEDB NOCREATEROLE;
>>>>>
>>>>> CREATE ROLE invalid_grantor
>>>>> SUPERUSER INHERIT NOCREATEDB NOCREATEROLE;
>>>>>
>>>>> SET ROLE invalid_grantor;
>>>>> GRANT "postgres" TO "test_role";
>>>>> SET ROLE postgres;
>>>>>
>>>>> select * from pg_roles;
>>>>>
>>>>> select pg_auth_members.*, ur.rolname, gr.rolname from
>>>>> pg_auth_members LEFT JOIN pg_roles ur ON roleid = oid
>>>>> LEFT JOIN pg_roles gr ON gr.oid = grantor;
>>>>>
>>>>> DROP ROLE invalid_grantor;
>>>>>
>>>>> select pg_auth_members.*, ur.rolname, gr.rolname from
>>>>> pg_auth_members LEFT JOIN pg_roles ur ON roleid = oid
>>>>> LEFT JOIN pg_roles gr ON gr.oid = grantor;
>>>>>
>>>>> DROP ROLE test_role;
>>>>>
>>> So does this make a todo item?
>>>
>>> But this still leaves the concerns about you can currently get the
>>> database into an invalid state that can't be dumped and restored.
>>>
>>
>> Correct, which makes it a bug (==> needs fixed) rather than a todo item.
>>
>> We now have a problem because there may already be databases that are
>> undumpable. We might need to provide a workaround for people with such
>> a database.
>>
[snip]

As I am not a frequent reporter of bugs, what happens now? It's been a
week since I wrote my last message and I'm unsure of whether anything
has, or is going to happen about this bug report. Alvaro has said it's
a "bug", so it needs fixing. But that has not translated into somebody
saying they will look at it. I've also had no comments on the possible
patch I attached in my last email.

I understand people are busy, but as a follower of pg development, from
this experience I can see who some new people get the feeling of not
being looked after. Even if something is happening, there is little
flow of information.

I would greatly appreciate an update, even though this is not a server
crash, it does allow creation of dumps that cannot be restored. They
can be alter to be restored, but I didn't think that was the point.

Regards

Russell Smith

Russell Smith wrote:

> As I am not a frequent reporter of bugs, what happens now? It's been a
> week since I wrote my last message and I'm unsure of whether anything
> has, or is going to happen about this bug report. Alvaro has said it's
> a "bug", so it needs fixing. But that has not translated into somebody
> saying they will look at it. I've also had no comments on the possible
> patch I attached in my last email.

Sorry. It's on my to-do list and I'll fix it shortly. I am very sorry
that the fix did not make it into 8.2.4.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Russell Smith wrote:
>
>
>> As I am not a frequent reporter of bugs, what happens now? It's been a
>> week since I wrote my last message and I'm unsure of whether anything
>> has, or is going to happen about this bug report. Alvaro has said it's
>> a "bug", so it needs fixing. But that has not translated into somebody
>> saying they will look at it. I've also had no comments on the possible
>> patch I attached in my last email.
>>
>
> Sorry. It's on my to-do list and I'll fix it shortly. I am very sorry
> that the fix did not make it into 8.2.4.
>
Thanks for the update it's most appreciated. I'm not so worried it
didn't make it into 8.2.4, just wanted to make sure it had made it to
somebodies todo list. I did post a pg_dumpall fix to make it so you can
dump existing invalid databases.

Regards

Russell

Jeff Davis wrote:

> GRANT "postgres" TO "test_role" GRANTED BY "";
>
> We either need to rethink the way we handle grantor information and when it's valid.
> Or we need to at least allow dump/restore to work as expected when a dropped role
> granted privileges to other users.

I've been staring at this for a while. Upon first reading it, I thought
that it would be simply a matter of adding pg_shdepend entries for the
pg_auth_members rows. This starts sounding suspicious the moment you
consider that there will be one pg_shdepend entry for each role granted,
that is, a lot.

The second problem with this idea is that it's not at all possible,
because pg_shdepend entries can reference an object by OID, but
pg_auth_members rows don't have OIDs. So the most we could do is add
entries for pg_authid.

So I'm currently considering the following alternatives:

1. do nothing at all with pg_shdepend. Upon role deletion, seqscan
pg_auth_members and reject the drop altogether if there is a role
granted to another which mentions the to-be-dropped role ID as grantor.
This is easiest in terms of code (it's even mentioned in the comments in
DropRole).

2. record one pg_shdepend entry for each role that has granted something
to each role (unless the grantor is the same role being granted, in
which case we needn't record anything). So if role A grants Z and X to
C, and role B grants Y and W to C, C now has access to W, Y, X and Z and
there are two pg_shdepend entries:
C -> A
C -> B
So dropping a role would be disallowed automatically without any code
changes, with the checkSharedDependencies() call that's already in
DropRole. Adding a role membership would require a bit more work,
because we'd first need to check that there's not already a pg_shdepend
entry for that combination. Removing a role membership also becomes
more work; we need to check that no other grant depends on the same
grantor before removing the entry.
Note that I'm considering that this alternative requires adding a
GRANTOR symbol to the SharedDependencyType, which probably rules this
out for backpatching.

Comments? I'm leaning towards implementing (2). The patch for
pg_dumpall would also be needed.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> So I'm currently considering the following alternatives:

> 1. do nothing at all with pg_shdepend. Upon role deletion, seqscan
> pg_auth_members and reject the drop altogether if there is a role
> granted to another which mentions the to-be-dropped role ID as grantor.
> This is easiest in terms of code (it's even mentioned in the comments in
> DropRole).

> 2. record one pg_shdepend entry for each role that has granted something
> to each role (unless the grantor is the same role being granted, in
> which case we needn't record anything). So if role A grants Z and X to
> C, and role B grants Y and W to C, C now has access to W, Y, X and Z and
> there are two pg_shdepend entries:
> C -> A
> C -> B
> So dropping a role would be disallowed automatically without any code
> changes, with the checkSharedDependencies() call that's already in
> DropRole. Adding a role membership would require a bit more work,
> because we'd first need to check that there's not already a pg_shdepend
> entry for that combination. Removing a role membership also becomes
> more work; we need to check that no other grant depends on the same
> grantor before removing the entry.

Both of these have got race conditions ... not but what the dependency
code has got race condition problems already, but maybe we should try
to avoid introducing more? I haven't got any better ideas though.

Why is it that we record grantor at all? One could argue that granting
membership in a role is done on behalf of that role and there's no real
need to remember exactly who did it.

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> > So I'm currently considering the following alternatives:
>
> > 1. do nothing at all with pg_shdepend. Upon role deletion, seqscan
> > pg_auth_members and reject the drop altogether if there is a role
> > granted to another which mentions the to-be-dropped role ID as grantor.
> > This is easiest in terms of code (it's even mentioned in the comments in
> > DropRole).
>
> > 2. record one pg_shdepend entry for each role that has granted something
> > to each role (unless the grantor is the same role being granted, in
> > which case we needn't record anything). So if role A grants Z and X to
> > C, and role B grants Y and W to C, C now has access to W, Y, X and Z and
> > there are two pg_shdepend entries:
> > C -> A
> > C -> B
> > So dropping a role would be disallowed automatically without any code
> > changes, with the checkSharedDependencies() call that's already in
> > DropRole. Adding a role membership would require a bit more work,
> > because we'd first need to check that there's not already a pg_shdepend
> > entry for that combination. Removing a role membership also becomes
> > more work; we need to check that no other grant depends on the same
> > grantor before removing the entry.
>
> Both of these have got race conditions ... not but what the dependency
> code has got race condition problems already, but maybe we should try
> to avoid introducing more? I haven't got any better ideas though.

I couldn't parse this paragraph very well. However I'm not sure why you
say the dependency code has got race conditions? We do lock the object
before checking the dependencies, so it's not possible to add a new
dependency while we're dropping the object.

.. right? I'm going to have a look at it again.

> Why is it that we record grantor at all? One could argue that granting
> membership in a role is done on behalf of that role and there's no real
> need to remember exactly who did it.

I think you should ask Stephen Frost about that -- added to CC.

If the grantor bit is not important, then what we should do is just omit
emitting the GRANTED BY part in pg_dumpall, which fixes this report.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:
> Tom Lane wrote:
>> Both of these have got race conditions ... not but what the dependency
>> code has got race condition problems already, but maybe we should try
>> to avoid introducing more? I haven't got any better ideas though.

> I couldn't parse this paragraph very well. However I'm not sure why you
> say the dependency code has got race conditions? We do lock the object
> before checking the dependencies, so it's not possible to add a new
> dependency while we're dropping the object.

Sorry, I was thinking of the regular dependency code, which has open
bug report(s) based on exactly the fact that there's no such locking.
shdepend may be OK, since it's fundamentally only dealing in roles.

>> Why is it that we record grantor at all? One could argue that granting
>> membership in a role is done on behalf of that role and there's no real
>> need to remember exactly who did it.

> I think you should ask Stephen Frost about that -- added to CC.

> If the grantor bit is not important, then what we should do is just omit
> emitting the GRANTED BY part in pg_dumpall, which fixes this report.

It's at least something we should reflect on before sweating hard to
make it work...

regards, tom lane

Tom Lane wrote:
> Alvaro Herrera <alvherre(at)commandprompt(dot)com> writes:

> >> Why is it that we record grantor at all? One could argue that granting
> >> membership in a role is done on behalf of that role and there's no real
> >> need to remember exactly who did it.
>
> > I think you should ask Stephen Frost about that -- added to CC.
>
> > If the grantor bit is not important, then what we should do is just omit
> > emitting the GRANTED BY part in pg_dumpall, which fixes this report.
>
> It's at least something we should reflect on before sweating hard to
> make it work...

I took a look, and concluded that the only bit of code that uses the
grantor at all is pg_dumpall.

Does this means we can remove it altogether? In back branches, we would
take out the pg_dumpall code.

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

* Alvaro Herrera (alvherre(at)commandprompt(dot)com) wrote:
> I took a look, and concluded that the only bit of code that uses the
> grantor at all is pg_dumpall.
>
> Does this means we can remove it altogether? In back branches, we would
> take out the pg_dumpall code.

I don't have time right at the moment (leaving shortly and will be gone
all weekend) but what I would do is check the SQL standard, especially
the information schema, for any requirement to track the grantor. Much
of what I did was based on the standard so that may have been the
instigation for tracking grantor. Though, even without that, we track
the grantor of most other grants (possibly all currently?) and it seems
like a useful bit of information for DBAs to be able to know who granted
what to whom.

I can't say I've used it though, personally. Of course, I'll be pretty
unhappy if a day comes when I do need it and it's not there. :)

Thanks,

Stephen

Stephen Frost wrote:

> I don't have time right at the moment (leaving shortly and will be gone
> all weekend) but what I would do is check the SQL standard, especially
> the information schema, for any requirement to track the grantor. Much
> of what I did was based on the standard so that may have been the
> instigation for tracking grantor.

Hmm. I had forgotten the information schema. I just checked: the only
view using pg_auth_members is APPLICABLE_ROLES, and that one doesn't
display the grantor column.

> Though, even without that, we track
> the grantor of most other grants (possibly all currently?) and it seems
> like a useful bit of information for DBAs to be able to know who granted
> what to whom.

I note that the grantor of ACLs are listed separately, for example in
COLUMN_PRIVILEGES, ROLE_COLUMN_GRANTS, etc.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Stephen Frost wrote:
>
> > I don't have time right at the moment (leaving shortly and will be gone
> > all weekend) but what I would do is check the SQL standard, especially
> > the information schema, for any requirement to track the grantor. Much
> > of what I did was based on the standard so that may have been the
> > instigation for tracking grantor.
>
> Hmm. I had forgotten the information schema. I just checked: the only
> view using pg_auth_members is APPLICABLE_ROLES, and that one doesn't
> display the grantor column.

This section of the standard is relevant:
4.34.3 Roles

Each grant is represented and identified by a role authorization descriptor. A
role authorization descriptor includes:

— The role name of the role.
— The <authorization identifier> of the grantor.
— The <authorization identifier> of the grantee.
— An indication of whether or not the role was granted with the WITH ADMIN
OPTION and hence is grantable.

... continues reading the spec ...

Ah, here it is, 12.7 <revoke statement>. It says that if role revokes
another role from a third role, it will only remove the privileges that
were granted by him, not someone else.

That is, if roles A and B grant a role Z to C, and then role A revokes Z
from C, then role C continues to have the role Z because of the grant B
gave.

So we have a problem here, because this

alvherre=# create role a;
CREATE ROLE
alvherre=# create role b;
CREATE ROLE
alvherre=# create role z admin a, b;
CREATE ROLE
alvherre=# create role c;
CREATE ROLE
alvherre=# set session authorization a;
SET
alvherre=> grant z to c;
GRANT ROLE
alvherre=> set session authorization b;
SET
alvherre=> grant z to c;
NOTICE: role "c" is already a member of role "z"

should not emit any noise, but instead add another grant of Z to C with
grantor B.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

* Alvaro Herrera (alvherre(at)commandprompt(dot)com) wrote:
> Ah, here it is, 12.7 <revoke statement>. It says that if role revokes
> another role from a third role, it will only remove the privileges that
> were granted by him, not someone else.

Hmm. I'm not sure, but that may have been a case where it was generally
decided that the spec was somewhat braindead in this fashion (it seems
so in my personal view of this, honestly...). To issue a revoke and
have it not work would be kind of concerning. If we do end up following
this path we should emit a warning (at least...) if the user still has
the rights which are being revoked, even if through someone else.
Perhaps that also implies that tracking the grantor is unnecessary.

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> Hmm. I'm not sure, but that may have been a case where it was generally
> decided that the spec was somewhat braindead in this fashion (it seems
> so in my personal view of this, honestly...). To issue a revoke and
> have it not work would be kind of concerning. If we do end up following
> this path we should emit a warning (at least...) if the user still has
> the rights which are being revoked, even if through someone else.

That's not how it works for rights on ordinary objects.

regards, tom lane

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > Hmm. I'm not sure, but that may have been a case where it was generally
> > decided that the spec was somewhat braindead in this fashion (it seems
> > so in my personal view of this, honestly...). To issue a revoke and
> > have it not work would be kind of concerning. If we do end up following
> > this path we should emit a warning (at least...) if the user still has
> > the rights which are being revoked, even if through someone else.
>
> That's not how it works for rights on ordinary objects.

Not quite sure which bit you're referring to here.. On 8.1, at least,
we ignore a grant which has a matching right and target:

sfrost=> set role u1;
sfrost=> \dp
Access privileges for database "sfrost"
Schema | Name | Type | Access privileges
--------+------+-------+-------------------------
sfrost | test | table | {u1=arwdRxt/u1,u3=r/u1}
(1 row)

sfrost=> reset role;
RESET
sfrost=> set role u2;
SET
sfrost=> grant select on test to u3;
GRANT
sfrost=> \dp
Access privileges for database "sfrost"
Schema | Name | Type | Access privileges
--------+------+-------+-------------------------
sfrost | test | table | {u1=arwdRxt/u1,u3=r/u1}
(1 row)

Additionally, any user with ownership rights on the table in question
can revoke the rights of a user. Still as u2:

sfrost=> revoke select on test from u3;
REVOKE
sfrost=> \dp
Access privileges for database "sfrost"
Schema | Name | Type | Access privileges
--------+------+-------+-------------------
sfrost | test | table | {u1=arwdRxt/u1}
(1 row)

If you're saying we don't currently warn if a revoke leaves the
priviledges in-tact for the right and target, I'm not sure you can
currently get in a state where it'd be possible to run into that.
Either you have the rights to remove the grant on the object
(you're an 'owner' of it), in which case the grant will be removed
if it exists (based on the right and target, regardless of who
granted it), or you don't, in which case you get a permission denied
ERROR outright. If regular object permissions were ever changed to
require the grantor to be the revoker, I would want a warning in the
case described for regular objects as well.

If you're saying we don't currently require that the grantor be the
revoker on regular objects, I would agree. :)

Thanks,

Stephen

Stephen Frost <sfrost(at)snowman(dot)net> writes:
> If you're saying we don't currently warn if a revoke leaves the
> priviledges in-tact for the right and target, I'm not sure you can
> currently get in a state where it'd be possible to run into that.

I'm thinking of the case that comes up periodically where newbies think
that revoking a right from a particular user overrides a grant to PUBLIC
of the same right.

regards, tom lane

Based on the discussion so far, it seems to me that the sane course of
action is to continue to register the grantor, because the standard
mandates that it should be there; but ignore the parts where we revoke
selectively, because that's a stupid thing to do. So we do deviate, if
slightly.

So we will have pg_dumpall do nothing special if the grantor has gone
away since granting the privilege. That is, exactly the patch that was
submitted, no new code needs to be written. (Maybe a mention in the
"compatibility" section of REVOKE is warranted, though I'm not sure).

Does anyone object to this course of action?

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > If you're saying we don't currently warn if a revoke leaves the
> > priviledges in-tact for the right and target, I'm not sure you can
> > currently get in a state where it'd be possible to run into that.
>
> I'm thinking of the case that comes up periodically where newbies think
> that revoking a right from a particular user overrides a grant to PUBLIC
> of the same right.

Technically, the grant to public is a different target from the target
of the revoke in such a case. Following the spec would mean that even
when the grant and the revoke target is the same (unless you're the
original grantor) the right won't be removed. I'm not against adding a
warning in the case you describe though, but I don't see it being as
necessary for that case. What the spec describes is, at least in my
view, much more counter-intuitive than how PG currently works.

Thanks,

Stephen

Stephen Frost wrote:
> * Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
>
>> Stephen Frost <sfrost(at)snowman(dot)net> writes:
>>
>>> If you're saying we don't currently warn if a revoke leaves the
>>> priviledges in-tact for the right and target, I'm not sure you can
>>> currently get in a state where it'd be possible to run into that.
>>>
>> I'm thinking of the case that comes up periodically where newbies think
>> that revoking a right from a particular user overrides a grant to PUBLIC
>> of the same right.
>>
>
> Technically, the grant to public is a different target from the target
> of the revoke in such a case. Following the spec would mean that even
> when the grant and the revoke target is the same (unless you're the
> original grantor) the right won't be removed. I'm not against adding a
> warning in the case you describe though, but I don't see it being as
> necessary for that case. What the spec describes is, at least in my
> view, much more counter-intuitive than how PG currently works.
>
>
>
If we were to follow the spec, I would expect that it would be possible
for the object owner to revoke privileges no matter what role granted
them. It need not be the default, but as an object owner, I'd expect to
be able to say that I want all privileges for a role revoked, no matter
who granted them.

8.2 docs state this on the revoke page:
--

REVOKE can also be done by a role that is not the owner of the affected
object, but is a member of the role that owns the object, or is a member
of a role that holds privileges WITH GRANT OPTION on the object. In this
case the command is performed as though it were issued by the containing
role that actually owns the object or holds the privileges WITH GRANT
OPTION. For example, if table t1 is owned by role g1, of which role u1
is a member, then u1 can revoke privileges on t1 that are recorded as
being granted by g1. This would include grants made by u1 as well as by
other members of role g1.

If the role executing REVOKE holds privileges indirectly via more than
one role membership path, it is unspecified which containing role will
be used to perform the command. In such cases it is best practice to use
SET ROLE to become the specific role you want to do the REVOKE as.
Failure to do so may lead to revoking privileges other than the ones you
intended, or not
revoking anything at all.

--

Paragraph 1 implies that we are meeting the standard now. I think
paragraph two is stating that if you are a member of multiple roles
which could have granted privileges, then you don't know which one you
are revoking. Makes sense if we are implementing the SQL standard.
Does this mean we were intending to be SQL compliant when we wrote the
documentation?
I also note that 8.1 says the same thing in its documentation.

My possible suggestion is;
1. Implement the standard for revoking only your privileges by default.
2. Allow the object owner to revoke privileges assigned by any role, as
if you drop and recreate the object you can achieve this anyway.

Regards

Russell Smith

So the discussion died again with nothing being decided. I see we have
several choices:

1. implement the standard, per Russell suggestion below
2. decide that the standard is braindead and just omit dumping the
grantor when it's no longer available, but don't remove
pg_auth_members.grantor
3. decide that the standard is braindead and remove
pg_auth_members.grantor

Which do people feel should be implemented? I can do whatever we
decide; if no one has a strong opinion on the matter, my opinion is we
do (2) which is the easiest.

Russell Smith wrote:

> My possible suggestion is;
> 1. Implement the standard for revoking only your privileges by default.
> 2. Allow the object owner to revoke privileges assigned by any role, as
> if you drop and recreate the object you can achieve this anyway.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:

> 2. decide that the standard is braindead and just omit dumping the
> grantor when it's no longer available, but don't remove
> pg_auth_members.grantor
>
> Which do people feel should be implemented? I can do whatever we
> decide; if no one has a strong opinion on the matter, my opinion is we
> do (2) which is the easiest.

Here is a patch implementing this idea, vaguely based on Russell's.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Alvaro Herrera wrote:
>
>
>> 2. decide that the standard is braindead and just omit dumping the
>> grantor when it's no longer available, but don't remove
>> pg_auth_members.grantor
>>
>> Which do people feel should be implemented? I can do whatever we
>> decide; if no one has a strong opinion on the matter, my opinion is we
>> do (2) which is the easiest.
>>
>
> Here is a patch implementing this idea, vaguely based on Russell's.
>

I haven't had time to finalize my research about this, but the admin
option with revoke doesn't appear to work as expected.

Here is my sample SQL for 8.2.4

create table test (x integer);
\z
create role test1 noinherit;
create role test2 noinherit;
grant select on test to test1 with grant option;
grant select on test to test2;
\z test
set role test1;
revoke select on test from test2;
\z test
set role test2;
select * from test;
reset role;
revoke all on test from test2;
revoke all on test from test1;
drop role test2;
drop role test1;
drop table test;
\q

The privilege doesn't appear to be revoked by test1 from test2. I'm not
sure if this is related, but I wanted to bring it up in light of the
options we have for grantor.

Russell Smith wrote:
> Alvaro Herrera wrote:
> >Alvaro Herrera wrote:
> >
> >
> >>2. decide that the standard is braindead and just omit dumping the
> >> grantor when it's no longer available, but don't remove
> >> pg_auth_members.grantor
> >>
> >>Which do people feel should be implemented? I can do whatever we
> >>decide; if no one has a strong opinion on the matter, my opinion is we
> >>do (2) which is the easiest.
> >
> >Here is a patch implementing this idea, vaguely based on Russell's.
>
> I haven't had time to finalize my research about this, but the admin
> option with revoke doesn't appear to work as expected.
>
> Here is my sample SQL for 8.2.4
>
> create table test (x integer);
> \z
> create role test1 noinherit;
> create role test2 noinherit;
> grant select on test to test1 with grant option;
> grant select on test to test2;
> \z test
> set role test1;
> revoke select on test from test2;
> \z test
> set role test2;
> select * from test;
> reset role;
> revoke all on test from test2;
> revoke all on test from test1;
> drop role test2;
> drop role test1;
> drop table test;
> \q
>
>
> The privilege doesn't appear to be revoked by test1 from test2. I'm not
> sure if this is related, but I wanted to bring it up in light of the
> options we have for grantor.

Humm, but the privilege was not granted by test1, but by the user you
were using initially. The docs state in a note that

A user can only revoke privileges that were granted directly by
that user.

I understand that this would apply to the grantor stuff being discussed
in this thread as well, but I haven't seen anyone arguing that we should
implement that for GRANT ROLE (and I asked three times if people felt it
was important and nobody answered).

--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Alvaro Herrera wrote:
> Alvaro Herrera wrote:
>
> > 2. decide that the standard is braindead and just omit dumping the
> > grantor when it's no longer available, but don't remove
> > pg_auth_members.grantor
> >
> > Which do people feel should be implemented? I can do whatever we
> > decide; if no one has a strong opinion on the matter, my opinion is we
> > do (2) which is the easiest.
>
> Here is a patch implementing this idea, vaguely based on Russell's.

Applied to CVS HEAD, 8.2 and 8.1.

If we want to start tracking the grantor as a shared dependency, and
have REVOKE work per spec (i.e. only revoke the privileges actually
granted by the role executing REVOKE), those are separate patches (and
they should be applied only to HEAD). This patch merely fixes the fact
that pg_dumpall failed to work for busted databases.

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Alvaro Herrera wrote:
> Alvaro Herrera wrote:
>
>> Alvaro Herrera wrote:
>>
>>
>>> 2. decide that the standard is braindead and just omit dumping the
>>> grantor when it's no longer available, but don't remove
>>> pg_auth_members.grantor
>>>
>>> Which do people feel should be implemented? I can do whatever we
>>> decide; if no one has a strong opinion on the matter, my opinion is we
>>> do (2) which is the easiest.
>>>
>> Here is a patch implementing this idea, vaguely based on Russell's.
>>
>
> Applied to CVS HEAD, 8.2 and 8.1.
>
> If we want to start tracking the grantor as a shared dependency, and
> have REVOKE work per spec (i.e. only revoke the privileges actually
> granted by the role executing REVOKE), those are separate patches (and
> they should be applied only to HEAD). This patch merely fixes the fact
> that pg_dumpall failed to work for busted databases.
>
>
Should there also be a doc patch for this, the document descriptions
seemed different to what is actually implemented. I'll check that
before I make any further comments.

Russell

Alvaro Herrera wrote:
> Russell Smith wrote:
>
>> Alvaro Herrera wrote:
>>
>>> Alvaro Herrera wrote:
>>>
>>>
>>>
>>>> 2. decide that the standard is braindead and just omit dumping the
>>>> grantor when it's no longer available, but don't remove
>>>> pg_auth_members.grantor
>>>>
>>>> Which do people feel should be implemented? I can do whatever we
>>>> decide; if no one has a strong opinion on the matter, my opinion is we
>>>> do (2) which is the easiest.
>>>>
>>> Here is a patch implementing this idea, vaguely based on Russell's.
>>>
>> I haven't had time to finalize my research about this, but the admin
>> option with revoke doesn't appear to work as expected.
>>
>> Here is my sample SQL for 8.2.4
>>
>> create table test (x integer);
>> \z
>> create role test1 noinherit;
>> create role test2 noinherit;
>> grant select on test to test1 with grant option;
>> grant select on test to test2;
>> \z test
>> set role test1;
>> revoke select on test from test2;
>> \z test
>> set role test2;
>> select * from test;
>> reset role;
>> revoke all on test from test2;
>> revoke all on test from test1;
>> drop role test2;
>> drop role test1;
>> drop table test;
>> \q
>>
>>
>> The privilege doesn't appear to be revoked by test1 from test2. I'm not
>> sure if this is related, but I wanted to bring it up in light of the
>> options we have for grantor.
>>
>
> Humm, but the privilege was not granted by test1, but by the user you
> were using initially. The docs state in a note that
>
> A user can only revoke privileges that were granted directly by
> that user.
>
> I understand that this would apply to the grantor stuff being discussed
> in this thread as well, but I haven't seen anyone arguing that we should
> implement that for GRANT ROLE (and I asked three times if people felt it
> was important and nobody answered).
>
>
Well, I would vote for implementing this in GRANT ROLE. I wish to use
it in my security model. I don't think the spec is brain dead when you
understand what it's trying to achieve.

Example:

2 Groups of administrators who are allowed to grant a role to users of
the system

App_Admin_G1
App_Admin_G2
App_User

SET ROLE App_Admin_G1
GRANT App_User TO "Fred";
SET ROLE App_Admin_G2
GRANT App_User TO "John";
SET ROLE App_Admin_G1
REVOKE App_User FROM "John";

As App_Admin_G1 did not grant App_User rights to John, he should not be
able to take them away.

I currently have a situation where I would like to be able to do the
above. I have two separate departments who might grant privileges for
the same application to the same user. One department administrator
should not be able to revoke the privileges set by the other one.

I would expect superusers to be able to revoke from anybody, or the
"owner". I'm not sure what the owner is when we talk about granting roles.

Regards

Russell Smith

Is there a TODO here?

---------------------------------------------------------------------------

Russell Smith wrote:
> Alvaro Herrera wrote:
> > Russell Smith wrote:
> >
> >> Alvaro Herrera wrote:
> >>
> >>> Alvaro Herrera wrote:
> >>>
> >>>
> >>>
> >>>> 2. decide that the standard is braindead and just omit dumping the
> >>>> grantor when it's no longer available, but don't remove
> >>>> pg_auth_members.grantor
> >>>>
> >>>> Which do people feel should be implemented? I can do whatever we
> >>>> decide; if no one has a strong opinion on the matter, my opinion is we
> >>>> do (2) which is the easiest.
> >>>>
> >>> Here is a patch implementing this idea, vaguely based on Russell's.
> >>>
> >> I haven't had time to finalize my research about this, but the admin
> >> option with revoke doesn't appear to work as expected.
> >>
> >> Here is my sample SQL for 8.2.4
> >>
> >> create table test (x integer);
> >> \z
> >> create role test1 noinherit;
> >> create role test2 noinherit;
> >> grant select on test to test1 with grant option;
> >> grant select on test to test2;
> >> \z test
> >> set role test1;
> >> revoke select on test from test2;
> >> \z test
> >> set role test2;
> >> select * from test;
> >> reset role;
> >> revoke all on test from test2;
> >> revoke all on test from test1;
> >> drop role test2;
> >> drop role test1;
> >> drop table test;
> >> \q
> >>
> >>
> >> The privilege doesn't appear to be revoked by test1 from test2. I'm not
> >> sure if this is related, but I wanted to bring it up in light of the
> >> options we have for grantor.
> >>
> >
> > Humm, but the privilege was not granted by test1, but by the user you
> > were using initially. The docs state in a note that
> >
> > A user can only revoke privileges that were granted directly by
> > that user.
> >
> > I understand that this would apply to the grantor stuff being discussed
> > in this thread as well, but I haven't seen anyone arguing that we should
> > implement that for GRANT ROLE (and I asked three times if people felt it
> > was important and nobody answered).
> >
> >
> Well, I would vote for implementing this in GRANT ROLE. I wish to use
> it in my security model. I don't think the spec is brain dead when you
> understand what it's trying to achieve.
>
> Example:
>
> 2 Groups of administrators who are allowed to grant a role to users of
> the system
>
> App_Admin_G1
> App_Admin_G2
> App_User
>
> SET ROLE App_Admin_G1
> GRANT App_User TO "Fred";
> SET ROLE App_Admin_G2
> GRANT App_User TO "John";
> SET ROLE App_Admin_G1
> REVOKE App_User FROM "John";
>
> As App_Admin_G1 did not grant App_User rights to John, he should not be
> able to take them away.
>
> I currently have a situation where I would like to be able to do the
> above. I have two separate departments who might grant privileges for
> the same application to the same user. One department administrator
> should not be able to revoke the privileges set by the other one.
>
> I would expect superusers to be able to revoke from anybody, or the
> "owner". I'm not sure what the owner is when we talk about granting roles.
>
> Regards
>
> Russell Smith
>
>
> ---------------------------(end of broadcast)---------------------------
> TIP 5: don't forget to increase your free space map settings

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Bruce Momjian wrote:
>
> Is there a TODO here?

Yes, I think so:

* Implement the SQL standard mechanism whereby REVOKE ROLE only revokes
the privilege as granted by the invoking role, and not those granted
by other roles

--
Alvaro Herrera http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Added to TODO:

---------------------------------------------------------------------------

Alvaro Herrera wrote:
> Bruce Momjian wrote:
> >
> > Is there a TODO here?
>
> Yes, I think so:
>
> * Implement the SQL standard mechanism whereby REVOKE ROLE only revokes
> the privilege as granted by the invoking role, and not those granted
> by other roles
>
>
> --
> Alvaro Herrera http://www.CommandPrompt.com/
> The PostgreSQL Company - Command Prompt, Inc.

--
Bruce Momjian <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://www.enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +