pgsql: Adjust interaction of CREATEROLE with role properties.
Adjust interaction of CREATEROLE with role properties.
Previously, a CREATEROLE user without SUPERUSER could not alter
REPLICATION users in any way, and could not set the BYPASSRLS
attribute. However, they could manipulate the CREATEDB property
even if they themselves did not possess it.
With this change, a CREATEROLE user without SUPERUSER can set or
clear the REPLICATION, BYPASSRLS, or CREATEDB property on a new
role or a role that they have rights to manage if and only if
that property is set for their own role.
This implements the standard idea that you can't give permissions
you don't have (but you can give the ones you do have). We might
in the future want to provide more powerful ways to constrain
what a CREATEROLE user can do - for example, to limit whether
CONNECTION LIMIT can be set or the values to which it can be set -
but that is left as future work.
Patch by me, reviewed by Nathan Bossart, Tushar Ahuja, and Neha
Sharma.
Discussion: /messages/by-id/CA+TgmobX=LHg_J5aT=0pi9gJy=JdtrUVGAu0zhr-i5v5nNbJDg@mail.gmail.com
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/f1358ca52dd7b8cedd29c6f2f8c163914f03ea2e
Modified Files
--------------
doc/src/sgml/ref/alter_role.sgml | 13 +++--
doc/src/sgml/ref/create_role.sgml | 23 +++------
src/backend/commands/dbcommands.c | 3 +-
src/backend/commands/user.c | 82 ++++++++++++++-----------------
src/include/commands/dbcommands.h | 1 +
src/test/regress/expected/create_role.out | 53 ++++++++++++++++----
src/test/regress/sql/create_role.sql | 45 ++++++++++++++---
7 files changed, 137 insertions(+), 83 deletions(-)
On 2023-Jan-24, Robert Haas wrote:
Adjust interaction of CREATEROLE with role properties.
This commit broke the ability to run 'make installcheck' repeatedly,
because it fails to drop role regress_role_limited_admin.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
Thou shalt study thy libraries and strive not to reinvent them without
cause, that thy code may be short and readable and thy days pleasant
and productive. (7th Commandment for C Programmers)
On Thu, Jan 26, 2023 at 6:58 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2023-Jan-24, Robert Haas wrote:
Adjust interaction of CREATEROLE with role properties.
This commit broke the ability to run 'make installcheck' repeatedly,
because it fails to drop role regress_role_limited_admin.
Argh, sorry. I keep making that mistake.
--
Robert Haas
EDB: http://www.enterprisedb.com
On 2023-01-26 Th 08:00, Robert Haas wrote:
On Thu, Jan 26, 2023 at 6:58 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2023-Jan-24, Robert Haas wrote:
Adjust interaction of CREATEROLE with role properties.
This commit broke the ability to run 'make installcheck' repeatedly,
because it fails to drop role regress_role_limited_admin.Argh, sorry. I keep making that mistake.
Would it be worth adding a check right at the end of the schedule that
makes sure there are no such roles left?
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Thu, Jan 26, 2023 at 09:46:07AM -0500, Andrew Dunstan wrote:
On 2023-01-26 Th 08:00, Robert Haas wrote:
On Thu, Jan 26, 2023 at 6:58 AM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
On 2023-Jan-24, Robert Haas wrote:
Adjust interaction of CREATEROLE with role properties.
This commit broke the ability to run 'make installcheck' repeatedly,
because it fails to drop role regress_role_limited_admin.Argh, sorry. I keep making that mistake.
Would it be worth adding a check right at the end of the schedule that
makes sure there are no such roles left?
Yes, because the alternative is to have cirrus run "installcheck"
twice..
On Thu, Jan 26, 2023 at 12:31:06PM -0600, Justin Pryzby wrote:
On Thu, Jan 26, 2023 at 09:46:07AM -0500, Andrew Dunstan wrote:
Would it be worth adding a check right at the end of the schedule that
makes sure there are no such roles left?Yes, because the alternative is to have cirrus run "installcheck"
twice..
Agreed about doing something like that, with a new SQL script perhaps?
We can rely on -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS here, and
I'd like to think that nobody creates such role names or they would
see failures with the new query. I can also be a sign that such
instances should be cleaned up to allow more repeatability with some
tests, actually..
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Thu, Jan 26, 2023 at 12:31:06PM -0600, Justin Pryzby wrote:
On Thu, Jan 26, 2023 at 09:46:07AM -0500, Andrew Dunstan wrote:
Would it be worth adding a check right at the end of the schedule that
makes sure there are no such roles left?
Yes, because the alternative is to have cirrus run "installcheck"
twice..
Agreed about doing something like that, with a new SQL script perhaps?
We can rely on -DENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS here, and
I'd like to think that nobody creates such role names or they would
see failures with the new query.
Yeah ... if we put it into an existing script, somebody will blindly
add more tests after it someday. I suggest calling it "test_cleanup"
to pair with "test_setup".
Is it worth checking for leftover regress_xxx tablespaces as well as
roles?
regards, tom lane
On Thu, Jan 26, 2023 at 09:08:23PM -0500, Tom Lane wrote:
Yeah ... if we put it into an existing script, somebody will blindly
add more tests after it someday. I suggest calling it "test_cleanup"
to pair with "test_setup".Is it worth checking for leftover regress_xxx tablespaces as well as
roles?
Guess so. If this is to be applied to everything that fails under the
naming restrictions, there could be a point in doing the same for
databases, subscriptions and replication origins. Now the argument
has less weight for these object types, surely, compared to roles and
tbspaces.
--
Michael
Michael Paquier <michael@paquier.xyz> writes:
On Thu, Jan 26, 2023 at 09:08:23PM -0500, Tom Lane wrote:
Is it worth checking for leftover regress_xxx tablespaces as well as
roles?
Guess so. If this is to be applied to everything that fails under the
naming restrictions, there could be a point in doing the same for
databases, subscriptions and replication origins. Now the argument
has less weight for these object types, surely, compared to roles and
tbspaces.
By the time we've spun up a new backend, we might as well do more
than one query, so it seems like we might as well check everything
that's globally visible.
However ... this'd only cover mistakes of this kind in the core
regression tests. Is there a way to cover contrib's installcheck
(without adding an additional psql run to each module's tests)?
Maybe we need to enforce this at some other level than the tests
themselves, perhaps in pg_regress?
regards, tom lane
On 2023-01-26 Th 22:18, Tom Lane wrote:
Michael Paquier <michael@paquier.xyz> writes:
On Thu, Jan 26, 2023 at 09:08:23PM -0500, Tom Lane wrote:
Is it worth checking for leftover regress_xxx tablespaces as well as
roles?Guess so. If this is to be applied to everything that fails under the
naming restrictions, there could be a point in doing the same for
databases, subscriptions and replication origins. Now the argument
has less weight for these object types, surely, compared to roles and
tbspaces.By the time we've spun up a new backend, we might as well do more
than one query, so it seems like we might as well check everything
that's globally visible.However ... this'd only cover mistakes of this kind in the core
regression tests. Is there a way to cover contrib's installcheck
(without adding an additional psql run to each module's tests)?Maybe we need to enforce this at some other level than the tests
themselves, perhaps in pg_regress?
Yeah, that seems like a better way to go. Then it would even apply to
external project tests.
cheers
andrew
--
Andrew Dunstan
EDB: https://www.enterprisedb.com
On Fri, Jan 27, 2023 at 08:27:40AM -0500, Andrew Dunstan wrote:
On 2023-01-26 Th 22:18, Tom Lane wrote:
Michael Paquier <michael@paquier.xyz> writes:
On Thu, Jan 26, 2023 at 09:08:23PM -0500, Tom Lane wrote:
Is it worth checking for leftover regress_xxx tablespaces as well as
roles?Guess so. If this is to be applied to everything that fails under the
naming restrictions, there could be a point in doing the same for
databases, subscriptions and replication origins. Now the argument
has less weight for these object types, surely, compared to roles and
tbspaces.By the time we've spun up a new backend, we might as well do more
than one query, so it seems like we might as well check everything
that's globally visible.However ... this'd only cover mistakes of this kind in the core
regression tests. Is there a way to cover contrib's installcheck
(without adding an additional psql run to each module's tests)?Maybe we need to enforce this at some other level than the tests
themselves, perhaps in pg_regress?Yeah, that seems like a better way to go. Then it would even apply to
external project tests.
Maybe this could be done by creating a pg_dump ?
The dump could be grepped (like pg_restore -l |perl -pe '/ROLE|.../')
for offending objects.
And the dump could be re-used for other things, like not re-running
regression tests in pg_upgrade and 027_stream_regress.
I don't know whether the pg_dump would be run directly by pg_regress or
something else.
--
Justin
Justin Pryzby <pryzby@telsasoft.com> writes:
On Fri, Jan 27, 2023 at 08:27:40AM -0500, Andrew Dunstan wrote:
On 2023-01-26 Th 22:18, Tom Lane wrote:
Maybe we need to enforce this at some other level than the tests
themselves, perhaps in pg_regress?
Yeah, that seems like a better way to go. Then it would even apply to
external project tests.
Maybe this could be done by creating a pg_dump ?
That'd be an amazingly expensive way to do it.
I was imagining that pg_regress could contain a function that
issues some predefined queries and complains if the results
aren't empty. It already has the ability to issue commands
to the cluster-under-test (for the initial CREATE DATABASE etc)
so at least most of the infrastructure is easily at hand.
regards, tom lane
On Fri, Jan 27, 2023 at 10:46:53AM -0500, Tom Lane wrote:
I was imagining that pg_regress could contain a function that
issues some predefined queries and complains if the results
aren't empty. It already has the ability to issue commands
to the cluster-under-test (for the initial CREATE DATABASE etc)
so at least most of the infrastructure is easily at hand.
Agreed. I was also thinking among these lines this morning after
waking up. There is a pattern here, actually. Without storing full
queries, we could have a function that has arguments about:
- The attribute name to report back if we find an unwanted name.
- The catalog name to query.
- The qual to use.
That's an implementation detail, though.
There is also a gotcha that has not been mentioned yet: there should
be an option switch to control if this check is run or not. In the
tree, test_pg_dump is an exception where this is not going to work,
because we leave around the role regress_dump_test_role after repeated
installchecks. It has been argued in the past that this should be
cleaned up, but also counter-argued that this is useful for pg_upgrade
in the buildfarm with the dump of extensions.
--
Michael