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 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().
return table->grant.privilege & FILE_ACL; // respect view's definer
return check_access(thd, FILE_ACL, db, NULL, NULL, 0, 0);