Register ActiveDocument in "Secure Mime Handlers" key in HKLM!

Register ActiveDocument in "Secure Mime Handlers" key in HKLM!

BUG=23549
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=28184

[amit, 2009-10-06 01:03:48]

This looks like a test code just copied from the test app, would you please
clean it up? Please consider making helper classes more generic and moving to
utils.

http://codereview.chromium.org/260015/diff/1/2
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/260015/diff/1/2#newcode315
Line 315: #include <atlsecurity.h>
headers, constants to the top please

http://codereview.chromium.org/260015/diff/1/2#newcode370
Line 370: wchar_t* ReadBackupKey() {
Wouldn't it be better if this helper returned bool and took std::string*
'sddl_string' as an argument? You can simply pass the 'sddl_string' to
key.Read()

http://codereview.chromium.org/260015/diff/1/2#newcode371
Line 371: RegKey key(HKEY_LOCAL_MACHINE, backup_key_.c_str(), KEY_READ);
RegKey backup_key?

http://codereview.chromium.org/260015/diff/1/2#newcode397
Line 397: std::wstring backup_key_;
backup_key_name_?

http://codereview.chromium.org/260015/diff/1/2#newcode438
Line 438: RegKey k(HKEY_LOCAL_MACHINE, key.c_str(), KEY_READ | KEY_WRITE);
please choose better names than 'k', here and elsewhere

[stoyan, 2009-10-06 18:08:53]

http://codereview.chromium.org/260015/diff/1/2
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/260015/diff/1/2#newcode315
Line 315: #include <atlsecurity.h>
On 2009/10/06 01:03:48, amit wrote:
> headers, constants to the top please

Done.

http://codereview.chromium.org/260015/diff/1/2#newcode370
Line 370: wchar_t* ReadBackupKey() {
RegKey::Read(std::string*) is limited to 1024 characters.

http://codereview.chromium.org/260015/diff/1/2#newcode371
Line 371: RegKey key(HKEY_LOCAL_MACHINE, backup_key_.c_str(), KEY_READ);
On 2009/10/06 01:03:48, amit wrote:
> RegKey backup_key?

Done.

http://codereview.chromium.org/260015/diff/1/2#newcode397
Line 397: std::wstring backup_key_;
On 2009/10/06 01:03:48, amit wrote:
> backup_key_name_?

Done.

http://codereview.chromium.org/260015/diff/1/2#newcode438
Line 438: RegKey k(HKEY_LOCAL_MACHINE, key.c_str(), KEY_READ | KEY_WRITE);
On 2009/10/06 01:03:48, amit wrote:
> please choose better names than 'k', here and elsewhere

Done.

[amit, 2009-10-06 19:29:30]

looks better, some more comments.

http://codereview.chromium.org/260015/diff/7003/8001
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/260015/diff/7003/8001#newcode28
Line 28: #include <atlsecurity.h>
shouldn't both of these includes be at the top?

http://codereview.chromium.org/260015/diff/7003/8001#newcode322
Line 322: explicit SecurityDescBackup(const std::wstring backup_key)
const std::wstring&?

http://codereview.chromium.org/260015/diff/7003/8001#newcode332
Line 332: RegKey key(HKEY_LOCAL_MACHINE, backup_key_name_.c_str(), KEY_READ |
KEY_WRITE);
nit: 80, key -> backup_key

http://codereview.chromium.org/260015/diff/7003/8001#newcode352
Line 352: // Restore DACL of the key from saved security descriptor.
Can use SetNamedSecurityInfo to set everything in one go?

http://codereview.chromium.org/260015/diff/7003/8001#newcode375
Line 375: // If the key exists, something bad happened.
Really?

http://codereview.chromium.org/260015/diff/7003/8001#newcode385
Line 385: if (!backup_key.ReadValue(NULL, WriteInto(sddl, wchar_count), &len,
&reg_type)) {
RegKey has ReadValue method that reads into std::string

http://codereview.chromium.org/260015/diff/7003/8001#newcode401
Line 401: token_.GetProcessToken(TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY);
Should we be using thread token and go to process token if there's no thread
token? CAccessToken::GetEffectiveToken should be handy here.

http://codereview.chromium.org/260015/diff/7003/8001#newcode455
Line 455: mime_key.append(L"\\Secure Mime Handlers");
This is being done twice, better to define another string constant. Also, move
this near the place of use?

http://codereview.chromium.org/260015/diff/7003/8001#newcode473
Line 473: backup.RestoreSecurity(object_name.c_str());
Instead of restoring and then restoring again, we can create a backup only if it
does not exist and always restore it only at the end.

[stoyan, 2009-10-06 21:20:56]

http://codereview.chromium.org/260015/diff/7003/8001
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/260015/diff/7003/8001#newcode322
Line 322: explicit SecurityDescBackup(const std::wstring backup_key)
On 2009/10/06 19:29:30, amit wrote:
> const std::wstring&?

Done.

http://codereview.chromium.org/260015/diff/7003/8001#newcode332
Line 332: RegKey key(HKEY_LOCAL_MACHINE, backup_key_name_.c_str(), KEY_READ |
KEY_WRITE);
On 2009/10/06 19:29:30, amit wrote:
> nit: 80, key -> backup_key

Done.

http://codereview.chromium.org/260015/diff/7003/8001#newcode352
Line 352: // Restore DACL of the key from saved security descriptor.
On 2009/10/06 19:29:30, amit wrote:
> Can use SetNamedSecurityInfo to set everything in one go?

Done.

http://codereview.chromium.org/260015/diff/7003/8001#newcode385
Line 385: if (!backup_key.ReadValue(NULL, WriteInto(sddl, wchar_count), &len,
&reg_type)) {
It's limited to 1024 chars.

http://codereview.chromium.org/260015/diff/7003/8001#newcode401
Line 401: token_.GetProcessToken(TOKEN_ADJUST_PRIVILEGES | TOKEN_QUERY);
On 2009/10/06 19:29:30, amit wrote:
> Should we be using thread token and go to process token if there's no thread
> token? CAccessToken::GetEffectiveToken should be handy here.

Done.

http://codereview.chromium.org/260015/diff/7003/8001#newcode455
Line 455: mime_key.append(L"\\Secure Mime Handlers");
On 2009/10/06 19:29:30, amit wrote:
> This is being done twice, better to define another string constant. Also, move
> this near the place of use?

Done.

http://codereview.chromium.org/260015/diff/7003/8001#newcode473
Line 473: backup.RestoreSecurity(object_name.c_str());
Lets keep it as it is for now. Seems more straightforward.

[amit, 2009-10-06 23:20:55]

lgtm!

http://codereview.chromium.org/260015/diff/2003/2004
File chrome_frame/chrome_tab.cc (right):

http://codereview.chromium.org/260015/diff/2003/2004#newcode375
Line 375: // If the key exists, something bad happened.
I think we expect the key to be valid here, right?