Hello, Sergei
2008/7/31 Sergei Golubchik <serg@stripped>:
> Hi.
>
> Please find below the review of your code as of the revision.
>
> ------------------------------------------------------------
> revno: 2678
> revision-id: dotsent@dot-sent-20080725202734-y3im7gyxcdowqab6
> parent: dotsent@dot-sent-20080721211334-o0pku001un2vrcjo
> committer: dot_sent <dotsent@dot-sent>
> branch nick: 6.0
> timestamp: Fri 2008-07-25 23:27:34 +0300
> message:
> Fixed server crash while GRANTing role TO user, fixed some bugs in loading
> mysql.roles table.
> ------------------------------------------------------------
>
> first - don't use tabs in the source. especially not with a non-default
> width of 2. Please fix the indentation in all your code to use spaces.
> (and I'd suggest to configure your editor to use tab width of 8 and
> <Tab> key to insert spaces, not tabs. At least for MySQL code)
>
Accepted. Will sed my code to eliminate tabs and then check code
for identation problems.
>> === modified file 'scripts/mysql_system_tables.sql'
>> --- scripts/mysql_system_tables.sql 2008-02-27 10:04:05 +0000
>> +++ scripts/mysql_system_tables.sql 2008-07-21 21:13:34 +0000
>> @@ -13,11 +13,19 @@ set @had_db_table= @@warning_count != 0;
>> CREATE TABLE IF NOT EXISTS host ( Host char(60) binary DEFAULT '' NOT NULL, Db
> char(64) binary DEFAULT '' NOT NULL, Select_priv enum('N','Y') COLLATE utf8_general_ci
> DEFAULT 'N' NOT NULL, Insert_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT
> NULL, Update_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Delete_priv
> enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Create_priv enum('N','Y')
> COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Drop_priv enum('N','Y') COLLATE
> utf8_general_ci DEFAULT 'N' NOT NULL, Grant_priv enum('N','Y') COLLATE utf8_general_ci
> DEFAULT 'N' NOT NULL, References_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N'
> NOT NULL, Index_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL,
> Alter_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL,
> Create_tmp_table_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL,
> Lock_tables_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL,
> Create_view_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL,
> Show_view_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL,
> Create_routine_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL,
> Alter_routine_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL,
> Execute_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Trigger_priv
> enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, PRIMARY KEY Host (Host,Db) )
> engine=MyISAM CHARACTER SET utf8 COLLATE utf8_bin comment='Host privileges; Merged with
> database privileges';
>>
>>
>> -CREATE TABLE IF NOT EXISTS user ( Host char(60) binary DEFAULT '' NOT NULL,
> User char(16) binary DEFAULT '' NOT NULL, Password char(41) character set latin1 collate
> latin1_bin DEFAULT '' NOT NULL, Select_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT
> 'N' NOT NULL, Insert_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL,
> Update_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Delete_priv
> enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Create_priv enum('N','Y')
> COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Drop_priv enum('N','Y') COLLATE
> utf8_general_ci DEFAULT 'N' NOT NULL, Reload_priv enum('N','Y') COLLATE utf8_general_ci
> DEFAULT 'N' NOT NULL, Shutdown_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT
> NULL, Process_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, File_priv
> enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Grant_priv enum('N','Y')
> COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, References_priv enum('N','Y') COLLATE
> utf8_general_ci DEFAULT 'N' NOT NULL, Index_priv enum('N','Y') COLLATE utf8_general_ci
> DEFAULT 'N' NOT NULL, Alter_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT
> NULL, Show_db_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Super_priv
> enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Create_tmp_table_priv
> enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Lock_tables_priv enum('N','Y')
> COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Execute_priv enum('N','Y') COLLATE
> utf8_general_ci DEFAULT 'N' NOT NULL, Repl_slave_priv enum('N','Y') COLLATE
> utf8_general_ci DEFAULT 'N' NOT NULL, Repl_client_priv enum('N','Y') COLLATE
> utf8_general_ci DEFAULT 'N' NOT NULL, Create_view_priv enum('N','Y') COLLATE
> utf8_general_ci DEFAULT 'N' NOT NULL, Show_view_priv enum('N','Y') COLLATE utf8_general_ci
> DEFAULT 'N' NOT NULL, Create_routine_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT
> 'N' NOT NULL, Alter_routine_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT
> NULL, Create_user_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL,
> Event_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Trigger_priv
> enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, ssl_type enum('','ANY','X509',
> 'SPECIFIED') COLLATE utf8_general_ci DEFAULT '' NOT NULL, ssl_cipher BLOB NOT NULL,
> x509_issuer BLOB NOT NULL, x509_subject BLOB NOT NULL, max_questions int(11) unsigned
> DEFAULT 0 NOT NULL, max_updates int(11) unsigned DEFAULT 0 NOT NULL, max_connections
> int(11) unsigned DEFAULT 0 NOT NULL, max_user_connections int(11) unsigned DEFAULT 0 NOT
> NULL, PRIMARY KEY Host (Host,User) ) engine=MyISAM CHARACTER SET utf8 COLLATE utf8_bin
> comment='Users and global privileges';
>> +CREATE TABLE IF NOT EXISTS user ( Host char(60) binary DEFAULT '' NOT NULL,
> User char(16) binary DEFAULT '' NOT NULL, Password char(41) character set latin1 collate
> latin1_bin DEFAULT '' NOT NULL, Select_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT
> 'N' NOT NULL, Insert_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL,
> Update_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Delete_priv
> enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Create_priv enum('N','Y')
> COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Drop_priv enum('N','Y') COLLATE
> utf8_general_ci DEFAULT 'N' NOT NULL, Reload_priv enum('N','Y') COLLATE utf8_general_ci
> DEFAULT 'N' NOT NULL, Shutdown_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT
> NULL, Process_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, File_priv
> enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Grant_priv enum('N','Y')
> COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, References_priv enum('N','Y') COLLATE
> utf8_general_ci DEFAULT 'N' NOT NULL, Index_priv enum('N','Y') COLLATE utf8_general_ci
> DEFAULT 'N' NOT NULL, Alter_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT
> NULL, Show_db_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Super_priv
> enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Create_tmp_table_priv
> enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Lock_tables_priv enum('N','Y')
> COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Execute_priv enum('N','Y') COLLATE
> utf8_general_ci DEFAULT 'N' NOT NULL, Repl_slave_priv enum('N','Y') COLLATE
> utf8_general_ci DEFAULT 'N' NOT NULL, Repl_client_priv enum('N','Y') COLLATE
> utf8_general_ci DEFAULT 'N' NOT NULL, Create_view_priv enum('N','Y') COLLATE
> utf8_general_ci DEFAULT 'N' NOT NULL, Show_view_priv enum('N','Y') COLLATE utf8_general_ci
> DEFAULT 'N' NOT NULL, Create_routine_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT
> 'N' NOT NULL, Alter_routine_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT
> NULL, Create_user_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL,
> Event_priv enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Trigger_priv
> enum('N','Y') COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Create_role_priv enum('N','Y')
> COLLATE utf8_general_ci DEFAULT 'N' NOT NULL, Drop_role_priv enum('N','Y') COLLATE
> utf8_general_ci DEFAULT 'N' NOT NULL, ssl_type enum('','ANY','X509', 'SPECIFIED') COLLATE
> utf8_general_ci DEFAULT '' NOT NULL, ssl_cipher BLOB NOT NULL, x509_issuer BLOB NOT NULL,
> x509_subject BLOB NOT NULL, max_questions int(11) unsigned DEFAULT 0 NOT NULL,
> max_updates int(11) unsigned DEFAULT 0 NOT NULL, max_connections int(11) unsigned DEFAULT
> 0 NOT NULL, max_user_connections int(11) unsigned DEFAULT 0 NOT NULL, PRIMARY KEY Host
> (Host,User) ) engine=MyISAM CHARACTER SET utf8 COLLATE utf8_bin comment='Users and global
> privileges';
>>
>> -- Remember for later if user table already existed
>> set @had_user_table= @@warning_count != 0;
>>
>> +CREATE TABLE IF NOT EXISTS `roles` (
>> + `Host` char(60) COLLATE utf8_bin NOT NULL DEFAULT '',
>> + `User` char(16) COLLATE utf8_bin NOT NULL DEFAULT '',
>> + `Is_role` enum('N','Y') CHARACTER SET utf8 NOT NULL DEFAULT 'N',
>> + `Role` char(16) COLLATE utf8_bin NOT NULL DEFAULT '',
>> + PRIMARY KEY (`Host`,`User`,`Is_role`,`Role`)
>> +) ENGINE=MyISAM DEFAULT CHARSET=utf8 COLLATE=utf8_bin COMMENT='Users and roles
> relationships';
>> +
>
> Hmm, you only have Is_role column in your roles table, not in user table.
> does it means roles aren't stored in user table ?
> ...
> okay from the code below I see that you simply forgot to update
> mysql_system_tables.sql
>
Exactly. Will be included in next revision.
>>
>> CREATE TABLE IF NOT EXISTS func ( name char(64) binary DEFAULT '' NOT NULL, ret
> tinyint(1) DEFAULT '0' NOT NULL, dl char(128) DEFAULT '' NOT NULL, type enum
> ('function','aggregate') COLLATE utf8_general_ci NOT NULL, PRIMARY KEY (name) )
> engine=MyISAM CHARACTER SET utf8 COLLATE utf8_bin comment='User defined functions';
>>
>> === modified file 'scripts/mysql_system_tables_fix.sql'
>> --- scripts/mysql_system_tables_fix.sql 2008-04-29 04:37:22 +0000
>> +++ scripts/mysql_system_tables_fix.sql 2008-07-06 19:53:28 +0000
>> @@ -520,6 +520,30 @@ ALTER TABLE tables_priv MODIFY Table_pri
>>
>> UPDATE user SET Trigger_priv=Super_priv WHERE @hadTriggerPriv = 0;
>>
>> +#
>> +# CREATE ROLE privilege
>> +#
>> +
>> +SET @hadCreateRolePriv := 0;
>> +SELECT @hadCreateRolePriv :=1 FROM user WHERE Create_role_priv LIKE '%';
>> +
>> +ALTER TABLE user ADD Create_role_priv enum('N','Y') COLLATE utf8_general_ci
> DEFAULT 'N' NOT NULL AFTER Trigger_priv;
>> +ALTER TABLE user MODIFY Create_role_priv enum('N','Y') COLLATE utf8_general_ci
> DEFAULT 'N' NOT NULL AFTER Trigger_priv;
>
> Why two ALTER TABLE statements instead of one ?
>
>> +
>> +UPDATE user SET Create_role_priv=Create_user_priv WHERE @hadCreateRolePriv = 0;
>> +
>> +#
>> +# DROP ROLE privilege
>> +#
>> +
>> +SET @hadDropRolePriv := 0;
>> +SELECT @hadDropRolePriv :=1 FROM user WHERE Drop_role_priv LIKE '%';
>> +
>> +ALTER TABLE user ADD Drop_role_priv enum('N','Y') COLLATE utf8_general_ci
> DEFAULT 'N' NOT NULL AFTER Create_role_priv;
>> +ALTER TABLE user MODIFY Drop_role_priv enum('N','Y') COLLATE utf8_general_ci
> DEFAULT 'N' NOT NULL AFTER Create_role_priv;
>
> Why do you handle Drop_role_priv separetely instead of adding it in the same
> ALTER TABLE that adds Create_role_priv ?
>
Thought it may help reading and understanding script.
>> +
>> +UPDATE user SET Drop_role_priv=Create_user_priv WHERE @hadDropRolePriv = 0;
>> +
>> # Activate the new, possible modified privilege tables
>> # This should not be needed, but gives us some extra testing that the above
>> # changes was correct
>> === modified file 'sql/sql_class.h'
>> --- sql/sql_class.h 2008-06-28 11:00:59 +0000
>> +++ sql/sql_class.h 2008-07-21 21:13:34 +0000
>> @@ -828,14 +828,17 @@ public:
>> the connection
>> priv_user - The user privilege we are using. May be "" for anonymous user.
>> ip - client IP
>> + role - current user role
>> */
>> - char *host, *user, *priv_user, *ip;
>> + char *host, *user, *priv_user, *ip, *role, *priv_role;
>> /* The host privilege we are using */
>> char priv_host[MAX_HOSTNAME];
>> /* points to host if host is available, otherwise points to ip */
>> const char *host_or_ip;
>> - ulong master_access; /* Global privileges from mysql.user */
>> - ulong db_access; /* Privileges for current db */
>> + ulonglong master_access; /* Global mixed privileges for current
> user an role from mysql.user */
>
> not "mixed" but "combined" or "merged"
>
Accepted.
>> + ulonglong db_access; /* Privileges for current db */
>
> also combined ?
>
Yes, of course.
>> + ulonglong true_user_master_access; /* Global privileges for current user
> from mysql.user */
>> + ulonglong true_user_db_access; /* Global privileges for current user and
> current db */
>>
>> void init();
>> void destroy();
>> === modified file 'sql/sql_connect.cc'
>> --- sql/sql_connect.cc 2008-06-27 09:26:03 +0000
>> +++ sql/sql_connect.cc 2008-07-21 21:13:34 +0000
>> @@ -400,7 +401,7 @@ check_user(THD *thd, enum enum_server_co
>> DBUG_PRINT("info",
>> ("Capabilities: %lu packet_length: %ld Host: '%s' "
>> "Login user: '%s' Priv_user: '%s' Using password: %s "
>> - "Access: %lu db: '%s'",
>> + "Access: %llu db: '%s'",
>
> sorry, we don't use %llu - it's not portable enough
> you need to use ullstr() function here
>
Okay, and the same for next %ll* statements.
>> thd->client_capabilities,
>> thd->max_client_packet_length,
>> thd->main_security_ctx.host_or_ip,
>> === modified file 'sql/sql_lex.h'
>> --- sql/sql_lex.h 2008-04-21 23:37:29 +0000
>> +++ sql/sql_lex.h 2008-07-21 21:13:34 +0000
>> @@ -96,6 +96,7 @@ enum enum_sql_command {
>> SQLCOM_SHOW_WARNS, SQLCOM_EMPTY_QUERY, SQLCOM_SHOW_ERRORS,
>> SQLCOM_SHOW_STORAGE_ENGINES, SQLCOM_SHOW_PRIVILEGES,
>> SQLCOM_HELP, SQLCOM_CREATE_USER, SQLCOM_DROP_USER, SQLCOM_RENAME_USER,
>> + SQLCOM_CREATE_ROLE, SQLCOM_DROP_ROLE,
>
> add new commands to the end of the list, just before SQLCOM_END
>
OK
>> SQLCOM_REVOKE_ALL, SQLCOM_CHECKSUM,
>> SQLCOM_CREATE_PROCEDURE, SQLCOM_CREATE_SPFUNCTION, SQLCOM_CALL,
>> SQLCOM_DROP_PROCEDURE, SQLCOM_ALTER_PROCEDURE,SQLCOM_ALTER_FUNCTION,
>> === modified file 'sql/sql_yacc.yy'
>> --- sql/sql_yacc.yy 2008-06-17 20:04:19 +0000
>> +++ sql/sql_yacc.yy 2008-07-21 21:13:34 +0000
>> @@ -10843,6 +10856,25 @@ user:
>> }
>> ;
>>
>> +role:
>> + ident_or_text
>> + {
>> + THD *thd= YYTHD;
>> + if (!($$= (LEX_USER*) thd->alloc(sizeof(st_lex_user))))
>> + MYSQL_YYABORT;
>> + $$->user = $1;
>> + $$->host.str= (char *) "%";
>
> you don't need to set host for roles, do you ?
> if you want to have is set (to simplify some code elsewhere) set it to null_lex_str,
> not to "%"
>
OK, will set it to null_lex_str
>> + $$->host.length= 1;
>> + $$->is_role = true;
>> + if (Lex->users_list.push_back($$))
>> + MYSQL_YYABORT;
>> + if (check_identifier_name(&$$->user, USERNAME_CHAR_LENGTH,
>> + ER_WRONG_STRING_LENGTH, ER(ER_USERNAME)))
>
> you need to use ER_ROLENAME, I assume
>
Yes, will be added.
>> + MYSQL_YYABORT;
>> + }
>> + ;
>> +
>> +
>> /* Keyword that we allow for identifiers (except SP labels) */
>> keyword:
>> keyword_sp {}
>> @@ -11907,6 +11941,13 @@ grant_command:
>> lex->sql_command= SQLCOM_GRANT;
>> lex->type= TYPE_ENUM_PROCEDURE;
>> }
>> + | role TO_SYM grant_list
>> + {
>> + LEX *lex= Lex;
>> + lex->sql_command= SQLCOM_GRANT;
>> + /* Consider defining a constant for role */
>> + lex->type= 3;
>
> Not "consider" - do define a constant. And it should be 4 not 3, as 3 is
> TYPE_ENUM_TRIGGER
>
OK.
>> + }
>> ;
>>
>> opt_table:
>> @@ -12095,6 +12151,19 @@ grant_list:
>> }
>> ;
>>
>> +role_grant_list:
>> + role
>> + {
>> + if (Lex->users_list.push_back($1))
>> + MYSQL_YYABORT;
>> + }
>> + | role_grant_list ',' role
>> + {
>> + if (Lex->users_list.push_back($3))
>> + MYSQL_YYABORT;
>> + }
>> + ;
>> +
>
> What's the difference between role_list and role_grant_list ?
>
role_grant_list was an `experiment` with sql_yacc.yy code and will be
removed and replaced
with role_list.
>> grant_user:
>> user IDENTIFIED_SYM BY TEXT_STRING
>> {
>> === modified file 'sql/sql_parse.cc'
>> --- sql/sql_parse.cc 2008-06-28 11:00:59 +0000
>> +++ sql/sql_parse.cc 2008-07-21 21:13:34 +0000
>> @@ -754,8 +754,8 @@ static my_bool deny_updates_if_read_only
>> LEX *lex= thd->lex;
>>
>> const my_bool user_is_super=
>> - ((ulong)(thd->security_ctx->master_access & SUPER_ACL) ==
>> - (ulong)SUPER_ACL);
>> + ((ulonglong)(thd->security_ctx->master_access & SUPER_ACL) ==
>> + (ulonglong)SUPER_ACL);
>
> There should be no casts here, why are they necessary ?
> Cannot you just remove all casts here ?
>
OK, casts will be removed.
>> if (user_is_super)
>> DBUG_RETURN(FALSE);
>> @@ -3693,6 +3718,12 @@ end_with_restore_list:
>> select_lex->db ? is_schema_db(select_lex->db) : 0))
>> goto error;
>>
>> + //~ if (lex->type == 3)
>> + //~ {
>> +
>> + //~ break;
>> + //~ }
>> +
>
> 1. What's is that ?
> 2. please, make sure your code doesn't have invisible endspaces.
>
Garbage left from implementing and debugging parsing of new commands.
Will be eliminated. Anyway, "//~" is the way SciTE comments code, so
those dirty places will be easy to track.
>> if (thd->security_ctx->user) // If not replication
>> {
>> LEX_USER *user, *tmp_user;
>> @@ -5057,11 +5093,11 @@ check_access(THD *thd, ulong want_access
>> The effective privileges are the union of the global privileges
>> and the the intersection of db- and host-privileges.
>> */
>> - *save_priv=sctx->master_access | db_access;
>> + *save_priv= sctx->master_access | db_access;
>> DBUG_RETURN(FALSE);
>> }
>> - if (((want_access & ~sctx->master_access) & ~DB_ACLS) ||
>> - ! db && dont_check_global_grants)
>> + if (((want_access & ~sctx->master_access) & ~(DB_ACLS | EXTRA_ACL))
> ||
>> + db && dont_check_global_grants)
>
> Why ?
>
Actually, I don't remember this at all O_O. What original version did
you use for review?
>> { // We can never grant this
>> DBUG_PRINT("error",("No possible access"));
>> if (!no_errors)
>> === modified file 'sql/sql_acl.h'
>> --- sql/sql_acl.h 2008-03-28 13:33:31 +0000
>> +++ sql/sql_acl.h 2008-07-21 21:13:34 +0000
>> @@ -15,34 +15,36 @@
>>
>> #include "slave.h" // for tables_ok(), rpl_filter
>>
>> -#define SELECT_ACL (1L << 0)
>> -#define INSERT_ACL (1L << 1)
>> -#define UPDATE_ACL (1L << 2)
>> -#define DELETE_ACL (1L << 3)
>> -#define CREATE_ACL (1L << 4)
>> -#define DROP_ACL (1L << 5)
>> -#define RELOAD_ACL (1L << 6)
>> -#define SHUTDOWN_ACL (1L << 7)
>> -#define PROCESS_ACL (1L << 8)
>> -#define FILE_ACL (1L << 9)
>> -#define GRANT_ACL (1L << 10)
>> -#define REFERENCES_ACL (1L << 11)
>> -#define INDEX_ACL (1L << 12)
>> -#define ALTER_ACL (1L << 13)
>> -#define SHOW_DB_ACL (1L << 14)
>> -#define SUPER_ACL (1L << 15)
>> -#define CREATE_TMP_ACL (1L << 16)
>> -#define LOCK_TABLES_ACL (1L << 17)
>> -#define EXECUTE_ACL (1L << 18)
>> -#define REPL_SLAVE_ACL (1L << 19)
>> -#define REPL_CLIENT_ACL (1L << 20)
>> -#define CREATE_VIEW_ACL (1L << 21)
>> -#define SHOW_VIEW_ACL (1L << 22)
>> -#define CREATE_PROC_ACL (1L << 23)
>> -#define ALTER_PROC_ACL (1L << 24)
>> -#define CREATE_USER_ACL (1L << 25)
>> -#define EVENT_ACL (1L << 26)
>> -#define TRIGGER_ACL (1L << 27)
>> +#define SELECT_ACL (1LL << 0)
>> +#define INSERT_ACL (1LL << 1)
>> +#define UPDATE_ACL (1LL << 2)
>> +#define DELETE_ACL (1LL << 3)
>> +#define CREATE_ACL (1LL << 4)
>> +#define DROP_ACL (1LL << 5)
>> +#define RELOAD_ACL (1LL << 6)
>> +#define SHUTDOWN_ACL (1LL << 7)
>> +#define PROCESS_ACL (1LL << 8)
>> +#define FILE_ACL (1LL << 9)
>> +#define GRANT_ACL (1LL << 10)
>> +#define REFERENCES_ACL (1LL << 11)
>> +#define INDEX_ACL (1LL << 12)
>> +#define ALTER_ACL (1LL << 13)
>> +#define SHOW_DB_ACL (1LL << 14)
>> +#define SUPER_ACL (1LL << 15)
>> +#define CREATE_TMP_ACL (1LL << 16)
>> +#define LOCK_TABLES_ACL (1LL << 17)
>> +#define EXECUTE_ACL (1LL << 18)
>> +#define REPL_SLAVE_ACL (1LL << 19)
>> +#define REPL_CLIENT_ACL (1LL << 20)
>> +#define CREATE_VIEW_ACL (1LL << 21)
>> +#define SHOW_VIEW_ACL (1LL << 22)
>> +#define CREATE_PROC_ACL (1LL << 23)
>> +#define ALTER_PROC_ACL (1LL << 24)
>> +#define CREATE_USER_ACL (1LL << 25)
>> +#define EVENT_ACL (1LL << 26)
>> +#define TRIGGER_ACL (1LL << 27)
>> +#define CREATE_ROLE_ACL (1LL << 28)
>> +#define DROP_ROLE_ACL (1LL << 29)
>
> write LL(1) not 1LL, even better use ULL(1)
>
OK.
>> /*
>> don't forget to update
>> 1. static struct show_privileges_st sys_privileges[]
>> @@ -188,13 +191,25 @@ public:
>> };
>>
>>
>> +class ACL_ROLE:public ACL_ACCESS
>> +{
>> + public:
>> + char* role;
>> + /* Shall be used in implementation of role-to-role assignment */
>
> you mean it's not used yet ? or just forgot to remove the comment ? :)
>
Not used _yet_. As strict timeline prevents me from implementing
role-to-role assignment in the scope of GSoC project, I think I'll
continue working on it and implement this.
>> + DYNAMIC_ARRAY assigned_roles;
>> +};
>> +
>> +
>> class ACL_USER :public ACL_ACCESS
>> {
>> public:
>> acl_host_and_ip host;
>> uint hostname_length;
>> USER_RESOURCES user_resource;
>> - char *user;
>> + char *user, *default_role_name;
>> + ACL_ROLE *default_role, *current_role;
>
> why do you need default_role_name ? Isn't it the same as
> default_role->role ?
>
Data from mysql.users table is loaded before data from mysql.roles table and
assigning of assigned_roles array, default_role and current_role members is done
after entire mysql.users table has already been loaded in acl_users array. So we
need a string variable to store default role name for each user in acl_users.
It could have been done through other array of type, say, char**, but I doubt
the work of synchronising those arrays is worth economy of some ten bytes in
ACL_USER class object.
>> + DYNAMIC_ARRAY assigned_roles;
>> +// bool is_role;
>> uint8 salt[SCRAMBLE_LENGTH+1]; // scrambled password in binary form
>> uint8 salt_len; // 0 - no password, 4 - 3.20, 8 - 3.23, 20 - 4.1.1
>> enum SSL_type ssl_type;
>> @@ -227,38 +243,40 @@ int check_change_password(THD *thd, cons
>> bool change_password(THD *thd, const char *host, const char *user,
>> char *password);
>> bool mysql_grant(THD *thd, const char *db, List <LEX_USER>
> &user_list,
>> - ulong rights, bool revoke);
>> + ulonglong rights, bool revoke);
>> +bool mysql_grant_role(THD *thd, List <LEX_USER> &user_list, bool
> revoke);
>> int mysql_table_grant(THD *thd, TABLE_LIST *table, List <LEX_USER>
> &user_list,
>> - List <LEX_COLUMN> &column_list, ulong rights,
>> + List <LEX_COLUMN> &column_list, ulonglong
> rights,
>> bool revoke);
>> bool mysql_routine_grant(THD *thd, TABLE_LIST *table, bool is_proc,
>> - List <LEX_USER> &user_list, ulong rights,
>> + List <LEX_USER> &user_list, ulonglong rights,
>> bool revoke, bool no_error);
>> my_bool grant_init();
>> void grant_free(void);
>> my_bool grant_reload(THD *thd);
>> -bool check_grant(THD *thd, ulong want_access, TABLE_LIST *tables,
>> +bool check_grant(THD *thd, ulonglong want_access, TABLE_LIST *tables,
>> bool show_command, uint number, bool dont_print_error);
>> bool check_grant_column (THD *thd, GRANT_INFO *grant,
>> const char *db_name, const char *table_name,
>> const char *name, uint length, Security_context *sctx);
>> bool check_column_grant_in_table_ref(THD *thd, TABLE_LIST * table_ref,
>> const char *name, uint length);
>> -bool check_grant_all_columns(THD *thd, ulong want_access,
>> +bool check_grant_all_columns(THD *thd, ulonglong want_access,
>> Field_iterator_table_ref *fields);
>> -bool check_grant_routine(THD *thd, ulong want_access,
>> +bool check_grant_routine(THD *thd, ulonglong want_access,
>> TABLE_LIST *procs, bool is_proc, bool no_error);
>> bool check_grant_db(THD *thd,const char *db);
>> -ulong get_table_grant(THD *thd, TABLE_LIST *table);
>> -ulong get_column_grant(THD *thd, GRANT_INFO *grant,
>> +ulonglong get_table_grant(THD *thd, TABLE_LIST *table);
>> +ulonglong get_column_grant(THD *thd, GRANT_INFO *grant,
>> const char *db_name, const char *table_name,
>> const char *field_name);
>> bool mysql_show_grants(THD *thd, LEX_USER *user);
>> -void get_privilege_desc(char *to, uint max_length, ulong access);
>> +void get_privilege_desc(char *to, uint max_length, ulonglong access);
>> void get_mqh(const char *user, const char *host, USER_CONN *uc);
>> bool mysql_create_user(THD *thd, List <LEX_USER> &list);
>> bool mysql_drop_user(THD *thd, List <LEX_USER> &list);
>> bool mysql_rename_user(THD *thd, List <LEX_USER> &list);
>> +bool mysql_create_role(THD *thd, List <LEX_USER> &list);
>
> you don't use mysql_create_role(), remove it
>
OK.
>> bool mysql_revoke_all(THD *thd, List <LEX_USER> &list);
>> void fill_effective_table_privileges(THD *thd, GRANT_INFO *grant,
>> const char *db, const char *table);
>> === modified file 'sql/sql_acl.cc'
>> --- sql/sql_acl.cc 2008-06-28 11:00:59 +0000
>> +++ sql/sql_acl.cc 2008-07-25 20:27:34 +0000
>> @@ -194,6 +194,40 @@ static bool compare_hostname(const acl_h
>> static my_bool acl_load(THD *thd, TABLE_LIST *tables);
>> static my_bool grant_load(THD *thd, TABLE_LIST *tables);
>>
>> +
>> +/* KILL THIS! */
>> +static void print_buffer_to_file(enum loglevel level, int error_code,
>> + const char *buffer, size_t buffer_length)
>
> Heh, that's why we have DBUG facility.
> just use DBUG_PRINT("roles", ... ) instead, e.g.
>
> DBUG_PRINT("roles", ("Loading information for role %s...", role.role));
>
> you could get your debug output with
>
> ./mysqld -#d,roles
>
> and you won't need to remove DBUG_PRINT() from the final patch.
> for complete manual see dbug/user.ps or dbug/user.t
>
As I've already mentioned to you, I did have difficulties with
building MySQL in
debug mode, so I used those messages to debug my code. Of course,
I will add DBUG_PRINT() calls wherever necessary.
>> +{
>> + time_t skr;
>> + struct tm tm_tmp;
>> + struct tm *start;
>> + DBUG_ENTER("print_buffer_to_file");
>> + DBUG_PRINT("enter",("buffer: %s", buffer));
>> +
>> + (void)pthread_mutex_lock(&LOCK_error_log);
>> +
>> + skr= my_time(0);
>> + localtime_r(&skr, &tm_tmp);
>> + start=&tm_tmp;
>> +
>> + fprintf(stderr, "%02d%02d%02d %2d:%02d:%02d [%s] %s\n",
>> + start->tm_year % 100,
>> + start->tm_mon+1,
>> + start->tm_mday,
>> + start->tm_hour,
>> + start->tm_min,
>> + start->tm_sec,
>> + (level == ERROR_LEVEL ? "ERROR" : level == WARNING_LEVEL ?
>> + "Warning" : "Note"),
>> + buffer);
>> +
>> + fflush(stderr);
>> +
>> + (void)pthread_mutex_unlock(&LOCK_error_log);
>> + DBUG_VOID_RETURN;
>> +}
>> +
>> /*
>> Convert scrambled password to binary form, according to scramble type,
>> Binary form is stored in user.salt.
>> @@ -420,6 +461,32 @@ static my_bool acl_load(THD *thd, TABLE_
>> allow_all_hosts=0;
>> while (!(read_record_info.read_record(&read_record_info)))
>> {
>> + is_role= false;
>
> I would declare is_role variable here, not in the beginning of the function.
> you don't use it outside of this scope anyway.
> (you use it also in while() loop below, but it's a different 'is_role' variable,
> which should be declared in that scope)
>
OK.
>> + if (table->s->fields >= 43)
>> + {
>> + /* Starting from 6.0.xy we have Is_role field */
>
> In the mysql.user table ??? I don't see it in mysql_system_tables.sql
>
Discussed at the very beginning of letter.
>> + char* ptr= get_field(thd->mem_root, table->field[41]);
>> + if (!strcmp(ptr, "Y"))
>> + is_role= true;
>> + else
>> + is_role= false;
>
> better (simpler, easier to understand):
>
> is_role= !strcmp(ptr, "Y");
>
> but you need to check first that ptr is not NULL. It is possible that somebody will
> do
> ALTER TABLE mysql.user MODIFY is_role ... NULL;
> and store a NULL value there. The server should not crash.
>
OK
>> + }
>> + if (is_role)
>> + {
>> + ACL_ROLE role;
>> + role.role= get_field(&mem, table->field[1]);
>> + role.sort= get_sort(1,role.role);
>
> do you need get_sort() ? it's for names with wildcards, you could use a constant
> here.
>
Copy/paste. Anyway, when I was actually working on this function, I did not
digged into sorting mechanism - just left it as it is. Now I think I will remove
thos get_sort() call along with `sort` field in ACL_ROLE class (i.e.
remove inheritance
from ACL_ACCESS).
>> + uint next_field;
>> + char buf[255];
>> + sprintf(buf, "Loading information for role %s...", role.role);
>> + print_buffer_to_file(INFORMATION_LEVEL, 2000, buf, 255);
>
> replace with DBUG_PRINT here and below
>
OK.
>> + /* No necessity in checking pre-6.0.xy privileges here as Is_role field
> exists */
>> + role.access= get_access(table,3,&next_field) & GLOBAL_ACLS;
>> + /* SSL encryption fields and resource limits not applicable for roles */
>> + (void) push_dynamic(&acl_roles,(uchar*) &role);
>> + }
>> + else
>> + {
>> ACL_USER user;
>> update_hostname(&user.host, get_field(&mem, table->field[0]));
>> user.user= get_field(&mem, table->field[1]);
>> @@ -531,12 +608,26 @@ static my_bool acl_load(THD *thd, TABLE_
>> }
>> else
>> user.user_resource.user_conn= 0;
>> +
>> + if (table->s->fields >= 43)
>> + {
>> + /* Starting from 6.0.xy we have Default_role field */
>
> No, you don't. Not in mysql_system_tables.sql at least.
>
Already discussed.
>> + next_field++;
>> + user.default_role_name= get_field(thd->mem_root,
> table->field[next_field++]);
>> + char buf[255];
>> + sprintf(buf, "nextfield = %d, user=%s, default_role_name=%s",
> next_field, user.user, user.default_role_name);
>> + print_buffer_to_file(INFORMATION_LEVEL, 2000, buf, 255);
>
> download a couple of old releases, create system tables from there (I think
> they're included in .zip windows releases, if not, use mysql_install_db script or
> mysql_system_tables.sql from the old release).
> And make sure that your code works correctly with old tables.
>
OK, will do that next week.
>> + }
>> + else
>> + {
>> + user.default_role_name= (char*) "NONE";
>> + }
>> }
>> else
>> {
>> user.ssl_type=SSL_TYPE_NONE;
>> bzero((char *)&(user.user_resource),sizeof(user.user_resource));
>> -#ifndef TO_BE_REMOVED
>> + #ifndef TO_BE_REMOVED
>
> revert that
>
OK.
>> if (table->s->fields <= 13)
>> { // Without grant
>> if (user.access & CREATE_ACL)
>> @@ -548,18 +639,24 @@ static my_bool acl_load(THD *thd, TABLE_
>> user.access|= REPL_CLIENT_ACL | REPL_SLAVE_ACL;
>> if (user.access & PROCESS_ACL)
>> user.access|= SUPER_ACL | EXECUTE_ACL;
>> -#endif
>> + user.default_role_name= (char*) "NONE";
>> + #endif
>> }
>> + user.default_role= 0;
>> + user.current_role= 0;
>> + (void)
> my_init_dynamic_array(&(user.assigned_roles),sizeof(ACL_ROLE*),50,100);
>
> 50,100 is too large. Check what the mean and think about values you use.
> also think whether it'd be better to use a hash instead of an array (see my comment
> about
> acl_roles array below). For acl_roles hash certainly makes sense, but here
> array usually won't be big - consider an overhead that hash adds and whether the
> speedup is worth it.
>
OK, will be done next week.
>> (void) push_dynamic(&acl_users,(uchar*) &user);
>> if (!user.host.hostname ||
>> (user.host.hostname[0] == wild_many && !user.host.hostname[1]))
>> allow_all_hosts=1; // Anyone can connect
>> }
>> }
>> + }
>> my_qsort((uchar*)
> dynamic_element(&acl_users,0,ACL_USER*),acl_users.elements,
>> sizeof(ACL_USER),(qsort_cmp) acl_compare);
>> end_read_record(&read_record_info);
>> freeze_size(&acl_users);
>> + freeze_size(&acl_roles);
>>
>> init_read_record(&read_record_info,thd,table=tables[2].table,NULL,1,0);
>> table->use_all_columns();
>> @@ -619,6 +716,77 @@ static my_bool acl_load(THD *thd, TABLE_
>> sizeof(ACL_DB),(qsort_cmp) acl_compare);
>> end_read_record(&read_record_info);
>> freeze_size(&acl_dbs);
>> +
>> + init_read_record(&read_record_info,thd,table=tables[3].table,NULL,1,0);
>> + table->use_all_columns();
>> + char buf[255];
>> + while (!(read_record_info.read_record(&read_record_info)))// &&
> false
>
> don't forget to remove it
>
OK.
>> + {
>> + char* host= get_field(&mem, table->field[0]);
>> + char* user= get_field(&mem, table->field[1]);
>> + char* ptr= get_field(thd->mem_root, table->field[2]);
>> + if (ptr)
>> + {
>> + if (!strcmp(ptr, "Y"))
>> + is_role= true;
>> + else
>> + is_role= false;
>> + }
>
> same here. check for NULL and is_role= !strcmp(ptr, "Y");
>
> on the second thought, why do you need is_role here ? I mean, why do you need
> is_role in mysql.roles table ? you have it in mysql.user already.
>
Discussed in private messages.
>> + char* role = get_field(&mem, table->field[3]);
>> +
>> + sprintf(buf, "Trying to assign role %s to user %s...", role, user);
>> + print_buffer_to_file(INFORMATION_LEVEL, 2000, buf, 255);
>> +
>> + if (is_role)
>> + {
>
> eh ???
>
Left for implementation of role-to-role assignments. I will leave
appropriate comment here.
>> + }
>> + else
>> + {
>> + for (uint i=0 ; i < acl_users.elements ; i++)
>> + {
>> + ACL_USER *acl_user_tmp= dynamic_element(&acl_users,i,ACL_USER*);
>> + if (user && acl_user_tmp->user)
>> + {
>> + if (!strcmp(user, acl_user_tmp->user))
>> + {
>> + if (host && &acl_user_tmp->host)
>
> what does that mean ?
>
null-check. Had server crash there sometimes.
>> + if (compare_hostname(&acl_user_tmp->host, host, host))
>> + {
>> + sprintf(buf, "User %s found...", user);
>> + print_buffer_to_file(INFORMATION_LEVEL, 2000, buf, 255);
>> + for (uint j=0 ; j < acl_roles.elements ; j++)
>
> you don't need nested loops.
>
>> + {
>> + ACL_ROLE *acl_role_tmp=
> dynamic_element(&acl_roles,j,ACL_ROLE*);
>
> actually, you'd better keep roles in a hash, not in a list.
> usernames are in the list, because they have wildcards and need to be matched
> linearly.
> but role names don't have wildcard.
>
OK, I will rework that.
>> + if (acl_role_tmp && acl_role_tmp->role)
>> + {
>> + if (!strcmp(acl_role_tmp->role, role))
>> + {
>> + sprintf(buf, "Role %s found, assigning...", role);
>> + print_buffer_to_file(INFORMATION_LEVEL, 2000, buf, 255);
>> + (void)
> push_dynamic(&(acl_user_tmp->assigned_roles),(uchar*) acl_role_tmp);
>> + if (acl_user_tmp->default_role_name)
>> + {
>> + sprintf(buf, "User %s has default role %s...", user,
> acl_user_tmp->default_role_name);
>> + print_buffer_to_file(INFORMATION_LEVEL, 2000, buf, 255);
>> + if (!strcmp(acl_role_tmp->role,
> acl_user_tmp->default_role_name))
>> + {
>> + acl_user_tmp->default_role= acl_role_tmp;
>> + acl_user_tmp->current_role= acl_role_tmp;
>> + }
>> + }
>> + break;
>> + }
>> + }
>> + }
>> + break;
>> + }
>> + }
>> + }
>> + }
>> + }
>> + }
>> + end_read_record(&read_record_info);
>> +
>> init_check_host();
>>
>> initialized=1;
>> @@ -866,8 +1039,8 @@ static int acl_compare(ACL_ACCESS *a,ACL
>> int acl_getroot(THD *thd, USER_RESOURCES *mqh,
>> const char *passwd, uint passwd_len)
>> {
>> - ulong user_access= NO_ACCESS;
>> - int res= 1;
>> + ulonglong user_access= NO_ACCESS;
>> +int res= 1;
>
> indentation
>
OK.
>> ACL_USER *acl_user= 0;
>> Security_context *sctx= thd->security_ctx;
>> DBUG_ENTER("acl_getroot");
>> @@ -1045,8 +1218,11 @@ int acl_getroot(THD *thd, USER_RESOURCES
>> break;
>> #endif /* HAVE_OPENSSL */
>> }
>> - sctx->master_access= user_access;
>> + print_buffer_to_file(INFORMATION_LEVEL, 2000, "Applying privs... ",
> strlen("Applying privs... "));
>> + sctx->master_access= user_access | (acl_user->current_role ?
> acl_user->current_role->access : 0);// | user_role_access
>
> don't forget to remove // | ...
>
OK.
>> sctx->priv_user= acl_user->user ? sctx->user : (char *) "";
>> + sctx->true_user_master_access= user_access;
>> + sctx->priv_role= acl_user->current_role ?
> acl_user->current_role->role : (char *) "";
>
> it's be more logical to have sctx->priv_role=0 when there's no current role.
> you don't use sctx->priv_role now at all, I assume you'll use it in
> CURRENT_ROLE() sql function. This function returns NULL when there's no current
> role,
> not an empty string.
>
Agree, standard says that. Will do in next revision.
>> *mqh= acl_user->user_resource;
>>
>> if (acl_user->host.hostname)
>> @@ -1141,7 +1320,7 @@ bool acl_getroot_no_password(Security_co
>> {
>> if (!acl_db->db || (db && !wild_compare(db, acl_db->db, 0)))
>> {
>> - sctx->db_access= acl_db->access;
>> + sctx->db_access|= acl_db->access;
>
> why ?
>
Error. Will be reverted to previous state.
>> break;
>> }
>> }
>> @@ -1149,6 +1328,9 @@ bool acl_getroot_no_password(Security_co
>> }
>> sctx->master_access= acl_user->access;
>> sctx->priv_user= acl_user->user ? user : (char *) "";
>> + sctx->true_user_master_access= acl_user->access;
>> + sctx->priv_role= (char *) "";
>> +
>
> add a comment, explaining why you ignore roles here. preferrably in the
> beginning of the function, in the function comment.
>
OK.
>> if (acl_user->host.hostname)
>> strmake(sctx->priv_host, acl_user->host.hostname, MAX_HOSTNAME);
>> @@ -1363,10 +1545,18 @@ ulong acl_get(const char *host, const ch
>> {
>> if (!acl_db->db || !wild_compare(db,acl_db->db,db_is_pattern))
>> {
>> - db_access=acl_db->access;
>> - if (acl_db->host.hostname)
>> - goto exit; // Fully specified. Take it
>> - break; /* purecov: tested */
>> + db_access|= acl_db->access;
>
> you added |= and removed goto exit and break.
> why ?
>
Error, will be reverted to prevoius state.
>> + }
>> + }
>> + }
>> + else if (!acl_db->user)
>> + {
>> + for (uint j=0 ; j < acl_users.elements ; j++)
>> + {
>> + ACL_USER *acl_user= dynamic_element(&acl_users,j,ACL_USER*);
>> + if (strcmp(acl_user->current_role->role, (char*) "NONE")
> && !strcmp(acl_user->current_role->role, acl_db->user))
>> + {
>> + db_access|= acl_db->access;
>> }
>
> I don't understand this loop at all.
>
sctx->db_access must contain composition of user's and current_role's
privileges on
current table, so we have to get both privileges. First is assigned by
exisitng code,
this loop does the second part (at least, it was supposed to).
>> }
>> }
>> @@ -3477,6 +3669,146 @@ bool mysql_grant(THD *thd, const char *d
>> }
>>
>>
>> +bool mysql_grant_role(THD *thd, List <LEX_USER> &list,
>> + bool revoke_grant)
>> +{
>> + List_iterator <LEX_USER> str_list (list);
>> + /* Skipping and saving first record in list as it contains desired role*/
>> + LEX_USER *Role= str_list++;
>> + LEX_USER *Str, *tmp_Str;
>
> as you have noticed, we use lowercase in variable names
>
OK, will change that. Anyway, mysql_grant looks the same. Should I
correct it, too?
>> + TABLE_LIST tables[2];
>> + TABLE *table;
>> + DBUG_ENTER("mysql_grant_role");
>> + if (!initialized)
>> + {
>> + my_error(ER_OPTION_PREVENTS_STATEMENT, MYF(0),
>> + "--skip-grant-tables");
>> + DBUG_RETURN(TRUE);
>> + }
>> +
>> + /* open the mysql.user and mysql.roles tables */
>> + bzero((char*) &tables,sizeof(tables));
>> + tables[0].alias=tables[0].table_name=(char*) "user";
>> + tables[1].alias=tables[1].table_name=(char*) "roles";
>> + tables[0].next_local= tables[0].next_global= tables+1;
>> + tables[0].lock_type=tables[1].lock_type=TL_WRITE;
>> + tables[0].db=tables[1].db=(char*) "mysql";
>> + alloc_mdl_locks(tables, thd->mem_root);
>> +
>> + /*
>> + This statement will be replicated as a statement, even when using
>> + row-based replication. The flag will be reset at the end of the
>> + statement.
>> + */
>> + thd->clear_current_stmt_binlog_row_based();
>
> for the above clear_current_stmt_binlog_row_based() under the #ifdef below
>
OK.
>> +
>> +#ifdef HAVE_REPLICATION
>> + /*
>> + GRANT and REVOKE are applied the slave in/exclusion rules as they are
>> + some kind of updates to the mysql.% tables.
>> + */
>> + if (thd->slave_thread && rpl_filter->is_on())
>> + {
>> + /*
>> + The tables must be marked "updating" so that tables_ok() takes them into
>> + account in tests.
>> + */
>> + tables[0].updating= tables[1].updating= 1;
>> + if (!(thd->spcont || rpl_filter->tables_ok(0, tables)))
>> + DBUG_RETURN(FALSE);
>> + }
>> +#endif
>> +
>> + if (simple_open_n_lock_tables(thd,tables))
>> + { // This should never happen
>> + close_thread_tables(thd);
>> + DBUG_RETURN(TRUE);
>> + }
>> +
>> + //~ if (!revoke_grant)
>> + //~ create_new_users= test_if_create_new_users(thd);
>
> don't forget to remove it
>
Of course.
>> +
>> + /* go through users in user_list */
>> + rw_wrlock(&LOCK_grant);
>> + //pthread_mutex_lock(&acl_cache->lock);
>> + grant_version++;
>> + table= tables[1].table;
>> + table->use_all_columns();
>> + int result= -1;
>> + int error= -1;
>> + while ((tmp_Str = str_list++))
>> + {
>> + result= 0;
>> + if (!(Str= get_current_user(thd, tmp_Str)))
>> + {
>> + result= TRUE;
>> + continue;
>> + }
>
> why do you need that ?
>
Erroneous copypaste, will be removed.
>> + for (uint i=0 ; i < acl_users.elements && !result ; i++)
>> + {
>> + ACL_USER *acl_user_tmp= dynamic_element(&acl_users,i,ACL_USER*);
>> + if (!acl_user_tmp->user || !strcmp(tmp_Str->user.str,
> acl_user_tmp->user))
>> + {
>> + if (compare_hostname(&acl_user_tmp->host, tmp_Str->host.str,
> tmp_Str->host.str))
>> + {
>> + /* linear search complete: */
>> +
>> + for (uint j=0 ; j < acl_user_tmp->assigned_roles.elements ;
> j++)
>> + {
>> + ACL_ROLE *acl_role_tmp=
> dynamic_element(&acl_user_tmp->assigned_roles, j, ACL_ROLE*);
>> + if (!strcmp(acl_role_tmp->role,Role->user.str))
>> + result=-1;
>
> where's "break;" ?
> alternatively, you may skip the check for duplicate grants above,
> and let ha_write_row() below fail with duplicate key.
>
> on the other hand, you *must* verify that the role exists before writing its name to
> the table.
>
OK, will add a break. About verification of existence: what about a
feature like auto-creating
user in case of GRANTing privileges to him?
>> + }
>> + if (!result)
>> + {
>> + restore_record(table,s->default_values);
>> +
> table->field[0]->store(acl_user_tmp->host.hostname,strlen(acl_user_tmp->host.hostname),
>> + system_charset_info);
>> +
> table->field[1]->store(acl_user_tmp->user,strlen(acl_user_tmp->user),
>> + system_charset_info);
>> + table->field[2]->store((char*)"N", 1, system_charset_info);
>> + table->field[3]->store(Role->user.str,
> strlen(Role->user.str), system_charset_info);
>> + if ((error=table->file->ha_write_row(table->record[0]))) //
> insert
>> + { // This should never happen
>> + if (table->file->is_fatal_error(error, HA_CHECK_DUP))
>> + {
>> + table->file->print_error(error,MYF(0));
>> + error= -1;
>> + break;
>> + }
>> + }
> ^^^^^^^^^^^
> remove invisible endspaces (here and everywhere in your code)
>
OK.
>> + for (uint j=0 ; j < acl_roles.elements ; j++)
>> + {
>> + ACL_ROLE *acl_role_tmp= dynamic_element(&acl_roles, j,
> ACL_ROLE*);
>> + if (!strcmp(acl_role_tmp->role,Role->user.str))
>> + {
>> + (void)
> push_dynamic(&acl_user_tmp->assigned_roles,(uchar*) acl_role_tmp);
>> + }
>> + }
>> + result= -1;
>> + }
>> + }
>> + }
>> + }
>> + }
>> + pthread_mutex_unlock(&acl_cache->lock);
>> +
>> + if (!result)
>> + {
>> + write_bin_log(thd, TRUE, thd->query, thd->query_length);
>> + }
>> +
>> + rw_unlock(&LOCK_grant);
>> + close_thread_tables(thd);
>> +
>> + if (!result)
>> + my_ok(thd);
>> +
>> + DBUG_RETURN(result);
>> +}
>> +
>> +
>> +
>> /* Free grant array if possible */
>>
>> void grant_free(void)
>> @@ -4103,9 +4435,9 @@ bool check_grant_column(THD *thd, GRANT_
>> {
>> GRANT_TABLE *grant_table;
>> GRANT_COLUMN *grant_column;
>> - ulong want_access= grant->want_privilege & ~grant->privilege;
>> + ulonglong want_access= grant->want_privilege & ~grant->privilege;
>> DBUG_ENTER("check_grant_column");
>> - DBUG_PRINT("enter", ("table: %s want_access: %lu", table_name,
> want_access));
>> + DBUG_PRINT("enter", ("table: %s want_access: %llu", table_name,
> want_access));
>
> no %llu
>
>>
>> if (!want_access)
>> DBUG_RETURN(0); // Already checked
>> @@ -6716,7 +7070,7 @@ void fill_effective_table_privileges(THD
>> {
>> DBUG_PRINT("info", ("skip grants"));
>> grant->privilege= ~NO_ACCESS; // everything is allowed
>> - DBUG_PRINT("info", ("privilege 0x%lx", grant->privilege));
>> + DBUG_PRINT("info", ("privilege 0x%llx", grant->privilege));
>
> no %llx
>
>> DBUG_VOID_RETURN;
>> }
>>
>> @@ -6725,7 +7079,7 @@ void fill_effective_table_privileges(THD
>>
>> if (!sctx->priv_user)
>> {
>> - DBUG_PRINT("info", ("privilege 0x%lx", grant->privilege));
>> + DBUG_PRINT("info", ("privilege 0x%llx", grant->privilege));
>
> no %llx
>
>> DBUG_VOID_RETURN; // it is slave
>> }
>>
>> @@ -6748,7 +7102,7 @@ void fill_effective_table_privileges(THD
>> }
>> rw_unlock(&LOCK_grant);
>>
>> - DBUG_PRINT("info", ("privilege 0x%lx", grant->privilege));
>> + DBUG_PRINT("info", ("privilege 0x%llx", grant->privilege));
>
> no %llx
>
>> DBUG_VOID_RETURN;
>> }
>
--
Best regards,
Sergey Kudriavtsev
