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@mariadb.org