Thread: almost-super-user problems that we haven't fixed yet

Due to cf5eb37c5ee0cc54c80d95c1695d7fca1f7c68cb,
e5b8a4c098ad6add39626a14475148872cd687e0, and prior commits touching
related code, it should now be possible to consider handing out
CREATEROLE as a reasonable alternative to handing out SUPERUSER. Prior
to cf5eb37c5ee0cc54c80d95c1695d7fca1f7c68cb, giving CREATEROLE meant
giving away control of pg_execute_server_programs and every other
built-in role, so it wasn't possible to give CREATEROLE to a user who
isn't completely trusted. Now, that should be OK. CREATEROLE users
will only gain control over roles they create (and any others that the
superuser grants to them). Furthermore, if you set
createrole_self_grant to 'inherit' or 'set, inherit', a CREATEROLE
user will automatically inherit the privileges of the users they
create, hopefully making them feel like they are almost a superuser
without letting them actually take over the world.

Not very surprisingly, those commits failed to solve every single
problem that anyone has ever thought about in this area.

Here is a probably-incomplete list of related problems that are so far unsolved:

1. It's still possible for a CREATEROLE user to hand out role
attributes that they don't possess. The new prohibitions in
cf5eb37c5ee0cc54c80d95c1695d7fca1f7c68cb prevent a CREATEROLE user
from handing out membership in a role on which they lack sufficient
permissions, but they don't prevent a CREATEROLE user who lacks
CREATEDB from creating a new user who does have CREATEDB. I think we
should subject the CREATEDB, REPLICATION, and BYPASSRLS attributes to
the same rule that we now use for role memberships: you've got to have
the property in order to give it to someone else. In the case of
CREATEDB, this would tighten the current rules, which allow you to
give out CREATEDB without having it. In the case of REPLICATION and
BYPASSRLS, this would liberalize the current rules: right now, a
CREATEROLE user cannot give REPLICATION or BYPASSRLS to another user
even if they possess those attributes.

This proposal doesn't address the CREATEROLE or CONNECTION LIMIT
properties. It seems possible to me that someone might want to set up
a CREATEROLE user who can't make more such users, and this proposal
doesn't manufacture any way of doing that. It also doesn't let you
constraint the ability of a CREATEROLE user to set a CONNECTION LIMIT
for some other user. I think that's OK. It might be nice to have ways
of imposing such restrictions at some point in the future, but it is
not very obvious what to do about such cases and, importantly, I don't
think there's any security impact from failing to address those cases.
If a CREATEROLE user without CREATEDB can create a new role that does
have CREATEDB, that's a privilege escalation. If they can hand out
CREATEROLE, that isn't: they already have it.

2. It's still impossible for a CREATEROLE user to execute CREATE
SUBSCRIPTION, so they can't get logical replication working. There was
a previous thread about fixing this at
https://www.postgresql.org/message-id/flat/9DFC88D3-1300-4DE8-ACBC-4CEF84399A53%40enterprisedb.com
and the corresponding CF entry is listed as committed, but
CreateSubscription() still requires superuser, so I think that maybe
that thread only got some of the preliminary permissions-check work
committed and the core problem is yet to be solved.

3. Only superusers can control event triggers. In the thread at
https://www.postgresql.org/message-id/flat/914FF898-5AC4-4E02-8A05-3876087007FB%40enterprisedb.com
it was proposed, based on an idea from Tom, to allow any user to
create event triggers but, approximately, to only have them fire for
code running as a user whose privileges the creator already has. I
don't recall the precise rule that was proposed and it might need
rethinking in view of 3d14e171e9e2236139e8976f3309a588bcc8683b, and I
think there was also some opposition to that proposal, so I'm not sure
what the way forward here is.

4. You can reserve a small number of connections for the superuser
with superuser_reserved_connections, but there's no way to do a
similar thing for any other user. As mentioned above, a CREATEROLE
user could set connection limits for every created role such that the
sum of those limits is less than max_connections by some margin, but
that restricts each of those roles individually, not all of them in
the aggregate. Maybe we could address this by inventing a new GUC
reserved_connections and a predefined role
pg_use_reserved_connections.

5. If you set createrole_self_grant = 'set, inherit' and make alice a
CREATEROLE user and she goes around and creates a bunch of other users
and they all run around and create a bunch of objects and then alice
tries to pg_dump the entire database, it will work ... provided that
there are no tables owned by any other user. If the superuser has
created any tables, or there's another CREATEROLE user wandering
around creating tables, or even a non-CREATEROLE user whose
permissions alice does not have, pg_dump will try to lock them and
die. I don't see any perfect solution to this problem: we can neither
let alice dump objects on which she does not have permission, nor can
we silently skip them in the interest of giving alice a better user
experience, because if we do that then somebody will end up with a
partial database backup that they think is a complete database backup
and that will be a really bad day. However, I think we could add a
pg_dump option that says, hey, please only try to dump tables we have
permission to dump, and skip the others. Or, of course, alice could
use -T and -N as required, but a dedicated switch for
skip-stuff-i-can't-access-quietly might be a better user experience. I
guess you could also argue that this isn't really a problem in the
first place because you could always choose to grant
pg_read_all_tables to the almost-super-user, but maybe that's not
always desirable. Not sure.

Just to be clear, there are lots of other things that a non-superuser
cannot do, such as CREATE LANGUAGE. However, I'm excluding that kind
of thing from this list because it's intrinsically unsafe to allow a
non-superuser to do that, since it's probably a gateway to arbitrary
code execution and then you can probably get superuser for real, and
control of the OS account, too. What I'm interested in is developing a
list of things that could, with the right infrastructure, be delegated
to non-superusers safely but which, as things stand today, cannot be
delegated to non-superusers. Contributions to the list are most
welcome as are thoughts on the proposals above.

Thanks,

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

On Mon, Jan 16, 2023 at 02:29:56PM -0500, Robert Haas wrote:
> 4. You can reserve a small number of connections for the superuser
> with superuser_reserved_connections, but there's no way to do a
> similar thing for any other user. As mentioned above, a CREATEROLE
> user could set connection limits for every created role such that the
> sum of those limits is less than max_connections by some margin, but
> that restricts each of those roles individually, not all of them in
> the aggregate. Maybe we could address this by inventing a new GUC
> reserved_connections and a predefined role
> pg_use_reserved_connections.

I've written something like this before, and I'd be happy to put together a
patch if there is interest.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Mon, Jan 16, 2023 at 5:37 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> On Mon, Jan 16, 2023 at 02:29:56PM -0500, Robert Haas wrote:
> > 4. You can reserve a small number of connections for the superuser
> > with superuser_reserved_connections, but there's no way to do a
> > similar thing for any other user. As mentioned above, a CREATEROLE
> > user could set connection limits for every created role such that the
> > sum of those limits is less than max_connections by some margin, but
> > that restricts each of those roles individually, not all of them in
> > the aggregate. Maybe we could address this by inventing a new GUC
> > reserved_connections and a predefined role
> > pg_use_reserved_connections.
>
> I've written something like this before, and I'd be happy to put together a
> patch if there is interest.

Cool. I had been thinking of coding it up myself, but you doing it works, too.

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

On Mon, Jan 16, 2023 at 09:06:10PM -0500, Robert Haas wrote:
> On Mon, Jan 16, 2023 at 5:37 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> On Mon, Jan 16, 2023 at 02:29:56PM -0500, Robert Haas wrote:
>> > 4. You can reserve a small number of connections for the superuser
>> > with superuser_reserved_connections, but there's no way to do a
>> > similar thing for any other user. As mentioned above, a CREATEROLE
>> > user could set connection limits for every created role such that the
>> > sum of those limits is less than max_connections by some margin, but
>> > that restricts each of those roles individually, not all of them in
>> > the aggregate. Maybe we could address this by inventing a new GUC
>> > reserved_connections and a predefined role
>> > pg_use_reserved_connections.
>>
>> I've written something like this before, and I'd be happy to put together a
>> patch if there is interest.
> 
> Cool. I had been thinking of coding it up myself, but you doing it works, too.

Alright.  The one design question I have is whether this should be a new
set of reserved connections or replace superuser_reserved_connections
entirely.

If we create a new batch of reserved connections, only roles with
privileges of pg_use_reserved_connections would be able to connect if the
number of remaining slots is greater than superuser_reserved_connections
but less than or equal to superuser_reserved_connections +
reserved_connections.  Only superusers would be able to connect if the
number of remaining slots is less than or equal to
superuser_reserved_connections.  This helps avoid blocking new superuser
connections even if you've reserved some connections for non-superusers.

Іf we replace superuser_reserved_connections, we're basically opening up
the existing functionality to non-superusers, which is simpler and probably
more in the spirit of this thread, but it doesn't provide a way to prevent
blocking new superuser connections.

My preference is the former approach.  This is closest to what I've written
before, and if I read your words carefully, it seems to be what you are
proposing.  WDYT?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, Jan 17, 2023 at 1:42 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> Alright.  The one design question I have is whether this should be a new
> set of reserved connections or replace superuser_reserved_connections
> entirely.

