Thread: Orphaned users in PG16 and above can only be managed by Superusers

Hi All,

Starting from PG16, it seems that orphaned users can only be managed
by superusers. For example, if userA creates userB, and userB creates
userC, then both userB (the parent of userC) and userA (the
grandparent of userC) would typically have the ability to
manage/administer userC. However, if userB is dropped, userA (the
grandparent of userC) loses the ability to administer userC as well.
This leads to a situation where only superusers can manage userC.

Shouldn't userA retain the permission to manage userC even if userB is
removed? Otherwise, only superusers would have the authority to
administer userC (the orphaned user in this case), which may not be
feasible for cloud environments where superuser access is restricted.

--
With Regards,
Ashutosh Sharma.

On Thu, Jan 30, 2025 at 8:45 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> Imagine a superuser creates role u1. Since the superuser is creating
> u1, it won't have membership in any role. Now, suppose u1 creates a
> new role, u2. In this case, u1 automatically becomes a member of u2
> with the admin option. However, at this point, there is no dependency
> between u1 and u2, meaning that dropping u2 shouldn't impact u1. Now,
> if u2 creates yet another role, u3, that's when u1 will start
> depending on u2. This is because if u2 were dropped, u1 would lose the
> ability to administer u3. At this stage, a dependency between u1 and
> u2 is recorded.

This seems to me to be assuming that who can administer which roles
won't change, but it can. The superuser is free to grant the ability
to administer u3 to some new user u4 or an existing user such as u1,
and is also free to remove that ability from u2.

I think if you want to legislate that attempting to drop u2 fails, you
have to do that by testing what the situation is at the time of the
drop command, not by adding dependencies in advance.

--
Robert Haas
EDB: http://www.enterprisedb.com

Hi Robert,

On Tue, Feb 4, 2025 at 10:54 PM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Thu, Jan 30, 2025 at 8:45 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> > Imagine a superuser creates role u1. Since the superuser is creating
> > u1, it won't have membership in any role. Now, suppose u1 creates a
> > new role, u2. In this case, u1 automatically becomes a member of u2
> > with the admin option. However, at this point, there is no dependency
> > between u1 and u2, meaning that dropping u2 shouldn't impact u1. Now,
> > if u2 creates yet another role, u3, that's when u1 will start
> > depending on u2. This is because if u2 were dropped, u1 would lose the
> > ability to administer u3. At this stage, a dependency between u1 and
> > u2 is recorded.
>
> This seems to me to be assuming that who can administer which roles
> won't change, but it can. The superuser is free to grant the ability
> to administer u3 to some new user u4 or an existing user such as u1,
> and is also free to remove that ability from u2.
>

You're right, I'll fix that in the next patch.

> I think if you want to legislate that attempting to drop u2 fails, you
> have to do that by testing what the situation is at the time of the
> drop command, not by adding dependencies in advance.

I agree, and I will give this some thought to see if we can find a way
forward along these lines.

--
Thanks & Regards,
Ashutosh Sharma.

Added a commitfest entry for this here:

https://commitfest.postgresql.org/patch/5608/

--
With Regards,
Ashutosh Sharma.

On Tue, Feb 18, 2025 at 2:54 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> Hi Robert,
>
> On Tue, Feb 11, 2025 at 9:48 PM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> >
> > Hi Robert,
> >
> > On Tue, Feb 4, 2025 at 10:54 PM Robert Haas <robertmhaas@gmail.com> wrote:
> > >
> > > On Thu, Jan 30, 2025 at 8:45 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> > > > Imagine a superuser creates role u1. Since the superuser is creating
> > > > u1, it won't have membership in any role. Now, suppose u1 creates a
> > > > new role, u2. In this case, u1 automatically becomes a member of u2
> > > > with the admin option. However, at this point, there is no dependency
> > > > between u1 and u2, meaning that dropping u2 shouldn't impact u1. Now,
> > > > if u2 creates yet another role, u3, that's when u1 will start
> > > > depending on u2. This is because if u2 were dropped, u1 would lose the
> > > > ability to administer u3. At this stage, a dependency between u1 and
> > > > u2 is recorded.
> > >
> > > This seems to me to be assuming that who can administer which roles
> > > won't change, but it can. The superuser is free to grant the ability
> > > to administer u3 to some new user u4 or an existing user such as u1,
> > > and is also free to remove that ability from u2.
> > >
> >
> > You're right, I'll fix that in the next patch.
> >
> > > I think if you want to legislate that attempting to drop u2 fails, you
> > > have to do that by testing what the situation is at the time of the
> > > drop command, not by adding dependencies in advance.
> >
> > I agree, and I will give this some thought to see if we can find a way
> > forward along these lines.
> >
>
> Attached is a patch that checks for role dependencies when the DROP
> ROLE command is executed. If dependencies are found, the command is
> prevented from succeeding. Please review the attached patch and share
> your feedback. thanks.!
>
> --
> With Regards,
> Ashutosh Sharma.

