Add regression coverage for REVOKE ADMIN OPTION
Hackers,
While working on a fix for dangling references to dropped roles in the pg_auth_members.grantor field, I happened to notice we entirely lack regression test coverage of the REVOKE ADMIN OPTION FOR form of the RevokeRoleStmt. I am unaware of any bugs in the current implementation, but future work on roles may benefit if we close the testing gap.
For consideration:
Attachments:
v1-0001-Add-test-for-REVOKE-ADMIN-OPTION.patchapplication/octet-stream; name=v1-0001-Add-test-for-REVOKE-ADMIN-OPTION.patch; x-unix-mode=0644Download
From e550d2f94cc97d0c37e42ab32fd4b419db211bbb Mon Sep 17 00:00:00 2001
From: Mark Dilger <mark.dilger@enterprisedb.com>
Date: Mon, 8 Nov 2021 19:18:07 -0800
Subject: [PATCH v1] Add test for REVOKE ADMIN OPTION
Close regression test gap for "REVOKE ADMIN OPTION FOR role_name"
syntax, which strangely had no test coverage.
---
src/test/regress/expected/privileges.out | 22 ++++++++++++++++++
src/test/regress/sql/privileges.sql | 29 ++++++++++++++++++++++++
2 files changed, 51 insertions(+)
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 9b91865dcc..5ab86e5d51 100644
--- a/src/test/regress/expected/privileges.out
+++ b/src/test/regress/expected/privileges.out
@@ -29,8 +29,30 @@ CREATE USER regress_priv_user5; -- duplicate
ERROR: role "regress_priv_user5" already exists
CREATE USER regress_priv_user6;
CREATE USER regress_priv_user7;
+CREATE USER regress_priv_user8;
+CREATE USER regress_priv_user9;
+CREATE USER regress_priv_user10;
GRANT pg_read_all_data TO regress_priv_user6;
GRANT pg_write_all_data TO regress_priv_user7;
+GRANT pg_read_all_settings TO regress_priv_user8 WITH ADMIN OPTION;
+SET SESSION AUTHORIZATION regress_priv_user8;
+GRANT pg_read_all_settings TO regress_priv_user9 WITH ADMIN OPTION;
+SET SESSION AUTHORIZATION regress_priv_user9;
+GRANT pg_read_all_settings TO regress_priv_user10;
+SET SESSION AUTHORIZATION regress_priv_user8;
+REVOKE pg_read_all_settings FROM regress_priv_user10;
+REVOKE ADMIN OPTION FOR pg_read_all_settings FROM regress_priv_user9;
+REVOKE pg_read_all_settings FROM regress_priv_user9;
+RESET SESSION AUTHORIZATION;
+REVOKE ADMIN OPTION FOR pg_read_all_settings FROM regress_priv_user8;
+SET SESSION AUTHORIZATION regress_priv_user8;
+SET ROLE pg_read_all_settings;
+RESET ROLE;
+RESET SESSION AUTHORIZATION;
+REVOKE pg_read_all_settings FROM regress_priv_user8;
+DROP USER regress_priv_user10;
+DROP USER regress_priv_user9;
+DROP USER regress_priv_user8;
CREATE GROUP regress_priv_group1;
CREATE GROUP regress_priv_group2 WITH USER regress_priv_user1, regress_priv_user2;
ALTER GROUP regress_priv_group1 ADD USER regress_priv_user4;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 6353a1cb8c..bc9f69d0bd 100644
--- a/src/test/regress/sql/privileges.sql
+++ b/src/test/regress/sql/privileges.sql
@@ -32,9 +32,38 @@ CREATE USER regress_priv_user5;
CREATE USER regress_priv_user5; -- duplicate
CREATE USER regress_priv_user6;
CREATE USER regress_priv_user7;
+CREATE USER regress_priv_user8;
+CREATE USER regress_priv_user9;
+CREATE USER regress_priv_user10;
GRANT pg_read_all_data TO regress_priv_user6;
GRANT pg_write_all_data TO regress_priv_user7;
+GRANT pg_read_all_settings TO regress_priv_user8 WITH ADMIN OPTION;
+
+SET SESSION AUTHORIZATION regress_priv_user8;
+GRANT pg_read_all_settings TO regress_priv_user9 WITH ADMIN OPTION;
+
+SET SESSION AUTHORIZATION regress_priv_user9;
+GRANT pg_read_all_settings TO regress_priv_user10;
+
+SET SESSION AUTHORIZATION regress_priv_user8;
+REVOKE pg_read_all_settings FROM regress_priv_user10;
+REVOKE ADMIN OPTION FOR pg_read_all_settings FROM regress_priv_user9;
+REVOKE pg_read_all_settings FROM regress_priv_user9;
+
+RESET SESSION AUTHORIZATION;
+REVOKE ADMIN OPTION FOR pg_read_all_settings FROM regress_priv_user8;
+
+SET SESSION AUTHORIZATION regress_priv_user8;
+SET ROLE pg_read_all_settings;
+RESET ROLE;
+
+RESET SESSION AUTHORIZATION;
+REVOKE pg_read_all_settings FROM regress_priv_user8;
+
+DROP USER regress_priv_user10;
+DROP USER regress_priv_user9;
+DROP USER regress_priv_user8;
CREATE GROUP regress_priv_group1;
CREATE GROUP regress_priv_group2 WITH USER regress_priv_user1, regress_priv_user2;
--
2.21.1 (Apple Git-122.3)
On 16 Nov 2021, at 00:58, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
While working on a fix for dangling references to dropped roles in the pg_auth_members.grantor field, I happened to notice we entirely lack regression test coverage of the REVOKE ADMIN OPTION FOR form of the RevokeRoleStmt. I am unaware of any bugs in the current implementation, but future work on roles may benefit if we close the testing gap.
LGTM. Reading this I realized that the GRANTED BY keyword for RevokeRoleStmt
isn't working as documented, it's not checking the role at all. I've sent a
diff for that with tests on the relevant thread, but I think it would be a good
to get this in too to boost coverage.
--
Daniel Gustafsson https://vmware.com/
On Nov 16, 2021, at 6:31 AM, Daniel Gustafsson <daniel@yesql.se> wrote:
On 16 Nov 2021, at 00:58, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
While working on a fix for dangling references to dropped roles in the pg_auth_members.grantor field, I happened to notice we entirely lack regression test coverage of the REVOKE ADMIN OPTION FOR form of the RevokeRoleStmt. I am unaware of any bugs in the current implementation, but future work on roles may benefit if we close the testing gap.
LGTM. Reading this I realized that the GRANTED BY keyword for RevokeRoleStmt
isn't working as documented, it's not checking the role at all. I've sent a
diff for that with tests on the relevant thread, but I think it would be a good
to get this in too to boost coverage.
Thanks for the review!
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On 16 Nov 2021, at 15:32, Mark Dilger <mark.dilger@enterprisedb.com> wrote:
Thanks for the review!
Pushed to master along with the bugfix it helped uncover, thanks!
--
Daniel Gustafsson https://vmware.com/