Hi, Dmitri, On Jun 06, Dmitri Shulga wrote:

commit b69bd351c1f Author: Dmitri Shulga <dmitry.shulga@mariadb.com> Date: Wed May 28 00:05:05 2025 +0700

MDEV-35617: DROP USER should leave no active session for that user

On handling the DROP USER statement it is counted a number of sessions established on behalf every user being dropped. In case the DROP USER statement is executed in sql_mode = oracle the error ER_CANNOT_USER if there are active connections for any of the users listed at the DROP USER statement. For sql_mode != oracle the warning ER_ACTIVE_CONNECTIONS_FOR_USER_TO_DROP if there are active connections.

This is difficult to read. Better: DROP USER looks for sessions by the do-be-dropped user and if found: * fails with ER_CANNOT_USER in Oracle mode * continues with ER_ACTIVE_CONNECTIONS_FOR_USER_TO_DROP warning otherwise New connections for that user are rejected with ER_CONNECT_WHILE_DROP_USER_IN_PROGRESS

Every user being dropped is marked with flag that disallow establishing a new connections on behalf this user. In case the user tries to establish a new session while the DROP USER is executed for this account, connection establishing is failed with the error ER_CONNECT_WHILE_DROP_USER_IN_PROGRESS

diff --git a/mysql-test/main/create_drop_user.result b/mysql-test/main/create_drop_user.result index 67717f3e4e0..4f46481dfc7 100644 --- a/mysql-test/main/create_drop_user.result +++ b/mysql-test/main/create_drop_user.result @@ -41,3 +41,57 @@ Warnings: Note 1974 Can't drop user 'u1'@'%'; it doesn't exist DROP USER u2; ERROR HY000: Operation DROP USER failed for 'u2'@'%' +# +# MDEV-35617: DROP USER should leave no active session for that user +# +CREATE USER u1; +CREATE USER u2; +CREATE USER u3; +GRANT ALL on test.* to u1; +GRANT ALL on test.* to u2; +GRANT ALL on test.* to u3; +# Establish two connections on behalf the users u1, u3 +# A connection on behalf the user u2 isn't etablished intentionally

"established"

+connect con1, localhost, u1, , test; +connect con3, localhost, u3, , test; +# Drop the users u1, u2, u3. Since the users u1 and u3 have active +# connections to the server, the warning about it be output

"will be"

+connection default; +DROP USER u1, u2, u3; +Warnings: +Note 4226 Dropped users ['u1'@'%','u3'@'%'] have active connections. Use KILL CONNECTION if they should not be used anymore.

I don't think we have any other error message that puts a list in square brackets. So let's not do it here either. See e.g. ER_CANNOT_USER

+# None of the users u1, u2, u3 should be presence in the system

"be present"

+SELECT user, host FROM mysql.user WHERE user IN ('u1', 'u2', 'u3'); +User Host +disconnect con1; +disconnect con3; +# Check behaviour of the DROP USER statement in +# oracle compatibility mode +SET @save_sql_mode = @@sql_mode; +SET sql_mode="oracle"; +CREATE USER u1; +CREATE USER u2; +CREATE USER u3; +GRANT ALL on test.* to u1; +GRANT ALL on test.* to u2; +GRANT ALL on test.* to u3; +# Established two connections on behalf the users u1, u3; +# A connection on behalf the user u2 isn't etablished intentionally

"established"

+connect con1, localhost, u1, , test; +connect con3, localhost, u3, , test; +connection default; +# In oracle compatibility mode, DROP USER fails in case +# there are connections on behalf the users being dropped. +DROP USER u1, u2, u3; +ERROR HY000: Operation DROP USER failed for 'u1'@'%','u3'@'%'

See? No square brackets :)

+# It is expected to see all three users in output of the query

I don't think it's a good idea. DROP USER drops everything it can and reports an error for everything it couldn't drop. It's not all-or-nothing behavior. Same for all other statements that can fail with ER_CANNOT_USER. Same for DROP TABLES. All-or-nothing here is rather unexpected and inconsisent with pretty much every other multi-object command in the server.

+SELECT user, host FROM mysql.user WHERE user IN ('u1', 'u2', 'u3'); +User Host +u1 % +u2 % +u3 % +SET sql_mode= @save_sql_mode; +disconnect con1; +disconnect con3; +# Clean up +DROP USER u1, u2, u3; diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index 26905f0d415..b519eec15c8 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -210,6 +210,12 @@ class ACL_USER_PARAM

class ACL_USER :public ACL_USER_BASE, public ACL_USER_PARAM { + /* + Ephemeral state (meaning it is not stored anywhere in the Data Dictionary) + to disable establishing sessions in case the user is being dropped. + */ + bool dont_accept_conn= false;

should be in ACL_USER_PARAM, I suppose, for consistency. password_errors is there, and it's ephemeral too.

+ public: ACL_USER() = default; ACL_USER(THD *, const LEX_USER &, const Account_options &, const privilege_t); @@ -243,6 +249,7 @@ class ACL_USER :public ACL_USER_BASE, public ACL_USER_PARAM dst->host.hostname= safe_strdup_root(root, host.hostname); dst->default_rolename= safe_lexcstrdup_root(root, default_rolename); bzero(&dst->role_grants, sizeof(role_grants)); + dst->dont_accept_conn= dont_accept_conn;

not needed, there's `*dst= *this` ~20 lines above.

return dst; }

