Re: [Maria-developers] [Commits] 7d386bb: MDEV-4549 [PATCH] Clean up code working with ACL tables
Hi Sergei, ok to push. A few minor questions inline. On Mon, Jun 16, 2014 at 09:34:21AM +0200, serg@mariadb.org wrote:
revision-id: 7d386bbce216745ba28df728f45a0b796fcced98 parent(s): f61f36b386e8d0c6448297bb84c92e8d9b82be6a committer: Sergei Golubchik branch nick: maria timestamp: 2014-06-15 17:12:57 +0200 message:
MDEV-4549 [PATCH] Clean up code working with ACL tables
enum values to index different ACL tables, instead of hard-coded numbers (even different in diffent functions). Also factor out TABLE_LIST initialization into a separate function.
--- mysql-test/r/not_embedded_server.result | 4 +- mysql-test/r/sp-destruct.result | 3 - .../suite/rpl/r/rpl_tmp_table_and_DDL.result | 22 +- mysql-test/t/not_embedded_server.test | 2 +- mysql-test/t/sp-destruct.test | 1 - sql/sql_acl.cc | 533 +++++++++------------ 6 files changed, 235 insertions(+), 330 deletions(-)
diff --git a/mysql-test/r/not_embedded_server.result b/mysql-test/r/not_embedded_server.result index 2295276..6dda855 100644 --- a/mysql-test/r/not_embedded_server.result +++ b/mysql-test/r/not_embedded_server.result @@ -1,4 +1,4 @@ -call mtr.add_suppression("Can't open and lock privilege tables: Table 'host' was not locked with LOCK TABLES"); +call mtr.add_suppression("Can't open and lock privilege tables: Table 'roles_mapping' was not locked with LOCK TABLES"); SHOW VARIABLES like 'slave_skip_errors'; Variable_name Value slave_skip_errors OFF Why did it change (here and in other tests)? I didn't notice any table order change in the code.
...skip...
diff --git a/mysql-test/r/sp-destruct.result b/mysql-test/r/sp-destruct.result index 172e40c..fe68229 100644 --- a/mysql-test/r/sp-destruct.result +++ b/mysql-test/r/sp-destruct.result @@ -128,11 +128,8 @@ CREATE FUNCTION f1() RETURNS INT RETURN 1; RENAME TABLE mysql.procs_priv TO procs_priv_backup; FLUSH TABLE mysql.procs_priv; DROP FUNCTION f1; -ERROR 42S02: Table 'mysql.procs_priv' doesn't exist SHOW WARNINGS; Level Code Message -Error 1146 Table 'mysql.procs_priv' doesn't exist -Warning 1405 Failed to revoke all privileges to dropped routine # Restore the procs_priv table RENAME TABLE procs_priv_backup TO mysql.procs_priv; FLUSH TABLE mysql.procs_priv; Hmm... why?
...skip...
diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index 5e8e0e7..442d512 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc @@ -380,7 +380,7 @@ class ACL_PROXY_USER :public ACL_ACCESS MYSQL_PROXIES_PRIV_PROXIED_USER, MYSQL_PROXIES_PRIV_WITH_GRANT, MYSQL_PROXIES_PRIV_GRANTOR, - MYSQL_PROXIES_PRIV_TIMESTAMP } old_acl_proxy_users; + MYSQL_PROXIES_PRIV_TIMESTAMP } proxy_table_fields; public: ACL_PROXY_USER () {};
@@ -760,6 +760,99 @@ static int traverse_role_graph_down(ACL_USER_BASE *, void *, int (*) (ACL_USER_BASE *, ACL_ROLE *, void *));
/* + Enumeration of ACL/GRANT tables in the mysql database +*/ +enum enum_acl_tables +{ + USER_TABLE, + DB_TABLE, + TABLES_PRIV_TABLE, + COLUMNS_PRIV_TABLE, +#define FIRST_OPTIONAL_TABLE HOST_TABLE + HOST_TABLE, + PROCS_PRIV_TABLE, + PROXIES_PRIV_TABLE, + ROLES_MAPPING_TABLE, + TABLES_MAX // <== always the last +}; +// bits for init_priv_tables +static const int Table_user= 1 << USER_TABLE; +static const int Table_db= 1 << DB_TABLE; +static const int Table_tables_priv= 1 << TABLES_PRIV_TABLE; +static const int Table_columns_priv= 1 << COLUMNS_PRIV_TABLE; +static const int Table_host= 1 << HOST_TABLE; +static const int Table_procs_priv= 1 << PROCS_PRIV_TABLE; +static const int Table_proxies_priv= 1 << PROXIES_PRIV_TABLE; +static const int Table_roles_mapping= 1 << ROLES_MAPPING_TABLE; + +const LEX_STRING acl_table_names[]= // matches enum_acl_tables +{ + { C_STRING_WITH_LEN("user") }, + { C_STRING_WITH_LEN("db") }, + { C_STRING_WITH_LEN("tables_priv") }, + { C_STRING_WITH_LEN("columns_priv") }, + { C_STRING_WITH_LEN("host") }, + { C_STRING_WITH_LEN("procs_priv") }, + { C_STRING_WITH_LEN("proxies_priv") }, + { C_STRING_WITH_LEN("roles_mapping") } +}; + +/** + Initialize a TABLE_LIST array to a set of acl/grant tables. + All tables will be opened with the same lock type, either read or write. + + @retval !0 a pointer to the first initialized element in the TABLE_LIST + @retval 0 replication filters matched. Abort operation, but return OK (!) +*/ +TABLE_LIST *init_priv_tables(THD *thd, TABLE_LIST *tables, + enum thr_lock_type lock_type, int tables_to_open) Nice clean-up, I assume there was a reason not rewrite open_grant_tables() instead.
+{ + int prev= -1; + bzero(tables, sizeof(TABLE_LIST) * TABLES_MAX); + for (int cur=0, mask=1; mask <= tables_to_open; cur++, mask <<= 1) + { + if ((tables_to_open & mask) == 0) + continue; + tables[cur].init_one_table(C_STRING_WITH_LEN("mysql"), + acl_table_names[cur].str, + acl_table_names[cur].length, + acl_table_names[cur].str, lock_type); + tables[cur].open_type= OT_BASE_ONLY; + if (lock_type >= TL_WRITE_ALLOW_WRITE) + tables[cur].updating= 1; + if (cur >= FIRST_OPTIONAL_TABLE) + tables[cur].open_strategy= TABLE_LIST::OPEN_IF_EXISTS; + if (prev != -1) + tables[cur].next_local= tables[cur].next_global= & tables[prev]; + prev= cur; + } + +#ifdef HAVE_REPLICATION + if (lock_type >= TL_WRITE_ALLOW_WRITE && thd->slave_thread && !thd->spcont) + { + /* + GRANT and REVOKE are applied the slave in/exclusion rules as they are + some kind of updates to the mysql.% tables. + */ + Rpl_filter *rpl_filter= thd->system_thread_info.rpl_sql_info->rpl_filter; + if (rpl_filter->is_on() && !rpl_filter->tables_ok(0, tables)) + return NULL; + } +#endif Was it a bug that mysql_grant_role() didn't have this code? open_grant_tables() does updating= 0 after tables_ok(). Is that needed?
...skip...
@@ -5783,8 +5796,9 @@ bool mysql_routine_grant(THD *thd, TABLE_LIST *table_list, bool is_proc, } }
- if (replace_routine_table(thd, grant_name, tables[1].table, *Str, - db_name, table_name, is_proc, rights, + if (no_such_table(tables + PROCS_PRIV_TABLE) || + replace_routine_table(thd, grant_name, tables[PROCS_PRIV_TABLE].table, + *Str, db_name, table_name, is_proc, rights, revoke_grant) != 0) { result= TRUE; Why no_such_table() here?
...skip...
@@ -6250,7 +6232,7 @@ bool mysql_grant(THD *thd, const char *db, List <LEX_USER> &list, ulong db_rights= rights & DB_ACLS; if (db_rights == rights) { - if (replace_db_table(tables[1].table, db, *Str, db_rights, + if (replace_db_table(tables[DB_TABLE].table, db, *Str, db_rights, revoke_grant)) result= -1; } Tabs here (and in a few hunks below).
@@ -6262,9 +6244,11 @@ bool mysql_grant(THD *thd, const char *db, List <LEX_USER> &list, } else if (is_proxy) { - if (replace_proxies_priv_table (thd, tables[1].table, Str, proxied_user, - rights & GRANT_ACL ? TRUE : FALSE, - revoke_grant)) + if (no_such_table(tables + PROXIES_PRIV_TABLE) || + replace_proxies_priv_table (thd, tables[PROXIES_PRIV_TABLE].table, + Str, proxied_user, + rights & GRANT_ACL ? TRUE : FALSE, + revoke_grant)) result= -1; } if (Str->is_role()) Why no_such_table() here?
...skip... Regards, Sergey
Hi, Sergey! Thanks for the review! I won't push yet, need to fix what I said below. Want to see a new patch? On Jun 23, Sergey Vojtovich wrote:
Hi Sergei,
ok to push. A few minor questions inline.
On Mon, Jun 16, 2014 at 09:34:21AM +0200, serg@mariadb.org wrote:
--- a/mysql-test/r/not_embedded_server.result +++ b/mysql-test/r/not_embedded_server.result @@ -1,4 +1,4 @@ -call mtr.add_suppression("Can't open and lock privilege tables: Table 'host' was not locked with LOCK TABLES"); +call mtr.add_suppression("Can't open and lock privilege tables: Table 'roles_mapping' was not locked with LOCK TABLES"); SHOW VARIABLES like 'slave_skip_errors'; Variable_name Value slave_skip_errors OFF Why did it change (here and in other tests)? I didn't notice any table order change in the code.
Because TABLE_LIST's entries are linked in the list in the different order. And this error is issued for the first table in the list. I thought this was fine (as the new behavior is also correct) and didn't want to do something complex only to preserve old error messages. But it just occurred to me that I can trivially do that, simply by reversing the for() loop in the init_priv_tables(). I'm testing this now and restore old error messages if everything will work ok.
...skip...
diff --git a/mysql-test/r/sp-destruct.result b/mysql-test/r/sp-destruct.result index 172e40c..fe68229 100644 --- a/mysql-test/r/sp-destruct.result +++ b/mysql-test/r/sp-destruct.result @@ -128,11 +128,8 @@ CREATE FUNCTION f1() RETURNS INT RETURN 1; RENAME TABLE mysql.procs_priv TO procs_priv_backup; FLUSH TABLE mysql.procs_priv; DROP FUNCTION f1; -ERROR 42S02: Table 'mysql.procs_priv' doesn't exist SHOW WARNINGS; Level Code Message -Error 1146 Table 'mysql.procs_priv' doesn't exist -Warning 1405 Failed to revoke all privileges to dropped routine # Restore the procs_priv table RENAME TABLE procs_priv_backup TO mysql.procs_priv; FLUSH TABLE mysql.procs_priv; Hmm... why?
...skip...
For certain objects it was ok to drop them (no error issued) is some of the privilege tables were missing. For example, one could do DROP USER with no error is, say, proxies_priv or roles_mapping doesn't exist. Which is pretty logical, DROP USER means you remove all existing privileges for this user, if a privilege table doesn't exist - well, it doesn't contain any privileges to drop, does it? :) But dropping a function behaved differently. It returned an error if procs_priv did not exist. But the function was dropped regardless. Which is pretty bad on itself. And inconsistent with how DROP generally behaves. I've fixed this as a side-effect (init_priv_tables() treats all tables equally, it cannot make a table optional for one DROP and mandatory for another DROP).
diff --git a/sql/sql_acl.cc b/sql/sql_acl.cc index 5e8e0e7..442d512 100644 --- a/sql/sql_acl.cc +++ b/sql/sql_acl.cc + + @retval !0 a pointer to the first initialized element in the TABLE_LIST + @retval 0 replication filters matched. Abort operation, but return OK (!) +*/ +TABLE_LIST *init_priv_tables(THD *thd, TABLE_LIST *tables, + enum thr_lock_type lock_type, int tables_to_open) Nice clean-up, I assume there was a reason not rewrite open_grant_tables() instead.
Not really. Thanks, I'll do that.
+#ifdef HAVE_REPLICATION + if (lock_type >= TL_WRITE_ALLOW_WRITE && thd->slave_thread && !thd->spcont) + { + /* + GRANT and REVOKE are applied the slave in/exclusion rules as they are + some kind of updates to the mysql.% tables. + */ + Rpl_filter *rpl_filter= thd->system_thread_info.rpl_sql_info->rpl_filter; + if (rpl_filter->is_on() && !rpl_filter->tables_ok(0, tables)) + return NULL; + } +#endif Was it a bug that mysql_grant_role() didn't have this code?
I suppose so.
open_grant_tables() does updating= 0 after tables_ok(). Is that needed?
I don't think so (also I set updating unconditionally, old code was only doing that in #ifdef HAVE_REPLICATION). This TABLE_LIST::updating is only used in rpl filtering, I believe it doesn't matter what it's set to after the filtering is done.
...skip...
@@ -5783,8 +5796,9 @@ bool mysql_routine_grant(THD *thd, TABLE_LIST *table_list, bool is_proc, } }
- if (replace_routine_table(thd, grant_name, tables[1].table, *Str, - db_name, table_name, is_proc, rights, + if (no_such_table(tables + PROCS_PRIV_TABLE) || + replace_routine_table(thd, grant_name, tables[PROCS_PRIV_TABLE].table, + *Str, db_name, table_name, is_proc, rights, revoke_grant) != 0) { result= TRUE; Why no_such_table() here?
Yeah... See above - it's ok for a privilege table to be missing when some object is DROP'ped or a privilege is revoked. But when a privilege is granted or updated in some way, we must ensure that a privilege table exists. Because init_priv_tables() doesn't do that for all tables, it has to be done manually now. Old code was achieving that by not using TABLE_LIST::OPEN_IF_EXISTS for mandatory tables. It was possible to have a different set of mandatory tables in every function. And that also resulted in the incorrect DROP FUNCTION behavior.
...skip...
Regards, Sergei
Hi Sergei, On Mon, Jun 23, 2014 at 10:01:58AM +0200, Sergei Golubchik wrote:
Hi, Sergey!
Thanks for the review! I won't push yet, need to fix what I said below. Want to see a new patch? Thanks for your clarifications. I believe there is no need to review new patch.
...skip... Regards, Sergey
participants (2)
-
Sergei Golubchik -
Sergey Vojtovich