[Sync] Adding integration tests for USS encryption and fixing a worker bug.

chrome/browser/sync/test/integration/two_client_uss_sync_test.cc

Index: chrome/browser/sync/test/integration/two_client_uss_sync_test.cc
diff --git a/chrome/browser/sync/test/integration/two_client_uss_sync_test.cc b/chrome/browser/sync/test/integration/two_client_uss_sync_test.cc
index 3cfa2a588026848d8e369613a5ee47ae01bc30a2..f830f14fb382c004b054a3e9db8fc0d7d3a25081 100644
--- a/chrome/browser/sync/test/integration/two_client_uss_sync_test.cc
+++ b/chrome/browser/sync/test/integration/two_client_uss_sync_test.cc
@@ -25,9 +25,12 @@ using syncer::SharedModelTypeProcessor;
 
 const char kKey1[] = "key1";
 const char kKey2[] = "key2";
+const char kKey3[] = "key3";
+const char kKey4[] = "key4";
 const char kValue1[] = "value1";
 const char kValue2[] = "value2";
 const char kValue3[] = "value3";
+const char* kPassphrase = "12345";

[maxbogue, 2016/10/11 15:56:38]

That's the stupidest combination I've ever heard in my life! That's the kind of
thing an idiot would have on his luggage!

[skym, 2016/10/11 16:41:00]