On Tue, Feb 18, 2025 at 02:54:46PM +0530, Ashutosh Sharma wrote:
> Attached is a patch that checks for role dependencies when the DROP
> ROLE command is executed. If dependencies are found, the command is
> prevented from succeeding. Please review the attached patch and share
> your feedback. thanks.!

Thanks for the patch.  I have two questions:

* The patch alleges to only block DROP ROLE commands when there exists
  _both_ admins of the target role and roles for which the target role is
  an admin.  However, it's not clear to me why both need to be true.  I
  might be able to glean the reason if I read this thread carefully or
  spend more time thinking about it, but IMHO that patch itself should make
  it obvious.  I'd suggest expanding the comment atop
  check_drop_role_dependency().

* Does this introduce any race conditions?  For example, is it possible for
  the new check to pass and then for a dependency to be added before the
  drop completes?

-- 
nathan

On Wed, Mar 5, 2025 at 3:13 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> * The patch alleges to only block DROP ROLE commands when there exists
>   _both_ admins of the target role and roles for which the target role is
>   an admin.  However, it's not clear to me why both need to be true.  I
>   might be able to glean the reason if I read this thread carefully or
>   spend more time thinking about it, but IMHO that patch itself should make
>   it obvious.  I'd suggest expanding the comment atop
>   check_drop_role_dependency().

The error message needs work, too. Nobody is ever going to guess what
the rule is from that error message.

> * Does this introduce any race conditions?  For example, is it possible for
>   the new check to pass and then for a dependency to be added before the
>   drop completes?

This is a serious concern for me as well.

--
Robert Haas
EDB: http://www.enterprisedb.com

Thanks, Nathan, for reviewing the patch. Below are my comments inline:

On Thu, Mar 6, 2025 at 1:43 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
>
> * The patch alleges to only block DROP ROLE commands when there exists
>   _both_ admins of the target role and roles for which the target role is
>   an admin.  However, it's not clear to me why both need to be true.  I
>   might be able to glean the reason if I read this thread carefully or
>   spend more time thinking about it, but IMHO that patch itself should make
>   it obvious.  I'd suggest expanding the comment atop
>   check_drop_role_dependency().
>

I'll update the comments above the check_drop_role_dependency()
function to clarify things.

> * Does this introduce any race conditions?  For example, is it possible for
>   the new check to pass and then for a dependency to be added before the
>   drop completes?
>

I believe it is; I may need to adjust the location from where I'm
calling check_drop_role_dependency() to take care of this. I'll
address this in the next patch version. Thanks for bringing up this
concern.

--
With Regards,
Ashutosh Sharma.

Hi Robert,

Thanks for the review comments.

On Thu, Mar 6, 2025 at 2:10 AM Robert Haas <robertmhaas@gmail.com> wrote:
>
> On Wed, Mar 5, 2025 at 3:13 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> > * The patch alleges to only block DROP ROLE commands when there exists
> >   _both_ admins of the target role and roles for which the target role is
> >   an admin.  However, it's not clear to me why both need to be true.  I
> >   might be able to glean the reason if I read this thread carefully or
> >   spend more time thinking about it, but IMHO that patch itself should make
> >   it obvious.  I'd suggest expanding the comment atop
> >   check_drop_role_dependency().
>
> The error message needs work, too. Nobody is ever going to guess what
> the rule is from that error message.
>

I'll handle this in the next patch version.

> > * Does this introduce any race conditions?  For example, is it possible for
> >   the new check to pass and then for a dependency to be added before the
> >   drop completes?
>
> This is a serious concern for me as well.
>

This too will be taken care of in the next patch.

--
With Regards,
Ashutosh Sharma.

Hi,

Attached is the v2 patch with the following updates:

1) Added detailed comments atop check_drop_role_dependency() to
clarify role dependencies, addressing Nathan's comment.

2) Fixed a race condition where the dependency check could pass, but a
new dependency might be added before the role drop is completed,
addressing comments from Nathan and Robert.

3) Improved the error message to display the role dependencies in
detail, addressing feedback from Robert.

