Thread: 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: http://postgr.es/m/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
Attachment
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
Attachment
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