diff options
-rw-r--r-- | chrome/browser/sync/engine/syncapi.cc | 2 | ||||
-rw-r--r-- | chrome/browser/sync/glue/sync_backend_host.cc | 33 | ||||
-rw-r--r-- | chrome/browser/sync/glue/sync_backend_host.h | 10 | ||||
-rw-r--r-- | chrome/browser/sync/profile_sync_service.cc | 70 | ||||
-rw-r--r-- | chrome/browser/sync/profile_sync_service.h | 11 | ||||
-rw-r--r-- | chrome/browser/sync/profile_sync_service_password_unittest.cc | 2 |
6 files changed, 85 insertions, 43 deletions
diff --git a/chrome/browser/sync/engine/syncapi.cc b/chrome/browser/sync/engine/syncapi.cc index f91d0f8..dabb926 100644 --- a/chrome/browser/sync/engine/syncapi.cc +++ b/chrome/browser/sync/engine/syncapi.cc @@ -1863,7 +1863,7 @@ void SyncManager::SyncInternal::HandleChannelEvent(const SyncerEvent& event) { sync_api::ReadTransaction trans(GetUserShare()); sync_api::ReadNode node(&trans); if (!node.InitByTagLookup(kNigoriTag)) { - NOTREACHED(); + DCHECK(!event.snapshot->is_share_usable); return; } const sync_pb::NigoriSpecifics& nigori = node.GetNigoriSpecifics(); diff --git a/chrome/browser/sync/glue/sync_backend_host.cc b/chrome/browser/sync/glue/sync_backend_host.cc index 74a8c47..05cbd7b 100644 --- a/chrome/browser/sync/glue/sync_backend_host.cc +++ b/chrome/browser/sync/glue/sync_backend_host.cc @@ -22,6 +22,8 @@ #include "chrome/browser/sync/glue/http_bridge.h" #include "chrome/browser/sync/glue/password_model_worker.h" #include "chrome/browser/sync/sessions/session_state.h" +// TODO(tim): Remove this! We should have a syncapi pass-thru instead. +#include "chrome/browser/sync/syncable/directory_manager.h" // Cryptographer. #include "chrome/common/chrome_switches.h" #include "chrome/common/chrome_version_info.h" #include "chrome/common/net/gaia/gaia_constants.h" @@ -115,12 +117,13 @@ void SyncBackendHost::Initialize( registrar_.routing_info[(*it)] = GROUP_PASSIVE; } - // TODO(tim): This should be encryption-specific instead of passwords - // specific. For now we have to do this to avoid NIGORI node lookups when - // we haven't downloaded that node. - if (profile_->GetPrefs()->GetBoolean(prefs::kSyncPasswords)) { + // TODO(tim): Remove this special case once NIGORI is populated by + // default. We piggy back off of the passwords flag for now to not + // require both encryption and passwords flags. + bool enable_encryption = CommandLine::ForCurrentProcess()->HasSwitch( + switches::kEnableSyncPasswords) || types.count(syncable::PASSWORDS); + if (enable_encryption) registrar_.routing_info[syncable::NIGORI] = GROUP_PASSIVE; - } InitCore(Core::DoInitializeOptions( sync_service_url, @@ -146,6 +149,20 @@ std::string SyncBackendHost::RestoreEncryptionBootstrapToken() { return token; } +bool SyncBackendHost::IsNigoriEnabled() const { + AutoLock lock(registrar_lock_); + // Note that NIGORI is only ever added/removed from routing_info once, + // during initialization / first configuration, so there is no real 'race' + // possible here or possibility of stale return value. + return registrar_.routing_info.find(syncable::NIGORI) != + registrar_.routing_info.end(); +} + +bool SyncBackendHost::IsCryptographerReady() const { + return syncapi_initialized_ && + GetUserShareHandle()->dir_manager->cryptographer()->is_ready(); +} + sync_api::HttpPostProviderFactory* SyncBackendHost::MakeHttpBridgeFactory( URLRequestContextGetter* getter) { return new HttpBridgeFactory(getter); @@ -170,6 +187,12 @@ void SyncBackendHost::StartSyncingWithServer() { } void SyncBackendHost::SetPassphrase(const std::string& passphrase) { + if (!IsNigoriEnabled()) { + LOG(WARNING) << "Silently dropping SetPassphrase request."; + return; + } + + // If encryption is enabled and we've got a SetPassphrase core_thread_.message_loop()->PostTask(FROM_HERE, NewRunnableMethod(core_.get(), &SyncBackendHost::Core::DoSetPassphrase, passphrase)); diff --git a/chrome/browser/sync/glue/sync_backend_host.h b/chrome/browser/sync/glue/sync_backend_host.h index d18bf26d..26d060a 100644 --- a/chrome/browser/sync/glue/sync_backend_host.h +++ b/chrome/browser/sync/glue/sync_backend_host.h @@ -191,6 +191,14 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { // ONLY CALL THIS IF OnInitializationComplete was called! bool HasUnsyncedItems() const; + // Whether or not we are syncing encryption keys. + bool IsNigoriEnabled() const; + + // True if the cryptographer has any keys available to attempt decryption. + // Could mean we've downloaded and loaded Nigori objects, or we bootstrapped + // using a token previously received. + bool IsCryptographerReady() const; + protected: // The real guts of SyncBackendHost, to keep the public client API clean. class Core : public base::RefCountedThreadSafe<SyncBackendHost::Core>, @@ -463,7 +471,7 @@ class SyncBackendHost : public browser_sync::ModelSafeWorkerRegistrar { // pointer value", and then invoke methods), because lifetimes are managed on // the UI thread. Of course, this comment only applies to ModelSafeWorker // impls that are themselves thread-safe, such as UIModelWorker. - Lock registrar_lock_; + mutable Lock registrar_lock_; // The frontend which we serve (and are owned by). SyncFrontend* frontend_; diff --git a/chrome/browser/sync/profile_sync_service.cc b/chrome/browser/sync/profile_sync_service.cc index 9788f2c..01f03bb 100644 --- a/chrome/browser/sync/profile_sync_service.cc +++ b/chrome/browser/sync/profile_sync_service.cc @@ -23,16 +23,13 @@ #include "chrome/browser/prefs/pref_service.h" #include "chrome/browser/profile.h" #include "chrome/browser/net/gaia/token_service.h" -#include "chrome/browser/sync/engine/syncapi.h" #include "chrome/browser/sync/glue/change_processor.h" #include "chrome/browser/sync/glue/data_type_controller.h" #include "chrome/browser/sync/glue/data_type_manager.h" #include "chrome/browser/sync/glue/session_data_type_controller.h" #include "chrome/browser/sync/profile_sync_factory.h" #include "chrome/browser/sync/signin_manager.h" -#include "chrome/browser/sync/syncable/directory_manager.h" #include "chrome/browser/sync/token_migrator.h" -#include "chrome/browser/sync/util/user_settings.h" #include "chrome/common/chrome_switches.h" #include "chrome/common/net/gaia/gaia_constants.h" #include "chrome/common/notification_details.h" @@ -73,6 +70,7 @@ ProfileSyncService::ProfileSyncService(ProfileSyncFactory* factory, unrecoverable_error_detected_(false), ALLOW_THIS_IN_INITIALIZER_LIST(scoped_runnable_method_factory_(this)), token_migrator_(NULL), + observed_passphrase_required_(false), clear_server_data_state_(CLEAR_NOT_STARTED) { DCHECK(factory); DCHECK(profile); @@ -307,7 +305,7 @@ void ProfileSyncService::RegisterPreferences() { #endif pref_service->RegisterBooleanPref(prefs::kSyncBookmarks, true); - pref_service->RegisterBooleanPref(prefs::kSyncPasswords, false); + pref_service->RegisterBooleanPref(prefs::kSyncPasswords, enable_by_default); pref_service->RegisterBooleanPref(prefs::kSyncPreferences, enable_by_default); pref_service->RegisterBooleanPref(prefs::kSyncAutofill, enable_by_default); pref_service->RegisterBooleanPref(prefs::kSyncThemes, enable_by_default); @@ -408,26 +406,40 @@ void ProfileSyncService::StartUp() { void ProfileSyncService::Shutdown(bool sync_disabled) { // Stop all data type controllers, if needed. - if (data_type_manager_.get() && - data_type_manager_->state() != DataTypeManager::STOPPED) { - data_type_manager_->Stop(); - } + if (data_type_manager_.get()) { + if (data_type_manager_->state() != DataTypeManager::STOPPED) { + data_type_manager_->Stop(); + } - data_type_manager_.reset(); + registrar_.Remove(this, + NotificationType::SYNC_CONFIGURE_START, + Source<DataTypeManager>(data_type_manager_.get())); + registrar_.Remove(this, + NotificationType::SYNC_CONFIGURE_DONE, + Source<DataTypeManager>(data_type_manager_.get())); + data_type_manager_.reset(); + } // Move aside the backend so nobody else tries to use it while we are // shutting it down. scoped_ptr<SyncBackendHost> doomed_backend(backend_.release()); - - if (doomed_backend.get()) + if (doomed_backend.get()) { doomed_backend->Shutdown(sync_disabled); - doomed_backend.reset(); + registrar_.Remove(this, + NotificationType::SYNC_PASSPHRASE_REQUIRED, + Source<SyncBackendHost>(doomed_backend.get())); + registrar_.Remove(this, + NotificationType::SYNC_PASSPHRASE_ACCEPTED, + Source<SyncBackendHost>(doomed_backend.get())); + doomed_backend.reset(); + } // Clear various flags. is_auth_in_progress_ = false; backend_initialized_ = false; + observed_passphrase_required_ = false; last_attempted_user_email_.clear(); } @@ -819,24 +831,7 @@ void ProfileSyncService::GetRegisteredDataTypes( } bool ProfileSyncService::IsCryptographerReady() const { - return backend_.get() && backend_initialized_ && - backend_->GetUserShareHandle()->dir_manager->cryptographer()->is_ready(); -} - -void ProfileSyncService::SetPassphrase(const std::string& passphrase) { - // TODO(tim): This should be encryption-specific instead of passwords - // specific. For now we have to do this to avoid NIGORI node lookups when - // we haven't downloaded that node. - if (!profile_->GetPrefs()->GetBoolean(prefs::kSyncPasswords)) { - LOG(WARNING) << "Silently dropping SetPassphrase request."; - return; - } - - if (!sync_initialized()) { - cached_passphrase_ = passphrase; - } else { - backend_->SetPassphrase(passphrase); - } + return backend_.get() && backend_->IsCryptographerReady(); } void ProfileSyncService::ConfigureDataTypeManager() { @@ -877,6 +872,14 @@ void ProfileSyncService::DeactivateDataType( backend_->DeactivateDataType(data_type_controller, change_processor); } +void ProfileSyncService::SetPassphrase(const std::string& passphrase) { + if (ShouldPushChanges() || observed_passphrase_required_) { + backend_->SetPassphrase(passphrase); + } else { + cached_passphrase_ = passphrase; + } +} + void ProfileSyncService::Observe(NotificationType type, const NotificationSource& source, const NotificationDetails& details) { @@ -913,6 +916,9 @@ void ProfileSyncService::Observe(NotificationType type, } case NotificationType::SYNC_PASSPHRASE_REQUIRED: { DCHECK(backend_.get()); + DCHECK(backend_->IsNigoriEnabled()); + observed_passphrase_required_ = true; + if (!cached_passphrase_.empty()) { SetPassphrase(cached_passphrase_); cached_passphrase_.clear(); @@ -955,8 +961,8 @@ void ProfileSyncService::Observe(NotificationType type, break; } case NotificationType::GOOGLE_SIGNIN_SUCCESSFUL: { - if (!profile_->GetPrefs()->GetBoolean( - prefs::kSyncUsingSecondaryPassphrase)) { + PrefService* prefs = profile_->GetPrefs(); + if (!prefs->GetBoolean(prefs::kSyncUsingSecondaryPassphrase)) { const GoogleServiceSigninSuccessDetails* successful = (Details<const GoogleServiceSigninSuccessDetails>(details).ptr()); SetPassphrase(successful->password); diff --git a/chrome/browser/sync/profile_sync_service.h b/chrome/browser/sync/profile_sync_service.h index dc3e67d..f0a3db2 100644 --- a/chrome/browser/sync/profile_sync_service.h +++ b/chrome/browser/sync/profile_sync_service.h @@ -328,9 +328,10 @@ class ProfileSyncService : public browser_sync::SyncFrontend, // for sensitive data types. virtual bool IsCryptographerReady() const; - // Sets the Cryptographer's passphrase. This will check asynchronously whether - // the passphrase is valid and notify ProfileSyncServiceObservers via the - // NotificationService when the outcome is known. + // Sets the Cryptographer's passphrase, or caches it until that is possible.
+ // This will check asynchronously whether the passphrase is valid and notify
+ // ProfileSyncServiceObservers via the NotificationService when the outcome + // is known. virtual void SetPassphrase(const std::string& passphrase); // Returns whether processing changes is allowed. Check this before doing @@ -484,6 +485,10 @@ class ProfileSyncService : public browser_sync::SyncFrontend, // credentials were provided. std::string cached_passphrase_; + // Whether we have seen a SYNC_PASSPHRASE_REQUIRED since initializing the + // backend, telling us that it is safe to send a passphrase down ASAP. + bool observed_passphrase_required_; + // Keep track of where we are in a server clear operation ClearServerDataState clear_server_data_state_; diff --git a/chrome/browser/sync/profile_sync_service_password_unittest.cc b/chrome/browser/sync/profile_sync_service_password_unittest.cc index d0598c5..a355c60 100644 --- a/chrome/browser/sync/profile_sync_service_password_unittest.cc +++ b/chrome/browser/sync/profile_sync_service_password_unittest.cc @@ -144,7 +144,7 @@ class ProfileSyncServicePasswordTest : public AbstractProfileSyncServiceTest { service_.reset(new TestProfileSyncService(&factory_, &profile_, "test_user", false, root_task)); service_->RegisterPreferences(); - profile_.GetPrefs()->SetBoolean(prefs::kSyncPasswords, true); + profile_.GetPrefs()->SetBoolean(prefs::kSyncPasswords, true); service_->set_num_expected_resumes(num_resume_expectations); service_->set_num_expected_pauses(num_pause_expectations); PasswordDataTypeController* data_type_controller = |