:)

 
 // A ChromeSyncClient that provides a ModelTypeService for PREFERENCES.
 class TestSyncClient : public ChromeSyncClient {
@@ -234,34 +237,34 @@ IN_PROC_BROWSER_TEST_F(TwoClientUssSyncTest, Sanity) {
   ASSERT_TRUE(SetupSync());
   ASSERT_EQ(2U, clients_.size());
   ASSERT_EQ(2U, services_.size());
-  TestModelTypeService* model1 = GetModelTypeService(0);
-  TestModelTypeService* model2 = GetModelTypeService(1);
+  TestModelTypeService* model0 = GetModelTypeService(0);
+  TestModelTypeService* model1 = GetModelTypeService(1);
 
   // Add an entity.
-  model1->WriteItem(kKey1, kValue1);
-  ASSERT_TRUE(DataChecker(model2, kKey1, kValue1).Wait());
+  model0->WriteItem(kKey1, kValue1);
+  ASSERT_TRUE(DataChecker(model1, kKey1, kValue1).Wait());
 
   // Update an entity.
-  model1->WriteItem(kKey1, kValue2);
-  ASSERT_TRUE(DataChecker(model2, kKey1, kValue2).Wait());
+  model0->WriteItem(kKey1, kValue2);
+  ASSERT_TRUE(DataChecker(model1, kKey1, kValue2).Wait());
 
   // Delete an entity.
-  model1->DeleteItem(kKey1);
-  ASSERT_TRUE(DataAbsentChecker(model2, kKey1).Wait());
+  model0->DeleteItem(kKey1);
+  ASSERT_TRUE(DataAbsentChecker(model1, kKey1).Wait());
 }
 
 IN_PROC_BROWSER_TEST_F(TwoClientUssSyncTest, DisableEnable) {
   ASSERT_TRUE(SetupSync());
-  TestModelTypeService* model1 = GetModelTypeService(0);
-  TestModelTypeService* model2 = GetModelTypeService(1);
+  TestModelTypeService* model0 = GetModelTypeService(0);
+  TestModelTypeService* model1 = GetModelTypeService(1);
 
   // Add an entity to test with.
-  model1->WriteItem(kKey1, kValue1);
-  ASSERT_TRUE(DataChecker(model2, kKey1, kValue1).Wait());
+  model0->WriteItem(kKey1, kValue1);
+  ASSERT_TRUE(DataChecker(model1, kKey1, kValue1).Wait());
+  ASSERT_EQ(1U, model0->db().data_count());
+  ASSERT_EQ(1U, model0->db().metadata_count());
   ASSERT_EQ(1U, model1->db().data_count());
   ASSERT_EQ(1U, model1->db().metadata_count());
-  ASSERT_EQ(1U, model2->db().data_count());
-  ASSERT_EQ(1U, model2->db().metadata_count());
 
   // Disable PREFERENCES.
   syncer::ModelTypeSet types = syncer::UserSelectableTypes();
@@ -269,60 +272,131 @@ IN_PROC_BROWSER_TEST_F(TwoClientUssSyncTest, DisableEnable) {
   GetSyncService(0)->OnUserChoseDatatypes(false, types);
 
   // Wait for it to take effect and remove the metadata.
-  ASSERT_TRUE(MetadataAbsentChecker(model1, kKey1).Wait());
-  ASSERT_EQ(1U, model1->db().data_count());
-  ASSERT_EQ(0U, model1->db().metadata_count());
+  ASSERT_TRUE(MetadataAbsentChecker(model0, kKey1).Wait());
+  ASSERT_EQ(1U, model0->db().data_count());
+  ASSERT_EQ(0U, model0->db().metadata_count());
   // Model 2 should not be affected.
-  ASSERT_EQ(1U, model2->db().data_count());
-  ASSERT_EQ(1U, model2->db().metadata_count());
+  ASSERT_EQ(1U, model1->db().data_count());
+  ASSERT_EQ(1U, model1->db().metadata_count());
 
   // Re-enable PREFERENCES.
   GetSyncService(0)->OnUserChoseDatatypes(true, syncer::UserSelectableTypes());
 
   // Wait for metadata to be re-added.
-  ASSERT_TRUE(MetadataPresentChecker(model1, kKey1).Wait());
+  ASSERT_TRUE(MetadataPresentChecker(model0, kKey1).Wait());
+  ASSERT_EQ(1U, model0->db().data_count());
+  ASSERT_EQ(1U, model0->db().metadata_count());
   ASSERT_EQ(1U, model1->db().data_count());
   ASSERT_EQ(1U, model1->db().metadata_count());
-  ASSERT_EQ(1U, model2->db().data_count());
-  ASSERT_EQ(1U, model2->db().metadata_count());
 }
 
 IN_PROC_BROWSER_TEST_F(TwoClientUssSyncTest, ConflictResolution) {
   ASSERT_TRUE(SetupSync());
-  TestModelTypeService* model1 = GetModelTypeService(0);
-  TestModelTypeService* model2 = GetModelTypeService(1);
-  model1->SetConflictResolution(ConflictResolution::UseNew(
+  TestModelTypeService* model0 = GetModelTypeService(0);
+  TestModelTypeService* model1 = GetModelTypeService(1);
+  model0->SetConflictResolution(ConflictResolution::UseNew(
       FakeModelTypeService::GenerateEntityData(kKey1, kValue3)));
-  model2->SetConflictResolution(ConflictResolution::UseNew(
+  model1->SetConflictResolution(ConflictResolution::UseNew(
       FakeModelTypeService::GenerateEntityData(kKey1, kValue3)));
 
   // Write conflicting entities.
-  model1->WriteItem(kKey1, kValue1);
-  model2->WriteItem(kKey1, kValue2);
+  model0->WriteItem(kKey1, kValue1);
+  model1->WriteItem(kKey1, kValue2);
 
   // Wait for them to be resolved to kResolutionValue by the custom conflict
   // resolution logic in TestModelTypeService.
+  ASSERT_TRUE(DataChecker(model0, kKey1, kValue3).Wait());
   ASSERT_TRUE(DataChecker(model1, kKey1, kValue3).Wait());
-  ASSERT_TRUE(DataChecker(model2, kKey1, kValue3).Wait());
 }
 
 IN_PROC_BROWSER_TEST_F(TwoClientUssSyncTest, Error) {
   ASSERT_TRUE(SetupSync());
-  TestModelTypeService* model1 = GetModelTypeService(0);
-  TestModelTypeService* model2 = GetModelTypeService(1);
+  TestModelTypeService* model0 = GetModelTypeService(0);
+  TestModelTypeService* model1 = GetModelTypeService(1);
 
   // Add an entity.
-  model1->WriteItem(kKey1, kValue1);
-  ASSERT_TRUE(DataChecker(model2, kKey1, kValue1).Wait());
+  model0->WriteItem(kKey1, kValue1);
+  ASSERT_TRUE(DataChecker(model1, kKey1, kValue1).Wait());
 
-  // Set an error in model 2 to trigger in the next GetUpdates.
-  model2->SetServiceError(syncer::SyncError::DATATYPE_ERROR);
-  // Write an item on model 1 to trigger a GetUpdates in model 2.
-  model1->WriteItem(kKey1, kValue2);
+  // Set an error in model 1 to trigger in the next GetUpdates.
+  model1->SetServiceError(syncer::SyncError::DATATYPE_ERROR);
+  // Write an item on model 0 to trigger a GetUpdates in model 2.
+  model0->WriteItem(kKey1, kValue2);
 
   // The type should stop syncing but keep tracking metadata.
   ASSERT_TRUE(PrefsNotRunningChecker(GetSyncService(1)).Wait());
-  ASSERT_EQ(1U, model2->db().metadata_count());
-  model2->WriteItem(kKey2, kValue2);
-  ASSERT_EQ(2U, model2->db().metadata_count());
+  ASSERT_EQ(1U, model1->db().metadata_count());
+  model1->WriteItem(kKey2, kValue2);
+  ASSERT_EQ(2U, model1->db().metadata_count());
+}
+
+IN_PROC_BROWSER_TEST_F(TwoClientUssSyncTest, Encryption) {
+  ASSERT_TRUE(SetupSync());
+  TestModelTypeService* model0 = GetModelTypeService(0);
+  TestModelTypeService* model1 = GetModelTypeService(1);
+
+  model0->WriteItem(kKey1, kValue1);
+  ASSERT_TRUE(DataChecker(model1, kKey1, kValue1).Wait());
+
+  GetSyncService(0)->SetEncryptionPassphrase(kPassphrase,
+                                             syncer::SyncService::EXPLICIT);
+  ASSERT_TRUE(PassphraseAcceptedChecker(GetSyncService(0)).Wait());

[maxbogue, 2016/10/11 15:56:38]

The passphrase can be not accepted?

[skym, 2016/10/11 16:41:00]

If it's async, it can before accepted, and after accepted. As for being
rejected, not explicitly but there might be a reason  we never reach the correct
state to consider it accepted. See
https://cs.chromium.org/chromium/src/chrome/browser/sync/test/integration/syn...

You could probably get a passphrase to not take if you already had one. The
client UI makes this impossible to do afaik.

Actually might be good to have an integ test around two clients simultaneously
adding conflicting passphrases, but I worry that the fake server's
implementation might not be good enough to do the right thing. Will investigate.

+  // Wait for client 1 to know that a passphrase is happening to avoid potential
+  // race conditions and make the functionality this case tests more consistent.
+  ASSERT_TRUE(PassphraseRequiredChecker(GetSyncService(1)).Wait());
+
+  model0->WriteItem(kKey1, kValue2);
+  model0->WriteItem(kKey2, kValue1);
+  model1->WriteItem(kKey3, kValue1);
+
+  ASSERT_TRUE(GetSyncService(1)->SetDecryptionPassphrase(kPassphrase));
+  ASSERT_TRUE(PassphraseAcceptedChecker(GetSyncService(1)).Wait());
+
+  model0->WriteItem(kKey4, kValue1);
+
+  ASSERT_TRUE(DataChecker(model1, kKey1, kValue2).Wait());
+  ASSERT_TRUE(DataChecker(model1, kKey3, kValue1).Wait());
+  ASSERT_TRUE(DataChecker(model1, kKey4, kValue1).Wait());
+
+  ASSERT_TRUE(DataChecker(model0, kKey1, kValue2).Wait());

[maxbogue, 2016/10/11 15:56:39]

Why do you check kKey1 and kKey3 twice but never kKey2? Maybe just check all
four on each client?

[skym, 2016/10/11 16:41:00]

Whoops, model1 should have checked key2, not key3 (line 358). Fixed.

Because we're verifying that the changes go where they're supposed to! We could
do all 8 checks but I worry that'd make it less obvious what we're testing
exactly.

+  ASSERT_TRUE(DataChecker(model0, kKey3, kValue1).Wait());
+}
+
+// TODO(skym): Rework nigori updates upon initial load so that this case no
+// longer tests anything interesting. Altenatively, if this test case is going
+// to stick around long term, we may want to rework this test case to be less
+// racy. We could potentially accomplish this by creating or re-creating client
+// 1 after we know the encrypted value of kKey1 was written to the server.
+IN_PROC_BROWSER_TEST_F(TwoClientUssSyncTest, EncryptionInitialSketchiness) {
+  ASSERT_TRUE(SetupSync());
+  TestModelTypeService* model0 = GetModelTypeService(0);
+  TestModelTypeService* model1 = GetModelTypeService(1);
+
+  GetSyncService(0)->SetEncryptionPassphrase(kPassphrase,
+                                             syncer::SyncService::EXPLICIT);
+  ASSERT_TRUE(PassphraseAcceptedChecker(GetSyncService(0)).Wait());
+
+  // Right after client 0 initializes, write an item with the new passphrase.
+  // Unfourtunately, the behavior here is kind of undefind and we're relying on

[maxbogue, 2016/10/11 15:56:38]

"Unfortunately", "undefined"

[skym, 2016/10/11 16:41:00]

Done.

+  // race conditoins to hit the logic we're trying to test. What typically

[maxbogue, 2016/10/11 15:56:39]

"conditions"

Can you make it clearer which things are racing? Also, does this test succeed
either way?

[skym, 2016/10/11 16:41:00]

Done. And I believe so, although it consistently executes on my machine. If you
put in a blocking checker between the passphrase and the WriteItem, you
basically get the TwoClientUssSyncTest.Encryption test case.

+  // happens is that client 1 is going to perform their initial download, and is
+  // going to restore/migrate nigori to its initial/default value. It will also

[maxbogue, 2016/10/11 15:56:38]

So the client 1 nigori is going from what to what? No encryption to KEYSTORE?
But not CUSTOM?

[skym, 2016/10/11 16:41:00]

I'm not sure actually. I was just printing out key names. Adding some logging
really fast to SyncEncryptionHandlerImpl::ApplyNigoriUpdateImpl I see both
PassphraseType::IMPLICIT_PASSPHRASE and PassphraseType::KEYSTORE_PASSPHRASE fly
by.

+  // be in this first GetUpdates that it retrieves the encrypted preference
+  // entity. It can correctly use the default nigori encryption, and picks up

[maxbogue, 2016/10/11 15:56:38]

Why can it use default encryption if it should be using passphrase, and why is
that correct?

[skym, 2016/10/11 16:41:00]

If it's just temporary, hasn't really mattered before. Though maybe we waste
bandwidth encrypting things and then re-encrypting them in quick succession.

+  // that preferences needs to be encrypted from the passphrase setting by
+  // client 0. But at this time the worker doesn't have the real passphrase
+  // nigori, and bizarrely it will never get it during this GetUpdates cycle. It

[maxbogue, 2016/10/11 15:56:38]

Seems like something that's broken...

[skym, 2016/10/11 16:41:00]

It does seem odd.

+  // isn't until a further GetUpdates call that the passphrase encryption key
+  // and cryptographer reaches the worker. This is particularly difficult for
+  // the worker to handle, because it cannot be certain it has the latest
+  // encryption key from looking at its cryptographer. Yes, this case should be
+  // a unit test, and it is, see ModelTypeWorkerTest.MultipleEncryptionKeys, but
+  // this test shows that this is a case that we currently need to handle.
+  model0->WriteItem(kKey1, kValue1);
+
+  ASSERT_TRUE(PassphraseRequiredChecker(GetSyncService(1)).Wait());
+  ASSERT_TRUE(GetSyncService(1)->SetDecryptionPassphrase(kPassphrase));
+  ASSERT_TRUE(PassphraseAcceptedChecker(GetSyncService(1)).Wait());
+
+  ASSERT_TRUE(DataChecker(model1, kKey1, kValue1).Wait());
 }