WL#2486 - natural/using joins according to SQL:2003

Post-review fixes that simplify the way access rights
are checked during name resolution and factor out all
entry points to check access rights into one single
function.


sql/item.cc:
  Simplfied find_field_in_table - factored out all acces right checks
  into a separate function.
sql/mysql_priv.h:
  Simplified the way we control whether to perform access right
  checks for columns.
sql/sql_acl.cc:
  - Added new functon check_column_grant_in_table_ref that serves as a single
    point of entry to check access rights during name resolution for different
    kinds of table references.
  - Moved check_grant_column_in_sctx to sql_acl.cc where it
    logically belongs.
  - Removed the parameter check_grants - it is checked before
    calling the function.
sql/sql_acl.h:
  - Added new function check_column_grant_in_table_ref.
  - Made check_grant_column_in_sctx available to other modules.
sql/sql_base.cc:
  - Factored out all code that check access rights for columns during
    name resolution into one function - check_column_grant_in_table_ref.
  - Moved check_grant_column_in_sctx to sql_acl.cc where it
    logically belongs.
  - Removed the parameter check_grants - it is checked before
    calling the function.
sql/table.cc:
  Removed code that duplicates the functionality of
  check_column_grant_in_table_ref, and called directly
  that function.
sql/table.h:
  check_grants method is replaced by more general
  check_column_grant_in_table_ref.

sql/item.cc

@@ -5179,7 +5179,7 @@ void Item_trigger_field::setup_field(THD *thd, TABLE *table)
     set field_idx properly.
   */
   (void)find_field_in_table(thd, table, field_name, (uint) strlen(field_name),
-                            0, 0, &field_idx, 0);
+                            0, &field_idx);
   thd->set_query_id= save_set_query_id;
   triggers= table->triggers;
 }

sql/mysql_priv.h

@@ -794,14 +794,12 @@ find_field_in_table_ref(THD *thd, TABLE_LIST *table_list,
                         const char *name, uint length,
                         const char *item_name, const char *db_name,
                         const char *table_name, Item **ref,
-                        bool check_grants_table, bool check_grants_view,
-                        bool allow_rowid, uint *cached_field_index_ptr,
+                        bool check_privileges, bool allow_rowid,
+                        uint *cached_field_index_ptr,
                         bool register_tree_change, TABLE_LIST **actual_table);
 Field *
-find_field_in_table(THD *thd, TABLE *table, const char *name,
-                    uint length, bool check_grants, bool allow_rowid,
-                    uint *cached_field_index_ptr,
-                    Security_context *sctx);
+find_field_in_table(THD *thd, TABLE *table, const char *name, uint length,
+                    bool allow_rowid, uint *cached_field_index_ptr);
 
 #ifdef HAVE_OPENSSL
 #include <openssl/des.h>

sql/sql_acl.cc

@@ -2763,7 +2763,7 @@ bool mysql_table_grant(THD *thd, TABLE_LIST *table_list,
         Field *f=find_field_in_table_ref(thd, table_list, column->column.ptr(),
                                          column->column.length(),
                                          column->column.ptr(), NULL, NULL,
-                                         0, 1, 1, 0,
+                                         NULL, TRUE, FALSE,
                                          &unused_field_idx, FALSE, &dummy);
         if (f == (Field*)0)
         {
@@ -3617,11 +3617,28 @@ err:
 }
 
 
+/*
+  Check column rights in given security context
+
+  SYNOPSIS
+    check_grant_column()
+    thd                  thread handler
+    grant                grant information structure
+    db_name              db name
+    table_name           table  name
+    name                 column name
+    length               column name length
+    sctx                 security context
+
+  RETURN
+    FALSE OK
+    TRUE  access denied
+*/
+
 bool check_grant_column(THD *thd, GRANT_INFO *grant,
 			const char *db_name, const char *table_name,
-			const char *name, uint length, uint show_tables)
+			const char *name, uint length,  Security_context *sctx)
 {
-  Security_context *sctx= thd->security_ctx;
   GRANT_TABLE *grant_table;
   GRANT_COLUMN *grant_column;
   ulong want_access= grant->want_privilege & ~grant->privilege;
@@ -3652,28 +3669,74 @@ bool check_grant_column(THD *thd, GRANT_INFO *grant,
     rw_unlock(&LOCK_grant);
     DBUG_RETURN(0);
   }
-#ifdef NOT_USED
-  if (show_tables && (grant_column || grant->privilege & COL_ACLS))
-  {
-    rw_unlock(&LOCK_grant);			/* purecov: deadcode */
-    DBUG_RETURN(0);				/* purecov: deadcode */
-  }
-#endif
 
 err:
   rw_unlock(&LOCK_grant);
-  if (!show_tables)
+  char command[128];
+  get_privilege_desc(command, sizeof(command), want_access);
+  my_error(ER_COLUMNACCESS_DENIED_ERROR, MYF(0),
+           command,
+           sctx->priv_user,
+           sctx->host_or_ip,
+           name,
+           table_name);
+  DBUG_RETURN(1);
+}
+
+
+/*
+  Check the access right to a column depending on the type of table.
+
+  SYNOPSIS
+    check_column_grant_in_table_ref()
+    thd              thread handler
+    table_ref        table reference where to check the field
+    name             name of field to check
+    length           length of name
+
+  DESCRIPTION
+    Check the access rights to a column depending on the type of table
+    reference where the column is checked. The function provides a
+    generic interface to check column access rights that hides the
+    heterogeneity of the column representation - whether it is a view
+    or a stored table colum.
+
+  RETURN
+    FALSE OK
+    TRUE  access denied
+*/
+
+bool check_column_grant_in_table_ref(THD *thd, TABLE_LIST * table_ref,
+                                     const char *name, uint length)
+{
+  GRANT_INFO *grant;
+  const char *db_name;
+  const char *table_name;
+  Security_context *sctx= test(table_ref->security_ctx) ?
+                          table_ref->security_ctx : thd->security_ctx;
+
+  if (table_ref->view || table_ref->field_translation)
   {
-    char command[128];
-    get_privilege_desc(command, sizeof(command), want_access);
-    my_error(ER_COLUMNACCESS_DENIED_ERROR, MYF(0),
-             command,
-             sctx->priv_user,
-             sctx->host_or_ip,
-             name,
-             table_name);
+    /* View or derived information schema table. */
+    grant= &(table_ref->grant);
+    db_name= table_ref->view_db.str;
+    table_name= table_ref->view_name.str;
   }
-  DBUG_RETURN(1);
+  else
+  {
+    /* Normal or temporary table. */
+    TABLE *table= table_ref->table;
+    grant= &(table->grant);
+    db_name= table->s->db;
+    table_name= table->s->table_name;
+  }
+
+  if (grant->want_privilege)
+    return check_grant_column(thd, grant, db_name, table_name, name,
+                              length, sctx);
+  else
+    return FALSE;
+
 }
 
 

sql/sql_acl.h

@@ -204,7 +204,9 @@ bool check_grant(THD *thd, ulong want_access, TABLE_LIST *tables,
 		 uint show_command, uint number, bool dont_print_error);
 bool check_grant_column (THD *thd, GRANT_INFO *grant,
 			 const char *db_name, const char *table_name,
-			 const char *name, uint length, uint show_command=0);
+			 const char *name, uint length, Security_context *sctx);
+bool check_column_grant_in_table_ref(THD *thd, TABLE_LIST * table_ref,
+                                     const char *name, uint length);
 bool check_grant_all_columns(THD *thd, ulong want_access, GRANT_INFO *grant,
                              const char* db_name, const char *table_name,
                              Field_iterator *fields);

sql/sql_base.cc

@@ -2683,47 +2683,6 @@ static void update_field_dependencies(THD *thd, Field *field, TABLE *table)
 }
 
 
-#ifndef NO_EMBEDDED_ACCESS_CHECKS
-/*
-  Check column rights in given security context
-
-  SYNOPSIS
-    check_grant_column_in_sctx()
-    thd                  thread handler
-    grant                grant information structure
-    db                   db name
-    table                table  name
-    name                 column name
-    length               column name length
-    check_grants         need to check grants
-    sctx                 0 or security context
-
-  RETURN
-    FALSE OK
-    TRUE  access denied
-*/
-
-static bool check_grant_column_in_sctx(THD *thd, GRANT_INFO *grant,
-                                       const char *db, const char *table,
-                                       const char *name, uint length,
-                                       bool check_grants,
-                                       Security_context *sctx)
-{
-  if (!check_grants)
-    return FALSE;
-  Security_context *save_security_ctx= thd->security_ctx;
-  bool res;
-  if (sctx)
-  {
-    thd->security_ctx= sctx;
-  }
-  res= check_grant_column(thd, grant, db, table, name, length);
-  thd->security_ctx= save_security_ctx;
-  return res;
-}
-#endif
-
-
 /*
   Find a field by name in a view that uses merge algorithm.
 
@@ -2736,7 +2695,6 @@ static bool check_grant_column_in_sctx(THD *thd, GRANT_INFO *grant,
     item_name                   name of item if it will be created (VIEW)
     ref				expression substituted in VIEW should be passed
                                 using this reference (return view_ref_found)
-    check_grants		do check columns grants for view?
     register_tree_change        TRUE if ref is not stack variable and we
                                 need register changes in item tree
 
@@ -2750,7 +2708,7 @@ static Field *
 find_field_in_view(THD *thd, TABLE_LIST *table_list,
                    const char *name, uint length,
                    const char *item_name, Item **ref,
-                   bool check_grants, bool register_tree_change)
+                   bool register_tree_change)
 {
   DBUG_ENTER("find_field_in_view");
   DBUG_PRINT("enter",
@@ -2766,14 +2724,6 @@ find_field_in_view(THD *thd, TABLE_LIST *table_list,
   {
     if (!my_strcasecmp(system_charset_info, field_it.name(), name))
     {
-#ifndef NO_EMBEDDED_ACCESS_CHECKS
-      if (check_grant_column_in_sctx(thd, &table_list->grant,
-                                     table_list->view_db.str,
-                                     table_list->view_name.str, name, length,
-                                     check_grants,
-                                     table_list->security_ctx))
-        DBUG_RETURN(WRONG_GRANT);
-#endif
       // in PS use own arena or data will be freed after prepare
       if (register_tree_change)
         arena= thd->activate_stmt_arena_if_needed(&backup);
@@ -2822,7 +2772,6 @@ find_field_in_view(THD *thd, TABLE_LIST *table_list,
     length		 [in]  length of name
     ref                  [in/out] if 'name' is resolved to a view field, ref is
                                set to point to the found view field
-    check_grants	 [in]  do check columns grants?
     register_tree_change [in]  TRUE if ref is not stack variable and we
                                need register changes in item tree
     actual_table         [out] the original table reference where the field
@@ -2843,8 +2792,7 @@ find_field_in_view(THD *thd, TABLE_LIST *table_list,
 
 static Field *
 find_field_in_natural_join(THD *thd, TABLE_LIST *table_ref, const char *name,
-                           uint length, Item **ref, bool check_grants,
-                           bool register_tree_change,
+                           uint length, Item **ref, bool register_tree_change,
                            TABLE_LIST **actual_table)
 {
   List_iterator_fast<Natural_join_column>
@@ -2869,11 +2817,6 @@ find_field_in_natural_join(THD *thd, TABLE_LIST *table_ref, const char *name,
       break;
   }
 
-#ifndef NO_EMBEDDED_ACCESS_CHECKS
-  if (check_grants && nj_col->check_grants(thd, name, length))
-    DBUG_RETURN(WRONG_GRANT);
-#endif
-
   if (nj_col->view_field)
   {
     Item *item;
@@ -2929,7 +2872,6 @@ find_field_in_natural_join(THD *thd, TABLE_LIST *table_ref, const char *name,
     table			table where to search for the field
     name			name of field
     length			length of name
-    check_grants		do check columns grants?
     allow_rowid			do allow finding of "_rowid" field?
     cached_field_index_ptr	cached position in field list (used to speedup
                                 lookup for fields in prepared tables)
@@ -2941,9 +2883,7 @@ find_field_in_natural_join(THD *thd, TABLE_LIST *table_ref, const char *name,
 
 Field *
 find_field_in_table(THD *thd, TABLE *table, const char *name, uint length,
-                    bool check_grants, bool allow_rowid,
-                    uint *cached_field_index_ptr,
-                    Security_context *sctx)
+                    bool allow_rowid, uint *cached_field_index_ptr)
 {
   Field **field_ptr, *field;
   uint cached_field_index= *cached_field_index_ptr;
@@ -2982,13 +2922,6 @@ find_field_in_table(THD *thd, TABLE *table, const char *name, uint length,
 
   update_field_dependencies(thd, field, table);
 
-#ifndef NO_EMBEDDED_ACCESS_CHECKS
-  if (check_grant_column_in_sctx(thd, &table->grant,
-                                 table->s->db, table->s->table_name,
-                                 name, length,
-                                 check_grants, sctx))
-    field= WRONG_GRANT;
-#endif
   DBUG_RETURN(field);
 }
 
@@ -3007,8 +2940,7 @@ find_field_in_table(THD *thd, TABLE *table, const char *name, uint length,
     table_name             [in]  optional table name that qualifies the field
     ref		       [in/out] if 'name' is resolved to a view field, ref
                                  is set to point to the found view field
-    check_grants_table	   [in]  do check columns grants for table?
-    check_grants_view	   [in]  do check columns grants for view?
+    check_privileges       [in]  check privileges
     allow_rowid		   [in]  do allow finding of "_rowid" field?
     cached_field_index_ptr [in]  cached position in field list (used to
                                  speedup lookup for fields in prepared tables)
@@ -3041,8 +2973,8 @@ find_field_in_table_ref(THD *thd, TABLE_LIST *table_list,
                         const char *name, uint length,
                         const char *item_name, const char *db_name,
                         const char *table_name, Item **ref,
-                        bool check_grants_table, bool check_grants_view,
-                        bool allow_rowid, uint *cached_field_index_ptr,
+                        bool check_privileges, bool allow_rowid,
+                        uint *cached_field_index_ptr,
                         bool register_tree_change, TABLE_LIST **actual_table)
 {
   Field *fld;
@@ -3087,8 +3019,7 @@ find_field_in_table_ref(THD *thd, TABLE_LIST *table_list,
   if (table_list->field_translation)
   {
     /* 'table_list' is a view or an information schema table. */
-    if ((fld= find_field_in_view(thd, table_list, name, length, item_name,
-                                 ref, check_grants_view,
+    if ((fld= find_field_in_view(thd, table_list, name, length, item_name, ref,
                                  register_tree_change)))
       *actual_table= table_list;
   }
@@ -3097,20 +3028,9 @@ find_field_in_table_ref(THD *thd, TABLE_LIST *table_list,
     /* 'table_list' is a stored table. */
     DBUG_ASSERT(table_list->table);
     if ((fld= find_field_in_table(thd, table_list->table, name, length,
-                                  check_grants_table, allow_rowid,
-                                  cached_field_index_ptr,
-                                  table_list->security_ctx)))
+                                  allow_rowid,
+                                  cached_field_index_ptr)))
       *actual_table= table_list;
-#ifndef NO_EMBEDDED_ACCESS_CHECKS
-    /* check for views with temporary table algorithm */
-    if (check_grants_view && table_list->view &&
-        fld && fld != WRONG_GRANT &&
-        check_grant_column(thd, &table_list->grant,
-                           table_list->view_db.str,
-                           table_list->view_name.str,
-                           name, length))
-    fld= WRONG_GRANT;
-#endif
   }
   else
   {
@@ -3129,9 +3049,8 @@ find_field_in_table_ref(THD *thd, TABLE_LIST *table_list,
       {
         if ((fld= find_field_in_table_ref(thd, table, name, length, item_name,
                                           db_name, table_name, ref,
-                                          check_grants_table,
-                                          check_grants_view,
-                                          allow_rowid, cached_field_index_ptr,
+                                          check_privileges, allow_rowid,
+                                          cached_field_index_ptr,
                                           register_tree_change, actual_table)))
           DBUG_RETURN(fld);
       }
@@ -3144,11 +3063,16 @@ find_field_in_table_ref(THD *thd, TABLE_LIST *table_list,
       directly the top-most NATURAL/USING join.
     */
     fld= find_field_in_natural_join(thd, table_list, name, length, ref,
-                                    /* TIMOUR_TODO: check this with Sanja */
-                                    check_grants_table || check_grants_view,
                                     register_tree_change, actual_table);
   }
 
+#ifndef NO_EMBEDDED_ACCESS_CHECKS
+  /* Check if there are sufficient access rights to the found field. */
+  if (fld && check_privileges &&
+      check_column_grant_in_table_ref(thd, *actual_table, name, length))
+    fld= WRONG_GRANT;
+#endif
+
   DBUG_RETURN(fld);
 }
 
@@ -3230,21 +3154,11 @@ find_field_in_tables(THD *thd, Item_ident *item,
       */
     if (table_ref->table && !table_ref->view)
       found= find_field_in_table(thd, table_ref->table, name, length,
-                                 test(table_ref->table->
-                                      grant.want_privilege) &&
-                                 check_privileges,
-                                 1, &(item->cached_field_index),
-                                 table_ref->security_ctx);
+                                 TRUE, &(item->cached_field_index));
     else
       found= find_field_in_table_ref(thd, table_ref, name, length, item->name,
-                                     NULL, NULL, ref,
-                                     (table_ref->table &&
-                                      test(table_ref->table->grant.
-                                           want_privilege) &&
-                                      check_privileges),
-                                     (test(table_ref->grant.want_privilege) &&
-                                      check_privileges),
-                                     1, &(item->cached_field_index),
+                                     NULL, NULL, ref, check_privileges,
+                                     TRUE, &(item->cached_field_index),
                                      register_tree_change,
                                      &actual_table);
     if (found)
@@ -3286,14 +3200,7 @@ find_field_in_tables(THD *thd, Item_ident *item,
   {
     Field *cur_field= find_field_in_table_ref(thd, cur_table, name, length,
                                               item->name, db, table_name, ref,
-                                              (cur_table->table &&
-                                               test(cur_table->table->grant.
-                                                    want_privilege) &&
-                                               check_privileges),
-                                              (test(cur_table->grant.
-                                                    want_privilege)
-                                               && check_privileges),
-                                              allow_rowid,
+                                              check_privileges, allow_rowid,
                                               &(item->cached_field_index),
                                               register_tree_change,
                                               &actual_table);

sql/table.cc

@@ -2606,60 +2606,6 @@ GRANT_INFO *Natural_join_column::grant()
 }
 
 
-
-#ifndef NO_EMBEDDED_ACCESS_CHECKS
-
-/*
-  Check the access rights for the current join column.
-  columns.
-
-  SYNOPSIS
-    Natural_join_column::check_grants()
-
-  DESCRIPTION
-    Check the access rights to a column from a natural join in a generic
-    way that hides the heterogeneity of the column representation - whether
-    it is a view or a stored table colum.
-
-  RETURN
-    FALSE  The column can be accessed
-    TRUE   There are no access rights to all equivalent columns
-*/
-
-bool
-Natural_join_column::check_grants(THD *thd, const char *name, uint length)
-{
-  GRANT_INFO *grant;
-  const char *db_name;
-  const char *table_name;
-  Security_context *save_security_ctx= thd->security_ctx;
-  Security_context *new_sctx= table_ref->security_ctx;
-  bool res;
-
-  if (view_field)
-  {
-    DBUG_ASSERT(table_field == NULL);
-    grant= &(table_ref->grant);
-    db_name= table_ref->view_db.str;
-    table_name= table_ref->view_name.str;
-  }
-  else
-  {
-    DBUG_ASSERT(table_field && view_field == NULL);
-    grant= &(table_ref->table->grant);
-    db_name= table_ref->table->s->db;
-    table_name= table_ref->table->s->table_name;
-  }
-
-  if (new_sctx)
-    thd->security_ctx= new_sctx;
-  res= check_grant_column(thd, grant, db_name, table_name, name, length);
-  thd->security_ctx= save_security_ctx;
-  return res;
-}
-#endif
-
-
 void Field_iterator_view::set(TABLE_LIST *table)
 {
   DBUG_ASSERT(table->field_translation);

sql/table.h

@@ -407,9 +407,6 @@ public:
   const char *table_name();
   const char *db_name();
   GRANT_INFO *grant();
-#ifndef NO_EMBEDDED_ACCESS_CHECKS
-  bool check_grants(THD *thd, const char *name, uint length);
-#endif
 };