Hi Sergei, Sorry for delay, I was busy with 10.1 issues. Thanks for review. A new patch is attached. This is a major rewrite since last time. I think the code now looks much easier to understand. Please see comments below. On 04/29/2015 06:44 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Feb 24, Alexander Barkov wrote:
There is only one problem with that. In case of embedded server table->grant.privilege is always 0, because the embedded version of check_table_access() is just an empty function.
This change in sql/handler.cc, in handler::ha_external_lock() helps:
+#ifdef NO_EMBEDDED_ACCESS_CHECKS + table->grant.privilege= ~NO_ACCESS; +#endif
May be, it'd be better to do it in check_table_access() ?
With a comment "plugins (e.g. CONNECT engine) should not depend on whether embedded is built with NO_EMBEDDED_ACCESS_CHECKS or without".
Thanks for the idea. Note, check_table_access() is not called in case of some SQLCOM_XXXX. So I slightly extended your idea and added this code into st_select_lex::add_table_to_list() instead. This is the place where TABLE_LIST is first initialized.
=== modified file 'sql/handler.cc' --- sql/handler.cc 2015-01-21 11:03:02 +0000 +++ sql/handler.cc 2015-02-24 11:47:05 +0000 @@ -5873,6 +5873,9 @@ int handler::ha_external_lock(THD *thd,
ha_statistic_increment(&SSV::ha_external_lock_count);
+#ifdef NO_EMBEDDED_ACCESS_CHECKS + table->grant.privilege= ~NO_ACCESS; +#endif
See above. If you could't put this in check_table_access(), then, at least, add this comment here.
/* We cache the table flags if the locking succeeded. Otherwise, we keep them as they were when they were fetched in ha_open().
=== modified file 'storage/connect/ha_connect.cc' --- storage/connect/ha_connect.cc 2015-02-11 20:39:41 +0000 +++ storage/connect/ha_connect.cc 2015-02-24 12:03:25 +0000 @@ -3922,7 +3922,21 @@ int ha_connect::delete_all_rows() } // end of delete_all_rows
-bool ha_connect::check_privileges(THD *thd, PTOS options, char *dbn) +/** + Check privileges. + @param THD - Current thread + @param options - Connect table options + @param dbn - database name + @param using_table_privilege - whether check table->grant.privilege, + or execute check_access(FILE_ACL). + + Using table->grant.privilege is important in cases when we need to take into + account privileges of the VIEW definer when accessing to a view created with + "CREATE VIEW v1 SQL SECURITY DEFINER". + See ha_connect::check_privileges_external_lock() for details. +*/ +bool ha_connect::check_privileges(THD *thd, PTOS options, + char *dbn, bool using_table_privilege) { const char *db= (dbn && *dbn) ? dbn : NULL; TABTYPE type=GetRealType(options); @@ -4143,6 +4180,67 @@ MODE ha_connect::CheckMode(PGLOBAL g, TH return newmode; } // end of check_mode
+ +/** + A check_privilege() wrapper for external_lock(). + Decides if check_privilege(): + - should test table->grant.privilege for FILE_ACL + - or should call check_access(FILE_ACL) + depending on the current SQL command and lock type. +*/ +bool ha_connect::check_privileges_external_lock(PGLOBAL g, THD *thd, + PTOS options, int lock_type) +{ + bool use_table_priv; + switch (thd_sql_command(thd)) + { + case SQLCOM_SELECT: + case SQLCOM_UPDATE: + case SQLCOM_INSERT: + case SQLCOM_DELETE: + case SQLCOM_REPLACE: + case SQLCOM_LOAD: + use_table_priv= true; // use table->grant.privilege + break; + + case SQLCOM_CREATE_TABLE: + case SQLCOM_INSERT_SELECT: + case SQLCOM_REPLACE_SELECT: + case SQLCOM_UPDATE_MULTI: + case SQLCOM_DELETE_MULTI: + /* + CREATE TABLE target_table AS SELECT * FROM source_table; + INSERT INTO target_table SELECT * FROM source_table; + REPLACE INTO target_table SELECT * FROM source_table; + UPDATE target_table,source_table SET target_table.column=xxx WHERE ...; + DELETE target_table FROM target_table,source_table WHERE ...; + + If we're working with "source_table", use table->grant.privilege. + If we're working with "target_table", use check_access(). + */ + use_table_priv= lock_type != F_WRLCK;
I don't quite understand that. Why use_table_priv is FALSE for these commands? Like, why it's true for SQLCOM_INSERT, but false for SQLCOM_INSERT_SELECT? True for SQLCOM_UPDATE, false for SQLCOM_UPDATE_MULTI?
Other cases below aren't clear either. Could you explain the rule - when one should use table->grant.privilege and when check_access()? I mean, not a list of cases, but a general underlying rule.
The main problem was that the result of check_access(), which is done in sql_parse.cc through a number of various SQLCOM_XXX dependent functions (e.g. insert_precheck), and which extracts the privileges for the current effective user (which can be invoker or definer, depending on SQL SECURITY clause, e.g. in case of VIEW), was not always available in external_lock() in table.grant->privileges (which was just 0 in some case) So I composed this code heuristically, just testing what works. The goal was: - to reuse table.grant->privileges when it has a valid value that originates from check_access() made in sql_parse.cc - to call its own check_access() when table.grant->privileges is just set to 0. Of course, using check_access() could be be wrong, because it could check privileges for invoker instead of definer in some case. This problem would be important for calling the affected statements from stored procedures. There were no general rule, because availability of the check_access() result heavily depended on the exact SQLCOM_XXXX command. So that was just a switch() that worked somehow. Now I rewrote the code to make the original check_access() result be available in much more cases and extended the test a lot. So external_lock() now uses table.grant->privilege for all SQLCOM_XXXX. Note, check_access() is still called in ha_connect::create() and ha_connect::delete_or_rename_table(). I'd like to get rid of this eventually, in a separate change. You'll find related comments in the patch. Thanks.
+ break; + + case SQLCOM_TRUNCATE: + case SQLCOM_LOCK_TABLES: + case SQLCOM_DROP_TABLE: + case SQLCOM_RENAME_TABLE: + case SQLCOM_CREATE_VIEW: + case SQLCOM_DROP_VIEW: + case SQLCOM_ALTER_TABLE: + case SQLCOM_DROP_INDEX: + case SQLCOM_CREATE_INDEX: + case SQLCOM_OPTIMIZE: + use_table_priv= false; // use check_access() + break; + default: + report_unsupported_sql_command(g, thd); + return true; // Something went wrong, deny access. + } + return check_privileges(thd, options, table->s->db.str, use_table_priv); +} + + int ha_connect::start_stmt(THD *thd, thr_lock_type lock_type) { int rc= 0; @@ -4614,7 +4712,7 @@ int ha_connect::delete_or_rename_table(c if (!open_table_def(thd, share)) { // Now we can work if ((pos= share->option_struct)) { - if (check_privileges(thd, pos, db)) + if (check_privileges(thd, pos, db, false)) rc= HA_ERR_INTERNAL_ERROR; // ??? else if (IsFileType(GetRealType(pos)) && !pos->filename) @@ -5592,7 +5690,7 @@ int ha_connect::create(const char *name, DBUG_RETURN(HA_ERR_INTERNAL_ERROR); } // endif ttp
- if (check_privileges(thd, options, GetDBfromName(name))) + if (check_privileges(thd, options, GetDBfromName(name), false)) DBUG_RETURN(HA_ERR_INTERNAL_ERROR);
inward= IsFileType(type) && !options->filename;
Regards, Sergei