Use the password_manager encryptor to encrypt the sync credentials under

Use the password_manager encryptor to encrypt the sync credentials under Linux and OSX.

BUG=none
TEST=Enable bookmark sync, close and re-launch chrome, make sure there are no sync error message at the bottom of the NTP.

Original patch by albertb@chromium.org
Committed by davemoore@chromium.org

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

[albertb, 2009-10-27 23:45:35]

I moved 303029 to a checkout that can commit. Also, since I can only use
davemoore's machine once a week to test on Windows, I've reverted the Windows
specific parts. Once I get my own Windows machine, I'll send you those changes.

[Zachary Kuznia, 2009-10-28 00:13:56]

http://codereview.chromium.org/341012/diff/2001/2002
File chrome/browser/sync/util/user_settings.cc (right):

http://codereview.chromium.org/341012/diff/2001/2002#newcode372
Line 372: #endif  // !defined(OS_WIN)
Why did you move these functions out of the platform specific file?

[albertb, 2009-10-28 01:01:42]

http://codereview.chromium.org/341012/diff/2001/2002
File chrome/browser/sync/util/user_settings.cc (right):

http://codereview.chromium.org/341012/diff/2001/2002#newcode372
Line 372: #endif  // !defined(OS_WIN)
On 2009/10/28 00:13:56, zork wrote:
> Why did you move these functions out of the platform specific file?

Oops, because I used my previous patch as a starting point. Moved back to
posix.cc.

[Zachary Kuznia, 2009-10-28 16:59:48]

http://codereview.chromium.org/341012/diff/4002/4003
File chrome/browser/sync/util/user_settings.cc (right):

http://codereview.chromium.org/341012/diff/4002/4003#newcode12
Line 12: #include <algorithm>
Nothing else has changed in this file; why update the includes?

http://codereview.chromium.org/341012/diff/4002/4004
File chrome/browser/sync/util/user_settings_posix.cc (right):

http://codereview.chromium.org/341012/diff/4002/4004#newcode20
Line 20: &encrypted_service_token))
nit: Put body of if statement in {}

http://codereview.chromium.org/341012/diff/4002/4004#newcode21
Line 21: LOG(ERROR) << "Encrytion failed: " << long_lived_service_token;
You should either make this an assert, or return here on fail.  Otherwise, we're
inserting a bad value into the database.

http://codereview.chromium.org/341012/diff/4002/4004#newcode36
Line 36: "SELECT email FROM cookies"));
Shouldn't this have an ORDER BY?

http://codereview.chromium.org/341012/diff/4002/4004#newcode59
Line 59: if (!Encryptor::DecryptString(encrypted_service_token, service_token))
nit: {}

http://codereview.chromium.org/341012/diff/4002/4004#newcode60
Line 60: LOG(ERROR) << "Decryption failed: " << encrypted_service_token;
We should probably return false if the decryption fails.  Also, username
shouldn't get set if the function fails here.

[albertb, 2009-10-28 19:01:42]

http://codereview.chromium.org/341012/diff/4002/4004
File chrome/browser/sync/util/user_settings_posix.cc (right):

http://codereview.chromium.org/341012/diff/4002/4004#newcode20
Line 20: &encrypted_service_token))
On 2009/10/28 16:59:48, zork wrote:
> nit: Put body of if statement in {}

Done.

http://codereview.chromium.org/341012/diff/4002/4004#newcode21
Line 21: LOG(ERROR) << "Encrytion failed: " << long_lived_service_token;
On 2009/10/28 16:59:48, zork wrote:
> You should either make this an assert, or return here on fail.  Otherwise,
we're
> inserting a bad value into the database.

Done.

http://codereview.chromium.org/341012/diff/4002/4004#newcode36
Line 36: "SELECT email FROM cookies"));
On 2009/10/28 16:59:48, zork wrote:
> Shouldn't this have an ORDER BY?

The cookies table doesn't have any timestamp that I can ORDER BY,
user_settings_win.cc has the same problem.

http://codereview.chromium.org/341012/diff/4002/4004#newcode59
Line 59: if (!Encryptor::DecryptString(encrypted_service_token, service_token))
On 2009/10/28 16:59:48, zork wrote:
> nit: {}

Done.

http://codereview.chromium.org/341012/diff/4002/4004#newcode60
Line 60: LOG(ERROR) << "Decryption failed: " << encrypted_service_token;
On 2009/10/28 16:59:48, zork wrote:
> We should probably return false if the decryption fails.  Also, username
> shouldn't get set if the function fails here.

Done.

[Zachary Kuznia, 2009-10-28 19:57:12]

LGTM