List:Commits« Previous MessageNext Message »
From:Ingo Strüwing Date:July 9 2009 3:06pm
Subject:Re: bzr commit into mysql-6.0-backup branch (charles.bell:2839)
Bug#45889
View as plain text  
Hi Chuck,

STATUS: not approved.

REQUIRED: Tests with the minimal set of privileges required to backup
databases with all possible objects.

OPTIONS: None of my other comments hinder progress. But I beg to
consider them carefully. This time I do not need to have answers on all
items. Even my questions are mostly of rethoric nature. But if you find
some comment, which indicates a misunderstanding of mine, I'd appreciate
to hear about it.

COMMENTARY: A nice, simple patch. I like it.

DETAILS:
Chuck Bell, 09.07.2009 00:17:

> #At file:///D:/source/bzr/mysql-6.0-bug-45889/ based on
> revid:ingo.struewing@stripped
> 
>  2839 Chuck Bell	2009-07-08
>       BUG#45889 : BACKUP DATABASE command should include all objects 
>       
>       The code currently silently succeeds if a user is not able to read
>       (SELECT) all of the objects in the database or, in the case of
>       BACKUP DATABASE *, is not able to read all of the databases.


We do not have a guideline for the tense to use in revision comments.
However, IMHO it would be best to write the comment from the standpoint
of a pushed patch. This would also be inline with the file comments,
which usually are in past tense: "Added this, changed that, ...".

Now look at the tenses in this patch' comments. The file comments say
that changed have been made. And the global comment says that the code
silently succeeds. But the goal of the changes that have been made was
to let it *not* silently succeed.

IMHO it would sound better and be more consistent if the global comment
says: "[The problem was that] the code did silently succeed ... This is
fixed by ..." or "This patch implements ..." though present tense, it is
still clear enough.

If someone reads the comment from the repository, it is clear what was
the previous behavior and how was it changed by this patch.

>       
>       This is an error because the BACKUP DATABASE command has been clarified
>       to error if a user cannot read all of the objects or databases.
>       
>       This patch implements a solution to produce an error if either of the
>       following are true:
>       
>       * The user cannot read (SELECT) all of the databases when issuing a
>         BACKUP DATABASE * command.
>       * The user cannot read (SELECT) all of teh objects in a database when


Typo teh

>         issuing a BACKUP DATABASE <list> command.
>       
>       Note: The code currently checks for invalid databases so a BACKUP DATABASE
>       a, b, c command could fail elsewhere in the code if a, b, or c is either
>       not a database or the user does not have SELECT privilege on at least one
>       other database.


Besides the same problem as above (it is not obvious if "current" refers
to the situation before or after applying the patch), I don't understand
the sentence. The problem lies in the last part "or the user does not
have SELECT privilege ...". What is "one other database" here? I guess,
you wanted to say "one of the databases". But wouldn't it be more clear
like this: "if a, b, or c is either not a database or the user does not
have SELECT privilege on it." ? In my understanding "it" would refer to
the subject of the sentence, which is "a,b, or c", which is probably,
what you want. Do you?

>      @ mysql-test/suite/backup/r/backup_security.result
>         New result file.


For me it's slightly confusing. I use to use "New ... file" for files
that are new. That is, they did not exist before. For changed result
files, I use to use "Updated result file". (For other files I explain
the changes). Could we make this a common habit?

>      @ mysql-test/suite/backup/t/backup_security.test
>         Reorganized test and added new test cases.


Oh yes, there was a recent proposal to change the nomenclature.
Old -> New.
Test case -> Test file.
Test -> Test case. (One unit within a test file.)
So I guess "Reorganized test" is "Reorganized test file" in new
nomenclature.

...
> === modified file 'mysql-test/suite/backup/r/backup_security.result'


A big thank you that you made the result file very good readable.

...

> +#

> +# Setup grants for bup_root_user
> +#
> +REVOKE ALL ON *.* FROM 'bup_root_user'@'localhost';
> +GRANT ALL ON *.* TO 'bup_root_user'@'localhost';


Why revoke, what will be granted in close succession?

> +GRANT GRANT OPTION ON *.* TO 'bup_root_user'@'localhost';
> +GRANT SELECT ON mysql.* TO 'bup_root_user'@'localhost';


Hu? Isn't this included in GRANT ALL ON *.* ?

...
>  FLUSH PRIVILEGES;


According to the manual, this should not be necessary here. It's
required after INSERT/UPDATE/DELETE on the privilege tables only.

Wouldn't it be nice to show the grants?

...
> === modified file 'mysql-test/suite/backup/t/backup_security.test'
...
> -connect (root_user,localhost,root,,);
> +connect (root,localhost,root,,);


I see an ambiguity between user name and connection name. In further
discussions it will be insufficient to talk about "root", as the default
connection is also "root".

Hence I beg for connection names that start with "con", e.g. "con_root".

>  
>  --disable_warnings
>  DROP DATABASE IF EXISTS backup_test;
>  --enable_warnings


Shouldn't other databases be included here too? There is at least
backup_test_alt. (I know it is old code, but if you restructure it, you
have to take responsibility for it, IMHO ;-)

...
>  --replace_regex /[0-9]/#/


Some time ago I had problems with this replacement. It replaced numbers
where it should not. I found that replacing column 2 accomplishes the
same for BACKUP, RESTORE, SHOW WARNINGS, and SHOW ERRORS, but with no
side effects.

...
> +connect (no_rights,localhost,bup_no_rights,,);
> +
> +--echo #
> +--echo # no_rights: Attempting backup. Should fail with 
> +--echo # error ER_BAD_DB_ERROR
>  --echo #
>  --replace_column 1 #
> -BACKUP DATABASE * to 'bup_with_rights_star.bak';
> +--error ER_BAD_DB_ERROR
> +BACKUP DATABASE backup_test to 'bup_no_rights.bak';
> +--replace_regex /[0-9]/#/
> +SHOW ERRORS;