I think it should definitely be something new, not a replacement.

> If we create a new batch of reserved connections, only roles with
> privileges of pg_use_reserved_connections would be able to connect if the
> number of remaining slots is greater than superuser_reserved_connections
> but less than or equal to superuser_reserved_connections +
> reserved_connections.  Only superusers would be able to connect if the
> number of remaining slots is less than or equal to
> superuser_reserved_connections.  This helps avoid blocking new superuser
> connections even if you've reserved some connections for non-superusers.

This is precisely what I had in mind.

I think the documentation will need some careful wordsmithing,
including adjustments to superuser_reserved_connections. We want to
recast superuser_reserved_connections as a final reserve to be touched
after even reserved_connections has been exhausted.

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

On Tue, Jan 17, 2023 at 02:59:31PM -0500, Robert Haas wrote:
> On Tue, Jan 17, 2023 at 1:42 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> If we create a new batch of reserved connections, only roles with
>> privileges of pg_use_reserved_connections would be able to connect if the
>> number of remaining slots is greater than superuser_reserved_connections
>> but less than or equal to superuser_reserved_connections +
>> reserved_connections.  Only superusers would be able to connect if the
>> number of remaining slots is less than or equal to
>> superuser_reserved_connections.  This helps avoid blocking new superuser
>> connections even if you've reserved some connections for non-superusers.
> 
> This is precisely what I had in mind.

Great.  Here is a first attempt at the patch.

> I think the documentation will need some careful wordsmithing,
> including adjustments to superuser_reserved_connections. We want to
> recast superuser_reserved_connections as a final reserve to be touched
> after even reserved_connections has been exhausted.

I tried to do this, but there is probably still room for improvement,
especially for the parts that discuss the relationship between
max_connections, superuser_reserved_connections, and reserved_connections.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, Jan 17, 2023 at 7:15 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> Great.  Here is a first attempt at the patch.

In general, looks good. I think this will often call HaveNFreeProcs
twice, though, and that would be better to avoid, e.g.

if (!am_superuser && !am_walsender && (SuperuserReservedBackends +
ReservedBackends) > 0)
    && !HaveNFreeProcs(SuperuserReservedBackends + ReservedBackends))
{
    if (!HaveNFreeProcs(SuperuserReservedBackends))
        remaining connection slots are reserved for non-replication
superuser connections;
    if (!has_privs_of_role(GetUserId(), ROLE_PG_USE_RESERVED_CONNECTIONS))
       remaining connection slots are reserved for roles with
privileges of pg_use_reserved_backends;
}

In the common case where we hit neither limit, this only counts free
connection slots once. We could do even better by making
HaveNFreeProcs have an out parameter for the number of free procs
actually found when it returns false, but that's probably not
important.

I don't think that we should default both the existing GUC and the new
one to 3, because that raises the default limit in the case where the
new feature is not used from 3 to 6. I think we should default one of
them to 0 and the other one to 3. Not sure which one should get which
value.

> > I think the documentation will need some careful wordsmithing,
> > including adjustments to superuser_reserved_connections. We want to
> > recast superuser_reserved_connections as a final reserve to be touched
> > after even reserved_connections has been exhausted.
>
> I tried to do this, but there is probably still room for improvement,
> especially for the parts that discuss the relationship between
> max_connections, superuser_reserved_connections, and reserved_connections.

I think it's pretty good the way you have it. I agree that there might
be a way to make it even better, but I don't think I know what it is.

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

On Mon, Jan 16, 2023 at 2:29 PM Robert Haas <robertmhaas@gmail.com> wrote:
> 1. It's still possible for a CREATEROLE user to hand out role
> attributes that they don't possess. The new prohibitions in
> cf5eb37c5ee0cc54c80d95c1695d7fca1f7c68cb prevent a CREATEROLE user
> from handing out membership in a role on which they lack sufficient
> permissions, but they don't prevent a CREATEROLE user who lacks
> CREATEDB from creating a new user who does have CREATEDB. I think we
> should subject the CREATEDB, REPLICATION, and BYPASSRLS attributes to
> the same rule that we now use for role memberships: you've got to have
> the property in order to give it to someone else. In the case of
> CREATEDB, this would tighten the current rules, which allow you to
> give out CREATEDB without having it. In the case of REPLICATION and
> BYPASSRLS, this would liberalize the current rules: right now, a
> CREATEROLE user cannot give REPLICATION or BYPASSRLS to another user
> even if they possess those attributes.
>
> This proposal doesn't address the CREATEROLE or CONNECTION LIMIT
> properties. It seems possible to me that someone might want to set up
> a CREATEROLE user who can't make more such users, and this proposal
> doesn't manufacture any way of doing that. It also doesn't let you
> constraint the ability of a CREATEROLE user to set a CONNECTION LIMIT
> for some other user. I think that's OK. It might be nice to have ways
> of imposing such restrictions at some point in the future, but it is
> not very obvious what to do about such cases and, importantly, I don't
> think there's any security impact from failing to address those cases.
> If a CREATEROLE user without CREATEDB can create a new role that does
> have CREATEDB, that's a privilege escalation. If they can hand out
> CREATEROLE, that isn't: they already have it.

Here is a patch implementing the above proposal. Since this is fairly
closely related to already-committed work, I would like to get this
into v16. That way, all the changes to how CREATEROLE works will go
into a single release, which seems less confusing for users. It is
also fairly clear to me that this is an improvement over the status
quo. Sometimes things that seem clear to me turn out to be false, so
if this change seems like a problem to you, please let me know.

Thanks,

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

On Wed, Jan 18, 2023 at 11:28:57AM -0500, Robert Haas wrote:
> In general, looks good. I think this will often call HaveNFreeProcs
> twice, though, and that would be better to avoid, e.g.

I should have thought of this.  This is fixed in v2.

> In the common case where we hit neither limit, this only counts free
> connection slots once. We could do even better by making
> HaveNFreeProcs have an out parameter for the number of free procs
> actually found when it returns false, but that's probably not
> important.

Actually, I think it might be important.  IIUC the separate calls to
HaveNFreeProcs might return different values for the same input, which
could result in incorrect error messages (e.g., you might get the
reserved_connections message despite setting reserved_connections to 0).
So, I made this change in v2, too.

> I don't think that we should default both the existing GUC and the new
> one to 3, because that raises the default limit in the case where the
> new feature is not used from 3 to 6. I think we should default one of
> them to 0 and the other one to 3. Not sure which one should get which
> value.

I chose to set reserved_connections to 0 since it is new and doesn't have a
pre-existing default value.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Wed, Jan 18, 2023 at 2:00 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> On Wed, Jan 18, 2023 at 11:28:57AM -0500, Robert Haas wrote:
> > In general, looks good. I think this will often call HaveNFreeProcs
> > twice, though, and that would be better to avoid, e.g.
>
> I should have thought of this.  This is fixed in v2.

Should (nfree < SuperuserReservedBackends) be using <=, or am I confused?

> > In the common case where we hit neither limit, this only counts free
> > connection slots once. We could do even better by making
> > HaveNFreeProcs have an out parameter for the number of free procs
> > actually found when it returns false, but that's probably not
> > important.
>
> Actually, I think it might be important.  IIUC the separate calls to
> HaveNFreeProcs might return different values for the same input, which
> could result in incorrect error messages (e.g., you might get the
> reserved_connections message despite setting reserved_connections to 0).
> So, I made this change in v2, too.

I thought of that briefly and it didn't seem that important, but the
way you did it seems fine, so let's go with that.

What's the deal with removing "and no new replication connections will
be accepted" from the documentation? Is the existing documentation
just wrong? If so, should we fix that first? And maybe delete
"non-replication" from the error message that says "remaining
connection slots are reserved for non-replication superuser
connections"? It seems like right now the comments say that
replication connections are a completely separate pool of connections,
but the documentation and the error message make it sound otherwise.
If that's true, then one of them is wrong, and I think it's the
docs/error message. Or am I just misreading it?

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

On Wed, Jan 18, 2023 at 02:51:38PM -0500, Robert Haas wrote:
> Should (nfree < SuperuserReservedBackends) be using <=, or am I confused?

I believe < is correct.  At this point, the new backend will have already
claimed a proc struct, so if the number of remaining free slots equals the
number of reserved slots, it is okay.

> What's the deal with removing "and no new replication connections will
> be accepted" from the documentation? Is the existing documentation
> just wrong? If so, should we fix that first? And maybe delete
> "non-replication" from the error message that says "remaining
> connection slots are reserved for non-replication superuser
> connections"? It seems like right now the comments say that
> replication connections are a completely separate pool of connections,
> but the documentation and the error message make it sound otherwise.
> If that's true, then one of them is wrong, and I think it's the
> docs/error message. Or am I just misreading it?

I think you are right.  This seems to have been missed in ea92368.  I moved
this part to a new patch that should probably be back-patched to v12.

