Обсуждение: REVOKE [ADMIN OPTION FOR] ROLE

Hi,
I found an inconsistency between documentation and real behavior of 
REVOKE [ADMIN OPTION FOR] ROLE.

As per documentation 
(http://www.postgresql.org/docs/9.4/static/sql-revoke.html):

-- 
If GRANT OPTION FOR is specified, only the grant option for the 
privilege is revoked, not the privilege itself. Otherwise, both the 
privilege and the grant option are revoked.
If a user holds a privilege with grant option and has granted it to 
other users then the privileges held by those other users are called 
dependent privileges. If the privilege or the grant option held by the 
first user is being revoked and dependent privileges exist, those 
dependent privileges are also revoked if CASCADE is specified; if it is 
not, the revoke action will fail.
...
When revoking membership in a role, GRANT OPTION is instead called ADMIN 
OPTION, but the behavior is similar.
-- 

So, revoking membership in a role (or admin option for a role) should 
revoke dependent memberships too. In fact it does not.

Here is a script to reproduce the issue:

\c - postgres
create user r1;
create user r2;
create role g;
grant g to r1 with admin option;
\c - r1
grant g to r2 with admin option;
\c - postgres
revoke g from r1 cascade;

I check membership with the following query:
select  (select rolname from pg_roles where oid=am.roleid) "role",  (select rolname from pg_roles where oid=am.member)
member, (select rolname from pg_roles where oid=am.grantor) grantor,  am.admin_option
 
from pg_auth_members am;

Before REVOKE it shows 2 records (which is correct):

role|g
member|r1
grantor|postgres
admin_option|t

role|g
member|r2
grantor|r1
admin_option|t

After revoke it shows 1 record:

role|g
member|r2
grantor|r1
admin_option|t

No records are expected according to documentation.

I looked into the code too (backend/commands/user.c, 
GrantRole(GrantRoleStmt *stmt) function) and didn't find any processing 
of stmt->behavior.

So, the question: is it a documentation bug (as it seems to me), code 
bug, or I missed something?

Thanks,
Egor Rogov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

On Thu, Jul 23, 2015 at 8:30 AM, Egor Rogov <e.rogov@postgrespro.ru> wrote:
> So, the question: is it a documentation bug (as it seems to me), code bug,
> or I missed something?

Your analysis looks right to me, but I don't know whether the code or
the documentation should be changed.  This claim was added by Tom Lane
in 2005 in commit 58d214e51fe50b10b4439da6ec263d54c155afbf.  It might
be worth checking whether the claim was true at that time and later
became false, or whether it was never true to begin with.

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

> On Thu, Jul 23, 2015 at 8:30 AM, Egor Rogov <e.rogov@postgrespro.ru> wrote:
>> So, the question: is it a documentation bug (as it seems to me), code bug,
>> or I missed something?
> Your analysis looks right to me, but I don't know whether the code or
> the documentation should be changed.  This claim was added by Tom Lane
> in 2005 in commit 58d214e51fe50b10b4439da6ec263d54c155afbf.  It might
> be worth checking whether the claim was true at that time and later
> became false, or whether it was never true to begin with.
>
As far as I can see, modern revoke syntax for revoking membership in a
role (along with "admin option") was introduced in commit 7762619 (by
Tom Lane, 2005). Code for handling this command didn't pay attention for
"restrict/cascade" keywords then, as it does not now.
Before that, another syntax was in use: alter group groupname drop user
username [, ...]. It did not include notion of "cascade" at all.
I guess that "revoke role_name from role_name" inherited
"[cascade|restrict]" section from general revoke command but never
actually used it. And I see no point in changing this, because role
membership is somewhat more static than privileges.
So I would propose the attached fix for documentation.

Thanks,
Egor Rogov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Egor,

* Egor Rogov (e.rogov@postgrespro.ru) wrote:
> >On Thu, Jul 23, 2015 at 8:30 AM, Egor Rogov <e.rogov@postgrespro.ru> wrote:
> >>So, the question: is it a documentation bug (as it seems to me), code bug,
> >>or I missed something?
> >Your analysis looks right to me, but I don't know whether the code or
> >the documentation should be changed.  This claim was added by Tom Lane
> >in 2005 in commit 58d214e51fe50b10b4439da6ec263d54c155afbf.  It might
> >be worth checking whether the claim was true at that time and later
> >became false, or whether it was never true to begin with.
> >
> As far as I can see, modern revoke syntax for revoking membership in
> a role (along with "admin option") was introduced in commit 7762619
> (by Tom Lane, 2005). Code for handling this command didn't pay
> attention for "restrict/cascade" keywords then, as it does not now.
> Before that, another syntax was in use: alter group groupname drop
> user username [, ...]. It did not include notion of "cascade" at
> all.
> I guess that "revoke role_name from role_name" inherited
> "[cascade|restrict]" section from general revoke command but never
> actually used it. And I see no point in changing this, because role
> membership is somewhat more static than privileges.
> So I would propose the attached fix for documentation.

Have you looked at the SQL spec at all for this..?  That's what we
really should be looking at to determine if this is a documentation
issue or a code issue.

I'll take a look in a day or two after I've caught up on other things,
if no one beats me to it.
Thanks!
    Stephen

On 27.07.2015 22:09, Stephen Frost wrote:
> * Egor Rogov (e.rogov@postgrespro.ru) wrote:
>>> On Thu, Jul 23, 2015 at 8:30 AM, Egor Rogov <e.rogov@postgrespro.ru> wrote:
>>>> So, the question: is it a documentation bug (as it seems to me), code bug,
>>>> or I missed something?
>>> Your analysis looks right to me, but I don't know whether the code or
>>> the documentation should be changed.  This claim was added by Tom Lane
>>> in 2005 in commit 58d214e51fe50b10b4439da6ec263d54c155afbf.  It might
>>> be worth checking whether the claim was true at that time and later
>>> became false, or whether it was never true to begin with.
>>>
>> As far as I can see, modern revoke syntax for revoking membership in
>> a role (along with "admin option") was introduced in commit 7762619
>> (by Tom Lane, 2005). Code for handling this command didn't pay
>> attention for "restrict/cascade" keywords then, as it does not now.
>> Before that, another syntax was in use: alter group groupname drop
>> user username [, ...]. It did not include notion of "cascade" at
>> all.
>> I guess that "revoke role_name from role_name" inherited
>> "[cascade|restrict]" section from general revoke command but never
>> actually used it. And I see no point in changing this, because role
>> membership is somewhat more static than privileges.
>> So I would propose the attached fix for documentation.
> Have you looked at the SQL spec at all for this..?  That's what we
> really should be looking at to determine if this is a documentation
> issue or a code issue.
>
> I'll take a look in a day or two after I've caught up on other things,
> if no one beats me to it.
>
Well, I looked into a draft of SQL:2003. It basically says that 
"cascade" for <revoke role statement> must behave the same way as for 
<revoke privilege statement>. That is, from standard's point of view we 
have a code issue.

Still I doubt about usefulness of this behavior. Do we really need it in 
PostgreSQL?

Thanks,
Egor Rogov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

On Tue, Jul 28, 2015 at 10:51 AM, Egor Rogov <e.rogov@postgrespro.ru> wrote:
>
> Well, I looked into a draft of SQL:2003. It basically says that "cascade"
> for <revoke role statement> must behave the same way as for <revoke
> privilege statement>. That is, from standard's point of view we have a code
> issue.
>
> Still I doubt about usefulness of this behavior. Do we really need it in
> PostgreSQL?

I think it's useful, as long as there are use-cases where instead of
granting privileges on the specific classes of database objects (i.e.
SELECT on all tables in a given schema) a role is granted instead
which 'accumulates' those privileges. This is the case in our
environment, and, while we are not using ADMIN OPTION, I can imagine
it  might be used in order to 'delegate' assignment of privileges from
the central authority to responsible sub-authorities in different
departments. Then, if you need to revoke those (i.e. because the
structure of departments had changed), it's enough to REVOKE ..FROM..
CASCADE instead of getting through each individual assignment case.

Kind regards,
--
Oleksii