Good. But I would like to see the positive test too. A user, who has
BACKUP on the database and SELECT on all its objects and nothing more.
That is, I'd like to see a successful BACKUP for a non-"root" user.

And, after Rafal noticed that events could still be omitted silently,
please check all objects for their existence after restore. Ideally we
should even "execute" every object as I have done in
backup_xpfm_compat_restore.inc. Perhaps we can extract some common code
into another include file for use by several test cases.

...
> === modified file 'sql/si_objects.cc'
...
> +/**
> +  Return a count of databases.
> +
> +  This method returns the number of databases excluding the internal
> +  databases 'mysql' and 'information_schema'.
> +
> +  @Note This method executes based on the current context of the THD
> +        and thus returns a value based on the visibility (i.e. SELECT
> +        privilege) of the user context.


Meanwhile we learned that "i.e. SELECT" is not correct. "e.g. SELECT"
would be better, but fuzzy. How about "based on the accessibility of
objects by the user context"?

Same comment applies to the other method too.

> +
> +  @param[in] THD     Current thread
> +  
> +  @returns uint Number of databases.
> +*/
> +uint get_num_dbs(THD *thd)
> +{
> +  Ed_connection ed_connection(thd);
> +  Ed_result_set *ed_result_set;
> +  String_stream s_stream;
> +
> +  s_stream << 
> +    "SELECT SCHEMA_NAME FROM INFORMATION_SCHEMA.SCHEMATA " <<
> +    "WHERE LCASE(schema_name) != 'mysql' AND " <<


IMHO the two << bewteen the string literals are not required. The
compiler concatenates successive string literals into one. Please see
also the concerning comment in the other method.

> +    "LCASE(schema_name) != 'information_schema'";
> +
> +  if (run_service_interface_sql(thd, &ed_connection, s_stream.lex_string()))
> +    /* Query failed with an error */
> +    return NULL;
> +  else if(ed_connection.get_warn_count())
> +    /* Push warnings to BACKUP's error stack. Calling method will
> +       decide if the warnings are a problem later when serializing the
> +       object. */


Please re-read the comment. I believe it's a copy-n-paste slip. We don't
handle an object here. Will the calling method indeed "decide if the
warnings are a problem"? I suggest to remove the second sentence.

Same comment applies to the other method too.

...
> +  s_stream <<
> +    "SELECT TABLE_NAME "
> +    "FROM INFORMATION_SCHEMA.TABLES "
> +    "WHERE table_schema = '" << db_name << "' UNION " <<


Aha. Here we have a mix of string literal concatenation and << string
append. I mean that the << after " UNION " are not required. The
compiler can concatenate this string literal with the next one, while <<
means a string append within the s_stream object in runtime.

> +    "SELECT TRIGGER_NAME "
> +    "FROM INFORMATION_SCHEMA.TRIGGERS "
> +    "WHERE trigger_schema = '" << db_name << "' UNION " <<
> +    "SELECT ROUTINE_NAME "
> +    "FROM INFORMATION_SCHEMA.ROUTINES "
> +    "WHERE routine_schema = '" << db_name << "' UNION " << 
> +    "SELECT EVENT_NAME "
> +    "FROM INFORMATION_SCHEMA.EVENTS "
> +    "WHERE event_schema = '" << db_name << "'";
...
> === modified file 'sql/si_objects.h'
...
>  /**
> +  Check user access for databases or objects in a given database.


I am surprised to see the function comment in the header file, not in
the implementation. Though I'm a proponent of this style, I have to
point out that it is not the current MySQL style. There are discussions
to change that style, but as far as I know, it has not been done yet.

> +
> +  This method can be used to check the user's access before 
> +  creating iterators and internal object classes for the items
> +  in the databases. This method is used in two situations:
> +
> +  1) When executing BACKUP DATABASE * ... commands.
> +
> +  If @c db_name is 0, the method checks to see if the user can read
> +  (has SELECT) on all databases. 


We learned that SELECT might not be sufficient here. Either write it as
fuzzy as the manual does: "if the user has privileges on all databases",
or specify the required privileges exactly.

> +
> +  2) When executing BACKUP DATABASE <list> ... commands.
> +
> +  This method checks to see if the user can read (has SELECT) on
> +  the objects for the database specified as @c db_name. 


We learned that SELECT might not be sufficient here. Either write it as
fuzzy as the manual does: "if the user has privileges on all databases",
or specify the required privileges exactly.

...

Regards
Ingo
-- 
Ingo Strüwing, Database Group
Sun Microsystems GmbH, Sonnenallee 1, D-85551 Kirchheim-Heimstetten
Geschäftsführer: Thomas Schröder,   Wolfgang Engels,   Wolf Frenkel
Vorsitzender des Aufsichtsrates: Martin Häring   HRB München 161028
Thread
bzr commit into mysql-6.0-backup branch (charles.bell:2839) Bug#45889Chuck Bell9 Jul
  • Re: bzr commit into mysql-6.0-backup branch (charles.bell:2839)Bug#45889Ingo Strüwing9 Jul
    • Re: bzr commit into mysql-6.0-backup branch (charles.bell:2839)Bug#45889Chuck Bell10 Jul
      • Re: bzr commit into mysql-6.0-backup branch (charles.bell:2839)Bug#45889Ingo Strüwing10 Jul
  • Re: bzr commit into mysql-6.0-backup branch (charles.bell:2839)Bug#45889Konstantin Osipov14 Jul