From b9462730d093ed92ae696728dae5901e93e8db83 Mon Sep 17 00:00:00 2001 From: ANJU BHARTI Date: Wed, 14 Aug 2024 16:04:49 +0000 Subject: [PATCH 1/7] Restrict DROP USER/ROLE from non-dbo user Signed-off-by: ANJU BHARTI --- contrib/babelfishpg_tsql/src/pl_handler.c | 12 ++ .../JDBC/expected/restrict_drop_user_role.out | 167 ++++++++++++++++++ test/JDBC/input/restrict_drop_user_role.mix | 121 +++++++++++++ 3 files changed, 300 insertions(+) create mode 100644 test/JDBC/expected/restrict_drop_user_role.out create mode 100644 test/JDBC/input/restrict_drop_user_role.mix diff --git a/contrib/babelfishpg_tsql/src/pl_handler.c b/contrib/babelfishpg_tsql/src/pl_handler.c index f2e2891873..04064867a8 100644 --- a/contrib/babelfishpg_tsql/src/pl_handler.c +++ b/contrib/babelfishpg_tsql/src/pl_handler.c @@ -3430,6 +3430,18 @@ bbf_ProcessUtility(PlannedStmt *pstmt, (errcode(ERRCODE_OBJECT_IN_USE), errmsg("Could not drop login '%s' as the user is currently logged in.", role_name))); } + /* If user/role, check for current_user's privileges */ + else if (drop_user || drop_role) + { + const char *db_owner_name; + + /* must be database owner to drop user/role*/ + db_owner_name = get_db_owner_name(get_cur_db_name()); + if (!has_privs_of_role(GetUserId(),get_role_oid(db_owner_name, false))) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("User does not have permission to perform this action."))); + } /* * We have performed all the permissions checks. diff --git a/test/JDBC/expected/restrict_drop_user_role.out b/test/JDBC/expected/restrict_drop_user_role.out new file mode 100644 index 0000000000..aa289078b6 --- /dev/null +++ b/test/JDBC/expected/restrict_drop_user_role.out @@ -0,0 +1,167 @@ +-- tsql +create login no_priv_login1 with password = '12345678' +go + +create role dont_drop_role +go + +create login no_priv_login2 with password = '12345678' +go + +create user no_priv_user2 for login no_priv_login2 +go + +create database restrict_user_db1 +go + +use restrict_user_db1 +go + +create role dont_drop_role +go + +create user no_priv_user1 for login no_priv_login1 +go + +use master +go + +-- tsql user=no_priv_login1 password=12345678 +-- case-1 - when current user is guest +-- permission denied +select current_user,db_name() +go +~~START~~ +varchar#!#nvarchar +guest#!#master +~~END~~ + + +drop role dont_drop_role +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: User does not have permission to perform this action.)~~ + + +drop user no_priv_user2 +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: User does not have permission to perform this action.)~~ + + +-- tsql +select current_user,db_name() +go +~~START~~ +varchar#!#nvarchar +dbo#!#master +~~END~~ + + +create user no_priv_user1 for login no_priv_login1 +go + +-- tsql user=no_priv_login1 password=12345678 +-- case-2 - when current user has no privilege +-- permission denied +use master +go + +select current_user +go +~~START~~ +varchar +no_priv_user1 +~~END~~ + + +drop role dont_drop_role +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: User does not have permission to perform this action.)~~ + + +drop user no_priv_user2 +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: User does not have permission to perform this action.)~~ + + +-- tsql + +-- case-3 - when current login has privilege +-- 3.1 - when login is database owner +Alter authorization on database::restrict_user_db1 to no_priv_login2 +go + +-- tsql user=no_priv_login2 password=12345678 database=restrict_user_db1 +-- allowed +select current_user +go +~~START~~ +varchar +dbo +~~END~~ + + +drop role dont_drop_role +go + +drop user no_priv_user1 +go + +create role dont_drop_role +go + +create user no_priv_user1 for login no_priv_login1 +go + +-- tsql +-- 3.2 - when login is sysadmin +-- allowed +drop user no_priv_user1 +go + +drop user no_priv_user2 +go + +drop role dont_drop_role +go + +-- psql +-- Need to terminate active session before cleaning up the login +SELECT pg_terminate_backend(pid) FROM pg_stat_get_activity(NULL) +WHERE sys.suser_name(usesysid) = 'no_priv_login1' AND backend_type = 'client backend' AND usesysid IS NOT NULL; +go +~~START~~ +bool +t +~~END~~ + + + +-- tsql +drop login no_priv_login1 +go + +-- psql +-- Need to terminate active session before cleaning up the login +SELECT pg_terminate_backend(pid) FROM pg_stat_get_activity(NULL) +WHERE sys.suser_name(usesysid) = 'no_priv_login2' AND backend_type = 'client backend' AND usesysid IS NOT NULL; +go +~~START~~ +bool +t +~~END~~ + + +-- tsql +drop login no_priv_login2 +go + +drop database restrict_user_db1 +go diff --git a/test/JDBC/input/restrict_drop_user_role.mix b/test/JDBC/input/restrict_drop_user_role.mix new file mode 100644 index 0000000000..194c46f64a --- /dev/null +++ b/test/JDBC/input/restrict_drop_user_role.mix @@ -0,0 +1,121 @@ +-- tsql +create login no_priv_login1 with password = '12345678' +go + +create role dont_drop_role +go + +create login no_priv_login2 with password = '12345678' +go + +create user no_priv_user2 for login no_priv_login2 +go + +create database restrict_user_db1 +go + +use restrict_user_db1 +go + +create role dont_drop_role +go + +create user no_priv_user1 for login no_priv_login1 +go + +use master +go + +-- case-1 - when current user is guest +-- permission denied +-- tsql user=no_priv_login1 password=12345678 +select current_user,db_name() +go + +drop role dont_drop_role +go + +drop user no_priv_user2 +go + +-- tsql +select current_user,db_name() +go + +create user no_priv_user1 for login no_priv_login1 +go + +-- case-2 - when current user has no privilege +-- permission denied +-- tsql user=no_priv_login1 password=12345678 +use master +go + +select current_user +go + +drop role dont_drop_role +go + +drop user no_priv_user2 +go + +-- case-3 - when current login has privilege +-- tsql + +-- 3.1 - when login is database owner +Alter authorization on database::restrict_user_db1 to no_priv_login2 +go + +-- allowed +-- tsql user=no_priv_login2 password=12345678 database=restrict_user_db1 +select current_user +go + +drop role dont_drop_role +go + +drop user no_priv_user1 +go + +create role dont_drop_role +go + +create user no_priv_user1 for login no_priv_login1 +go + +-- 3.2 - when login is sysadmin +-- allowed +-- tsql +drop user no_priv_user1 +go + +drop user no_priv_user2 +go + +drop role dont_drop_role +go + +-- psql +-- Need to terminate active session before cleaning up the login +SELECT pg_terminate_backend(pid) FROM pg_stat_get_activity(NULL) +WHERE sys.suser_name(usesysid) = 'no_priv_login1' AND backend_type = 'client backend' AND usesysid IS NOT NULL; +go + + +-- tsql +drop login no_priv_login1 +go + +-- psql +-- Need to terminate active session before cleaning up the login +SELECT pg_terminate_backend(pid) FROM pg_stat_get_activity(NULL) +WHERE sys.suser_name(usesysid) = 'no_priv_login2' AND backend_type = 'client backend' AND usesysid IS NOT NULL; +go + +-- tsql +drop login no_priv_login2 +go + +drop database restrict_user_db1 +go \ No newline at end of file From ec5d2b544db8dc4d58b266fbc190e9851e895d93 Mon Sep 17 00:00:00 2001 From: ANJU BHARTI Date: Thu, 15 Aug 2024 08:50:26 +0000 Subject: [PATCH 2/7] Restrict dropping of dbo and db_owner Signed-off-by: ANJU BHARTI --- contrib/babelfishpg_tsql/src/pl_handler.c | 41 ++++++++++++++----- .../JDBC/expected/restrict_drop_user_role.out | 41 +++++++++++++++++-- test/JDBC/input/restrict_drop_user_role.mix | 17 ++++++++ 3 files changed, 84 insertions(+), 15 deletions(-) diff --git a/contrib/babelfishpg_tsql/src/pl_handler.c b/contrib/babelfishpg_tsql/src/pl_handler.c index 04064867a8..7a694adf45 100644 --- a/contrib/babelfishpg_tsql/src/pl_handler.c +++ b/contrib/babelfishpg_tsql/src/pl_handler.c @@ -3285,9 +3285,21 @@ bbf_ProcessUtility(PlannedStmt *pstmt, { RoleSpec *rolspec = lfirst(item); char *user_name; + char *db_principal; user_name = get_physical_user_name(db_name, rolspec->rolename, false); - + role_name = rolspec->rolename; + if (drop_user) + db_principal = "user"; + else + db_principal = "role"; + + /* If user is dbo or role is db_owner, restrict dropping */ + if ((drop_user && strncmp(role_name, "dbo", 3) == 0) || (drop_role && strncmp(role_name, "db_owner", 8) == 0)) + ereport(ERROR, + (errcode(ERRCODE_CHECK_VIOLATION), + errmsg("Cannot drop the %s '%s'.", db_principal, role_name))); + /* * If a role has members, do not drop it. * Note that here we don't handle invalid @@ -3431,17 +3443,24 @@ bbf_ProcessUtility(PlannedStmt *pstmt, errmsg("Could not drop login '%s' as the user is currently logged in.", role_name))); } /* If user/role, check for current_user's privileges */ - else if (drop_user || drop_role) - { - const char *db_owner_name; + else if (drop_user || drop_role) + { + const char *db_owner_name; + char *db_principal; + int role_oid = get_role_oid(role_name, true); - /* must be database owner to drop user/role*/ - db_owner_name = get_db_owner_name(get_cur_db_name()); - if (!has_privs_of_role(GetUserId(),get_role_oid(db_owner_name, false))) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("User does not have permission to perform this action."))); - } + if (drop_user) + db_principal = "user"; + else + db_principal = "role"; + + /* must be database owner to drop user/role */ + db_owner_name = get_db_owner_name(get_cur_db_name()); + if ((!stmt->missing_ok && !OidIsValid(role_oid)) || !is_member_of_role(GetUserId(), get_role_oid(db_owner_name, false))) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("Cannot drop the %s '%s', because it does not exist or you do not have permission.", db_principal, role_name))); + } /* * We have performed all the permissions checks. diff --git a/test/JDBC/expected/restrict_drop_user_role.out b/test/JDBC/expected/restrict_drop_user_role.out index aa289078b6..83de2f760a 100644 --- a/test/JDBC/expected/restrict_drop_user_role.out +++ b/test/JDBC/expected/restrict_drop_user_role.out @@ -41,14 +41,14 @@ drop role dont_drop_role go ~~ERROR (Code: 33557097)~~ -~~ERROR (Message: User does not have permission to perform this action.)~~ +~~ERROR (Message: Cannot drop the role 'master_dont_drop_role', because it does not exist or you do not have permission.)~~ drop user no_priv_user2 go ~~ERROR (Code: 33557097)~~ -~~ERROR (Message: User does not have permission to perform this action.)~~ +~~ERROR (Message: Cannot drop the user 'master_no_priv_user2', because it does not exist or you do not have permission.)~~ -- tsql @@ -81,14 +81,14 @@ drop role dont_drop_role go ~~ERROR (Code: 33557097)~~ -~~ERROR (Message: User does not have permission to perform this action.)~~ +~~ERROR (Message: Cannot drop the role 'master_dont_drop_role', because it does not exist or you do not have permission.)~~ drop user no_priv_user2 go ~~ERROR (Code: 33557097)~~ -~~ERROR (Message: User does not have permission to perform this action.)~~ +~~ERROR (Message: Cannot drop the user 'master_no_priv_user2', because it does not exist or you do not have permission.)~~ -- tsql @@ -114,6 +114,39 @@ go drop user no_priv_user1 go +-- error +-- try to drop non existing role/user +drop role fake_role +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the role 'restrict_user_db1_fake_role', because it does not exist or you do not have permission.)~~ + + +drop user fake_user +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the user 'restrict_user_db1_fake_user', because it does not exist or you do not have permission.)~~ + + +-- should deny +-- try to drop dbo user, db_owner role +drop role db_owner +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the role 'db_owner'.)~~ + + +drop user dbo +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the user 'dbo'.)~~ + + + create role dont_drop_role go diff --git a/test/JDBC/input/restrict_drop_user_role.mix b/test/JDBC/input/restrict_drop_user_role.mix index 194c46f64a..aa03f1faa0 100644 --- a/test/JDBC/input/restrict_drop_user_role.mix +++ b/test/JDBC/input/restrict_drop_user_role.mix @@ -78,6 +78,23 @@ go drop user no_priv_user1 go +-- error +-- try to drop non existing role/user +drop role fake_role +go + +drop user fake_user +go + +-- should deny +-- try to drop dbo user, db_owner role +drop role db_owner +go + +drop user dbo +go + + create role dont_drop_role go From 149ad22d38da9d7e4b51580bcc804c5c1fafc32d Mon Sep 17 00:00:00 2001 From: ANJU BHARTI Date: Fri, 16 Aug 2024 03:24:32 +0000 Subject: [PATCH 3/7] Updated BABEL-3844-vu-cleanup expected file Signed-off-by: ANJU BHARTI --- contrib/babelfishpg_tsql/src/pl_handler.c | 34 ++++++++++---------- test/JDBC/expected/BABEL-3844-vu-cleanup.out | 2 +- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/contrib/babelfishpg_tsql/src/pl_handler.c b/contrib/babelfishpg_tsql/src/pl_handler.c index 7a694adf45..ed2023d1fc 100644 --- a/contrib/babelfishpg_tsql/src/pl_handler.c +++ b/contrib/babelfishpg_tsql/src/pl_handler.c @@ -3443,24 +3443,24 @@ bbf_ProcessUtility(PlannedStmt *pstmt, errmsg("Could not drop login '%s' as the user is currently logged in.", role_name))); } /* If user/role, check for current_user's privileges */ - else if (drop_user || drop_role) - { - const char *db_owner_name; - char *db_principal; - int role_oid = get_role_oid(role_name, true); + else if (drop_user || drop_role) + { + const char *db_owner_name; + char *db_principal; + int role_oid = get_role_oid(role_name, true); - if (drop_user) - db_principal = "user"; - else - db_principal = "role"; - - /* must be database owner to drop user/role */ - db_owner_name = get_db_owner_name(get_cur_db_name()); - if ((!stmt->missing_ok && !OidIsValid(role_oid)) || !is_member_of_role(GetUserId(), get_role_oid(db_owner_name, false))) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("Cannot drop the %s '%s', because it does not exist or you do not have permission.", db_principal, role_name))); - } + if (drop_user) + db_principal = "user"; + else + db_principal = "role"; + + /* must be database owner to drop user/role */ + db_owner_name = get_db_owner_name(get_cur_db_name()); + if ((!stmt->missing_ok && !OidIsValid(role_oid)) || !is_member_of_role(GetUserId(), get_role_oid(db_owner_name, false))) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("Cannot drop the %s '%s', because it does not exist or you do not have permission.", db_principal, role_name))); + } /* * We have performed all the permissions checks. diff --git a/test/JDBC/expected/BABEL-3844-vu-cleanup.out b/test/JDBC/expected/BABEL-3844-vu-cleanup.out index 2f51052245..556272790e 100644 --- a/test/JDBC/expected/BABEL-3844-vu-cleanup.out +++ b/test/JDBC/expected/BABEL-3844-vu-cleanup.out @@ -6,7 +6,7 @@ drop user test_user; GO ~~ERROR (Code: 33557097)~~ -~~ERROR (Message: role "master_test_user" does not exist)~~ +~~ERROR (Message: Cannot drop the user 'master_test_user', because it does not exist or you do not have permission.)~~ drop login [babel\aduser1]; From 675f72ea2a787d2b99ab5ebe685b1e9510e19fb2 Mon Sep 17 00:00:00 2001 From: ANJU BHARTI Date: Fri, 16 Aug 2024 05:42:48 +0000 Subject: [PATCH 4/7] Replaced physical rolename with logical rolename in error message Signed-off-by: ANJU BHARTI --- contrib/babelfishpg_tsql/src/pl_handler.c | 50 +++++++++---------- test/JDBC/expected/BABEL-3844-vu-cleanup.out | 2 +- .../JDBC/expected/restrict_drop_user_role.out | 12 ++--- 3 files changed, 32 insertions(+), 32 deletions(-) diff --git a/contrib/babelfishpg_tsql/src/pl_handler.c b/contrib/babelfishpg_tsql/src/pl_handler.c index ed2023d1fc..847576b3aa 100644 --- a/contrib/babelfishpg_tsql/src/pl_handler.c +++ b/contrib/babelfishpg_tsql/src/pl_handler.c @@ -3286,26 +3286,45 @@ bbf_ProcessUtility(PlannedStmt *pstmt, RoleSpec *rolspec = lfirst(item); char *user_name; char *db_principal; + const char *db_owner_name; + int role_oid; + int rolename_len; + char *logical_role_name = NULL; user_name = get_physical_user_name(db_name, rolspec->rolename, false); - role_name = rolspec->rolename; + db_owner_name = get_db_owner_name(get_cur_db_name()); + role_oid = get_role_oid(user_name, true); + logical_role_name = rolspec->rolename; + rolename_len = strlen(logical_role_name); + if (drop_user) db_principal = "user"; else db_principal = "role"; /* If user is dbo or role is db_owner, restrict dropping */ - if ((drop_user && strncmp(role_name, "dbo", 3) == 0) || (drop_role && strncmp(role_name, "db_owner", 8) == 0)) + if ((drop_user && strncmp(logical_role_name, "dbo", rolename_len) == 0) || + (drop_role && strncmp(logical_role_name, "db_owner", rolename_len) == 0)) ereport(ERROR, (errcode(ERRCODE_CHECK_VIOLATION), - errmsg("Cannot drop the %s '%s'.", db_principal, role_name))); + errmsg("Cannot drop the %s '%s'.", db_principal, logical_role_name))); + + /* + * Check for current_user's privileges + * must be database owner to drop user/role + */ + if ((!stmt->missing_ok && !OidIsValid(role_oid)) || + !is_member_of_role(GetUserId(), get_role_oid(db_owner_name, false))) + ereport(ERROR, + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), + errmsg("Cannot drop the %s '%s', because it does not exist or you do not have permission.", db_principal, logical_role_name))); /* * If a role has members, do not drop it. * Note that here we don't handle invalid * roles. */ - if (drop_role && !is_empty_role(get_role_oid(user_name, true))) + if (drop_role && !is_empty_role(role_oid)) ereport(ERROR, (errcode(ERRCODE_CHECK_VIOLATION), errmsg("The role has members. It must be empty before it can be dropped."))); @@ -3318,7 +3337,7 @@ bbf_ProcessUtility(PlannedStmt *pstmt, * if enabled. 3. Otherwise throw an * error. */ - if (drop_user && strcmp(rolspec->rolename, "guest") == 0) + if (drop_user && strcmp(logical_role_name, "guest") == 0) { if (guest_has_dbaccess(db_name)) { @@ -3327,7 +3346,7 @@ bbf_ProcessUtility(PlannedStmt *pstmt, (errcode(ERRCODE_CHECK_VIOLATION), errmsg("Cannot disable access to the guest user in master or tempdb."))); - alter_user_can_connect(false, rolspec->rolename, db_name); + alter_user_can_connect(false, logical_role_name, db_name); return; } else @@ -3442,25 +3461,6 @@ bbf_ProcessUtility(PlannedStmt *pstmt, (errcode(ERRCODE_OBJECT_IN_USE), errmsg("Could not drop login '%s' as the user is currently logged in.", role_name))); } - /* If user/role, check for current_user's privileges */ - else if (drop_user || drop_role) - { - const char *db_owner_name; - char *db_principal; - int role_oid = get_role_oid(role_name, true); - - if (drop_user) - db_principal = "user"; - else - db_principal = "role"; - - /* must be database owner to drop user/role */ - db_owner_name = get_db_owner_name(get_cur_db_name()); - if ((!stmt->missing_ok && !OidIsValid(role_oid)) || !is_member_of_role(GetUserId(), get_role_oid(db_owner_name, false))) - ereport(ERROR, - (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("Cannot drop the %s '%s', because it does not exist or you do not have permission.", db_principal, role_name))); - } /* * We have performed all the permissions checks. diff --git a/test/JDBC/expected/BABEL-3844-vu-cleanup.out b/test/JDBC/expected/BABEL-3844-vu-cleanup.out index 556272790e..9dd45e6462 100644 --- a/test/JDBC/expected/BABEL-3844-vu-cleanup.out +++ b/test/JDBC/expected/BABEL-3844-vu-cleanup.out @@ -6,7 +6,7 @@ drop user test_user; GO ~~ERROR (Code: 33557097)~~ -~~ERROR (Message: Cannot drop the user 'master_test_user', because it does not exist or you do not have permission.)~~ +~~ERROR (Message: Cannot drop the user 'test_user', because it does not exist or you do not have permission.)~~ drop login [babel\aduser1]; diff --git a/test/JDBC/expected/restrict_drop_user_role.out b/test/JDBC/expected/restrict_drop_user_role.out index 83de2f760a..54dab41cf7 100644 --- a/test/JDBC/expected/restrict_drop_user_role.out +++ b/test/JDBC/expected/restrict_drop_user_role.out @@ -41,14 +41,14 @@ drop role dont_drop_role go ~~ERROR (Code: 33557097)~~ -~~ERROR (Message: Cannot drop the role 'master_dont_drop_role', because it does not exist or you do not have permission.)~~ +~~ERROR (Message: Cannot drop the role 'dont_drop_role', because it does not exist or you do not have permission.)~~ drop user no_priv_user2 go ~~ERROR (Code: 33557097)~~ -~~ERROR (Message: Cannot drop the user 'master_no_priv_user2', because it does not exist or you do not have permission.)~~ +~~ERROR (Message: Cannot drop the user 'no_priv_user2', because it does not exist or you do not have permission.)~~ -- tsql @@ -81,14 +81,14 @@ drop role dont_drop_role go ~~ERROR (Code: 33557097)~~ -~~ERROR (Message: Cannot drop the role 'master_dont_drop_role', because it does not exist or you do not have permission.)~~ +~~ERROR (Message: Cannot drop the role 'dont_drop_role', because it does not exist or you do not have permission.)~~ drop user no_priv_user2 go ~~ERROR (Code: 33557097)~~ -~~ERROR (Message: Cannot drop the user 'master_no_priv_user2', because it does not exist or you do not have permission.)~~ +~~ERROR (Message: Cannot drop the user 'no_priv_user2', because it does not exist or you do not have permission.)~~ -- tsql @@ -120,14 +120,14 @@ drop role fake_role go ~~ERROR (Code: 33557097)~~ -~~ERROR (Message: Cannot drop the role 'restrict_user_db1_fake_role', because it does not exist or you do not have permission.)~~ +~~ERROR (Message: Cannot drop the role 'fake_role', because it does not exist or you do not have permission.)~~ drop user fake_user go ~~ERROR (Code: 33557097)~~ -~~ERROR (Message: Cannot drop the user 'restrict_user_db1_fake_user', because it does not exist or you do not have permission.)~~ +~~ERROR (Message: Cannot drop the user 'fake_user', because it does not exist or you do not have permission.)~~ -- should deny From 4330cddead01f7f5e0adbb093c157cc324721af1 Mon Sep 17 00:00:00 2001 From: ANJU BHARTI Date: Fri, 16 Aug 2024 08:37:18 +0000 Subject: [PATCH 5/7] Added more testcases Signed-off-by: ANJU BHARTI --- contrib/babelfishpg_tsql/src/pl_handler.c | 20 ++++++------- .../JDBC/expected/restrict_drop_user_role.out | 28 +++++++++++++++++++ test/JDBC/input/restrict_drop_user_role.mix | 12 ++++++++ 3 files changed, 49 insertions(+), 11 deletions(-) diff --git a/contrib/babelfishpg_tsql/src/pl_handler.c b/contrib/babelfishpg_tsql/src/pl_handler.c index 847576b3aa..36283898ae 100644 --- a/contrib/babelfishpg_tsql/src/pl_handler.c +++ b/contrib/babelfishpg_tsql/src/pl_handler.c @@ -3287,15 +3287,13 @@ bbf_ProcessUtility(PlannedStmt *pstmt, char *user_name; char *db_principal; const char *db_owner_name; - int role_oid; - int rolename_len; - char *logical_role_name = NULL; + int role_oid; + int rolename_len; user_name = get_physical_user_name(db_name, rolspec->rolename, false); db_owner_name = get_db_owner_name(get_cur_db_name()); role_oid = get_role_oid(user_name, true); - logical_role_name = rolspec->rolename; - rolename_len = strlen(logical_role_name); + rolename_len = strlen(rolspec->rolename); if (drop_user) db_principal = "user"; @@ -3303,11 +3301,11 @@ bbf_ProcessUtility(PlannedStmt *pstmt, db_principal = "role"; /* If user is dbo or role is db_owner, restrict dropping */ - if ((drop_user && strncmp(logical_role_name, "dbo", rolename_len) == 0) || - (drop_role && strncmp(logical_role_name, "db_owner", rolename_len) == 0)) + if ((drop_user && rolename_len == 3 && strncmp(rolspec->rolename, "dbo", 3) == 0) || + (drop_role && rolename_len == 8 && strncmp(rolspec->rolename, "db_owner", 8) == 0)) ereport(ERROR, (errcode(ERRCODE_CHECK_VIOLATION), - errmsg("Cannot drop the %s '%s'.", db_principal, logical_role_name))); + errmsg("Cannot drop the %s '%s'.", db_principal, rolspec->rolename))); /* * Check for current_user's privileges @@ -3317,7 +3315,7 @@ bbf_ProcessUtility(PlannedStmt *pstmt, !is_member_of_role(GetUserId(), get_role_oid(db_owner_name, false))) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("Cannot drop the %s '%s', because it does not exist or you do not have permission.", db_principal, logical_role_name))); + errmsg("Cannot drop the %s '%s', because it does not exist or you do not have permission.", db_principal, rolspec->rolename))); /* * If a role has members, do not drop it. @@ -3337,7 +3335,7 @@ bbf_ProcessUtility(PlannedStmt *pstmt, * if enabled. 3. Otherwise throw an * error. */ - if (drop_user && strcmp(logical_role_name, "guest") == 0) + if (drop_user && strcmp(rolspec->rolename, "guest") == 0) { if (guest_has_dbaccess(db_name)) { @@ -3346,7 +3344,7 @@ bbf_ProcessUtility(PlannedStmt *pstmt, (errcode(ERRCODE_CHECK_VIOLATION), errmsg("Cannot disable access to the guest user in master or tempdb."))); - alter_user_can_connect(false, logical_role_name, db_name); + alter_user_can_connect(false, rolspec->rolename, db_name); return; } else diff --git a/test/JDBC/expected/restrict_drop_user_role.out b/test/JDBC/expected/restrict_drop_user_role.out index 54dab41cf7..12cc6ea3d0 100644 --- a/test/JDBC/expected/restrict_drop_user_role.out +++ b/test/JDBC/expected/restrict_drop_user_role.out @@ -130,6 +130,34 @@ go ~~ERROR (Message: Cannot drop the user 'fake_user', because it does not exist or you do not have permission.)~~ +drop user db +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the user 'db', because it does not exist or you do not have permission.)~~ + + +drop role db_own +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the role 'db_own', because it does not exist or you do not have permission.)~~ + + +drop user dbo_u1 +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the user 'dbo_u1', because it does not exist or you do not have permission.)~~ + + +drop role db_owner_r1 +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the role 'db_owner_r1', because it does not exist or you do not have permission.)~~ + + -- should deny -- try to drop dbo user, db_owner role drop role db_owner diff --git a/test/JDBC/input/restrict_drop_user_role.mix b/test/JDBC/input/restrict_drop_user_role.mix index aa03f1faa0..83af81fa0e 100644 --- a/test/JDBC/input/restrict_drop_user_role.mix +++ b/test/JDBC/input/restrict_drop_user_role.mix @@ -86,6 +86,18 @@ go drop user fake_user go +drop user db +go + +drop role db_own +go + +drop user dbo_u1 +go + +drop role db_owner_r1 +go + -- should deny -- try to drop dbo user, db_owner role drop role db_owner From 64538e6f45f557994e1fdfe91ab3ca8a313f4df9 Mon Sep 17 00:00:00 2001 From: Harsh Lunagariya Date: Mon, 19 Aug 2024 09:23:34 +0000 Subject: [PATCH 6/7] Address comments Signed-off-by: Harsh Lunagariya --- contrib/babelfishpg_tsql/src/pl_handler.c | 13 ++----- .../JDBC/expected/restrict_drop_user_role.out | 38 ++++++++++++++++++- test/JDBC/input/restrict_drop_user_role.mix | 33 +++++++++++++++- 3 files changed, 73 insertions(+), 11 deletions(-) diff --git a/contrib/babelfishpg_tsql/src/pl_handler.c b/contrib/babelfishpg_tsql/src/pl_handler.c index 36283898ae..9e5fe2a449 100644 --- a/contrib/babelfishpg_tsql/src/pl_handler.c +++ b/contrib/babelfishpg_tsql/src/pl_handler.c @@ -3285,27 +3285,22 @@ bbf_ProcessUtility(PlannedStmt *pstmt, { RoleSpec *rolspec = lfirst(item); char *user_name; - char *db_principal; + const char *db_principal_type = drop_user ? "user" : "role"; const char *db_owner_name; int role_oid; int rolename_len; user_name = get_physical_user_name(db_name, rolspec->rolename, false); - db_owner_name = get_db_owner_name(get_cur_db_name()); + db_owner_name = get_db_owner_name(db_name); role_oid = get_role_oid(user_name, true); rolename_len = strlen(rolspec->rolename); - if (drop_user) - db_principal = "user"; - else - db_principal = "role"; - /* If user is dbo or role is db_owner, restrict dropping */ if ((drop_user && rolename_len == 3 && strncmp(rolspec->rolename, "dbo", 3) == 0) || (drop_role && rolename_len == 8 && strncmp(rolspec->rolename, "db_owner", 8) == 0)) ereport(ERROR, (errcode(ERRCODE_CHECK_VIOLATION), - errmsg("Cannot drop the %s '%s'.", db_principal, rolspec->rolename))); + errmsg("Cannot drop the %s '%s'.", db_principal_type, rolspec->rolename))); /* * Check for current_user's privileges @@ -3315,7 +3310,7 @@ bbf_ProcessUtility(PlannedStmt *pstmt, !is_member_of_role(GetUserId(), get_role_oid(db_owner_name, false))) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), - errmsg("Cannot drop the %s '%s', because it does not exist or you do not have permission.", db_principal, rolspec->rolename))); + errmsg("Cannot drop the %s '%s', because it does not exist or you do not have permission.", db_principal_type, rolspec->rolename))); /* * If a role has members, do not drop it. diff --git a/test/JDBC/expected/restrict_drop_user_role.out b/test/JDBC/expected/restrict_drop_user_role.out index 12cc6ea3d0..5dc7538987 100644 --- a/test/JDBC/expected/restrict_drop_user_role.out +++ b/test/JDBC/expected/restrict_drop_user_role.out @@ -11,6 +11,15 @@ go create user no_priv_user2 for login no_priv_login2 go +create login no_priv_login3 with password = '12345678' +go + +create user no_priv_user3 for login no_priv_login3 +go + +create role no_priv_role3; +go + create database restrict_user_db1 go @@ -123,7 +132,7 @@ go ~~ERROR (Message: Cannot drop the role 'fake_role', because it does not exist or you do not have permission.)~~ -drop user fake_user +drop user fake_user; go ~~ERROR (Code: 33557097)~~ @@ -193,6 +202,30 @@ go drop role dont_drop_role go +-- 3.3 - when drop user/role has IF EXISTS clause +-- Should not throw error for the case where IF EXISTS clause is included +drop role if exists fake_role; +go + +drop user if exists fake_user; +go + +-- both of the following statements should be executed successfully since user/role exists +drop user if exists no_priv_user3; +go + +drop user if exists no_priv_role3; +go + +select count(*) from sys.database_principals where name like '%no_priv_%3'; +go +~~START~~ +int +0 +~~END~~ + + + -- psql -- Need to terminate active session before cleaning up the login SELECT pg_terminate_backend(pid) FROM pg_stat_get_activity(NULL) @@ -224,5 +257,8 @@ t drop login no_priv_login2 go +drop login no_priv_login3 +go + drop database restrict_user_db1 go diff --git a/test/JDBC/input/restrict_drop_user_role.mix b/test/JDBC/input/restrict_drop_user_role.mix index 83af81fa0e..b852723edf 100644 --- a/test/JDBC/input/restrict_drop_user_role.mix +++ b/test/JDBC/input/restrict_drop_user_role.mix @@ -11,6 +11,15 @@ go create user no_priv_user2 for login no_priv_login2 go +create login no_priv_login3 with password = '12345678' +go + +create user no_priv_user3 for login no_priv_login3 +go + +create role no_priv_role3; +go + create database restrict_user_db1 go @@ -83,7 +92,7 @@ go drop role fake_role go -drop user fake_user +drop user fake_user; go drop user db @@ -125,6 +134,25 @@ go drop role dont_drop_role go +-- 3.3 - when drop user/role has IF EXISTS clause +-- Should not throw error for the case where IF EXISTS clause is included +drop role if exists fake_role; +go + +drop user if exists fake_user; +go + +-- both of the following statements should be executed successfully since user/role exists +drop user if exists no_priv_user3; +go + +drop user if exists no_priv_role3; +go + +select count(*) from sys.database_principals where name like '%no_priv_%3'; +go + + -- psql -- Need to terminate active session before cleaning up the login SELECT pg_terminate_backend(pid) FROM pg_stat_get_activity(NULL) @@ -146,5 +174,8 @@ go drop login no_priv_login2 go +drop login no_priv_login3 +go + drop database restrict_user_db1 go \ No newline at end of file From 918f0872538748259b50df90e6dc9352d4066ab7 Mon Sep 17 00:00:00 2001 From: Harsh Lunagariya Date: Mon, 19 Aug 2024 12:20:23 +0000 Subject: [PATCH 7/7] Address more comments Signed-off-by: Harsh Lunagariya --- contrib/babelfishpg_tsql/src/pl_handler.c | 10 +++-- .../JDBC/expected/restrict_drop_user_role.out | 38 +++++++++++++++++++ test/JDBC/input/restrict_drop_user_role.mix | 22 +++++++++++ 3 files changed, 67 insertions(+), 3 deletions(-) diff --git a/contrib/babelfishpg_tsql/src/pl_handler.c b/contrib/babelfishpg_tsql/src/pl_handler.c index 9e5fe2a449..9e6988d2e6 100644 --- a/contrib/babelfishpg_tsql/src/pl_handler.c +++ b/contrib/babelfishpg_tsql/src/pl_handler.c @@ -3287,13 +3287,17 @@ bbf_ProcessUtility(PlannedStmt *pstmt, char *user_name; const char *db_principal_type = drop_user ? "user" : "role"; const char *db_owner_name; - int role_oid; - int rolename_len; + int role_oid; + int rolename_len; + bool is_tsql_db_principal = false; user_name = get_physical_user_name(db_name, rolspec->rolename, false); db_owner_name = get_db_owner_name(db_name); role_oid = get_role_oid(user_name, true); rolename_len = strlen(rolspec->rolename); + is_tsql_db_principal = OidIsValid(role_oid) && + ((drop_user && is_user(role_oid)) || + (drop_role && is_role(role_oid))); /* If user is dbo or role is db_owner, restrict dropping */ if ((drop_user && rolename_len == 3 && strncmp(rolspec->rolename, "dbo", 3) == 0) || @@ -3306,7 +3310,7 @@ bbf_ProcessUtility(PlannedStmt *pstmt, * Check for current_user's privileges * must be database owner to drop user/role */ - if ((!stmt->missing_ok && !OidIsValid(role_oid)) || + if ((!stmt->missing_ok && !is_tsql_db_principal) || !is_member_of_role(GetUserId(), get_role_oid(db_owner_name, false))) ereport(ERROR, (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), diff --git a/test/JDBC/expected/restrict_drop_user_role.out b/test/JDBC/expected/restrict_drop_user_role.out index 5dc7538987..fa3bcc0426 100644 --- a/test/JDBC/expected/restrict_drop_user_role.out +++ b/test/JDBC/expected/restrict_drop_user_role.out @@ -225,8 +225,46 @@ int ~~END~~ +-- psql +-- 3.4 - Try dropping role created from PG role +create role master_pguser1; +go + +-- tsql +drop user pguser1; +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the user 'pguser1', because it does not exist or you do not have permission.)~~ + + +drop role pguser1; +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: Cannot drop the role 'pguser1', because it does not exist or you do not have permission.)~~ + + +-- it will still try to drop it, but permission denied will be generated since +-- bbf_role_admin won't have privilege to drop non-babelfish roles +drop user if exists pguser1; +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: permission denied to drop role)~~ + + +drop role if exists pguser1; +go +~~ERROR (Code: 33557097)~~ + +~~ERROR (Message: permission denied to drop role)~~ + -- psql +drop role master_pguser1; +go + -- Need to terminate active session before cleaning up the login SELECT pg_terminate_backend(pid) FROM pg_stat_get_activity(NULL) WHERE sys.suser_name(usesysid) = 'no_priv_login1' AND backend_type = 'client backend' AND usesysid IS NOT NULL; diff --git a/test/JDBC/input/restrict_drop_user_role.mix b/test/JDBC/input/restrict_drop_user_role.mix index b852723edf..6bac815143 100644 --- a/test/JDBC/input/restrict_drop_user_role.mix +++ b/test/JDBC/input/restrict_drop_user_role.mix @@ -152,8 +152,30 @@ go select count(*) from sys.database_principals where name like '%no_priv_%3'; go +-- 3.4 - Try dropping role created from PG role +-- psql +create role master_pguser1; +go + +-- tsql +drop user pguser1; +go + +drop role pguser1; +go + +-- it will still try to drop it, but permission denied will be generated since +-- bbf_role_admin won't have privilege to drop non-babelfish roles +drop user if exists pguser1; +go + +drop role if exists pguser1; +go -- psql +drop role master_pguser1; +go + -- Need to terminate active session before cleaning up the login SELECT pg_terminate_backend(pid) FROM pg_stat_get_activity(NULL) WHERE sys.suser_name(usesysid) = 'no_priv_login1' AND backend_type = 'client backend' AND usesysid IS NOT NULL;