Please have a look and let me know for any further comments. Thanks.

--
With Regards,
Ashutosh Sharma.

On Thu, Mar 06, 2025 at 04:10:10PM +0530, Ashutosh Sharma wrote:
> Attached is the v2 patch with the following updates:
> 
> 1) Added detailed comments atop check_drop_role_dependency() to
> clarify role dependencies, addressing Nathan's comment.

Thanks!

> 2) Fixed a race condition where the dependency check could pass, but a
> new dependency might be added before the role drop is completed,
> addressing comments from Nathan and Robert.
> 
> 3) Improved the error message to display the role dependencies in
> detail, addressing feedback from Robert.
> 
> Please have a look and let me know for any further comments. Thanks.

I noticed that much of this code is lifted from DropRole(), and the new
check_drop_role_dependency() function is only used by DropRole() right
before it does the exact same scans.  Couldn't we put the new dependency
detection in those existing scans in DropRole()?

-- 
nathan

Hi,

On Fri, Mar 7, 2025 at 10:55 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> I noticed that much of this code is lifted from DropRole(), and the new
> check_drop_role_dependency() function is only used by DropRole() right
> before it does the exact same scans.  Couldn't we put the new dependency
> detection in those existing scans in DropRole()?
>

It can be done, but mixing the code that checks for the drop role
dependency with the code that removes entries for the role being
dropped from pg_auth_members could reduce clarity and precision. This
is more of a sanity check which I felt was necessary before we proceed
with actually dropping the role, starting with the deletion of drop
role entries from the system catalogs. I’m aware there’s some code
duplication, but I think it should be fine.

--
With Regards,
Ashutosh Sharma.

On Mon, Mar 10, 2025 at 11:15:04AM +0530, Ashutosh Sharma wrote:
> On Fri, Mar 7, 2025 at 10:55 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> I noticed that much of this code is lifted from DropRole(), and the new
>> check_drop_role_dependency() function is only used by DropRole() right
>> before it does the exact same scans.  Couldn't we put the new dependency
>> detection in those existing scans in DropRole()?
> 
> It can be done, but mixing the code that checks for the drop role
> dependency with the code that removes entries for the role being
> dropped from pg_auth_members could reduce clarity and precision. This
> is more of a sanity check which I felt was necessary before we proceed
> with actually dropping the role, starting with the deletion of drop
> role entries from the system catalogs. I’m aware there’s some code
> duplication, but I think it should be fine.

Looking closer, we probably need to move this check to the second pass,
anyway:

    postgres=# CREATE ROLE a CREATEROLE;
    CREATE ROLE
    postgres=# SET ROLE a;
    SET
    postgres=> CREATE ROLE b CREATEROLE;
    CREATE ROLE
    postgres=> SET ROLE b;
    SET
    postgres=> CREATE ROLE c;
    CREATE ROLE
    postgres=> RESET ROLE;
    RESET
    postgres=# DROP ROLE b, c;
    ERROR:  role "b" cannot be dropped because some objects depend on it
    DETAIL:  role a inherits ADMIN privileges on role c through role b
    postgres=# DROP ROLE c, b;
    DROP ROLE

The first DROP ROLE should probably succeed, if for no other reason than
individually dropping role c followed by role b would succeed.

-- 
nathan

Hi Nathan,

Thanks for the review comment.

On Mon, Mar 10, 2025 at 8:31 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Mon, Mar 10, 2025 at 11:15:04AM +0530, Ashutosh Sharma wrote:
> > On Fri, Mar 7, 2025 at 10:55 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> >> I noticed that much of this code is lifted from DropRole(), and the new
> >> check_drop_role_dependency() function is only used by DropRole() right
> >> before it does the exact same scans.  Couldn't we put the new dependency
> >> detection in those existing scans in DropRole()?
> >
> > It can be done, but mixing the code that checks for the drop role
> > dependency with the code that removes entries for the role being
> > dropped from pg_auth_members could reduce clarity and precision. This
> > is more of a sanity check which I felt was necessary before we proceed
> > with actually dropping the role, starting with the deletion of drop
> > role entries from the system catalogs. I’m aware there’s some code
> > duplication, but I think it should be fine.
>
> Looking closer, we probably need to move this check to the second pass,
> anyway:
>
>         postgres=# CREATE ROLE a CREATEROLE;
>         CREATE ROLE
>         postgres=# SET ROLE a;
>         SET
>         postgres=> CREATE ROLE b CREATEROLE;
>         CREATE ROLE
>         postgres=> SET ROLE b;
>         SET
>         postgres=> CREATE ROLE c;
>         CREATE ROLE
>         postgres=> RESET ROLE;
>         RESET
>         postgres=# DROP ROLE b, c;
>         ERROR:  role "b" cannot be dropped because some objects depend on it
>         DETAIL:  role a inherits ADMIN privileges on role c through role b
>         postgres=# DROP ROLE c, b;
>         DROP ROLE
>
> The first DROP ROLE should probably succeed, if for no other reason than
> individually dropping role c followed by role b would succeed.
>

