maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

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);

Regards,
Sergei