Hi Sergei, On 07/26/2015 02:13 AM, Sergei Golubchik wrote:
Hi, Alexander!
On Jul 24, Alexander Barkov wrote:
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.
Not really, I couldn't understand what you're trying to do this time :( Besides it's 32K of changes in 10.0-GA, and for what? Corner case CONNECT issue. Could you try to come up with a less intrusive solution, please?
Let me try to explain. The idea is very simple, really. ... Does this explanation help?
Yes, thanks. But it still affects the rather hot execution path - opening of a table - for every user and every table. While it is only needed when a user opens a CONNECT table via a view with SQL SECURITY DEFINER. See? In the absolute majority of cases, practically all of them, your patch only adds an overhead.
Instead, I suggest another patch - this is based on your first fix for this bug:
diff --git a/storage/connect/ha_connect.cc b/storage/connect/ha_connect.cc index c2fb648..0a024b0 100644 --- a/storage/connect/ha_connect.cc +++ b/storage/connect/ha_connect.cc @@ -4020,7 +4020,24 @@ bool ha_connect::check_privileges(THD *thd, PTOS options, case TAB_MAC: case TAB_WMI: case TAB_OEM: - return check_access(thd, FILE_ACL, db, NULL, NULL, 0, 0); + /* + If table or table->mdl_ticket is NULL - it's a DLL, e.g. CREATE TABLE. + if the table has an MDL_EXCLUSIVE lock - it's a DDL too, e.g. the + insert step of CREATE ... SELECT. + + Otherwise it's a DML, the table was normally opened, locked, + privilege were already checked, and table->grant.privilege is set. + With SQL SECURITY DEFINER, table->grant.privilege has definer's privileges. + */ + if (!table || !table->mdl_ticket || table->mdl_ticket->get_type() == MDL_EXCLUSIVE) + return check_access(thd, FILE_ACL, db, NULL, NULL, 0, 0); + if (table->grant.privilege & FILE_ACL) + return false; + return true;
// This is temporary until a solution is found case TAB_TBL:
It passes your test case. In fact, your first fix passes it too :)
Yeah, I guess my condition that switches between checking grant.privilege and doing check_access() was effectively the same. But your version looks simpler.
This one also passes the additional test I've added - where a user can access the table, but view's definer cannot:
--connection default CREATE DEFINER=user@localhost SQL SECURITY DEFINER VIEW v1_baddefiner AS SELECT * FROM t1; --error ER_ACCESS_DENIED_ERROR SELECT * FROM v1_baddefiner;
This is a nice idea. Thanks. I just tried this: # Run this as root: DROP TABLE IF EXISTS t1; DROP PROCEDURE IF EXISTS p1; CREATE PROCEDURE p1() SQL SECURITY DEFINER CREATE TABLE t1 (a INT) ENGINE=CONNECT TABLE_TYPE=fix FILE_NAME='t1.fix'; # Run this as a user with no FILE_ACL CALL p1(); and it also worked as expected, CALL p1() succeeded.
See the commit mail with the complete patch.
The patch is Ok. Thanks for help with this. Can you please push this? I'm on vacation starting from tomorrow. Thanks.
Regards, Sergei