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".
=== 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.
+ 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