@@ -11337,6 +11354,85 @@ bool mysql_create_user(THD *thd, List <LEX_USER> &list, bool handle_as_role) DBUG_RETURN(result); }

+ +struct count_user_threads_callback_arg +{ + explicit count_user_threads_callback_arg(LEX_USER *user_arg) + : user(user_arg), counter(0) + {} + + LEX_USER *user; + uint counter; +}; + + +/** + Callback function invoked for every active THD to count a number of + sessions established by specified user + + @param[in] thd Thread context + @param arg Account info for that a number of active connections + should be countered +*/ + +static my_bool count_threads_callback(THD *thd, + count_user_threads_callback_arg *arg) +{ + if (thd->security_ctx->user) + { + /* + Check that hostname (if given) and user name matches. + + host.str[0] == '%' means that host name was not given. See sql_yacc.yy + */ + if (((arg->user->host.str[0] == '%' && !arg->user->host.str[1]) || + !strcmp(thd->security_ctx->host_or_ip, arg->user->host.str)) && + !strcmp(thd->security_ctx->user, arg->user->user.str))

eh, no. Don't invent new username matching rules. In particular, incorrect ones :) This session was already matched, so do if (!strcmp(arg->user->host.str, thd->main_security_ctx.priv_host) && !strcmp(arg->user->user.str, thd->main_security_ctx.priv_user)) And let's add a test, like create user u@localhost; create user u@'%'; connect u,localhost,u; connection default; drop user u@'%'; and there should be neither warning nor an error because of the existing connection by u@localhost.

+ arg->counter++; + } + return false; +} + + +/** + Get a number of connections currently established on behalf the user + + @param[in] thd Thread context + @param[in] user User credential to count up a number of connections + + @return a number of connections established by the user at the moment of + the function call +*/ + +static int count_sessions_for_user(LEX_USER *user) +{ + count_user_threads_callback_arg arg(user); + bool ret __attribute__((unused)); + + ret= server_threads.iterate(count_threads_callback, &arg);

iterate() takes a lock. You want to release it as soon as possible. And you don't really use the value of a counter, so why bother? Abort the search and return on the first match.

+ DBUG_ASSERT(ret == false); + + return arg.counter; +} + + +/** + Find the specified user and mark it as not accepting incoming sessions + + @param user_name the user for that accept of incoming connections + should be disabled +*/ + +static void disable_connections_for_user(LEX_USER *user) +{ + ACL_USER *found_user= find_user_or_anon(user->host.str, user->user.str, + nullptr);

No, find_user_exact(). And let's add this test case: create user u1@'%'; --error ER_CANNOT_USER drop user u1@localhost; connect u1,localhost,u1; select user(), current_user();

+ + if (found_user != nullptr) + found_user->disable_new_connections(); +} + + /* Drop a list of users and all their privileges.

@@ -11395,6 +11496,39 @@ bool mysql_drop_user(THD *thd, List <LEX_USER> &list, bool handle_as_role) continue; }

+ if (count_sessions_for_user(user_name) != 0) + append_user(thd, &connected_users, user_name); + + correct_users_list.push_back(user_name); + /* + Prevent new connections to be established on behalf the user + being dropped. + */ + disable_connections_for_user(user_name); + } + + if (!connected_users.is_empty() && + (thd->variables.sql_mode & MODE_ORACLE)) + { + /* + Throw error in case there are connections for the user being dropped AND + sql_mode = oracle + */ + mysql_mutex_unlock(&acl_cache->lock); + mysql_rwlock_unlock(&LOCK_grant); + + my_error(ER_CANNOT_USER, MYF(0), + (handle_as_role) ? "DROP ROLE" : "DROP USER",

There can be no DROP ROLE here, everything you're doing above shouldn't even be done for DROP ROLE, check handle_as_role to avoid unnecessary locks.

+ connected_users.c_ptr_safe()); + + DBUG_RETURN(true);

ok, another test case to add: create user u@localhost; set sql_mode=oracle; connect u,localhost,u; connection default; --error ER_CANNOT_USER drop user u@'%'; connect u,localhost,u; select user(), current_user();

+ } + + user_list.init(correct_users_list);

you don't need correct_users_list, because there shouldn't be inconsistent all-or-nothing behavior

+ while ((user_name= user_list++)) + { + int rc; + if ((rc= handle_grant_data(thd, tables, 1, user_name, NULL)) > 0) { // The user or role was successfully deleted @@ -15127,6 +15267,18 @@ bool acl_authenticate(THD *thd, uint com_change_user_pkt_len) } #endif

+ /* + Attempt to establish a new connection on behalf the user that is + currently being dropped from a concurrent session. + Terminate the connection. + */ + if (acl_user->dont_accept_new_connections()) + { + my_error(ER_CONNECT_WHILE_DROP_USER_IN_PROGRESS, MYF(0), + acl_user->get_username());

I'd just say user not found. That is, no need to a special error, but find_mpvio_user() can simply skip entries that are being dropped. Also, how can a connection see a user being dropped, if mysql_drop_user() takes acl_cache->lock and doesn't release it until everything is dropped?

+ DBUG_RETURN(1); + } + if (set_privs_on_login(thd, acl_user)) DBUG_RETURN(1); }

Regards, Sergei Chief Architect, MariaDB Server and security@mariadb.org