On that note, I wonder if it's worth changing the "sorry, too many clients
already" message to make it clear that max_connections has been reached.
IME some users are confused by this error, and I think it would be less
confusing if it pointed to the parameter that governs the number of
connection slots.  I'll create a new thread for this.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Wed, Jan 18, 2023 at 12:15:33PM -0500, Robert Haas wrote:
> On Mon, Jan 16, 2023 at 2:29 PM Robert Haas <robertmhaas@gmail.com> wrote:
>> 1. It's still possible for a CREATEROLE user to hand out role
>> attributes that they don't possess. The new prohibitions in
>> cf5eb37c5ee0cc54c80d95c1695d7fca1f7c68cb prevent a CREATEROLE user
>> from handing out membership in a role on which they lack sufficient
>> permissions, but they don't prevent a CREATEROLE user who lacks
>> CREATEDB from creating a new user who does have CREATEDB. I think we
>> should subject the CREATEDB, REPLICATION, and BYPASSRLS attributes to
>> the same rule that we now use for role memberships: you've got to have
>> the property in order to give it to someone else. In the case of
>> CREATEDB, this would tighten the current rules, which allow you to
>> give out CREATEDB without having it. In the case of REPLICATION and
>> BYPASSRLS, this would liberalize the current rules: right now, a
>> CREATEROLE user cannot give REPLICATION or BYPASSRLS to another user
>> even if they possess those attributes.
>>
>> This proposal doesn't address the CREATEROLE or CONNECTION LIMIT
>> properties. It seems possible to me that someone might want to set up
>> a CREATEROLE user who can't make more such users, and this proposal
>> doesn't manufacture any way of doing that. It also doesn't let you
>> constraint the ability of a CREATEROLE user to set a CONNECTION LIMIT
>> for some other user. I think that's OK. It might be nice to have ways
>> of imposing such restrictions at some point in the future, but it is
>> not very obvious what to do about such cases and, importantly, I don't
>> think there's any security impact from failing to address those cases.
>> If a CREATEROLE user without CREATEDB can create a new role that does
>> have CREATEDB, that's a privilege escalation. If they can hand out
>> CREATEROLE, that isn't: they already have it.
> 
> Here is a patch implementing the above proposal. Since this is fairly
> closely related to already-committed work, I would like to get this
> into v16. That way, all the changes to how CREATEROLE works will go
> into a single release, which seems less confusing for users. It is
> also fairly clear to me that this is an improvement over the status
> quo. Sometimes things that seem clear to me turn out to be false, so
> if this change seems like a problem to you, please let me know.

This seems like a clear improvement to me.  However, as the attribute
system becomes more sophisticated, I think we ought to improve the error
messages in user.c.  IMHO messages like "permission denied" could be
greatly improved with some added context.

For example, if I want to change a role's password, I need both CREATEROLE
and ADMIN OPTION on the role, but the error message only mentions
CREATEROLE.

    postgres=# create role createrole with createrole;
    CREATE ROLE
    postgres=# create role otherrole;
    CREATE ROLE
    postgres=# set role createrole;
    SET
    postgres=> alter role otherrole password 'test';
    ERROR:  must have CREATEROLE privilege to change another user's password

Similarly, if I want to allow a role to grant REPLICATION to another role,
I have to give it CREATEROLE, REPLICATION, and membership with ADMIN
OPTION.  If the role is missing CREATEROLE or membership with ADMIN OPTION,
it'll only ever see a "permission denied" error.

    postgres=# create role createrole with createrole;
    CREATE ROLE
    postgres=# create role otherrole;
    CREATE ROLE
    postgres=# set role createrole;
    SET
    postgres=> alter role otherrole with replication;
    ERROR:  permission denied
    postgres=> reset role;
    RESET
    postgres=# alter role createrole with replication;
    ALTER ROLE
    postgres=# set role createrole;
    SET
    postgres=> alter role otherrole with replication;
    ERROR:  permission denied
    postgres=> reset role;
    RESET
    postgres=# grant otherrole to createrole;
    GRANT ROLE
    postgres=# set role createrole;
    SET
    postgres=> alter role otherrole with replication;
    ERROR:  permission denied
    postgres=> reset role;
    RESET
    postgres=# grant otherrole to createrole with admin option;
    GRANT ROLE
    postgres=# set role createrole;
    SET
    postgres=> alter role otherrole with replication;
    ALTER ROLE

If it has both CREATEROLE and membership with ADMIN OPTION (but not
REPLICATION), it'll see a more helpful message.

    postgres=# create role createrole with createrole;
    CREATE ROLE
    postgres=# create role otherrole;
    CREATE ROLE
    postgres=# grant otherrole to createrole with admin option;
    GRANT ROLE
    postgres=# set role createrole;
    SET
    postgres=> alter role otherrole with replication;
    ERROR:  must have replication privilege to change replication attribute

This probably shouldn't block your patch, but I think it's worth doing in
v16 since there are other changes in this area.  I'm happy to help.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On 1/19/23 4:47 AM, Nathan Bossart wrote:
> This seems like a clear improvement to me.  However, as the attribute
> system becomes more sophisticated, I think we ought to improve the error
> messages in user.c.  IMHO messages like "permission denied" could be
> greatly improved with some added context.
I observed this behavior where the role is having creatrole but still 
it's unable to pass it to another user.

postgres=# create role abc1 login createrole;
CREATE ROLE
postgres=# create user test1;
CREATE ROLE
postgres=# \c - abc1
You are now connected to database "postgres" as user "abc1".
postgres=> alter role test1 with createrole ;
ERROR:  permission denied
postgres=>

which was working previously without patch.

Is this an expected behavior?

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On 1/19/23 3:05 PM, tushar wrote:
> which was working previously without patch. 
My bad, I was testing against PG v15 but this issue is not
reproducible on master (without patch).

As you mentioned- "This implements the standard idea that you can't give 
permissions
you don't have (but you can give the ones you do have)" but here the 
role is having
createrole  privilege that he cannot pass on to another user? Is this 
expected?

postgres=# create role fff with createrole;
CREATE ROLE
postgres=# create role xxx;
CREATE ROLE
postgres=# set role fff;
SET
postgres=> alter role xxx with createrole;
ERROR:  permission denied
postgres=>

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On 1/19/23 2:44 AM, Nathan Bossart wrote:
> On Wed, Jan 18, 2023 at 02:51:38PM -0500, Robert Haas wrote:
>> Should (nfree < SuperuserReservedBackends) be using <=, or am I confused?
> I believe < is correct.  At this point, the new backend will have already
> claimed a proc struct, so if the number of remaining free slots equals the
> number of reserved slots, it is okay.
>
>> What's the deal with removing "and no new replication connections will
>> be accepted" from the documentation? Is the existing documentation
>> just wrong? If so, should we fix that first? And maybe delete
>> "non-replication" from the error message that says "remaining
>> connection slots are reserved for non-replication superuser
>> connections"? It seems like right now the comments say that
>> replication connections are a completely separate pool of connections,
>> but the documentation and the error message make it sound otherwise.
>> If that's true, then one of them is wrong, and I think it's the
>> docs/error message. Or am I just misreading it?
> I think you are right.  This seems to have been missed in ea92368.  I moved
> this part to a new patch that should probably be back-patched to v12.
>
> On that note, I wonder if it's worth changing the "sorry, too many clients
> already" message to make it clear that max_connections has been reached.
> IME some users are confused by this error, and I think it would be less
> confusing if it pointed to the parameter that governs the number of
> connection slots.  I'll create a new thread for this.
>
There is  one typo , for the doc changes, it is  mentioned 
"pg_use_reserved_backends" but i think it supposed to be 
"pg_use_reserved_connections"
under Table 22.1. Predefined Roles.

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company

On Thu, Jan 19, 2023 at 6:15 AM tushar <tushar.ahuja@enterprisedb.com> wrote:
> postgres=# create role fff with createrole;
> CREATE ROLE
> postgres=# create role xxx;
> CREATE ROLE
> postgres=# set role fff;
> SET
> postgres=> alter role xxx with createrole;
> ERROR:  permission denied
> postgres=>

Here fff would need ADMIN OPTION on xxx to be able to make modifications to it.

See https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=cf5eb37c5ee0cc54c80d95c1695d7fca1f7c68cb

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

On Wed, Jan 18, 2023 at 6:17 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> > Here is a patch implementing the above proposal. Since this is fairly
> > closely related to already-committed work, I would like to get this
> > into v16. That way, all the changes to how CREATEROLE works will go
> > into a single release, which seems less confusing for users. It is
> > also fairly clear to me that this is an improvement over the status
> > quo. Sometimes things that seem clear to me turn out to be false, so
> > if this change seems like a problem to you, please let me know.
>
> This seems like a clear improvement to me.

Cool.

> However, as the attribute
> system becomes more sophisticated, I think we ought to improve the error
> messages in user.c.  IMHO messages like "permission denied" could be
> greatly improved with some added context.
>
> This probably shouldn't block your patch, but I think it's worth doing in
> v16 since there are other changes in this area.  I'm happy to help.

That would be great. I agree that it's good to try to improve the
error messages. It hasn't been entirely clear to me how to do that.
For instance, I don't think we want to say something like:

