diff options
29 files changed, 910 insertions, 673 deletions
diff --git a/chrome/browser/sync/backend_migrator.cc b/chrome/browser/sync/backend_migrator.cc index 72e711c..e6c5002 100644 --- a/chrome/browser/sync/backend_migrator.cc +++ b/chrome/browser/sync/backend_migrator.cc @@ -101,22 +101,10 @@ bool BackendMigrator::TryStart() { void BackendMigrator::RestartMigration() { // We'll now disable any running types that need to be migrated. ChangeState(DISABLING_TYPES); - const ModelTypeSet full_set = service_->GetPreferredDataTypes(); - const ModelTypeSet difference = Difference(full_set, to_migrate_); - bool configure_with_nigori = !to_migrate_.Has(syncer::NIGORI); SDVLOG(1) << "BackendMigrator disabling types " - << ModelTypeSetToString(to_migrate_) << "; configuring " - << ModelTypeSetToString(difference) - << (configure_with_nigori ? " with nigori" : " without nigori"); - - // Add nigori for config or not based upon if the server told us to migrate - // nigori or not. - if (configure_with_nigori) { - manager_->Configure(difference, syncer::CONFIGURE_REASON_MIGRATION); - } else { - manager_->ConfigureWithoutNigori(difference, - syncer::CONFIGURE_REASON_MIGRATION); - } + << ModelTypeSetToString(to_migrate_); + + manager_->PurgeForMigration(to_migrate_, syncer::CONFIGURE_REASON_MIGRATION); } void BackendMigrator::OnConfigureDone( diff --git a/chrome/browser/sync/backend_migrator_unittest.cc b/chrome/browser/sync/backend_migrator_unittest.cc index 7331072..bc28fcb 100644 --- a/chrome/browser/sync/backend_migrator_unittest.cc +++ b/chrome/browser/sync/backend_migrator_unittest.cc @@ -130,8 +130,12 @@ TEST_F(SyncBackendMigratorTest, Sanity) { EXPECT_CALL(*manager(), state()) .WillOnce(Return(DataTypeManager::CONFIGURED)); - EXPECT_CALL(*manager(), Configure(_, syncer::CONFIGURE_REASON_MIGRATION)) - .Times(2); + EXPECT_CALL( + *manager(), + PurgeForMigration(_, syncer::CONFIGURE_REASON_MIGRATION)).Times(1); + EXPECT_CALL( + *manager(), + Configure(_, syncer::CONFIGURE_REASON_MIGRATION)).Times(1); migrator()->MigrateTypes(to_migrate); EXPECT_EQ(BackendMigrator::DISABLING_TYPES, migrator()->state()); @@ -158,7 +162,7 @@ TEST_F(SyncBackendMigratorTest, MigrateNigori) { EXPECT_CALL(*manager(), state()) .WillOnce(Return(DataTypeManager::CONFIGURED)); - EXPECT_CALL(*manager(), ConfigureWithoutNigori(_, + EXPECT_CALL(*manager(), PurgeForMigration(_, syncer::CONFIGURE_REASON_MIGRATION)); migrator()->MigrateTypes(to_migrate); @@ -189,7 +193,8 @@ TEST_F(SyncBackendMigratorTest, WaitToStart) { Mock::VerifyAndClearExpectations(manager()); EXPECT_CALL(*manager(), state()) .WillOnce(Return(DataTypeManager::CONFIGURED)); - EXPECT_CALL(*manager(), Configure(_, syncer::CONFIGURE_REASON_MIGRATION)); + EXPECT_CALL(*manager(), + PurgeForMigration(_, syncer::CONFIGURE_REASON_MIGRATION)); SetUnsyncedTypes(syncer::ModelTypeSet()); SendConfigureDone(DataTypeManager::OK, syncer::ModelTypeSet()); @@ -208,8 +213,9 @@ TEST_F(SyncBackendMigratorTest, RestartMigration) { EXPECT_CALL(*manager(), state()) .WillOnce(Return(DataTypeManager::CONFIGURED)); - EXPECT_CALL(*manager(), Configure(_, syncer::CONFIGURE_REASON_MIGRATION)) - .Times(2); + EXPECT_CALL( + *manager(), + PurgeForMigration(_, syncer::CONFIGURE_REASON_MIGRATION)).Times(2); migrator()->MigrateTypes(to_migrate1); EXPECT_EQ(BackendMigrator::DISABLING_TYPES, migrator()->state()); @@ -219,8 +225,11 @@ TEST_F(SyncBackendMigratorTest, RestartMigration) { Difference(preferred_types(), to_migrate1); Mock::VerifyAndClearExpectations(manager()); + EXPECT_CALL( + *manager(), + PurgeForMigration(_, syncer::CONFIGURE_REASON_MIGRATION)).Times(1); EXPECT_CALL(*manager(), Configure(_, syncer::CONFIGURE_REASON_MIGRATION)) - .Times(2); + .Times(1); SetUnsyncedTypes(to_migrate1); SendConfigureDone(DataTypeManager::OK, difference1); EXPECT_EQ(BackendMigrator::DISABLING_TYPES, migrator()->state()); @@ -241,13 +250,13 @@ TEST_F(SyncBackendMigratorTest, InterruptedWhileDisablingTypes) { EXPECT_CALL(*manager(), state()) .WillOnce(Return(DataTypeManager::CONFIGURED)); - EXPECT_CALL(*manager(), Configure(HasModelTypes(difference), + EXPECT_CALL(*manager(), PurgeForMigration(HasModelTypes(to_migrate), syncer::CONFIGURE_REASON_MIGRATION)); migrator()->MigrateTypes(to_migrate); EXPECT_EQ(BackendMigrator::DISABLING_TYPES, migrator()->state()); Mock::VerifyAndClearExpectations(manager()); - EXPECT_CALL(*manager(), Configure(HasModelTypes(difference), + EXPECT_CALL(*manager(), PurgeForMigration(HasModelTypes(to_migrate), syncer::CONFIGURE_REASON_MIGRATION)); SetUnsyncedTypes(syncer::ModelTypeSet()); SendConfigureDone(DataTypeManager::OK, preferred_types()); @@ -266,8 +275,12 @@ TEST_F(SyncBackendMigratorTest, WaitingForPurge) { EXPECT_CALL(*manager(), state()) .WillOnce(Return(DataTypeManager::CONFIGURED)); - EXPECT_CALL(*manager(), Configure(_, syncer::CONFIGURE_REASON_MIGRATION)) - .Times(2); + EXPECT_CALL( + *manager(), + PurgeForMigration(_, syncer::CONFIGURE_REASON_MIGRATION)).Times(1); + EXPECT_CALL( + *manager(), + Configure(_, syncer::CONFIGURE_REASON_MIGRATION)).Times(1); migrator()->MigrateTypes(to_migrate); EXPECT_EQ(BackendMigrator::DISABLING_TYPES, migrator()->state()); @@ -292,8 +305,12 @@ TEST_F(SyncBackendMigratorTest, MigratedTypeDisabledByUserDuringMigration) { EXPECT_CALL(*manager(), state()) .WillOnce(Return(DataTypeManager::CONFIGURED)); - EXPECT_CALL(*manager(), Configure(_, syncer::CONFIGURE_REASON_MIGRATION)) - .Times(2); + EXPECT_CALL( + *manager(), + PurgeForMigration(_, syncer::CONFIGURE_REASON_MIGRATION)).Times(1); + EXPECT_CALL( + *manager(), + Configure(_, syncer::CONFIGURE_REASON_MIGRATION)).Times(1); migrator()->MigrateTypes(to_migrate); RemovePreferredType(syncer::PREFERENCES); @@ -311,8 +328,9 @@ TEST_F(SyncBackendMigratorTest, ConfigureFailure) { EXPECT_CALL(*manager(), state()) .WillOnce(Return(DataTypeManager::CONFIGURED)); - EXPECT_CALL(*manager(), Configure(_, syncer::CONFIGURE_REASON_MIGRATION)) - .Times(1); + EXPECT_CALL( + *manager(), + PurgeForMigration(_, syncer::CONFIGURE_REASON_MIGRATION)).Times(1); migrator()->MigrateTypes(to_migrate); SetUnsyncedTypes(syncer::ModelTypeSet()); SendConfigureDone(DataTypeManager::ABORTED, syncer::ModelTypeSet()); diff --git a/chrome/browser/sync/glue/data_type_manager.h b/chrome/browser/sync/glue/data_type_manager.h index 0351088..18db082 100644 --- a/chrome/browser/sync/glue/data_type_manager.h +++ b/chrome/browser/sync/glue/data_type_manager.h @@ -93,8 +93,8 @@ class DataTypeManager { virtual void Configure(TypeSet desired_types, syncer::ConfigureReason reason) = 0; - virtual void ConfigureWithoutNigori(TypeSet desired_types, - syncer::ConfigureReason reason) = 0; + virtual void PurgeForMigration(TypeSet undesired_types, + syncer::ConfigureReason reason) = 0; // Synchronously stops all registered data types. If called after // Configure() is called but before it finishes, it will abort the diff --git a/chrome/browser/sync/glue/data_type_manager_impl.cc b/chrome/browser/sync/glue/data_type_manager_impl.cc index d9e0bb4..6c81668 100644 --- a/chrome/browser/sync/glue/data_type_manager_impl.cc +++ b/chrome/browser/sync/glue/data_type_manager_impl.cc @@ -46,15 +46,15 @@ DataTypeManagerImpl::~DataTypeManagerImpl() {} void DataTypeManagerImpl::Configure(TypeSet desired_types, syncer::ConfigureReason reason) { - desired_types.Put(syncer::NIGORI); + desired_types.PutAll(syncer::ControlTypes()); ConfigureImpl(desired_types, reason); } -void DataTypeManagerImpl::ConfigureWithoutNigori( - TypeSet desired_types, +void DataTypeManagerImpl::PurgeForMigration( + TypeSet undesired_types, syncer::ConfigureReason reason) { - DCHECK(!desired_types.Has(syncer::NIGORI)); - ConfigureImpl(desired_types, reason); + TypeSet remainder = Difference(last_requested_types_, undesired_types); + ConfigureImpl(remainder, reason); } void DataTypeManagerImpl::ConfigureImpl( @@ -114,8 +114,8 @@ void DataTypeManagerImpl::Restart(syncer::ConfigureReason reason) { controllers_->begin(); it != controllers_->end(); ++it) { all_types.Put(it->first); } - // NIGORI has no controller. We must add it manually. - all_types.Put(syncer::NIGORI); + // These have no controller. We must add them manually. + all_types.PutAll(syncer::ControlTypes()); const syncer::ModelTypeSet types_to_add = last_requested_types_; // Check that types_to_add \subseteq all_types. DCHECK(all_types.HasAll(types_to_add)); diff --git a/chrome/browser/sync/glue/data_type_manager_impl.h b/chrome/browser/sync/glue/data_type_manager_impl.h index da417c0..9f38a45 100644 --- a/chrome/browser/sync/glue/data_type_manager_impl.h +++ b/chrome/browser/sync/glue/data_type_manager_impl.h @@ -36,8 +36,8 @@ class DataTypeManagerImpl : public DataTypeManager, syncer::ConfigureReason reason) OVERRIDE; // Needed only for backend migration. - virtual void ConfigureWithoutNigori( - TypeSet desired_types, + virtual void PurgeForMigration( + TypeSet undesired_types, syncer::ConfigureReason reason) OVERRIDE; virtual void Stop() OVERRIDE; diff --git a/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc b/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc index f15f7dd..c0a5142 100644 --- a/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc +++ b/chrome/browser/sync/glue/data_type_manager_impl_unittest.cc @@ -26,15 +26,11 @@ using syncer::BOOKMARKS; using syncer::APPS; using syncer::PASSWORDS; using syncer::PREFERENCES; +using syncer::NIGORI; using testing::_; using testing::Mock; using testing::ResultOf; -enum NigoriState { - WITH_NIGORI, - WITHOUT_NIGORI -}; - // Fake BackendDataTypeConfigurer implementation that simply stores away the // callback passed into ConfigureDataTypes. class FakeBackendDataTypeConfigurer : public BackendDataTypeConfigurer { @@ -80,8 +76,7 @@ DataTypeManager::ConfigureStatus GetStatus( // The actual test harness class, parametrized on nigori state (i.e., tests are // run both configuring with nigori, and configuring without). -class SyncDataTypeManagerImplTest - : public testing::TestWithParam<NigoriState> { +class SyncDataTypeManagerImplTest : public testing::Test { public: SyncDataTypeManagerImplTest() : ui_thread_(content::BrowserThread::UI, &ui_loop_) {} @@ -93,11 +88,6 @@ class SyncDataTypeManagerImplTest virtual void SetUp() { } - // A clearer name for the param accessor. - NigoriState GetNigoriState() { - return GetParam(); - } - void SetConfigureStartExpectation() { EXPECT_CALL(observer_, OnConfigureStart()); } @@ -113,13 +103,7 @@ class SyncDataTypeManagerImplTest // Configure the given DTM with the given desired types. void Configure(DataTypeManagerImpl* dtm, const DataTypeManager::TypeSet& desired_types) { - const syncer::ConfigureReason kReason = - syncer::CONFIGURE_REASON_RECONFIGURATION; - if (GetNigoriState() == WITH_NIGORI) { - dtm->Configure(desired_types, kReason); - } else { - dtm->ConfigureWithoutNigori(desired_types, kReason); - } + dtm->Configure(desired_types, syncer::CONFIGURE_REASON_RECONFIGURATION); } // Finish downloading for the given DTM. Should be done only after @@ -159,7 +143,7 @@ class SyncDataTypeManagerImplTest // Set up a DTM with no controllers, configure it, finish downloading, // and then stop it. -TEST_P(SyncDataTypeManagerImplTest, NoControllers) { +TEST_F(SyncDataTypeManagerImplTest, NoControllers) { DataTypeManagerImpl dtm(&configurer_, &controllers_, &observer_); SetConfigureStartExpectation(); SetConfigureDoneExpectation(DataTypeManager::OK); @@ -176,7 +160,7 @@ TEST_P(SyncDataTypeManagerImplTest, NoControllers) { // Set up a DTM with a single controller, configure it, finish // downloading, finish starting the controller, and then stop the DTM. -TEST_P(SyncDataTypeManagerImplTest, ConfigureOne) { +TEST_F(SyncDataTypeManagerImplTest, ConfigureOne) { AddController(BOOKMARKS); DataTypeManagerImpl dtm(&configurer_, &controllers_, &observer_); @@ -198,7 +182,7 @@ TEST_P(SyncDataTypeManagerImplTest, ConfigureOne) { // Set up a DTM with 2 controllers. configure it. One of them finishes loading // after the timeout. Make sure eventually all are configured. -TEST_P(SyncDataTypeManagerImplTest, ConfigureSlowLoadingType) { +TEST_F(SyncDataTypeManagerImplTest, ConfigureSlowLoadingType) { AddController(BOOKMARKS); AddController(APPS); @@ -245,7 +229,7 @@ TEST_P(SyncDataTypeManagerImplTest, ConfigureSlowLoadingType) { // Set up a DTM with a single controller, configure it, but stop it // before finishing the download. It should still be safe to run the // download callback even after the DTM is stopped and destroyed. -TEST_P(SyncDataTypeManagerImplTest, ConfigureOneStopWhileDownloadPending) { +TEST_F(SyncDataTypeManagerImplTest, ConfigureOneStopWhileDownloadPending) { AddController(BOOKMARKS); { @@ -267,7 +251,7 @@ TEST_P(SyncDataTypeManagerImplTest, ConfigureOneStopWhileDownloadPending) { // downloading, but stop the DTM before the controller finishes // starting up. It should still be safe to finish starting up the // controller even after the DTM is stopped and destroyed. -TEST_P(SyncDataTypeManagerImplTest, ConfigureOneStopWhileStartingModel) { +TEST_F(SyncDataTypeManagerImplTest, ConfigureOneStopWhileStartingModel) { AddController(BOOKMARKS); { @@ -293,7 +277,7 @@ TEST_P(SyncDataTypeManagerImplTest, ConfigureOneStopWhileStartingModel) { // the controller finishes starting up. It should still be safe to // finish starting up the controller even after the DTM is stopped and // destroyed. -TEST_P(SyncDataTypeManagerImplTest, ConfigureOneStopWhileAssociating) { +TEST_F(SyncDataTypeManagerImplTest, ConfigureOneStopWhileAssociating) { AddController(BOOKMARKS); { DataTypeManagerImpl dtm(&configurer_, &controllers_, &observer_); @@ -322,7 +306,7 @@ TEST_P(SyncDataTypeManagerImplTest, ConfigureOneStopWhileAssociating) { // 5) Finish the download for step 4. // 6) Finish starting the controller successfully. // 7) Stop the DTM. -TEST_P(SyncDataTypeManagerImplTest, OneWaitingForCrypto) { +TEST_F(SyncDataTypeManagerImplTest, OneWaitingForCrypto) { AddController(PASSWORDS); DataTypeManagerImpl dtm(&configurer_, &controllers_, &observer_); @@ -372,7 +356,7 @@ TEST_P(SyncDataTypeManagerImplTest, OneWaitingForCrypto) { // 5) Finish the download for step 4. // 6) Finish starting the second controller. // 7) Stop the DTM. -TEST_P(SyncDataTypeManagerImplTest, ConfigureOneThenBoth) { +TEST_F(SyncDataTypeManagerImplTest, ConfigureOneThenBoth) { AddController(BOOKMARKS); AddController(PREFERENCES); @@ -422,7 +406,7 @@ TEST_P(SyncDataTypeManagerImplTest, ConfigureOneThenBoth) { // 5) Finish the download for step 4. // 6) Finish starting the second controller. // 7) Stop the DTM. -TEST_P(SyncDataTypeManagerImplTest, ConfigureOneThenSwitch) { +TEST_F(SyncDataTypeManagerImplTest, ConfigureOneThenSwitch) { AddController(BOOKMARKS); AddController(PREFERENCES); @@ -472,7 +456,7 @@ TEST_P(SyncDataTypeManagerImplTest, ConfigureOneThenSwitch) { // 5) Finish the download for step 3. // 6) Finish starting the second controller. // 7) Stop the DTM. -TEST_P(SyncDataTypeManagerImplTest, ConfigureWhileOneInFlight) { +TEST_F(SyncDataTypeManagerImplTest, ConfigureWhileOneInFlight) { AddController(BOOKMARKS); AddController(PREFERENCES); @@ -518,7 +502,7 @@ TEST_P(SyncDataTypeManagerImplTest, ConfigureWhileOneInFlight) { // Set up a DTM with one controller. Then configure, finish // downloading, and start the controller with an unrecoverable error. // The unrecoverable error should cause the DTM to stop. -TEST_P(SyncDataTypeManagerImplTest, OneFailingController) { +TEST_F(SyncDataTypeManagerImplTest, OneFailingController) { AddController(BOOKMARKS); DataTypeManagerImpl dtm(&configurer_, &controllers_, &observer_); @@ -544,7 +528,7 @@ TEST_P(SyncDataTypeManagerImplTest, OneFailingController) { // 4) Finish starting the second controller with an unrecoverable error. // // The failure from step 4 should cause the DTM to stop. -TEST_P(SyncDataTypeManagerImplTest, SecondControllerFails) { +TEST_F(SyncDataTypeManagerImplTest, SecondControllerFails) { AddController(BOOKMARKS); AddController(PREFERENCES); @@ -582,7 +566,7 @@ TEST_P(SyncDataTypeManagerImplTest, SecondControllerFails) { // // TODO(akalin): Check that the data type that failed association is // recorded in the CONFIGURE_DONE notification. -TEST_P(SyncDataTypeManagerImplTest, OneControllerFailsAssociation) { +TEST_F(SyncDataTypeManagerImplTest, OneControllerFailsAssociation) { AddController(BOOKMARKS); AddController(PREFERENCES); @@ -620,7 +604,7 @@ TEST_P(SyncDataTypeManagerImplTest, OneControllerFailsAssociation) { // 4) Finish the download for step 2. // 5) Finish starting both controllers. // 6) Stop the DTM. -TEST_P(SyncDataTypeManagerImplTest, ConfigureWhileDownloadPending) { +TEST_F(SyncDataTypeManagerImplTest, ConfigureWhileDownloadPending) { AddController(BOOKMARKS); AddController(PREFERENCES); @@ -672,7 +656,7 @@ TEST_P(SyncDataTypeManagerImplTest, ConfigureWhileDownloadPending) { // // The failure from step 3 should be ignored since there's a // reconfigure pending from step 2. -TEST_P(SyncDataTypeManagerImplTest, ConfigureWhileDownloadPendingWithFailure) { +TEST_F(SyncDataTypeManagerImplTest, ConfigureWhileDownloadPendingWithFailure) { AddController(BOOKMARKS); AddController(PREFERENCES); @@ -713,12 +697,102 @@ TEST_P(SyncDataTypeManagerImplTest, ConfigureWhileDownloadPendingWithFailure) { EXPECT_EQ(DataTypeManager::STOPPED, dtm.state()); } -INSTANTIATE_TEST_CASE_P( - WithoutNigori, SyncDataTypeManagerImplTest, - ::testing::Values(WITHOUT_NIGORI)); +// Tests a Purge then Configure. This is similar to the sequence of +// operations that would be invoked by the BackendMigrator. +TEST_F(SyncDataTypeManagerImplTest, MigrateAll) { + AddController(BOOKMARKS); + + DataTypeManagerImpl dtm(&configurer_, &controllers_, &observer_); + SetConfigureStartExpectation(); + SetConfigureDoneExpectation(DataTypeManager::OK); + + // Initial setup. + Configure(&dtm, ModelTypeSet(BOOKMARKS)); + FinishDownload(dtm, ModelTypeSet()); + GetController(BOOKMARKS)->FinishStart(DataTypeController::OK); + + // We've now configured bookmarks and (implicitly) the control types. + EXPECT_EQ(DataTypeManager::CONFIGURED, dtm.state()); + Mock::VerifyAndClearExpectations(&observer_); + + // Pretend we were told to migrate all types. + ModelTypeSet to_migrate; + to_migrate.Put(BOOKMARKS); + to_migrate.PutAll(syncer::ControlTypes()); -INSTANTIATE_TEST_CASE_P( - WithNigori, SyncDataTypeManagerImplTest, - ::testing::Values(WITH_NIGORI)); + SetConfigureStartExpectation(); + SetConfigureDoneExpectation(DataTypeManager::OK); + dtm.PurgeForMigration(to_migrate, + syncer::CONFIGURE_REASON_MIGRATION); + EXPECT_EQ(DataTypeManager::DOWNLOAD_PENDING, dtm.state()); + + // The DTM will call ConfigureDataTypes(), even though it is unnecessary. + FinishDownload(dtm, ModelTypeSet()); + EXPECT_EQ(DataTypeManager::CONFIGURED, dtm.state()); + Mock::VerifyAndClearExpectations(&observer_); + + // Re-enable the migrated types. + SetConfigureStartExpectation(); + SetConfigureDoneExpectation(DataTypeManager::OK); + Configure(&dtm, to_migrate); + FinishDownload(dtm, ModelTypeSet()); + GetController(BOOKMARKS)->FinishStart(DataTypeController::OK); + EXPECT_EQ(DataTypeManager::CONFIGURED, dtm.state()); +} + +// Test receipt of a Configure request while a purge is in flight. +TEST_F(SyncDataTypeManagerImplTest, ConfigureDuringPurge) { + AddController(BOOKMARKS); + AddController(PREFERENCES); + + DataTypeManagerImpl dtm(&configurer_, &controllers_, &observer_); + + // Initial configure. + SetConfigureStartExpectation(); + SetConfigureDoneExpectation(DataTypeManager::OK); + Configure(&dtm, ModelTypeSet(BOOKMARKS)); + FinishDownload(dtm, ModelTypeSet()); + GetController(BOOKMARKS)->FinishStart(DataTypeController::OK); + EXPECT_EQ(DataTypeManager::CONFIGURED, dtm.state()); + Mock::VerifyAndClearExpectations(&observer_); + + // Purge the Nigori type. + SetConfigureStartExpectation(); + dtm.PurgeForMigration(ModelTypeSet(NIGORI), + syncer::CONFIGURE_REASON_MIGRATION); + EXPECT_EQ(DataTypeManager::DOWNLOAD_PENDING, dtm.state()); + Mock::VerifyAndClearExpectations(&observer_); + + // Before the backend configuration completes, ask for a different + // set of types. This request asks for + // - BOOKMARKS: which is redundant because it was already enabled, + // - PREFERENCES: which is new and will need to be downloaded, and + // - NIGORI: (added implicitly because it is a control type) which + // the DTM is part-way through purging. + SetConfigureBlockedExpectation(); + Configure(&dtm, ModelTypeSet(BOOKMARKS, PREFERENCES)); + EXPECT_EQ(DataTypeManager::DOWNLOAD_PENDING, dtm.state()); + + // Invoke the callback we've been waiting for since we asked to purge NIGORI. + FinishDownload(dtm, ModelTypeSet()); + EXPECT_EQ(DataTypeManager::BLOCKED, dtm.state()); + Mock::VerifyAndClearExpectations(&observer_); + + SetConfigureDoneExpectation(DataTypeManager::OK); + // Pump the loop to run the posted DTMI::ConfigureImpl() task from + // DTMI::ProcessReconfigure(). + ui_loop_.RunAllPending(); + EXPECT_EQ(DataTypeManager::DOWNLOAD_PENDING, dtm.state()); + + // Now invoke the callback for the second configure request. + FinishDownload(dtm, ModelTypeSet()); + EXPECT_EQ(DataTypeManager::CONFIGURING, dtm.state()); + + // Start the preferences controller. We don't need to start controller for + // the NIGORI because it has none. We don't need to start the controller for + // the BOOKMARKS because it was never stopped. + GetController(PREFERENCES)->FinishStart(DataTypeController::OK); + EXPECT_EQ(DataTypeManager::CONFIGURED, dtm.state()); +} } // namespace browser_sync diff --git a/chrome/browser/sync/glue/data_type_manager_mock.h b/chrome/browser/sync/glue/data_type_manager_mock.h index 5f6d192..a1447b3 100644 --- a/chrome/browser/sync/glue/data_type_manager_mock.h +++ b/chrome/browser/sync/glue/data_type_manager_mock.h @@ -18,8 +18,7 @@ class DataTypeManagerMock : public DataTypeManager { virtual ~DataTypeManagerMock(); MOCK_METHOD2(Configure, void(TypeSet, syncer::ConfigureReason)); - MOCK_METHOD2(ConfigureWithoutNigori, - void(TypeSet, syncer::ConfigureReason)); + MOCK_METHOD2(PurgeForMigration, void(TypeSet, syncer::ConfigureReason)); MOCK_METHOD0(Stop, void()); MOCK_METHOD0(controllers, const DataTypeController::TypeMap&()); MOCK_CONST_METHOD0(state, State()); diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc index 579a40c..7d1711d 100644 --- a/chrome/browser/sync/glue/sync_backend_host.cc +++ b/chrome/browser/sync/glue/sync_backend_host.cc @@ -1349,7 +1349,7 @@ void SyncBackendHost::HandleInitializationCompletedOnFrontendLoop( initialization_state_ = DOWNLOADING_NIGORI; ConfigureDataTypes( syncer::CONFIGURE_REASON_NEW_CLIENT, - syncer::ModelTypeSet(syncer::NIGORI), + syncer::ModelTypeSet(syncer::ControlTypes()), syncer::ModelTypeSet(), // Calls back into this function. base::Bind( diff --git a/chrome/browser/sync/glue/sync_backend_host_unittest.cc b/chrome/browser/sync/glue/sync_backend_host_unittest.cc index ec96c74..5a4ae88 100644 --- a/chrome/browser/sync/glue/sync_backend_host_unittest.cc +++ b/chrome/browser/sync/glue/sync_backend_host_unittest.cc @@ -204,7 +204,7 @@ class SyncBackendHostTest : public testing::Test { // Synchronously configures the backend's datatypes. void ConfigureDataTypes(syncer::ModelTypeSet types_to_add, syncer::ModelTypeSet types_to_remove) { - types_to_add.Put(syncer::NIGORI); + types_to_add.PutAll(syncer::ControlTypes()); backend_->ConfigureDataTypes( syncer::CONFIGURE_REASON_RECONFIGURATION, types_to_add, @@ -247,28 +247,28 @@ class SyncBackendHostTest : public testing::Test { TEST_F(SyncBackendHostTest, InitShutdown) { InitializeBackend(); EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().Equals( - syncer::ModelTypeSet(syncer::NIGORI))); + syncer::ControlTypes())); EXPECT_TRUE(fake_manager_->InitialSyncEndedTypes().Equals( - syncer::ModelTypeSet(syncer::NIGORI))); + syncer::ControlTypes())); EXPECT_TRUE(fake_manager_->GetTypesWithEmptyProgressMarkerToken( - syncer::ModelTypeSet(syncer::NIGORI)).Empty()); + syncer::ControlTypes()).Empty()); } // Test first time sync scenario. All types should be properly configured. TEST_F(SyncBackendHostTest, FirstTimeSync) { InitializeBackend(); EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().Equals( - syncer::ModelTypeSet(syncer::NIGORI))); + syncer::ControlTypes())); EXPECT_TRUE(fake_manager_->InitialSyncEndedTypes().Equals( - syncer::ModelTypeSet(syncer::NIGORI))); + syncer::ControlTypes())); EXPECT_TRUE(fake_manager_->GetTypesWithEmptyProgressMarkerToken( - syncer::ModelTypeSet(syncer::NIGORI)).Empty()); + syncer::ControlTypes()).Empty()); ConfigureDataTypes(enabled_types_, Difference(syncer::ModelTypeSet::All(), enabled_types_)); EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().HasAll( - enabled_types_)); + Difference(enabled_types_, syncer::ControlTypes()))); EXPECT_TRUE(fake_manager_->InitialSyncEndedTypes().Equals(enabled_types_)); EXPECT_TRUE(fake_manager_->GetAndResetEnabledTypes().Equals(enabled_types_)); EXPECT_TRUE(fake_manager_->GetTypesWithEmptyProgressMarkerToken( @@ -348,12 +348,12 @@ TEST_F(SyncBackendHostTest, LostDB) { // left untouched. InitializeBackend(); EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().Equals( - syncer::ModelTypeSet(syncer::NIGORI))); + syncer::ModelTypeSet(syncer::ControlTypes()))); EXPECT_TRUE(fake_manager_->InitialSyncEndedTypes().Equals( - syncer::ModelTypeSet(syncer::NIGORI))); + syncer::ModelTypeSet(syncer::ControlTypes()))); EXPECT_TRUE(fake_manager_->GetTypesWithEmptyProgressMarkerToken( enabled_types_).Equals( - Difference(enabled_types_, syncer::ModelTypeSet(syncer::NIGORI)))); + Difference(enabled_types_, syncer::ControlTypes()))); // The database was empty, so any cleaning is entirely optional. We want to // reset this value before running the next part of the test, though. @@ -364,7 +364,7 @@ TEST_F(SyncBackendHostTest, LostDB) { Difference(syncer::ModelTypeSet::All(), enabled_types_)); EXPECT_TRUE(fake_manager_->GetAndResetDownloadedTypes().HasAll( - enabled_types_)); + Difference(enabled_types_, syncer::ControlTypes()))); EXPECT_TRUE(Intersection(fake_manager_->GetAndResetCleanedTypes(), enabled_types_).Empty()); EXPECT_TRUE(fake_manager_->InitialSyncEndedTypes().Equals(enabled_types_)); diff --git a/chrome/browser/sync/sync_prefs.cc b/chrome/browser/sync/sync_prefs.cc index 2ba2dd9..1b7a92f 100644 --- a/chrome/browser/sync/sync_prefs.cc +++ b/chrome/browser/sync/sync_prefs.cc @@ -336,15 +336,15 @@ void SyncPrefs::RegisterPreferences() { enable_by_default, PrefService::UNSYNCABLE_PREF); + syncer::ModelTypeSet user_types = syncer::UserTypes(); + // Treat bookmarks specially. RegisterDataTypePreferredPref(syncer::BOOKMARKS, true); - for (int i = syncer::PREFERENCES; i < syncer::MODEL_TYPE_COUNT; ++i) { - const syncer::ModelType type = syncer::ModelTypeFromInt(i); - // Also treat nigori specially. - if (type == syncer::NIGORI) { - continue; - } - RegisterDataTypePreferredPref(type, enable_by_default); + user_types.Remove(syncer::BOOKMARKS); + + for (syncer::ModelTypeSet::Iterator it = user_types.First(); + it.Good(); it.Inc()) { + RegisterDataTypePreferredPref(it.Get(), enable_by_default); } pref_service_->RegisterBooleanPref(prefs::kSyncManaged, diff --git a/sync/engine/apply_control_data_updates.cc b/sync/engine/apply_control_data_updates.cc new file mode 100644 index 0000000..2aa7dd4 --- /dev/null +++ b/sync/engine/apply_control_data_updates.cc @@ -0,0 +1,138 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "sync/engine/apply_control_data_updates.h" + +#include "base/metrics/histogram.h" +#include "sync/engine/conflict_resolver.h" +#include "sync/engine/conflict_util.h" +#include "sync/engine/syncer_util.h" +#include "sync/syncable/directory.h" +#include "sync/syncable/mutable_entry.h" +#include "sync/syncable/nigori_handler.h" +#include "sync/syncable/nigori_util.h" +#include "sync/syncable/write_transaction.h" +#include "sync/util/cryptographer.h" + +namespace syncer { + +using syncable::GET_BY_SERVER_TAG; +using syncable::IS_UNAPPLIED_UPDATE; +using syncable::IS_UNSYNCED; +using syncable::SERVER_SPECIFICS; +using syncable::SPECIFICS; +using syncable::SYNCER; + +void ApplyControlDataUpdates(syncable::Directory* dir) { + syncable::WriteTransaction trans(FROM_HERE, SYNCER, dir); + + if (ApplyNigoriUpdates(&trans, dir->GetCryptographer(&trans))) { + dir->set_initial_sync_ended_for_type(NIGORI, true); + } +} + +// Update the sync encryption handler with the server's nigori node. +// +// If we have a locally modified nigori node, we merge them manually. This +// handles the case where two clients both set a different passphrase. The +// second client to attempt to commit will go into a state of having pending +// keys, unioned the set of encrypted types, and eventually re-encrypt +// everything with the passphrase of the first client and commit the set of +// merged encryption keys. Until the second client provides the pending +// passphrase, the cryptographer will preserve the encryption keys based on the +// local passphrase, while the nigori node will preserve the server encryption +// keys. +bool ApplyNigoriUpdates(syncable::WriteTransaction* trans, + Cryptographer* cryptographer) { + syncable::MutableEntry nigori_node(trans, GET_BY_SERVER_TAG, + ModelTypeToRootTag(NIGORI)); + + // Mainly for unit tests. We should have a Nigori node by this point. + if (!nigori_node.good()) { + return false; + } + + if (!nigori_node.Get(IS_UNAPPLIED_UPDATE)) { + return true; + } + + const sync_pb::NigoriSpecifics& nigori = + nigori_node.Get(SERVER_SPECIFICS).nigori(); + 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 + // re-encrypted at SetDecryptionPassphrase time (via ReEncryptEverything). + // This logic covers the case where the nigori update marked new datatypes + // for encryption, but didn't change the passphrase. + if (cryptographer->is_ready()) { + // Note that we don't bother to encrypt any data for which IS_UNSYNCED + // == false here. The machine that turned on encryption should know about + // and re-encrypt all synced data. It's possible it could get interrupted + // during this process, but we currently reencrypt everything at startup + // as well, so as soon as a client is restarted with this datatype marked + // for encryption, all the data should be updated as necessary. + + // 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); + } + + if (!nigori_node.Get(IS_UNSYNCED)) { // Update only. + UpdateLocalDataFromServerData(trans, &nigori_node); + } else { // Conflict. + // Create a new set of specifics based on the server specifics (which + // preserves their encryption keys). + sync_pb::EntitySpecifics specifics = nigori_node.Get(SERVER_SPECIFICS); + sync_pb::NigoriSpecifics* server_nigori = specifics.mutable_nigori(); + // Store the merged set of encrypted types. + // (NigoriHandler::ApplyNigoriUpdate(..) will have merged the local types + // already). + trans->directory()->GetNigoriHandler()->UpdateNigoriFromEncryptedTypes( + server_nigori, + trans); + // The cryptographer has the both the local and remote encryption keys + // (added at NigoriHandler::ApplyNigoriUpdate(..) time). + // If the cryptographer is ready, then it already merged both sets of keys + // and we can store them back in. In that case, the remote key was already + // part of the local keybag, so we preserve the local key as the default + // (including whether it's an explicit key). + // If the cryptographer is not ready, then the user will have to provide + // the passphrase to decrypt the pending keys. When they do so, the + // SetDecryptionPassphrase code will act based on whether the server + // update has an explicit passphrase or not. + // - If the server had an explicit passphrase, that explicit passphrase + // will be preserved as the default encryption key. + // - If the server did not have an explicit passphrase, we assume the + // local passphrase is the most up to date and preserve the local + // default encryption key marked as an implicit passphrase. + // This works fine except for the case where we had locally set an + // explicit passphrase. In that case the nigori node will have the default + // key based on the local explicit passphassphrase, but will not have it + // marked as explicit. To fix this we'd have to track whether we have a + // explicit passphrase or not separate from the nigori, which would + // introduce even more complexity, so we leave it up to the user to reset + // that passphrase as an explicit one via settings. The goal here is to + // ensure both sets of encryption keys are preserved. + if (cryptographer->is_ready()) { + cryptographer->GetKeys(server_nigori->mutable_encrypted()); + server_nigori->set_using_explicit_passphrase( + nigori_node.Get(SPECIFICS).nigori().using_explicit_passphrase()); + } + nigori_node.Put(SPECIFICS, specifics); + DVLOG(1) << "Resolving simple conflict, merging nigori nodes: " + << nigori_node; + + OverwriteServerChanges(&nigori_node); + + UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict", + ConflictResolver::NIGORI_MERGE, + ConflictResolver::CONFLICT_RESOLUTION_SIZE); + } + + return true; +} + +} // namespace syncer diff --git a/sync/engine/apply_control_data_updates.h b/sync/engine/apply_control_data_updates.h new file mode 100644 index 0000000..efe2cc8 --- /dev/null +++ b/sync/engine/apply_control_data_updates.h @@ -0,0 +1,23 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#ifndef SYNC_ENGINE_APPLY_CONTROL_DATA_UPDATES_H_ +#define SYNC_ENGINE_APPLY_CONTROL_DATA_UPDATES_H_ + +namespace syncer { + +class Cryptographer; + +namespace syncable { +class Directory; +class WriteTransaction; +} + +void ApplyControlDataUpdates(syncable::Directory* dir); +bool ApplyNigoriUpdates(syncable::WriteTransaction* trans, + Cryptographer* cryptographer); + +} // namespace syncer + +#endif // SYNC_ENGINE_APPLY_CONTROL_DATA_UPDATES_H_ diff --git a/sync/engine/apply_control_data_updates_unittest.cc b/sync/engine/apply_control_data_updates_unittest.cc new file mode 100644 index 0000000..dbfc98d --- /dev/null +++ b/sync/engine/apply_control_data_updates_unittest.cc @@ -0,0 +1,347 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "base/format_macros.h" +#include "base/location.h" +#include "base/memory/scoped_ptr.h" +#include "base/stringprintf.h" +#include "sync/engine/apply_control_data_updates.h" +#include "sync/engine/syncer.h" +#include "sync/engine/syncer_util.h" +#include "sync/internal_api/public/test/test_entry_factory.h" +#include "sync/protocol/nigori_specifics.pb.h" +#include "sync/syncable/mutable_entry.h" +#include "sync/syncable/nigori_util.h" +#include "sync/syncable/read_transaction.h" +#include "sync/syncable/syncable_util.h" +#include "sync/syncable/write_transaction.h" +#include "sync/test/engine/fake_model_worker.h" +#include "sync/test/engine/syncer_command_test.h" +#include "sync/test/engine/test_id_factory.h" +#include "sync/test/fake_sync_encryption_handler.h" +#include "sync/util/cryptographer.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace syncer { + +using syncable::MutableEntry; +using syncable::UNITTEST; +using syncable::Id; + +class ApplyControlDataUpdatesTest : public SyncerCommandTest { + public: + protected: + ApplyControlDataUpdatesTest() {} + virtual ~ApplyControlDataUpdatesTest() {} + + virtual void SetUp() { + workers()->clear(); + mutable_routing_info()->clear(); + workers()->push_back(make_scoped_refptr(new FakeModelWorker(GROUP_UI))); + workers()->push_back( + make_scoped_refptr(new FakeModelWorker(GROUP_PASSWORD))); + (*mutable_routing_info())[BOOKMARKS] = GROUP_UI; + (*mutable_routing_info())[PASSWORDS] = GROUP_PASSWORD; + (*mutable_routing_info())[NIGORI] = GROUP_PASSIVE; + SyncerCommandTest::SetUp(); + entry_factory_.reset(new TestEntryFactory(directory())); + + syncable::ReadTransaction trans(FROM_HERE, directory()); + } + + TestIdFactory id_factory_; + scoped_ptr<TestEntryFactory> entry_factory_; + private: + DISALLOW_COPY_AND_ASSIGN(ApplyControlDataUpdatesTest); +}; + +TEST_F(ApplyControlDataUpdatesTest, NigoriUpdate) { + // Storing the cryptographer separately is bad, but for this test we + // know it's safe. + Cryptographer* cryptographer; + ModelTypeSet encrypted_types; + encrypted_types.Put(PASSWORDS); + + // We start with initial_sync_ended == false. This is wrong, since would have + // no nigori node if that were the case. Howerver, it makes it easier to + // verify that ApplyControlDataUpdates sets initial_sync_ended correctly. + EXPECT_FALSE(directory()->initial_sync_ended_types().Has(NIGORI)); + + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + cryptographer = directory()->GetCryptographer(&trans); + EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) + .Equals(encrypted_types)); + } + + // Nigori node updates should update the Cryptographer. + Cryptographer other_cryptographer(cryptographer->encryptor()); + KeyParams params = {"localhost", "dummy", "foobar"}; + other_cryptographer.AddKey(params); + + sync_pb::EntitySpecifics specifics; + sync_pb::NigoriSpecifics* nigori = specifics.mutable_nigori(); + other_cryptographer.GetKeys(nigori->mutable_encrypted()); + nigori->set_encrypt_everything(true); + entry_factory_->CreateUnappliedNewItem( + ModelTypeToRootTag(NIGORI), specifics, true); + EXPECT_FALSE(cryptographer->has_pending_keys()); + + ApplyControlDataUpdates(directory()); + + EXPECT_FALSE(cryptographer->is_ready()); + EXPECT_TRUE(cryptographer->has_pending_keys()); + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) + .Equals(ModelTypeSet::All())); + EXPECT_TRUE(directory()->initial_sync_ended_types().Has(NIGORI)); + } +} + +TEST_F(ApplyControlDataUpdatesTest, NigoriUpdateForDisabledTypes) { + // Storing the cryptographer separately is bad, but for this test we + // know it's safe. + Cryptographer* cryptographer; + ModelTypeSet encrypted_types; + encrypted_types.Put(PASSWORDS); + + // We start with initial_sync_ended == false. This is wrong, since would have + // no nigori node if that were the case. Howerver, it makes it easier to + // verify that ApplyControlDataUpdates sets initial_sync_ended correctly. + EXPECT_FALSE(directory()->initial_sync_ended_types().Has(NIGORI)); + + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + cryptographer = directory()->GetCryptographer(&trans); + EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) + .Equals(encrypted_types)); + } + + // Nigori node updates should update the Cryptographer. + Cryptographer other_cryptographer(cryptographer->encryptor()); + KeyParams params = {"localhost", "dummy", "foobar"}; + other_cryptographer.AddKey(params); + + sync_pb::EntitySpecifics specifics; + sync_pb::NigoriSpecifics* nigori = specifics.mutable_nigori(); + other_cryptographer.GetKeys(nigori->mutable_encrypted()); + nigori->set_encrypt_everything(true); + entry_factory_->CreateUnappliedNewItem( + ModelTypeToRootTag(NIGORI), specifics, true); + EXPECT_FALSE(cryptographer->has_pending_keys()); + + ApplyControlDataUpdates(directory()); + + EXPECT_FALSE(cryptographer->is_ready()); + EXPECT_TRUE(cryptographer->has_pending_keys()); + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) + .Equals(ModelTypeSet::All())); + EXPECT_TRUE(directory()->initial_sync_ended_types().Has(NIGORI)); + } +} + +// Create some local unsynced and unencrypted data. Apply a nigori update that +// turns on encryption for the unsynced data. Ensure we properly encrypt the +// data as part of the nigori update. Apply another nigori update with no +// changes. Ensure we ignore already-encrypted unsynced data and that nothing +// breaks. +TEST_F(ApplyControlDataUpdatesTest, EncryptUnsyncedChanges) { + // Storing the cryptographer separately is bad, but for this test we + // know it's safe. + Cryptographer* cryptographer; + ModelTypeSet encrypted_types; + encrypted_types.Put(PASSWORDS); + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + cryptographer = directory()->GetCryptographer(&trans); + EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) + .Equals(encrypted_types)); + + // With default encrypted_types, this should be true. + EXPECT_TRUE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types)); + + Syncer::UnsyncedMetaHandles handles; + GetUnsyncedEntries(&trans, &handles); + EXPECT_TRUE(handles.empty()); + } + + // Create unsynced bookmarks without encryption. + // First item is a folder + Id folder_id = id_factory_.NewLocalId(); + entry_factory_->CreateUnsyncedItem(folder_id, id_factory_.root(), "folder", + true, BOOKMARKS, NULL); + // Next five items are children of the folder + size_t i; + size_t batch_s = 5; + for (i = 0; i < batch_s; ++i) { + entry_factory_->CreateUnsyncedItem(id_factory_.NewLocalId(), folder_id, + base::StringPrintf("Item %"PRIuS"", i), + false, BOOKMARKS, NULL); + } + // Next five items are children of the root. + for (; i < 2*batch_s; ++i) { + entry_factory_->CreateUnsyncedItem( + id_factory_.NewLocalId(), id_factory_.root(), + base::StringPrintf("Item %"PRIuS"", i), false, + BOOKMARKS, NULL); + } + + KeyParams params = {"localhost", "dummy", "foobar"}; + cryptographer->AddKey(params); + sync_pb::EntitySpecifics specifics; + sync_pb::NigoriSpecifics* nigori = specifics.mutable_nigori(); + cryptographer->GetKeys(nigori->mutable_encrypted()); + nigori->set_encrypt_everything(true); + encrypted_types.Put(BOOKMARKS); + entry_factory_->CreateUnappliedNewItem( + ModelTypeToRootTag(NIGORI), specifics, true); + EXPECT_FALSE(cryptographer->has_pending_keys()); + EXPECT_TRUE(cryptographer->is_ready()); + + { + // Ensure we have unsynced nodes that aren't properly encrypted. + syncable::ReadTransaction trans(FROM_HERE, directory()); + EXPECT_FALSE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types)); + + Syncer::UnsyncedMetaHandles handles; + GetUnsyncedEntries(&trans, &handles); + EXPECT_EQ(2*batch_s+1, handles.size()); + } + + ApplyControlDataUpdates(directory()); + + EXPECT_FALSE(cryptographer->has_pending_keys()); + EXPECT_TRUE(cryptographer->is_ready()); + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + + // If ProcessUnsyncedChangesForEncryption worked, all our unsynced changes + // should be encrypted now. + EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) + .Equals(ModelTypeSet::All())); + EXPECT_TRUE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types)); + + Syncer::UnsyncedMetaHandles handles; + GetUnsyncedEntries(&trans, &handles); + EXPECT_EQ(2*batch_s+1, handles.size()); + } + + // Simulate another nigori update that doesn't change anything. + { + syncable::WriteTransaction trans(FROM_HERE, UNITTEST, directory()); + MutableEntry entry(&trans, syncable::GET_BY_SERVER_TAG, + ModelTypeToRootTag(NIGORI)); + ASSERT_TRUE(entry.good()); + entry.Put(syncable::SERVER_VERSION, entry_factory_->GetNextRevision()); + entry.Put(syncable::IS_UNAPPLIED_UPDATE, true); + } + + ApplyControlDataUpdates(directory()); + + EXPECT_FALSE(cryptographer->has_pending_keys()); + EXPECT_TRUE(cryptographer->is_ready()); + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + + // All our changes should still be encrypted. + EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) + .Equals(ModelTypeSet::All())); + EXPECT_TRUE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types)); + + Syncer::UnsyncedMetaHandles handles; + GetUnsyncedEntries(&trans, &handles); + EXPECT_EQ(2*batch_s+1, handles.size()); + } +} + +TEST_F(ApplyControlDataUpdatesTest, CannotEncryptUnsyncedChanges) { + // Storing the cryptographer separately is bad, but for this test we + // know it's safe. + Cryptographer* cryptographer; + ModelTypeSet encrypted_types; + encrypted_types.Put(PASSWORDS); + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + cryptographer = directory()->GetCryptographer(&trans); + EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) + .Equals(encrypted_types)); + + // With default encrypted_types, this should be true. + EXPECT_TRUE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types)); + + Syncer::UnsyncedMetaHandles handles; + GetUnsyncedEntries(&trans, &handles); + EXPECT_TRUE(handles.empty()); + } + + // Create unsynced bookmarks without encryption. + // First item is a folder + Id folder_id = id_factory_.NewLocalId(); + entry_factory_->CreateUnsyncedItem( + folder_id, id_factory_.root(), "folder", true, + BOOKMARKS, NULL); + // Next five items are children of the folder + size_t i; + size_t batch_s = 5; + for (i = 0; i < batch_s; ++i) { + entry_factory_->CreateUnsyncedItem(id_factory_.NewLocalId(), folder_id, + base::StringPrintf("Item %"PRIuS"", i), + false, BOOKMARKS, NULL); + } + // Next five items are children of the root. + for (; i < 2*batch_s; ++i) { + entry_factory_->CreateUnsyncedItem( + id_factory_.NewLocalId(), id_factory_.root(), + base::StringPrintf("Item %"PRIuS"", i), false, + BOOKMARKS, NULL); + } + + // We encrypt with new keys, triggering the local cryptographer to be unready + // and unable to decrypt data (once updated). + Cryptographer other_cryptographer(cryptographer->encryptor()); + KeyParams params = {"localhost", "dummy", "foobar"}; + other_cryptographer.AddKey(params); + sync_pb::EntitySpecifics specifics; + sync_pb::NigoriSpecifics* nigori = specifics.mutable_nigori(); + other_cryptographer.GetKeys(nigori->mutable_encrypted()); + nigori->set_encrypt_everything(true); + encrypted_types.Put(BOOKMARKS); + entry_factory_->CreateUnappliedNewItem( + ModelTypeToRootTag(NIGORI), specifics, true); + EXPECT_FALSE(cryptographer->has_pending_keys()); + + { + // Ensure we have unsynced nodes that aren't properly encrypted. + syncable::ReadTransaction trans(FROM_HERE, directory()); + EXPECT_FALSE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types)); + Syncer::UnsyncedMetaHandles handles; + GetUnsyncedEntries(&trans, &handles); + EXPECT_EQ(2*batch_s+1, handles.size()); + } + + ApplyControlDataUpdates(directory()); + + EXPECT_FALSE(cryptographer->is_ready()); + EXPECT_TRUE(cryptographer->has_pending_keys()); + { + syncable::ReadTransaction trans(FROM_HERE, directory()); + + // 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(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) + .Equals(ModelTypeSet::All())); + EXPECT_FALSE(cryptographer->is_ready()); + EXPECT_TRUE(cryptographer->has_pending_keys()); + + Syncer::UnsyncedMetaHandles handles; + GetUnsyncedEntries(&trans, &handles); + EXPECT_EQ(2*batch_s+1, handles.size()); + } +} + +} // namespace syncer diff --git a/sync/engine/apply_updates_command.cc b/sync/engine/apply_updates_command.cc index da8a1cc..7d4e72e 100644 --- a/sync/engine/apply_updates_command.cc +++ b/sync/engine/apply_updates_command.cc @@ -57,6 +57,10 @@ SyncerError ApplyUpdatesCommand::ModelChangingExecuteImpl( } } + // Don't process control type updates here. They will be handled elsewhere. + FullModelTypeSet control_types = ToFullModelTypeSet(ControlTypes()); + server_type_restriction.RemoveAll(control_types); + std::vector<int64> handles; dir->GetUnappliedUpdateMetaHandles( &trans, server_type_restriction, &handles); @@ -77,6 +81,10 @@ SyncerError ApplyUpdatesCommand::ModelChangingExecuteImpl( if (status.ServerSaysNothingMoreToDownload()) { for (ModelTypeSet::Iterator it = status.updates_request_types().First(); it.Good(); it.Inc()) { + // Don't set the flag for control types. We didn't process them here. + if (IsControlType(it.Get())) + continue; + // This gets persisted to the directory's backing store. dir->set_initial_sync_ended_for_type(it.Get(), true); } diff --git a/sync/engine/apply_updates_command_unittest.cc b/sync/engine/apply_updates_command_unittest.cc index 5e14c59..eee3edb 100644 --- a/sync/engine/apply_updates_command_unittest.cc +++ b/sync/engine/apply_updates_command_unittest.cc @@ -4,7 +4,6 @@ #include <string> -#include "base/format_macros.h" #include "base/location.h" #include "base/memory/scoped_ptr.h" #include "base/stringprintf.h" @@ -13,9 +12,7 @@ #include "sync/internal_api/public/test/test_entry_factory.h" #include "sync/protocol/bookmark_specifics.pb.h" #include "sync/protocol/password_specifics.pb.h" -#include "sync/sessions/sync_session.h" #include "sync/syncable/mutable_entry.h" -#include "sync/syncable/nigori_util.h" #include "sync/syncable/read_transaction.h" #include "sync/syncable/syncable_id.h" #include "sync/syncable/syncable_util.h" @@ -23,14 +20,12 @@ #include "sync/test/engine/fake_model_worker.h" #include "sync/test/engine/syncer_command_test.h" #include "sync/test/engine/test_id_factory.h" -#include "sync/test/fake_encryptor.h" #include "sync/test/fake_sync_encryption_handler.h" #include "sync/util/cryptographer.h" #include "testing/gtest/include/gtest/gtest.h" namespace syncer { -using sessions::SyncSession; using std::string; using syncable::Id; using syncable::MutableEntry; @@ -71,7 +66,6 @@ class ApplyUpdatesCommandTest : public SyncerCommandTest { DISALLOW_COPY_AND_ASSIGN(ApplyUpdatesCommandTest); ApplyUpdatesCommand apply_updates_command_; - TestIdFactory id_factory_; scoped_ptr<TestEntryFactory> entry_factory_; }; @@ -168,7 +162,7 @@ TEST_F(ApplyUpdatesCommandTest, HierarchyAndSimpleConflict) { ASSERT_TRUE(entry.good()); entry.Put(syncable::SERVER_PARENT_ID, - id_factory_.MakeServer("bogus_parent")); + TestIdFactory::MakeServer("bogus_parent")); } ExpectGroupToChange(apply_updates_command_, GROUP_UI); @@ -205,12 +199,12 @@ TEST_F(ApplyUpdatesCommandTest, HierarchyConflictDirectoryLoop) { // Re-parent from root to "Y" entry.Put(syncable::SERVER_VERSION, entry_factory_->GetNextRevision()); entry.Put(syncable::IS_UNAPPLIED_UPDATE, true); - entry.Put(syncable::SERVER_PARENT_ID, id_factory_.MakeServer("Y")); + entry.Put(syncable::SERVER_PARENT_ID, TestIdFactory::MakeServer("Y")); } // Item 'Y' is child of 'X'. entry_factory_->CreateUnsyncedItem( - id_factory_.MakeServer("Y"), id_factory_.MakeServer("X"), "Y", true, + TestIdFactory::MakeServer("Y"), TestIdFactory::MakeServer("X"), "Y", true, BOOKMARKS, NULL); // If the server's update were applied, we would have X be a child of Y, and Y @@ -240,7 +234,7 @@ TEST_F(ApplyUpdatesCommandTest, HierarchyConflictDeletedParent) { // Create a locally deleted parent item. int64 parent_handle; entry_factory_->CreateUnsyncedItem( - Id::CreateFromServerId("parent"), id_factory_.root(), + Id::CreateFromServerId("parent"), TestIdFactory::root(), "parent", true, BOOKMARKS, &parent_handle); { WriteTransaction trans(FROM_HERE, UNITTEST, directory()); @@ -286,13 +280,13 @@ TEST_F(ApplyUpdatesCommandTest, HierarchyConflictDeleteNonEmptyDirectory) { // Delete it on the server. entry.Put(syncable::SERVER_VERSION, entry_factory_->GetNextRevision()); entry.Put(syncable::IS_UNAPPLIED_UPDATE, true); - entry.Put(syncable::SERVER_PARENT_ID, id_factory_.root()); + entry.Put(syncable::SERVER_PARENT_ID, TestIdFactory::root()); entry.Put(syncable::SERVER_IS_DEL, true); } // Create a local child of the server-deleted directory. entry_factory_->CreateUnsyncedItem( - id_factory_.MakeServer("child"), id_factory_.MakeServer("parent"), + TestIdFactory::MakeServer("child"), TestIdFactory::MakeServer("parent"), "child", false, BOOKMARKS, NULL); // The server's request to delete the directory must be ignored, otherwise our @@ -515,349 +509,4 @@ TEST_F(ApplyUpdatesCommandTest, SomeUndecryptablePassword) { } } -TEST_F(ApplyUpdatesCommandTest, NigoriUpdate) { - // Storing the cryptographer separately is bad, but for this test we - // know it's safe. - Cryptographer* cryptographer; - ModelTypeSet encrypted_types; - encrypted_types.Put(PASSWORDS); - encrypted_types.Put(NIGORI); - { - syncable::ReadTransaction trans(FROM_HERE, directory()); - cryptographer = directory()->GetCryptographer(&trans); - EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) - .Equals(encrypted_types)); - } - - // Nigori node updates should update the Cryptographer. - Cryptographer other_cryptographer(cryptographer->encryptor()); - KeyParams params = {"localhost", "dummy", "foobar"}; - other_cryptographer.AddKey(params); - - sync_pb::EntitySpecifics specifics; - sync_pb::NigoriSpecifics* nigori = specifics.mutable_nigori(); - other_cryptographer.GetKeys(nigori->mutable_encrypted()); - nigori->set_encrypt_everything(true); - entry_factory_->CreateUnappliedNewItem( - ModelTypeToRootTag(NIGORI), specifics, true); - EXPECT_FALSE(cryptographer->has_pending_keys()); - - ExpectGroupToChange(apply_updates_command_, GROUP_PASSIVE); - apply_updates_command_.ExecuteImpl(session()); - - sessions::StatusController* status = session()->mutable_status_controller(); - sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE); - ASSERT_TRUE(status->update_progress()); - EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize()) - << "All updates should have been attempted"; - ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) - << "The nigori update shouldn't be in conflict"; - EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount()) - << "The nigori update should be applied"; - - EXPECT_FALSE(cryptographer->is_ready()); - EXPECT_TRUE(cryptographer->has_pending_keys()); - { - syncable::ReadTransaction trans(FROM_HERE, directory()); - EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) - .Equals(ModelTypeSet::All())); - } -} - -TEST_F(ApplyUpdatesCommandTest, NigoriUpdateForDisabledTypes) { - // Storing the cryptographer separately is bad, but for this test we - // know it's safe. - Cryptographer* cryptographer; - ModelTypeSet encrypted_types; - encrypted_types.Put(PASSWORDS); - encrypted_types.Put(NIGORI); - { - syncable::ReadTransaction trans(FROM_HERE, directory()); - cryptographer = directory()->GetCryptographer(&trans); - EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) - .Equals(encrypted_types)); - } - - // Nigori node updates should update the Cryptographer. - Cryptographer other_cryptographer(cryptographer->encryptor()); - KeyParams params = {"localhost", "dummy", "foobar"}; - other_cryptographer.AddKey(params); - - sync_pb::EntitySpecifics specifics; - sync_pb::NigoriSpecifics* nigori = specifics.mutable_nigori(); - other_cryptographer.GetKeys(nigori->mutable_encrypted()); - nigori->set_encrypt_everything(true); - entry_factory_->CreateUnappliedNewItem( - ModelTypeToRootTag(NIGORI), specifics, true); - EXPECT_FALSE(cryptographer->has_pending_keys()); - - ExpectGroupToChange(apply_updates_command_, GROUP_PASSIVE); - apply_updates_command_.ExecuteImpl(session()); - - sessions::StatusController* status = session()->mutable_status_controller(); - sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE); - ASSERT_TRUE(status->update_progress()); - EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize()) - << "All updates should have been attempted"; - ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) - << "The nigori update shouldn't be in conflict"; - EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount()) - << "The nigori update should be applied"; - - EXPECT_FALSE(cryptographer->is_ready()); - EXPECT_TRUE(cryptographer->has_pending_keys()); - { - 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 -// turns on encryption for the unsynced data. Ensure we properly encrypt the -// data as part of the nigori update. Apply another nigori update with no -// changes. Ensure we ignore already-encrypted unsynced data and that nothing -// breaks. -TEST_F(ApplyUpdatesCommandTest, EncryptUnsyncedChanges) { - // Storing the cryptographer separately is bad, but for this test we - // know it's safe. - Cryptographer* cryptographer; - ModelTypeSet encrypted_types; - encrypted_types.Put(PASSWORDS); - encrypted_types.Put(NIGORI); - { - syncable::ReadTransaction trans(FROM_HERE, directory()); - cryptographer = directory()->GetCryptographer(&trans); - EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) - .Equals(encrypted_types)); - - // With default encrypted_types, this should be true. - EXPECT_TRUE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types)); - - Syncer::UnsyncedMetaHandles handles; - GetUnsyncedEntries(&trans, &handles); - EXPECT_TRUE(handles.empty()); - } - - // Create unsynced bookmarks without encryption. - // First item is a folder - Id folder_id = id_factory_.NewLocalId(); - entry_factory_->CreateUnsyncedItem(folder_id, id_factory_.root(), "folder", - true, BOOKMARKS, NULL); - // Next five items are children of the folder - size_t i; - size_t batch_s = 5; - for (i = 0; i < batch_s; ++i) { - entry_factory_->CreateUnsyncedItem(id_factory_.NewLocalId(), folder_id, - base::StringPrintf("Item %"PRIuS"", i), - false, BOOKMARKS, NULL); - } - // Next five items are children of the root. - for (; i < 2*batch_s; ++i) { - entry_factory_->CreateUnsyncedItem( - id_factory_.NewLocalId(), id_factory_.root(), - base::StringPrintf("Item %"PRIuS"", i), false, - BOOKMARKS, NULL); - } - - KeyParams params = {"localhost", "dummy", "foobar"}; - cryptographer->AddKey(params); - sync_pb::EntitySpecifics specifics; - sync_pb::NigoriSpecifics* nigori = specifics.mutable_nigori(); - cryptographer->GetKeys(nigori->mutable_encrypted()); - nigori->set_encrypt_everything(true); - encrypted_types.Put(BOOKMARKS); - entry_factory_->CreateUnappliedNewItem( - ModelTypeToRootTag(NIGORI), specifics, true); - EXPECT_FALSE(cryptographer->has_pending_keys()); - EXPECT_TRUE(cryptographer->is_ready()); - - { - // Ensure we have unsynced nodes that aren't properly encrypted. - syncable::ReadTransaction trans(FROM_HERE, directory()); - EXPECT_FALSE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types)); - - Syncer::UnsyncedMetaHandles handles; - GetUnsyncedEntries(&trans, &handles); - EXPECT_EQ(2*batch_s+1, handles.size()); - } - - ExpectGroupToChange(apply_updates_command_, GROUP_PASSIVE); - apply_updates_command_.ExecuteImpl(session()); - - { - sessions::StatusController* status = session()->mutable_status_controller(); - sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE); - ASSERT_TRUE(status->update_progress()); - EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize()) - << "All updates should have been attempted"; - ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) - << "No updates should be in conflict"; - EXPECT_EQ(0, status->conflict_progress()->EncryptionConflictingItemsSize()) - << "No updates should be in conflict"; - EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount()) - << "The nigori update should be applied"; - } - EXPECT_FALSE(cryptographer->has_pending_keys()); - EXPECT_TRUE(cryptographer->is_ready()); - { - syncable::ReadTransaction trans(FROM_HERE, directory()); - - // If ProcessUnsyncedChangesForEncryption worked, all our unsynced changes - // should be encrypted now. - EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) - .Equals(ModelTypeSet::All())); - EXPECT_TRUE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types)); - - Syncer::UnsyncedMetaHandles handles; - GetUnsyncedEntries(&trans, &handles); - EXPECT_EQ(2*batch_s+1, handles.size()); - } - - // Simulate another nigori update that doesn't change anything. - { - WriteTransaction trans(FROM_HERE, UNITTEST, directory()); - MutableEntry entry(&trans, syncable::GET_BY_SERVER_TAG, - ModelTypeToRootTag(NIGORI)); - ASSERT_TRUE(entry.good()); - entry.Put(syncable::SERVER_VERSION, entry_factory_->GetNextRevision()); - entry.Put(syncable::IS_UNAPPLIED_UPDATE, true); - } - ExpectGroupToChange(apply_updates_command_, GROUP_PASSIVE); - apply_updates_command_.ExecuteImpl(session()); - { - sessions::StatusController* status = session()->mutable_status_controller(); - sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE); - ASSERT_TRUE(status->update_progress()); - EXPECT_EQ(2, status->update_progress()->AppliedUpdatesSize()) - << "All updates should have been attempted"; - ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) - << "No updates should be in conflict"; - EXPECT_EQ(0, status->conflict_progress()->EncryptionConflictingItemsSize()) - << "No updates should be in conflict"; - EXPECT_EQ(2, status->update_progress()->SuccessfullyAppliedUpdateCount()) - << "The nigori update should be applied"; - } - EXPECT_FALSE(cryptographer->has_pending_keys()); - EXPECT_TRUE(cryptographer->is_ready()); - { - syncable::ReadTransaction trans(FROM_HERE, directory()); - - // All our changes should still be encrypted. - EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) - .Equals(ModelTypeSet::All())); - EXPECT_TRUE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types)); - - Syncer::UnsyncedMetaHandles handles; - GetUnsyncedEntries(&trans, &handles); - EXPECT_EQ(2*batch_s+1, handles.size()); - } -} - -TEST_F(ApplyUpdatesCommandTest, CannotEncryptUnsyncedChanges) { - // Storing the cryptographer separately is bad, but for this test we - // know it's safe. - Cryptographer* cryptographer; - ModelTypeSet encrypted_types; - encrypted_types.Put(PASSWORDS); - encrypted_types.Put(NIGORI); - { - syncable::ReadTransaction trans(FROM_HERE, directory()); - cryptographer = directory()->GetCryptographer(&trans); - EXPECT_TRUE(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) - .Equals(encrypted_types)); - - // With default encrypted_types, this should be true. - EXPECT_TRUE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types)); - - Syncer::UnsyncedMetaHandles handles; - GetUnsyncedEntries(&trans, &handles); - EXPECT_TRUE(handles.empty()); - } - - // Create unsynced bookmarks without encryption. - // First item is a folder - Id folder_id = id_factory_.NewLocalId(); - entry_factory_->CreateUnsyncedItem( - folder_id, id_factory_.root(), "folder", true, - BOOKMARKS, NULL); - // Next five items are children of the folder - size_t i; - size_t batch_s = 5; - for (i = 0; i < batch_s; ++i) { - entry_factory_->CreateUnsyncedItem(id_factory_.NewLocalId(), folder_id, - base::StringPrintf("Item %"PRIuS"", i), - false, BOOKMARKS, NULL); - } - // Next five items are children of the root. - for (; i < 2*batch_s; ++i) { - entry_factory_->CreateUnsyncedItem( - id_factory_.NewLocalId(), id_factory_.root(), - base::StringPrintf("Item %"PRIuS"", i), false, - BOOKMARKS, NULL); - } - - // We encrypt with new keys, triggering the local cryptographer to be unready - // and unable to decrypt data (once updated). - Cryptographer other_cryptographer(cryptographer->encryptor()); - KeyParams params = {"localhost", "dummy", "foobar"}; - other_cryptographer.AddKey(params); - sync_pb::EntitySpecifics specifics; - sync_pb::NigoriSpecifics* nigori = specifics.mutable_nigori(); - other_cryptographer.GetKeys(nigori->mutable_encrypted()); - nigori->set_encrypt_everything(true); - encrypted_types.Put(BOOKMARKS); - entry_factory_->CreateUnappliedNewItem( - ModelTypeToRootTag(NIGORI), specifics, true); - EXPECT_FALSE(cryptographer->has_pending_keys()); - - { - // Ensure we have unsynced nodes that aren't properly encrypted. - syncable::ReadTransaction trans(FROM_HERE, directory()); - EXPECT_FALSE(VerifyUnsyncedChangesAreEncrypted(&trans, encrypted_types)); - Syncer::UnsyncedMetaHandles handles; - GetUnsyncedEntries(&trans, &handles); - EXPECT_EQ(2*batch_s+1, handles.size()); - } - - ExpectGroupToChange(apply_updates_command_, GROUP_PASSIVE); - apply_updates_command_.ExecuteImpl(session()); - - sessions::StatusController* status = session()->mutable_status_controller(); - sessions::ScopedModelSafeGroupRestriction r(status, GROUP_PASSIVE); - ASSERT_TRUE(status->update_progress()); - EXPECT_EQ(1, status->update_progress()->AppliedUpdatesSize()) - << "All updates should have been attempted"; - ASSERT_TRUE(status->conflict_progress()); - EXPECT_EQ(0, status->conflict_progress()->SimpleConflictingItemsSize()) - << "The unsynced changes don't trigger a blocking conflict with the " - << "nigori update."; - EXPECT_EQ(0, status->conflict_progress()->EncryptionConflictingItemsSize()) - << "The unsynced changes don't trigger an encryption conflict with the " - << "nigori update."; - EXPECT_EQ(1, status->update_progress()->SuccessfullyAppliedUpdateCount()) - << "The nigori update should be applied"; - EXPECT_FALSE(cryptographer->is_ready()); - EXPECT_TRUE(cryptographer->has_pending_keys()); - { - syncable::ReadTransaction trans(FROM_HERE, directory()); - - // 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(directory()->GetNigoriHandler()->GetEncryptedTypes(&trans) - .Equals(ModelTypeSet::All())); - EXPECT_FALSE(cryptographer->is_ready()); - EXPECT_TRUE(cryptographer->has_pending_keys()); - - Syncer::UnsyncedMetaHandles handles; - GetUnsyncedEntries(&trans, &handles); - EXPECT_EQ(2*batch_s+1, handles.size()); - } -} - } // namespace syncer diff --git a/sync/engine/conflict_resolver.cc b/sync/engine/conflict_resolver.cc index 736e668..73c25ea 100644 --- a/sync/engine/conflict_resolver.cc +++ b/sync/engine/conflict_resolver.cc @@ -11,9 +11,9 @@ #include "base/location.h" #include "base/metrics/histogram.h" +#include "sync/engine/conflict_util.h" #include "sync/engine/syncer.h" #include "sync/engine/syncer_util.h" -#include "sync/protocol/nigori_specifics.pb.h" #include "sync/sessions/status_controller.h" #include "sync/syncable/directory.h" #include "sync/syncable/mutable_entry.h" @@ -48,25 +48,6 @@ ConflictResolver::ConflictResolver() { ConflictResolver::~ConflictResolver() { } -void ConflictResolver::IgnoreLocalChanges(MutableEntry* entry) { - // An update matches local actions, merge the changes. - // This is a little fishy because we don't actually merge them. - // In the future we should do a 3-way merge. - // With IS_UNSYNCED false, changes should be merged. - entry->Put(syncable::IS_UNSYNCED, false); -} - -void ConflictResolver::OverwriteServerChanges(WriteTransaction* trans, - MutableEntry * entry) { - // This is similar to an overwrite from the old client. - // This is equivalent to a scenario where we got the update before we'd - // made our local client changes. - // TODO(chron): This is really a general property clobber. We clobber - // the server side property. Perhaps we should actually do property merging. - entry->Put(syncable::BASE_VERSION, entry->Get(syncable::SERVER_VERSION)); - entry->Put(syncable::IS_UNAPPLIED_UPDATE, false); -} - ConflictResolver::ProcessSimpleConflictResult ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans, const Id& id, @@ -221,70 +202,11 @@ ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans, base_server_specifics_match = true; } - // We manually merge nigori data. - if (entry.GetModelType() == NIGORI) { - // Create a new set of specifics based on the server specifics (which - // preserves their encryption keys). - sync_pb::EntitySpecifics specifics = - entry.Get(syncable::SERVER_SPECIFICS); - sync_pb::NigoriSpecifics* server_nigori = specifics.mutable_nigori(); - // Store the merged set of encrypted types (cryptographer->Update(..) will - // have merged the local types already). - 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 - // and we can store them back in. In that case, the remote key was already - // part of the local keybag, so we preserve the local key as the default - // (including whether it's an explicit key). - // If the cryptographer is not ready, then the user will have to provide - // the passphrase to decrypt the pending keys. When they do so, the - // SetDecryptionPassphrase code will act based on whether the server - // update has an explicit passphrase or not. - // - If the server had an explicit passphrase, that explicit passphrase - // will be preserved as the default encryption key. - // - If the server did not have an explicit passphrase, we assume the - // local passphrase is the most up to date and preserve the local - // default encryption key marked as an implicit passphrase. - // This works fine except for the case where we had locally set an - // explicit passphrase. In that case the nigori node will have the default - // key based on the local explicit passphassphrase, but will not have it - // marked as explicit. To fix this we'd have to track whether we have a - // explicit passphrase or not separate from the nigori, which would - // introduce even more complexity, so we leave it up to the user to - // reset that passphrase as an explicit one via settings. The goal here - // is to ensure both sets of encryption keys are preserved. - if (cryptographer->is_ready()) { - cryptographer->GetKeys(server_nigori->mutable_encrypted()); - server_nigori->set_using_explicit_passphrase( - entry.Get(syncable::SPECIFICS).nigori(). - using_explicit_passphrase()); - } - // We deliberately leave the server's device information. This client will - // add its own device information on restart. - entry.Put(syncable::SPECIFICS, specifics); - DVLOG(1) << "Resolving simple conflict, merging nigori nodes: " << entry; - status->increment_num_server_overwrites(); - OverwriteServerChanges(trans, &entry); - UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict", - NIGORI_MERGE, - CONFLICT_RESOLUTION_SIZE); - } else if (!entry_deleted && name_matches && parent_matches && - specifics_match && !needs_reinsertion) { + if (!entry_deleted && name_matches && parent_matches && specifics_match && + !needs_reinsertion) { DVLOG(1) << "Resolving simple conflict, everything matches, ignoring " << "changes for: " << entry; - // This unsets both IS_UNSYNCED and IS_UNAPPLIED_UPDATE, and sets the - // BASE_VERSION to match the SERVER_VERSION. If we didn't also unset - // IS_UNAPPLIED_UPDATE, then we would lose unsynced positional data from - // adjacent entries when the server update gets applied and the item is - // re-inserted into the PREV_ID/NEXT_ID linked list. This is primarily - // an issue because we commit after applying updates, and is most - // commonly seen when positional changes are made while a passphrase - // is required (and hence there will be many encryption conflicts). - OverwriteServerChanges(trans, &entry); - IgnoreLocalChanges(&entry); + IgnoreConflict(&entry); UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict", CHANGES_MATCH, CONFLICT_RESOLUTION_SIZE); @@ -292,12 +214,12 @@ ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans, DVLOG(1) << "Resolving simple conflict, ignoring server encryption " << " changes for: " << entry; status->increment_num_server_overwrites(); - OverwriteServerChanges(trans, &entry); + OverwriteServerChanges(&entry); UMA_HISTOGRAM_ENUMERATION("Sync.ResolveSimpleConflict", IGNORE_ENCRYPTION, CONFLICT_RESOLUTION_SIZE); } else if (entry_deleted || !name_matches || !parent_matches) { - OverwriteServerChanges(trans, &entry); + OverwriteServerChanges(&entry); status->increment_num_server_overwrites(); DVLOG(1) << "Resolving simple conflict, overwriting server changes " << "for: " << entry; @@ -341,7 +263,7 @@ ConflictResolver::ProcessSimpleConflict(WriteTransaction* trans, DCHECK_EQ(entry.Get(syncable::SERVER_VERSION), 0) << "For the server to " "know to re-create, client-tagged items should revert to version 0 " "when server-deleted."; - OverwriteServerChanges(trans, &entry); + OverwriteServerChanges(&entry); status->increment_num_server_overwrites(); DVLOG(1) << "Resolving simple conflict, undeleting server entry: " << entry; @@ -384,6 +306,14 @@ bool ConflictResolver::ResolveConflicts(syncable::WriteTransaction* trans, if (processed_items.count(id) > 0) continue; + // We don't resolve conflicts for control types here. + Entry conflicting_node(trans, syncable::GET_BY_ID, id); + CHECK(conflicting_node.good()); + if (IsControlType( + GetModelTypeFromSpecifics(conflicting_node.Get(syncable::SPECIFICS)))) { + continue; + } + // We have a simple conflict. In order check if positions have changed, // we need to process conflicting predecessors before successors. Traverse // backwards through all continuous conflicting predecessors, building a diff --git a/sync/engine/conflict_resolver.h b/sync/engine/conflict_resolver.h index 2efb1e9..7e5be74 100644 --- a/sync/engine/conflict_resolver.h +++ b/sync/engine/conflict_resolver.h @@ -65,10 +65,6 @@ class ConflictResolver { SYNC_PROGRESS, // Progress made. }; - void IgnoreLocalChanges(syncable::MutableEntry* entry); - void OverwriteServerChanges(syncable::WriteTransaction* trans, - syncable::MutableEntry* entry); - ProcessSimpleConflictResult ProcessSimpleConflict( syncable::WriteTransaction* trans, const syncable::Id& id, diff --git a/sync/engine/conflict_util.cc b/sync/engine/conflict_util.cc new file mode 100644 index 0000000..fbb684e --- /dev/null +++ b/sync/engine/conflict_util.cc @@ -0,0 +1,51 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "sync/engine/conflict_util.h" + +#include "sync/syncable/mutable_entry.h" + +namespace syncer { + +using syncable::BASE_VERSION; +using syncable::IS_UNAPPLIED_UPDATE; +using syncable::IS_UNSYNCED; +using syncable::SERVER_VERSION; + +using syncable::MutableEntry; + +// Allow the server's changes to take precedence. +// This will take effect during the next ApplyUpdates step. +void IgnoreLocalChanges(MutableEntry* entry) { + DCHECK(entry->Get(IS_UNSYNCED)); + DCHECK(entry->Get(IS_UNAPPLIED_UPDATE)); + entry->Put(IS_UNSYNCED, false); +} + +// Overwrite the server with our own value. +// We will commit our local data, overwriting the server, at the next +// opportunity. +void OverwriteServerChanges(MutableEntry* entry) { + DCHECK(entry->Get(IS_UNSYNCED)); + DCHECK(entry->Get(IS_UNAPPLIED_UPDATE)); + entry->Put(BASE_VERSION, entry->Get(SERVER_VERSION)); + entry->Put(IS_UNAPPLIED_UPDATE, false); +} + +// Having determined that everything matches, we ignore the non-conflict. +void IgnoreConflict(MutableEntry* entry) { + // If we didn't also unset IS_UNAPPLIED_UPDATE, then we would lose unsynced + // positional data from adjacent entries when the server update gets applied + // and the item is re-inserted into the PREV_ID/NEXT_ID linked list. This is + // primarily an issue because we commit after applying updates, and is most + // commonly seen when positional changes are made while a passphrase is + // required (and hence there will be many encryption conflicts). + DCHECK(entry->Get(IS_UNSYNCED)); + DCHECK(entry->Get(IS_UNAPPLIED_UPDATE)); + entry->Put(BASE_VERSION, entry->Get(SERVER_VERSION)); + entry->Put(IS_UNAPPLIED_UPDATE, false); + entry->Put(IS_UNSYNCED, false); +} + +} // namespace syncer diff --git a/sync/engine/conflict_util.h b/sync/engine/conflict_util.h new file mode 100644 index 0000000..c3efa5e --- /dev/null +++ b/sync/engine/conflict_util.h @@ -0,0 +1,31 @@ +// Copyright (c) 2012 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. +// +// Utility functions that act on syncable::MutableEntry to resolve conflicts. + +#ifndef SYNC_ENGINE_CONFLICT_UTIL_H_ +#define SYNC_ENGINE_CONFLICT_UTIL_H_ + +namespace syncable { +class MutableEntry; +} + +namespace syncer { + +// Marks the item as no longer requiring sync, allowing the server's version +// to 'win' during the next update application step. +void IgnoreLocalChanges(syncable::MutableEntry* entry); + +// Marks the item as no longer requiring update from server data. This will +// cause the item to be committed to the server, overwriting the server's +// version. +void OverwriteServerChanges(syncable::MutableEntry* entry); + +// The local and server versions are identical, so unset the bits that put them +// into a conflicting state. +void IgnoreConflict(syncable::MutableEntry *trans); + +} // namespace syncer + +#endif // SYNC_ENGINE_CONFLICT_UTIL_H_ diff --git a/sync/engine/syncer.cc b/sync/engine/syncer.cc index 66afdbc..d8d2ce3 100644 --- a/sync/engine/syncer.cc +++ b/sync/engine/syncer.cc @@ -10,6 +10,7 @@ #include "base/message_loop.h" #include "base/time.h" #include "build/build_config.h" +#include "sync/engine/apply_control_data_updates.h" #include "sync/engine/apply_updates_command.h" #include "sync/engine/build_commit_command.h" #include "sync/engine/commit.h" @@ -158,6 +159,9 @@ void Syncer::SyncShare(sessions::SyncSession* session, break; } case APPLY_UPDATES: { + // These include encryption updates that should be applied early. + ApplyControlDataUpdates(session->context()->directory()); + ApplyUpdatesCommand apply_updates; apply_updates.Execute(session); session->SendEventNotification(SyncEngineEvent::STATUS_CHANGED); diff --git a/sync/engine/syncer_util.cc b/sync/engine/syncer_util.cc index 8a243df..85e1ff5 100644 --- a/sync/engine/syncer_util.cc +++ b/sync/engine/syncer_util.cc @@ -16,14 +16,11 @@ #include "sync/engine/syncer_types.h" #include "sync/internal_api/public/base/model_type.h" #include "sync/protocol/bookmark_specifics.pb.h" -#include "sync/protocol/nigori_specifics.pb.h" #include "sync/protocol/password_specifics.pb.h" #include "sync/protocol/sync.pb.h" #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" #include "sync/syncable/syncable_proto_util.h" @@ -195,51 +192,6 @@ UpdateAttemptResponse AttemptToUpdateEntry( syncable::Id id = entry->Get(ID); const sync_pb::EntitySpecifics& specifics = entry->Get(SERVER_SPECIFICS); - // We intercept updates to the Nigori node, update the Cryptographer and - // encrypt any unsynced changes here because there is no Nigori - // ChangeProcessor. We never put the nigori node in a state of - // conflict_encryption. - // - // We always update the cryptographer with the server's nigori node, - // even if we have a locally modified nigori node (we manually merge nigori - // data in the conflict resolver in that case). This handles the case where - // two clients both set a different passphrase. The second client to attempt - // to commit will go into a state of having pending keys, unioned the set of - // encrypted types, and eventually re-encrypt everything with the passphrase - // of the first client and commit the set of merged encryption keys. Until the - // second client provides the pending passphrase, the cryptographer will - // preserve the encryption keys based on the local passphrase, while the - // nigori node will preserve the server encryption keys. - // - // If non-encryption changes are made to the nigori node, they will be - // lost as part of conflict resolution. This is intended, as we place a higher - // priority on preserving the server's passphrase change to preserving local - // non-encryption changes. Next time the non-encryption changes are made to - // the nigori node (e.g. on restart), they will commit without issue. - if (specifics.has_nigori()) { - const sync_pb::NigoriSpecifics& nigori = specifics.nigori(); - 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 - // re-encrypted at SetDecryptionPassphrase time (via ReEncryptEverything). - // This logic covers the case where the nigori update marked new datatypes - // for encryption, but didn't change the passphrase. - if (cryptographer->is_ready()) { - // Note that we don't bother to encrypt any data for which IS_UNSYNCED - // == false here. The machine that turned on encryption should know about - // and re-encrypt all synced data. It's possible it could get interrupted - // during this process, but we currently reencrypt everything at startup - // as well, so as soon as a client is restarted with this datatype marked - // for encryption, all the data should be updated as necessary. - - // 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); - } - } - // Only apply updates that we can decrypt. If we can't decrypt the update, it // is likely because the passphrase has not arrived yet. Because the // passphrase may not arrive within this GetUpdates, we can't just return diff --git a/sync/internal_api/public/base/model_type.h b/sync/internal_api/public/base/model_type.h index 20c8ffa..8037e54 100644 --- a/sync/internal_api/public/base/model_type.h +++ b/sync/internal_api/public/base/model_type.h @@ -52,7 +52,8 @@ enum ModelType { // // A bookmark folder or a bookmark URL object. BOOKMARKS, - FIRST_REAL_MODEL_TYPE = BOOKMARKS, // Declared 2nd, for debugger prettiness. + FIRST_USER_MODEL_TYPE = BOOKMARKS, // Declared 2nd, for debugger prettiness. + FIRST_REAL_MODEL_TYPE = FIRST_USER_MODEL_TYPE, // A preference folder or a preference object. PREFERENCES, @@ -69,8 +70,6 @@ enum ModelType { TYPED_URLS, // An extension folder or an extension object. EXTENSIONS, - // An object representing a set of Nigori keys. - NIGORI, // An object representing a custom search engine. SEARCH_ENGINES, // An object representing a browser session. @@ -83,7 +82,14 @@ enum ModelType { EXTENSION_SETTINGS, // App notifications. APP_NOTIFICATIONS, - LAST_REAL_MODEL_TYPE = APP_NOTIFICATIONS, + LAST_USER_MODEL_TYPE = APP_NOTIFICATIONS, + + // An object representing a set of Nigori keys. + NIGORI, + FIRST_CONTROL_MODEL_TYPE = NIGORI, + LAST_CONTROL_MODEL_TYPE = NIGORI, + + LAST_REAL_MODEL_TYPE = LAST_CONTROL_MODEL_TYPE, // If you are adding a new sync datatype that is exposed to the user via the // sync preferences UI, be sure to update the list in @@ -124,6 +130,27 @@ SYNC_EXPORT ModelType GetModelTypeFromSpecifics( // value (sibling ordering) for this item. bool ShouldMaintainPosition(ModelType model_type); +// These are the user-selectable data types. Note that some of these share a +// preference flag, so not all of them are individually user-selectable. +ModelTypeSet UserTypes(); + +// Returns a list of all control types. +// +// The control types are intended to contain metadata nodes that are essential +// for the normal operation of the syncer. As such, they have the following +// special properties: +// - They are downloaded early during SyncBackend initialization. +// - They are always enabled. Users may not disable these types. +// - Their contents are not encrypted automatically. +// - They support custom update application and conflict resolution logic. +// - All change processing occurs on the sync thread (GROUP_PASSIVE). +ModelTypeSet ControlTypes(); + +// Returns true if this is a control type. +// +// See comment above for more information on what makes these types special. +bool IsControlType(ModelType model_type); + // Determine a model type from the field number of its associated // EntitySpecifics field. ModelType GetModelTypeFromSpecificsFieldNumber(int field_number); @@ -135,6 +162,8 @@ ModelType GetModelTypeFromSpecificsFieldNumber(int field_number); SYNC_EXPORT int GetSpecificsFieldNumberFromModelType( ModelType model_type); +FullModelTypeSet ToFullModelTypeSet(ModelTypeSet in); + // TODO(sync): The functions below badly need some cleanup. // Returns a pointer to a string with application lifetime that represents diff --git a/sync/internal_api/public/sync_encryption_handler.cc b/sync/internal_api/public/sync_encryption_handler.cc index d2b1ca2..e967600 100644 --- a/sync/internal_api/public/sync_encryption_handler.cc +++ b/sync/internal_api/public/sync_encryption_handler.cc @@ -14,11 +14,9 @@ SyncEncryptionHandler::~SyncEncryptionHandler() {} // Static. ModelTypeSet SyncEncryptionHandler::SensitiveTypes() { - // Both of these have their own encryption schemes, but we include them - // anyways. + // It has its own encryption scheme, but we include it anyway. ModelTypeSet types; types.Put(PASSWORDS); - types.Put(NIGORI); return types; } diff --git a/sync/internal_api/sync_encryption_handler_impl.cc b/sync/internal_api/sync_encryption_handler_impl.cc index 82149b1..7cd5be8 100644 --- a/sync/internal_api/sync_encryption_handler_impl.cc +++ b/sync/internal_api/sync_encryption_handler_impl.cc @@ -375,14 +375,14 @@ void SyncEncryptionHandlerImpl::EnableEncryptEverything() { ModelTypeSet* encrypted_types = &UnlockVaultMutable(trans.GetWrappedTrans())->encrypted_types; if (encrypt_everything_) { - DCHECK(encrypted_types->Equals(ModelTypeSet::All())); + DCHECK(encrypted_types->Equals(UserTypes())); return; } 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 = UserTypes(); FOR_EACH_OBSERVER( Observer, observers_, OnEncryptedTypesChanged(*encrypted_types, encrypt_everything_)); @@ -477,7 +477,7 @@ void SyncEncryptionHandlerImpl::ReEncryptEverything( for (ModelTypeSet::Iterator iter = UnlockVault(trans->GetWrappedTrans()).encrypted_types.First(); iter.Good(); iter.Inc()) { - if (iter.Get() == PASSWORDS || iter.Get() == NIGORI) + if (iter.Get() == PASSWORDS || IsControlType(iter.Get())) continue; // These types handle encryption differently. ReadNode type_root(trans); @@ -662,13 +662,13 @@ bool SyncEncryptionHandlerImpl::UpdateEncryptedTypesFromNigori( if (nigori.encrypt_everything()) { if (!encrypt_everything_) { encrypt_everything_ = true; - *encrypted_types = ModelTypeSet::All(); + *encrypted_types = UserTypes(); DVLOG(1) << "Enabling encrypt everything via nigori node update"; FOR_EACH_OBSERVER( Observer, observers_, OnEncryptedTypesChanged(*encrypted_types, encrypt_everything_)); } - DCHECK(encrypted_types->Equals(ModelTypeSet::All())); + DCHECK(encrypted_types->Equals(UserTypes())); return true; } @@ -683,12 +683,12 @@ bool SyncEncryptionHandlerImpl::UpdateEncryptedTypesFromNigori( !Difference(nigori_encrypted_types, SensitiveTypes()).Empty()) { if (!encrypt_everything_) { encrypt_everything_ = true; - *encrypted_types = ModelTypeSet::All(); + *encrypted_types = UserTypes(); FOR_EACH_OBSERVER( Observer, observers_, OnEncryptedTypesChanged(*encrypted_types, encrypt_everything_)); } - DCHECK(encrypted_types->Equals(ModelTypeSet::All())); + DCHECK(encrypted_types->Equals(UserTypes())); return false; } @@ -767,6 +767,10 @@ void SyncEncryptionHandlerImpl::MergeEncryptedTypes( ModelTypeSet new_encrypted_types, syncable::BaseTransaction* const trans) { DCHECK(thread_checker_.CalledOnValidThread()); + + // Only UserTypes may be encrypted. + DCHECK(UserTypes().HasAll(new_encrypted_types)); + ModelTypeSet* encrypted_types = &UnlockVaultMutable(trans)->encrypted_types; if (!encrypted_types->HasAll(new_encrypted_types)) { *encrypted_types = new_encrypted_types; diff --git a/sync/internal_api/sync_encryption_handler_impl_unittest.cc b/sync/internal_api/sync_encryption_handler_impl_unittest.cc index f1472c2..cc31a8e 100644 --- a/sync/internal_api/sync_encryption_handler_impl_unittest.cc +++ b/sync/internal_api/sync_encryption_handler_impl_unittest.cc @@ -155,13 +155,13 @@ TEST_F(SyncEncryptionHandlerImplTest, NigoriEncryptionTypes) { EXPECT_CALL(*observer(), OnEncryptedTypesChanged( - HasModelTypes(ModelTypeSet::All()), false)); + HasModelTypes(UserTypes()), false)); EXPECT_CALL(observer2, OnEncryptedTypesChanged( - HasModelTypes(ModelTypeSet::All()), false)); + HasModelTypes(UserTypes()), false)); // Set all encrypted types - encrypted_types = ModelTypeSet::All(); + encrypted_types = UserTypes(); { WriteTransaction trans(FROM_HERE, user_share()); encryption_handler()->MergeEncryptedTypes( @@ -192,24 +192,17 @@ TEST_F(SyncEncryptionHandlerImplTest, NigoriEncryptionTypes) { // Verify the encryption handler processes the encrypt everything field // properly. TEST_F(SyncEncryptionHandlerImplTest, EncryptEverythingExplicit) { - ModelTypeSet real_types = ModelTypeSet::All(); sync_pb::NigoriSpecifics nigori; nigori.set_encrypt_everything(true); EXPECT_CALL(*observer(), OnEncryptedTypesChanged( - HasModelTypes(ModelTypeSet::All()), true)); + HasModelTypes(UserTypes()), true)); EXPECT_FALSE(encryption_handler()->EncryptEverythingEnabled()); ModelTypeSet encrypted_types = encryption_handler()->GetEncryptedTypesUnsafe(); - for (ModelTypeSet::Iterator iter = real_types.First(); - iter.Good(); iter.Inc()) { - if (iter.Get() == PASSWORDS || iter.Get() == NIGORI) - EXPECT_TRUE(encrypted_types.Has(iter.Get())); - else - EXPECT_FALSE(encrypted_types.Has(iter.Get())); - } + EXPECT_TRUE(encrypted_types.Equals(ModelTypeSet(PASSWORDS))); { WriteTransaction trans(FROM_HERE, user_share()); @@ -220,10 +213,7 @@ TEST_F(SyncEncryptionHandlerImplTest, EncryptEverythingExplicit) { EXPECT_TRUE(encryption_handler()->EncryptEverythingEnabled()); encrypted_types = encryption_handler()->GetEncryptedTypesUnsafe(); - for (ModelTypeSet::Iterator iter = real_types.First(); - iter.Good(); iter.Inc()) { - EXPECT_TRUE(encrypted_types.Has(iter.Get())); - } + EXPECT_TRUE(encrypted_types.HasAll(UserTypes())); // Receiving the nigori node again shouldn't trigger another notification. Mock::VerifyAndClearExpectations(observer()); @@ -238,24 +228,17 @@ TEST_F(SyncEncryptionHandlerImplTest, EncryptEverythingExplicit) { // 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 nigori; nigori.set_encrypt_bookmarks(true); // Non-passwords = encrypt everything EXPECT_CALL(*observer(), OnEncryptedTypesChanged( - HasModelTypes(ModelTypeSet::All()), true)); + HasModelTypes(UserTypes()), true)); EXPECT_FALSE(encryption_handler()->EncryptEverythingEnabled()); ModelTypeSet encrypted_types = encryption_handler()->GetEncryptedTypesUnsafe(); - for (ModelTypeSet::Iterator iter = real_types.First(); - iter.Good(); iter.Inc()) { - if (iter.Get() == PASSWORDS || iter.Get() == NIGORI) - EXPECT_TRUE(encrypted_types.Has(iter.Get())); - else - EXPECT_FALSE(encrypted_types.Has(iter.Get())); - } + EXPECT_TRUE(encrypted_types.Equals(ModelTypeSet(PASSWORDS))); { WriteTransaction trans(FROM_HERE, user_share()); @@ -266,10 +249,7 @@ TEST_F(SyncEncryptionHandlerImplTest, EncryptEverythingImplicit) { EXPECT_TRUE(encryption_handler()->EncryptEverythingEnabled()); encrypted_types = encryption_handler()->GetEncryptedTypesUnsafe(); - for (ModelTypeSet::Iterator iter = real_types.First(); - iter.Good(); iter.Inc()) { - EXPECT_TRUE(encrypted_types.Has(iter.Get())); - } + EXPECT_TRUE(encrypted_types.HasAll(UserTypes())); // Receiving a nigori node with encrypt everything explicitly set shouldn't // trigger another notification. @@ -287,7 +267,6 @@ TEST_F(SyncEncryptionHandlerImplTest, EncryptEverythingImplicit) { // as Sensitive, and that it does not consider this an implicit encrypt // everything case. TEST_F(SyncEncryptionHandlerImplTest, UnknownSensitiveTypes) { - ModelTypeSet real_types = ModelTypeSet::All(); sync_pb::NigoriSpecifics nigori; nigori.set_encrypt_everything(false); nigori.set_encrypt_bookmarks(true); @@ -303,13 +282,7 @@ TEST_F(SyncEncryptionHandlerImplTest, UnknownSensitiveTypes) { EXPECT_FALSE(encryption_handler()->EncryptEverythingEnabled()); ModelTypeSet encrypted_types = encryption_handler()->GetEncryptedTypesUnsafe(); - for (ModelTypeSet::Iterator iter = real_types.First(); - iter.Good(); iter.Inc()) { - if (iter.Get() == PASSWORDS || iter.Get() == NIGORI) - EXPECT_TRUE(encrypted_types.Has(iter.Get())); - else - EXPECT_FALSE(encrypted_types.Has(iter.Get())); - } + EXPECT_TRUE(encrypted_types.Equals(ModelTypeSet(PASSWORDS))); { WriteTransaction trans(FROM_HERE, user_share()); @@ -320,15 +293,7 @@ TEST_F(SyncEncryptionHandlerImplTest, UnknownSensitiveTypes) { EXPECT_FALSE(encryption_handler()->EncryptEverythingEnabled()); encrypted_types = encryption_handler()->GetEncryptedTypesUnsafe(); - for (ModelTypeSet::Iterator iter = real_types.First(); - iter.Good(); iter.Inc()) { - if (iter.Get() == PASSWORDS || - iter.Get() == NIGORI || - iter.Get() == BOOKMARKS) - EXPECT_TRUE(encrypted_types.Has(iter.Get())); - else - EXPECT_FALSE(encrypted_types.Has(iter.Get())); - } + EXPECT_TRUE(encrypted_types.Equals(ModelTypeSet(BOOKMARKS, PASSWORDS))); } // Receive an old nigori with old encryption keys and encrypted types. We should @@ -348,7 +313,7 @@ TEST_F(SyncEncryptionHandlerImplTest, ReceiveOldNigori) { other_encrypted_specifics.mutable_encrypted()); sync_pb::EntitySpecifics our_encrypted_specifics; our_encrypted_specifics.mutable_bookmark()->set_title("title2"); - ModelTypeSet encrypted_types = ModelTypeSet::All(); + ModelTypeSet encrypted_types = UserTypes(); // Set up the current encryption state (containing both keys and encrypt // everything). @@ -364,7 +329,7 @@ TEST_F(SyncEncryptionHandlerImplTest, ReceiveOldNigori) { EXPECT_CALL(*observer(), OnCryptographerStateChanged(_)); EXPECT_CALL(*observer(), OnEncryptedTypesChanged( - HasModelTypes(ModelTypeSet::All()), true)); + HasModelTypes(UserTypes()), true)); { // Update the encryption handler. WriteTransaction trans(FROM_HERE, user_share()); diff --git a/sync/internal_api/sync_manager_impl_unittest.cc b/sync/internal_api/sync_manager_impl_unittest.cc index ec08939..9f5538e 100644 --- a/sync/internal_api/sync_manager_impl_unittest.cc +++ b/sync/internal_api/sync_manager_impl_unittest.cc @@ -1424,7 +1424,7 @@ TEST_F(SyncManagerTest, EncryptDataTypesWithNoData) { EXPECT_TRUE(SetUpEncryption(WRITE_TO_NIGORI, DEFAULT_ENCRYPTION)); EXPECT_CALL(encryption_observer_, OnEncryptedTypesChanged( - HasModelTypes(ModelTypeSet::All()), true)); + HasModelTypes(UserTypes()), true)); EXPECT_CALL(encryption_observer_, OnEncryptionComplete()); sync_manager_.GetEncryptionHandler()->EnableEncryptEverything(); EXPECT_TRUE(EncryptEverythingEnabledForTest()); @@ -1478,14 +1478,14 @@ TEST_F(SyncManagerTest, EncryptDataTypesWithData) { EXPECT_CALL(encryption_observer_, OnEncryptedTypesChanged( - HasModelTypes(ModelTypeSet::All()), true)); + HasModelTypes(UserTypes()), true)); EXPECT_CALL(encryption_observer_, OnEncryptionComplete()); sync_manager_.GetEncryptionHandler()->EnableEncryptEverything(); EXPECT_TRUE(EncryptEverythingEnabledForTest()); { ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare()); EXPECT_TRUE(GetEncryptedTypesWithTrans(&trans).Equals( - ModelTypeSet::All())); + UserTypes())); EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest( trans.GetWrappedTrans(), BOOKMARKS, @@ -1514,7 +1514,7 @@ TEST_F(SyncManagerTest, EncryptDataTypesWithData) { EXPECT_TRUE(EncryptEverythingEnabledForTest()); { ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare()); - EXPECT_TRUE(GetEncryptedTypesWithTrans(&trans).Equals(ModelTypeSet::All())); + EXPECT_TRUE(GetEncryptedTypesWithTrans(&trans).Equals(UserTypes())); EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest( trans.GetWrappedTrans(), BOOKMARKS, @@ -2007,14 +2007,14 @@ TEST_F(SyncManagerTest, EncryptBookmarksWithLegacyData) { EXPECT_CALL(encryption_observer_, OnEncryptedTypesChanged( - HasModelTypes(ModelTypeSet::All()), true)); + HasModelTypes(UserTypes()), true)); EXPECT_CALL(encryption_observer_, OnEncryptionComplete()); sync_manager_.GetEncryptionHandler()->EnableEncryptEverything(); EXPECT_TRUE(EncryptEverythingEnabledForTest()); { ReadTransaction trans(FROM_HERE, sync_manager_.GetUserShare()); - EXPECT_TRUE(GetEncryptedTypesWithTrans(&trans).Equals(ModelTypeSet::All())); + EXPECT_TRUE(GetEncryptedTypesWithTrans(&trans).Equals(UserTypes())); EXPECT_TRUE(syncable::VerifyDataTypeEncryptionForTest( trans.GetWrappedTrans(), BOOKMARKS, @@ -2094,7 +2094,7 @@ TEST_F(SyncManagerTest, UpdateEntryWithEncryption) { // Encrypt the datatatype, should set is_unsynced. EXPECT_CALL(encryption_observer_, OnEncryptedTypesChanged( - HasModelTypes(ModelTypeSet::All()), true)); + HasModelTypes(UserTypes()), true)); EXPECT_CALL(encryption_observer_, OnEncryptionComplete()); EXPECT_TRUE(SetUpEncryption(WRITE_TO_NIGORI, FULL_ENCRYPTION)); @@ -2419,7 +2419,7 @@ TEST_F(SyncManagerTest, SetBookmarkTitleWithEncryption) { // Encrypt the datatatype, should set is_unsynced. EXPECT_CALL(encryption_observer_, OnEncryptedTypesChanged( - HasModelTypes(ModelTypeSet::All()), true)); + HasModelTypes(UserTypes()), true)); EXPECT_CALL(encryption_observer_, OnEncryptionComplete()); EXPECT_TRUE(SetUpEncryption(WRITE_TO_NIGORI, FULL_ENCRYPTION)); EXPECT_CALL(encryption_observer_, OnCryptographerStateChanged(_)); @@ -2516,7 +2516,7 @@ TEST_F(SyncManagerTest, SetNonBookmarkTitleWithEncryption) { // Encrypt the datatatype, should set is_unsynced. EXPECT_CALL(encryption_observer_, OnEncryptedTypesChanged( - HasModelTypes(ModelTypeSet::All()), true)); + HasModelTypes(UserTypes()), true)); EXPECT_CALL(encryption_observer_, OnEncryptionComplete()); EXPECT_TRUE(SetUpEncryption(WRITE_TO_NIGORI, FULL_ENCRYPTION)); EXPECT_CALL(encryption_observer_, OnCryptographerStateChanged(_)); diff --git a/sync/sync.gyp b/sync/sync.gyp index 369605b..a5a9e80 100644 --- a/sync/sync.gyp +++ b/sync/sync.gyp @@ -73,6 +73,8 @@ 'internal_api/public/util/weak_handle.h', 'engine/all_status.cc', 'engine/all_status.h', + 'engine/apply_control_data_updates.cc', + 'engine/apply_control_data_updates.h', 'engine/apply_updates_command.cc', 'engine/apply_updates_command.h', 'engine/backoff_delay_provider.cc', @@ -83,6 +85,8 @@ 'engine/commit.h', 'engine/conflict_resolver.cc', 'engine/conflict_resolver.h', + 'engine/conflict_util.cc', + 'engine/conflict_util.h', 'engine/download_updates_command.cc', 'engine/download_updates_command.h', 'engine/get_commit_ids_command.cc', @@ -582,6 +586,7 @@ 'internal_api/public/base/model_type_state_map_unittest.cc', 'internal_api/public/engine/model_safe_worker_unittest.cc', 'internal_api/public/util/immutable_unittest.cc', + 'engine/apply_control_data_updates_unittest.cc', 'engine/apply_updates_command_unittest.cc', 'engine/backoff_delay_provider_unittest.cc', 'engine/build_commit_command_unittest.cc', @@ -590,10 +595,10 @@ 'engine/process_commit_response_command_unittest.cc', 'engine/process_updates_command_unittest.cc', 'engine/resolve_conflicts_command_unittest.cc', - 'engine/syncer_proto_util_unittest.cc', - 'engine/syncer_unittest.cc', 'engine/sync_scheduler_unittest.cc', 'engine/sync_scheduler_whitebox_unittest.cc', + 'engine/syncer_proto_util_unittest.cc', + 'engine/syncer_unittest.cc', 'engine/throttled_data_type_tracker_unittest.cc', 'engine/traffic_recorder_unittest.cc', 'engine/verify_updates_command_unittest.cc', diff --git a/sync/syncable/model_type.cc b/sync/syncable/model_type.cc index 83e803c..1674206 100644 --- a/sync/syncable/model_type.cc +++ b/sync/syncable/model_type.cc @@ -144,6 +144,14 @@ int GetSpecificsFieldNumberFromModelType(ModelType model_type) { return 0; } +FullModelTypeSet ToFullModelTypeSet(ModelTypeSet in) { + FullModelTypeSet out; + for (ModelTypeSet::Iterator i = in.First(); i.Good(); i.Inc()) { + out.Put(i.Get()); + } + return out; +} + // Note: keep this consistent with GetModelType in syncable.cc! ModelType GetModelType(const sync_pb::SyncEntity& sync_entity) { DCHECK(!IsRoot(sync_entity)); // Root shouldn't ever go over the wire. @@ -226,6 +234,26 @@ bool ShouldMaintainPosition(ModelType model_type) { return model_type == BOOKMARKS; } +ModelTypeSet UserTypes() { + ModelTypeSet set; + for (int i = FIRST_USER_MODEL_TYPE; i <= LAST_USER_MODEL_TYPE; ++i) { + set.Put(ModelTypeFromInt(i)); + } + return set; +} + +ModelTypeSet ControlTypes() { + ModelTypeSet set; + for (int i = FIRST_CONTROL_MODEL_TYPE; i <= LAST_CONTROL_MODEL_TYPE; ++i) { + set.Put(ModelTypeFromInt(i)); + } + return set; +} + +bool IsControlType(ModelType model_type) { + return ControlTypes().Has(model_type); +} + const char* ModelTypeToString(ModelType model_type) { // This is used in serialization routines as well as for displaying debug // information. Do not attempt to change these string values unless you know diff --git a/sync/syncable/nigori_util.cc b/sync/syncable/nigori_util.cc index 4b59516..70cf41b 100644 --- a/sync/syncable/nigori_util.cc +++ b/sync/syncable/nigori_util.cc @@ -70,7 +70,7 @@ bool EntryNeedsEncryption(ModelTypeSet encrypted_types, if (!entry.Get(UNIQUE_SERVER_TAG).empty()) return false; // We don't encrypt unique server nodes. ModelType type = entry.GetModelType(); - if (type == PASSWORDS || type == NIGORI) + if (type == PASSWORDS || IsControlType(type)) return false; // Checking NON_UNIQUE_NAME is not necessary for the correctness of encrypting // the data, nor for determining if data is encrypted. We simply ensure it has @@ -83,7 +83,7 @@ bool EntryNeedsEncryption(ModelTypeSet encrypted_types, bool SpecificsNeedsEncryption(ModelTypeSet encrypted_types, const sync_pb::EntitySpecifics& specifics) { const ModelType type = GetModelTypeFromSpecifics(specifics); - if (type == PASSWORDS || type == NIGORI) + if (type == PASSWORDS || IsControlType(type)) return false; // These types have their own encryption schemes. if (!encrypted_types.Has(type)) return false; // This type does not require encryption @@ -96,7 +96,7 @@ bool VerifyDataTypeEncryptionForTest( ModelType type, bool is_encrypted) { Cryptographer* cryptographer = trans->directory()->GetCryptographer(trans); - if (type == PASSWORDS || type == NIGORI) { + if (type == PASSWORDS || IsControlType(type)) { NOTREACHED(); return true; } |