maria-developers team mailing list archive
Mailing list archive
Re: PLEASE REVIEW: (MDEV-7574) Security definer views don't work with CONNECT ODBC tables
On 07/26/2015 02:13 AM, Sergei Golubchik wrote:
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,
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
diff --git a/storage/connect/ha_connect.cc b/storage/connect/ha_connect.cc
index c2fb648..0a024b0 100644
@@ -4020,7 +4020,24 @@ bool ha_connect::check_privileges(THD *thd, PTOS options,
- 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
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:
CREATE DEFINER=user@localhost SQL SECURITY DEFINER VIEW v1_baddefiner AS SELECT * FROM t1;
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
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.