Ingo,
Alas, but this was the wrong patch. Let us all remember the human
element of our job and make the best of a difficult review cycle.
That being said, only some of your comments apply to the correct patch.
So take heart in your review was helpful nonetheless.
I also have responses for two of your comments.
> Status: Approved pending changes.
>
> Required:
> 8) Indentation glitches (twice).
Only the ones in mysqld.cc apply all others were fixed.
> Request:
> 1) Improve revision comment. (like last patch)
Done.
> 2) Simplify revision comment.
Done.
> 3) Remove tests, if duplicate.
See below.
> 4) Narrow MTR suppressions. (twice)
Done. Note: I did goof and leave them out of the patch.
> 5) Improve test case comment.
Done.
> 7) Suggestion for code improvement in ACL elevation/restore.
See below.
> Options:
> 6) End-space.
Not applicable -- was fixed in correct patch.
>
> Details:
> Chuck Bell, 02.11.2009 23:28:
> ...
>
>> 2889 Chuck Bell 2009-11-02
>> BUG#44787 : Backup: Check privileges before executing BACKUP/RESTORE
This is the wrong patch. It should have been the one from 2009-11-05.
...
>> +# Test Cases
>> +# ----------
>> +# 1. Ensure backup_elevation = OFF fails for not enough privileges.
>> +# 2. Ensure restore_elevation = OFF fails for not enough privileges.
>> +# 3. Ensure restore_precheck = OFF fails for not enough privileges
>> +# (Restore will fail in the middle).
>
>
> 3) The difference in the tests for 2 and 3 are small. We don't really
> see that RESTORE fails differently. I suggest to show the existing
> objects after RESTORE. So one should see that all do still exist after
> failed RESTORE in 2, but some are dropped in 3.
I put this test case in because with restore elevation off the
prechecking code still runs. I want to show the patch through the code
with prechecking off still generates an error -- that by turning both
off the failure occurs elsewhere in the code and that deserves a special
test case.
> I understand that this test shall show that the command line options
> work. However, during the test case we toggle the values using the
> system variables. The tests themselves seem to be covered by
> backup_security_var.test mostly. If this is correct, we could keep this
> test case very short. Just show that all options are off, and start
> backup_security_var.test with showing that all options are on.
All options are ON for the backup_security_var test. In this test I
merely show the startup options control the same variable and that they
can be reset and the code works as expected. This is also good to
include if someone changes the startup options and/or tries to make the
variables read only or some such. I think the test should stand as is.
...
>> + if (!check_access(info->m_thd, RESTORE_ACL, name_str, 0, 1, 1, 0)
> &&
>> + (info->m_thd->security_ctx->master_access & SUPER_ACL)
> &&
>> + info->m_skip_precheck)
>
>
> 7) Hm. Do I understand this correctly?: With restore_elevation we set
> all global privileges, including RESTORE_ACL and SUPER_ACL. Now we need
> to know, if the user has these privileges natively. Hence we restore the
> original privileges and check for these two.
Elevation is turned off prior to this check. This check is performed per
database. The fact that a user had RESTORE+SUPER for one of the
databases does *not* mean he has RESTORE for all of the databases. Thus,
with each check elevation must be turned off so that the RESTORE can be
checked for each database.
> Great. That should work. But why the hack do we set RESTORE_ACL and
> SUPER_ACL in the first place? Wouldn't it be nicer to have this in
> set_all_global_privileges() (aka skip_grants_user())?:
>
> master_access|= ~(BACKUP_ACL | RESTORE_ACL | SUPER_ACL);
>
> Wouldn't this avoid the back and forth?
> [Well, for BACKUP, we might want to elevate SUPER_ACL too. So we might
> need two similar functions, or a parametrized one.]
No, that is not how the elevation works. Elevation works by turning on
ALL privileges so that no matter what a statement will not fail for
privilege issues. If we assume the user has RESTORE+SUPER on one
database and elevate that means he can restore other databases in the
image that he otherwise might not have RESTORE on.
All that being said, I suppose I could rearrange the code to check for
SUPER once but that would mean saving the result so that it can be
checked again later. I don't see the gain in optimization however.
Chuck