I initially thought this was fine because the sequence of dropping
roles matters. In the previous case (the first drop role statement),
the referenced role is dropped first, followed by the dependent role,
which causes the statement to fail. However, in the second statement,
the order is reversed, with the dependent role being dropped first,
allowing the referenced role to be dropped afterward, which makes the
statement succeed.

That said, I've realized this doesn't apply to the grantor role. If
both the grantor and grantee roles are dropped in a single statement,
the drop command succeeds regardless of the order. So, we need to
ensure that the same behavior is maintained here as well. I’ll address
this in the next patch version.

--
With Regards,
Ashutosh Sharma.

Hi Nathan,

On Mon, Mar 10, 2025 at 8:31 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> On Mon, Mar 10, 2025 at 11:15:04AM +0530, Ashutosh Sharma wrote:
> > On Fri, Mar 7, 2025 at 10:55 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> >> I noticed that much of this code is lifted from DropRole(), and the new
> >> check_drop_role_dependency() function is only used by DropRole() right
> >> before it does the exact same scans.  Couldn't we put the new dependency
> >> detection in those existing scans in DropRole()?
> >
> > It can be done, but mixing the code that checks for the drop role
> > dependency with the code that removes entries for the role being
> > dropped from pg_auth_members could reduce clarity and precision. This
> > is more of a sanity check which I felt was necessary before we proceed
> > with actually dropping the role, starting with the deletion of drop
> > role entries from the system catalogs. I’m aware there’s some code
> > duplication, but I think it should be fine.
>
> Looking closer, we probably need to move this check to the second pass,
> anyway:
>

I think moving the check to the second pass won’t work in this case.
The reason is that we rely on entries in the pg_auth_members table. By
the time the check occurs in the second pass, the first pass will have
already removed all entries related to the roles being dropped from
pg_auth_members and incremented the command counter. As a result, when
check_drop_role_dependency() is called in the second pass, it won’t
find any entries in the table and won't be able to detect any ADMIN
privilege-related dependencies. Let me illustrate this with an example
(similar to yours, but with b creating an additional role d):

CREATE ROLE a CREATEROLE;
SET ROLE a;
CREATE ROLE b CREATEROLE;
SET ROLE b;
CREATE ROLE c;
CREATE ROLE d;
RESET ROLE;

At this point, the pg_auth_members table will contain the following entries:

ashu@postgres=# SELECT oid, roleid::regrole, member::regrole,
grantor::regrole, admin_option, xmin, xmax FROM pg_auth_members WHERE
roleid::regrole::text NOT LIKE 'pg_%';
  oid  | roleid | member | grantor | admin_option | xmin | xmax
-------+--------+--------+---------+--------------+------+------
 16394 | b      | a      | ashu    | t            |  756 |    0
 16396 | c      | b      | ashu    | t            |  757 |    0
 16398 | d      | b      | ashu    | t            |  758 |    0
(3 rows)

Now, when we run DROP ROLE b, c, the first pass in DropRole() will
remove all the entries from pg_auth_members for these roles, as
expected. So when check_drop_role_dependency() is called in the second
pass, it won’t find any entries in the table, and thus won’t identify
the dependency of role 'a' on role 'b' for role 'd'. As a result, the
drop would succeed, even though it should fail due to the dependency.

So, we need to explore alternative approaches to handle this better.
I’ll spend some more time today to investigate other possibilities for
addressing this issue.

--
With Regards,
Ashutosh Sharma.

On Wed, Mar 12, 2025 at 12:10:30PM +0530, Ashutosh Sharma wrote:
> I think moving the check to the second pass won´t work in this case.
> The reason is that we rely on entries in the pg_auth_members table. By
> the time the check occurs in the second pass, the first pass will have
> already removed all entries related to the roles being dropped from
> pg_auth_members and incremented the command counter. As a result, when
> check_drop_role_dependency() is called in the second pass, it won´t
> find any entries in the table and won't be able to detect any ADMIN
> privilege-related dependencies.

