maria-developers team mailing list archive

[Sergei Golubchik <serg@xxxxxxxxxxx>]

Hi, Alexander!

On Feb 24, Alexander Barkov wrote:
> 
> There is only one problem with that. In case of embedded server
> table->grant.privilege is always 0, because the embedded version
> of check_table_access() is just an empty function.
> 
> This change in sql/handler.cc, in handler::ha_external_lock() helps:
> 
> +#ifdef NO_EMBEDDED_ACCESS_CHECKS
> +  table->grant.privilege= ~NO_ACCESS;
> +#endif

May be, it'd be better to do it in check_table_access() ?

With a comment "plugins (e.g. CONNECT engine) should not depend on
whether embedded is built with NO_EMBEDDED_ACCESS_CHECKS or without".

> === modified file 'sql/handler.cc'
> --- sql/handler.cc	2015-01-21 11:03:02 +0000
> +++ sql/handler.cc	2015-02-24 11:47:05 +0000
> @@ -5873,6 +5873,9 @@ int handler::ha_external_lock(THD *thd,
>  
>    ha_statistic_increment(&SSV::ha_external_lock_count);
>  
> +#ifdef NO_EMBEDDED_ACCESS_CHECKS
> +  table->grant.privilege= ~NO_ACCESS;
> +#endif

See above.
If you could't put this in check_table_access(), then, at least, add
this comment here.

>    /*
>      We cache the table flags if the locking succeeded. Otherwise, we
>      keep them as they were when they were fetched in ha_open().
> 
> === modified file 'storage/connect/ha_connect.cc'
> --- storage/connect/ha_connect.cc	2015-02-11 20:39:41 +0000
> +++ storage/connect/ha_connect.cc	2015-02-24 12:03:25 +0000
> @@ -3922,7 +3922,21 @@ int ha_connect::delete_all_rows()
>  } // end of delete_all_rows
>  
>  
> -bool ha_connect::check_privileges(THD *thd, PTOS options, char *dbn)
> +/**
> +  Check privileges.
> +  @param THD                   - Current thread
> +  @param options               - Connect table options
> +  @param dbn                   - database name
> +  @param using_table_privilege - whether check table->grant.privilege,
> +                                 or execute check_access(FILE_ACL).
> +
> +  Using table->grant.privilege is important in cases when we need to take into
> +  account privileges of the VIEW definer when accessing to a view created with
> +    "CREATE VIEW v1 SQL SECURITY DEFINER".
> +  See ha_connect::check_privileges_external_lock() for details.
> +*/
> +bool ha_connect::check_privileges(THD *thd, PTOS options,
> +                                  char *dbn, bool using_table_privilege)
>  {
>    const char *db= (dbn && *dbn) ? dbn : NULL;
>    TABTYPE     type=GetRealType(options);
> @@ -4143,6 +4180,67 @@ MODE ha_connect::CheckMode(PGLOBAL g, TH
>    return newmode;
>  } // end of check_mode
>  
> +
> +/**
> +  A check_privilege() wrapper for external_lock().
> +  Decides if check_privilege():
> +  - should test table->grant.privilege for FILE_ACL
> +  - or should call check_access(FILE_ACL)
> +  depending on the current SQL command and lock type.
> +*/
> +bool ha_connect::check_privileges_external_lock(PGLOBAL g, THD *thd,
> +                                                PTOS options, int lock_type)
> +{
> +  bool use_table_priv;
> +  switch (thd_sql_command(thd))
> +  {
> +  case SQLCOM_SELECT:
> +  case SQLCOM_UPDATE:
> +  case SQLCOM_INSERT:
> +  case SQLCOM_DELETE:
> +  case SQLCOM_REPLACE:
> +  case SQLCOM_LOAD:
> +    use_table_priv= true;              // use table->grant.privilege
> +    break;
> +
> +  case SQLCOM_CREATE_TABLE:
> +  case SQLCOM_INSERT_SELECT:
> +  case SQLCOM_REPLACE_SELECT:
> +  case SQLCOM_UPDATE_MULTI:
> +  case SQLCOM_DELETE_MULTI:
> +    /*
> +      CREATE TABLE target_table AS SELECT * FROM source_table;
> +      INSERT INTO  target_table SELECT * FROM source_table;
> +      REPLACE INTO target_table SELECT * FROM source_table;    
> +      UPDATE target_table,source_table SET target_table.column=xxx WHERE ...;
> +      DELETE target_table FROM target_table,source_table WHERE ...;
> +
> +      If we're working with "source_table", use table->grant.privilege.
> +      If we're working with "target_table", use check_access().
> +    */
> +    use_table_priv= lock_type != F_WRLCK;

I don't quite understand that. Why use_table_priv is FALSE for these
commands? Like, why it's true for SQLCOM_INSERT, but false for
SQLCOM_INSERT_SELECT? True for SQLCOM_UPDATE, false for
SQLCOM_UPDATE_MULTI?

Other cases below aren't clear either.
Could you explain the rule - when one should use table->grant.privilege
and when check_access()? I mean, not a list of cases, but a general
underlying rule.

> +    break;
> +
> +  case SQLCOM_TRUNCATE:
> +  case SQLCOM_LOCK_TABLES:
> +  case SQLCOM_DROP_TABLE:
> +  case SQLCOM_RENAME_TABLE:
> +  case SQLCOM_CREATE_VIEW:
> +  case SQLCOM_DROP_VIEW:
> +  case SQLCOM_ALTER_TABLE:
> +  case SQLCOM_DROP_INDEX:
> +  case SQLCOM_CREATE_INDEX:
> +  case SQLCOM_OPTIMIZE:
> +    use_table_priv= false;             // use check_access()
> +    break;
> +  default:
> +    report_unsupported_sql_command(g, thd);
> +    return true;                       // Something went wrong, deny access.
> +  }
> +  return check_privileges(thd, options, table->s->db.str, use_table_priv);
> +}
> +
> +
>  int ha_connect::start_stmt(THD *thd, thr_lock_type lock_type)
>  {
>    int     rc= 0;
> @@ -4614,7 +4712,7 @@ int ha_connect::delete_or_rename_table(c
>      if (!open_table_def(thd, share)) {
>        // Now we can work
>        if ((pos= share->option_struct)) {
> -        if (check_privileges(thd, pos, db))
> +        if (check_privileges(thd, pos, db, false))
>            rc= HA_ERR_INTERNAL_ERROR;         // ???
>          else
>            if (IsFileType(GetRealType(pos)) && !pos->filename)
> @@ -5592,7 +5690,7 @@ int ha_connect::create(const char *name,
>      DBUG_RETURN(HA_ERR_INTERNAL_ERROR);
>    } // endif ttp
>  
> -  if (check_privileges(thd, options, GetDBfromName(name)))
> +  if (check_privileges(thd, options, GetDBfromName(name), false))
>      DBUG_RETURN(HA_ERR_INTERNAL_ERROR);
>  
>    inward= IsFileType(type) && !options->filename;

Regards,
Sergei