Restrict DROP USER/ROLE from non-dbo user (#2859) (#2864) (#2881)

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]>
babelfish-for-postgresql · Aug 26, 2024 · 253cd4d · 253cd4d
1 parent a779f7e
commit 253cd4d
Show file tree

Hide file tree

Showing 4 changed files with 728 additions and 2 deletions.
diff --git a/contrib/babelfishpg_tsql/src/pl_handler.c b/contrib/babelfishpg_tsql/src/pl_handler.c
@@ -3214,15 +3214,49 @@ 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;
+									bool		is_psql_db_principal = false;
+									Oid			dbowner;
 
 									user_name = get_physical_user_name(db_name, rolspec->rolename);
+
+									db_owner_name = get_db_owner_name(db_name);
+									dbowner = get_role_oid(db_owner_name, false);
+									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)));
+									is_psql_db_principal = OidIsValid(role_oid) && !is_tsql_db_principal;
+
+									/* 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(), dbowner) ||
+										(is_tsql_db_principal && !is_member_of_role(dbowner, role_oid)) || is_psql_db_principal)
+										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];