maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Vicentiu!

Below are my comments on the combined patch:

> diff --git a/mysql-test/r/alter_user.result b/mysql-test/r/alter_user.result
> new file mode 100644
> index 0000000..2acc249
> --- /dev/null
> +++ b/mysql-test/r/alter_user.result
> @@ -0,0 +1,79 @@
> +select * from mysql.user where user = 'root' and host = 'localhost';
> +Host	User	Password	Select_priv	Insert_priv	Update_priv	Delete_priv	Create_priv	Drop_priv	Reload_priv	Shutdown_priv	Process_priv	File_priv	Grant_priv	References_priv	Index_priv	Alter_priv	Show_db_priv	Super_priv	Create_tmp_table_priv	Lock_tables_priv	Execute_priv	Repl_slave_priv	Repl_client_priv	Create_view_priv	Show_view_priv	Create_routine_priv	Alter_routine_priv	Create_user_priv	Event_priv	Trigger_priv	Create_tablespace_priv	ssl_type	ssl_cipher	x509_issuer	x509_subject	max_questions	max_updates	max_connections	max_user_connections	plugin	authentication_string	password_expired	is_role	default_role	max_statement_time
> +localhost	root		Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y					0	0	0	0			N	N		0.000000
> +# Test syntax
> +#
> +# These 2 selects should have no changes from the first one.
> +alter user CURRENT_USER;

I'm not sure this should be a valid statement.
After all ALTER TABLE t1; is not valid. There must be something
after the user name

> +select * from mysql.user where user = 'root' and host = 'localhost';
> +Host	User	Password	Select_priv	Insert_priv	Update_priv	Delete_priv	Create_priv	Drop_priv	Reload_priv	Shutdown_priv	Process_priv	File_priv	Grant_priv	References_priv	Index_priv	Alter_priv	Show_db_priv	Super_priv	Create_tmp_table_priv	Lock_tables_priv	Execute_priv	Repl_slave_priv	Repl_client_priv	Create_view_priv	Show_view_priv	Create_routine_priv	Alter_routine_priv	Create_user_priv	Event_priv	Trigger_priv	Create_tablespace_priv	ssl_type	ssl_cipher	x509_issuer	x509_subject	max_questions	max_updates	max_connections	max_user_connections	plugin	authentication_string	password_expired	is_role	default_role	max_statement_time
> +localhost	root		Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y					0	0	0	0			N	N		0.000000
> +alter user CURRENT_USER();
> +select * from mysql.user where user = 'root' and host = 'localhost';
> +Host	User	Password	Select_priv	Insert_priv	Update_priv	Delete_priv	Create_priv	Drop_priv	Reload_priv	Shutdown_priv	Process_priv	File_priv	Grant_priv	References_priv	Index_priv	Alter_priv	Show_db_priv	Super_priv	Create_tmp_table_priv	Lock_tables_priv	Execute_priv	Repl_slave_priv	Repl_client_priv	Create_view_priv	Show_view_priv	Create_routine_priv	Alter_routine_priv	Create_user_priv	Event_priv	Trigger_priv	Create_tablespace_priv	ssl_type	ssl_cipher	x509_issuer	x509_subject	max_questions	max_updates	max_connections	max_user_connections	plugin	authentication_string	password_expired	is_role	default_role	max_statement_time
> +localhost	root		Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y	Y					0	0	0	0			N	N		0.000000
> +create user foo;
> +select * from mysql.user where user = 'foo';
> +Host	User	Password	Select_priv	Insert_priv	Update_priv	Delete_priv	Create_priv	Drop_priv	Reload_priv	Shutdown_priv	Process_priv	File_priv	Grant_priv	References_priv	Index_priv	Alter_priv	Show_db_priv	Super_priv	Create_tmp_table_priv	Lock_tables_priv	Execute_priv	Repl_slave_priv	Repl_client_priv	Create_view_priv	Show_view_priv	Create_routine_priv	Alter_routine_priv	Create_user_priv	Event_priv	Trigger_priv	Create_tablespace_priv	ssl_type	ssl_cipher	x509_issuer	x509_subject	max_questions	max_updates	max_connections	max_user_connections	plugin	authentication_string	password_expired	is_role	default_role	max_statement_time
> +%	foo		N	N	N	N	N	N	N	N	N	N	N	N	N	N	N	N	N	N	N	N	N	N	N	N	N	N	N	N	N					0	0	0	0			N	N		0.000000
> diff --git a/mysql-test/t/alter_user.test b/mysql-test/t/alter_user.test
> new file mode 100644
> index 0000000..3a3a7d7
> --- /dev/null
> +++ b/mysql-test/t/alter_user.test
> @@ -0,0 +1,71 @@
> +--source include/not_embedded.inc
> +--enable_connect_log
> +
> +
> +select * from mysql.user where user = 'root' and host = 'localhost';
> +--echo # Test syntax
> +--echo #
> +--echo # These 2 selects should have no changes from the first one.
> +alter user CURRENT_USER;
> +select * from mysql.user where user = 'root' and host = 'localhost';
> +alter user CURRENT_USER();
> +select * from mysql.user where user = 'root' and host = 'localhost';
> +
> +create user foo;
> +select * from mysql.user where user = 'foo';
> +alter user foo;
> +select * from mysql.user where user = 'foo';
> +
> +--echo # Test super privilege works correctly with a read only database.
> +SET @start_read_only = @@global.read_only;
> +SET GLOBAL read_only=1;
> +grant create user on *.* to foo;
> +
> +--echo # Currently no super privileges.
> +connect (a, localhost, foo);
> +select @@global.read_only;
> +
> +--error ER_OPTION_PREVENTS_STATEMENT
> +alter user foo;
> +
> +--echo # Grant super privilege to the user.
> +connection default;
> +grant super on *.* to foo;
> +
> +--echo # We now have super privilege. We should be able to run alter user.
> +connect (b, localhost, foo);
> +alter user foo;
> +
> +connection default;
> +SET GLOBAL read_only = @start_read_only;
> +
> +--echo # Test inexistant user.
> +--error ER_CANNOT_USER
> +alter user boo;
> +--echo #--warning ER_CANNOT_USER
> +alter if exists user boo;
> +
> +--echo # Test SSL related altering.

