From dc4d0153b235bff62506f5a8a9261e2b66d00a19 Mon Sep 17 00:00:00 2001 From: "thestig@chromium.org" Date: Tue, 5 Aug 2014 02:19:35 +0000 Subject: Revert of sync: Add non-blocking type encryption support (https://codereview.chromium.org/423193002/) Reason for revert: Both LSAN and Valgrind complains about leaks in the tests. Original issue's description: > sync: Add non-blocking type encryption support > > Introduces the framework for dealing with sync encryption in > non-blocking types. Unlike directory sync types, non-blocking type > encryption only encrypts data before it is sent to the server. > Encrypting the data on-disk is a separate problem. > > Adds code to the ModelTypeSyncWorker so it can access the directory's > cryptographer (through a CryptographerProvider interface) and use it to > encrypt entities before it sends them to the server. If the > cryptographer is unable to encrypt with the desired key, the worker will > not commit until the cryptographer returns to a good state. > > Adds the concept of a "desired encryption key" to the data type state. > When the cryptographer key to be used to encrypt a type changes, this > will be reflected in the data type state. The ModelTypeSyncProxy is > responsible for ensuring that all items which have not yet been > encrypted with this desired key are enqueued for commit. > > Makes the ModelTypeSyncWorker, EntityTracker, and ModelTypeSyncProxy > collaborate on the management of undecryptable (inapplicable) updates. > The EntityTracker keeps track of their version numbers and content, and > prevents the committing of new items to the server until the > inapplicable update has been dealt with. The ModelTypeSyncProxy is > responsible for saving inapplicable updates across restarts. > > This CL alone is not enough to enable encryption support for > non-blocking types. It requires additional code to hook up the > ModelTypeSyncWorkers to receive cryptographer events. This will be > added in a future commit. In the meantime, this CL includes plenty > of unit tests to verify the functionality that's being added. > > BUG=351005 > > Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287428 TBR=zea@chromium.org,stanisc@chromium.org,rlarocque@chromium.org NOTREECHECKS=true NOTRY=true BUG=351005 Review URL: https://codereview.chromium.org/442623002 git-svn-id: svn://svn.chromium.org/chrome/trunk/src@287433 0039d316-1c4b-4281-b951-d872f2087c98 --- sync/engine/entity_tracker.cc | 35 +- sync/engine/entity_tracker.h | 21 - sync/engine/model_type_entity.cc | 36 +- sync/engine/model_type_entity.h | 21 +- sync/engine/model_type_entity_unittest.cc | 18 +- sync/engine/model_type_sync_proxy.h | 3 +- sync/engine/model_type_sync_proxy_impl.cc | 83 +--- sync/engine/model_type_sync_proxy_impl.h | 18 +- sync/engine/model_type_sync_proxy_impl_unittest.cc | 234 +--------- sync/engine/model_type_sync_worker_impl.cc | 209 +-------- sync/engine/model_type_sync_worker_impl.h | 48 +- .../engine/model_type_sync_worker_impl_unittest.cc | 493 +-------------------- 12 files changed, 70 insertions(+), 1149 deletions(-) (limited to 'sync/engine') diff --git a/sync/engine/entity_tracker.cc b/sync/engine/entity_tracker.cc index bad76f9..0f10906 100644 --- a/sync/engine/entity_tracker.cc +++ b/sync/engine/entity_tracker.cc @@ -193,14 +193,8 @@ void EntityTracker::ReceiveCommitResponse(const std::string& response_id, } void EntityTracker::ReceiveUpdate(int64 version) { - if (version <= highest_gu_response_version_) - return; - - highest_gu_response_version_ = version; - - // Got an applicable update newer than any pending updates. It must be safe - // to discard the old pending update, if there was one. - ClearPendingUpdate(); + highest_gu_response_version_ = + std::max(highest_gu_response_version_, version); if (IsInConflict()) { // Incoming update clobbers the pending commit on the sync thread. @@ -209,35 +203,10 @@ void EntityTracker::ReceiveUpdate(int64 version) { } } -bool EntityTracker::ReceivePendingUpdate(const UpdateResponseData& data) { - if (data.response_version < highest_gu_response_version_) - return false; - - highest_gu_response_version_ = data.response_version; - pending_update_.reset(new UpdateResponseData(data)); - ClearPendingCommit(); - return true; -} - -bool EntityTracker::HasPendingUpdate() const { - return !!pending_update_; -} - -UpdateResponseData EntityTracker::GetPendingUpdate() const { - return *pending_update_; -} - -void EntityTracker::ClearPendingUpdate() { - pending_update_.reset(); -} - bool EntityTracker::IsInConflict() const { if (!is_commit_pending_) return false; - if (HasPendingUpdate()) - return true; - if (highest_gu_response_version_ <= highest_commit_response_version_) { // The most recent server state was created in a commit made by this // client. We're fully up to date, and therefore not in conflict. diff --git a/sync/engine/entity_tracker.h b/sync/engine/entity_tracker.h index e969ff9..db2d09b 100644 --- a/sync/engine/entity_tracker.h +++ b/sync/engine/entity_tracker.h @@ -8,10 +8,8 @@ #include #include "base/basictypes.h" -#include "base/memory/scoped_ptr.h" #include "base/time/time.h" #include "sync/base/sync_export.h" -#include "sync/internal_api/public/non_blocking_sync_common.h" #include "sync/protocol/sync.pb.h" namespace syncer { @@ -83,20 +81,6 @@ class SYNC_EXPORT EntityTracker { // Handles receipt of an update from the server. void ReceiveUpdate(int64 version); - // Handles the receipt of an pending update from the server. - // - // Returns true if the tracker decides this item is worth keeping. Returns - // false if the item is discarded, which could happen if the version number - // is out of date. - bool ReceivePendingUpdate(const UpdateResponseData& data); - - // Functions to fetch the latest pending update. - bool HasPendingUpdate() const; - UpdateResponseData GetPendingUpdate() const; - - // Clears the pending update. Allows us to resume regular commit behavior. - void ClearPendingUpdate(); - private: // Initializes received update state. Does not initialize state related to // pending commits and sets |is_commit_pending_| to false. @@ -162,11 +146,6 @@ class SYNC_EXPORT EntityTracker { bool deleted_; sync_pb::EntitySpecifics specifics_; - // An update for this item which can't be applied right now. The presence of - // an pending update prevents commits. As of this writing, the only source - // of pending updates is updates we can't decrypt right now. - scoped_ptr pending_update_; - DISALLOW_COPY_AND_ASSIGN(EntityTracker); }; diff --git a/sync/engine/model_type_entity.cc b/sync/engine/model_type_entity.cc index 2f758a8..5acf5db 100644 --- a/sync/engine/model_type_entity.cc +++ b/sync/engine/model_type_entity.cc @@ -24,8 +24,7 @@ scoped_ptr ModelTypeEntity::NewLocalItem( specifics, false, now, - now, - std::string())); + now)); } scoped_ptr ModelTypeEntity::FromServerUpdate( @@ -36,8 +35,7 @@ scoped_ptr ModelTypeEntity::FromServerUpdate( const sync_pb::EntitySpecifics& specifics, bool deleted, base::Time ctime, - base::Time mtime, - const std::string& encryption_key_name) { + base::Time mtime) { return scoped_ptr(new ModelTypeEntity(0, 0, 0, @@ -49,8 +47,7 @@ scoped_ptr ModelTypeEntity::FromServerUpdate( specifics, deleted, ctime, - mtime, - encryption_key_name)); + mtime)); } ModelTypeEntity::ModelTypeEntity(int64 sequence_number, @@ -64,8 +61,7 @@ ModelTypeEntity::ModelTypeEntity(int64 sequence_number, const sync_pb::EntitySpecifics& specifics, bool deleted, base::Time ctime, - base::Time mtime, - const std::string& encryption_key_name) + base::Time mtime) : sequence_number_(sequence_number), commit_requested_sequence_number_(commit_requested_sequence_number), acked_sequence_number_(acked_sequence_number), @@ -77,8 +73,7 @@ ModelTypeEntity::ModelTypeEntity(int64 sequence_number, specifics_(specifics), deleted_(deleted), ctime_(ctime), - mtime_(mtime), - encryption_key_name_(encryption_key_name) { + mtime_(mtime) { } ModelTypeEntity::~ModelTypeEntity() { @@ -108,8 +103,7 @@ void ModelTypeEntity::ApplyUpdateFromServer( int64 update_version, bool deleted, const sync_pb::EntitySpecifics& specifics, - base::Time mtime, - const std::string& encryption_key_name) { + base::Time mtime) { // There was a conflict and the server just won it. // This implicitly acks all outstanding commits because a received update // will clobber any pending commits on the sync thread. @@ -127,15 +121,6 @@ void ModelTypeEntity::MakeLocalChange( specifics_ = specifics; } -void ModelTypeEntity::UpdateDesiredEncryptionKey(const std::string& name) { - if (encryption_key_name_ == name) - return; - - // Schedule commit with the expectation that the worker will re-encrypt with - // the latest encryption key as it does. - sequence_number_++; -} - void ModelTypeEntity::Delete() { sequence_number_++; specifics_.Clear(); @@ -159,15 +144,12 @@ void ModelTypeEntity::SetCommitRequestInProgress() { commit_requested_sequence_number_ = sequence_number_; } -void ModelTypeEntity::ReceiveCommitResponse( - const std::string& id, - int64 sequence_number, - int64 response_version, - const std::string& encryption_key_name) { +void ModelTypeEntity::ReceiveCommitResponse(const std::string& id, + int64 sequence_number, + int64 response_version) { id_ = id; // The server can assign us a new ID in a commit response. acked_sequence_number_ = sequence_number; base_version_ = response_version; - encryption_key_name_ = encryption_key_name; } void ModelTypeEntity::ClearTransientSyncState() { diff --git a/sync/engine/model_type_entity.h b/sync/engine/model_type_entity.h index e05d469..abc67a9 100644 --- a/sync/engine/model_type_entity.h +++ b/sync/engine/model_type_entity.h @@ -46,8 +46,7 @@ class SYNC_EXPORT_PRIVATE ModelTypeEntity { const sync_pb::EntitySpecifics& specifics, bool deleted, base::Time ctime, - base::Time mtime, - const std::string& encryption_key_name); + base::Time mtime); // TODO(rlarocque): Implement FromDisk constructor when we implement storage. @@ -80,17 +79,11 @@ class SYNC_EXPORT_PRIVATE ModelTypeEntity { void ApplyUpdateFromServer(int64 update_version, bool deleted, const sync_pb::EntitySpecifics& specifics, - base::Time mtime, - const std::string& encryption_key_name); + base::Time mtime); // Applies a local change to this item. void MakeLocalChange(const sync_pb::EntitySpecifics& specifics); - // Schedule a commit if the |name| does not match this item's last known - // encryption key. The worker that performs the commit is expected to - // encrypt the item using the latest available key. - void UpdateDesiredEncryptionKey(const std::string& name); - // Applies a local deletion to this item. void Delete(); @@ -111,8 +104,7 @@ class SYNC_EXPORT_PRIVATE ModelTypeEntity { // reached the server. void ReceiveCommitResponse(const std::string& id, int64 sequence_number, - int64 response_version, - const std::string& encryption_key_name); + int64 response_version); // Clears any in-memory sync state associated with outstanding commits. void ClearTransientSyncState(); @@ -132,8 +124,7 @@ class SYNC_EXPORT_PRIVATE ModelTypeEntity { const sync_pb::EntitySpecifics& specifics, bool deleted, base::Time ctime, - base::Time mtime, - const std::string& encryption_key_name); + base::Time mtime); // A sequence number used to track in-progress commits. Each local change // increments this number. @@ -194,10 +185,6 @@ class SYNC_EXPORT_PRIVATE ModelTypeEntity { // doesn't bother to inspect their values. base::Time ctime_; base::Time mtime_; - - // The name of the encryption key used to encrypt this item on the server. - // Empty when no encryption is in use. - std::string encryption_key_name_; }; } // namespace syncer diff --git a/sync/engine/model_type_entity_unittest.cc b/sync/engine/model_type_entity_unittest.cc index 817bc84..c605732 100644 --- a/sync/engine/model_type_entity_unittest.cc +++ b/sync/engine/model_type_entity_unittest.cc @@ -64,8 +64,7 @@ TEST_F(ModelTypeEntityTest, FromServerUpdate) { specifics, false, kCtime, - kMtime, - std::string())); + kMtime)); EXPECT_TRUE(entity->IsWriteRequired()); EXPECT_FALSE(entity->IsUnsynced()); @@ -88,8 +87,7 @@ TEST_F(ModelTypeEntityTest, TombstoneUpdate) { sync_pb::EntitySpecifics(), true, kCtime, - kMtime, - std::string())); + kMtime)); EXPECT_TRUE(entity->IsWriteRequired()); EXPECT_FALSE(entity->IsUnsynced()); @@ -109,15 +107,13 @@ TEST_F(ModelTypeEntityTest, ApplyUpdate) { specifics, false, kCtime, - kMtime, - std::string())); + kMtime)); // A deletion update one version later. entity->ApplyUpdateFromServer(11, true, sync_pb::EntitySpecifics(), - kMtime + base::TimeDelta::FromSeconds(10), - std::string()); + kMtime + base::TimeDelta::FromSeconds(10)); EXPECT_TRUE(entity->IsWriteRequired()); EXPECT_FALSE(entity->IsUnsynced()); @@ -134,8 +130,7 @@ TEST_F(ModelTypeEntityTest, LocalChange) { specifics, false, kCtime, - kMtime, - std::string())); + kMtime)); sync_pb::EntitySpecifics specifics2; specifics2.CopyFrom(specifics); @@ -161,8 +156,7 @@ TEST_F(ModelTypeEntityTest, LocalDeletion) { specifics, false, kCtime, - kMtime, - std::string())); + kMtime)); entity->Delete(); diff --git a/sync/engine/model_type_sync_proxy.h b/sync/engine/model_type_sync_proxy.h index 3c6e079..706fbc7 100644 --- a/sync/engine/model_type_sync_proxy.h +++ b/sync/engine/model_type_sync_proxy.h @@ -21,8 +21,7 @@ class SYNC_EXPORT_PRIVATE ModelTypeSyncProxy { const CommitResponseDataList& response_list) = 0; virtual void OnUpdateReceived( const DataTypeState& type_state, - const UpdateResponseDataList& response_list, - const UpdateResponseDataList& pending_updates) = 0; + const UpdateResponseDataList& response_list) = 0; }; } // namespace syncer diff --git a/sync/engine/model_type_sync_proxy_impl.cc b/sync/engine/model_type_sync_proxy_impl.cc index 7ea5d52..7b2148a 100644 --- a/sync/engine/model_type_sync_proxy_impl.cc +++ b/sync/engine/model_type_sync_proxy_impl.cc @@ -18,7 +18,6 @@ ModelTypeSyncProxyImpl::ModelTypeSyncProxyImpl(ModelType type) is_preferred_(false), is_connected_(false), entities_deleter_(&entities_), - pending_updates_map_deleter_(&pending_updates_map_), weak_ptr_factory_for_ui_(this), weak_ptr_factory_for_sync_(this) { } @@ -52,12 +51,10 @@ void ModelTypeSyncProxyImpl::Enable( data_type_state_.progress_marker.set_data_type_id( GetSpecificsFieldNumberFromModelType(type_)); - UpdateResponseDataList saved_pending_updates = GetPendingUpdates(); sync_context_proxy_ = sync_context_proxy.Pass(); sync_context_proxy_->ConnectTypeToSync( GetModelType(), data_type_state_, - saved_pending_updates, weak_ptr_factory_for_sync_.GetWeakPtr()); } @@ -183,18 +180,16 @@ void ModelTypeSyncProxyImpl::OnCommitCompleted( } else { it->second->ReceiveCommitResponse(response_data.id, response_data.sequence_number, - response_data.response_version, - data_type_state_.encryption_key_name); + response_data.response_version); } } } void ModelTypeSyncProxyImpl::OnUpdateReceived( const DataTypeState& data_type_state, - const UpdateResponseDataList& response_list, - const UpdateResponseDataList& pending_updates) { - bool got_new_encryption_requirements = data_type_state_.encryption_key_name != - data_type_state.encryption_key_name; + const UpdateResponseDataList& response_list) { + bool initial_sync_just_finished = + !data_type_state_.initial_sync_done && data_type_state.initial_sync_done; data_type_state_ = data_type_state; @@ -204,14 +199,6 @@ void ModelTypeSyncProxyImpl::OnUpdateReceived( const UpdateResponseData& response_data = *list_it; const std::string& client_tag_hash = response_data.client_tag_hash; - UpdateMap::iterator old_it = pending_updates_map_.find(client_tag_hash); - if (old_it != pending_updates_map_.end()) { - // If we're being asked to apply an update to this entity, this overrides - // the previous pending updates. - delete old_it->second; - pending_updates_map_.erase(old_it); - } - EntityMap::iterator it = entities_.find(client_tag_hash); if (it == entities_.end()) { scoped_ptr entity = @@ -222,74 +209,22 @@ void ModelTypeSyncProxyImpl::OnUpdateReceived( response_data.specifics, response_data.deleted, response_data.ctime, - response_data.mtime, - response_data.encryption_key_name); + response_data.mtime); entities_.insert(std::make_pair(client_tag_hash, entity.release())); } else { ModelTypeEntity* entity = it->second; entity->ApplyUpdateFromServer(response_data.response_version, response_data.deleted, response_data.specifics, - response_data.mtime, - response_data.encryption_key_name); - + response_data.mtime); // TODO: Do something special when conflicts are detected. } - - // If the received entity has out of date encryption, we schedule another - // commit to fix it. - if (data_type_state_.encryption_key_name != - response_data.encryption_key_name) { - EntityMap::iterator it2 = entities_.find(client_tag_hash); - it2->second->UpdateDesiredEncryptionKey( - data_type_state_.encryption_key_name); - } } - // Save pending updates in the appropriate data structure. - for (UpdateResponseDataList::const_iterator list_it = pending_updates.begin(); - list_it != pending_updates.end(); - ++list_it) { - const UpdateResponseData& update = *list_it; - const std::string& client_tag_hash = update.client_tag_hash; - - UpdateMap::iterator lookup_it = pending_updates_map_.find(client_tag_hash); - if (lookup_it == pending_updates_map_.end()) { - pending_updates_map_.insert( - std::make_pair(client_tag_hash, new UpdateResponseData(update))); - } else if (lookup_it->second->response_version <= update.response_version) { - delete lookup_it->second; - pending_updates_map_.erase(lookup_it); - pending_updates_map_.insert( - std::make_pair(client_tag_hash, new UpdateResponseData(update))); - } else { - // Received update is stale, do not overwrite existing. - } - } - - if (got_new_encryption_requirements) { - for (EntityMap::iterator it = entities_.begin(); it != entities_.end(); - ++it) { - it->second->UpdateDesiredEncryptionKey( - data_type_state_.encryption_key_name); - } - } - - // We may have new reasons to commit by the time this function is done. - FlushPendingCommitRequests(); + if (initial_sync_just_finished) + FlushPendingCommitRequests(); // TODO: Inform the model of the new or updated data. - // TODO: Persist the new data on disk. -} - -UpdateResponseDataList ModelTypeSyncProxyImpl::GetPendingUpdates() { - UpdateResponseDataList pending_updates_list; - for (UpdateMap::const_iterator it = pending_updates_map_.begin(); - it != pending_updates_map_.end(); - ++it) { - pending_updates_list.push_back(*it->second); - } - return pending_updates_list; } void ModelTypeSyncProxyImpl::ClearTransientSyncState() { @@ -304,7 +239,7 @@ void ModelTypeSyncProxyImpl::ClearSyncState() { ++it) { it->second->ClearSyncState(); } - STLDeleteValues(&pending_updates_map_); + data_type_state_ = DataTypeState(); } diff --git a/sync/engine/model_type_sync_proxy_impl.h b/sync/engine/model_type_sync_proxy_impl.h index f160669..2390eda 100644 --- a/sync/engine/model_type_sync_proxy_impl.h +++ b/sync/engine/model_type_sync_proxy_impl.h @@ -73,16 +73,7 @@ class SYNC_EXPORT_PRIVATE ModelTypeSyncProxyImpl : base::NonThreadSafe { // Informs this object that there are some incoming updates is should // handle. void OnUpdateReceived(const DataTypeState& type_state, - const UpdateResponseDataList& response_list, - const UpdateResponseDataList& pending_updates); - - // Returns the list of pending updates. - // - // This is used as a helper function, but it's public mainly for testing. - // The current test harness setup doesn't allow us to test the data that the - // proxy sends to the worker during initialization, so we use this to inspect - // its state instead. - UpdateResponseDataList GetPendingUpdates(); + const UpdateResponseDataList& response_list); // Returns the long-lived WeakPtr that is intended to be registered with the // ProfileSyncService. @@ -90,7 +81,6 @@ class SYNC_EXPORT_PRIVATE ModelTypeSyncProxyImpl : base::NonThreadSafe { private: typedef std::map EntityMap; - typedef std::map UpdateMap; // Sends all commit requests that are due to be sent to the sync thread. void FlushPendingCommitRequests(); @@ -133,12 +123,6 @@ class SYNC_EXPORT_PRIVATE ModelTypeSyncProxyImpl : base::NonThreadSafe { EntityMap entities_; STLValueDeleter entities_deleter_; - // A set of updates that can not be applied at this time. These are never - // used by the model. They are kept here only so we can save and restore - // them across restarts, and keep them in sync with our progress markers. - UpdateMap pending_updates_map_; - STLValueDeleter pending_updates_map_deleter_; - // We use two different WeakPtrFactories because we want the pointers they // issue to have different lifetimes. When asked to disconnect from the sync // thread, we want to make sure that no tasks generated as part of the diff --git a/sync/engine/model_type_sync_proxy_impl_unittest.cc b/sync/engine/model_type_sync_proxy_impl_unittest.cc index 581d511..0597657 100644 --- a/sync/engine/model_type_sync_proxy_impl_unittest.cc +++ b/sync/engine/model_type_sync_proxy_impl_unittest.cc @@ -77,22 +77,6 @@ class ModelTypeSyncProxyImplTest : public ::testing::Test { const std::string& value); void TombstoneFromServer(int64 version_offset, const std::string& tag); - // Emulate the receipt of pending updates from the server. - // Pending updates are usually caused by a temporary decryption failure. - void PendingUpdateFromServer(int64 version_offset, - const std::string& tag, - const std::string& value, - const std::string& key_name); - - // Returns true if the proxy has an pending update with specified tag. - bool HasPendingUpdate(const std::string& tag) const; - - // Returns the pending update with the specified tag. - UpdateResponseData GetPendingUpdate(const std::string& tag) const; - - // Returns the number of pending updates. - size_t GetNumPendingUpdates() const; - // Read emitted commit requests as batches. size_t GetNumCommitRequestLists(); CommitRequestDataList GetNthCommitRequestList(size_t n); @@ -104,22 +88,10 @@ class ModelTypeSyncProxyImplTest : public ::testing::Test { // Sends the type sync proxy a successful commit response. void SuccessfulCommitResponse(const CommitRequestData& request_data); - // Sends the type sync proxy an updated DataTypeState to let it know that - // the desired encryption key has changed. - void UpdateDesiredEncryptionKey(const std::string& key_name); - - // Sets the key_name that the mock ModelTypeSyncWorker will claim is in use - // when receiving items. - void SetServerEncryptionKey(const std::string& key_name); - private: static std::string GenerateTagHash(const std::string& tag); static sync_pb::EntitySpecifics GenerateSpecifics(const std::string& tag, const std::string& value); - static sync_pb::EntitySpecifics GenerateEncryptedSpecifics( - const std::string& tag, - const std::string& value, - const std::string& key_name); int64 GetServerVersion(const std::string& tag); void SetServerVersion(const std::string& tag, int64 version); @@ -169,7 +141,7 @@ void ModelTypeSyncProxyImplTest::Disable() { void ModelTypeSyncProxyImplTest::ReEnable() { DCHECK(!type_sync_proxy_->IsConnected()); - // Prepare a new MockModelTypeSyncWorker instance, just as we would + // Prepare a new NonBlockingTypeProcesorCore instance, just as we would // if this happened in the real world. mock_worker_ = new MockModelTypeSyncWorker(); injectable_sync_context_proxy_.reset( @@ -193,8 +165,7 @@ void ModelTypeSyncProxyImplTest::OnInitialSyncDone() { data_type_state_.initial_sync_done = true; UpdateResponseDataList empty_update_list; - type_sync_proxy_->OnUpdateReceived( - data_type_state_, empty_update_list, empty_update_list); + type_sync_proxy_->OnUpdateReceived(data_type_state_, empty_update_list); } void ModelTypeSyncProxyImplTest::UpdateFromServer(int64 version_offset, @@ -206,25 +177,7 @@ void ModelTypeSyncProxyImplTest::UpdateFromServer(int64 version_offset, UpdateResponseDataList list; list.push_back(data); - type_sync_proxy_->OnUpdateReceived( - data_type_state_, list, UpdateResponseDataList()); -} - -void ModelTypeSyncProxyImplTest::PendingUpdateFromServer( - int64 version_offset, - const std::string& tag, - const std::string& value, - const std::string& key_name) { - const std::string tag_hash = GenerateTagHash(tag); - UpdateResponseData data = mock_worker_->UpdateFromServer( - version_offset, - tag_hash, - GenerateEncryptedSpecifics(tag, value, key_name)); - - UpdateResponseDataList list; - list.push_back(data); - type_sync_proxy_->OnUpdateReceived( - data_type_state_, UpdateResponseDataList(), list); + type_sync_proxy_->OnUpdateReceived(data_type_state_, list); } void ModelTypeSyncProxyImplTest::TombstoneFromServer(int64 version_offset, @@ -237,40 +190,7 @@ void ModelTypeSyncProxyImplTest::TombstoneFromServer(int64 version_offset, UpdateResponseDataList list; list.push_back(data); - type_sync_proxy_->OnUpdateReceived( - data_type_state_, list, UpdateResponseDataList()); -} - -bool ModelTypeSyncProxyImplTest::HasPendingUpdate( - const std::string& tag) const { - const std::string client_tag_hash = GenerateTagHash(tag); - const UpdateResponseDataList list = type_sync_proxy_->GetPendingUpdates(); - for (UpdateResponseDataList::const_iterator it = list.begin(); - it != list.end(); - ++it) { - if (it->client_tag_hash == client_tag_hash) - return true; - } - return false; -} - -UpdateResponseData ModelTypeSyncProxyImplTest::GetPendingUpdate( - const std::string& tag) const { - DCHECK(HasPendingUpdate(tag)); - const std::string client_tag_hash = GenerateTagHash(tag); - const UpdateResponseDataList list = type_sync_proxy_->GetPendingUpdates(); - for (UpdateResponseDataList::const_iterator it = list.begin(); - it != list.end(); - ++it) { - if (it->client_tag_hash == client_tag_hash) - return *it; - } - NOTREACHED(); - return UpdateResponseData(); -} - -size_t ModelTypeSyncProxyImplTest::GetNumPendingUpdates() const { - return type_sync_proxy_->GetPendingUpdates().size(); + type_sync_proxy_->OnUpdateReceived(data_type_state_, list); } void ModelTypeSyncProxyImplTest::SuccessfulCommitResponse( @@ -280,18 +200,6 @@ void ModelTypeSyncProxyImplTest::SuccessfulCommitResponse( type_sync_proxy_->OnCommitCompleted(data_type_state_, list); } -void ModelTypeSyncProxyImplTest::UpdateDesiredEncryptionKey( - const std::string& key_name) { - data_type_state_.encryption_key_name = key_name; - type_sync_proxy_->OnUpdateReceived( - data_type_state_, UpdateResponseDataList(), UpdateResponseDataList()); -} - -void ModelTypeSyncProxyImplTest::SetServerEncryptionKey( - const std::string& key_name) { - mock_worker_->SetServerEncryptionKey(key_name); -} - std::string ModelTypeSyncProxyImplTest::GenerateTagHash( const std::string& tag) { return syncable::GenerateSyncableHash(kModelType, tag); @@ -306,19 +214,6 @@ sync_pb::EntitySpecifics ModelTypeSyncProxyImplTest::GenerateSpecifics( return specifics; } -// These tests never decrypt anything, so we can get away with faking the -// encryption for now. -sync_pb::EntitySpecifics ModelTypeSyncProxyImplTest::GenerateEncryptedSpecifics( - const std::string& tag, - const std::string& value, - const std::string& key_name) { - sync_pb::EntitySpecifics specifics; - AddDefaultFieldValue(kModelType, &specifics); - specifics.mutable_encrypted()->set_key_name(key_name); - specifics.mutable_encrypted()->set_blob("BLOB" + key_name); - return specifics; -} - size_t ModelTypeSyncProxyImplTest::GetNumCommitRequestLists() { return mock_worker_->GetNumCommitRequestLists(); } @@ -565,125 +460,4 @@ TEST_F(ModelTypeSyncProxyImplTest, Disable) { EXPECT_TRUE(HasCommitRequestForTag("tag3")); } -// Test receipt of pending updates. -TEST_F(ModelTypeSyncProxyImplTest, ReceivePendingUpdates) { - EXPECT_FALSE(HasPendingUpdate("tag1")); - EXPECT_EQ(0U, GetNumPendingUpdates()); - - // Receive a pending update. - PendingUpdateFromServer(5, "tag1", "value1", "key1"); - EXPECT_EQ(1U, GetNumPendingUpdates()); - ASSERT_TRUE(HasPendingUpdate("tag1")); - UpdateResponseData data1 = GetPendingUpdate("tag1"); - EXPECT_EQ(5, data1.response_version); - - // Receive an updated version of a pending update. - // It should overwrite the existing item. - PendingUpdateFromServer(10, "tag1", "value15", "key1"); - EXPECT_EQ(1U, GetNumPendingUpdates()); - ASSERT_TRUE(HasPendingUpdate("tag1")); - UpdateResponseData data2 = GetPendingUpdate("tag1"); - EXPECT_EQ(15, data2.response_version); - - // Receive a stale version of a pending update. - // It should have no effect. - PendingUpdateFromServer(-3, "tag1", "value12", "key1"); - EXPECT_EQ(1U, GetNumPendingUpdates()); - ASSERT_TRUE(HasPendingUpdate("tag1")); - UpdateResponseData data3 = GetPendingUpdate("tag1"); - EXPECT_EQ(15, data3.response_version); -} - -// Test that Disable clears pending update state. -TEST_F(ModelTypeSyncProxyImplTest, DisableWithPendingUpdates) { - PendingUpdateFromServer(5, "tag1", "value1", "key1"); - EXPECT_EQ(1U, GetNumPendingUpdates()); - ASSERT_TRUE(HasPendingUpdate("tag1")); - - Disable(); - ReEnable(); - - EXPECT_EQ(0U, GetNumPendingUpdates()); - EXPECT_FALSE(HasPendingUpdate("tag1")); -} - -// Test that Disconnect does not clear pending update state. -TEST_F(ModelTypeSyncProxyImplTest, DisconnectWithPendingUpdates) { - PendingUpdateFromServer(5, "tag1", "value1", "key1"); - EXPECT_EQ(1U, GetNumPendingUpdates()); - ASSERT_TRUE(HasPendingUpdate("tag1")); - - Disconnect(); - ReEnable(); - - EXPECT_EQ(1U, GetNumPendingUpdates()); - EXPECT_TRUE(HasPendingUpdate("tag1")); -} - -// Test re-encrypt everything when desired encryption key changes. -TEST_F(ModelTypeSyncProxyImplTest, ReEncryptCommitsWithNewKey) { - InitializeToReadyState(); - - // Commit an item. - WriteItem("tag1", "value1"); - ASSERT_TRUE(HasCommitRequestForTag("tag1")); - const CommitRequestData& tag1_v1_data = GetLatestCommitRequestForTag("tag1"); - SuccessfulCommitResponse(tag1_v1_data); - - // Create another item and don't wait for its commit response. - WriteItem("tag2", "value2"); - - ASSERT_EQ(2U, GetNumCommitRequestLists()); - - // Receive notice that the account's desired encryption key has changed. - UpdateDesiredEncryptionKey("k1"); - - // That should trigger a new commit request. - ASSERT_EQ(3U, GetNumCommitRequestLists()); - EXPECT_EQ(2U, GetNthCommitRequestList(2).size()); - - const CommitRequestData& tag1_enc = GetLatestCommitRequestForTag("tag1"); - const CommitRequestData& tag2_enc = GetLatestCommitRequestForTag("tag2"); - - SuccessfulCommitResponse(tag1_enc); - SuccessfulCommitResponse(tag2_enc); - - // And that should be the end of it. - ASSERT_EQ(3U, GetNumCommitRequestLists()); -} - -// Test receipt of updates with new and old keys. -TEST_F(ModelTypeSyncProxyImplTest, ReEncryptUpdatesWithNewKey) { - InitializeToReadyState(); - - // Receive an unencrpted update. - UpdateFromServer(5, "no_enc", "value1"); - - ASSERT_EQ(0U, GetNumCommitRequestLists()); - - // Set desired encryption key to k2 to force updates to some items. - UpdateDesiredEncryptionKey("k2"); - - ASSERT_EQ(1U, GetNumCommitRequestLists()); - EXPECT_EQ(1U, GetNthCommitRequestList(0).size()); - EXPECT_TRUE(HasCommitRequestForTag("no_enc")); - - // Receive an update that was encrypted with key k1. - SetServerEncryptionKey("k1"); - UpdateFromServer(10, "enc_k1", "value1"); - - // Receipt of updates encrypted with old key also forces a re-encrypt commit. - ASSERT_EQ(2U, GetNumCommitRequestLists()); - EXPECT_EQ(1U, GetNthCommitRequestList(1).size()); - EXPECT_TRUE(HasCommitRequestForTag("enc_k1")); - - // Receive an update that was encrypted with key k2. - SetServerEncryptionKey("k2"); - UpdateFromServer(15, "enc_k2", "value1"); - - // That was the correct key, so no re-encryption is required. - EXPECT_EQ(2U, GetNumCommitRequestLists()); - EXPECT_FALSE(HasCommitRequestForTag("enc_k2")); -} - } // namespace syncer diff --git a/sync/engine/model_type_sync_worker_impl.cc b/sync/engine/model_type_sync_worker_impl.cc index a58400f4..a5402c2 100644 --- a/sync/engine/model_type_sync_worker_impl.cc +++ b/sync/engine/model_type_sync_worker_impl.cc @@ -13,7 +13,6 @@ #include "sync/engine/model_type_sync_proxy.h" #include "sync/engine/non_blocking_type_commit_contribution.h" #include "sync/syncable/syncable_util.h" -#include "sync/util/cryptographer.h" #include "sync/util/time.h" namespace syncer { @@ -21,14 +20,11 @@ namespace syncer { ModelTypeSyncWorkerImpl::ModelTypeSyncWorkerImpl( ModelType type, const DataTypeState& initial_state, - const UpdateResponseDataList& saved_pending_updates, - CryptographerProvider* cryptographer_provider, NudgeHandler* nudge_handler, scoped_ptr type_sync_proxy) : type_(type), data_type_state_(initial_state), type_sync_proxy_(type_sync_proxy.Pass()), - cryptographer_provider_(cryptographer_provider), nudge_handler_(nudge_handler), entities_deleter_(&entities_), weak_ptr_factory_(this) { @@ -36,18 +32,6 @@ ModelTypeSyncWorkerImpl::ModelTypeSyncWorkerImpl( if (!data_type_state_.initial_sync_done) { nudge_handler_->NudgeForInitialDownload(type_); } - - for (UpdateResponseDataList::const_iterator it = - saved_pending_updates.begin(); - it != saved_pending_updates.end(); - ++it) { - EntityTracker* entity_tracker = EntityTracker::FromServerUpdate( - it->id, it->client_tag_hash, it->response_version); - entity_tracker->ReceivePendingUpdate(*it); - entities_.insert(std::make_pair(it->client_tag_hash, entity_tracker)); - } - - TryDecryptPendingUpdates(); } ModelTypeSyncWorkerImpl::~ModelTypeSyncWorkerImpl() { @@ -58,33 +42,6 @@ ModelType ModelTypeSyncWorkerImpl::GetModelType() const { return type_; } -bool ModelTypeSyncWorkerImpl::IsEncryptionRequired() const { - return !data_type_state_.encryption_key_name.empty(); -} - -void ModelTypeSyncWorkerImpl::SetEncryptionKeyName(const std::string& name) { - if (data_type_state_.encryption_key_name == name) - return; - - data_type_state_.encryption_key_name = name; - - // Pretend to send an update. This will cause the TypeSyncProxy to notice - // the new encryption key and take appropriate action. - type_sync_proxy_->OnUpdateReceived( - data_type_state_, UpdateResponseDataList(), UpdateResponseDataList()); -} - -void ModelTypeSyncWorkerImpl::OnCryptographerStateChanged() { - TryDecryptPendingUpdates(); - - ScopedCryptographerRef scoped_cryptographer_ref; - cryptographer_provider_->InitScopedCryptographerRef( - &scoped_cryptographer_ref); - Cryptographer* cryptographer = scoped_cryptographer_ref.Get(); - if (CanCommitItems(cryptographer)) - nudge_handler_->NudgeForCommit(type_); -} - // UpdateHandler implementation. void ModelTypeSyncWorkerImpl::GetDownloadProgress( sync_pb::DataTypeProgressMarker* progress_marker) const { @@ -109,14 +66,7 @@ SyncerError ModelTypeSyncWorkerImpl::ProcessGetUpdatesResponse( data_type_state_.type_context = mutated_context; data_type_state_.progress_marker = progress_marker; - ScopedCryptographerRef scoped_cryptographer_ref; - cryptographer_provider_->InitScopedCryptographerRef( - &scoped_cryptographer_ref); - Cryptographer* cryptographer = scoped_cryptographer_ref.Get(); - DCHECK(cryptographer); - UpdateResponseDataList response_datas; - UpdateResponseDataList pending_updates; for (SyncEntityList::const_iterator update_it = applicable_updates.begin(); update_it != applicable_updates.end(); @@ -136,17 +86,16 @@ SyncerError ModelTypeSyncWorkerImpl::ProcessGetUpdatesResponse( const std::string& client_tag_hash = update_entity->client_defined_unique_tag(); DCHECK(!client_tag_hash.empty()); - - EntityTracker* entity_tracker = NULL; EntityMap::const_iterator map_it = entities_.find(client_tag_hash); if (map_it == entities_.end()) { - entity_tracker = + EntityTracker* entity = EntityTracker::FromServerUpdate(update_entity->id_string(), client_tag_hash, update_entity->version()); - entities_.insert(std::make_pair(client_tag_hash, entity_tracker)); + entities_.insert(std::make_pair(client_tag_hash, entity)); } else { - entity_tracker = map_it->second; + EntityTracker* entity = map_it->second; + entity->ReceiveUpdate(update_entity->version()); } // Prepare the message for the model thread. @@ -158,39 +107,14 @@ SyncerError ModelTypeSyncWorkerImpl::ProcessGetUpdatesResponse( response_data.mtime = ProtoTimeToTime(update_entity->mtime()); response_data.non_unique_name = update_entity->name(); response_data.deleted = update_entity->deleted(); + response_data.specifics = update_entity->specifics(); - const sync_pb::EntitySpecifics& specifics = update_entity->specifics(); - - if (!specifics.has_encrypted()) { - // No encryption. - entity_tracker->ReceiveUpdate(update_entity->version()); - response_data.specifics = specifics; - response_datas.push_back(response_data); - } else if (specifics.has_encrypted() && - cryptographer->CanDecrypt(specifics.encrypted())) { - // Encrypted, but we know the key. - if (DecryptSpecifics( - cryptographer, specifics, &response_data.specifics)) { - entity_tracker->ReceiveUpdate(update_entity->version()); - response_data.encryption_key_name = specifics.encrypted().key_name(); - response_datas.push_back(response_data); - } - } else if (specifics.has_encrypted() && - !cryptographer->CanDecrypt(specifics.encrypted())) { - // Can't decrypt right now. Ask the entity tracker to handle it. - response_data.specifics = specifics; - if (entity_tracker->ReceivePendingUpdate(response_data)) { - // Send to the model thread for safe-keeping across restarts if the - // tracker decides the update is worth keeping. - pending_updates.push_back(response_data); - } - } + response_datas.push_back(response_data); } } // Forward these updates to the model thread so it can do the rest. - type_sync_proxy_->OnUpdateReceived( - data_type_state_, response_datas, pending_updates); + type_sync_proxy_->OnUpdateReceived(data_type_state_, response_datas); return SYNCER_OK; } @@ -204,8 +128,8 @@ void ModelTypeSyncWorkerImpl::ApplyUpdates(sessions::StatusController* status) { if (!data_type_state_.initial_sync_done) { data_type_state_.initial_sync_done = true; - type_sync_proxy_->OnUpdateReceived( - data_type_state_, UpdateResponseDataList(), UpdateResponseDataList()); + UpdateResponseDataList empty_update_list; + type_sync_proxy_->OnUpdateReceived(data_type_state_, empty_update_list); } } @@ -220,7 +144,7 @@ void ModelTypeSyncWorkerImpl::EnqueueForCommit( const CommitRequestDataList& list) { DCHECK(CalledOnValidThread()); - DCHECK(IsTypeInitialized()) + DCHECK(CanCommitItems()) << "Asked to commit items before type was initialized. " << "ModelType is: " << ModelTypeToString(type_); @@ -229,13 +153,6 @@ void ModelTypeSyncWorkerImpl::EnqueueForCommit( ++it) { StorePendingCommit(*it); } - - ScopedCryptographerRef scoped_cryptographer_ref; - cryptographer_provider_->InitScopedCryptographerRef( - &scoped_cryptographer_ref); - Cryptographer* cryptographer = scoped_cryptographer_ref.Get(); - if (CanCommitItems(cryptographer)) - nudge_handler_->NudgeForCommit(type_); } // CommitContributor implementation. @@ -247,12 +164,7 @@ scoped_ptr ModelTypeSyncWorkerImpl::GetContribution( std::vector sequence_numbers; google::protobuf::RepeatedPtrField commit_entities; - ScopedCryptographerRef scoped_cryptographer_ref; - cryptographer_provider_->InitScopedCryptographerRef( - &scoped_cryptographer_ref); - Cryptographer* cryptographer = scoped_cryptographer_ref.Get(); - - if (!CanCommitItems(cryptographer)) + if (!CanCommitItems()) return scoped_ptr(); // TODO(rlarocque): Avoid iterating here. @@ -265,7 +177,7 @@ scoped_ptr ModelTypeSyncWorkerImpl::GetContribution( int64 sequence_number = -1; entity->PrepareCommitProto(commit_entity, &sequence_number); - HelpInitializeCommitEntity(cryptographer, commit_entity); + HelpInitializeCommitEntity(commit_entity); sequence_numbers.push_back(sequence_number); space_remaining--; @@ -310,6 +222,9 @@ void ModelTypeSyncWorkerImpl::StorePendingCommit( request.deleted, request.specifics); } + + if (CanCommitItems()) + nudge_handler_->NudgeForCommit(type_); } void ModelTypeSyncWorkerImpl::OnCommitResponse( @@ -345,30 +260,14 @@ base::WeakPtr ModelTypeSyncWorkerImpl::AsWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); } -bool ModelTypeSyncWorkerImpl::IsTypeInitialized() const { - return !data_type_state_.type_root_id.empty() && - data_type_state_.initial_sync_done; -} - -bool ModelTypeSyncWorkerImpl::CanCommitItems( - Cryptographer* cryptographer) const { +bool ModelTypeSyncWorkerImpl::CanCommitItems() const { // We can't commit anything until we know the type's parent node. // We'll get it in the first update response. - if (!IsTypeInitialized()) - return false; - - // Don't commit if we should be encrypting but don't have the required keys. - if (IsEncryptionRequired() && (!cryptographer || !cryptographer->is_ready() || - cryptographer->GetDefaultNigoriKeyName() != - data_type_state_.encryption_key_name)) { - return false; - } - - return true; + return !data_type_state_.type_root_id.empty() && + data_type_state_.initial_sync_done; } void ModelTypeSyncWorkerImpl::HelpInitializeCommitEntity( - Cryptographer* cryptographer, sync_pb::SyncEntity* sync_entity) { // Initial commits need our help to generate a client ID. if (!sync_entity->has_id_string()) { @@ -378,80 +277,14 @@ void ModelTypeSyncWorkerImpl::HelpInitializeCommitEntity( base::StringPrintf("%s-%" PRId64, ModelTypeToString(type_), id)); } - // Encrypt the specifics and hide the title if necessary. - if (IsEncryptionRequired()) { - sync_pb::EntitySpecifics encrypted_specifics; - cryptographer->Encrypt(sync_entity->specifics(), - encrypted_specifics.mutable_encrypted()); - sync_entity->mutable_specifics()->CopyFrom(encrypted_specifics); - sync_entity->set_name("encrypted"); - } - // Always include enough specifics to identify the type. Do this even in // deletion requests, where the specifics are otherwise invalid. - AddDefaultFieldValue(type_, sync_entity->mutable_specifics()); + if (!sync_entity->has_specifics()) { + AddDefaultFieldValue(type_, sync_entity->mutable_specifics()); + } // We're always responsible for the parent ID. sync_entity->set_parent_id_string(data_type_state_.type_root_id); } -void ModelTypeSyncWorkerImpl::TryDecryptPendingUpdates() { - UpdateResponseDataList response_datas; - - ScopedCryptographerRef scoped_cryptographer_ref; - cryptographer_provider_->InitScopedCryptographerRef( - &scoped_cryptographer_ref); - Cryptographer* cryptographer = scoped_cryptographer_ref.Get(); - DCHECK(cryptographer); - - for (EntityMap::const_iterator it = entities_.begin(); it != entities_.end(); - ++it) { - if (it->second->HasPendingUpdate()) { - const UpdateResponseData& saved_pending = it->second->GetPendingUpdate(); - - // We assume all pending updates are encrypted items for which we - // don't have the key. - DCHECK(saved_pending.specifics.has_encrypted()); - - if (cryptographer->CanDecrypt(saved_pending.specifics.encrypted())) { - UpdateResponseData decrypted_response = saved_pending; - if (DecryptSpecifics(cryptographer, - saved_pending.specifics, - &decrypted_response.specifics)) { - decrypted_response.encryption_key_name = - saved_pending.specifics.encrypted().key_name(); - response_datas.push_back(decrypted_response); - - it->second->ClearPendingUpdate(); - } - } - } - } - - if (!response_datas.empty()) { - type_sync_proxy_->OnUpdateReceived( - data_type_state_, response_datas, UpdateResponseDataList()); - } -} - -bool ModelTypeSyncWorkerImpl::DecryptSpecifics( - Cryptographer* cryptographer, - const sync_pb::EntitySpecifics& in, - sync_pb::EntitySpecifics* out) { - DCHECK(in.has_encrypted()); - DCHECK(cryptographer->CanDecrypt(in.encrypted())); - - std::string plaintext; - plaintext = cryptographer->DecryptToString(in.encrypted()); - if (plaintext.empty()) { - LOG(ERROR) << "Failed to decrypt a decryptable entity"; - return false; - } - if (!out->ParseFromString(plaintext)) { - LOG(ERROR) << "Failed to parse decrypted entity"; - return false; - } - return true; -} - } // namespace syncer diff --git a/sync/engine/model_type_sync_worker_impl.h b/sync/engine/model_type_sync_worker_impl.h index f2df976..6905c50 100644 --- a/sync/engine/model_type_sync_worker_impl.h +++ b/sync/engine/model_type_sync_worker_impl.h @@ -10,13 +10,11 @@ #include "base/threading/non_thread_safe.h" #include "sync/base/sync_export.h" #include "sync/engine/commit_contributor.h" -#include "sync/engine/cryptographer_provider.h" #include "sync/engine/model_type_sync_worker.h" #include "sync/engine/nudge_handler.h" #include "sync/engine/update_handler.h" #include "sync/internal_api/public/base/model_type.h" #include "sync/internal_api/public/non_blocking_sync_common.h" -#include "sync/internal_api/public/sync_encryption_handler.h" #include "sync/protocol/sync.pb.h" namespace base { @@ -55,19 +53,12 @@ class SYNC_EXPORT ModelTypeSyncWorkerImpl : public UpdateHandler, public: ModelTypeSyncWorkerImpl(ModelType type, const DataTypeState& initial_state, - const UpdateResponseDataList& saved_pending_updates, - CryptographerProvider* cryptographer_provider, NudgeHandler* nudge_handler, scoped_ptr type_sync_proxy); virtual ~ModelTypeSyncWorkerImpl(); ModelType GetModelType() const; - bool IsEncryptionRequired() const; - void SetEncryptionKeyName(const std::string& name); - - void OnCryptographerStateChanged(); - // UpdateHandler implementation. virtual void GetDownloadProgress( sync_pb::DataTypeProgressMarker* progress_marker) const OVERRIDE; @@ -96,45 +87,20 @@ class SYNC_EXPORT ModelTypeSyncWorkerImpl : public UpdateHandler, private: typedef std::map EntityMap; - typedef std::map UpdateMap; // Stores a single commit request in this object's internal state. void StorePendingCommit(const CommitRequestData& request); - // Returns true if this type has successfully fetched all available updates - // from the server at least once. Our state may or may not be stale, but at - // least we know that it was valid at some point in the past. - bool IsTypeInitialized() const; - - // Returns true if this type is prepared to commit items. Currently, this - // depends on having downloaded the initial data and having the encryption - // settings in a good state. - bool CanCommitItems(Cryptographer* cryptographer) const; + // Returns true if all data type state required for commits is available. In + // practice, this means that it returns true from the time this object first + // receives notice of a successful update fetch from the server. + bool CanCommitItems() const; // Initializes the parts of a commit entity that are the responsibility of // this class, and not the EntityTracker. Some fields, like the // client-assigned ID, can only be set by an entity with knowledge of the // entire data type's state. - void HelpInitializeCommitEntity(Cryptographer* cryptographer, - sync_pb::SyncEntity* commit_entity); - - // Attempts to decrypt pending updates stored in the EntityMap. If - // successful, will remove the update from the its EntityTracker and forward - // it to the proxy thread for application. - void TryDecryptPendingUpdates(); - - // Attempts to decrypt the given specifics and return them in the |out| - // parameter. Assumes cryptographer->CanDecrypt(specifics) returned true. - // - // Returns false if the decryption failed. There are no guarantees about the - // contents of |out| when that happens. - // - // In theory, this should never fail. Only corrupt or invalid entries could - // cause this to fail, and no clients are known to create such entries. The - // failure case is an attempt to be defensive against bad input. - static bool DecryptSpecifics(Cryptographer* cryptographer, - const sync_pb::EntitySpecifics& in, - sync_pb::EntitySpecifics* out); + void HelpInitializeCommitEntity(sync_pb::SyncEntity* commit_entity); ModelType type_; @@ -145,10 +111,6 @@ class SYNC_EXPORT ModelTypeSyncWorkerImpl : public UpdateHandler, // This is NULL when no proxy is connected.. scoped_ptr type_sync_proxy_; - // A helper to provide access to the syncable::Directory's cryptographer. - // Not owned. - CryptographerProvider* cryptographer_provider_; - // Interface used to access and send nudges to the sync scheduler. Not owned. NudgeHandler* nudge_handler_; diff --git a/sync/engine/model_type_sync_worker_impl_unittest.cc b/sync/engine/model_type_sync_worker_impl_unittest.cc index df2ed66..0c1fa13 100644 --- a/sync/engine/model_type_sync_worker_impl_unittest.cc +++ b/sync/engine/model_type_sync_worker_impl_unittest.cc @@ -4,7 +4,6 @@ #include "sync/engine/model_type_sync_worker_impl.h" -#include "base/strings/stringprintf.h" #include "sync/engine/commit_contribution.h" #include "sync/engine/model_type_sync_proxy.h" #include "sync/internal_api/public/base/model_type.h" @@ -14,18 +13,13 @@ #include "sync/syncable/syncable_util.h" #include "sync/test/engine/mock_model_type_sync_proxy.h" #include "sync/test/engine/mock_nudge_handler.h" -#include "sync/test/engine/simple_cryptographer_provider.h" #include "sync/test/engine/single_type_mock_server.h" -#include "sync/test/fake_encryptor.h" #include "testing/gtest/include/gtest/gtest.h" static const std::string kTypeParentId = "PrefsRootNodeID"; static const syncer::ModelType kModelType = syncer::PREFERENCES; -// Special constant value taken from cryptographer.cc. -const char kNigoriKeyName[] = "nigori-key"; - namespace syncer { // Tests the ModelTypeSyncWorkerImpl. @@ -71,23 +65,8 @@ class ModelTypeSyncWorkerImplTest : public ::testing::Test { // committing items right away. void NormalInitialize(); - // Initialize with some saved pending updates from the model thread. - void InitializeWithPendingUpdates( - const UpdateResponseDataList& initial_pending_updates); - - // Initialize with a custom initial DataTypeState and pending updates. - void InitializeWithState(const DataTypeState& state, - const UpdateResponseDataList& pending_updates); - - // Introduce a new key that the local cryptographer can't decrypt. - void NewForeignEncryptionKey(); - - // Update the local cryptographer with all relevant keys. - void UpdateLocalCryptographer(); - - // Use the Nth nigori instance to encrypt incoming updates. - // The default value, zero, indicates no encryption. - void SetUpdateEncryptionFilter(int n); + // Initialize with a custom initial DataTypeState. + void InitializeWithState(const DataTypeState& state); // Modifications on the model thread that get sent to the worker under test. void CommitRequest(const std::string& tag, const std::string& value); @@ -100,13 +79,6 @@ class ModelTypeSyncWorkerImplTest : public ::testing::Test { const std::string& value); void TriggerTombstoneFromServer(int64 version_offset, const std::string& tag); - // Delivers specified protos as updates. - // - // Does not update mock server state. Should be used as a last resort when - // writing test cases that require entities that don't fit the normal sync - // protocol. Try to use the other, higher level methods if possible. - void DeliverRawUpdates(const SyncEntityList& update_list); - // By default, this harness behaves as if all tasks posted to the model // thread are executed immediately. However, this is not necessarily true. // The model's TaskRunner has a queue, and the tasks we post to it could @@ -138,7 +110,6 @@ class ModelTypeSyncWorkerImplTest : public ::testing::Test { // be updated until the response is actually processed by the model thread. size_t GetNumModelThreadUpdateResponses() const; UpdateResponseDataList GetNthModelThreadUpdateResponse(size_t n) const; - UpdateResponseDataList GetNthModelThreadPendingUpdates(size_t n) const; DataTypeState GetNthModelThreadUpdateState(size_t n) const; // Reads the latest update response datas on the model thread. @@ -173,39 +144,7 @@ class ModelTypeSyncWorkerImplTest : public ::testing::Test { static sync_pb::EntitySpecifics GenerateSpecifics(const std::string& tag, const std::string& value); - // Returns a set of KeyParams for the cryptographer. Each input 'n' value - // results in a different set of parameters. - static KeyParams GetNthKeyParams(int n); - - // Returns the name for the given Nigori. - // - // Uses some 'white-box' knowledge to mimic the names that a real sync client - // would generate. It's probably not necessary to do so, but it can't hurt. - static std::string GetNigoriName(const Nigori& nigori); - - // Modifies the input/output parameter |specifics| by encrypting it with - // a Nigori intialized with the specified KeyParams. - static void EncryptUpdate(const KeyParams& params, - sync_pb::EntitySpecifics* specifics); - private: - // An encryptor for our cryptographer. - FakeEncryptor fake_encryptor_; - - // The cryptographer itself. - Cryptographer cryptographer_; - - // A CryptographerProvider for the ModelTypeSyncWorkerImpl. - SimpleCryptographerProvider cryptographer_provider_; - - // The number of the most recent foreign encryption key known to our - // cryptographer. Note that not all of these will be decryptable. - int foreign_encryption_key_index_; - - // The number of the encryption key used to encrypt incoming updates. A zero - // value implies no encryption. - int update_encryption_filter_index_; - // The ModelTypeSyncWorkerImpl being tested. scoped_ptr worker_; @@ -224,12 +163,7 @@ class ModelTypeSyncWorkerImplTest : public ::testing::Test { }; ModelTypeSyncWorkerImplTest::ModelTypeSyncWorkerImplTest() - : cryptographer_(&fake_encryptor_), - cryptographer_provider_(&cryptographer_), - foreign_encryption_key_index_(0), - update_encryption_filter_index_(0), - mock_type_sync_proxy_(NULL), - mock_server_(kModelType) { + : mock_type_sync_proxy_(NULL), mock_server_(kModelType) { } ModelTypeSyncWorkerImplTest::~ModelTypeSyncWorkerImplTest() { @@ -241,15 +175,10 @@ void ModelTypeSyncWorkerImplTest::FirstInitialize() { GetSpecificsFieldNumberFromModelType(kModelType)); initial_state.next_client_id = 0; - InitializeWithState(initial_state, UpdateResponseDataList()); + InitializeWithState(initial_state); } void ModelTypeSyncWorkerImplTest::NormalInitialize() { - InitializeWithPendingUpdates(UpdateResponseDataList()); -} - -void ModelTypeSyncWorkerImplTest::InitializeWithPendingUpdates( - const UpdateResponseDataList& initial_pending_updates) { DataTypeState initial_state; initial_state.progress_marker.set_data_type_id( GetSpecificsFieldNumberFromModelType(kModelType)); @@ -259,79 +188,21 @@ void ModelTypeSyncWorkerImplTest::InitializeWithPendingUpdates( initial_state.type_root_id = kTypeParentId; initial_state.initial_sync_done = true; - InitializeWithState(initial_state, initial_pending_updates); + InitializeWithState(initial_state); mock_nudge_handler_.ClearCounters(); } void ModelTypeSyncWorkerImplTest::InitializeWithState( - const DataTypeState& state, - const UpdateResponseDataList& initial_pending_updates) { + const DataTypeState& state) { DCHECK(!worker_); // We don't get to own this object. The |worker_| keeps a scoped_ptr to it. mock_type_sync_proxy_ = new MockModelTypeSyncProxy(); scoped_ptr proxy(mock_type_sync_proxy_); - worker_.reset(new ModelTypeSyncWorkerImpl(kModelType, - state, - initial_pending_updates, - &cryptographer_provider_, - &mock_nudge_handler_, - proxy.Pass())); -} - -void ModelTypeSyncWorkerImplTest::NewForeignEncryptionKey() { - foreign_encryption_key_index_++; - - sync_pb::NigoriKeyBag bag; - - for (int i = 0; i <= foreign_encryption_key_index_; ++i) { - Nigori nigori; - KeyParams params = GetNthKeyParams(i); - nigori.InitByDerivation(params.hostname, params.username, params.password); - - sync_pb::NigoriKey* key = bag.add_key(); - - key->set_name(GetNigoriName(nigori)); - nigori.ExportKeys(key->mutable_user_key(), - key->mutable_encryption_key(), - key->mutable_mac_key()); - } - - // Re-create the last nigori from that loop. - Nigori last_nigori; - KeyParams params = GetNthKeyParams(foreign_encryption_key_index_); - last_nigori.InitByDerivation( - params.hostname, params.username, params.password); - - // Serialize and encrypt the bag with the last nigori. - std::string serialized_bag; - bag.SerializeToString(&serialized_bag); - - sync_pb::EncryptedData encrypted; - encrypted.set_key_name(GetNigoriName(last_nigori)); - last_nigori.Encrypt(serialized_bag, encrypted.mutable_blob()); - - // Update the cryptographer with new pending keys. - cryptographer_.SetPendingKeys(encrypted); - - // Update the worker with the latest encryption key name. - if (worker_) - worker_->SetEncryptionKeyName(encrypted.key_name()); -} - -void ModelTypeSyncWorkerImplTest::UpdateLocalCryptographer() { - KeyParams params = GetNthKeyParams(foreign_encryption_key_index_); - bool success = cryptographer_.DecryptPendingKeys(params); - DCHECK(success); - - if (worker_) - worker_->OnCryptographerStateChanged(); -} - -void ModelTypeSyncWorkerImplTest::SetUpdateEncryptionFilter(int n) { - update_encryption_filter_index_ = n; + worker_.reset(new ModelTypeSyncWorkerImpl( + kModelType, state, &mock_nudge_handler_, proxy.Pass())); } void ModelTypeSyncWorkerImplTest::CommitRequest(const std::string& name, @@ -372,12 +243,6 @@ void ModelTypeSyncWorkerImplTest::TriggerUpdateFromServer( const std::string& value) { sync_pb::SyncEntity entity = mock_server_.UpdateFromServer( version_offset, GenerateTagHash(tag), GenerateSpecifics(tag, value)); - - if (update_encryption_filter_index_ != 0) { - EncryptUpdate(GetNthKeyParams(update_encryption_filter_index_), - entity.mutable_specifics()); - } - SyncEntityList entity_list; entity_list.push_back(&entity); @@ -390,27 +255,11 @@ void ModelTypeSyncWorkerImplTest::TriggerUpdateFromServer( worker_->ApplyUpdates(&dummy_status); } -void ModelTypeSyncWorkerImplTest::DeliverRawUpdates( - const SyncEntityList& list) { - sessions::StatusController dummy_status; - worker_->ProcessGetUpdatesResponse(mock_server_.GetProgress(), - mock_server_.GetContext(), - list, - &dummy_status); - worker_->ApplyUpdates(&dummy_status); -} - void ModelTypeSyncWorkerImplTest::TriggerTombstoneFromServer( int64 version_offset, const std::string& tag) { sync_pb::SyncEntity entity = mock_server_.TombstoneFromServer(version_offset, GenerateTagHash(tag)); - - if (update_encryption_filter_index_ != 0) { - EncryptUpdate(GetNthKeyParams(update_encryption_filter_index_), - entity.mutable_specifics()); - } - SyncEntityList entity_list; entity_list.push_back(&entity); @@ -497,12 +346,6 @@ ModelTypeSyncWorkerImplTest::GetNthModelThreadUpdateResponse(size_t n) const { return mock_type_sync_proxy_->GetNthUpdateResponse(n); } -UpdateResponseDataList -ModelTypeSyncWorkerImplTest::GetNthModelThreadPendingUpdates(size_t n) const { - DCHECK_LT(n, GetNumModelThreadUpdateResponses()); - return mock_type_sync_proxy_->GetNthPendingUpdates(n); -} - DataTypeState ModelTypeSyncWorkerImplTest::GetNthModelThreadUpdateState( size_t n) const { DCHECK_LT(n, GetNumModelThreadUpdateResponses()); @@ -558,7 +401,6 @@ int ModelTypeSyncWorkerImplTest::GetNumInitialDownloadNudges() const { return mock_nudge_handler_.GetNumInitialDownloadNudges(); } -// static. std::string ModelTypeSyncWorkerImplTest::GenerateTagHash( const std::string& tag) { const std::string& client_tag_hash = @@ -566,7 +408,6 @@ std::string ModelTypeSyncWorkerImplTest::GenerateTagHash( return client_tag_hash; } -// static. sync_pb::EntitySpecifics ModelTypeSyncWorkerImplTest::GenerateSpecifics( const std::string& tag, const std::string& value) { @@ -576,46 +417,6 @@ sync_pb::EntitySpecifics ModelTypeSyncWorkerImplTest::GenerateSpecifics( return specifics; } -// static. -std::string ModelTypeSyncWorkerImplTest::GetNigoriName(const Nigori& nigori) { - std::string name; - if (!nigori.Permute(Nigori::Password, kNigoriKeyName, &name)) { - NOTREACHED(); - return std::string(); - } - - return name; -} - -// static. -KeyParams ModelTypeSyncWorkerImplTest::GetNthKeyParams(int n) { - KeyParams params; - params.hostname = std::string("localhost"); - params.username = std::string("userX"); - params.password = base::StringPrintf("pw%02d", n); - return params; -} - -// static. -void ModelTypeSyncWorkerImplTest::EncryptUpdate( - const KeyParams& params, - sync_pb::EntitySpecifics* specifics) { - Nigori nigori; - nigori.InitByDerivation(params.hostname, params.username, params.password); - - sync_pb::EntitySpecifics original_specifics = *specifics; - std::string plaintext; - original_specifics.SerializeToString(&plaintext); - - std::string encrypted; - nigori.Encrypt(plaintext, &encrypted); - - specifics->Clear(); - AddDefaultFieldValue(kModelType, specifics); - specifics->mutable_encrypted()->set_key_name(GetNigoriName(nigori)); - specifics->mutable_encrypted()->set_blob(encrypted); -} - // Requests a commit and verifies the messages sent to the client and server as // a result. // @@ -799,7 +600,6 @@ TEST_F(ModelTypeSyncWorkerImplTest, TwoNewItemsCommittedSeparately) { EXPECT_EQ(2U, GetNumModelThreadCommitResponses()); } -// Test normal update receipt code path. TEST_F(ModelTypeSyncWorkerImplTest, ReceiveUpdates) { NormalInitialize(); @@ -825,281 +625,4 @@ TEST_F(ModelTypeSyncWorkerImplTest, ReceiveUpdates) { EXPECT_EQ("value1", update.specifics.preference().value()); } -// Test commit of encrypted updates. -TEST_F(ModelTypeSyncWorkerImplTest, EncryptedCommit) { - NormalInitialize(); - - NewForeignEncryptionKey(); - UpdateLocalCryptographer(); - - // Normal commit request stuff. - CommitRequest("tag1", "value1"); - DoSuccessfulCommit(); - ASSERT_EQ(1U, GetNumCommitMessagesOnServer()); - EXPECT_EQ(1, GetNthCommitMessageOnServer(0).commit().entries_size()); - ASSERT_TRUE(HasCommitEntityOnServer("tag1")); - const sync_pb::SyncEntity& tag1_entity = - GetLatestCommitEntityOnServer("tag1"); - - EXPECT_TRUE(tag1_entity.specifics().has_encrypted()); - - // The title should be overwritten. - EXPECT_EQ(tag1_entity.name(), "encrypted"); - - // The type should be set, but there should be no non-encrypted contents. - EXPECT_TRUE(tag1_entity.specifics().has_preference()); - EXPECT_FALSE(tag1_entity.specifics().preference().has_name()); - EXPECT_FALSE(tag1_entity.specifics().preference().has_value()); -} - -// Test items are not committed when encryption is required but unavailable. -TEST_F(ModelTypeSyncWorkerImplTest, EncryptionBlocksCommits) { - NormalInitialize(); - - CommitRequest("tag1", "value1"); - EXPECT_TRUE(WillCommit()); - - // We know encryption is in use on this account, but don't have the necessary - // encryption keys. The worker should refuse to commit. - NewForeignEncryptionKey(); - EXPECT_FALSE(WillCommit()); - - // Once the cryptographer is returned to a normal state, we should be able to - // commit again. - EXPECT_EQ(1, GetNumCommitNudges()); - UpdateLocalCryptographer(); - EXPECT_EQ(2, GetNumCommitNudges()); - EXPECT_TRUE(WillCommit()); - - // Verify the committed entity was properly encrypted. - DoSuccessfulCommit(); - ASSERT_EQ(1U, GetNumCommitMessagesOnServer()); - EXPECT_EQ(1, GetNthCommitMessageOnServer(0).commit().entries_size()); - ASSERT_TRUE(HasCommitEntityOnServer("tag1")); - const sync_pb::SyncEntity& tag1_entity = - GetLatestCommitEntityOnServer("tag1"); - EXPECT_TRUE(tag1_entity.specifics().has_encrypted()); - EXPECT_EQ(tag1_entity.name(), "encrypted"); - EXPECT_TRUE(tag1_entity.specifics().has_preference()); - EXPECT_FALSE(tag1_entity.specifics().preference().has_name()); - EXPECT_FALSE(tag1_entity.specifics().preference().has_value()); -} - -// Test the receipt of decryptable entities. -TEST_F(ModelTypeSyncWorkerImplTest, ReceiveDecryptableEntities) { - NormalInitialize(); - - // Create a new Nigori and allow the cryptographer to decrypt it. - NewForeignEncryptionKey(); - UpdateLocalCryptographer(); - - // First, receive an unencrypted entry. - TriggerUpdateFromServer(10, "tag1", "value1"); - - // Test some basic properties regarding the update. - ASSERT_TRUE(HasUpdateResponseOnModelThread("tag1")); - UpdateResponseData update1 = GetUpdateResponseOnModelThread("tag1"); - EXPECT_EQ("tag1", update1.specifics.preference().name()); - EXPECT_EQ("value1", update1.specifics.preference().value()); - EXPECT_TRUE(update1.encryption_key_name.empty()); - - // Set received updates to be encrypted using the new nigori. - SetUpdateEncryptionFilter(1); - - // This next update will be encrypted. - TriggerUpdateFromServer(10, "tag2", "value2"); - - // Test its basic features and the value of encryption_key_name. - ASSERT_TRUE(HasUpdateResponseOnModelThread("tag2")); - UpdateResponseData update2 = GetUpdateResponseOnModelThread("tag2"); - EXPECT_EQ("tag2", update2.specifics.preference().name()); - EXPECT_EQ("value2", update2.specifics.preference().value()); - EXPECT_FALSE(update2.encryption_key_name.empty()); -} - -// Receive updates that are initially undecryptable, then ensure they get -// delivered to the model thread when decryption becomes possible. -TEST_F(ModelTypeSyncWorkerImplTest, ReceiveUndecryptableEntries) { - NormalInitialize(); - - // Set a new encryption key. The model thread will be notified of the new - // encryption key through a faked update response. - NewForeignEncryptionKey(); - EXPECT_EQ(1U, GetNumModelThreadUpdateResponses()); - - // Send an update using that new key. - SetUpdateEncryptionFilter(1); - TriggerUpdateFromServer(10, "tag1", "value1"); - - // At this point, the cryptographer does not have access to the key, so the - // updates will be undecryptable. They'll be transfered to the model thread - // for safe-keeping as pending updates. - ASSERT_EQ(2U, GetNumModelThreadUpdateResponses()); - UpdateResponseDataList updates_list = GetNthModelThreadUpdateResponse(1); - EXPECT_EQ(0U, updates_list.size()); - UpdateResponseDataList pending_updates = GetNthModelThreadPendingUpdates(1); - EXPECT_EQ(1U, pending_updates.size()); - - // The update will be delivered as soon as decryption becomes possible. - UpdateLocalCryptographer(); - ASSERT_TRUE(HasUpdateResponseOnModelThread("tag1")); - UpdateResponseData update = GetUpdateResponseOnModelThread("tag1"); - EXPECT_EQ("tag1", update.specifics.preference().name()); - EXPECT_EQ("value1", update.specifics.preference().value()); - EXPECT_FALSE(update.encryption_key_name.empty()); -} - -// Ensure that even encrypted updates can cause conflicts. -TEST_F(ModelTypeSyncWorkerImplTest, EncryptedUpdateOverridesPendingCommit) { - NormalInitialize(); - - // Prepeare to commit an item. - CommitRequest("tag1", "value1"); - EXPECT_TRUE(WillCommit()); - - // Receive an encrypted update for that item. - SetUpdateEncryptionFilter(1); - TriggerUpdateFromServer(10, "tag1", "value1"); - - // The pending commit state should be cleared. - EXPECT_FALSE(WillCommit()); - - // The encrypted update will be delivered to the model thread. - ASSERT_EQ(1U, GetNumModelThreadUpdateResponses()); - UpdateResponseDataList updates_list = GetNthModelThreadUpdateResponse(0); - EXPECT_EQ(0U, updates_list.size()); - UpdateResponseDataList pending_updates = GetNthModelThreadPendingUpdates(0); - EXPECT_EQ(1U, pending_updates.size()); -} - -// Test decryption of pending updates saved across a restart. -TEST_F(ModelTypeSyncWorkerImplTest, RestorePendingEntries) { - // Create a fake pending update. - UpdateResponseData update; - - update.client_tag_hash = GenerateTagHash("tag1"); - update.id = "SomeID"; - update.response_version = 100; - update.ctime = base::Time::UnixEpoch() + base::TimeDelta::FromSeconds(10); - update.mtime = base::Time::UnixEpoch() + base::TimeDelta::FromSeconds(11); - update.non_unique_name = "encrypted"; - update.deleted = false; - - update.specifics = GenerateSpecifics("tag1", "value1"); - EncryptUpdate(GetNthKeyParams(1), &(update.specifics)); - - // Inject the update during ModelTypeSyncWorker initialization. - UpdateResponseDataList saved_pending_updates; - saved_pending_updates.push_back(update); - InitializeWithPendingUpdates(saved_pending_updates); - - // Update will be undecryptable at first. - EXPECT_EQ(0U, GetNumModelThreadUpdateResponses()); - ASSERT_FALSE(HasUpdateResponseOnModelThread("tag1")); - - // Update the cryptographer so it can decrypt that update. - NewForeignEncryptionKey(); - UpdateLocalCryptographer(); - - // Verify the item gets decrypted and sent back to the model thread. - ASSERT_TRUE(HasUpdateResponseOnModelThread("tag1")); -} - -// Test decryption of pending updates saved across a restart. This test -// differs from the previous one in that the restored updates can be decrypted -// immediately after the ModelTypeSyncWorker is constructed. -TEST_F(ModelTypeSyncWorkerImplTest, RestoreApplicableEntries) { - // Update the cryptographer so it can decrypt that update. - NewForeignEncryptionKey(); - UpdateLocalCryptographer(); - - // Create a fake pending update. - UpdateResponseData update; - update.client_tag_hash = GenerateTagHash("tag1"); - update.id = "SomeID"; - update.response_version = 100; - update.ctime = base::Time::UnixEpoch() + base::TimeDelta::FromSeconds(10); - update.mtime = base::Time::UnixEpoch() + base::TimeDelta::FromSeconds(11); - update.non_unique_name = "encrypted"; - update.deleted = false; - - update.specifics = GenerateSpecifics("tag1", "value1"); - EncryptUpdate(GetNthKeyParams(1), &(update.specifics)); - - // Inject the update during ModelTypeSyncWorker initialization. - UpdateResponseDataList saved_pending_updates; - saved_pending_updates.push_back(update); - InitializeWithPendingUpdates(saved_pending_updates); - - // Verify the item gets decrypted and sent back to the model thread. - ASSERT_TRUE(HasUpdateResponseOnModelThread("tag1")); -} - -// Test that undecryptable updates provide sufficient reason to not commit. -// -// This should be rare in practice. Usually the cryptographer will be in an -// unusable state when we receive undecryptable updates, and that alone will be -// enough to prevent all commits. -TEST_F(ModelTypeSyncWorkerImplTest, CommitBlockedByPending) { - NormalInitialize(); - - // Prepeare to commit an item. - CommitRequest("tag1", "value1"); - EXPECT_TRUE(WillCommit()); - - // Receive an encrypted update for that item. - SetUpdateEncryptionFilter(1); - TriggerUpdateFromServer(10, "tag1", "value1"); - - // The pending commit state should be cleared. - EXPECT_FALSE(WillCommit()); - - // The pending update will be delivered to the model thread. - HasUpdateResponseOnModelThread("tag1"); - - // Pretend the update arrived too late to prevent another commit request. - CommitRequest("tag1", "value2"); - - EXPECT_FALSE(WillCommit()); -} - -// Verify that corrupted encrypted updates don't cause crashes. -TEST_F(ModelTypeSyncWorkerImplTest, ReceiveCorruptEncryption) { - // Initialize the worker with basic encryption state. - NormalInitialize(); - NewForeignEncryptionKey(); - UpdateLocalCryptographer(); - - // Manually create an update. - sync_pb::SyncEntity entity; - entity.set_client_defined_unique_tag(GenerateTagHash("tag1")); - entity.set_id_string("SomeID"); - entity.set_version(1); - entity.set_ctime(1000); - entity.set_mtime(1001); - entity.set_name("encrypted"); - entity.set_deleted(false); - - // Encrypt it. - entity.mutable_specifics()->CopyFrom(GenerateSpecifics("tag1", "value1")); - EncryptUpdate(GetNthKeyParams(1), entity.mutable_specifics()); - - // Replace a few bytes to corrupt it. - entity.mutable_specifics()->mutable_encrypted()->mutable_blob()->replace( - 0, 4, "xyz!"); - - SyncEntityList entity_list; - entity_list.push_back(&entity); - - // If a corrupt update could trigger a crash, this is where it would happen. - DeliverRawUpdates(entity_list); - - EXPECT_FALSE(HasUpdateResponseOnModelThread("tag1")); - - // Deliver a non-corrupt update to see if the everything still works. - SetUpdateEncryptionFilter(1); - TriggerUpdateFromServer(10, "tag1", "value1"); - EXPECT_TRUE(HasUpdateResponseOnModelThread("tag1")); -} - } // namespace syncer -- cgit v1.1