Oh, good catch.

> So, we need to explore alternative approaches to handle this better.
> I´ll spend some more time today to investigate other possibilities for
> addressing this issue.

I think this approach has other problems.  For example, even if a role has
admin directly on the dropped role, we'll block DROP ROLE if it also has
admin indirectly:

    postgres=# create role a createrole;
    CREATE ROLE
    postgres=# set role a;
    SET
    postgres=> create role b createrole;
    CREATE ROLE
    postgres=> set role b;
    SET
    postgres=> create role c admin a;
    CREATE ROLE
    postgres=> reset role;
    RESET
    postgres=# drop role b;
    ERROR:  role "b" cannot be dropped because some objects depend on it
    DETAIL:  role a inherits ADMIN privileges on role c through role b

There are also other ways besides DROP ROLE that roles can end up without
any admins:

    postgres=# create role a createrole;
    CREATE ROLE
    postgres=# set role a;
    SET
    postgres=> create role b createrole;
    CREATE ROLE
    postgres=> set role b;
    SET
    postgres=> create role c;
    CREATE ROLE
    postgres=> reset role;
    RESET
    postgres=# revoke c from b;
    REVOKE ROLE

I wonder if we should adjust the requirements here to be "an operation
cannot result in a role with 0 admins."  That does mean you might lose
indirect admin privileges, but I'm not sure that's sustainable, anyway.
For example, if a role has 2 admins, should we block REVOKE from one of the
admins because another role inherited its privileges?  If nothing else, it
sounds like a complicated user experience.

If we just want to make sure that roles don't end up without admins, I
think we could just collect the set of roles losing an admin during DROP
ROLE, REVOKE, etc., and then once all removals have completed, we verify
that each of the collected roles has an admin.

-- 
nathan

Hi,

On Wed, Mar 12, 2025 at 9:06 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> I think this approach has other problems.  For example, even if a role has
> admin directly on the dropped role, we'll block DROP ROLE if it also has
> admin indirectly:
>

This is exactly what we're aiming for. We don't want the ADMIN
privilege, inherited by role 'a' on role 'c' via role 'b', to be
removed by the drop command. Therefore, we expect the command to fail.

> There are also other ways besides DROP ROLE that roles can end up without
> any admins:
>
>         postgres=# create role a createrole;
>         CREATE ROLE
>         postgres=# set role a;
>         SET
>         postgres=> create role b createrole;
>         CREATE ROLE
>         postgres=> set role b;
>         SET
>         postgres=> create role c;
>         CREATE ROLE
>         postgres=> reset role;
>         RESET
>         postgres=# revoke c from b;
>         REVOKE ROLE
>
> I wonder if we should adjust the requirements here to be "an operation
> cannot result in a role with 0 admins."  That does mean you might lose
> indirect admin privileges, but I'm not sure that's sustainable, anyway.
> For example, if a role has 2 admins, should we block REVOKE from one of the
> admins because another role inherited its privileges?  If nothing else, it
> sounds like a complicated user experience.
>

I think this is acceptable because role 'b', from which role 'a'
inherited the ADMIN privilege on role 'c', no longer has the privilege
either. Therefore, it's understandable for role 'a' to lose the
indirect privilege, as the role it was inheriting from has also lost
it. Moreover, this action is being performed by a superuser, the same
superuser who granted the ADMIN privilege on role 'c' to role 'b', and
it should have the ability to revoke it at any time. Once role 'b'
loses the ADMIN privilege on role 'c', it's expected that role 'a'
will also lose the privilege, as it was inherited from role 'b'. After
the ADMIN privilege is revoked, role 'a' will no longer have a
dependency on role 'b', so dropping role 'b' should succeed.

--
With Regards,
Ashutosh Sharma.

Hi,