ERROR: must have CREATEROLE privilege and ADMIN OPTION on the target
role, or in lieu of both of those to be superuser, to set the
CONNECTION LIMIT for another role
ERROR: must have CREATEROLE privilege and ADMIN OPTION on the target
role, plus also CREATEDB, or in lieu of all that to be superuser, to
remove the CREATEDB property from another role

Such messages are long and we'd end up with a lot of variants. It's
possible that the messages could be multi-tier. For instance, if we
determine that you're trying to manage users and you don't have
permission to manage ANY user, we could say:

ERROR: permission to manage roles denied
DETAIL: You must have the CREATEROLE privilege or be a superuser to
manage roles.

If you could potentially manage some user, but not the one you're
trying to manage, we could say:

ERROR: permission to manage role "%s" denied
DETAIL: You need ADMIN OPTION on the target role to manage it.

If you have permission to manage the target role but not in the
requested manner, we could then say something like:

ERROR: permission to manage CREATEDB for role "%s" denied
DETAIL: You need CREATEDB to manage it.

This is just one idea, and maybe not the best one. I'm just trying to
say that I think this is basically an organizational problem. We need
a plan for how we're going to report errors that is not too
complicated to implement with reasonable effort, and that will produce
messages that users will understand. I'd be delighted if you wanted to
provide either ideas or patches...

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

On Thu, Jan 19, 2023 at 9:21 AM tushar <tushar.ahuja@enterprisedb.com> wrote:
> that is not true because the superuser can still able to connect,

It is true, but because superusers have all privileges.

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

On Wed, Jan 18, 2023 at 4:14 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> On Wed, Jan 18, 2023 at 02:51:38PM -0500, Robert Haas wrote:
> > Should (nfree < SuperuserReservedBackends) be using <=, or am I confused?
>
> I believe < is correct.  At this point, the new backend will have already
> claimed a proc struct, so if the number of remaining free slots equals the
> number of reserved slots, it is okay.

OK. Might be worth a short comment.

> > What's the deal with removing "and no new replication connections will
> > be accepted" from the documentation? Is the existing documentation
> > just wrong? If so, should we fix that first? And maybe delete
> > "non-replication" from the error message that says "remaining
> > connection slots are reserved for non-replication superuser
> > connections"? It seems like right now the comments say that
> > replication connections are a completely separate pool of connections,
> > but the documentation and the error message make it sound otherwise.
> > If that's true, then one of them is wrong, and I think it's the
> > docs/error message. Or am I just misreading it?
>
> I think you are right.  This seems to have been missed in ea92368.  I moved
> this part to a new patch that should probably be back-patched to v12.

I'm inclined to commit it to master and not back-patch. It doesn't
seem important enough to perturb translations.

Tushar seems to have a point about pg_use_reserved_connections vs.
pg_use_reserved_backends. I think we should standardize on the former,
as backends is an internal term.

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

On Thu, Jan 19, 2023 at 11:40:53AM -0500, Robert Haas wrote:
> On Wed, Jan 18, 2023 at 4:14 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> On Wed, Jan 18, 2023 at 02:51:38PM -0500, Robert Haas wrote:
>> > Should (nfree < SuperuserReservedBackends) be using <=, or am I confused?
>>
>> I believe < is correct.  At this point, the new backend will have already
>> claimed a proc struct, so if the number of remaining free slots equals the
>> number of reserved slots, it is okay.
> 
> OK. Might be worth a short comment.

I added one.

>> > What's the deal with removing "and no new replication connections will
>> > be accepted" from the documentation? Is the existing documentation
>> > just wrong? If so, should we fix that first? And maybe delete
>> > "non-replication" from the error message that says "remaining
>> > connection slots are reserved for non-replication superuser
>> > connections"? It seems like right now the comments say that
>> > replication connections are a completely separate pool of connections,
>> > but the documentation and the error message make it sound otherwise.
>> > If that's true, then one of them is wrong, and I think it's the
>> > docs/error message. Or am I just misreading it?
>>
>> I think you are right.  This seems to have been missed in ea92368.  I moved
>> this part to a new patch that should probably be back-patched to v12.
> 
> I'm inclined to commit it to master and not back-patch. It doesn't
> seem important enough to perturb translations.

That seems reasonable to me.

> Tushar seems to have a point about pg_use_reserved_connections vs.
> pg_use_reserved_backends. I think we should standardize on the former,
> as backends is an internal term.

Oops.  This is what I meant to do.  I probably flubbed it because I was
wondering why the parameter uses "connections" and the variable uses
"backends," especially considering that the variable for max_connections is
called MaxConnections.  I went ahead and renamed everything to use
"connections."

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, Jan 19, 2023 at 12:54 PM Nathan Bossart
<nathandbossart@gmail.com> wrote:
> > OK. Might be worth a short comment.
>
> I added one.

Thanks. I'd move it to the inner indentation level so it's closer to
the test at issue.

I would also suggest reordering the documentation and the
postgresql.conf.sample file so that reserved_connections precedes
superuser_reserved_connections, instead of following it.

Other than that, this seems like it might be about ready to commit,
barring objections or bug reports.

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

On Thu, Jan 19, 2023 at 02:17:35PM -0500, Robert Haas wrote:
> On Thu, Jan 19, 2023 at 12:54 PM Nathan Bossart
> <nathandbossart@gmail.com> wrote:
>> > OK. Might be worth a short comment.
>>
>> I added one.
> 
> Thanks. I'd move it to the inner indentation level so it's closer to
> the test at issue.

I meant for it to cover the call to HaveNFreeProcs() as well since the same
idea applies.  I left it the same for now, but if you still think it makes
sense to move it, I'll do so.

> I would also suggest reordering the documentation and the
> postgresql.conf.sample file so that reserved_connections precedes
> superuser_reserved_connections, instead of following it.

Makes sense.

> Other than that, this seems like it might be about ready to commit,
> barring objections or bug reports.

Awesome.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, Jan 19, 2023 at 2:46 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> > Thanks. I'd move it to the inner indentation level so it's closer to
> > the test at issue.
>
> I meant for it to cover the call to HaveNFreeProcs() as well since the same
> idea applies.  I left it the same for now, but if you still think it makes
> sense to move it, I'll do so.

Hmm, OK. If you want to leave it where it is, I won't argue further.

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

There is  one typo , for the doc changes, it is  mentioned "pg_use_reserved_backends" but i think it supposed to be "pg_use_reserved_connections"
under Table 22.1. Predefined Roles.

-- 
regards,tushar
EnterpriseDB  https://www.enterprisedb.com/
The Enterprise PostgreSQL Company 

On Fri, Jan 20, 2023 at 07:04:58PM +0530, tushar wrote:
> On 1/19/23 6:28 PM, tushar wrote:
>> There is  one typo , for the doc changes, it is  mentioned
>> "pg_use_reserved_backends" but i think it supposed to be
>> "pg_use_reserved_connections"
>> under Table 22.1. Predefined Roles.
> 
> Thanks, this is fixed now with the latest patches.

Thank you for reviewing.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Fri, Jan 20, 2023 at 1:10 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> > Thanks, this is fixed now with the latest patches.
>
> Thank you for reviewing.

Thanks to you both. I have committed these patches.

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

On Fri, Jan 20, 2023 at 03:42:03PM -0500, Robert Haas wrote:
> Thanks to you both. I have committed these patches.

Thanks!  Does this need a catversion bump?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Fri, Jan 20, 2023 at 4:02 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> On Fri, Jan 20, 2023 at 03:42:03PM -0500, Robert Haas wrote:
> > Thanks to you both. I have committed these patches.
>
> Thanks!  Does this need a catversion bump?

I was surprised by this question because I thought I'd included one.

But it turns out I didn't include that in the commit and it's still in
my working tree. *facepalm*

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

On Mon, Jan 23, 2023 at 10:25 AM tushar <tushar.ahuja@enterprisedb.com> wrote:
> Please refer to this scenario where I am able to give createrole privileges but not replication  privilege to role
>
> postgres=# create role t1 createrole;
> CREATE ROLE
> postgres=# create role t2 replication;
> CREATE ROLE
> postgres=# create role t3;
> CREATE ROLE
> postgres=# grant t3 to t1,t2 with admin option;
> GRANT ROLE
> postgres=# set session authorization t1;
> SET
> postgres=> alter role t3 createrole ;
> ALTER ROLE
> postgres=> set session authorization t2;
> SET
> postgres=> alter role t3 replication;
> ERROR:  permission denied
>
> This same behavior was observed in v14 as well but why i am able to give createrole grant but not replication?

In previous releases, you needed to have CREATEROLE in order to be
able to perform user management functions. In master, you still need
CREATEROLE, and you also need ADMIN OPTION on the role. In this
scenario, only t1 meets those requirements with respect to t3, so only
t1 can manage t3. t2 can SET ROLE to t3 and grant membership in t3,
but it can't set role properties on t3 or change t3's password or
things like that, because the ability to make user management changes
is controlled by CREATEROLE.

