Hi Kristofer,
the patch is Ok to push with some minor notes (as usual :)) -- see below.
On Friday 07 December 2007 14:40, kpettersson@stripped wrote:
> ChangeSet@stripped, 2007-12-07 12:40:08+01:00, thek@adventure.(none) +3 -0
> Bug #27440 read_only allows create and drop database
>
> When read_only option was enabled, a user without SUPER privilege could
> perform CREATE DATABASE and DROP DATABASE operations.
So, why did that happen? What was the technical cause of the user-visible
problem? Could you please clarify.
> diff -Nrup a/mysql-test/t/read_only.test b/mysql-test/t/read_only.test
> --- a/mysql-test/t/read_only.test 2006-11-20 15:35:19 +01:00
> +++ b/mysql-test/t/read_only.test 2007-12-07 12:40:06 +01:00
> @@ -117,4 +117,38 @@ connection default;
> drop table t1,t2;
> drop user test@localhost;
>
> +--echo #
> +--echo # Bug #27440 read_only allows create and drop database
> +--echo #
> +--disable_warnings
> +drop database if exists mysqltest_db1;
> +drop database if exists mysqltest_db2;
> +--enable_warnings
> +
> +delete from mysql.user where User like 'mysqltest_%';
> +delete from mysql.db where User like 'mysqltest_%';
> +delete from mysql.tables_priv where User like 'mysqltest_%';
> +delete from mysql.columns_priv where User like 'mysqltest_%';
> +flush privileges;
> +
> +grant all on mysqltest_db2.* to `mysqltest_u1`@`%`;
> +create database mysqltest_db1;
> +grant all on mysqltest_db1.* to `mysqltest_u1`@`%`;
> +flush privileges;
> +connect (con_bug27440,127.0.0.1,mysqltest_u1,,test,$MASTER_MYPORT,);
> +connection con_bug27440;
> +show grants for current_user();
> +--error ER_OPTION_PREVENTS_STATEMENT
> +create database mysqltest_db2;
> +show databases like '%mysqltest_db2%';
> +--error ER_OPTION_PREVENTS_STATEMENT
> +drop database mysqltest_db1;
> +disconnect con_bug27440;
> +connection default;
> +delete from mysql.user where User like 'mysqltest_%';
> +delete from mysql.db where User like 'mysqltest_%';
> +delete from mysql.tables_priv where User like 'mysqltest_%';
> +delete from mysql.columns_priv where User like 'mysqltest_%';
> +flush privileges;
> +drop database mysqltest_db1;
> set global read_only=0;
Just wondering. You're using '--' prefix for directives like "echo" and
"error" and not using it for "connect", "disconnect" and "connection".
Just wondering why? :) I'm not forcing anything here.
> + const my_bool user_is_super=
> + ((long)(thd->security_ctx->master_access & SUPER_ACL) ==
> + (long)SUPER_ACL);
> +
> + if (user_is_super)
> + DBUG_RETURN(FALSE);
Well... The original code contained just
"(thd->security_ctx->master_access & SUPER_ACL)" check.
I wonder why do you need "user_is_super" and all these casts here?
Again, Security_ctx::master_access is ulong; SUPER_ACL is long --
it seems, casts are not needed here.
Why not just:
if (thd->security_ctx->master_access & SUPER_ACL)
DBUG_RETURN(FALSE);
> + const my_bool create_temp_tables=
> + (lex->sql_command == SQLCOM_CREATE_TABLE) &&
> + (lex->create_info.options & HA_LEX_CREATE_TMP_TABLE);
> +
> + const my_bool drop_temp_tables=
> + (lex->sql_command == SQLCOM_DROP_TABLE) &&
> + lex->drop_temporary;
> +
> + const my_bool update_real_tables=
> + some_non_temp_table_to_be_updated(thd, all_tables) &&
> + !(create_temp_tables || drop_temp_tables);
> +
> + const my_bool update_temp_tables=
> + !update_real_tables;
> +
> + const my_bool create_or_drop_databases=
> + (lex->sql_command == SQLCOM_CREATE_DB) ||
> + (lex->sql_command == SQLCOM_DROP_DB);
> +
> + if (update_real_tables ||
> + (update_temp_tables && create_or_drop_databases))
1. I don't get why do you need "update_temp_tables" flag here?
It seems, this expression is like:
x || !x && y
which is equialent to
x || y
so, why not just:
if (update_real_tables || create_or_drop_databases)
?
2. Generally (and according to the coding style), the following code
if (x)
return true;
return false;
should be written as
return x;
So, I would write just
return update_real_tables || create_or_drop_databases;
but if you do believe the comments are useful, I think, you can leave
things as is (i.e. keep the "if").
That's it, thanks for working on this!
--
Alexander Nozdrin, Software Developer
MySQL AB, Moscow, Russia, www.mysql.com
