Apps&Extensions for Supervised Users: send permission request on outdated re-enables

chrome/browser/extensions/extension_service_unittest.cc

Index: chrome/browser/extensions/extension_service_unittest.cc
diff --git a/chrome/browser/extensions/extension_service_unittest.cc b/chrome/browser/extensions/extension_service_unittest.cc
index 00fcdc74fb103ab186cf71c0966cb0dffeef3257..d4c8c7d7a23486edfb749e582f0bafaa675063c4 100644
--- a/chrome/browser/extensions/extension_service_unittest.cc
+++ b/chrome/browser/extensions/extension_service_unittest.cc
@@ -7197,6 +7197,12 @@ class ExtensionServiceTestSupervisedUserPermissionIncrease :
 
 TEST_P(ExtensionServiceTestSupervisedUserPermissionIncrease,
        UpdateWithPermissionIncrease) {
+  // This is the update URL specified in the test extension. Setting it here is
+  // necessary to make the extension considered syncable.
+  base::CommandLine::ForCurrentProcess()->AppendSwitchASCII(
+      switches::kAppsGalleryUpdateURL,
+      "http://localhost/autoupdate/updates.xml");
+
   bool need_custodian_approval = GetParam();
   base::FieldTrialList field_trial_list(new base::MockEntropyProvider());
   base::FieldTrialList::CreateFieldTrial(
@@ -7215,6 +7221,10 @@ TEST_P(ExtensionServiceTestSupervisedUserPermissionIncrease,
   supervised_user_service->AddPermissionRequestCreator(
       make_scoped_ptr(creator));
 
+  const std::string version1("1");
+  const std::string version2("2");
+  const std::string version3("3");
+
   base::FilePath base_path = data_dir().AppendASCII("permissions_increase");
   base::FilePath pem_path = base_path.AppendASCII("permissions.pem");
 
@@ -7225,15 +7235,14 @@ TEST_P(ExtensionServiceTestSupervisedUserPermissionIncrease,
   // The extension must now be installed and enabled.
   ASSERT_TRUE(extension);
   ASSERT_TRUE(registry()->enabled_extensions().Contains(extension->id()));
+  ASSERT_EQ(version1, extension->VersionString());
 
   // Save the id, as the extension object will be destroyed during updating.
   std::string id = extension->id();
 
-  std::string old_version = extension->VersionString();
-
   // Update to a new version with increased permissions.
   EXPECT_CALL(*creator,
-              CreateExtensionUpdateRequest(id + ":2", testing::_))
+              CreateExtensionUpdateRequest(id + ":" + version2, testing::_))
       .Times(need_custodian_approval ? 1 : 0);
   path = base_path.AppendASCII("v2");
   PackCRXAndUpdateExtension(id, path, pem_path, DISABLED);
@@ -7243,7 +7252,64 @@ TEST_P(ExtensionServiceTestSupervisedUserPermissionIncrease,
   extension = registry()->disabled_extensions().GetByID(id);
   ASSERT_TRUE(extension);
   // The version should have changed.
-  EXPECT_NE(extension->VersionString(), old_version);
+  EXPECT_EQ(version2, extension->VersionString());
+
+  if (!need_custodian_approval)

[Devlin, 2015/10/16 02:40:24]

What has this succeeded in testing in this case?  Would it be more valuable to
check other branches and see the expected outcome?  (What *is* the expected
outcome?)

[Marc Treib, 2015/10/16 09:22:20]

The extension was updated but disabled, and (in particular) a permission request
was sent iff we need custodian approval.
What "other branches"?

+    return;
+
+  // Now, simulate custodian approvals for re-enabling the extension coming in
+  // through Sync.
+
+  // Create a sync update to re-enable the extension, but set the old version.
+  // This can happen when there already was a pending request for an earlier
+  // version of the extension.
+  sync_pb::EntitySpecifics specifics;
+  sync_pb::ExtensionSpecifics* ext_specifics = specifics.mutable_extension();
+  ext_specifics->set_id(id);
+  ext_specifics->set_enabled(true);
+  ext_specifics->set_disable_reasons(Extension::DISABLE_NONE);
+  ext_specifics->set_installed_by_custodian(true);
+  ext_specifics->set_version(version1);
+  {
+    // Attempting to re-enable an old version should result in a permission
+    // request for the current version.
+    EXPECT_CALL(*creator,
+                CreateExtensionUpdateRequest(id + ":" + version2, testing::_));
+
+    syncer::SyncData sync_data =
+        syncer::SyncData::CreateLocalData(id, "Name", specifics);
+    syncer::SyncChange sync_change(FROM_HERE,
+                                   syncer::SyncChange::ACTION_UPDATE,
+                                   sync_data);
+    syncer::SyncChangeList change_list(1, sync_change);
+    extension_sync_service()->ProcessSyncChanges(FROM_HERE, change_list);
+    // The re-enable should be ignored, since the version doesn't match.
+    EXPECT_FALSE(registry()->enabled_extensions().Contains(id));
+  }
+
+  // Again, but now set a newer version than what is installed.
+  ext_specifics->set_version(version3);
+  {
+    // This should *not* result in a new permission request.
+    EXPECT_CALL(*creator,
+                CreateExtensionUpdateRequest(id + ":" + version3, testing::_))
+        .Times(0);
+
+    syncer::SyncData sync_data =
+        syncer::SyncData::CreateLocalData(id, "Name", specifics);
+    syncer::SyncChange sync_change(FROM_HERE,
+                                   syncer::SyncChange::ACTION_UPDATE,
+                                   sync_data);
+    syncer::SyncChangeList change_list(1, sync_change);
+    extension_sync_service()->ProcessSyncChanges(FROM_HERE, change_list);
+    // The re-enable should be delayed until the extension is updated to the

[Devlin, 2015/10/16 02:40:24]

Do we have a test for it being enabled when the version does match?

[Marc Treib, 2015/10/16 09:22:20]

Good point, we don't :D
I'll add one and ping you again. (I think I'll have to split this test into
multiple ones. Oh well.)

+    // matching version.
+    EXPECT_FALSE(registry()->enabled_extensions().Contains(id));
+  }
+
+  // Update to the matching version. Now the extension should get enabled.
+  path = base_path.AppendASCII("v3");

[Devlin, 2015/10/16 02:40:24]

nit: inline path (everywhere)

[Marc Treib, 2015/10/16 09:22:20]

Done.

+  PackCRXAndUpdateExtension(id, path, pem_path, ENABLED);
 }
 INSTANTIATE_TEST_CASE_P(NeedCustodianApproval,
                         ExtensionServiceTestSupervisedUserPermissionIncrease,