Ingo,
Here are my more detailed responses and the new patch.
I will wait to push the patch until I receive your email stating you
agree that my solutions are acceptable.
See: http://lists.mysql.com/commits/91716
> REQUIRED:
> 1) Better comment.
> 15) Fix copyright.
>
> REQUESTS:
> 3) Do we need privileges on mysql.* for RESTORE? (recurring)
> 4) Check if elevation disturbs the test goal. (recurring)
> 7) Better comment.
> 8) Check letter case.
> 9) Remove "comparison". Or, better yet, add it.
> 10) Create 'joe'.
> 11) Show, it fails in the middle. (recurring)
> 14) Improve session variable testing.
> 16) Better comment.
> 17) Declare variables in backup code. (recurring)
> 18) Please initialize [my_]bool variables with TRUE or FALSE.
...
>> 3) The backup system must be changed to allow users to adjust the
>> privilege behavior of MySQL Backup by allowing them
>> to turn off backup elevation, restore elevation, and restore
>> prechecking using startup options and/or variables. This patch
>> implements startup options and global + session variables to do
>> the following:
>
>
> 1) This is now inaccurate. Please specify variable type per option.
This has been changed to:
3) The backup system must be changed to allow users to adjust the
privilege behavior of MySQL Backup by allowing them
to turn off backup elevation, restore elevation, and restore
prechecking using startup options. It shall also permit the user
to turn restore prechecking on or off via a variable. This patch
implements the following:
backup_elevation - startup option, global read only variable
ON = turn on backup elevation
OFF = turn off backup elevation
Note: Default is ON
restore_elevation - startup option, global read only variable
ON = turn on restore elevation
OFF = turn off restore elevation
Note: Default is ON
restore_precheck - startup option, global and session variable
ON = turn on restore precheck
OFF = turn off restore precheck
Note: Default is ON
>
> ...
>> === added file 'mysql-test/suite/backup/r/backup_restore_security.result'
> ...
>> +# Test Case 1 : Ensure a user with only RESTORE cannot restore
>> +# the database. Note: we also add CREATE and DROP so
>> +# the error will occur in the precheck to show that
>> +# having CREATE + DROP on the database is not enough to
>> +# restore the objects in the database.
>> +#
>> +GRANT RESTORE ON backup_test.* TO 'bup_some_priv'@'localhost';
>> +GRANT CREATE, DROP ON backup_test.* TO 'bup_some_priv'@'localhost';
>> +FLUSH PRIVILEGES;
>> +#
>> +# Show grants for user.
>> +#
>> +SHOW GRANTS FOR 'bup_some_priv'@'localhost';
>> +Grants for bup_some_priv@localhost
>> +GRANT USAGE ON *.* TO 'bup_some_priv'@'localhost'
>> +GRANT CREATE, DROP, RESTORE ON `backup_test`.* TO 'bup_some_priv'@'localhost'
>> +#
>> +# Connect as user with only some privileges.
>> +#
>> +#
>> +# conn_some_priv: Attempting restore. Should fail with
>> +# error ER_RESTORE_ACCESS_OBJS_INCOMPLETE
>> +#
>> +RESTORE FROM 'backup_test_orig.bak' OVERWRITE;
>> +ERROR HY000: Insufficient privileges. You do not have privileges to restore the
> object 'p1' from this backup image.
>> +SHOW ERRORS;
>> +Level Code Message
>> +Error # Insufficient privileges. You do not have privileges to restore the
> object 'p1' from this backup image.
>
>
> 2) A plea:
>
> I often see "REVOKE ALL ON *.* FROM 'bup_some_priv'@'localhost'", that
> is, revoke all global privileges. This sometimes happens even after
> "CREATE USER". Just to be sure.
What does 'Just to be sure' mean?
To satisfy this and similar complaints, I now revoke the _exact_
privileges rather than a blanket revoke.
Regardless, I have removed all unnecessary REVOKES from the following tests:
backup_restore_security
backup_security
backup_security_var
backup_security_options
> I often see "FLUSH PRIVILEGES", even though no updates on the privilege
> tables are done (GRANT/REVOKE don't need it). Just to be sure.
I was told always use FLUSH PRIVILEGES (despite what the documentation
says) because it is 'good form'. If you disagree, then you can omit it
as you see fit but I choose to leave them in.
> But I see "SHOW VARIABLES like 'restore_%'" pretty seldom. Since it is
> less easy to calculate their values, based on former settings, or
> presence/absence of a -master.opt file, or implicit default values, than
> it is to calculate the grants, I ask for a "SHOW VARIABLES like
> 'restore_%'" before every RESTORE. Just to be sure.
Yes, it is seldom because they're global read only variables so we don't
need to check their values. However, I will add one SELECT for each of
these at the start of each test.
> ...
>> +# Test Case 3 : Show that SUPER is needed to complete restore
>> +# when there are objects with definer clauses.
>> +#
>> +#
>> +# Connect as root and add privileges.
>> +#
>> +REVOKE ALL ON *.* FROM 'bup_some_priv'@'localhost';
>> +GRANT ALL ON backup_test.* TO 'bup_some_priv'@'localhost' WITH GRANT OPTION;
>> +GRANT ALL ON mysql.* TO 'bup_some_priv'@'localhost';
>
>
> 3) Do we need privileges on mysql.* for RESTORE? If yes, do we need more
> than SELECT?
Once again, I was asked to do it this way to ensure we have all
privileges covered. My own explanations as to what the minimal set of
privileges was rejected. Hence we compromised and added all privileges
on mysql database. If you severely object you can open a bug but I will
leave it like this in the patch.
>> +FLUSH PRIVILEGES;
>> +#
>> +# Show grants for user.
>> +#
>> +SHOW GRANTS FOR 'bup_some_priv'@'localhost';
>> +Grants for bup_some_priv@localhost
>> +GRANT USAGE ON *.* TO 'bup_some_priv'@'localhost'
>> +GRANT ALL PRIVILEGES ON `mysql`.* TO 'bup_some_priv'@'localhost'
>> +GRANT ALL PRIVILEGES ON `backup_test`.* TO 'bup_some_priv'@'localhost' WITH
> GRANT OPTION
>> +#
>> +# Connect as user with only some privileges.
>> +#
>> +#
>> +# conn_some_priv: Attempting restore. Should fail with
>> +# ER_RESTORE_ACCESS_DEFINER
>> +#
>> +RESTORE FROM 'backup_test_no_proc.bak' OVERWRITE;
>> +ERROR HY000: Insufficient privileges. You must have the SUPER privilege to
> restore the object 'backup_test'.'v1'.
>> +SHOW ERRORS;
>> +Level Code Message
>> +Error # Insufficient privileges. You must have the SUPER privilege to restore
> the object 'backup_test'.'v1'.
>> +#
>> +# Connect as root and add privileges.
>> +#
>> +GRANT SUPER ON *.* TO 'bup_some_priv'@'localhost';
>> +FLUSH PRIVILEGES;
>> +#
>> +# Show grants for user.
>> +#
>> +SHOW GRANTS FOR 'bup_some_priv'@'localhost';
>> +Grants for bup_some_priv@localhost
>> +GRANT SUPER ON *.* TO 'bup_some_priv'@'localhost'
>> +GRANT ALL PRIVILEGES ON `mysql`.* TO 'bup_some_priv'@'localhost'
>> +GRANT ALL PRIVILEGES ON `backup_test`.* TO 'bup_some_priv'@'localhost' WITH
> GRANT OPTION
>> +#
>> +# Connect as user with only some privileges.
>> +#
>> +#
>> +# conn_some_priv: Attempting restore. Should succeed.
>> +#
>> +RESTORE FROM 'backup_test_no_proc.bak' OVERWRITE;
>> +backup_id
>> +#
>
>
> 4) But this ran with privilege elevation, right? Unfortunately the
> variable settings are not shown. :( Since this test case does not have a
> -master.opt file, the variables should be ON. But then the test shows
> that definer clauses can be restored with elevation. It does not really
> show that SUPER+ALL works even without elevation. I guess that test case
> description suggests this.
I added an option file so that all of the tests will run with
backup_elevation and restore elevation OFF.
This required moving test cases 6 and 7 to backup_security_var where
they became test case 4 and 5 and we know backup_elevation and
restore_elevation are ON.
>
>> +#
>> +# Test Case 4 : Ensure that a user can restore the database if she
>> +# has ALL privileges on the database, ALL privileges
>> +# on the system tables (mysql.*), except GRANT, and the
>> +# global SUPER privilege.
>> +#
>> +#
>> +# Connect as root and add privileges.
>> +#
>> +REVOKE ALL ON *.* FROM 'bup_some_priv'@'localhost';
>> +GRANT ALL ON backup_test.* TO 'bup_some_priv'@'localhost' WITH GRANT OPTION;
>> +GRANT SELECT ON mysql.* TO 'bup_some_priv'@'localhost';
>> +GRANT SUPER ON *.* TO 'bup_some_priv'@'localhost';
>> +FLUSH PRIVILEGES;
>> +#
>> +# Show grants for user.
>> +#
>> +SHOW GRANTS FOR 'bup_some_priv'@'localhost';
>> +Grants for bup_some_priv@localhost
>> +GRANT SUPER ON *.* TO 'bup_some_priv'@'localhost'
>> +GRANT ALL PRIVILEGES ON `mysql`.* TO 'bup_some_priv'@'localhost'
>> +GRANT ALL PRIVILEGES ON `backup_test`.* TO 'bup_some_priv'@'localhost' WITH
> GRANT OPTION
>
>
> 5) All is fine. Just a hint for future tests. It looks unexpected to see
> ALL PRIVILEGES ON `mysql`.* after REVOKE ALL ON *.* and GRANT SELECT ON
> mysql.*. I dug in the manual and found that REVOKE ALL ON *.* revokes
> just all global privileges, but none from databases or database objects.
> If one wants to remove all privileges, global, database, and objects,
> one must use REVOKE ALL PRIVILEGES, GRANT OPTION FROM user. That is, no
> ON clause.
>
> Not a strong requirement to change, as the test case description matches
> the resulting privileges. GRANT SELECT ON mysql.* might be slightly
> confusing, but is not exactly wrong. If you like, you could change
> SELECT to ALL in that statement. Or, just in case you wasn't aware of
> the described REVOKE behavior, you can also change the REVOKE and the
> test case description regarding the system table privileges, if you
> prefer. It might be preferable to show that SELECT ON mysql.* suffices.
Removed all unnecessary REVOKE statements as described above.
>
> 3) Or, if RESTORE doesn't need privileges on mysql.* at all, please
> revoke them.
I leave them in as described above.
>
>> +#
>> +# Connect as user with only some privileges.
>> +#
>> +#
>> +# conn_some_priv: Attempting restore. Should succeed.
>> +#
>> +RESTORE FROM 'backup_test_orig.bak' OVERWRITE;
>> +backup_id
>> +#
>
>
> 4) So this ran with elevation too, I think. And the final privileges are
> exactly the same as the privileges for the second RESTORE in test case
> 3.
I turned elevation OFF for this test as described above.
>
> 6) So I wonder, what test case 4 adds? Suggest to remove it.
Test case 4 adds value now that elevation is off.
>
>> +#
>> +# Connect as root and cleanup.
>> +#
>> +#
>> +# Test Case 5 : Show that a check of the trigger privilege is required
>> +# to restore.
>
>
> 7) Sorry, but this new text does not address my concern.
>
> What the sentence says is: "It is required to check the trigger
> privilege to restore." (Or: "... to do a successful RESTORE.", right?)
>
> So in reverse it means that RESTORE would fail, if we would not check
> the trigger privilege? How is it possible that *not* checking a
> privilege could make an operation fail?
>
> What the test seems to try to show is that the TRIGGER privilege is
> required. And hence, perhaps, that the precheck needs to include a check
> of the trigger privilege (to prevent RESTORE from failing in the middle).
>
Ok, so how about this?
--echo #
--echo # Test Case 5 : Show the TRIGGER_ACL privilege is needed for
--echo # restore when restore_elevation is OFF and
--echo # restore_precheck is ON.
--echo #
I also add a SELECT of the variables for clarification. In this case, I
am going with your (I think) suggestion to be safe and display them
where it matters for clarity.
>> +#
>> +# Perform a backup with only a table and a trigger.
>> +# No grants either.
>> +#
>> +DROP USER 'bup_some_priv'@'localhost';
>> +FLUSH PRIVILEGES;
>> +CREATE DATABASE backup_test_trig;
>> +CREATE TABLE backup_test_trig.t1 (a char(30)) ENGINE=MEMORY;
>> +CREATE TRIGGER backup_test_trig.trg AFTER INSERT ON backup_test_trig.t1 FOR EACH
> ROW
>> +INSERT INTO backup_test_trig.t1 VALUES('Test objects count');
>> +BACKUP DATABASE backup_test_trig TO 'backup_test_trig.bak';
>> +backup_id
>> +#
>> +CREATE USER 'bup_some_priv'@'localhost';
>> +REVOKE ALL ON *.* FROM 'bup_some_priv'@'localhost';
>> +GRANT ALL ON backup_test_trig.* TO 'bup_some_priv'@'localhost' WITH GRANT
> OPTION;
>> +REVOKE TRIGGER ON backuP_test_trig.* FROM 'bup_some_priv'@'localhost';
>
>
> 8) Is it intentional that you have the upper case 'P' here? It doesn't
> seem to hurt though. SHOW GRANTS does not list the TRIGGER privilege on
> backup_test_trig. This seems to be yet another letter case mystery of MySQL.
>
No, not intentional. I fixed it.
>> +GRANT SELECT ON mysql.* TO 'bup_some_priv'@'localhost';
>
>
> 3) Again, do we need privileges on mysql.* for RESTORE?
Explained above.
>
>> +FLUSH PRIVILEGES;
>> +#
>> +# Show grants for user.
>> +#
>> +SHOW GRANTS FOR 'bup_some_priv'@'localhost';
>> +Grants for bup_some_priv@localhost
>> +GRANT USAGE ON *.* TO 'bup_some_priv'@'localhost'
>> +GRANT SELECT ON `mysql`.* TO 'bup_some_priv'@'localhost'
>> +GRANT SELECT, INSERT, UPDATE, DELETE, CREATE, DROP, REFERENCES, INDEX, ALTER,
> CREATE TEMPORARY TABLES, LOCK TABLES, EXECUTE, CREATE VIEW, SHOW VIEW, CREATE ROUTINE,
> ALTER ROUTINE, EVENT, BACKUP, RESTORE ON `backup_test_trig`.* TO
> 'bup_some_priv'@'localhost' WITH GRANT OPTION
> ...
>> +RESTORE FROM 'backup_test_trig.bak' OVERWRITE;
>> +ERROR HY000: Insufficient privileges. You do not have privileges to restore the
> object 'trg' from this backup image.
> ...
>> +GRANT TRIGGER ON backup_test_trig.t1 TO 'bup_some_priv'@'localhost';
> ...
>> +RESTORE FROM 'backup_test_trig.bak' OVERWRITE;
>> +ERROR HY000: Insufficient privileges. You must have the SUPER privilege to
> restore the object 'backup_test_trig'.'trg'.
> ...
>> +GRANT SUPER ON *.* TO 'bup_some_priv'@'localhost';
>> +FLUSH PRIVILEGES;
> ...
>> +# conn_some_priv: Attempting restore. Should succeed.
>
>
> 4) Yes, we do now have RESTORE + SUPER. I don't see a show of the
> restore_elevation variable, but I guess it is ON by default. Hence
> *this* test does not prove that we need TRIGGER.
It is now shown.
> Perhaps you can move this test into a file, where restore_elevation=OFF?
> If it is OFF here, please show the variable value. Or, better yet,
> extend the test by revoking TRIGGER and try to restore again. Now that
> we have SUPER, but not TRIGGER, it should also fail.
Ok, done.
>
> ...
>> +# Test Case 7 : Show that users who have RESTORE + SUPER for db1 but not
>> +# for db2 cannot restore the image.
>> +#
> ...
>> +# conn_some_priv: Attempting restore. Should succeed.
>> +#
>> +RESTORE FROM 'backup_test_orig.bak' OVERWRITE;
>> +backup_id
>> +#
>> +#
>> +# Connect as root and cleanup.
>> +#
>> +#
>> +# Compare to original backup image file.
>> +#
>> +RESTORE FROM 'backup_test_orig.bak' OVERWRITE;
>> +backup_id
>> +#
>> +#
>> +# Show list of all objects in the database.
>> +#
>> +SHOW FULL TABLES FROM backup_test;
>> +Tables_in_backup_test Table_type
>> +t1 BASE TABLE
>> +t2 BASE TABLE
>> +v1 VIEW
>> +SELECT event_name FROM INFORMATION_SCHEMA.EVENTS WHERE event_schema =
> 'backup_test';
>> +event_name
>> +e1
>> +SELECT routine_name FROM INFORMATION_SCHEMA.ROUTINES WHERE routine_schema =
> 'backup_test';
>> +routine_name
>> +f1
>> +p1
>> +SELECT trigger_name FROM INFORMATION_SCHEMA.TRIGGERS WHERE trigger_schema =
> 'backup_test';
>> +trigger_name
>> +trg
>
>
> 9) Here is still one of these "comparisons", which I don't understand.
> If we would "Show list of all objects in the database" after the
> previous RESTORE too, then I'd get the idea.
>
> See also my my email of Fri, 20 Nov 2009 14:10:49 +0100.
Removed.
General comment, Ingo: I think I miss some of these because when I read
the review comments it isn't clear to me that there is more than one of
these types of things. Please in the future help me out by stating
something to that effect. You need not repetitively cite the issue, but
a gentle nudge/reminder like 'This occurs elsewhere' or something would
be very helpful. In a large patch it is sometimes hard to be thorough.
> ...
>> === added file 'mysql-test/suite/backup/r/backup_security_options.result'
> ...
>> +#
>> +# Create users.
>> +#
>> +CREATE USER 'bup_some_priv'@'localhost';
>
>
> 10) Here's another 'joe' missing.
Added, but not really needed. The first GRANT in basic_data will result
in the user being added. I add the explicit CREATE USER for clarity and
consistency.
> ...
>> +# Test Case 3 : Ensure restore_precheck = OFF fails for not enough
>> +# privileges (Restore will fail during the metadata
>> +# execution stage).
>> +#
>> +REVOKE ALL ON backup_test.* FROM 'bup_some_priv'@'localhost';
>> +REVOKE SUPER ON *.* FROM 'bup_some_priv'@'localhost';
>> +GRANT RESTORE ON backup_test.* TO 'bup_some_priv'@'localhost';
>> +FLUSH PRIVILEGES;
>> +#
>> +# Show grants for user.
>> +#
>> +SHOW GRANTS FOR 'bup_some_priv'@'localhost';
>> +Grants for bup_some_priv@localhost
>> +GRANT USAGE ON *.* TO 'bup_some_priv'@'localhost'
>> +GRANT RESTORE ON `backup_test`.* TO 'bup_some_priv'@'localhost'
>> +#
>> +# Connect as user with only some privileges.
>> +#
>> +#
>> +# conn_some_priv: Attempting restore. Should fail with
>> +# error ER_BACKUP_CANT_RESTORE_DB
>> +#
>> +RESTORE FROM 'backup_test_orig.bak' OVERWRITE;
>> +ERROR HY000: Could not restore database `backup_test`
>> +SHOW ERRORS;
>> +Level Code Message
>> +Error # Access denied for user 'bup_some_priv'@'localhost' to database
> 'backup_test'
>> +Error # Could not restore database `backup_test`
>> +#
>> +# Connect as root and prepare next test case.
>
>
> 11) Sad that the test ends here. Sad that my suggestion from my email of
> Fri, 20 Nov 2009 14:10:49 +0100 are not followed.
>
> The test does not show that RESTORE fails during metadata execution
> stage. At least not to someone with little knowledge of the code. The
> error message "Access denied" could come from everywhere.
I strongly disagree. It does for the reasons I stated. It is failing
during the metadata CREATE stage.
> My suggestion was to add CREATE, DROP to the privileges. Then RESTORE
> will fail with ER_BACKUP_CANT_RESTORE_SROUT instead of
> ER_BACKUP_CANT_RESTORE_DB. And another SHOW FULL TABLES, SELECT events,
> etc after switching back to root will show that only t1 and t2 exist
> and all other objects have been destroyed. This makes it very obvious
> that RESTORE "failed in the middle".
To you perhaps it makes more sense but not to me as I know it is failing
in the middle. Besides, users are not the general audience for these
tests -- we are. If we wanted better feedback to the users to indicate
it is failing in the middle I might suggest we open a bug to request
that the error messages be made more user friendly.
Regardless, I have added such a test case step to satisfy your concerns.
>
> ...
>> +# Test Case 8 : Ensure restore_precheck = OFF fails for not enough
>> +# privileges (Restore will fail during metadata
>> +# execution stage).
>> +#
>> +REVOKE ALL ON backup_test.* FROM 'bup_some_priv'@'localhost';
>> +REVOKE SUPER ON *.* FROM 'bup_some_priv'@'localhost';
>> +GRANT RESTORE ON backup_test.* TO 'bup_some_priv'@'localhost';
>> +FLUSH PRIVILEGES;
>> +#
>> +# Show grants for user.
>> +#
>> +SHOW GRANTS FOR 'bup_some_priv'@'localhost';
>> +Grants for bup_some_priv@localhost
>> +GRANT USAGE ON *.* TO 'bup_some_priv'@'localhost'
>> +GRANT SELECT ON `mysql`.* TO 'bup_some_priv'@'localhost'
>> +GRANT RESTORE ON `backup_test`.* TO 'bup_some_priv'@'localhost' WITH GRANT
> OPTION
>> +#
>> +# Connect as user with only some privileges.
>> +#
>> +#
>> +# conn_some_priv: Attempting restore. Should fail with
>> +# error ER_BACKUP_CANT_RESTORE_DB
>> +#
>> +RESTORE FROM 'backup_test_orig.bak' OVERWRITE;
>> +ERROR HY000: Could not restore database `backup_test`
>> +SHOW ERRORS;
>> +Level Code Message
>> +Error # Access denied for user 'bup_some_priv'@'localhost' to database
> 'backup_test'
>> +Error # Could not restore database `backup_test`
>> +#
>> +# Connect as root and prepare next test case.
>
>
> 11) Sad that the test ends here. Sad that my suggestion from my email of
> Fri, 20 Nov 2009 14:10:49 +0100 are not followed.
>
> The test does not show that RESTORE fails during metadata execution
> stage. At least not to someone with little knowledge of the code. The
> error message "Access denied" could come from everywhere.
>
> My suggestion was to add CREATE, DROP to the privileges. Then RESTORE
> will fail with ER_BACKUP_CANT_RESTORE_SROUT instead of
> ER_BACKUP_CANT_RESTORE_DB. And another SHOW FULL TABLES, SELECT events,
> etc after switching back to root" will show that only t1 and t2 exist
> and all other objects have been destroyed. This makes it very obvious
> that RESTORE "failed in the middle".
Added such a step to the test case as above.
>
> ...
>> +# Test Case 11 : Ensure restore_elevation = OFF and
>> +# restore_precheck = OFF can succeed
>> +# if privileges granted.
>
>
> 12) Test Case 11 is identical to test case 9 (unless I miss something).
> Please remove one of them.
>
>> +#
>> +REVOKE RESTORE ON backup_test.* FROM 'bup_some_priv'@'localhost';
>> +GRANT ALL ON backup_test.* TO 'bup_some_priv'@'localhost' WITH GRANT OPTION;
>
You are correct. Test case 11 removed.
>
> 13) New funstuff: revoke one before grant all. :)
>
Extra REVOKE statements have been removed as above.
> ...
>> === added file 'mysql-test/suite/backup/r/backup_security_var.result'
> ...
>> +# Test Case 3 : Show session variables obey initial values of
>> +# global variables.
>> +#
>> +#
>> +# Do session change. Set to ON locally.
>> +#
>> +SET restore_precheck = ON;
>> +SHOW VARIABLES LIKE 'restore_precheck';
>> +Variable_name Value
>> +restore_precheck ON
>
>
> 14) This looks a little pointless. From test file start, we know that
> the global value is ON already. It would give us more, not to set the
> local value before showing, which value the new connection did inherit.
Ok.
> And if setting the local value, perhaps to show, that it does indeed
> change, then please set the inverse value and check it. Otherwise we
> have nothing but the absence of an error message.
I am sorry but I do not understand this. I have changed the test case as
I interpret these paragraphs and hope it is better. If not, you will
need to explain it a bit more, please.
>
> ...
>> +--echo #
>> +--echo # Test Case 3 : Show that SUPER is needed to complete restore
>> +--echo # when there are objects with definer clauses.
> ...
>> +REVOKE ALL ON *.* FROM 'bup_some_priv'@'localhost';
>> +GRANT ALL ON backup_test.* TO 'bup_some_priv'@'localhost' WITH GRANT OPTION;
>> +GRANT ALL ON mysql.* TO 'bup_some_priv'@'localhost';
>
>
> 3) Again, do we need privileges on mysql.* for RESTORE?
Again, left in due to unknown privileges needed on mysql database.
> ...
>> +--echo #
>> +--echo # Test Case 4 : Ensure that a user can restore the database if she
>> +--echo # has ALL privileges on the database, ALL privileges
>> +--echo # on the system tables (mysql.*), except GRANT, and the
>> +--echo # global SUPER privilege.
> ...
>> +REVOKE ALL ON *.* FROM 'bup_some_priv'@'localhost';
>> +GRANT ALL ON backup_test.* TO 'bup_some_priv'@'localhost' WITH GRANT OPTION;
>> +GRANT SELECT ON mysql.* TO 'bup_some_priv'@'localhost';
>
>
> 3) Again, do we need privileges on mysql.* for RESTORE?
Again, left in due to unknown privileges needed to mysql database.
> ...
>> +--echo #
>> +--echo # Test Case 5 : Show that a check of the trigger privilege is required
>> +--echo # to restore.
> ...
>> +CREATE USER 'bup_some_priv'@'localhost';
>> +
>> +REVOKE ALL ON *.* FROM 'bup_some_priv'@'localhost';
>> +
>> +GRANT ALL ON backup_test_trig.* TO 'bup_some_priv'@'localhost' WITH GRANT
> OPTION;
>> +REVOKE TRIGGER ON backuP_test_trig.* FROM 'bup_some_priv'@'localhost';
>> +GRANT SELECT ON mysql.* TO 'bup_some_priv'@'localhost';
>
>
> 3) Again, do we need privileges on mysql.* for RESTORE?
Again, left in due to unknown privileges needed to mysql database.
> ...
>> === added file 'sql/backup/restore_info.cc'
> ...
>> @@ -0,0 +1,226 @@
>> +/* Copyright (C) 2009 MySQL AB, 2009 Sun Microsystems, Inc.
>
>
> 15) IMHO the "2009 MySQL AB, " should go away.
> See https://inside.mysql.com/wiki/LegalInfoCopyright
> "
> Code newly developed since Oct 1 2008 should use only a Sun Microsystems
> copyright notice as follows:
>
> Copyright 2008 Sun Microsystems, Inc.
> "
Ok. This is extremely confusing because I see all manner of copyright
statements. I will use what you request and dare others to challenge.
>
> ...
>> +/**
>> + Perform privilege checking for restore.
>> +
>> + This method checks the known minimal privileges needed to restore each
>> + object in the backup image. This method is called once for every object in
>> + the catalog. It ensures there are no privilege-based restrictions that would
>> + prohibit a successful restore.
>> +
>> + It begins by checking that the user has RESTORE for the database. An error is
>> + generated if the user does not have RESTORE for any database in the backup
>> + image.
>
>
> 16) Hehe. You promised to make the sentence more clear. Formerly it was:
> "If the user does not have RESTORE on any database in the backup image,
> an error is generated". Now you have: "An error is generated if the user
> does not have RESTORE for any database in the backup image". This is no
> difference whatsoever in my perception.
>
> My discomfort comes from "any database". I feel like it must be "all
> databases".
No, it means any as in 'any single one'.
> I feel like "any" means "just one is enough". So the error would not be
> generated if the user has RESTORE on one of the databases.
Yes! That is correct. Any means 'just one' or even 'the first one'. This
is how the code is written. When the first database is encountered that
the user does not have RESTORE_ACL (the security check fails), the code
will throw an error and fail.
> Please fix or confirm that I don't understand English.
>
The sentence semantics are very clear and they are proper English (I
asked). As it is written, it means if the user does not have RESTORE_ACL
on any single database in the backup image, restore will fail. Please
understand this implies that the first encounter is enough to fail.
For the sake of clarity I change the wording to this:
It begins by checking that the user has RESTORE for each database listed
in the backup image. If the user does not have RESTORE for any database
in the list, an error is generated. Next, specific object-level
privilege checking is performed for all of the objects in the database.
The object-level privileges checked include the following.
I honestly don't know how I can make this any clearer.
> ...
>> === modified file 'sql/mysql_priv.h'
> ...
>> +extern my_bool opt_backup_elevation;
>> +extern my_bool opt_restore_elevation;
>
>
> 17) There is no reason to declare these for the whole server.
> Declaring them in a backup header would be sufficient, yes preferable.
>
Done. Moved to api_types.h. I am not sure this is the right place but it
is used ubiquitously in backup so there it goes.
> ...
>> === modified file 'sql/mysqld.cc'
> ...
>> +my_bool opt_backup_elevation= 1;
>> +my_bool opt_restore_elevation= 1;
>
>
> 18) Please initialize [my_]bool variables with TRUE or FALSE.
>
Done. Note: This breaks convention in the code (not that it matters to
me, BTW).
> ...
>> === modified file 'sql/set_var.cc'
> ...
>> +static sys_var_const sys_backup_elevation(&vars, "backup_elevation",
>> + OPT_GLOBAL, SHOW_BOOL,
>> + (uchar*)
> &opt_backup_elevation);
> ...
>> +static sys_var_const sys_restore_elevation(&vars, "restore_elevation",
>> + OPT_GLOBAL, SHOW_BOOL,
>> + (uchar*)
> &opt_restore_elevation);
>> +static sys_var_thd_bool sys_restore_precheck(&vars, "restore_precheck",
>> + &SV::restore_precheck);
>
>
> 17) At this place we could code more similar to replication. No part of
> the server, but BACKUP/RESTORE, needs to know about the new variables.
> Replication defines its variables in sql_repl.cc. Likewise we could
> define our variables in one of our sources.
>
Ok, well I just moved the external declarations is all.
> ...
>> === modified file 'sql/sql_class.cc'
> ...
>> +// Turns off grants but keeps user context.
>> +void Security_context::set_all_global_privileges()
>> +{
>> + master_access= (ulong)~NO_ACCESS;
>> +}
>
>
> 19) Did I already state my personal disagreement for the records? I
> yield to the team decision. But I don't agree that we need to copy the
> replication behavior in such detail that we keep a one-line function
> equal to the equivalent one-line function used by replication. We do not
> use the same function as replication uses as we need to do small
> changes. So it would be no problem to exclude privileges, which
> BACKUP/RESTORE *never* will need, just because replication might need
> them in its own function.
>
Understood. Do you expect something from me on this?
Chuck