On Thu, Mar 13, 2025 at 11:14 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
>
> Hi,
>
> On Wed, Mar 12, 2025 at 9:06 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> >
> > There are also other ways besides DROP ROLE that roles can end up without
> > any admins:
> >
> >         postgres=# create role a createrole;
> >         CREATE ROLE
> >         postgres=# set role a;
> >         SET
> >         postgres=> create role b createrole;
> >         CREATE ROLE
> >         postgres=> set role b;
> >         SET
> >         postgres=> create role c;
> >         CREATE ROLE
> >         postgres=> reset role;
> >         RESET
> >         postgres=# revoke c from b;
> >         REVOKE ROLE
> >
> > I wonder if we should adjust the requirements here to be "an operation
> > cannot result in a role with 0 admins."  That does mean you might lose
> > indirect admin privileges, but I'm not sure that's sustainable, anyway.
> > For example, if a role has 2 admins, should we block REVOKE from one of the
> > admins because another role inherited its privileges?  If nothing else, it
> > sounds like a complicated user experience.
> >
>
> I think this is acceptable because role 'b', from which role 'a'
> inherited the ADMIN privilege on role 'c', no longer has the privilege
> either. Therefore, it's understandable for role 'a' to lose the
> indirect privilege, as the role it was inheriting from has also lost
> it. Moreover, this action is being performed by a superuser, the same
> superuser who granted the ADMIN privilege on role 'c' to role 'b', and
> it should have the ability to revoke it at any time. Once role 'b'
> loses the ADMIN privilege on role 'c', it's expected that role 'a'
> will also lose the privilege, as it was inherited from role 'b'. After
> the ADMIN privilege is revoked, role 'a' will no longer have a
> dependency on role 'b', so dropping role 'b' should succeed.
>

Here’s an example demonstrating the existing behavior of the DROP ROLE
command when attempting to drop a grantor role, which closely
resembles the behavior of the DROP ROLE command trying to drop a role
through which admin privileges are inherited by other role(s).

ashu@postgres=# create user a createrole login;
CREATE ROLE
ashu@postgres=# set role a;
SET
ashu@postgres=> create user b createrole login;
CREATE ROLE
ashu@postgres=> create user c createrole login;
CREATE ROLE
ashu@postgres=> grant c to b with admin true;
GRANT ROLE
ashu@postgres=> reset role;
RESET
ashu@postgres=# drop role a;
ERROR:  2BP01: role "a" cannot be dropped because some objects depend on it
DETAIL:  privileges for membership of role b in role c
LOCATION:  DropRole, user.c:1303
ashu@postgres=# set role a;
SET
ashu@postgres=> revoke c from b;
REVOKE ROLE
ashu@postgres=# drop role a;
DROP ROLE

In the example above, the DROP ROLE command for role 'a' failed
because 'a' had granted role 'c' to role 'b'. However, once role 'c'
was revoked from role 'b', the DROP ROLE command succeeded.

--
With Regards,
Ashutosh Sharma.

On Thu, Mar 13, 2025 at 1:44 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> On Wed, Mar 12, 2025 at 9:06 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> > I think this approach has other problems.  For example, even if a role has
> > admin directly on the dropped role, we'll block DROP ROLE if it also has
> > admin indirectly:
> >
>
> This is exactly what we're aiming for. We don't want the ADMIN
> privilege, inherited by role 'a' on role 'c' via role 'b', to be
> removed by the drop command. Therefore, we expect the command to fail.

I'm not sure what the right thing to do is here. This argument does
make some sense to me...

> >         postgres=# revoke c from b;
> >         REVOKE ROLE
>
> I think this is acceptable because role 'b', from which role 'a'
> inherited the ADMIN privilege on role 'c', no longer has the privilege
> either. Therefore, it's understandable for role 'a' to lose the
> indirect privilege, as the role it was inheriting from has also lost
> it. Moreover, this action is being performed by a superuser, the same
> superuser who granted the ADMIN privilege on role 'c' to role 'b', and
> it should have the ability to revoke it at any time. Once role 'b'
> loses the ADMIN privilege on role 'c', it's expected that role 'a'
> will also lose the privilege, as it was inherited from role 'b'. After
> the ADMIN privilege is revoked, role 'a' will no longer have a
> dependency on role 'b', so dropping role 'b' should succeed.

...as does this one. But if I accept both of those arguments, then
it's correct for the superuser to be blocked trying to "DROP ROLE b"
and also correct for the superuser to succeed trying to "REVOKE c FROM
b," and I'm not sure that really makes sense. I can understand why we
don't want a non-superuser to create a situation where the only
remaining administrator for a certain role is the superuser. But the
superuser themselves is not forbidden from creating such situations,
so why does it matter whether they try to create that situation via
DROP ROLE or via REVOKE?