The patch is only intended to change behavior in the case where you
possess both CREATEROLE and also ADMIN OPTION on the target role (but
not SUPERUSER). In that scenario, it intends to change whether you can
give or remove the CREATEDB, REPLICATION, and BYPASSRLS properties
from a user.

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

On Tue, Jan 24, 2023 at 9:07 AM tushar <tushar.ahuja@enterprisedb.com> wrote:
> right, Neha/I have tested with different scenarios using createdb/replication/bypassrls and other
> privileges properties on the role. also checked pg_dumpall/pg_basebackup and everything looks fine.

Thanks. I have committed the patch.

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

moving this discussion to a new thread...

On Thu, Jan 19, 2023 at 10:20:33AM -0500, Robert Haas wrote:
> On Wed, Jan 18, 2023 at 6:17 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
>> However, as the attribute
>> system becomes more sophisticated, I think we ought to improve the error
>> messages in user.c.  IMHO messages like "permission denied" could be
>> greatly improved with some added context.
>>
>> This probably shouldn't block your patch, but I think it's worth doing in
>> v16 since there are other changes in this area.  I'm happy to help.
> 
> That would be great. I agree that it's good to try to improve the
> error messages. It hasn't been entirely clear to me how to do that.
> For instance, I don't think we want to say something like:
> 
> ERROR: must have CREATEROLE privilege and ADMIN OPTION on the target
> role, or in lieu of both of those to be superuser, to set the
> CONNECTION LIMIT for another role
> ERROR: must have CREATEROLE privilege and ADMIN OPTION on the target
> role, plus also CREATEDB, or in lieu of all that to be superuser, to
> remove the CREATEDB property from another role
> 
> Such messages are long and we'd end up with a lot of variants. It's
> possible that the messages could be multi-tier. For instance, if we
> determine that you're trying to manage users and you don't have
> permission to manage ANY user, we could say:
> 
> ERROR: permission to manage roles denied
> DETAIL: You must have the CREATEROLE privilege or be a superuser to
> manage roles.
> 
> If you could potentially manage some user, but not the one you're
> trying to manage, we could say:
> 
> ERROR: permission to manage role "%s" denied
> DETAIL: You need ADMIN OPTION on the target role to manage it.
> 
> If you have permission to manage the target role but not in the
> requested manner, we could then say something like:
> 
> ERROR: permission to manage CREATEDB for role "%s" denied
> DETAIL: You need CREATEDB to manage it.
> 
> This is just one idea, and maybe not the best one. I'm just trying to
> say that I think this is basically an organizational problem. We need
> a plan for how we're going to report errors that is not too
> complicated to implement with reasonable effort, and that will produce
> messages that users will understand. I'd be delighted if you wanted to
> provide either ideas or patches...

Here is an early draft of some modest improvements to the user.c error
messages.  I basically just tried to standardize the style of and add
context to the existing error messages.  I used errhint() for this extra
context, but errdetail() would work, too.  This isn't perfect.  You might
still have to go through a couple rounds of errors before your role has all
the privileges it needs for a command, but this seems to improve matters a
little.

I think there is still a lot of room for improvement, but I wanted to at
least get the discussion started before I went too far.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Thu, Jan 19, 2023 at 10:20:33AM -0500, Robert Haas wrote:
>> That would be great. I agree that it's good to try to improve the
>> error messages. It hasn't been entirely clear to me how to do that.
>> For instance, I don't think we want to say something like:
>> 
>> ERROR: must have CREATEROLE privilege and ADMIN OPTION on the target
>> role, or in lieu of both of those to be superuser, to set the
>> CONNECTION LIMIT for another role
>> ERROR: must have CREATEROLE privilege and ADMIN OPTION on the target
>> role, plus also CREATEDB, or in lieu of all that to be superuser, to
>> remove the CREATEDB property from another role

> Here is an early draft of some modest improvements to the user.c error
> messages.  I basically just tried to standardize the style of and add
> context to the existing error messages.  I used errhint() for this extra
> context, but errdetail() would work, too.

Yeah, I think the right fix is to keep the primary message pretty terse
and add detail in secondary messages.  IMO most of these are errdetail not
errhint, because they are factual details about the rules [1].  But other
than that quibble, Nathan's draft looked pretty good in a quick once-over.

            regards, tom lane

[1] https://www.postgresql.org/docs/devel/error-style-guide.html

Please use 
        errdetail("You must have %s privilege to create roles with %s.",
            "SUPERUSER", "SUPERUSER")));

in this kind of message where multiple copies appear that only differ in
the keyword to use, to avoid creating four copies of essentially the
same string.

This applies in several places.


> -                     errmsg("must have createdb privilege to change createdb attribute")));
> +                     errmsg("permission denied to alter role"),
> +                     errhint("You must have CREATEDB privilege to alter roles with CREATEDB.")));

I think this one is a bit ambiguous; does "with" mean that roles that
have that priv cannot be changed, or does it mean that you cannot meddle
with that bit in particular?  I think it'd be better to say
  "You must have %s privilege to change the %s attribute."
or something like that.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
Maybe there's lots of data loss but the records of data loss are also lost.
(Lincoln Yeoh)

Thanks for taking a look.

On Thu, Jan 26, 2023 at 10:07:39AM +0100, Alvaro Herrera wrote:
> Please use 
>         errdetail("You must have %s privilege to create roles with %s.",
>             "SUPERUSER", "SUPERUSER")));
> 
> in this kind of message where multiple copies appear that only differ in
> the keyword to use, to avoid creating four copies of essentially the
> same string.
> 
> This applies in several places.

I did this in v2.

>> -                     errmsg("must have createdb privilege to change createdb attribute")));
>> +                     errmsg("permission denied to alter role"),
>> +                     errhint("You must have CREATEDB privilege to alter roles with CREATEDB.")));
> 
> I think this one is a bit ambiguous; does "with" mean that roles that
> have that priv cannot be changed, or does it mean that you cannot meddle
> with that bit in particular?  I think it'd be better to say
>   "You must have %s privilege to change the %s attribute."
> or something like that.

Yeah, it's probably better to say "to alter roles with %s" to refer to
roles that presently have the attribute and "to change the %s attribute"
when referring to privileges for the attribute.  I did this in v2, too.

I've also switched from errhint() to errdetail() as suggested by Tom.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, Jan 26, 2023 at 2:14 PM Nathan Bossart <nathandbossart@gmail.com> wrote:
> Yeah, it's probably better to say "to alter roles with %s" to refer to
> roles that presently have the attribute and "to change the %s attribute"
> when referring to privileges for the attribute.  I did this in v2, too.
>
> I've also switched from errhint() to errdetail() as suggested by Tom.

This seems fine to me in general but I'm not entirely sure about this part:

@@ -758,16 +776,13 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
  {
  /* things an unprivileged user certainly can't do */
  if (dinherit || dcreaterole || dcreatedb || dcanlogin || dconnlimit ||
- dvalidUntil || disreplication || dbypassRLS)
+ dvalidUntil || disreplication || dbypassRLS ||
+ (dpassword && roleid != currentUserId))
  ereport(ERROR,
  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("permission denied")));