eh? I don't see any SSL related altering here

> +alter user foo identified by 'something';
> +select * from mysql.user where user = 'foo';
> +
> +alter user foo identified by 'something2';
> +select * from mysql.user where user = 'foo';
> +
> +alter user foo identified by password '*88C89BE093D4ECF72D039F62EBB7477EA1FD4D63';
> +select * from mysql.user where user = 'foo';
> +
> +alter user foo identified with 'somecoolplugin';
> +select * from mysql.user where user = 'foo';
> +
> +alter user foo identified with 'somecoolplugin' using 'somecoolpassphrase';
> +select * from mysql.user where user = 'foo';
> +
> +--echo # Test resource limits altering.
> +alter user foo with MAX_QUERIES_PER_HOUR 10
> +                    MAX_UPDATES_PER_HOUR 20
> +                    MAX_CONNECTIONS_PER_HOUR 30
> +                    MAX_USER_CONNECTIONS 40;
> +select * from mysql.user where user = 'foo';
> +drop user foo;
> +--disable_connect_log
> diff --git a/sql/sql_parse.cc b/sql/sql_parse.cc
> index 96985aa..9fcd505 100644
> --- a/sql/sql_parse.cc
> +++ b/sql/sql_parse.cc
> @@ -4635,7 +4640,11 @@ mysql_execute_command(THD *thd)
>        break;
>      /* Conditionally writes to binlog */
>      WSREP_TO_ISOLATION_BEGIN(WSREP_MYSQL_DB, NULL, NULL)
> -    if (!(res= mysql_rename_user(thd, lex->users_list)))
> +    if (lex->sql_command == SQLCOM_ALTER_USER)
> +      res= mysql_alter_user(thd, lex->users_list);

I don't think ALTER USER needs same privileges as RENAME USER.
A user should be able to ALTER herself.
We have added a check_alter_user() helper exactly for that, remember? :)

> +    else
> +      res= mysql_rename_user(thd, lex->users_list);
> +    if (!res)
>        my_ok(thd);
>      break;
>    }
> diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc
> index a6c6859..1285165 100644
> --- a/sql/sql_acl.cc
> +++ b/sql/sql_acl.cc
> @@ -7710,6 +7798,78 @@ static bool print_grants_for_role(THD *thd, ACL_ROLE * role)
>  }
>  
>  
> +bool mysql_show_create_user(THD *thd, LEX_USER *lex_user)
> +{
> +  const char *username = safe_str(lex_user->user.str);
> +  const char *hostname = safe_str(lex_user->host.str);
> +  Protocol *protocol= thd->protocol;
> +  bool error= false;
> +  ACL_USER *acl_user;
> +  DBUG_ENTER("mysql_show_create_user");
> +
> +  // Check if the command specifies a username or not.
> +  if (lex_user->user.str == current_user.str)
> +  {
> +    username= thd->security_ctx->priv_user;
> +    hostname= thd->security_ctx->priv_host;
> +  }
> +
> +  char buff[1024]; //Show create user should not take more than 1024 bytes.
> +  String field_name(buff, sizeof(buff), system_charset_info);

There's a helper for that:

  StringBuffer<1024> field_name;

on the other hand... you don't use field_name anywhere.

> +  List<Item> field_list;
> +  strxmov(buff, "CREATE USER for ", username, "@", hostname, NullS);
> +  Item_string *field = new (thd->mem_root) Item_string_ascii(thd, "", 0);
> +  if (!field)
> +  {
> +    my_error(ER_OUTOFMEMORY, MYF(0));
> +    DBUG_RETURN(true);
> +  }
> +

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx