Pantek Library
Hosting Provided By
CybrHost
High Speed Hosting
Mailing Lists: lists.mysql.com > commits > 07 > 094383.html (Request Expert lists.mysql.com Support)

Re: bk commit into 5.0 tree (evgen:1.2424) BUG#29908

From: Alexander Nozdrin <alik(at)mysql.com>
Date: Thu Sep 06 2007 - 02:53:23 EDT


Hi,

thank you for working on this! I have a few comments on the patch.

First of all, I'd like just to point out that the patch changes the behaviour and breaks backward compatibility in some sense.

That's Ok, since Trudy approved this in the bug report. However, could you please emphasize this in the CS comment (also, please specify what should happen if a view was created as SQL SECURITY INVOKER). On Monday 20 August 2007 01:20, eugene@mysql.com wrote: 

> --- 1.107/sql/sql_view.cc	2007-08-20 01:20:46 +04:00
> +++ 1.108/sql/sql_view.cc	2007-08-20 01:20:46 +04:00
> @@ -224,9 +224,6 @@
>  {

> LEX *lex= thd->lex;
> bool link_to_local;
> -#ifndef NO_EMBEDDED_ACCESS_CHECKS
> -  bool definer_check_is_needed= mode != VIEW_ALTER || lex->definer;
> -#endif

> /* first table in list is target VIEW name => cut off it */
> TABLE_LIST *view= lex->unlink_first_table(&link_to_local);
> TABLE_LIST *tables= lex->query_tables;
> @@ -281,11 +278,10 @@

>        - same as current user
>        - current user has SUPER_ACL
> */
> -  if (definer_check_is_needed &&
> -      (strcmp(lex->definer->user.str, thd->security_ctx->priv_user) != 0 ||
> +  if (strcmp(lex->definer->user.str, thd->security_ctx->priv_user) != 0 ||

>         my_strcasecmp(system_charset_info,

I'm afraid, we'll have a crash under some circumstances here -- lex->definer can be NULL, if I'm not mistaken. For example, that can happen if the server is running in the bootstrap mode (--bootstrap option).

Also, it seems, we should not check definer if a view has been created with SQL SECURITY INVOKER. May be you could also refactor this code a little bit -- it seems, we firstly check if the current user is a definer. If it is not, we check if the current user is a super user.

It seems, the more natural way (and faster!! :)) for this is to:

  1. Check if the current user is a super user;
  2. If not: check if the current user is a definer.

Then, if the current user is a super user, we check if the definer exists. I don't see the point in such a check -- either we should check it in any case, or we don't need it.   

> --- 1.22/mysql-test/t/view_grant.test	2007-08-20 01:20:46 +04:00
> +++ 1.23/mysql-test/t/view_grant.test	2007-08-20 01:20:46 +04:00
> @@ -1053,10 +1053,11 @@
>  
>  connect (u1,localhost,u26813,,db26813);
>  connection u1;
> ---error 1142
> +--error 1227
Do you need help?X

Could you please use text identifiers here and in other places from include/mysqld_error.h file (ER_SPECIFIC_ACCESS_DENIED_ERROR in this case)? 

> +#
> +# Bug#29908: A user can gain additional access through the ALTER VIEW.
> +
> +#
> +connection root;

I believe, it's a good rule to start all the "test-suite-commands" from '--':
  --connection root
However, I can not force you to do that... :)

> +CREATE DATABASE db29908;

Well, I was told, that we should use "standard" database names in the test suite -- mysqltest1, mysqltest2, ... That is required to minimize pollution of the user installation when the test suite is run against existing database.

However, our test cases don't stick to this rule, so probably I can not ask you to do that... :) 

> +USE db29908;
> +CREATE TABLE t1(f1 INT, f2 INT);
> +CREATE USER u29908_1@localhost;

> +CREATE DEFINER = u29908_1@localhost VIEW v1 AS SELECT f1 FROM t1;
> +GRANT DROP, CREATE VIEW, SHOW VIEW ON db29908.v1 TO u29908_1@localhost;
> +GRANT SELECT ON db29908.t1 TO u29908_1@localhost;
> +CREATE USER u29908_2@localhost;
> +GRANT DROP, CREATE VIEW ON db29908.v1 TO u29908_2@localhost;
> +GRANT SELECT ON db29908.t1 TO u29908_2@localhost;
> +

> +connect (u2,localhost,u29908_2,,db29908); > +connection u2;

Generally speaking, "connect" automatically switches the active connection, so you don't need the second command. 

> +--error 1227
> +ALTER VIEW v1 AS SELECT f2 FROM t1;
> +
> +connect (u1,localhost,u29908_1,,db29908);
> +connection u1;
> +ALTER VIEW v1 AS SELECT f2 FROM t1;
> +SHOW CREATE VIEW v1;
> +
> +connection root;
> +ALTER VIEW v1 AS SELECT f1 FROM t1;
> +SHOW CREATE VIEW v1;

Could you please add a test case for a view created with SQL SECURITY INVOKER? 

> +
> +DROP USER u29908_1@localhost;
> +DROP USER u29908_2@localhost;
> +DROP DATABASE db29908;
> +disconnect u1;
> +disconnect u2;

Could I ask you for one more favor -- put "--echo" statements in the beginning and at the end of the test case. Something like: 

--echo #
--echo # Bug#29908: A user can gain additional access through the ALTER VIEW.
--echo #
Do you need more help?X

... (test case) 

--echo ####################################################################

Can we help you?X

That would help merging result-files, since there will be clear boundaries between test cases.

Thanks! 

-- 
Alexander Nozdrin, Software Developer
MySQL AB, Moscow, Russia, www.mysql.com

-- 
MySQL Code Commits Mailing List
For list archives: 
http://lists.mysql.com/commits
To unsubscribe:    
http://lists.mysql.com/commits?unsub=lists@pantek.com
Received on Thu Sep 6 02:56:26 2007

This archive was generated by hypermail 2.1.8 : Sun Oct 07 2007 - 09:09:57 EDT

Contact Us  Legal Notices  Order Services Online 
Pantek Home  Privacy Policy  IT news  Site Map  Pantek Library