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 _______________________________________________ Mailing list: https://launchpad.net/~maria-developers Post to : maria-developers@lists.launchpad.net Unsubscribe : https://launchpad.net/~maria-developers More help : https://help.launchpad.net/ListHelp