Hi, Dmitri,
On Jun 06, Dmitri Shulga wrote:
> commit b69bd351c1f
> Author: Dmitri Shulga <dmitry.shulga(a)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(a)mariadb.org