I in general dislike throwing up barriers that prevent objects from
being dropped. As a user, I find such rules frustrating, especially if
I'm still allowed to accomplish the same drop indirectly by some
series of commands (e.g. REVOKE first, then DROP). If I'm allowed to
do it indirectly, then I should also be allowed to it directly, at
least in cases where there's only one way of fixing the problem that
is preventing me from doing the DROP, which I think is the case here.
For example, if bob owns a tractor and I want to DROP bob but the
tractor is indestructible, then it's reasonable to make the operation
fail. I need to give away bob's tractor before I drop him. But here,
if I say I want to DROP ROLE b, I'm going to have to first REVOKE c
FROM b -- there is no real other alternative. So why not make that
happen automatically? When I say I want to DROP something, I'm
serious: I really want it gone.

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> if I say I want to DROP ROLE b, I'm going to have to first REVOKE c
> FROM b -- there is no real other alternative. So why not make that
> happen automatically? When I say I want to DROP something, I'm
> serious: I really want it gone.

For privileges on ordinary objects, we make you REVOKE them all first
before you can drop the grantee role, but that stems from an
implementation limitation: some of the privileges may be recorded in
the catalogs of other databases that we can't reach.  (We can see in
the shared pg_shdepend catalog that some privilege exists in another
DB, but we can't do anything about it.)

For grants on roles, all the moving parts are in the shared
pg_auth_members catalog, so from the implementation standpoint there's
nothing stopping us from auto-dropping such grants.  And AFAICS we do.
Certainly this is a bit inconsistent with the behavior for other
kinds of grants, but it's stood that way for awhile.  If we were to
try to make this consistent, I think we'd look for a way to auto-drop
privileges on ordinary objects, not start failing DROP ROLE because
privileges on roles exist.

That being the case, I'm against imposing restrictions on DROP ROLE
because of the properties of particular role grants.  If you get
into a situation where you need a superuser's help to undo something,
well hopefully you learned better and won't do that again.

I'm especially against making life more difficult for everyone who
uses Postgres in order to remove a problem that's only a problem for
people who don't have a superuser account available.

Furthermore, there is a standards-compliance argument.  The SQL
standard is unambiguous that auto-dropping privileges is what's
supposed to happen:

         <drop role statement> ::= DROP ROLE <role name>

         Syntax Rules

         1) Let R be the role identified by the specified <role name>.

         Access Rules

         1) At least one of the enabled authorization identifiers shall have
            a role authorization identifier that authorizes R with the WITH
            ADMIN OPTION.

         General Rules

         1) Let A be any <authorization identifier> identified by a role
            authorization descriptor as having been granted to R.

         2) The following <revoke role statement> is effectively executed
            without further Access Rule checking:

              REVOKE R FROM A

         3) The descriptor of R is destroyed.

There is nothing in rule (2) that suggests that implementations are
allowed to fail the DROP if they don't agree with what privileges are
being dropped.  (So our behavior for role privileges is spec-compliant
and our behavior for other privileges is not.)

In short, I don't like anything about this proposed patch and
think we should reject it.

            regards, tom lane

On Wed, Mar 19, 2025 at 1:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> That being the case, I'm against imposing restrictions on DROP ROLE
> because of the properties of particular role grants.  If you get
> into a situation where you need a superuser's help to undo something,
> well hopefully you learned better and won't do that again.
>
> I'm especially against making life more difficult for everyone who
> uses Postgres in order to remove a problem that's only a problem for
> people who don't have a superuser account available.

You kind of lost me at this point. I mean, technically I agree that we
don't want to make life worse for everyone to help people who don't
have a superuser account available, but I don't see why it's written
in stone that we should have to make life worse for
superuser-administered installs in order to make it better for
non-superuser-administered installs.

Also, non-superuser-administered installs probably outnumber
superuser-administered ones already, maybe by a large margin, and I
think that's only going to accelerate as more things are done via
cloud providers. It's not some niche use case.

I am interested by your comment about the automatic DROP ROLE being
required by the spec, though. I rarely understand the spec, but I like
it when somebody says it agrees with what I already thought. :-)

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Mar 19, 2025 at 1:55 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I'm especially against making life more difficult for everyone who
>> uses Postgres in order to remove a problem that's only a problem for
>> people who don't have a superuser account available.

> You kind of lost me at this point. I mean, technically I agree that we
> don't want to make life worse for everyone to help people who don't
> have a superuser account available, but I don't see why it's written
> in stone that we should have to make life worse for
> superuser-administered installs in order to make it better for
> non-superuser-administered installs.

I didn't assert that that's a general problem.  I meant that this
particular patch makes life worse, by causing DROP ROLE to fail
unexpectedly.