-
- /* an unprivileged user can change their own password */
- if (dpassword && roleid != currentUserId)
- ereport(ERROR,
- (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- errmsg("must have CREATEROLE privilege to change another user's password")));
+ errmsg("permission denied to alter role"),
+ errdetail("You must have %s privilege and %s on role \"%s\".",
+    "CREATEROLE", "ADMIN OPTION", rolename)));
  }
  else if (!superuser())
  {

Basically my question is whether having one error message for all of
those cases is good enough, or whether we should be trying harder. I
don't mind if the conclusion is that it's OK as-is, and I'm not
entirely sure what would be better. But when I was working on this
code, all of those cases OR'd together feeding into a single error
message seemed a little sketchy to me, so I am wondering what others
think.

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

On Thu, Jan 26, 2023 at 02:42:05PM -0500, Robert Haas wrote:
> @@ -758,16 +776,13 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
>   {
>   /* things an unprivileged user certainly can't do */
>   if (dinherit || dcreaterole || dcreatedb || dcanlogin || dconnlimit ||
> - dvalidUntil || disreplication || dbypassRLS)
> + dvalidUntil || disreplication || dbypassRLS ||
> + (dpassword && roleid != currentUserId))
>   ereport(ERROR,
>   (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> - errmsg("permission denied")));
> -
> - /* an unprivileged user can change their own password */
> - if (dpassword && roleid != currentUserId)
> - ereport(ERROR,
> - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> - errmsg("must have CREATEROLE privilege to change another user's password")));
> + errmsg("permission denied to alter role"),
> + errdetail("You must have %s privilege and %s on role \"%s\".",
> +    "CREATEROLE", "ADMIN OPTION", rolename)));
>   }
>   else if (!superuser())
>   {
> 
> Basically my question is whether having one error message for all of
> those cases is good enough, or whether we should be trying harder. I
> don't mind if the conclusion is that it's OK as-is, and I'm not
> entirely sure what would be better. But when I was working on this
> code, all of those cases OR'd together feeding into a single error
> message seemed a little sketchy to me, so I am wondering what others
> think.

I wondered the same thing, but I hesitated because I didn't want to change
too much in a patch for error messaging.  I can give it a try.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Thu, Jan 26, 2023 at 02:42:05PM -0500, Robert Haas wrote:
>> Basically my question is whether having one error message for all of
>> those cases is good enough, or whether we should be trying harder.

I think the password case needs to be kept separate, because the
conditions for it are different (specifically the exception that
you can alter your own password).  Lumping the rest together
seems OK to me.

            regards, tom lane

On Thu, Jan 26, 2023 at 03:07:43PM -0500, Tom Lane wrote:
> Nathan Bossart <nathandbossart@gmail.com> writes:
>> On Thu, Jan 26, 2023 at 02:42:05PM -0500, Robert Haas wrote:
>>> Basically my question is whether having one error message for all of
>>> those cases is good enough, or whether we should be trying harder.
> 
> I think the password case needs to be kept separate, because the
> conditions for it are different (specifically the exception that
> you can alter your own password).  Lumping the rest together
> seems OK to me.

Hm.  In v2, the error message for both cases is the same:

    ERROR:  permission denied to alter role
    DETAIL:  You must have CREATEROLE privilege and ADMIN OPTION on role "regress_priv_user2".

We could add "to change its attributes" and "to change its password" to
separate the two, but I'm not sure that adds much.  ISTM the current error
message for ALTER ROLE PASSWORD implies that you can change your own
password, and that's lost with my patch.  Perhaps we should add an
errhint() with that information instead.  WDYT?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Nathan Bossart <nathandbossart@gmail.com> writes:
> On Thu, Jan 26, 2023 at 03:07:43PM -0500, Tom Lane wrote:
>> I think the password case needs to be kept separate, because the
>> conditions for it are different (specifically the exception that
>> you can alter your own password).  Lumping the rest together
>> seems OK to me.

> Hm.  In v2, the error message for both cases is the same:

>     ERROR:  permission denied to alter role
>     DETAIL:  You must have CREATEROLE privilege and ADMIN OPTION on role "regress_priv_user2".

> We could add "to change its attributes" and "to change its password" to
> separate the two, but I'm not sure that adds much.  ISTM the current error
> message for ALTER ROLE PASSWORD implies that you can change your own
> password, and that's lost with my patch.  Perhaps we should add an
> errhint() with that information instead.  WDYT?

Well, it's not a hint.  I think the above is fine for non-password
cases, but for passwords maybe

    ERROR:  permission denied to alter role password
    DETAIL:  To change another role's password, you must have CREATEROLE privilege and ADMIN OPTION on role "%s".

            regards, tom lane

On Thu, Jan 26, 2023 at 05:41:32PM -0500, Tom Lane wrote:
> Well, it's not a hint.  I think the above is fine for non-password
> cases, but for passwords maybe
> 
>     ERROR:  permission denied to alter role password
>     DETAIL:  To change another role's password, you must have CREATEROLE privilege and ADMIN OPTION on role "%s".

Okay.  I used this phrasing in v3.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On 26.01.23 01:22, Nathan Bossart wrote:
> Here is an early draft of some modest improvements to the user.c error
> messages.  I basically just tried to standardize the style of and add
> context to the existing error messages.  I used errhint() for this extra
> context, but errdetail() would work, too.  This isn't perfect.  You might
> still have to go through a couple rounds of errors before your role has all
> the privileges it needs for a command, but this seems to improve matters a
> little.
> 
> I think there is still a lot of room for improvement, but I wanted to at
> least get the discussion started before I went too far.

This is good.  If I may assign some more work ;-), we have a bunch of 
error messages like

errmsg("must be superuser or a role with privileges of the 
pg_write_server_files role to create backup stored on server")

errmsg("must be superuser or have privileges of the 
pg_execute_server_program role to COPY to or from an external program")

errmsg("must be superuser or have privileges of pg_read_all_settings to 
examine \"%s\"", ...)

which could also be split up into a pair of

errmsg("permission denied to xxx")
errdetail("You must be superuser or ...")

On Fri, Jan 27, 2023 at 5:00 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> This is good.  If I may assign some more work ;-), we have a bunch of
> error messages like
>
> errmsg("must be superuser or a role with privileges of the
> pg_write_server_files role to create backup stored on server")
>
> errmsg("must be superuser or have privileges of the
> pg_execute_server_program role to COPY to or from an external program")
>
> errmsg("must be superuser or have privileges of pg_read_all_settings to
> examine \"%s\"", ...)
>
> which could also be split up into a pair of
>
> errmsg("permission denied to xxx")
> errdetail("You must be superuser or ...")

I almost hate to bring this up since I'm not sure how far we want to
go down this rat hole, but what should be our policy about mentioning
superuser? I don't think we're entirely consistent right now, and I'm
not sure whether every error message needs to mention that if you were
the superuser you could do everything. Is that something we should
mention always, never, or in some set of circumstances?

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

On Fri, Jan 27, 2023 at 08:31:32AM -0500, Robert Haas wrote:
> I almost hate to bring this up since I'm not sure how far we want to
> go down this rat hole, but what should be our policy about mentioning
> superuser? I don't think we're entirely consistent right now, and I'm
> not sure whether every error message needs to mention that if you were
> the superuser you could do everything. Is that something we should
> mention always, never, or in some set of circumstances?

IMHO superuser should typically only be mentioned when it is the only way
to do something.  Since superusers have all privileges, I think logs like
"superuser or privileges of X" are kind of redundant.  If Robert has
privileges of X, we wouldn't say "privileges of X or Robert."  We'd just
point to X.  Ultimately, I feel like mentioning superuser in error messages
usually just makes the message longer without adding any useful
information.

I recognize that this is a bold opinion and that the policy to mention
superuser might need to be more nuanced in practice...

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Fri, Jan 27, 2023 at 10:52 AM Nathan Bossart
<nathandbossart@gmail.com> wrote:
> IMHO superuser should typically only be mentioned when it is the only way
> to do something.  Since superusers have all privileges, I think logs like
> "superuser or privileges of X" are kind of redundant.  If Robert has
> privileges of X, we wouldn't say "privileges of X or Robert."  We'd just
> point to X.  Ultimately, I feel like mentioning superuser in error messages
> usually just makes the message longer without adding any useful
> information.

That's kind of my opinion too, but I'm not sure whether there are
cases where it will lead to confusion.

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

Robert Haas <robertmhaas@gmail.com> writes:
> I almost hate to bring this up since I'm not sure how far we want to
> go down this rat hole, but what should be our policy about mentioning
> superuser? I don't think we're entirely consistent right now, and I'm
> not sure whether every error message needs to mention that if you were
> the superuser you could do everything. Is that something we should
> mention always, never, or in some set of circumstances?

Good point.  My vote is for standardizing on *not* mentioning it.
Error messages should say "you need privilege X".  That is not
the place to go into all the ways you could hold privilege X
(one of which is being superuser).

            regards, tom lane

On 2023-Jan-27, Tom Lane wrote:

> Good point.  My vote is for standardizing on *not* mentioning it.
> Error messages should say "you need privilege X".  That is not
> the place to go into all the ways you could hold privilege X
> (one of which is being superuser).

+1

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"El sabio habla porque tiene algo que decir;
el tonto, porque tiene que decir algo" (Platon).

While we're here,

On 2023-Jan-26, Nathan Bossart wrote:

> @@ -838,7 +867,8 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
>          if (!should_be_super && roleid == BOOTSTRAP_SUPERUSERID)
>              ereport(ERROR,
>                      (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> -                     errmsg("permission denied: bootstrap user must be superuser")));
> +                     errmsg("permission denied to alter role"),
> +                     errdetail("The bootstrap user must be superuser.")));

I think this one isn't using the right errcode; this is not a case of
insufficient privileges.  There's no priv you can acquire that lets you
do it.  So I'd change it to unsupported operation.


I was confused a bit by this one:

>         /* an unprivileged user can change their own password */
>         if (dpassword && roleid != currentUserId)
>             ereport(ERROR,
>                     (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> -                    errmsg("must have CREATEROLE privilege to change another user's password")));
> +                    errmsg("permission denied to alter role"),
> +                    errdetail("To change another role's password, you must have %s privilege and %s on the role.",
> +                              "CREATEROLE", "ADMIN OPTION")));
>     }

In no other message we say what operation is being attempted in the
errdetail; all the others start with "You must have" and that's it.
However, looking closer I think this one being different is okay,
because the errmsg() you're using is vague, and I think the error report
would be confusing if you were to remove the "To change another role's
password" bit.

The patch looks good to me.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/

On Fri, Jan 27, 2023 at 07:31:19PM +0100, Alvaro Herrera wrote:
> On 2023-Jan-26, Nathan Bossart wrote:
>>              ereport(ERROR,
>>                      (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>> -                     errmsg("permission denied: bootstrap user must be superuser")));
>> +                     errmsg("permission denied to alter role"),
>> +                     errdetail("The bootstrap user must be superuser.")));
> 
> I think this one isn't using the right errcode; this is not a case of
> insufficient privileges.  There's no priv you can acquire that lets you
> do it.  So I'd change it to unsupported operation.

