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

chrome/browser/extensions/extension_service_unittest.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <algorithm>
 #include <set>
 #include <vector>
 
 #include "base/at_exit.h"
 #include "base/basictypes.h"
@@ skipping 7175 matching lines @@
   // The extension should still be there and enabled.
   extension = registry()->enabled_extensions().GetByID(id);
   ASSERT_TRUE(extension);
   // The version should have changed.
   EXPECT_NE(extension->VersionString(), old_version);
 }
 
 // Helper class that allows us to parameterize the UpdateWithPermissionIncrease
 // test over |bool need_custodian_approval|.
 class ExtensionServiceTestSupervisedUserPermissionIncrease :
     public ExtensionServiceTest, public testing::WithParamInterface<bool> {};

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

Parameterized tests?!?!?  What is this black magic??

... nifty.

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

In this case, it's arguably more trouble than it's worth... but oh well. It
works :)

 
 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(
       "SupervisedUserExtensionPermissionIncrease",
       need_custodian_approval ? "NeedCustodianApproval" : "");
 
   ExtensionServiceInitParams params = CreateDefaultInitParams();
   params.profile_is_supervised = true;
   InitializeExtensionService(params);
 
   SupervisedUserService* supervised_user_service =
       SupervisedUserServiceFactory::GetForProfile(profile());
   ScopedSupervisedUserServiceDelegate delegate(supervised_user_service);
   supervised_user_service->Init();
   MockPermissionRequestCreator* creator = new MockPermissionRequestCreator;
   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");
 
   base::FilePath path = base_path.AppendASCII("v1");
   const Extension* extension =
       PackAndInstallCRX(path, pem_path, INSTALL_NEW,
                         Extension::WAS_INSTALLED_BY_CUSTODIAN);
   // 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);
 
   // The extension should still be there, but disabled.
   EXPECT_FALSE(registry()->enabled_extensions().Contains(id));
   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,
                         testing::Bool());
 
 TEST_F(ExtensionServiceTest,
        SupervisedUserSyncUninstallByCustodianSkipsPolicy) {
   InitializeEmptyExtensionService();
   extension_sync_service()->MergeDataAndStartSyncing(
       syncer::EXTENSIONS,
@@ skipping 967 matching lines @@
 
   service()->Observe(chrome::NOTIFICATION_PROFILE_DESTRUCTION_STARTED,
                      content::Source<Profile>(profile()),
                      content::NotificationService::NoDetails());
   EXPECT_EQ(UnloadedExtensionInfo::REASON_PROFILE_SHUTDOWN, unloaded_reason_);
   EXPECT_EQ(0u, registry()->enabled_extensions().size());
   EXPECT_EQ(0u, registry()->disabled_extensions().size());
   EXPECT_EQ(0u, registry()->terminated_extensions().size());
   EXPECT_EQ(0u, registry()->blacklisted_extensions().size());
 }