maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander!

On Apr 26, Alexander Barkov wrote:
> 
> The patch looks fine.
> 
> I'd suggest two things.
> 
> 1. Don't use "default"
> Can you please list all possible type value instead of "default"?
> Otherwise, when a new table type is added the next time, it will be
> easier to forget to update check_cached_privileges() accordingly.
> 
> 2. Try to avoid duplicate code.
> 
> Btw, I'd probably try to remove the duplicate switch,
> by adding a new method like this:
> 
> enum Extra_acl
> {
>    CONNECT_SECURE_FILE_PATH= 1,
>    CONNECT_FILE_ACL=         1<<1;
> }
> 
> int ha_connect::extra_acl_needed(THD *thd, PTOS options)
> {
>    ...
>    switch (GetRealType(options)) {
>    ...
>    case TAB_DOS:
>    case TAB_FIX:
>    case TAB_BIN:
>    case TAB_CSV:
>    case TAB_FMT:
>    case TAB_DBF:
>    case TAB_XML:
>    case TAB_INI:
>    case TAB_VEC:
>    case TAB_JSON:
>      return (options->filename && *options->filename) ?
>              CONNECT_SECURE_FILE_PATH : 0;
>    case TAB_ODBC:
>    case TAB_MYSQL:
>    case TAB_DIR:
>    case TAB_MAC:
>    case TAB_WMI:
>    case TAB_OEM:
> #ifdef NO_EMBEDDED_ACCESS_CHECKS
>      return 0;
> #endif
>      return CONNECT_FILE_ACL;
>    ...
>    }
>    ...
> }
> 
> And then reuse it in both ha_connect::check_privilege()
> and ha_connect::check_cached_privileges().

I wanted to avoid doing the switch twice in check_privilege().
But ok, I'll change it (I'll simply reuse check_privilege, there will be
no duplicate code this way).

> The latter would look about like this:
> 
> bool ha_connect::check_cached_privileges(THD *thd)
> {
>    PTOS  options= GetTableOptionStruct();
>    if (!(extra_acl_needed(options) & CONNECT_FILE_ACL) ||
>        (table->grant.privilege & FILE_ACL))
>      return false;
>    issue_access_denied_error(thd);
>    return true;
> }

Regards,
Sergei
Chief Architect MariaDB
and security@xxxxxxxxxxx