diff options
author | zea@chromium.org <zea@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-08-24 22:54:19 +0000 |
---|---|---|
committer | zea@chromium.org <zea@chromium.org@0039d316-1c4b-4281-b951-d872f2087c98> | 2012-08-24 22:54:19 +0000 |
commit | 69b3b8f04b759b700b78987fd74cd16c081e6d0e (patch) | |
tree | 03942e40523cfc328b66d77186e0d1552c19beb9 | |
parent | c09eeea393004f61f55adca9aeda4aa387cee903 (diff) | |
download | chromium_src-69b3b8f04b759b700b78987fd74cd16c081e6d0e.zip chromium_src-69b3b8f04b759b700b78987fd74cd16c081e6d0e.tar.gz chromium_src-69b3b8f04b759b700b78987fd74cd16c081e6d0e.tar.bz2 |
[Sync] Refactor GetEncryptedTypes usage.
The cryptographer is now owned by the SyncEncryptionHandlerImpl, and
no longer needs any connection to the nigori handler or encrypted types.
Those are accessed via the directory, which now has a GetNigoriHandler
method.
Because of this, we can now clean up the thread safety guarantees in the
sync encryption handler impl, enforcing that everything except
GetEncryptedTypes (and IsUsingExplicitPassphrase, which will be fixed in a
followup patch) are invoked on the syncer thread. This lets us not have to
open unnecessary transactions.
At the chrome layer, encrypted types are accessed via BaseTransaction::
GetEncryptedTypes(). At the syncer layer, they're accessed via directory::
GetNigoriHandler()->GetEncryptedTypes(BaseTransaction* trans const).
BUG=142476,139848
Review URL: https://chromiumcodereview.appspot.com/10844005
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@153335 0039d316-1c4b-4281-b951-d872f2087c98
39 files changed, 568 insertions, 416 deletions
diff --git a/chrome/browser/sync/DEPS b/chrome/browser/sync/DEPS index 2c53c1b2..15a1111 100644 --- a/chrome/browser/sync/DEPS +++ b/chrome/browser/sync/DEPS @@ -13,7 +13,4 @@ include_rules = [ "+sync/syncable", # Used by autofill tests, which should use the public sync API instead. "+sync/test/engine/test_id_factory.h", - # Bug 142476. Used by profile sync service bookmark test, which should rely on - # TestProfileSyncService instead. - "+sync/test/fake_sync_encryption_handler.h", ] diff --git a/chrome/browser/sync/glue/bookmark_change_processor.cc b/chrome/browser/sync/glue/bookmark_change_processor.cc index b382eae..cca79d0 100644 --- a/chrome/browser/sync/glue/bookmark_change_processor.cc +++ b/chrome/browser/sync/glue/bookmark_change_processor.cc @@ -233,7 +233,7 @@ void BookmarkChangeProcessor::BookmarkNodeChanged(BookmarkModel* model, LOG(ERROR) << "Deleted entry."; } else { syncer::Cryptographer* crypto = trans.GetCryptographer(); - syncer::ModelTypeSet encrypted_types(crypto->GetEncryptedTypes()); + syncer::ModelTypeSet encrypted_types(trans.GetEncryptedTypes()); const sync_pb::EntitySpecifics& specifics = sync_node.GetEntry()->Get(syncer::syncable::SPECIFICS); CHECK(specifics.has_encrypted()); diff --git a/chrome/browser/sync/glue/bookmark_model_associator.cc b/chrome/browser/sync/glue/bookmark_model_associator.cc index 79ae543..5040400 100644 --- a/chrome/browser/sync/glue/bookmark_model_associator.cc +++ b/chrome/browser/sync/glue/bookmark_model_associator.cc @@ -664,8 +664,7 @@ bool BookmarkModelAssociator::LoadAssociations() { bool BookmarkModelAssociator::CryptoReadyIfNecessary() { // We only access the cryptographer while holding a transaction. syncer::ReadTransaction trans(FROM_HERE, user_share_); - const syncer::ModelTypeSet encrypted_types = - syncer::GetEncryptedTypes(&trans); + const syncer::ModelTypeSet encrypted_types = trans.GetEncryptedTypes(); return !encrypted_types.Has(syncer::BOOKMARKS) || trans.GetCryptographer()->is_ready(); } diff --git a/chrome/browser/sync/glue/generic_change_processor.cc b/chrome/browser/sync/glue/generic_change_processor.cc index 4cdfb9e..ef081c4 100644 --- a/chrome/browser/sync/glue/generic_change_processor.cc +++ b/chrome/browser/sync/glue/generic_change_processor.cc @@ -376,7 +376,7 @@ syncer::SyncError GenericChangeProcessor::ProcessSyncChanges( return error; } else { syncer::Cryptographer* crypto = trans.GetCryptographer(); - syncer::ModelTypeSet encrypted_types(crypto->GetEncryptedTypes()); + syncer::ModelTypeSet encrypted_types(trans.GetEncryptedTypes()); const sync_pb::EntitySpecifics& specifics = sync_node.GetEntry()->Get(syncer::syncable::SPECIFICS); CHECK(specifics.has_encrypted()); @@ -475,7 +475,7 @@ bool GenericChangeProcessor::CryptoReadyIfNecessary(syncer::ModelType type) { DCHECK_NE(type, syncer::UNSPECIFIED); // We only access the cryptographer while holding a transaction. syncer::ReadTransaction trans(FROM_HERE, share_handle()); - const syncer::ModelTypeSet encrypted_types = GetEncryptedTypes(&trans); + const syncer::ModelTypeSet encrypted_types = trans.GetEncryptedTypes(); return !encrypted_types.Has(type) || trans.GetCryptographer()->is_ready(); } diff --git a/chrome/browser/sync/glue/session_model_associator.cc b/chrome/browser/sync/glue/session_model_associator.cc index 6efb308..9c690d3 100644 --- a/chrome/browser/sync/glue/session_model_associator.cc +++ b/chrome/browser/sync/glue/session_model_associator.cc @@ -1595,8 +1595,7 @@ void SessionModelAssociator::BlockUntilLocalChangeForTest( bool SessionModelAssociator::CryptoReadyIfNecessary() { // We only access the cryptographer while holding a transaction. syncer::ReadTransaction trans(FROM_HERE, sync_service_->GetUserShare()); - const syncer::ModelTypeSet encrypted_types = - syncer::GetEncryptedTypes(&trans); + const syncer::ModelTypeSet encrypted_types = trans.GetEncryptedTypes(); return !encrypted_types.Has(SESSIONS) || sync_service_->IsCryptographerReady(&trans); } diff --git a/chrome/browser/sync/glue/theme_model_associator.cc b/chrome/browser/sync/glue/theme_model_associator.cc index 2c6dd3c..6af9c35 100644 --- a/chrome/browser/sync/glue/theme_model_associator.cc +++ b/chrome/browser/sync/glue/theme_model_associator.cc @@ -109,8 +109,7 @@ bool ThemeModelAssociator::SyncModelHasUserCreatedNodes(bool* has_nodes) { bool ThemeModelAssociator::CryptoReadyIfNecessary() { // We only access the cryptographer while holding a transaction. syncer::ReadTransaction trans(FROM_HERE, sync_service_->GetUserShare()); - const syncer::ModelTypeSet encrypted_types = - syncer::GetEncryptedTypes(&trans); + const syncer::ModelTypeSet encrypted_types = trans.GetEncryptedTypes(); return !encrypted_types.Has(syncer::THEMES) || sync_service_->IsCryptographerReady(&trans); } diff --git a/chrome/browser/sync/glue/typed_url_change_processor.cc b/chrome/browser/sync/glue/typed_url_change_processor.cc index c98459e..c0fa9b37 100644 --- a/chrome/browser/sync/glue/typed_url_change_processor.cc +++ b/chrome/browser/sync/glue/typed_url_change_processor.cc @@ -137,7 +137,7 @@ bool TypedUrlChangeProcessor::CreateOrUpdateSyncNode( } else if (result == syncer::BaseNode::INIT_FAILED_DECRYPT_IF_NECESSARY) { // TODO(tim): Investigating bug 121587. syncer::Cryptographer* crypto = trans->GetCryptographer(); - syncer::ModelTypeSet encrypted_types(crypto->GetEncryptedTypes()); + syncer::ModelTypeSet encrypted_types(trans->GetEncryptedTypes()); const sync_pb::EntitySpecifics& specifics = update_node.GetEntry()->Get(syncer::syncable::SPECIFICS); CHECK(specifics.has_encrypted()); diff --git a/chrome/browser/sync/glue/typed_url_model_associator.cc b/chrome/browser/sync/glue/typed_url_model_associator.cc index c303188..0d58f38 100644 --- a/chrome/browser/sync/glue/typed_url_model_associator.cc +++ b/chrome/browser/sync/glue/typed_url_model_associator.cc @@ -826,8 +826,7 @@ void TypedUrlModelAssociator::UpdateURLRowFromTypedUrlSpecifics( bool TypedUrlModelAssociator::CryptoReadyIfNecessary() { // We only access the cryptographer while holding a transaction. syncer::ReadTransaction trans(FROM_HERE, sync_service_->GetUserShare()); - const syncer::ModelTypeSet encrypted_types = - syncer::GetEncryptedTypes(&trans); + const syncer::ModelTypeSet encrypted_types = trans.GetEncryptedTypes(); return !encrypted_types.Has(syncer::TYPED_URLS) || sync_service_->IsCryptographerReady(&trans); } diff --git a/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc b/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc index 4630511..d20016c 100644 --- a/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_bookmark_unittest.cc @@ -41,7 +41,6 @@ #include "sync/test/engine/test_id_factory.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" -#include "sync/test/fake_sync_encryption_handler.h" namespace browser_sync { @@ -346,19 +345,12 @@ class ProfileSyncServiceBookmarkTest : public testing::Test { virtual void SetUp() { test_user_share_.SetUp(); - SetUpEncryption(); } virtual void TearDown() { test_user_share_.TearDown(); } - void SetUpEncryption() { - syncer::ReadTransaction trans(FROM_HERE, test_user_share_.user_share()); - fake_encryption_handler_.set_cryptographer(trans.GetCryptographer()); - trans.GetCryptographer()->SetNigoriHandler(&fake_encryption_handler_); - } - // Load (or re-load) the bookmark model. |load| controls use of the // bookmarks file on disk. |save| controls whether the newly loaded // bookmark model will write out a bookmark file as it goes. @@ -573,7 +565,6 @@ class ProfileSyncServiceBookmarkTest : public testing::Test { syncer::TestUserShare test_user_share_; scoped_ptr<BookmarkChangeProcessor> change_processor_; StrictMock<DataTypeErrorHandlerMock> mock_error_handler_; - syncer::FakeSyncEncryptionHandler fake_encryption_handler_; }; TEST_F(ProfileSyncServiceBookmarkTest, InitialState) { diff --git a/sync/engine/apply_updates_command_unittest.cc b/sync/engine/apply_updates_command_unittest.cc index 91fa259..5e14c59 100644 --- a/sync/engine/apply_updates_command_unittest.cc +++ b/sync/engine/apply_updates_command_unittest.cc @@ -65,22 +65,14 @@ class ApplyUpdatesCommandTest : public SyncerCommandTest { SyncerCommandTest::SetUp(); entry_factory_.reset(new TestEntryFactory(directory())); ExpectNoGroupsToChange(apply_updates_command_); - - syncable::ReadTransaction trans(FROM_HERE, directory()); - directory()->GetCryptographer(&trans)->SetNigoriHandler( - &fake_encryption_handler_); - fake_encryption_handler_.set_cryptographer( - directory()->GetCryptographer(&trans)); } protected: DISALLOW_COPY_AND_ASSIGN(ApplyUpdatesCommandTest); ApplyUpdatesCommand apply_updates_command_; - FakeEncryptor encryptor_; TestIdFactory id_factory_; scoped_ptr<TestEntryFactory> entry_factory_; - FakeSyncEncryptionHandler fake_encryption_handler_; }; TEST_F(ApplyUpdatesCommandTest, Simple) { @@ -466,6 +458,7 @@ TEST_F(ApplyUpdatesCommandTest, UndecryptableData) { } TEST_F(ApplyUpdatesCommandTest, SomeUndecryptablePassword) { + Cryptographer* cryptographer; // Only decryptable password updates should be applied. { sync_pb::EntitySpecifics specifics; @@ -473,7 +466,7 @@ TEST_F(ApplyUpdatesCommandTest, SomeUndecryptablePassword) { data.set_origin("http://example.com/1"); { syncable::ReadTransaction trans(FROM_HERE, directory()); - Cryptographer* cryptographer = directory()->GetCryptographer(&trans); + cryptographer = directory()->GetCryptographer(&trans); KeyParams params = {"localhost", "dummy", "foobar"}; cryptographer->AddKey(params); @@ -485,15 +478,15 @@ TEST_F(ApplyUpdatesCommandTest, SomeUndecryptablePassword) { } { // Create a new cryptographer, independent of the one in the session. - Cryptographer cryptographer(&encryptor_); + Cryptographer other_cryptographer(cryptographer->encryptor()); KeyParams params = {"localhost", "dummy", "bazqux"}; - cryptographer.AddKey(params); + other_cryptographer.AddKey(params); sync_pb::EntitySpecifics specifics; sync_pb::PasswordSpecificsData data; data.set_origin("http://example.com/2"); - cryptographer.Encrypt(data, + other_cryptographer.Encrypt(data, specifics.mutable_password()->mutable_encrypted()); entry_factory_->CreateUnappliedNewItem("item2", specifics, false); } @@ -532,11 +525,12 @@ TEST_F(ApplyUpdatesCommandTest, NigoriUpdate) { { syncable::ReadTransaction trans(FROM_HERE, directory()); cryptographer = directory()->GetCryptographer(&trans); - EXPECT_TRUE(cryptographer->GetEncryptedTypes().Equals(encrypted_types)); + EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) + .Equals(encrypted_types)); } // Nigori node updates should update the Cryptographer. - Cryptographer other_cryptographer(&encryptor_); + Cryptographer other_cryptographer(cryptographer->encryptor()); KeyParams params = {"localhost", "dummy", "foobar"}; other_cryptographer.AddKey(params); @@ -564,7 +558,11 @@ TEST_F(ApplyUpdatesCommandTest, NigoriUpdate) { EXPECT_FALSE(cryptographer->is_ready()); EXPECT_TRUE(cryptographer->has_pending_keys()); - EXPECT_TRUE(cryptographer->GetEncryptedTypes().Equals(ModelTypeSet::All())); + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) + .Equals(ModelTypeSet::All())); + } } TEST_F(ApplyUpdatesCommandTest, NigoriUpdateForDisabledTypes) { @@ -577,11 +575,12 @@ TEST_F(ApplyUpdatesCommandTest, NigoriUpdateForDisabledTypes) { { syncable::ReadTransaction trans(FROM_HERE, directory()); cryptographer = directory()->GetCryptographer(&trans); - EXPECT_TRUE(cryptographer->GetEncryptedTypes().Equals(encrypted_types)); + EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) + .Equals(encrypted_types)); } // Nigori node updates should update the Cryptographer. - Cryptographer other_cryptographer(&encryptor_); + Cryptographer other_cryptographer(cryptographer->encryptor()); KeyParams params = {"localhost", "dummy", "foobar"}; other_cryptographer.AddKey(params); @@ -609,7 +608,11 @@ TEST_F(ApplyUpdatesCommandTest, NigoriUpdateForDisabledTypes) { EXPECT_FALSE(cryptographer->is_ready()); EXPECT_TRUE(cryptographer->has_pending_keys()); - EXPECT_TRUE(cryptographer->GetEncryptedTypes().Equals(ModelTypeSet::All())); + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) + .Equals(ModelTypeSet::All())); + } } // Create some local unsynced and unencrypted data. Apply a nigori update that @@ -627,7 +630,8 @@ TEST_F(ApplyUpdatesCommandTest, EncryptUnsyncedChanges) { { syncable::ReadTransaction trans(FROM_HERE, directory()); cryptographer = directory()->GetCryptographer(&trans); - EXPECT_TRUE(cryptographer->GetEncryptedTypes().Equals(encrypted_types)); + EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) + .Equals(encrypted_types)); // With default encrypted_types, this should be true. EXPECT_TRUE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types)); @@ -704,8 +708,8 @@ TEST_F(ApplyUpdatesCommandTest, EncryptUnsyncedChanges) { // If ProcessUnsyncedChangesForEncryption worked, all our unsynced changes // should be encrypted now. - EXPECT_TRUE(ModelTypeSet::All().Equals( - cryptographer->GetEncryptedTypes())); + EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) + .Equals(ModelTypeSet::All())); EXPECT_TRUE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types)); Syncer::UnsyncedMetaHandles handles; @@ -744,8 +748,8 @@ TEST_F(ApplyUpdatesCommandTest, EncryptUnsyncedChanges) { syncable::ReadTransaction trans(FROM_HERE, directory()); // All our changes should still be encrypted. - EXPECT_TRUE(ModelTypeSet::All().Equals( - cryptographer->GetEncryptedTypes())); + EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) + .Equals(ModelTypeSet::All())); EXPECT_TRUE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types)); Syncer::UnsyncedMetaHandles handles; @@ -764,7 +768,8 @@ TEST_F(ApplyUpdatesCommandTest, CannotEncryptUnsyncedChanges) { { syncable::ReadTransaction trans(FROM_HERE, directory()); cryptographer = directory()->GetCryptographer(&trans); - EXPECT_TRUE(cryptographer->GetEncryptedTypes().Equals(encrypted_types)); + EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) + .Equals(encrypted_types)); // With default encrypted_types, this should be true. EXPECT_TRUE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types)); @@ -798,7 +803,7 @@ TEST_F(ApplyUpdatesCommandTest, CannotEncryptUnsyncedChanges) { // We encrypt with new keys, triggering the local cryptographer to be unready // and unable to decrypt data (once updated). - Cryptographer other_cryptographer(&encryptor_); + Cryptographer other_cryptographer(cryptographer->encryptor()); KeyParams params = {"localhost", "dummy", "foobar"}; other_cryptographer.AddKey(params); sync_pb::EntitySpecifics specifics; @@ -844,8 +849,8 @@ TEST_F(ApplyUpdatesCommandTest, CannotEncryptUnsyncedChanges) { // Since we have pending keys, we would have failed to encrypt, but the // cryptographer should be updated. EXPECT_FALSE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types)); - EXPECT_TRUE(cryptographer->GetEncryptedTypes().Equals( - ModelTypeSet().All())); + EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) + .Equals(ModelTypeSet::All())); EXPECT_FALSE(cryptographer->is_ready()); EXPECT_TRUE(cryptographer->has_pending_keys()); diff --git a/sync/engine/conflict_resolver.cc b/sync/engine/conflict_resolver.cc index 5912e46..736e668 100644 --- a/sync/engine/conflict_resolver.cc +++ b/sync/engine/conflict_resolver.cc @@ -17,6 +17,7 @@ #include "sync/sessions/status_controller.h" #include "sync/syncable/directory.h" #include "sync/syncable/mutable_entry.h" +#include "sync/syncable/nigori_handler.h" #include "sync/syncable/write_transaction.h" #include "sync/util/cryptographer.h" @@ -229,7 +230,9 @@ ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans, sync_pb::NigoriSpecifics* server_nigori = specifics.mutable_nigori(); // Store the merged set of encrypted types (cryptographer->Update(..) will // have merged the local types already). - cryptographer->UpdateNigoriFromEncryptedTypes(server_nigori, trans); + trans->directory()->GetNigoriHandler()->UpdateNigoriFromEncryptedTypes( + server_nigori, + trans); // The cryptographer has the both the local and remote encryption keys // (added at cryptographer->Update(..) time). // If the cryptographer is ready, then it already merged both sets of keys diff --git a/sync/engine/download_updates_command.cc b/sync/engine/download_updates_command.cc index 0c413b6..fea8f88 100644 --- a/sync/engine/download_updates_command.cc +++ b/sync/engine/download_updates_command.cc @@ -12,6 +12,7 @@ #include "sync/internal_api/public/base/model_type_state_map.h" #include "sync/syncable/directory.h" #include "sync/syncable/read_transaction.h" +#include "sync/util/cryptographer.h" using sync_pb::DebugInfo; diff --git a/sync/engine/get_commit_ids_command.cc b/sync/engine/get_commit_ids_command.cc index a7a52dd..bb6d228 100644 --- a/sync/engine/get_commit_ids_command.cc +++ b/sync/engine/get_commit_ids_command.cc @@ -12,6 +12,7 @@ #include "sync/engine/throttled_data_type_tracker.h" #include "sync/syncable/entry.h" #include "sync/syncable/mutable_entry.h" +#include "sync/syncable/nigori_handler.h" #include "sync/syncable/nigori_util.h" #include "sync/syncable/syncable_util.h" #include "sync/syncable/write_transaction.h" @@ -49,7 +50,8 @@ SyncerError GetCommitIdsCommand::ExecuteImpl(SyncSession* session) { session->context()-> directory()->GetCryptographer(session->write_transaction()); if (cryptographer) { - encrypted_types = cryptographer->GetEncryptedTypes(); + encrypted_types = session->context()->directory()->GetNigoriHandler()-> + GetEncryptedTypes(session->write_transaction()); passphrase_missing = cryptographer->has_pending_keys(); }; diff --git a/sync/engine/syncer_unittest.cc b/sync/engine/syncer_unittest.cc index 506fa3f..df587a7 100644 --- a/sync/engine/syncer_unittest.cc +++ b/sync/engine/syncer_unittest.cc @@ -256,7 +256,6 @@ class SyncerTest : public testing::Test, child_id_ = ids_.MakeServer("child id"); directory()->set_store_birthday(mock_server_->store_birthday()); mock_server_->SetKeystoreKey("encryption_key"); - GetCryptographer(&trans)->SetNigoriHandler(&fake_encryption_handler_); } virtual void TearDown() { @@ -568,8 +567,6 @@ class SyncerTest : public testing::Test, ModelTypeSet enabled_datatypes_; TrafficRecorder traffic_recorder_; - FakeSyncEncryptionHandler fake_encryption_handler_; - DISALLOW_COPY_AND_ASSIGN(SyncerTest); }; @@ -727,7 +724,7 @@ TEST_F(SyncerTest, GetCommitIdsFiltersUnreadyEntries) { sync_pb::EntitySpecifics specifics; sync_pb::NigoriSpecifics* nigori = specifics.mutable_nigori(); other_cryptographer.GetKeys(nigori->mutable_encrypted()); - fake_encryption_handler_.EnableEncryptEverything(); + dir_maker_.encryption_handler()->EnableEncryptEverything(); // Set up with an old passphrase, but have pending keys GetCryptographer(&wtrans)->AddKey(key_params); GetCryptographer(&wtrans)->Encrypt(bookmark, @@ -840,7 +837,7 @@ TEST_F(SyncerTest, EncryptionAwareConflicts) { sync_pb::EntitySpecifics specifics; sync_pb::NigoriSpecifics* nigori = specifics.mutable_nigori(); other_cryptographer.GetKeys(nigori->mutable_encrypted()); - fake_encryption_handler_.EnableEncryptEverything(); + dir_maker_.encryption_handler()->EnableEncryptEverything(); GetCryptographer(&wtrans)->SetPendingKeys(nigori->encrypted()); EXPECT_TRUE(GetCryptographer(&wtrans)->has_pending_keys()); } @@ -1018,8 +1015,8 @@ TEST_F(SyncerTest, NigoriConflicts) { our_encrypted_specifics.mutable_encrypted()); GetCryptographer(&wtrans)->GetKeys( nigori->mutable_encrypted()); - fake_encryption_handler_.EnableEncryptEverything(); - GetCryptographer(&wtrans)->UpdateNigoriFromEncryptedTypes( + dir_maker_.encryption_handler()->EnableEncryptEverything(); + directory()->GetNigoriHandler()->UpdateNigoriFromEncryptedTypes( nigori, &wtrans); MutableEntry nigori_entry(&wtrans, GET_BY_SERVER_TAG, @@ -1029,8 +1026,7 @@ TEST_F(SyncerTest, NigoriConflicts) { nigori_entry.Put(IS_UNSYNCED, true); EXPECT_FALSE(GetCryptographer(&wtrans)->has_pending_keys()); EXPECT_TRUE(encrypted_types.Equals( - GetCryptographer(&wtrans)->GetEncryptedTypes())); - fake_encryption_handler_.set_cryptographer(GetCryptographer(&wtrans)); + directory()->GetNigoriHandler()->GetEncryptedTypes(&wtrans))); } { sync_pb::EntitySpecifics specifics; @@ -1061,8 +1057,8 @@ TEST_F(SyncerTest, NigoriConflicts) { sync_pb::EntitySpecifics specifics = nigori_entry.Get(SPECIFICS); ASSERT_TRUE(GetCryptographer(&wtrans)->has_pending_keys()); EXPECT_TRUE(encrypted_types.Equals( - GetCryptographer(&wtrans)->GetEncryptedTypes())); - EXPECT_TRUE(fake_encryption_handler_.EncryptEverythingEnabled()); + directory()->GetNigoriHandler()->GetEncryptedTypes(&wtrans))); + EXPECT_TRUE(dir_maker_.encryption_handler()->EncryptEverythingEnabled()); EXPECT_TRUE(specifics.nigori().using_explicit_passphrase()); // Supply the pending keys. Afterwards, we should be able to decrypt both // our own encrypted data and data encrypted by the other cryptographer, @@ -1072,7 +1068,9 @@ TEST_F(SyncerTest, NigoriConflicts) { EXPECT_FALSE(GetCryptographer(&wtrans)->has_pending_keys()); sync_pb::NigoriSpecifics* nigori = specifics.mutable_nigori(); GetCryptographer(&wtrans)->GetKeys(nigori->mutable_encrypted()); - GetCryptographer(&wtrans)->UpdateNigoriFromEncryptedTypes(nigori, &wtrans); + directory()->GetNigoriHandler()->UpdateNigoriFromEncryptedTypes( + nigori, + &wtrans); // Normally this would be written as part of SetPassphrase, but we do it // manually for the test. nigori_entry.Put(SPECIFICS, specifics); diff --git a/sync/engine/syncer_util.cc b/sync/engine/syncer_util.cc index ca21f2e..8a243df 100644 --- a/sync/engine/syncer_util.cc +++ b/sync/engine/syncer_util.cc @@ -22,6 +22,7 @@ #include "sync/syncable/directory.h" #include "sync/syncable/entry.h" #include "sync/syncable/mutable_entry.h" +#include "sync/syncable/nigori_handler.h" #include "sync/syncable/nigori_util.h" #include "sync/syncable/read_transaction.h" #include "sync/syncable/syncable_changes_version.h" @@ -217,7 +218,7 @@ UpdateAttemptResponse AttemptToUpdateEntry( // the nigori node (e.g. on restart), they will commit without issue. if (specifics.has_nigori()) { const sync_pb::NigoriSpecifics& nigori = specifics.nigori(); - cryptographer->ApplyNigoriUpdate(nigori, trans); + trans->directory()->GetNigoriHandler()->ApplyNigoriUpdate(nigori, trans); // Make sure any unsynced changes are properly encrypted as necessary. // We only perform this if the cryptographer is ready. If not, these are @@ -235,7 +236,7 @@ UpdateAttemptResponse AttemptToUpdateEntry( // If this fails, something is wrong with the cryptographer, but there's // nothing we can do about it here. DVLOG(1) << "Received new nigori, encrypting unsynced changes."; - syncable::ProcessUnsyncedChangesForEncryption(trans, cryptographer); + syncable::ProcessUnsyncedChangesForEncryption(trans); } } diff --git a/sync/internal_api/base_transaction.cc b/sync/internal_api/base_transaction.cc index 61c1f48..11a8ca4 100644 --- a/sync/internal_api/base_transaction.cc +++ b/sync/internal_api/base_transaction.cc @@ -5,6 +5,7 @@ #include "sync/internal_api/public/base_transaction.h" #include "sync/syncable/directory.h" +#include "sync/syncable/nigori_handler.h" #include "sync/util/cryptographer.h" namespace syncer { @@ -22,8 +23,9 @@ Cryptographer* BaseTransaction::GetCryptographer() const { return directory_->GetCryptographer(this->GetWrappedTrans()); } -ModelTypeSet GetEncryptedTypes(const BaseTransaction* trans) { - return trans->GetCryptographer()->GetEncryptedTypes(); +ModelTypeSet BaseTransaction::GetEncryptedTypes() const { + return directory_->GetNigoriHandler()->GetEncryptedTypes( + this->GetWrappedTrans()); } } // namespace syncer diff --git a/sync/internal_api/public/base_transaction.h b/sync/internal_api/public/base_transaction.h index f097fb3..242a6b9 100644 --- a/sync/internal_api/public/base_transaction.h +++ b/sync/internal_api/public/base_transaction.h @@ -7,6 +7,7 @@ #include "sync/internal_api/public/user_share.h" +#include "sync/internal_api/public/base/model_type.h" #include "sync/util/cryptographer.h" namespace syncer { @@ -28,6 +29,7 @@ class BaseTransaction { // Provide access to the underlying syncable objects from BaseNode. virtual syncable::BaseTransaction* GetWrappedTrans() const = 0; Cryptographer* GetCryptographer() const; + ModelTypeSet GetEncryptedTypes() const; syncable::Directory* GetDirectory() const { return directory_; @@ -45,8 +47,6 @@ class BaseTransaction { DISALLOW_COPY_AND_ASSIGN(BaseTransaction); }; -ModelTypeSet GetEncryptedTypes(const BaseTransaction* trans); - } // namespace syncer #endif // SYNC_INTERNAL_API_PUBLIC_BASE_TRANSACTION_H_ diff --git a/sync/internal_api/public/test/test_user_share.h b/sync/internal_api/public/test/test_user_share.h index ea811a1..5322d90 100644 --- a/sync/internal_api/public/test/test_user_share.h +++ b/sync/internal_api/public/test/test_user_share.h @@ -35,6 +35,8 @@ namespace syncer { +class SyncEncryptionHandler; + class TestDirectorySetterUpper; class TestUserShare { @@ -55,6 +57,10 @@ class TestUserShare { // Non-NULL iff called between a call to SetUp() and TearDown(). UserShare* user_share(); + // Sync's encryption handler. Used by tests to invoke the sync encryption + // methods normally handled via the SyncBackendHost + SyncEncryptionHandler* encryption_handler(); + private: scoped_ptr<TestDirectorySetterUpper> dir_maker_; scoped_ptr<UserShare> user_share_; diff --git a/sync/internal_api/sync_encryption_handler_impl.cc b/sync/internal_api/sync_encryption_handler_impl.cc index 6ebe4a9..b9f5ed1 100644 --- a/sync/internal_api/sync_encryption_handler_impl.cc +++ b/sync/internal_api/sync_encryption_handler_impl.cc @@ -19,6 +19,7 @@ #include "sync/internal_api/public/write_transaction.h" #include "sync/protocol/encryption.pb.h" #include "sync/protocol/nigori_specifics.pb.h" +#include "sync/protocol/sync.pb.h" #include "sync/syncable/base_transaction.h" #include "sync/syncable/directory.h" #include "sync/syncable/entry.h" @@ -36,13 +37,22 @@ namespace { static const int kNigoriOverwriteLimit = 10; } +SyncEncryptionHandlerImpl::Vault::Vault( + Encryptor* encryptor, + ModelTypeSet encrypted_types) + : cryptographer(encryptor), + encrypted_types(encrypted_types) { +} + +SyncEncryptionHandlerImpl::Vault::~Vault() { +} + SyncEncryptionHandlerImpl::SyncEncryptionHandlerImpl( UserShare* user_share, - Cryptographer* cryptographer) + Encryptor* encryptor) : weak_ptr_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)), user_share_(user_share), - cryptographer_(cryptographer), - encrypted_types_(SensitiveTypes()), + vault_unsafe_(encryptor, SensitiveTypes()), encrypt_everything_(false), explicit_passphrase_(false), nigori_overwrite_count_(0) { @@ -51,20 +61,21 @@ SyncEncryptionHandlerImpl::SyncEncryptionHandlerImpl( SyncEncryptionHandlerImpl::~SyncEncryptionHandlerImpl() {} void SyncEncryptionHandlerImpl::AddObserver(Observer* observer) { + DCHECK(thread_checker_.CalledOnValidThread()); DCHECK(!observers_.HasObserver(observer)); observers_.AddObserver(observer); } void SyncEncryptionHandlerImpl::RemoveObserver(Observer* observer) { + DCHECK(thread_checker_.CalledOnValidThread()); DCHECK(observers_.HasObserver(observer)); observers_.RemoveObserver(observer); } void SyncEncryptionHandlerImpl::Init() { + DCHECK(thread_checker_.CalledOnValidThread()); WriteTransaction trans(FROM_HERE, user_share_); WriteNode node(&trans); - Cryptographer* cryptographer = trans.GetCryptographer(); - cryptographer_ = cryptographer; if (node.InitByTagLookup(kNigoriTag) != BaseNode::INIT_OK) return; @@ -73,44 +84,31 @@ void SyncEncryptionHandlerImpl::Init() { WriteEncryptionStateToNigori(&trans); } - FOR_EACH_OBSERVER(SyncEncryptionHandler::Observer, observers_, - OnCryptographerStateChanged(cryptographer)); + // Always trigger an encrypted types and cryptographer state change event at + // init time so observers get the initial values. + FOR_EACH_OBSERVER( + Observer, observers_, + OnEncryptedTypesChanged( + UnlockVault(trans.GetWrappedTrans()).encrypted_types, + encrypt_everything_)); + FOR_EACH_OBSERVER( + SyncEncryptionHandler::Observer, + observers_, + OnCryptographerStateChanged( + &UnlockVaultMutable(trans.GetWrappedTrans())->cryptographer)); // If the cryptographer is not ready (either it has pending keys or we // failed to initialize it), we don't want to try and re-encrypt the data. // If we had encrypted types, the DataTypeManager will block, preventing // sync from happening until the the passphrase is provided. - if (cryptographer->is_ready()) + if (UnlockVault(trans.GetWrappedTrans()).cryptographer.is_ready()) ReEncryptEverything(&trans); } -// Note: this is called from within a syncable transaction, so we need to post -// tasks if we want to do any work that creates a new sync_api transaction. -void SyncEncryptionHandlerImpl::ApplyNigoriUpdate( - const sync_pb::NigoriSpecifics& nigori, - syncable::BaseTransaction* const trans) { - DCHECK(trans); - if (!ApplyNigoriUpdateImpl(nigori, trans)) { - MessageLoop::current()->PostTask( - FROM_HERE, - base::Bind(&SyncEncryptionHandlerImpl::RewriteNigori, - weak_ptr_factory_.GetWeakPtr())); - } - - FOR_EACH_OBSERVER(SyncEncryptionHandler::Observer, observers_, - OnCryptographerStateChanged(cryptographer_)); -} - -// Note: this is always called via the Cryptographer interface right now, -// so a transaction is already held. Once we remove that interface, we'll -// need to enforce holding a transaction when calling this method. -ModelTypeSet SyncEncryptionHandlerImpl::GetEncryptedTypes() const { - return encrypted_types_; -} - void SyncEncryptionHandlerImpl::SetEncryptionPassphrase( const std::string& passphrase, bool is_explicit) { + DCHECK(thread_checker_.CalledOnValidThread()); // We do not accept empty passphrases. if (passphrase.empty()) { NOTREACHED() << "Cannot encrypt with an empty passphrase."; @@ -119,7 +117,6 @@ void SyncEncryptionHandlerImpl::SetEncryptionPassphrase( // All accesses to the cryptographer are protected by a transaction. WriteTransaction trans(FROM_HERE, user_share_); - Cryptographer* cryptographer = trans.GetCryptographer(); KeyParams key_params = {"localhost", "dummy", passphrase}; WriteNode node(&trans); if (node.InitByTagLookup(kNigoriTag) != BaseNode::INIT_OK) { @@ -131,11 +128,12 @@ void SyncEncryptionHandlerImpl::SetEncryptionPassphrase( node.GetNigoriSpecifics().using_explicit_passphrase(); std::string bootstrap_token; sync_pb::EncryptedData pending_keys; + Cryptographer* cryptographer = + &UnlockVaultMutable(trans.GetWrappedTrans())->cryptographer; if (cryptographer->has_pending_keys()) pending_keys = cryptographer->GetPendingKeys(); bool success = false; - // There are six cases to handle here: // 1. The user has no pending keys and is setting their current GAIA password // as the encryption passphrase. This happens either during first time sync @@ -224,6 +222,7 @@ void SyncEncryptionHandlerImpl::SetEncryptionPassphrase( void SyncEncryptionHandlerImpl::SetDecryptionPassphrase( const std::string& passphrase) { + DCHECK(thread_checker_.CalledOnValidThread()); // We do not accept empty passphrases. if (passphrase.empty()) { NOTREACHED() << "Cannot decrypt with an empty passphrase."; @@ -232,7 +231,6 @@ void SyncEncryptionHandlerImpl::SetDecryptionPassphrase( // All accesses to the cryptographer are protected by a transaction. WriteTransaction trans(FROM_HERE, user_share_); - Cryptographer* cryptographer = trans.GetCryptographer(); KeyParams key_params = {"localhost", "dummy", passphrase}; WriteNode node(&trans); if (node.InitByTagLookup(kNigoriTag) != BaseNode::INIT_OK) { @@ -240,6 +238,8 @@ void SyncEncryptionHandlerImpl::SetDecryptionPassphrase( return; } + Cryptographer* cryptographer = + &UnlockVaultMutable(trans.GetWrappedTrans())->cryptographer; if (!cryptographer->has_pending_keys()) { // Note that this *can* happen in a rare situation where data is // re-encrypted on another client while a SetDecryptionPassphrase() call is @@ -364,43 +364,94 @@ void SyncEncryptionHandlerImpl::SetDecryptionPassphrase( } void SyncEncryptionHandlerImpl::EnableEncryptEverything() { + DCHECK(thread_checker_.CalledOnValidThread()); + WriteTransaction trans(FROM_HERE, user_share_); + ModelTypeSet* encrypted_types = + &UnlockVaultMutable(trans.GetWrappedTrans())->encrypted_types; if (encrypt_everything_) { - DCHECK(encrypted_types_.Equals(ModelTypeSet::All())); + DCHECK(encrypted_types->Equals(ModelTypeSet::All())); return; } - WriteTransaction trans(FROM_HERE, user_share_); + DVLOG(1) << "Enabling encrypt everything."; encrypt_everything_ = true; // Change |encrypted_types_| directly to avoid sending more than one // notification. - encrypted_types_ = ModelTypeSet::All(); + *encrypted_types = ModelTypeSet::All(); FOR_EACH_OBSERVER( Observer, observers_, - OnEncryptedTypesChanged(encrypted_types_, encrypt_everything_)); + OnEncryptedTypesChanged(*encrypted_types, encrypt_everything_)); WriteEncryptionStateToNigori(&trans); - ReEncryptEverything(&trans); + if (UnlockVault(trans.GetWrappedTrans()).cryptographer.is_ready()) + ReEncryptEverything(&trans); } bool SyncEncryptionHandlerImpl::EncryptEverythingEnabled() const { - ReadTransaction trans(FROM_HERE, user_share_); + DCHECK(thread_checker_.CalledOnValidThread()); return encrypt_everything_; } bool SyncEncryptionHandlerImpl::IsUsingExplicitPassphrase() const { + // TODO(zea): this is called from the UI thread, so we have to have a + // transaction while accessing it. Add an OnPassphraseTypeChanged observer + // and have the SBH cache the value on the UI thread. ReadTransaction trans(FROM_HERE, user_share_); return explicit_passphrase_; } +// Note: this is called from within a syncable transaction, so we need to post +// tasks if we want to do any work that creates a new sync_api transaction. +void SyncEncryptionHandlerImpl::ApplyNigoriUpdate( + const sync_pb::NigoriSpecifics& nigori, + syncable::BaseTransaction* const trans) { + DCHECK(thread_checker_.CalledOnValidThread()); + DCHECK(trans); + if (!ApplyNigoriUpdateImpl(nigori, trans)) { + MessageLoop::current()->PostTask( + FROM_HERE, + base::Bind(&SyncEncryptionHandlerImpl::RewriteNigori, + weak_ptr_factory_.GetWeakPtr())); + } + + FOR_EACH_OBSERVER( + SyncEncryptionHandler::Observer, + observers_, + OnCryptographerStateChanged( + &UnlockVaultMutable(trans)->cryptographer)); +} + +void SyncEncryptionHandlerImpl::UpdateNigoriFromEncryptedTypes( + sync_pb::NigoriSpecifics* nigori, + syncable::BaseTransaction* const trans) const { + syncable::UpdateNigoriFromEncryptedTypes(UnlockVault(trans).encrypted_types, + encrypt_everything_, + nigori); +} + +ModelTypeSet SyncEncryptionHandlerImpl::GetEncryptedTypes( + syncable::BaseTransaction* const trans) const { + return UnlockVault(trans).encrypted_types; +} + +Cryptographer* SyncEncryptionHandlerImpl::GetCryptographerUnsafe() { + DCHECK(thread_checker_.CalledOnValidThread()); + return &vault_unsafe_.cryptographer; +} + +ModelTypeSet SyncEncryptionHandlerImpl::GetEncryptedTypesUnsafe() { + DCHECK(thread_checker_.CalledOnValidThread()); + return vault_unsafe_.encrypted_types; +} + // This function iterates over all encrypted types. There are many scenarios in // which data for some or all types is not currently available. In that case, // the lookup of the root node will fail and we will skip encryption for that // type. void SyncEncryptionHandlerImpl::ReEncryptEverything( WriteTransaction* trans) { - Cryptographer* cryptographer = trans->GetCryptographer(); - if (!cryptographer->is_ready()) - return; - ModelTypeSet encrypted_types = GetEncryptedTypes(); - for (ModelTypeSet::Iterator iter = encrypted_types.First(); + DCHECK(thread_checker_.CalledOnValidThread()); + DCHECK(UnlockVault(trans->GetWrappedTrans()).cryptographer.is_ready()); + for (ModelTypeSet::Iterator iter = + UnlockVault(trans->GetWrappedTrans()).encrypted_types.First(); iter.Good(); iter.Inc()) { if (iter.Get() == PASSWORDS || iter.Get() == NIGORI) continue; // These types handle encryption differently. @@ -455,6 +506,8 @@ void SyncEncryptionHandlerImpl::ReEncryptEverything( } } + DVLOG(1) << "Re-encrypt everything complete."; + // NOTE: We notify from within a transaction. FOR_EACH_OBSERVER(SyncEncryptionHandler::Observer, observers_, OnEncryptionComplete()); @@ -463,11 +516,13 @@ void SyncEncryptionHandlerImpl::ReEncryptEverything( bool SyncEncryptionHandlerImpl::ApplyNigoriUpdateImpl( const sync_pb::NigoriSpecifics& nigori, syncable::BaseTransaction* const trans) { - Cryptographer* cryptographer = trans->directory()->GetCryptographer(trans); - bool nigori_types_need_update = !UpdateEncryptedTypesFromNigori(nigori); + DCHECK(thread_checker_.CalledOnValidThread()); + bool nigori_types_need_update = !UpdateEncryptedTypesFromNigori(nigori, + trans); if (nigori.using_explicit_passphrase()) explicit_passphrase_ = true; + Cryptographer* cryptographer = &UnlockVaultMutable(trans)->cryptographer; bool nigori_needs_new_keys = false; if (!nigori.encrypted().blob().empty()) { if (cryptographer->CanDecrypt(nigori.encrypted())) { @@ -522,24 +577,28 @@ bool SyncEncryptionHandlerImpl::ApplyNigoriUpdateImpl( } void SyncEncryptionHandlerImpl::RewriteNigori() { + DVLOG(1) << "Overwriting stale nigori node."; + DCHECK(thread_checker_.CalledOnValidThread()); WriteTransaction trans(FROM_HERE, user_share_); WriteEncryptionStateToNigori(&trans); } void SyncEncryptionHandlerImpl::WriteEncryptionStateToNigori( WriteTransaction* trans) { + DCHECK(thread_checker_.CalledOnValidThread()); WriteNode nigori_node(trans); // This can happen in tests that don't have nigori nodes. - if (!nigori_node.InitByTagLookup(kNigoriTag) == BaseNode::INIT_OK) + if (nigori_node.InitByTagLookup(kNigoriTag) != BaseNode::INIT_OK) return; sync_pb::NigoriSpecifics nigori = nigori_node.GetNigoriSpecifics(); - Cryptographer* cryptographer = trans->GetCryptographer(); - if (cryptographer->is_ready() && + const Cryptographer& cryptographer = + UnlockVault(trans->GetWrappedTrans()).cryptographer; + if (cryptographer.is_ready() && nigori_overwrite_count_ < kNigoriOverwriteLimit) { // Does not modify the encrypted blob if the unencrypted data already // matches what is about to be written. sync_pb::EncryptedData original_keys = nigori.encrypted(); - if (!cryptographer->GetKeys(nigori.mutable_encrypted())) + if (!cryptographer.GetKeys(nigori.mutable_encrypted())) NOTREACHED(); if (nigori.encrypted().SerializeAsString() != @@ -556,58 +615,55 @@ void SyncEncryptionHandlerImpl::WriteEncryptionStateToNigori( // is lost the user can always set it again. The main point is to preserve // the encryption keys so all data remains decryptable. } - syncable::UpdateNigoriFromEncryptedTypes(encrypted_types_, - encrypt_everything_, - &nigori); + syncable::UpdateNigoriFromEncryptedTypes( + UnlockVault(trans->GetWrappedTrans()).encrypted_types, + encrypt_everything_, + &nigori); // If nothing has changed, this is a no-op. nigori_node.SetNigoriSpecifics(nigori); } bool SyncEncryptionHandlerImpl::UpdateEncryptedTypesFromNigori( - const sync_pb::NigoriSpecifics& nigori) { + const sync_pb::NigoriSpecifics& nigori, + syncable::BaseTransaction* const trans) { + DCHECK(thread_checker_.CalledOnValidThread()); + ModelTypeSet* encrypted_types = &UnlockVaultMutable(trans)->encrypted_types; if (nigori.encrypt_everything()) { if (!encrypt_everything_) { encrypt_everything_ = true; - encrypted_types_ = ModelTypeSet::All(); + *encrypted_types = ModelTypeSet::All(); + DVLOG(1) << "Enabling encrypt everything via nigori node update"; FOR_EACH_OBSERVER( Observer, observers_, - OnEncryptedTypesChanged(encrypted_types_, encrypt_everything_)); + OnEncryptedTypesChanged(*encrypted_types, encrypt_everything_)); } - DCHECK(encrypted_types_.Equals(ModelTypeSet::All())); + DCHECK(encrypted_types->Equals(ModelTypeSet::All())); return true; } - ModelTypeSet encrypted_types; - encrypted_types = syncable::GetEncryptedTypesFromNigori(nigori); - encrypted_types.PutAll(SensitiveTypes()); + ModelTypeSet nigori_encrypted_types; + nigori_encrypted_types = syncable::GetEncryptedTypesFromNigori(nigori); + nigori_encrypted_types.PutAll(SensitiveTypes()); // If anything more than the sensitive types were encrypted, and // encrypt_everything is not explicitly set to false, we assume it means // a client intended to enable encrypt everything. if (!nigori.has_encrypt_everything() && - !Difference(encrypted_types, SensitiveTypes()).Empty()) { + !Difference(nigori_encrypted_types, SensitiveTypes()).Empty()) { if (!encrypt_everything_) { encrypt_everything_ = true; - encrypted_types_ = ModelTypeSet::All(); + *encrypted_types = ModelTypeSet::All(); FOR_EACH_OBSERVER( Observer, observers_, - OnEncryptedTypesChanged(encrypted_types_, encrypt_everything_)); + OnEncryptedTypesChanged(*encrypted_types, encrypt_everything_)); } - DCHECK(encrypted_types_.Equals(ModelTypeSet::All())); + DCHECK(encrypted_types->Equals(ModelTypeSet::All())); return false; } - MergeEncryptedTypes(encrypted_types); - return encrypted_types_.Equals(encrypted_types); -} - -void SyncEncryptionHandlerImpl::UpdateNigoriFromEncryptedTypes( - sync_pb::NigoriSpecifics* nigori, - syncable::BaseTransaction* const trans) const { - syncable::UpdateNigoriFromEncryptedTypes(encrypted_types_, - encrypt_everything_, - nigori); + MergeEncryptedTypes(nigori_encrypted_types, trans); + return encrypted_types->Equals(nigori_encrypted_types); } void SyncEncryptionHandlerImpl::FinishSetPassphrase( @@ -616,9 +672,12 @@ void SyncEncryptionHandlerImpl::FinishSetPassphrase( bool is_explicit, WriteTransaction* trans, WriteNode* nigori_node) { - Cryptographer* cryptographer = trans->GetCryptographer(); - FOR_EACH_OBSERVER(SyncEncryptionHandler::Observer, observers_, - OnCryptographerStateChanged(cryptographer)); + DCHECK(thread_checker_.CalledOnValidThread()); + FOR_EACH_OBSERVER( + SyncEncryptionHandler::Observer, + observers_, + OnCryptographerStateChanged( + &UnlockVaultMutable(trans->GetWrappedTrans())->cryptographer)); // It's possible we need to change the bootstrap token even if we failed to // set the passphrase (for example if we need to preserve the new GAIA @@ -629,14 +688,16 @@ void SyncEncryptionHandlerImpl::FinishSetPassphrase( OnBootstrapTokenUpdated(bootstrap_token)); } + const Cryptographer& cryptographer = + UnlockVault(trans->GetWrappedTrans()).cryptographer; if (!success) { - if (cryptographer->is_ready()) { + if (cryptographer.is_ready()) { LOG(ERROR) << "Attempt to change passphrase failed while cryptographer " << "was ready."; - } else if (cryptographer->has_pending_keys()) { + } else if (cryptographer.has_pending_keys()) { FOR_EACH_OBSERVER(SyncEncryptionHandler::Observer, observers_, OnPassphraseRequired(REASON_DECRYPTION, - cryptographer->GetPendingKeys())); + cryptographer.GetPendingKeys())); } else { FOR_EACH_OBSERVER(SyncEncryptionHandler::Observer, observers_, OnPassphraseRequired(REASON_ENCRYPTION, @@ -647,32 +708,44 @@ void SyncEncryptionHandlerImpl::FinishSetPassphrase( FOR_EACH_OBSERVER(SyncEncryptionHandler::Observer, observers_, OnPassphraseAccepted()); - DCHECK(cryptographer->is_ready()); + DCHECK(cryptographer.is_ready()); sync_pb::NigoriSpecifics specifics(nigori_node->GetNigoriSpecifics()); // Does not modify specifics.encrypted() if the original decrypted data was // the same. - if (!cryptographer->GetKeys(specifics.mutable_encrypted())) { + if (!cryptographer.GetKeys(specifics.mutable_encrypted())) NOTREACHED(); - return; - } explicit_passphrase_ = is_explicit; specifics.set_using_explicit_passphrase(is_explicit); nigori_node->SetNigoriSpecifics(specifics); - // Does nothing if everything is already encrypted or the cryptographer has - // pending keys. + // Does nothing if everything is already encrypted. ReEncryptEverything(trans); } void SyncEncryptionHandlerImpl::MergeEncryptedTypes( - ModelTypeSet encrypted_types) { - if (!encrypted_types_.HasAll(encrypted_types)) { - encrypted_types_ = encrypted_types; + ModelTypeSet new_encrypted_types, + syncable::BaseTransaction* const trans) { + DCHECK(thread_checker_.CalledOnValidThread()); + ModelTypeSet* encrypted_types = &UnlockVaultMutable(trans)->encrypted_types; + if (!encrypted_types->HasAll(new_encrypted_types)) { + *encrypted_types = new_encrypted_types; FOR_EACH_OBSERVER( Observer, observers_, - OnEncryptedTypesChanged(encrypted_types_, encrypt_everything_)); + OnEncryptedTypesChanged(*encrypted_types, encrypt_everything_)); } } +SyncEncryptionHandlerImpl::Vault* SyncEncryptionHandlerImpl::UnlockVaultMutable( + syncable::BaseTransaction* const trans) { + DCHECK_EQ(user_share_->directory.get(), trans->directory()); + return &vault_unsafe_; +} + +const SyncEncryptionHandlerImpl::Vault& SyncEncryptionHandlerImpl::UnlockVault( + syncable::BaseTransaction* const trans) const { + DCHECK_EQ(user_share_->directory.get(), trans->directory()); + return vault_unsafe_; +} + } // namespace browser_sync diff --git a/sync/internal_api/sync_encryption_handler_impl.h b/sync/internal_api/sync_encryption_handler_impl.h index 6c605a8..8966da7 100644 --- a/sync/internal_api/sync_encryption_handler_impl.h +++ b/sync/internal_api/sync_encryption_handler_impl.h @@ -9,14 +9,17 @@ #include "base/compiler_specific.h" #include "base/gtest_prod_util.h" +#include "base/threading/thread_checker.h" #include "base/memory/scoped_ptr.h" #include "base/memory/weak_ptr.h" #include "base/observer_list.h" #include "sync/internal_api/public/sync_encryption_handler.h" #include "sync/syncable/nigori_handler.h" +#include "sync/util/cryptographer.h" namespace syncer { +class Encryptor; struct UserShare; class WriteNode; class WriteTransaction; @@ -35,16 +38,14 @@ class WriteTransaction; // Note: See sync_encryption_handler.h for a description of the chrome visible // methods and what they do, and nigori_handler.h for a description of the // sync methods. -// -// TODO(zea): Make this class explicitly non-thread safe and ensure its only -// accessed from the sync thread, with the possible exception of -// GetEncryptedTypes. Need to cache explicit passphrase state on the UI thread. +// All methods are non-thread-safe and should only be called from the sync +// thread unless explicitly noted otherwise. class SyncEncryptionHandlerImpl : public SyncEncryptionHandler, public syncable::NigoriHandler { public: SyncEncryptionHandlerImpl(UserShare* user_share, - Cryptographer* cryptographer); + Encryptor* encryptor); virtual ~SyncEncryptionHandlerImpl(); // SyncEncryptionHandler implementation. @@ -56,6 +57,8 @@ class SyncEncryptionHandlerImpl virtual void SetDecryptionPassphrase(const std::string& passphrase) OVERRIDE; virtual void EnableEncryptEverything() OVERRIDE; virtual bool EncryptEverythingEnabled() const OVERRIDE; + // Can be called from any thread. + // TODO(zea): enforce this is only called on sync thread. virtual bool IsUsingExplicitPassphrase() const OVERRIDE; // NigoriHandler implementation. @@ -66,7 +69,14 @@ class SyncEncryptionHandlerImpl virtual void UpdateNigoriFromEncryptedTypes( sync_pb::NigoriSpecifics* nigori, syncable::BaseTransaction* const trans) const OVERRIDE; - virtual ModelTypeSet GetEncryptedTypes() const OVERRIDE; + // Can be called from any thread. + virtual ModelTypeSet GetEncryptedTypes( + syncable::BaseTransaction* const trans) const OVERRIDE; + + // Unsafe getters. Use only if sync is not up and running and there is no risk + // of other threads calling this. + Cryptographer* GetCryptographerUnsafe(); + ModelTypeSet GetEncryptedTypesUnsafe(); private: FRIEND_TEST_ALL_PREFIXES(SyncEncryptionHandlerImplTest, @@ -78,6 +88,23 @@ class SyncEncryptionHandlerImpl FRIEND_TEST_ALL_PREFIXES(SyncEncryptionHandlerImplTest, UnknownSensitiveTypes); + // Container for members that require thread safety protection. All members + // that can be accessed from more than one thread should be held here and + // accessed via UnlockVault(..) and UnlockVaultMutable(..), which enforce + // that a transaction is held. + struct Vault { + Vault(Encryptor* encryptor, ModelTypeSet encrypted_types); + ~Vault(); + + // Sync's cryptographer. Used for encrypting and decrypting sync data. + Cryptographer cryptographer; + // The set of types that require encryption. + ModelTypeSet encrypted_types; + + private: + DISALLOW_COPY_AND_ASSIGN(Vault); + }; + // Iterate over all encrypted types ensuring each entry is properly encrypted. void ReEncryptEverything(WriteTransaction* trans); @@ -103,7 +130,9 @@ class SyncEncryptionHandlerImpl // had stricter encryption than |nigori|, and the nigori node needs to be // updated with the newer encryption state. // Note: must be called from within a transaction. - bool UpdateEncryptedTypesFromNigori(const sync_pb::NigoriSpecifics& nigori); + bool UpdateEncryptedTypesFromNigori( + const sync_pb::NigoriSpecifics& nigori, + syncable::BaseTransaction* const trans); // The final step of SetEncryptionPassphrase and SetDecryptionPassphrase that // notifies observers of the result of the set passphrase operation, updates @@ -125,7 +154,15 @@ class SyncEncryptionHandlerImpl // Merges the given set of encrypted types with the existing set and emits a // notification if necessary. // Note: must be called from within a transaction. - void MergeEncryptedTypes(ModelTypeSet encrypted_types); + void MergeEncryptedTypes(ModelTypeSet new_encrypted_types, + syncable::BaseTransaction* const trans); + + // Helper methods for ensuring transactions are held when accessing + // |vault_unsafe_|. + Vault* UnlockVaultMutable(syncable::BaseTransaction* const trans); + const Vault& UnlockVault(syncable::BaseTransaction* const trans) const; + + base::ThreadChecker thread_checker_; base::WeakPtrFactory<SyncEncryptionHandlerImpl> weak_ptr_factory_; @@ -134,18 +171,16 @@ class SyncEncryptionHandlerImpl // The current user share (for creating transactions). UserShare* user_share_; - // TODO(zea): have the sync encryption handler own the cryptographer, and live - // in the directory. - Cryptographer* cryptographer_; - - // The set of types that require encryption. This is accessed on all sync - // datatype threads when we write to a node, so we must hold a transaction - // whenever we touch/read it. - ModelTypeSet encrypted_types_; + // Container for all data that can be accessed from multiple threads. Do not + // access this object directly. Instead access it via UnlockVault(..) and + // UnlockVaultMutable(..). + Vault vault_unsafe_; - // Sync encryption state. These are only modified and accessed from the sync + // Sync encryption state that is only modified and accessed from the sync // thread. + // Whether all current and future types should be encrypted. bool encrypt_everything_; + // Whether the user is using a custom passphrase for encryption. bool explicit_passphrase_; // The number of times we've automatically (i.e. not via SetPassphrase or diff --git a/sync/internal_api/sync_encryption_handler_impl_unittest.cc b/sync/internal_api/sync_encryption_handler_impl_unittest.cc index af58d54..b37afa4 100644 --- a/sync/internal_api/sync_encryption_handler_impl_unittest.cc +++ b/sync/internal_api/sync_encryption_handler_impl_unittest.cc @@ -20,6 +20,7 @@ #include "sync/syncable/mutable_entry.h" #include "sync/syncable/write_transaction.h" #include "sync/test/engine/test_id_factory.h" +#include "sync/test/fake_encryptor.h" #include "sync/util/cryptographer.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -50,7 +51,7 @@ class SyncEncryptionHandlerObserverMock class SyncEncryptionHandlerImplTest : public ::testing::Test { public: - SyncEncryptionHandlerImplTest() : cryptographer_(NULL) {} + SyncEncryptionHandlerImplTest() {} virtual ~SyncEncryptionHandlerImplTest() {} virtual void SetUp() { @@ -66,12 +67,9 @@ class SyncEncryptionHandlerImplTest : public ::testing::Test { protected: void SetUpEncryption() { ReadTransaction trans(FROM_HERE, user_share()); - cryptographer_ = trans.GetCryptographer(); encryption_handler_.reset( new SyncEncryptionHandlerImpl(user_share(), - cryptographer_)); - cryptographer_->SetNigoriHandler( - encryption_handler_.get()); + &encryptor_)); encryption_handler_->AddObserver(&observer_); } @@ -109,13 +107,15 @@ class SyncEncryptionHandlerImplTest : public ::testing::Test { return encryption_handler_.get(); } SyncEncryptionHandlerObserverMock* observer() { return &observer_; } - Cryptographer* cryptographer() { return cryptographer_; } + Cryptographer* GetCryptographer() { + return encryption_handler_->GetCryptographerUnsafe(); + } - private: + protected: TestUserShare test_user_share_; + FakeEncryptor encryptor_; scoped_ptr<SyncEncryptionHandlerImpl> encryption_handler_; StrictMock<SyncEncryptionHandlerObserverMock> observer_; - Cryptographer* cryptographer_; TestIdFactory ids_; MessageLoop message_loop_; }; @@ -127,23 +127,25 @@ TEST_F(SyncEncryptionHandlerImplTest, NigoriEncryptionTypes) { StrictMock<SyncEncryptionHandlerObserverMock> observer2; SyncEncryptionHandlerImpl handler2(user_share(), - cryptographer()); + &encryptor_); handler2.AddObserver(&observer2); // Just set the sensitive types (shouldn't trigger any notifications). ModelTypeSet encrypted_types(SyncEncryptionHandler::SensitiveTypes()); - encryption_handler()->MergeEncryptedTypes(encrypted_types); { WriteTransaction trans(FROM_HERE, user_share()); + encryption_handler()->MergeEncryptedTypes( + encrypted_types, + trans.GetWrappedTrans()); encryption_handler()->UpdateNigoriFromEncryptedTypes( &nigori, trans.GetWrappedTrans()); + handler2.UpdateEncryptedTypesFromNigori(nigori, trans.GetWrappedTrans()); } - handler2.UpdateEncryptedTypesFromNigori(nigori); EXPECT_TRUE(encrypted_types.Equals( - encryption_handler()->GetEncryptedTypes())); + encryption_handler()->GetEncryptedTypesUnsafe())); EXPECT_TRUE(encrypted_types.Equals( - handler2.GetEncryptedTypes())); + handler2.GetEncryptedTypesUnsafe())); Mock::VerifyAndClearExpectations(observer()); Mock::VerifyAndClearExpectations(&observer2); @@ -157,41 +159,47 @@ TEST_F(SyncEncryptionHandlerImplTest, NigoriEncryptionTypes) { // Set all encrypted types encrypted_types = ModelTypeSet::All(); - encryption_handler()->MergeEncryptedTypes(encrypted_types); { WriteTransaction trans(FROM_HERE, user_share()); + encryption_handler()->MergeEncryptedTypes( + encrypted_types, + trans.GetWrappedTrans()); encryption_handler()->UpdateNigoriFromEncryptedTypes( &nigori, trans.GetWrappedTrans()); + handler2.UpdateEncryptedTypesFromNigori(nigori, trans.GetWrappedTrans()); } - handler2.UpdateEncryptedTypesFromNigori(nigori); EXPECT_TRUE(encrypted_types.Equals( - encryption_handler()->GetEncryptedTypes())); - EXPECT_TRUE(encrypted_types.Equals(handler2.GetEncryptedTypes())); + encryption_handler()->GetEncryptedTypesUnsafe())); + EXPECT_TRUE(encrypted_types.Equals(handler2.GetEncryptedTypesUnsafe())); // Receiving an empty nigori should not reset any encrypted types or trigger // an observer notification. Mock::VerifyAndClearExpectations(observer()); Mock::VerifyAndClearExpectations(&observer2); nigori = sync_pb::NigoriSpecifics(); - encryption_handler()->UpdateEncryptedTypesFromNigori(nigori); + { + WriteTransaction trans(FROM_HERE, user_share()); + handler2.UpdateEncryptedTypesFromNigori(nigori, trans.GetWrappedTrans()); + } EXPECT_TRUE(encrypted_types.Equals( - encryption_handler()->GetEncryptedTypes())); + encryption_handler()->GetEncryptedTypesUnsafe())); } // Verify the encryption handler processes the encrypt everything field // properly. TEST_F(SyncEncryptionHandlerImplTest, EncryptEverythingExplicit) { ModelTypeSet real_types = ModelTypeSet::All(); - sync_pb::NigoriSpecifics specifics; - specifics.set_encrypt_everything(true); + sync_pb::NigoriSpecifics nigori; + nigori.set_encrypt_everything(true); EXPECT_CALL(*observer(), OnEncryptedTypesChanged( HasModelTypes(ModelTypeSet::All()), true)); EXPECT_FALSE(encryption_handler()->EncryptEverythingEnabled()); - ModelTypeSet encrypted_types = encryption_handler()->GetEncryptedTypes(); + ModelTypeSet encrypted_types = + encryption_handler()->GetEncryptedTypesUnsafe(); for (ModelTypeSet::Iterator iter = real_types.First(); iter.Good(); iter.Inc()) { if (iter.Get() == PASSWORDS || iter.Get() == NIGORI) @@ -200,10 +208,15 @@ TEST_F(SyncEncryptionHandlerImplTest, EncryptEverythingExplicit) { EXPECT_FALSE(encrypted_types.Has(iter.Get())); } - encryption_handler()->UpdateEncryptedTypesFromNigori(specifics); + { + WriteTransaction trans(FROM_HERE, user_share()); + encryption_handler()->UpdateEncryptedTypesFromNigori( + nigori, + trans.GetWrappedTrans()); + } EXPECT_TRUE(encryption_handler()->EncryptEverythingEnabled()); - encrypted_types = encryption_handler()->GetEncryptedTypes(); + encrypted_types = encryption_handler()->GetEncryptedTypesUnsafe(); for (ModelTypeSet::Iterator iter = real_types.First(); iter.Good(); iter.Inc()) { EXPECT_TRUE(encrypted_types.Has(iter.Get())); @@ -211,22 +224,28 @@ TEST_F(SyncEncryptionHandlerImplTest, EncryptEverythingExplicit) { // Receiving the nigori node again shouldn't trigger another notification. Mock::VerifyAndClearExpectations(observer()); - encryption_handler()->UpdateEncryptedTypesFromNigori(specifics); + { + WriteTransaction trans(FROM_HERE, user_share()); + encryption_handler()->UpdateEncryptedTypesFromNigori( + nigori, + trans.GetWrappedTrans()); + } } // Verify the encryption handler can detect an implicit encrypt everything state // (from clients that failed to write the encrypt everything field). TEST_F(SyncEncryptionHandlerImplTest, EncryptEverythingImplicit) { ModelTypeSet real_types = ModelTypeSet::All(); - sync_pb::NigoriSpecifics specifics; - specifics.set_encrypt_bookmarks(true); // Non-passwords = encrypt everything + sync_pb::NigoriSpecifics nigori; + nigori.set_encrypt_bookmarks(true); // Non-passwords = encrypt everything EXPECT_CALL(*observer(), OnEncryptedTypesChanged( HasModelTypes(ModelTypeSet::All()), true)); EXPECT_FALSE(encryption_handler()->EncryptEverythingEnabled()); - ModelTypeSet encrypted_types = encryption_handler()->GetEncryptedTypes(); + ModelTypeSet encrypted_types = + encryption_handler()->GetEncryptedTypesUnsafe(); for (ModelTypeSet::Iterator iter = real_types.First(); iter.Good(); iter.Inc()) { if (iter.Get() == PASSWORDS || iter.Get() == NIGORI) @@ -235,10 +254,15 @@ TEST_F(SyncEncryptionHandlerImplTest, EncryptEverythingImplicit) { EXPECT_FALSE(encrypted_types.Has(iter.Get())); } - encryption_handler()->UpdateEncryptedTypesFromNigori(specifics); + { + WriteTransaction trans(FROM_HERE, user_share()); + encryption_handler()->UpdateEncryptedTypesFromNigori( + nigori, + trans.GetWrappedTrans()); + } EXPECT_TRUE(encryption_handler()->EncryptEverythingEnabled()); - encrypted_types = encryption_handler()->GetEncryptedTypes(); + encrypted_types = encryption_handler()->GetEncryptedTypesUnsafe(); for (ModelTypeSet::Iterator iter = real_types.First(); iter.Good(); iter.Inc()) { EXPECT_TRUE(encrypted_types.Has(iter.Get())); @@ -247,8 +271,13 @@ TEST_F(SyncEncryptionHandlerImplTest, EncryptEverythingImplicit) { // Receiving a nigori node with encrypt everything explicitly set shouldn't // trigger another notification. Mock::VerifyAndClearExpectations(observer()); - specifics.set_encrypt_everything(true); - encryption_handler()->UpdateEncryptedTypesFromNigori(specifics); + nigori.set_encrypt_everything(true); + { + WriteTransaction trans(FROM_HERE, user_share()); + encryption_handler()->UpdateEncryptedTypesFromNigori( + nigori, + trans.GetWrappedTrans()); + } } // Verify the encryption handler can deal with new versions treating new types @@ -256,9 +285,9 @@ TEST_F(SyncEncryptionHandlerImplTest, EncryptEverythingImplicit) { // everything case. TEST_F(SyncEncryptionHandlerImplTest, UnknownSensitiveTypes) { ModelTypeSet real_types = ModelTypeSet::All(); - sync_pb::NigoriSpecifics specifics; - specifics.set_encrypt_everything(false); - specifics.set_encrypt_bookmarks(true); + sync_pb::NigoriSpecifics nigori; + nigori.set_encrypt_everything(false); + nigori.set_encrypt_bookmarks(true); ModelTypeSet expected_encrypted_types = SyncEncryptionHandler::SensitiveTypes(); @@ -269,7 +298,8 @@ TEST_F(SyncEncryptionHandlerImplTest, UnknownSensitiveTypes) { HasModelTypes(expected_encrypted_types), false)); EXPECT_FALSE(encryption_handler()->EncryptEverythingEnabled()); - ModelTypeSet encrypted_types = encryption_handler()->GetEncryptedTypes(); + ModelTypeSet encrypted_types = + encryption_handler()->GetEncryptedTypesUnsafe(); for (ModelTypeSet::Iterator iter = real_types.First(); iter.Good(); iter.Inc()) { if (iter.Get() == PASSWORDS || iter.Get() == NIGORI) @@ -278,10 +308,15 @@ TEST_F(SyncEncryptionHandlerImplTest, UnknownSensitiveTypes) { EXPECT_FALSE(encrypted_types.Has(iter.Get())); } - encryption_handler()->UpdateEncryptedTypesFromNigori(specifics); + { + WriteTransaction trans(FROM_HERE, user_share()); + encryption_handler()->UpdateEncryptedTypesFromNigori( + nigori, + trans.GetWrappedTrans()); + } EXPECT_FALSE(encryption_handler()->EncryptEverythingEnabled()); - encrypted_types = encryption_handler()->GetEncryptedTypes(); + encrypted_types = encryption_handler()->GetEncryptedTypesUnsafe(); for (ModelTypeSet::Iterator iter = real_types.First(); iter.Good(); iter.Inc()) { if (iter.Get() == PASSWORDS || @@ -301,7 +336,7 @@ TEST_F(SyncEncryptionHandlerImplTest, ReceiveOldNigori) { KeyParams current_key = {"localhost", "dummy", "cur"}; // Data for testing encryption/decryption. - Cryptographer other_cryptographer(cryptographer()->encryptor()); + Cryptographer other_cryptographer(GetCryptographer()->encryptor()); other_cryptographer.AddKey(old_key); sync_pb::EntitySpecifics other_encrypted_specifics; other_encrypted_specifics.mutable_bookmark()->set_title("title"); @@ -315,12 +350,12 @@ TEST_F(SyncEncryptionHandlerImplTest, ReceiveOldNigori) { // Set up the current encryption state (containing both keys and encrypt // everything). sync_pb::NigoriSpecifics current_nigori_specifics; - cryptographer()->AddKey(old_key); - cryptographer()->AddKey(current_key); - cryptographer()->Encrypt( + GetCryptographer()->AddKey(old_key); + GetCryptographer()->AddKey(current_key); + GetCryptographer()->Encrypt( our_encrypted_specifics, our_encrypted_specifics.mutable_encrypted()); - cryptographer()->GetKeys( + GetCryptographer()->GetKeys( current_nigori_specifics.mutable_encrypted()); current_nigori_specifics.set_encrypt_everything(true); @@ -349,8 +384,8 @@ TEST_F(SyncEncryptionHandlerImplTest, ReceiveOldNigori) { old_nigori, trans.GetWrappedTrans()); } - EXPECT_TRUE(cryptographer()->is_ready()); - EXPECT_FALSE(cryptographer()->has_pending_keys()); + EXPECT_TRUE(GetCryptographer()->is_ready()); + EXPECT_FALSE(GetCryptographer()->has_pending_keys()); // Encryption handler should have posted a task to overwrite the old // specifics. @@ -366,13 +401,14 @@ TEST_F(SyncEncryptionHandlerImplTest, ReceiveOldNigori) { ASSERT_EQ(nigori_node.InitByTagLookup(ModelTypeToRootTag(NIGORI)), BaseNode::INIT_OK); const sync_pb::NigoriSpecifics& nigori = nigori_node.GetNigoriSpecifics(); - EXPECT_TRUE(cryptographer()->CanDecryptUsingDefaultKey( + EXPECT_TRUE(GetCryptographer()->CanDecryptUsingDefaultKey( our_encrypted_specifics.encrypted())); - EXPECT_TRUE(cryptographer()->CanDecrypt( + EXPECT_TRUE(GetCryptographer()->CanDecrypt( other_encrypted_specifics.encrypted())); - EXPECT_TRUE(cryptographer()->CanDecrypt(nigori.encrypted())); + EXPECT_TRUE(GetCryptographer()->CanDecrypt(nigori.encrypted())); EXPECT_TRUE(nigori.encrypt_everything()); - EXPECT_TRUE(cryptographer()->CanDecryptUsingDefaultKey(nigori.encrypted())); + EXPECT_TRUE( + GetCryptographer()->CanDecryptUsingDefaultKey(nigori.encrypted())); } EXPECT_TRUE(encryption_handler()->EncryptEverythingEnabled()); } diff --git a/sync/internal_api/sync_manager_impl.cc b/sync/internal_api/sync_manager_impl.cc index ccf5cea..ded97f5 100644 --- a/sync/internal_api/sync_manager_impl.cc +++ b/sync/internal_api/sync_manager_impl.cc @@ -383,6 +383,13 @@ void SyncManagerImpl::Init( unrecoverable_error_handler_ = unrecoverable_error_handler; report_unrecoverable_error_function_ = report_unrecoverable_error_function; + sync_encryption_handler_.reset(new SyncEncryptionHandlerImpl( + &share_, + encryptor)); + sync_encryption_handler_->AddObserver(this); + sync_encryption_handler_->AddObserver(&debug_info_event_listener_); + sync_encryption_handler_->AddObserver(&js_sync_encryption_handler_observer_); + FilePath absolute_db_path(database_path_); file_util::AbsolutePath(&absolute_db_path); scoped_ptr<syncable::DirectoryBackingStore> backing_store = @@ -392,10 +399,12 @@ void SyncManagerImpl::Init( DCHECK(backing_store.get()); share_.name = credentials.email; share_.directory.reset( - new syncable::Directory(encryptor_, - unrecoverable_error_handler_, - report_unrecoverable_error_function_, - backing_store.release())); + new syncable::Directory( + backing_store.release(), + unrecoverable_error_handler_, + report_unrecoverable_error_function_, + sync_encryption_handler_.get(), + sync_encryption_handler_->GetCryptographerUnsafe())); DVLOG(1) << "Username: " << username_for_share(); if (!OpenDirectory()) { @@ -465,15 +474,6 @@ void SyncManagerImpl::Init( trans.GetCryptographer()->BootstrapKeystoreKey( restored_keystore_key_for_bootstrapping); - sync_encryption_handler_.reset(new SyncEncryptionHandlerImpl( - &share_, - trans.GetCryptographer())); - sync_encryption_handler_->AddObserver(this); - sync_encryption_handler_->AddObserver(&debug_info_event_listener_); - sync_encryption_handler_->AddObserver(&js_sync_encryption_handler_observer_); - trans.GetCryptographer()->SetNigoriHandler( - sync_encryption_handler_.get()); - FOR_EACH_OBSERVER(SyncManager::Observer, observers_, OnInitializationComplete( MakeWeakHandle(weak_ptr_factory_.GetWeakPtr()), diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc index 613150c..36e3da1 100644 --- a/sync/internal_api/sync_manager_impl_unittest.cc +++ b/sync/internal_api/sync_manager_impl_unittest.cc @@ -225,35 +225,25 @@ class SyncApiTest : public testing::Test { public: virtual void SetUp() { test_user_share_.SetUp(); - SetUpEncryption(); } virtual void TearDown() { test_user_share_.TearDown(); } - void SetUpEncryption() { - ReadTransaction trans(FROM_HERE, test_user_share_.user_share()); - encryption_handler_.reset( - new SyncEncryptionHandlerImpl(test_user_share_.user_share(), - trans.GetCryptographer())); - trans.GetCryptographer()->SetNigoriHandler(encryption_handler_.get()); - } - protected: MessageLoop message_loop_; TestUserShare test_user_share_; - scoped_ptr<SyncEncryptionHandlerImpl> encryption_handler_; }; TEST_F(SyncApiTest, SanityCheckTest) { { ReadTransaction trans(FROM_HERE, test_user_share_.user_share()); - EXPECT_TRUE(trans.GetWrappedTrans() != NULL); + EXPECT_TRUE(trans.GetWrappedTrans()); } { WriteTransaction trans(FROM_HERE, test_user_share_.user_share()); - EXPECT_TRUE(trans.GetWrappedTrans() != NULL); + EXPECT_TRUE(trans.GetWrappedTrans()); } { // No entries but root should exist @@ -483,7 +473,7 @@ TEST_F(SyncApiTest, WriteEncryptedTitle) { ReadTransaction trans(FROM_HERE, test_user_share_.user_share()); trans.GetCryptographer()->AddKey(params); } - encryption_handler_->EnableEncryptEverything(); + test_user_share_.encryption_handler()->EnableEncryptEverything(); { WriteTransaction trans(FROM_HERE, test_user_share_.user_share()); ReadNode root_node(&trans); @@ -841,7 +831,7 @@ class SyncManagerTest : public testing::Test, if (nigori_status == WRITE_TO_NIGORI) { sync_pb::NigoriSpecifics nigori; cryptographer->GetKeys(nigori.mutable_encrypted()); - cryptographer->UpdateNigoriFromEncryptedTypes( + share->directory->GetNigoriHandler()->UpdateNigoriFromEncryptedTypes( &nigori, trans.GetWrappedTrans()); WriteNode node(&trans); @@ -904,9 +894,14 @@ class SyncManagerTest : public testing::Test, // Gets the set of encrypted types from the cryptographer // Note: opens a transaction. May be called from any thread. - syncer::ModelTypeSet GetEncryptedDataTypesForTest() { + ModelTypeSet GetEncryptedTypes() { ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare()); - return GetEncryptedTypes(&trans); + return GetEncryptedTypesWithTrans(&trans); + } + + ModelTypeSet GetEncryptedTypesWithTrans(BaseTransaction* trans) { + return trans->GetDirectory()->GetNigoriHandler()-> + GetEncryptedTypes(trans->GetWrappedTrans()); } void SimulateEnableNotificationsForTest() { @@ -1356,11 +1351,12 @@ TEST_F(SyncManagerTest, RefreshEncryptionReady) { EXPECT_TRUE(SetUpEncryption(WRITE_TO_NIGORI, DEFAULT_ENCRYPTION)); EXPECT_CALL(encryption_observer_, OnEncryptionComplete()); EXPECT_CALL(encryption_observer_, OnCryptographerStateChanged(_)); + EXPECT_CALL(encryption_observer_, OnEncryptedTypesChanged(_, false)); sync_manager_.GetEncryptionHandler()->Init(); PumpLoop(); - const ModelTypeSet encrypted_types = GetEncryptedDataTypesForTest(); + const ModelTypeSet encrypted_types = GetEncryptedTypes(); EXPECT_TRUE(encrypted_types.Has(PASSWORDS)); EXPECT_FALSE(EncryptEverythingEnabledForTest()); @@ -1385,10 +1381,11 @@ TEST_F(SyncManagerTest, RefreshEncryptionNotReady) { // is not ready. EXPECT_CALL(encryption_observer_, OnPassphraseRequired(_, _)).Times(1); EXPECT_CALL(encryption_observer_, OnCryptographerStateChanged(_)); + EXPECT_CALL(encryption_observer_, OnEncryptedTypesChanged(_, false)); sync_manager_.GetEncryptionHandler()->Init(); PumpLoop(); - const ModelTypeSet encrypted_types = GetEncryptedDataTypesForTest(); + const ModelTypeSet encrypted_types = GetEncryptedTypes(); EXPECT_TRUE(encrypted_types.Has(PASSWORDS)); // Hardcoded. EXPECT_FALSE(EncryptEverythingEnabledForTest()); } @@ -1398,12 +1395,13 @@ TEST_F(SyncManagerTest, RefreshEncryptionEmptyNigori) { EXPECT_TRUE(SetUpEncryption(DONT_WRITE_NIGORI, DEFAULT_ENCRYPTION)); EXPECT_CALL(encryption_observer_, OnEncryptionComplete()).Times(1); EXPECT_CALL(encryption_observer_, OnCryptographerStateChanged(_)); + EXPECT_CALL(encryption_observer_, OnEncryptedTypesChanged(_, false)); // Should write to nigori. sync_manager_.GetEncryptionHandler()->Init(); PumpLoop(); - const ModelTypeSet encrypted_types = GetEncryptedDataTypesForTest(); + const ModelTypeSet encrypted_types = GetEncryptedTypes(); EXPECT_TRUE(encrypted_types.Has(PASSWORDS)); // Hardcoded. EXPECT_FALSE(EncryptEverythingEnabledForTest()); @@ -1460,21 +1458,18 @@ TEST_F(SyncManagerTest, EncryptDataTypesWithData) { { ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare()); - EXPECT_TRUE(GetEncryptedTypes(&trans).Equals( + EXPECT_TRUE(GetEncryptedTypesWithTrans(&trans).Equals( SyncEncryptionHandler::SensitiveTypes())); EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest( trans.GetWrappedTrans(), - trans.GetCryptographer(), BOOKMARKS, false /* not encrypted */)); EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest( trans.GetWrappedTrans(), - trans.GetCryptographer(), SESSIONS, false /* not encrypted */)); EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest( trans.GetWrappedTrans(), - trans.GetCryptographer(), THEMES, false /* not encrypted */)); } @@ -1487,21 +1482,18 @@ TEST_F(SyncManagerTest, EncryptDataTypesWithData) { EXPECT_TRUE(EncryptEverythingEnabledForTest()); { ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare()); - EXPECT_TRUE(GetEncryptedTypes(&trans).Equals( + EXPECT_TRUE(GetEncryptedTypesWithTrans(&trans).Equals( ModelTypeSet::All())); EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest( trans.GetWrappedTrans(), - trans.GetCryptographer(), BOOKMARKS, true /* is encrypted */)); EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest( trans.GetWrappedTrans(), - trans.GetCryptographer(), SESSIONS, true /* is encrypted */)); EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest( trans.GetWrappedTrans(), - trans.GetCryptographer(), THEMES, true /* is encrypted */)); } @@ -1517,20 +1509,17 @@ TEST_F(SyncManagerTest, EncryptDataTypesWithData) { EXPECT_TRUE(EncryptEverythingEnabledForTest()); { ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare()); - EXPECT_TRUE(GetEncryptedTypes(&trans).Equals(ModelTypeSet::All())); + EXPECT_TRUE(GetEncryptedTypesWithTrans(&trans).Equals(ModelTypeSet::All())); EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest( trans.GetWrappedTrans(), - trans.GetCryptographer(), BOOKMARKS, true /* is encrypted */)); EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest( trans.GetWrappedTrans(), - trans.GetCryptographer(), SESSIONS, true /* is encrypted */)); EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest( trans.GetWrappedTrans(), - trans.GetCryptographer(), THEMES, true /* is encrypted */)); } @@ -1991,7 +1980,6 @@ TEST_F(SyncManagerTest, EncryptBookmarksWithLegacyData) { ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare()); EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest( trans.GetWrappedTrans(), - trans.GetCryptographer(), BOOKMARKS, false /* not encrypted */)); } @@ -2005,10 +1993,9 @@ TEST_F(SyncManagerTest, EncryptBookmarksWithLegacyData) { { ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare()); - EXPECT_TRUE(GetEncryptedTypes(&trans).Equals(ModelTypeSet::All())); + EXPECT_TRUE(GetEncryptedTypesWithTrans(&trans).Equals(ModelTypeSet::All())); EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest( trans.GetWrappedTrans(), - trans.GetCryptographer(), BOOKMARKS, true /* is encrypted */)); @@ -2091,6 +2078,7 @@ TEST_F(SyncManagerTest, UpdateEntryWithEncryption) { EXPECT_TRUE(SetUpEncryption(WRITE_TO_NIGORI, FULL_ENCRYPTION)); EXPECT_CALL(encryption_observer_, OnCryptographerStateChanged(_)); + EXPECT_CALL(encryption_observer_, OnEncryptedTypesChanged(_, true)); sync_manager_.GetEncryptionHandler()->Init(); PumpLoop(); { @@ -2138,6 +2126,7 @@ TEST_F(SyncManagerTest, UpdateEntryWithEncryption) { testing::Mock::VerifyAndClearExpectations(&encryption_observer_); EXPECT_CALL(encryption_observer_, OnEncryptionComplete()); EXPECT_CALL(encryption_observer_, OnCryptographerStateChanged(_)); + EXPECT_CALL(encryption_observer_, OnEncryptedTypesChanged(_, true)); sync_manager_.GetEncryptionHandler()->Init(); PumpLoop(); @@ -2344,6 +2333,7 @@ TEST_F(SyncManagerTest, UpdatePasswordReencryptEverything) { testing::Mock::VerifyAndClearExpectations(&encryption_observer_); EXPECT_CALL(encryption_observer_, OnEncryptionComplete()); EXPECT_CALL(encryption_observer_, OnCryptographerStateChanged(_)); + EXPECT_CALL(encryption_observer_, OnEncryptedTypesChanged(_, false)); sync_manager_.GetEncryptionHandler()->Init(); PumpLoop(); EXPECT_FALSE(ResetUnsyncedEntry(PASSWORDS, client_tag)); @@ -2406,6 +2396,7 @@ TEST_F(SyncManagerTest, SetBookmarkTitleWithEncryption) { EXPECT_CALL(encryption_observer_, OnEncryptionComplete()); EXPECT_TRUE(SetUpEncryption(WRITE_TO_NIGORI, FULL_ENCRYPTION)); EXPECT_CALL(encryption_observer_, OnCryptographerStateChanged(_)); + EXPECT_CALL(encryption_observer_, OnEncryptedTypesChanged(_, true)); sync_manager_.GetEncryptionHandler()->Init(); PumpLoop(); EXPECT_TRUE(ResetUnsyncedEntry(BOOKMARKS, client_tag)); @@ -2502,6 +2493,7 @@ TEST_F(SyncManagerTest, SetNonBookmarkTitleWithEncryption) { EXPECT_CALL(encryption_observer_, OnEncryptionComplete()); EXPECT_TRUE(SetUpEncryption(WRITE_TO_NIGORI, FULL_ENCRYPTION)); EXPECT_CALL(encryption_observer_, OnCryptographerStateChanged(_)); + EXPECT_CALL(encryption_observer_, OnEncryptedTypesChanged(_, true)); sync_manager_.GetEncryptionHandler()->Init(); PumpLoop(); EXPECT_TRUE(ResetUnsyncedEntry(PREFERENCES, client_tag)); diff --git a/sync/internal_api/test/test_user_share.cc b/sync/internal_api/test/test_user_share.cc index 21433b1..ef93b18 100644 --- a/sync/internal_api/test/test_user_share.cc +++ b/sync/internal_api/test/test_user_share.cc @@ -39,4 +39,8 @@ UserShare* TestUserShare::user_share() { return user_share_.get(); } +SyncEncryptionHandler* TestUserShare::encryption_handler() { + return dir_maker_->encryption_handler(); +} + } // namespace syncer diff --git a/sync/internal_api/write_node.cc b/sync/internal_api/write_node.cc index 22d7e5f..c9d4755 100644 --- a/sync/internal_api/write_node.cc +++ b/sync/internal_api/write_node.cc @@ -42,10 +42,9 @@ void WriteNode::SetIsFolder(bool folder) { void WriteNode::SetTitle(const std::wstring& title) { DCHECK_NE(GetModelType(), UNSPECIFIED); ModelType type = GetModelType(); - Cryptographer* cryptographer = GetTransaction()->GetCryptographer(); // It's possible the nigori lost the set of encrypted types. If the current // specifics are already encrypted, we want to ensure we continue encrypting. - bool needs_encryption = cryptographer->GetEncryptedTypes().Has(type) || + bool needs_encryption = GetTransaction()->GetEncryptedTypes().Has(type) || entry_->Get(SPECIFICS).has_encrypted(); // If this datatype is encrypted and is not a bookmark, we disregard the @@ -203,7 +202,6 @@ void WriteNode::SetEntitySpecifics( if (GetModelType() != UNSPECIFIED) { DCHECK_EQ(new_specifics_type, GetModelType()); } - Cryptographer* cryptographer = GetTransaction()->GetCryptographer(); // Preserve unknown fields. const sync_pb::EntitySpecifics& old_specifics = entry_->Get(SPECIFICS); @@ -213,7 +211,9 @@ void WriteNode::SetEntitySpecifics( old_specifics.unknown_fields()); // Will update the entry if encryption was necessary. - if (!UpdateEntryWithEncryption(cryptographer, new_specifics, entry_)) { + if (!UpdateEntryWithEncryption(GetTransaction()->GetWrappedTrans(), + new_specifics, + entry_)) { return; } if (entry_->Get(SPECIFICS).has_encrypted()) { diff --git a/sync/syncable/directory.cc b/sync/syncable/directory.cc index 92ec65e..77979ae 100644 --- a/sync/syncable/directory.cc +++ b/sync/syncable/directory.cc @@ -136,17 +136,19 @@ Directory::Kernel::~Kernel() { } Directory::Directory( - Encryptor* encryptor, + DirectoryBackingStore* store, UnrecoverableErrorHandler* unrecoverable_error_handler, ReportUnrecoverableErrorFunction report_unrecoverable_error_function, - DirectoryBackingStore* store) - : cryptographer_(encryptor), - kernel_(NULL), + NigoriHandler* nigori_handler, + Cryptographer* cryptographer) + : kernel_(NULL), store_(store), unrecoverable_error_handler_(unrecoverable_error_handler), report_unrecoverable_error_function_( report_unrecoverable_error_function), unrecoverable_error_set_(false), + nigori_handler_(nigori_handler), + cryptographer_(cryptographer), invariant_check_level_(VERIFY_CHANGES) { } @@ -724,9 +726,13 @@ string Directory::cache_guid() const { return kernel_->cache_guid; } +NigoriHandler* Directory::GetNigoriHandler() { + return nigori_handler_; +} + Cryptographer* Directory::GetCryptographer(const BaseTransaction* trans) { DCHECK_EQ(this, trans->directory()); - return &cryptographer_; + return cryptographer_; } void Directory::GetAllMetaHandles(BaseTransaction* trans, diff --git a/sync/syncable/directory.h b/sync/syncable/directory.h index 5cf6d55..b34882f 100644 --- a/sync/syncable/directory.h +++ b/sync/syncable/directory.h @@ -17,21 +17,21 @@ #include "sync/syncable/entry_kernel.h" #include "sync/syncable/metahandle_set.h" #include "sync/syncable/scoped_kernel_lock.h" -#include "sync/util/cryptographer.h" namespace syncer { -class Encryptor; +class Cryptographer; class UnrecoverableErrorHandler; namespace syncable { +class BaseTransaction; class DirectoryChangeDelegate; +class DirectoryBackingStore; +class NigoriHandler; +class ScopedKernelLock; class TransactionObserver; -class BaseTransaction; class WriteTransaction; -class ScopedKernelLock; -class DirectoryBackingStore; // How syncable indices & Indexers work. // @@ -207,11 +207,12 @@ class Directory { // |report_unrecoverable_error_function| may be NULL. // Takes ownership of |store|. Directory( - Encryptor* encryptor, + DirectoryBackingStore* store, UnrecoverableErrorHandler* unrecoverable_error_handler, ReportUnrecoverableErrorFunction report_unrecoverable_error_function, - DirectoryBackingStore* store); + NigoriHandler* nigori_handler, + Cryptographer* cryptographer); virtual ~Directory(); // Does not take ownership of |delegate|, which must not be NULL. @@ -265,9 +266,11 @@ class Directory { // Unique to each account / client pair. std::string cache_guid() const; - // Returns a pointer to our cryptographer. Does not transfer ownership. The - // cryptographer is not thread safe; it should not be accessed after the - // transaction has been released. + // Returns a pointer to our Nigori node handler. + NigoriHandler* GetNigoriHandler(); + + // Returns a pointer to our cryptographer. Does not transfer ownership. + // Not thread safe, so should only be accessed while holding a transaction. Cryptographer* GetCryptographer(const BaseTransaction* trans); // Returns true if the directory had encountered an unrecoverable error. @@ -592,8 +595,6 @@ class Directory { EntryKernel* GetPossibleFirstChild( const ScopedKernelLock& lock, const Id& parent_id); - Cryptographer cryptographer_; - Kernel* kernel_; scoped_ptr<DirectoryBackingStore> store_; @@ -602,6 +603,10 @@ class Directory { const ReportUnrecoverableErrorFunction report_unrecoverable_error_function_; bool unrecoverable_error_set_; + // Not owned. + NigoriHandler* const nigori_handler_; + Cryptographer* const cryptographer_; + InvariantCheckLevel invariant_check_level_; }; diff --git a/sync/syncable/nigori_handler.h b/sync/syncable/nigori_handler.h index 94bb644..9ea1b0e 100644 --- a/sync/syncable/nigori_handler.h +++ b/sync/syncable/nigori_handler.h @@ -36,8 +36,8 @@ class NigoriHandler { syncable::BaseTransaction* const trans) const = 0; // Returns the set of currently encrypted types. - // TODO(zea): force callers to pass their syncable trans here. - virtual ModelTypeSet GetEncryptedTypes() const = 0; + virtual ModelTypeSet GetEncryptedTypes( + syncable::BaseTransaction* const trans) const = 0; }; } // namespace syncable diff --git a/sync/syncable/nigori_util.cc b/sync/syncable/nigori_util.cc index 72d9281..4b59516 100644 --- a/sync/syncable/nigori_util.cc +++ b/sync/syncable/nigori_util.cc @@ -11,6 +11,7 @@ #include "base/json/json_writer.h" #include "sync/syncable/directory.h" #include "sync/syncable/entry.h" +#include "sync/syncable/nigori_handler.h" #include "sync/syncable/mutable_entry.h" #include "sync/syncable/syncable_util.h" #include "sync/syncable/write_transaction.h" @@ -20,9 +21,12 @@ namespace syncer { namespace syncable { bool ProcessUnsyncedChangesForEncryption( - WriteTransaction* const trans, - Cryptographer* cryptographer) { + WriteTransaction* const trans) { + NigoriHandler* nigori_handler = trans->directory()->GetNigoriHandler(); + ModelTypeSet encrypted_types = nigori_handler->GetEncryptedTypes(trans); + Cryptographer* cryptographer = trans->directory()->GetCryptographer(trans); DCHECK(cryptographer->is_ready()); + // Get list of all datatypes with unsynced changes. It's possible that our // local changes need to be encrypted if encryption for that datatype was // just turned on (and vice versa). @@ -36,14 +40,10 @@ bool ProcessUnsyncedChangesForEncryption( const sync_pb::EntitySpecifics& specifics = entry.Get(SPECIFICS); // Ignore types that don't need encryption or entries that are already // encrypted. - if (!SpecificsNeedsEncryption(cryptographer->GetEncryptedTypes(), - specifics)) { + if (!SpecificsNeedsEncryption(encrypted_types, specifics)) continue; - } - if (!UpdateEntryWithEncryption(cryptographer, specifics, &entry)) { - NOTREACHED(); + if (!UpdateEntryWithEncryption(trans, specifics, &entry)) return false; - } } return true; } @@ -93,9 +93,9 @@ bool SpecificsNeedsEncryption(ModelTypeSet encrypted_types, // Mainly for testing. bool VerifyDataTypeEncryptionForTest( BaseTransaction* const trans, - Cryptographer* cryptographer, ModelType type, bool is_encrypted) { + Cryptographer* cryptographer = trans->directory()->GetCryptographer(trans); if (type == PASSWORDS || type == NIGORI) { NOTREACHED(); return true; @@ -157,13 +157,15 @@ bool VerifyDataTypeEncryptionForTest( } bool UpdateEntryWithEncryption( - Cryptographer* cryptographer, + BaseTransaction* const trans, const sync_pb::EntitySpecifics& new_specifics, syncable::MutableEntry* entry) { + NigoriHandler* nigori_handler = trans->directory()->GetNigoriHandler(); + Cryptographer* cryptographer = trans->directory()->GetCryptographer(trans); ModelType type = GetModelTypeFromSpecifics(new_specifics); DCHECK_GE(type, FIRST_REAL_MODEL_TYPE); const sync_pb::EntitySpecifics& old_specifics = entry->Get(SPECIFICS); - const ModelTypeSet encrypted_types = cryptographer->GetEncryptedTypes(); + const ModelTypeSet encrypted_types = nigori_handler->GetEncryptedTypes(trans); // It's possible the nigori lost the set of encrypted types. If the current // specifics are already encrypted, we want to ensure we continue encrypting. bool was_encrypted = old_specifics.has_encrypted(); diff --git a/sync/syncable/nigori_util.h b/sync/syncable/nigori_util.h index 0c020b4..7eefed4 100644 --- a/sync/syncable/nigori_util.h +++ b/sync/syncable/nigori_util.h @@ -40,8 +40,7 @@ bool VerifyUnsyncedChangesAreEncrypted( // Processes all unsynced changes and ensures they are appropriately encrypted // or unencrypted, based on |encrypted_types|. bool ProcessUnsyncedChangesForEncryption( - WriteTransaction* const trans, - Cryptographer* cryptographer); + WriteTransaction* const trans); // Returns true if the entry requires encryption but is not encrypted, false // otherwise. Note: this does not check that already encrypted entries are @@ -56,7 +55,6 @@ bool SpecificsNeedsEncryption(ModelTypeSet encrypted_types, // Verifies all data of type |type| is encrypted appropriately. bool VerifyDataTypeEncryptionForTest( BaseTransaction* const trans, - Cryptographer* cryptographer, ModelType type, bool is_encrypted) WARN_UNUSED_RESULT; @@ -64,7 +62,7 @@ bool VerifyDataTypeEncryptionForTest( // Returns false if an error encrypting occurred (does not modify |entry|). // Note: gracefully handles new_specifics aliasing with entry->Get(SPECIFICS). bool UpdateEntryWithEncryption( - Cryptographer* cryptographer, + BaseTransaction* const trans, const sync_pb::EntitySpecifics& new_specifics, MutableEntry* entry); diff --git a/sync/syncable/syncable_mock.cc b/sync/syncable/syncable_mock.cc index 8000504..9bc68e4 100644 --- a/sync/syncable/syncable_mock.cc +++ b/sync/syncable/syncable_mock.cc @@ -13,8 +13,11 @@ namespace syncer { namespace syncable { MockDirectory::MockDirectory(UnrecoverableErrorHandler* handler) - : Directory(&encryptor_, handler, NULL, - new syncable::InMemoryDirectoryBackingStore("store")) { + : Directory(new syncable::InMemoryDirectoryBackingStore("store"), + handler, + NULL, + NULL, + NULL) { InitKernelForTest("myk", &delegate_, syncable::NullTransactionObserver()); } diff --git a/sync/syncable/syncable_mock.h b/sync/syncable/syncable_mock.h index 83f5b95..eb4ac27 100644 --- a/sync/syncable/syncable_mock.h +++ b/sync/syncable/syncable_mock.h @@ -9,7 +9,6 @@ #include "sync/syncable/directory.h" #include "sync/syncable/write_transaction.h" -#include "sync/test/fake_encryptor.h" #include "sync/test/null_directory_change_delegate.h" #include "testing/gmock/include/gmock/gmock.h" #include "testing/gtest/include/gtest/gtest.h" @@ -32,7 +31,6 @@ class MockDirectory : public Directory { MOCK_METHOD1(PurgeEntriesWithTypeIn, bool(ModelTypeSet)); private: - FakeEncryptor encryptor_; syncable::NullDirectoryChangeDelegate delegate_; }; diff --git a/sync/syncable/syncable_unittest.cc b/sync/syncable/syncable_unittest.cc index e61504b..6a25f75 100644 --- a/sync/syncable/syncable_unittest.cc +++ b/sync/syncable/syncable_unittest.cc @@ -106,8 +106,12 @@ class SyncableGeneralTest : public testing::Test { const char SyncableGeneralTest::kIndexTestName[] = "IndexTest"; TEST_F(SyncableGeneralTest, General) { - Directory dir(&encryptor_, &handler_, NULL, - new InMemoryDirectoryBackingStore("SimpleTest")); + Directory dir(new InMemoryDirectoryBackingStore("SimpleTest"), + &handler_, + NULL, + NULL, + NULL); + ASSERT_EQ(OPENED, dir.Open( "SimpleTest", &delegate_, NullTransactionObserver())); @@ -207,8 +211,11 @@ TEST_F(SyncableGeneralTest, General) { } TEST_F(SyncableGeneralTest, ChildrenOps) { - Directory dir(&encryptor_, &handler_, NULL, - new InMemoryDirectoryBackingStore("SimpleTest")); + Directory dir(new InMemoryDirectoryBackingStore("SimpleTest"), + &handler_, + NULL, + NULL, + NULL); ASSERT_EQ(OPENED, dir.Open( "SimpleTest", &delegate_, NullTransactionObserver())); @@ -281,8 +288,11 @@ TEST_F(SyncableGeneralTest, ClientIndexRebuildsProperly) { // Test creating a new meta entry. { - Directory dir(&encryptor_, &handler_, NULL, - new OnDiskDirectoryBackingStore(kIndexTestName, db_path_)); + Directory dir(new OnDiskDirectoryBackingStore(kIndexTestName, db_path_), + &handler_, + NULL, + NULL, + NULL); ASSERT_EQ(OPENED, dir.Open(kIndexTestName, &delegate_, NullTransactionObserver())); { @@ -299,8 +309,11 @@ TEST_F(SyncableGeneralTest, ClientIndexRebuildsProperly) { // The DB was closed. Now reopen it. This will cause index regeneration. { - Directory dir(&encryptor_, &handler_, NULL, - new OnDiskDirectoryBackingStore(kIndexTestName, db_path_)); + Directory dir(new OnDiskDirectoryBackingStore(kIndexTestName, db_path_), + &handler_, + NULL, + NULL, + NULL); ASSERT_EQ(OPENED, dir.Open(kIndexTestName, &delegate_, NullTransactionObserver())); @@ -321,8 +334,11 @@ TEST_F(SyncableGeneralTest, ClientIndexRebuildsDeletedProperly) { // Test creating a deleted, unsynced, server meta entry. { - Directory dir(&encryptor_, &handler_, NULL, - new OnDiskDirectoryBackingStore(kIndexTestName, db_path_)); + Directory dir(new OnDiskDirectoryBackingStore(kIndexTestName, db_path_), + &handler_, + NULL, + NULL, + NULL); ASSERT_EQ(OPENED, dir.Open(kIndexTestName, &delegate_, NullTransactionObserver())); { @@ -341,8 +357,11 @@ TEST_F(SyncableGeneralTest, ClientIndexRebuildsDeletedProperly) { // The DB was closed. Now reopen it. This will cause index regeneration. // Should still be present and valid in the client tag index. { - Directory dir(&encryptor_, &handler_, NULL, - new OnDiskDirectoryBackingStore(kIndexTestName, db_path_)); + Directory dir(new OnDiskDirectoryBackingStore(kIndexTestName, db_path_), + &handler_, + NULL, + NULL, + NULL); ASSERT_EQ(OPENED, dir.Open(kIndexTestName, &delegate_, NullTransactionObserver())); @@ -357,8 +376,11 @@ TEST_F(SyncableGeneralTest, ClientIndexRebuildsDeletedProperly) { } TEST_F(SyncableGeneralTest, ToValue) { - Directory dir(&encryptor_, &handler_, NULL, - new InMemoryDirectoryBackingStore("SimpleTest")); + Directory dir(new InMemoryDirectoryBackingStore("SimpleTest"), + &handler_, + NULL, + NULL, + NULL); ASSERT_EQ(OPENED, dir.Open( "SimpleTest", &delegate_, NullTransactionObserver())); @@ -400,8 +422,11 @@ class SyncableDirectoryTest : public testing::Test { static const char kName[]; virtual void SetUp() { - dir_.reset(new Directory(&encryptor_, &handler_, NULL, - new InMemoryDirectoryBackingStore(kName))); + dir_.reset(new Directory(new InMemoryDirectoryBackingStore(kName), + &handler_, + NULL, + NULL, + NULL)); ASSERT_TRUE(dir_.get()); ASSERT_EQ(OPENED, dir_->Open(kName, &delegate_, NullTransactionObserver())); @@ -1414,7 +1439,7 @@ TestDirectory* TestDirectory::Create( TestDirectory::TestDirectory(Encryptor* encryptor, UnrecoverableErrorHandler* handler, TestBackingStore* backing_store) - : Directory(encryptor, handler, NULL, backing_store), + : Directory(backing_store, handler, NULL, NULL, NULL), backing_store_(backing_store) { } @@ -1610,8 +1635,12 @@ TEST_F(OnDiskSyncableDirectoryTest, } dir_->SaveChanges(); - dir_.reset(new Directory(&encryptor_, &handler_, NULL, - new OnDiskDirectoryBackingStore(kName, file_path_))); + dir_.reset(new Directory(new OnDiskDirectoryBackingStore(kName, file_path_), + &handler_, + NULL, + NULL, + NULL)); + ASSERT_TRUE(dir_.get()); ASSERT_EQ(OPENED, dir_->Open(kName, &delegate_, NullTransactionObserver())); ASSERT_TRUE(dir_->good()); @@ -1827,7 +1856,11 @@ DirOpenResult SyncableDirectoryTest::ReloadDirImpl() { dir_->Close(); dir_.reset(); - dir_.reset(new Directory(&encryptor_, &handler_, NULL, saved_store)); + dir_.reset(new Directory(saved_store, + &handler_, + NULL, + NULL, + NULL)); DirOpenResult result = dir_->OpenImpl(kName, &delegate_, NullTransactionObserver()); @@ -1861,8 +1894,11 @@ TEST_F(SyncableDirectoryManagement, TestFileRelease) { FilePath path = temp_dir_.path().Append( Directory::kSyncDatabaseFilename); - syncable::Directory dir(&encryptor_, &handler_, NULL, - new OnDiskDirectoryBackingStore("ScopeTest", path)); + syncable::Directory dir(new OnDiskDirectoryBackingStore("ScopeTest", path), + &handler_, + NULL, + NULL, + NULL); DirOpenResult result = dir.Open("ScopeTest", &delegate_, NullTransactionObserver()); ASSERT_EQ(result, OPENED); @@ -1923,8 +1959,11 @@ TEST(SyncableDirectory, StressTransactions) { TestUnrecoverableErrorHandler handler; NullDirectoryChangeDelegate delegate; std::string dirname = "stress"; - Directory dir(&encryptor, &handler, NULL, - new InMemoryDirectoryBackingStore(dirname)); + Directory dir(new InMemoryDirectoryBackingStore(dirname), + &handler, + NULL, + NULL, + NULL); dir.Open(dirname, &delegate, NullTransactionObserver()); const int kThreadCount = 7; diff --git a/sync/test/engine/test_directory_setter_upper.cc b/sync/test/engine/test_directory_setter_upper.cc index 1a68389..318900d 100644 --- a/sync/test/engine/test_directory_setter_upper.cc +++ b/sync/test/engine/test_directory_setter_upper.cc @@ -23,8 +23,12 @@ TestDirectorySetterUpper::TestDirectorySetterUpper() : name_("Test") {} TestDirectorySetterUpper::~TestDirectorySetterUpper() {} void TestDirectorySetterUpper::SetUp() { - directory_.reset(new syncable::Directory(&encryptor_, &handler_, NULL, - new syncable::InMemoryDirectoryBackingStore(name_))); + directory_.reset(new syncable::Directory( + new syncable::InMemoryDirectoryBackingStore(name_), + &handler_, + NULL, + &encryption_handler_, + encryption_handler_.cryptographer())); ASSERT_TRUE(temp_dir_.CreateUniqueTempDir()); ASSERT_EQ(syncable::OPENED, directory_->Open( name_, &delegate_, NullTransactionObserver())); diff --git a/sync/test/engine/test_directory_setter_upper.h b/sync/test/engine/test_directory_setter_upper.h index b492064..5dbce2d 100644 --- a/sync/test/engine/test_directory_setter_upper.h +++ b/sync/test/engine/test_directory_setter_upper.h @@ -35,7 +35,7 @@ #include "base/compiler_specific.h" #include "base/memory/scoped_ptr.h" #include "base/scoped_temp_dir.h" -#include "sync/test/fake_encryptor.h" +#include "sync/test/fake_sync_encryption_handler.h" #include "sync/test/null_directory_change_delegate.h" #include "sync/util/test_unrecoverable_error_handler.h" #include "testing/gmock/include/gmock/gmock.h" @@ -61,6 +61,8 @@ class TestDirectorySetterUpper { syncable::Directory* directory() { return directory_.get(); } + SyncEncryptionHandler* encryption_handler() { return &encryption_handler_; } + private: syncable::NullDirectoryChangeDelegate delegate_; TestUnrecoverableErrorHandler handler_; @@ -68,7 +70,7 @@ class TestDirectorySetterUpper { void RunInvariantCheck(); ScopedTempDir temp_dir_; - FakeEncryptor encryptor_; + FakeSyncEncryptionHandler encryption_handler_; scoped_ptr<syncable::Directory> directory_; std::string name_; diff --git a/sync/test/fake_sync_encryption_handler.cc b/sync/test/fake_sync_encryption_handler.cc index b490862..c589558 100644 --- a/sync/test/fake_sync_encryption_handler.cc +++ b/sync/test/fake_sync_encryption_handler.cc @@ -6,7 +6,6 @@ #include "sync/protocol/nigori_specifics.pb.h" #include "sync/syncable/nigori_util.h" -#include "sync/util/cryptographer.h" namespace syncer { @@ -14,7 +13,7 @@ FakeSyncEncryptionHandler::FakeSyncEncryptionHandler() : encrypted_types_(SensitiveTypes()), encrypt_everything_(false), explicit_passphrase_(false), - cryptographer_(NULL) { + cryptographer_(&encryptor_) { } FakeSyncEncryptionHandler::~FakeSyncEncryptionHandler() {} @@ -30,21 +29,18 @@ void FakeSyncEncryptionHandler::ApplyNigoriUpdate( if (nigori.using_explicit_passphrase()) explicit_passphrase_ = true; - if (!cryptographer_) - return; - - if (cryptographer_->CanDecrypt(nigori.encrypted())) - cryptographer_->InstallKeys(nigori.encrypted()); - else - cryptographer_->SetPendingKeys(nigori.encrypted()); + if (cryptographer_.CanDecrypt(nigori.encrypted())) + cryptographer_.InstallKeys(nigori.encrypted()); + else if (nigori.has_encrypted()) + cryptographer_.SetPendingKeys(nigori.encrypted()); - if (cryptographer_->has_pending_keys()) { + if (cryptographer_.has_pending_keys()) { DVLOG(1) << "OnPassPhraseRequired Sent"; - sync_pb::EncryptedData pending_keys = cryptographer_->GetPendingKeys(); + sync_pb::EncryptedData pending_keys = cryptographer_.GetPendingKeys(); FOR_EACH_OBSERVER(SyncEncryptionHandler::Observer, observers_, OnPassphraseRequired(REASON_DECRYPTION, pending_keys)); - } else if (!cryptographer_->is_ready()) { + } else if (!cryptographer_.is_ready()) { DVLOG(1) << "OnPassphraseRequired sent because cryptographer is not " << "ready"; FOR_EACH_OBSERVER(SyncEncryptionHandler::Observer, observers_, @@ -61,6 +57,11 @@ void FakeSyncEncryptionHandler::UpdateNigoriFromEncryptedTypes( nigori); } +ModelTypeSet FakeSyncEncryptionHandler::GetEncryptedTypes( + syncable::BaseTransaction* const trans) const { + return encrypted_types_; +} + void FakeSyncEncryptionHandler::AddObserver(Observer* observer) { observers_.AddObserver(observer); } @@ -95,10 +96,6 @@ bool FakeSyncEncryptionHandler::EncryptEverythingEnabled() const { return encrypt_everything_; } -ModelTypeSet FakeSyncEncryptionHandler::GetEncryptedTypes() const { - return encrypted_types_; -} - bool FakeSyncEncryptionHandler::IsUsingExplicitPassphrase() const { return explicit_passphrase_; } diff --git a/sync/test/fake_sync_encryption_handler.h b/sync/test/fake_sync_encryption_handler.h index 83a2e63..7456a08 100644 --- a/sync/test/fake_sync_encryption_handler.h +++ b/sync/test/fake_sync_encryption_handler.h @@ -11,11 +11,11 @@ #include "base/observer_list.h" #include "sync/internal_api/public/sync_encryption_handler.h" #include "sync/syncable/nigori_handler.h" +#include "sync/test/fake_encryptor.h" +#include "sync/util/cryptographer.h" namespace syncer { -class Cryptographer; - // A fake sync encryption handler capable of keeping track of the encryption // state without opening any transactions or interacting with the nigori node. // Note that this only performs basic interactions with the cryptographer @@ -28,10 +28,6 @@ class FakeSyncEncryptionHandler : public SyncEncryptionHandler, FakeSyncEncryptionHandler(); virtual ~FakeSyncEncryptionHandler(); - void set_cryptographer(Cryptographer* cryptographer) { - cryptographer_ = cryptographer; - } - // SyncEncryptionHandler implementation. virtual void AddObserver(Observer* observer) OVERRIDE; virtual void RemoveObserver(Observer* observer) OVERRIDE; @@ -47,10 +43,13 @@ class FakeSyncEncryptionHandler : public SyncEncryptionHandler, virtual void ApplyNigoriUpdate( const sync_pb::NigoriSpecifics& nigori, syncable::BaseTransaction* const trans) OVERRIDE; - virtual ModelTypeSet GetEncryptedTypes() const OVERRIDE; virtual void UpdateNigoriFromEncryptedTypes( sync_pb::NigoriSpecifics* nigori, syncable::BaseTransaction* const trans) const OVERRIDE; + virtual ModelTypeSet GetEncryptedTypes( + syncable::BaseTransaction* const trans) const OVERRIDE; + + Cryptographer* cryptographer() { return &cryptographer_; } private: ObserverList<SyncEncryptionHandler::Observer> observers_; @@ -58,7 +57,8 @@ class FakeSyncEncryptionHandler : public SyncEncryptionHandler, bool encrypt_everything_; bool explicit_passphrase_; - Cryptographer* cryptographer_; + FakeEncryptor encryptor_; + Cryptographer cryptographer_; }; } // namespace syncer diff --git a/sync/util/cryptographer.cc b/sync/util/cryptographer.cc index 0cdb389..92f9795 100644 --- a/sync/util/cryptographer.cc +++ b/sync/util/cryptographer.cc @@ -9,7 +9,6 @@ #include "base/base64.h" #include "base/logging.h" #include "sync/protocol/nigori_specifics.pb.h" -#include "sync/syncable/nigori_handler.h" #include "sync/util/encryptor.h" namespace syncer { @@ -25,33 +24,12 @@ const char kNigoriKeyName[] = "nigori-key"; Cryptographer::Cryptographer(Encryptor* encryptor) : encryptor_(encryptor), default_nigori_(NULL), - keystore_nigori_(NULL), - nigori_node_handler_(NULL) { + keystore_nigori_(NULL) { DCHECK(encryptor); } Cryptographer::~Cryptographer() {} -void Cryptographer::SetNigoriHandler(syncable::NigoriHandler* delegate) { - nigori_node_handler_ = delegate; -} - -void Cryptographer::ApplyNigoriUpdate( - const sync_pb::NigoriSpecifics& nigori, - syncable::BaseTransaction* const trans) { - nigori_node_handler_->ApplyNigoriUpdate(nigori, trans); -} - -ModelTypeSet Cryptographer::GetEncryptedTypes() const { - return nigori_node_handler_->GetEncryptedTypes(); -} - -void Cryptographer::UpdateNigoriFromEncryptedTypes( - sync_pb::NigoriSpecifics* nigori, - syncable::BaseTransaction* const trans) const { - nigori_node_handler_->UpdateNigoriFromEncryptedTypes(nigori, trans); -} - void Cryptographer::Bootstrap(const std::string& restored_bootstrap_token) { if (is_initialized()) { @@ -213,6 +191,7 @@ void Cryptographer::SetDefaultKey(const std::string& key_name) { void Cryptographer::SetPendingKeys(const sync_pb::EncryptedData& encrypted) { DCHECK(!CanDecrypt(encrypted)); + DCHECK(!encrypted.blob().empty()); pending_keys_.reset(new sync_pb::EncryptedData(encrypted)); } diff --git a/sync/util/cryptographer.h b/sync/util/cryptographer.h index 6f9ab5c..77a164f 100644 --- a/sync/util/cryptographer.h +++ b/sync/util/cryptographer.h @@ -11,7 +11,6 @@ #include "base/gtest_prod_util.h" #include "base/memory/linked_ptr.h" #include "base/memory/scoped_ptr.h" -#include "sync/internal_api/public/base/model_type.h" #include "sync/protocol/encryption.pb.h" #include "sync/util/nigori.h" @@ -24,11 +23,6 @@ namespace syncer { class Encryptor; -namespace syncable { -class BaseTransaction; -class NigoriHandler; -} - extern const char kNigoriTag[]; // The parameters used to initialize a Nigori instance. @@ -58,19 +52,6 @@ class Cryptographer { explicit Cryptographer(Encryptor* encryptor); ~Cryptographer(); - // Set the sync nigori node handler. - // TODO(zea): refactor so that Cryptographer doesn't need any connection - // to a NigoriHandler. crbug.com/139848 - void SetNigoriHandler(syncable::NigoriHandler* delegate); - - // NigoriHandler delegator methods (passes through to delegate). - void ApplyNigoriUpdate(const sync_pb::NigoriSpecifics& nigori, - syncable::BaseTransaction* const trans); - void UpdateNigoriFromEncryptedTypes( - sync_pb::NigoriSpecifics* nigori, - syncable::BaseTransaction* const trans) const; - ModelTypeSet GetEncryptedTypes() const; - // |restored_bootstrap_token| can be provided via this method to bootstrap // Cryptographer instance into the ready state (is_ready will be true). // It must be a string that was previously built by the @@ -209,10 +190,6 @@ class Cryptographer { scoped_ptr<sync_pb::EncryptedData> pending_keys_; - // The sync nigori node handler. Necessary until we decouple the encrypted - // types from the cryptographer. - syncable::NigoriHandler* nigori_node_handler_; - DISALLOW_COPY_AND_ASSIGN(Cryptographer); }; |