І fixed this in v4.  I've also attached a second patch in which I've
adjusted the messages that Peter mentioned upthread.

One thing that feels a bit odd is how some of the DETAILs mention the
operation being attempted while others do not.  For example, we have

    ERROR:  permission denied to drop role
    DETAIL: You must have SUPERUSER privilege to drop roles with SUPERUSER.

In this case, the DETAIL explains the action that is prohibited.  In other
cases, we have something like

    ERROR:  permission denied to alter role
    DETAIL: You must have CREATEROLE privilege and ADMIN OPTION on role "myrole".

which does not.  I think this is okay because adding "to alter the role" to
the end of the DETAIL seems kind of awkward.  But in other cases, such as

    ERROR:  permission denied to use replication slots
    DETAIL:  You must have REPLICATION privilege.

adding the operation to the end seems less awkward (i.e., "You must have
REPLICATION privilege to use replication slots.").  I don't think there's
any information lost by omitting the action in the DETAIL, so perhaps this
is just a stylistic choice.  I think I'm inclined to add the action to the
DETAIL whenever it doesn't make the message lengthy and awkward, and leave
it out otherwise.  Thoughts?

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Fri, Jan 27, 2023 at 03:15:07PM -0800, Nathan Bossart wrote:
> One thing that feels a bit odd is how some of the DETAILs mention the
> operation being attempted while others do not.  For example, we have
> 
>     ERROR:  permission denied to drop role
>     DETAIL: You must have SUPERUSER privilege to drop roles with SUPERUSER.
> 
> In this case, the DETAIL explains the action that is prohibited.  In other
> cases, we have something like
> 
>     ERROR:  permission denied to alter role
>     DETAIL: You must have CREATEROLE privilege and ADMIN OPTION on role "myrole".
> 
> which does not.  I think this is okay because adding "to alter the role" to
> the end of the DETAIL seems kind of awkward.  But in other cases, such as
> 
>     ERROR:  permission denied to use replication slots
>     DETAIL:  You must have REPLICATION privilege.
> 
> adding the operation to the end seems less awkward (i.e., "You must have
> REPLICATION privilege to use replication slots.").  I don't think there's
> any information lost by omitting the action in the DETAIL, so perhaps this
> is just a stylistic choice.  I think I'm inclined to add the action to the
> DETAIL whenever it doesn't make the message lengthy and awkward, and leave
> it out otherwise.  Thoughts?

Here is a new patch set with this change and some other light editing.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On 07.02.23 21:10, Nathan Bossart wrote:
>>     ERROR:  permission denied to use replication slots
>>     DETAIL:  You must have REPLICATION privilege.
>>
>> adding the operation to the end seems less awkward (i.e., "You must have
>> REPLICATION privilege to use replication slots.").  I don't think there's
>> any information lost by omitting the action in the DETAIL, so perhaps this
>> is just a stylistic choice.  I think I'm inclined to add the action to the
>> DETAIL whenever it doesn't make the message lengthy and awkward, and leave
>> it out otherwise.  Thoughts?
> Here is a new patch set with this change and some other light editing.

I'm concerned about the loose use of "privilege" here.  A privilege is 
something I can grant.  So if someone doesn't have the "REPLICATION 
privilege", as in the above example, I would expect to be able to do 
"GRANT REPLICATION TO someuser".  Since that is not what is happening, 
we should use some other term.  The documentation around CREATE USER 
uses the terms "attribute" and "option" (and also "privilege") for these 
things.

Similarly -- this is an existing issue but we might as well look at it 
-- in something like

     must be superuser or a role with privileges of the
     pg_write_server_files role

the phrase "a role with the privileges of that other role" seems 
ambiguous.  Doesn't it really mean you must be a member of that role?

I also feel that in sentences like

     "You must have %s privilege to create roles."

a "the" is missing.

On Mon, Feb 20, 2023 at 08:54:48AM +0100, Peter Eisentraut wrote:
> I'm concerned about the loose use of "privilege" here.  A privilege is
> something I can grant.  So if someone doesn't have the "REPLICATION
> privilege", as in the above example, I would expect to be able to do "GRANT
> REPLICATION TO someuser".  Since that is not what is happening, we should
> use some other term.  The documentation around CREATE USER uses the terms
> "attribute" and "option" (and also "privilege") for these things.

Good point.  I will adjust these to use "attribute" instead.

> Similarly -- this is an existing issue but we might as well look at it -- in
> something like
> 
>     must be superuser or a role with privileges of the
>     pg_write_server_files role
> 
> the phrase "a role with the privileges of that other role" seems ambiguous.
> Doesn't it really mean you must be a member of that role?

Membership alone is not sufficient.  You must also inherit the privileges
of the role via the INHERIT option.  I thought about making this something
like

    must have the INHERIT option on role %s

but I'm not sure that's accurate either.  That wording makes it sound lіke
you need to be granted membership to the role directly WITH INHERIT OPTION,
but what you really need is membership, direct or indirect, with an INHERIT
chain up to the role in question.  However, it looks like "must have the
ADMIN option on role %s" is used to mean something similar, so perhaps I am
overthinking it.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Mon, Feb 20, 2023 at 11:02:10AM -0800, Nathan Bossart wrote:
> On Mon, Feb 20, 2023 at 08:54:48AM +0100, Peter Eisentraut wrote:
>> I'm concerned about the loose use of "privilege" here.  A privilege is
>> something I can grant.  So if someone doesn't have the "REPLICATION
>> privilege", as in the above example, I would expect to be able to do "GRANT
>> REPLICATION TO someuser".  Since that is not what is happening, we should
>> use some other term.  The documentation around CREATE USER uses the terms
>> "attribute" and "option" (and also "privilege") for these things.
> 
> Good point.  I will adjust these to use "attribute" instead.

done in v6

>> Similarly -- this is an existing issue but we might as well look at it -- in
>> something like
>> 
>>     must be superuser or a role with privileges of the
>>     pg_write_server_files role
>> 
>> the phrase "a role with the privileges of that other role" seems ambiguous.
>> Doesn't it really mean you must be a member of that role?
> 
> Membership alone is not sufficient.  You must also inherit the privileges
> of the role via the INHERIT option.  I thought about making this something
> like
> 
>     must have the INHERIT option on role %s
> 
> but I'm not sure that's accurate either.  That wording makes it sound lіke
> you need to be granted membership to the role directly WITH INHERIT OPTION,
> but what you really need is membership, direct or indirect, with an INHERIT
> chain up to the role in question.  However, it looks like "must have the
> ADMIN option on role %s" is used to mean something similar, so perhaps I am
> overthinking it.

For now, I've reworded these as "must inherit privileges of".

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On 20.02.23 23:58, Nathan Bossart wrote:
>>> Similarly -- this is an existing issue but we might as well look at it -- in
>>> something like
>>>
>>>      must be superuser or a role with privileges of the
>>>      pg_write_server_files role
>>>
>>> the phrase "a role with the privileges of that other role" seems ambiguous.
>>> Doesn't it really mean you must be a member of that role?
>>
>> Membership alone is not sufficient.  You must also inherit the privileges
>> of the role via the INHERIT option.  I thought about making this something
>> like
>>
>>     must have the INHERIT option on role %s
>>
>> but I'm not sure that's accurate either.  That wording makes it sound lіke
>> you need to be granted membership to the role directly WITH INHERIT OPTION,
>> but what you really need is membership, direct or indirect, with an INHERIT
>> chain up to the role in question.  However, it looks like "must have the
>> ADMIN option on role %s" is used to mean something similar, so perhaps I am
>> overthinking it.
> 
> For now, I've reworded these as "must inherit privileges of".

I don't have a good mental model of all this role inheritance, 
personally, but I fear that this change makes the messages more jargony 
and less clear.  Maybe the original wording was good enough.

A couple of other thoughts:

"admin option" is sort of a natural language term, I think, so we don't 
need to parametrize it as "%s option".  Also, there are no other 
"options" in this context, I think.

A general thought: It seems we currently don't have any error messages 
that address the user like "You must do this".  Do we want to go there? 
Should we try for a more impersonal wording like

"You must have the %s attribute to create roles."

"Current user must have the %s attribute to create roles."

"%s attribute is required to create roles."

By the way, I'm not sure what the separation between 0001 and 0002 is 
supposed to be.

On Thu, Mar 09, 2023 at 10:55:54AM +0100, Peter Eisentraut wrote:
> On 20.02.23 23:58, Nathan Bossart wrote:
>> For now, I've reworded these as "must inherit privileges of".
> 
> I don't have a good mental model of all this role inheritance, personally,
> but I fear that this change makes the messages more jargony and less clear.
> Maybe the original wording was good enough.

I'm fine with that.

> "admin option" is sort of a natural language term, I think, so we don't need
> to parametrize it as "%s option".  Also, there are no other "options" in
> this context, I think.

v16 introduces the INHERIT and SET options.  I don't have a strong opinion
about parameterizing it, though.  My intent was to consistently capitalize
all the attributes and options.

> A general thought: It seems we currently don't have any error messages that
> address the user like "You must do this".  Do we want to go there? Should we
> try for a more impersonal wording like
> 
> "You must have the %s attribute to create roles."
> 
> "Current user must have the %s attribute to create roles."
> 
> "%s attribute is required to create roles."

I think I like the last option the most.  In general, I agree with trying
to avoid the second-person phrasing.

> By the way, I'm not sure what the separation between 0001 and 0002 is
> supposed to be.

I'll combine them.  I first started with user.c only, but we kept finding
new messages to improve.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, Mar 09, 2023 at 09:58:46AM -0800, Nathan Bossart wrote:
> On Thu, Mar 09, 2023 at 10:55:54AM +0100, Peter Eisentraut wrote:
>> On 20.02.23 23:58, Nathan Bossart wrote:
>>> For now, I've reworded these as "must inherit privileges of".
>> 
>> I don't have a good mental model of all this role inheritance, personally,
>> but I fear that this change makes the messages more jargony and less clear.
>> Maybe the original wording was good enough.
> 
> I'm fine with that.

I used the original wording in v7.

>> "admin option" is sort of a natural language term, I think, so we don't need
>> to parametrize it as "%s option".  Also, there are no other "options" in
>> this context, I think.
> 
> v16 introduces the INHERIT and SET options.  I don't have a strong opinion
> about parameterizing it, though.  My intent was to consistently capitalize
> all the attributes and options.

I didn't change this in v7, but I can do so if you still think it shouldn't
be parameterized.

>> A general thought: It seems we currently don't have any error messages that
>> address the user like "You must do this".  Do we want to go there? Should we
>> try for a more impersonal wording like
>> 
>> "You must have the %s attribute to create roles."
>> 
>> "Current user must have the %s attribute to create roles."
>> 
>> "%s attribute is required to create roles."
> 
> I think I like the last option the most.  In general, I agree with trying
> to avoid the second-person phrasing.

I ended up using the "current user must have" wording in a few places, and
for most others, I used "only roles with X may do Y."  That seemed to flow
relatively well, and IMO it made the required privileges abundantly clear.
I initially was going to use the "X attribute is required to Y" wording,
but I was worried that didn't make it sufficiently clear that the _role_
must have the attribute.  In any case, I'm not wedded to the approach I
used in the patch and am willing to try out other wordings.

BTW I did find one example of a "you must" message while I was updating the
patch:

    write_stderr("%s does not know where to find the server configuration file.\n"
                 "You must specify the --config-file or -D invocation "
                 "option or set the PGDATA environment variable.\n",
                 progname);

I don't think it's a common style, though.

>> By the way, I'm not sure what the separation between 0001 and 0002 is
>> supposed to be.
> 
> I'll combine them.  I first started with user.c only, but we kept finding
> new messages to improve.

I combined the patches in v7.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On 10.03.23 01:03, Nathan Bossart wrote:
>>> By the way, I'm not sure what the separation between 0001 and 0002 is
>>> supposed to be.
>> I'll combine them.  I first started with user.c only, but we kept finding
>> new messages to improve.
> I combined the patches in v7.

I have committed two pieces that were not message changes separately.


I think the following change in DropRole() is incorrect:

         if (!is_admin_of_role(GetUserId(), roleid))
             ereport(ERROR,
                     (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
-                    errmsg("must have admin option on role \"%s\"",
-                           role)));
+                    errmsg("permission denied to drop role"),
+                    errdetail("Only roles with the %s attribute and the 
%s option on role \"%s\" may drop this role.",
+                              "CREATEROLE", "ADMIN", 
NameStr(roleform->rolname))));

The message does not reflect what check is actually performed.  (Perhaps 
this was confused with a similar but not exactly the same check in 
RenameRole().)

That was the only "factual" error that I found.


In file_fdw_validator(), the option names "filename" and "program" could 
be parameterized.


In DropOwnedObjects() and ReassignOwnedObjects(), I suggest the 
following changes, for clarity:

- errdetail("Only roles with privileges of role \"%s\" may drop its 
objects.",
+ errdetail("Only roles with privileges of role \"%s\" may drop objects 
owned by it.",

- errdetail("Only roles with privileges of role \"%s\" may reassign its 
objects.",
+ errdetail("Only roles with privileges of role \"%s\" may reassign 
objects owned by it.",


The rest looks okay to me.

On Thu, Mar 16, 2023 at 04:24:07PM +0100, Peter Eisentraut wrote:
> I have committed two pieces that were not message changes separately.

Thanks!

> I think the following change in DropRole() is incorrect:
> 
>         if (!is_admin_of_role(GetUserId(), roleid))
>             ereport(ERROR,
>                     (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
> -                    errmsg("must have admin option on role \"%s\"",
> -                           role)));
> +                    errmsg("permission denied to drop role"),
> +                    errdetail("Only roles with the %s attribute and the %s
> option on role \"%s\" may drop this role.",
> +                              "CREATEROLE", "ADMIN",
> NameStr(roleform->rolname))));
> 
> The message does not reflect what check is actually performed.  (Perhaps
> this was confused with a similar but not exactly the same check in
> RenameRole().)

Hm.  Is your point that we should only mention the admin option here?  I
mentioned both createrole and admin option in this message (and the
createrole check above this point) in an attempt to avoid giving partial
information.

> In file_fdw_validator(), the option names "filename" and "program" could be
> parameterized.
> 
> 
> In DropOwnedObjects() and ReassignOwnedObjects(), I suggest the following
> changes, for clarity:
> 
> - errdetail("Only roles with privileges of role \"%s\" may drop its
> objects.",
> + errdetail("Only roles with privileges of role \"%s\" may drop objects
> owned by it.",
> 
> - errdetail("Only roles with privileges of role \"%s\" may reassign its
> objects.",
> + errdetail("Only roles with privileges of role \"%s\" may reassign objects
> owned by it.",

Will do.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On 16.03.23 16:48, Nathan Bossart wrote:
>> I think the following change in DropRole() is incorrect:
>>
>>          if (!is_admin_of_role(GetUserId(), roleid))
>>              ereport(ERROR,
>>                      (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>> -                    errmsg("must have admin option on role \"%s\"",
>> -                           role)));
>> +                    errmsg("permission denied to drop role"),
>> +                    errdetail("Only roles with the %s attribute and the %s
>> option on role \"%s\" may drop this role.",
>> +                              "CREATEROLE", "ADMIN",
>> NameStr(roleform->rolname))));
>>
>> The message does not reflect what check is actually performed.  (Perhaps
>> this was confused with a similar but not exactly the same check in
>> RenameRole().)
> Hm.  Is your point that we should only mention the admin option here?  I
> mentioned both createrole and admin option in this message (and the
> createrole check above this point) in an attempt to avoid giving partial
> information.

AFAICT, the mention of CREATEROLE is incorrect, because the code doesn't 
actually check for the CREATEROLE attribute.

On Thu, Mar 16, 2023 at 04:59:53PM +0100, Peter Eisentraut wrote:
> On 16.03.23 16:48, Nathan Bossart wrote:
>> > I think the following change in DropRole() is incorrect:
>> > 
>> >          if (!is_admin_of_role(GetUserId(), roleid))
>> >              ereport(ERROR,
>> >                      (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
>> > -                    errmsg("must have admin option on role \"%s\"",
>> > -                           role)));
>> > +                    errmsg("permission denied to drop role"),
>> > +                    errdetail("Only roles with the %s attribute and the %s
>> > option on role \"%s\" may drop this role.",
>> > +                              "CREATEROLE", "ADMIN",
>> > NameStr(roleform->rolname))));
>> > 
>> > The message does not reflect what check is actually performed.  (Perhaps
>> > this was confused with a similar but not exactly the same check in
>> > RenameRole().)
>> Hm.  Is your point that we should only mention the admin option here?  I
>> mentioned both createrole and admin option in this message (and the
>> createrole check above this point) in an attempt to avoid giving partial
>> information.
> 
> AFAICT, the mention of CREATEROLE is incorrect, because the code doesn't
> actually check for the CREATEROLE attribute.

There is a createrole check at the top of DropRole():

    /*
     * DROP ROLE
     */
    void
    DropRole(DropRoleStmt *stmt)
    {
        Relation    pg_authid_rel,
                    pg_auth_members_rel;
        ListCell   *item;
        List       *role_addresses = NIL;
    
        if (!have_createrole_privilege())
            ereport(ERROR,
                    (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
                     errmsg("permission denied to drop role")));

Granted, no one will see the admin option error unless they at least have
createrole, so we could leave it out, but my intent was to list the full
set of privileges required to drop the role to avoid ambiguity.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

Here is a rebased patch in which I've addressed the latest feedback except
for the DropRole() part that is under discussion.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On 17.03.23 00:47, Nathan Bossart wrote:
> Here is a rebased patch in which I've addressed the latest feedback except
> for the DropRole() part that is under discussion.

committed

On Fri, Mar 17, 2023 at 10:40:06AM +0100, Peter Eisentraut wrote:
> committed

Thanks!

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com