Hi Sergei, On 02/19/2015 07:12 PM, Sergei Golubchik wrote:
Hi, Alexander!
On Feb 17, Alexander Barkov wrote:
From what I understood, FILE_ACL is written (among the other privileges) into thd->security_ctx.privilege in TABLE_LIST::prepare_security(). In case of a DEFINER view, thd->security_ctx.privilege is filled exactly with the definer privileges, and to the invoker privileges otherwise.
So inside ha_connect::check_privileges() the fact that there is FILE_ACL in thd->security_ctx.privilege means that TABLE_LIST::prepare_security() was previously called and FILE_ACL is set to DEFINER or INVOKER, according to the view definition. This is exactly what we need.
Agree, looks good so far :)
I'm not sure about the opposite: if there is no FILE_ACL in thd->security_ctx.privilege, what does it mean? Does it mean that there is no FILE_ACL for the effective user? Or can it also mean that TABLE_LIST::prepare_security() was not called?
As far as I can see, TABLE_LIST::prepare_security() is always called for a view that was successfully opened. That is, no FILE_ACL bit means that there is no FILE privilege.
=== modified file 'storage/connect/ha_connect.cc' --- storage/connect/ha_connect.cc 2015-01-20 00:21:56 +0000 +++ storage/connect/ha_connect.cc 2015-02-17 13:46:34 +0000 @@ -3865,6 +3865,8 @@ bool ha_connect::check_privileges(THD *t case TAB_MAC: case TAB_WMI: case TAB_OEM: + if (table && (table->grant.privilege & FILE_ACL)) + return false; return check_access(thd, FILE_ACL, db, NULL, NULL, 0, 0);
I don't like that you use both approaches to check the privileges. This just looks wrong. It should rather be
* if it's called from ::external_lock() - use table->grant.privilege. * otherwise (::create() or ::delete_or_rename_table()) - don't use table->grant.privilege, only use check_access().
Something like
if (called_from_external_lock) return table->grant.privilege & FILE_ACL; // respect view's definer else return check_access(thd, FILE_ACL, db, NULL, NULL, 0, 0);
Thanks for the suggestion! Please find a new version attached. It now uses thd_sql_command() and lock_type to decide whether to use table->grant.privilege or check_access(). 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 With this change "mtr --embedded" passes all tests, including those I added in this patch. Possibly, a there is a better way to do this. Please suggest. Thanks.
Regards, Sergei