BTW, I should note that I was quoting SQL99, because I have that
handy as plain text which is a lot easier to copy-n-paste from
than the PDF form of the later specs.  I looked into SQL:2021
and noted that DROP ROLE has grown a RESTRICT/CASCADE option,
which we have not implemented.  The RESTRICT form allows (if
I'm reading it right) failure if any indirect privilege grants
would have to be removed.  I haven't thought about it hard
enough to be sure if the situation Ashutosh is concerned about
would amount to removal of an indirect grant.  In any case,
CASCADE would still require such removal, and flat-out refusing
it would still be a spec violation.

Perhaps if we implemented RESTRICT/CASCADE here, that would
at least make it harder to fall into this trap?  In the spec,
that's simply passed on to the implied REVOKE commands, and
we do already support REVOKE ... RESTRICT/CASCADE, so maybe
that's not a hugely difficult patch.

            regards, tom lane

On Wed, Mar 19, 2025 at 2:32 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I didn't assert that that's a general problem.  I meant that this
> particular patch makes life worse, by causing DROP ROLE to fail
> unexpectedly.

I think that's only true of this particular version of the patch. I
believe it's likely resolvable somehow. As I see it, we're still
debating what the exact design should be.

> Perhaps if we implemented RESTRICT/CASCADE here, that would
> at least make it harder to fall into this trap?  In the spec,
> that's simply passed on to the implied REVOKE commands, and
> we do already support REVOKE ... RESTRICT/CASCADE, so maybe
> that's not a hugely difficult patch.

I have always assumed that the reason DROP ROLE blah CASCADE is not
implemented is (1) it would have to cascade to objects in other
databases which we can't do from an implementation perspective and (2)
cascading from roles to tables would create a terrifying amount of
room for user error. One could dismiss (2) if one were brave enough,
but (1) seems like an irreducible problem. No?

--
Robert Haas
EDB: http://www.enterprisedb.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, Mar 19, 2025 at 2:32 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Perhaps if we implemented RESTRICT/CASCADE here, that would
>> at least make it harder to fall into this trap?

> I have always assumed that the reason DROP ROLE blah CASCADE is not
> implemented is (1) it would have to cascade to objects in other
> databases which we can't do from an implementation perspective and (2)
> cascading from roles to tables would create a terrifying amount of
> room for user error. One could dismiss (2) if one were brave enough,
> but (1) seems like an irreducible problem. No?

Yeah, I don't care for having it cascade to physical objects either.
But our current behavior is "RESTRICT if there are owned objects or
permissions on objects, but auto-CASCADE to role grants".  There's
no implementation reason why we couldn't make RESTRICT/CASCADE work
for role grants, and that'd be at least a smidge closer to what the
spec says.  It's not clear to me though if that could help for the
concern at hand.

            regards, tom lane

Thank you, Robert and Tom, for sharing your valuable insights, and
apologies for the slight delay in my response. From the discussion,
what I understand is that we aim to extend the current DROP ROLE
syntax to include the CASCADE/RESTRICT option, which has been
introduced in the latest SQL standard, as mentioned by Tom. However,
as Robert pointed out, implementing the CASCADE option for handling
dependent objects that span multiple databases is not feasible at the
moment. The RESTRICT option, as I understand it, is already the
default behavior. Therefore, we will proceed with implementing the
CASCADE/RESTRICT syntax specifically for handling dependent roles,
rather than the dependent database objects like tables, views, etc.,
which can span multiple databases.

Please correct me if I’m mistaken or if there’s anything I’ve missed
in my understanding. thanks.

--
With Regards,
Ashutosh Sharma.

On Mon, Mar 24, 2025 at 2:18 AM Ashutosh Sharma <ashu.coek88@gmail.com> wrote:
> Thank you, Robert and Tom, for sharing your valuable insights, and
> apologies for the slight delay in my response. From the discussion,
> what I understand is that we aim to extend the current DROP ROLE
> syntax to include the CASCADE/RESTRICT option, which has been
> introduced in the latest SQL standard, as mentioned by Tom. However,
> as Robert pointed out, implementing the CASCADE option for handling
> dependent objects that span multiple databases is not feasible at the
> moment. The RESTRICT option, as I understand it, is already the
> default behavior. Therefore, we will proceed with implementing the
> CASCADE/RESTRICT syntax specifically for handling dependent roles,
> rather than the dependent database objects like tables, views, etc.,
> which can span multiple databases.

I would be fairly strongly opposed to implementing RESTRICT and
CASCADE but having them only apply to role grants and not other
database objects.

I'm not entirely certain what the right thing to do is here, but I
don't think it's that.

--
Robert Haas
EDB: http://www.enterprisedb.com