Restrict DROP USER/ROLE from non-dbo user (babelfish-for-postgresql#2859

) Earlier, any user was able to drop user/role, irrespective of whether that user has required privileges or not. With this commit, Only dbo and members of db_owner will have the permission to drop user/role. Additionally, this restricts dropping internal database principal such as dbo and db_owner, it restricts dropping non-Babelfish roles from TDS endpoint. Task: BABEL-5173 Authored-by: ANJU BHARTI <[email protected]> Co-authored-by: Harsh Lunagariya <[email protected]> Signed-off-by: Harsh Lunagariya <[email protected]>
amazon-aurora · Aug 26, 2024 · ad3268b · ad3268b
1 parent 8607687
commit ad3268b
Show file tree

Hide file tree

Showing 4 changed files with 535 additions and 2 deletions.
diff --git a/contrib/babelfishpg_tsql/src/pl_handler.c b/contrib/babelfishpg_tsql/src/pl_handler.c
@@ -3285,15 +3285,43 @@ bbf_ProcessUtility(PlannedStmt *pstmt,
 								{
 									RoleSpec   *rolspec = lfirst(item);
 									char	   *user_name;
+									const char *db_principal_type = drop_user ? "user" : "role";
+									const char *db_owner_name;
+									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) ||
+										(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_type, rolspec->rolename)));
 
+									/* 
+									 * Check for current_user's privileges 
+									 * must be database owner to drop user/role
+									 */
+									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),
+												 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.
 									 * 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.")));

diff --git 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 '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
@@ -0,0 +1,302 @@
+-- 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 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
+
+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: 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 'no_priv_user2', because it does not exist or you do not have permission.)~~
+
+
+-- 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: 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 'no_priv_user2', because it does not exist or you do not have permission.)~~
+
+
+-- 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
+
+-- error
+-- try to drop non existing role/user
+drop role fake_role
+go
+~~ERROR (Code: 33557097)~~
+
+~~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 '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
+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
+
+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
+
+-- 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
+-- 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;
+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 login no_priv_login3
+go
+
+drop database restrict_user_db1
+go