Supervised Users Initiated Installs v2

chrome/browser/extensions/extension_service_sync_unittest.cc

Index: chrome/browser/extensions/extension_service_sync_unittest.cc
diff --git a/chrome/browser/extensions/extension_service_sync_unittest.cc b/chrome/browser/extensions/extension_service_sync_unittest.cc
index 8e9ac8d150782cd6541ad382d8d1def162acb17d..26169159601bc473ea443b0a215bccd7d25a8ffc 100644
--- a/chrome/browser/extensions/extension_service_sync_unittest.cc
+++ b/chrome/browser/extensions/extension_service_sync_unittest.cc
@@ -11,6 +11,7 @@
 
 #include "base/bind.h"
 #include "base/command_line.h"
+#include "base/feature_list.h"
 #include "base/files/file_util.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
@@ -52,8 +53,11 @@
 #if defined(ENABLE_SUPERVISED_USERS)
 #include "chrome/browser/supervised_user/permission_request_creator.h"
 #include "chrome/browser/supervised_user/supervised_user_constants.h"
+#include "chrome/browser/supervised_user/supervised_user_features.h"
 #include "chrome/browser/supervised_user/supervised_user_service.h"
 #include "chrome/browser/supervised_user/supervised_user_service_factory.h"
+#include "chrome/browser/supervised_user/supervised_user_settings_service.h"
+#include "chrome/browser/supervised_user/supervised_user_settings_service_factory.h"
 #endif
 
 using extensions::AppSorting;
@@ -310,9 +314,9 @@ TEST_F(ExtensionServiceSyncTest, DisableExtensionFromSync) {
       base::WrapUnique(new syncer::SyncErrorFactoryMock()));
 
   // Then sync data arrives telling us to disable |good0|.
-  ExtensionSyncData disable_good_crx(*extension, false,
-                                     Extension::DISABLE_USER_ACTION, false,
-                                     false, ExtensionSyncData::BOOLEAN_UNSET);
+  ExtensionSyncData disable_good_crx(

[Marc Treib, 2016/05/23 15:32:42]

And once more: no unrelated formatting changes please

[mamir, 2016/05/23 19:35:14]

Sorry!
I made sure not to produce those but seems I overlooked some.

+      *extension, false, Extension::DISABLE_USER_ACTION, false, false,
+      ExtensionSyncData::BOOLEAN_UNSET);
   SyncChangeList list(
       1, disable_good_crx.GetSyncChange(SyncChange::ACTION_UPDATE));
   extension_sync_service()->ProcessSyncChanges(FROM_HERE, list);
@@ -442,10 +446,10 @@ TEST_F(ExtensionServiceSyncTest, DontSelfNotify) {
     ASSERT_TRUE(extension);
 
     // Add another disable reason.
-    ExtensionSyncData data(*extension, false,
-                           Extension::DISABLE_USER_ACTION |
+    ExtensionSyncData data(
+        *extension, false, Extension::DISABLE_USER_ACTION |
                                Extension::DISABLE_PERMISSIONS_INCREASE,
-                           false, false, ExtensionSyncData::BOOLEAN_UNSET);
+        false, false, ExtensionSyncData::BOOLEAN_UNSET);
     SyncChangeList list(1, data.GetSyncChange(SyncChange::ACTION_UPDATE));
 
     extension_sync_service()->ProcessSyncChanges(FROM_HERE, list);
@@ -458,10 +462,10 @@ TEST_F(ExtensionServiceSyncTest, DontSelfNotify) {
     ASSERT_TRUE(extension);
 
     // Uninstall the extension.
-    ExtensionSyncData data(*extension, false,
-                           Extension::DISABLE_USER_ACTION |
+    ExtensionSyncData data(
+        *extension, false, Extension::DISABLE_USER_ACTION |
                                Extension::DISABLE_PERMISSIONS_INCREASE,
-                           false, false, ExtensionSyncData::BOOLEAN_UNSET);
+        false, false, ExtensionSyncData::BOOLEAN_UNSET);
     SyncChangeList list(1, data.GetSyncChange(SyncChange::ACTION_DELETE));
 
     extension_sync_service()->ProcessSyncChanges(FROM_HERE, list);
@@ -769,7 +773,7 @@ TEST_F(ExtensionServiceSyncTest, GetSyncAppDataUserSettings) {
   }
 }
 
-// TODO (rdevlin.cronin): The OnExtensionMoved() method has been removed from
+// TODO(rdevlin.cronin): The OnExtensionMoved() method has been removed from
 // ExtensionService, so this test probably needs a new home. Unfortunately, it
 // relies pretty heavily on things like InitializeExtension[Sync]Service() and
 // PackAndInstallCRX(). When we clean up a bit more, this should move out.
@@ -1569,9 +1573,24 @@ class ExtensionServiceTestSupervised : public ExtensionServiceSyncTest,
         "SupervisedUserExtensionPermissionIncrease", "group", params);
   }
 
+  void InitSupervisedUserInitiatedExtensionInstallFeature(bool enabled) {
+    base::FeatureList::ClearInstanceForTesting();
+    std::unique_ptr<base::FeatureList> feature_list(new base::FeatureList);
+    if (enabled) {
+      feature_list->InitializeFromCommandLine(
+          "SupervisedUserInitiatedExtensionInstall", std::string());
+    }
+    base::FeatureList::SetInstance(std::move(feature_list));
+  }
+
   void InitServices(bool profile_is_supervised) {
     ExtensionServiceInitParams params = CreateDefaultInitParams();
     params.profile_is_supervised = profile_is_supervised;
+    // If profile is supervised, don't pass a pref file to force the testing
+    // profile to create a pref service to uses SupervisedUserPrefStore instead.
+    if (profile_is_supervised) {
+      params.pref_file = base::FilePath();
+    }
     InitializeExtensionService(params);
     StartSyncing(syncer::EXTENSIONS);
 
@@ -1608,10 +1627,10 @@ class ExtensionServiceTestSupervised : public ExtensionServiceSyncTest,
     return SupervisedUserServiceFactory::GetForProfile(profile());
   }
 
-  static std::string UpdateRequestId(const std::string& extension_id,
-                                     const std::string& version) {
-    return SupervisedUserService::GetExtensionUpdateRequestId(
-        extension_id, base::Version(version));
+  static std::string RequestId(const std::string& extension_id,
+                               const std::string& version) {
+    return SupervisedUserService::GetExtensionRequestId(extension_id,
+                                                        base::Version(version));
   }
 
  private:
@@ -1643,6 +1662,10 @@ class MockPermissionRequestCreator : public PermissionRequestCreator {
     FAIL();
   }
 
+  MOCK_METHOD2(CreateExtensionInstallRequest,
+               void(const std::string& id,
+                    const SupervisedUserService::SuccessCallback& callback));
+
   MOCK_METHOD2(CreateExtensionUpdateRequest,
                void(const std::string& id,
                     const SupervisedUserService::SuccessCallback& callback));
@@ -1653,28 +1676,94 @@ class MockPermissionRequestCreator : public PermissionRequestCreator {
 
 TEST_F(ExtensionServiceTestSupervised, InstallOnlyAllowedByCustodian) {
   InitServices(true /* profile_is_supervised */);
+  InitSupervisedUserInitiatedExtensionInstallFeature(false);
 
   base::FilePath path1 = data_dir().AppendASCII("good.crx");
   base::FilePath path2 = data_dir().AppendASCII("good2048.crx");
   const Extension* extensions[] = {
-    InstallCRX(path1, INSTALL_FAILED),
-    InstallCRX(path2, INSTALL_NEW, Extension::WAS_INSTALLED_BY_CUSTODIAN)
-  };
+      InstallCRX(path1, INSTALL_FAILED),
+      InstallCRX(path2, INSTALL_NEW, Extension::WAS_INSTALLED_BY_CUSTODIAN)};
 
   // Only the extension with the "installed by custodian" flag should have been
   // installed and enabled.
+  // While the extension missing the flag is a supervised user initiated install
+  // and hence it is not installed.
   EXPECT_FALSE(extensions[0]);
   ASSERT_TRUE(extensions[1]);
   EXPECT_TRUE(registry()->enabled_extensions().Contains(extensions[1]->id()));
 }
 
-TEST_F(ExtensionServiceTestSupervised, PreinstalledExtension) {
+TEST_F(ExtensionServiceTestSupervised,
+       InstallAllowedByCustodianAndSupervisedUser) {
+  InitServices(true /* profile_is_supervised */);
+  InitSupervisedUserInitiatedExtensionInstallFeature(true);
+
+  base::FilePath path1 = data_dir().AppendASCII("good.crx");
+  base::FilePath path2 = data_dir().AppendASCII("good2048.crx");
+  const Extension* extensions[] = {
+      InstallCRX(path1, INSTALL_WITHOUT_LOAD),
+      InstallCRX(path2, INSTALL_NEW, Extension::WAS_INSTALLED_BY_CUSTODIAN)};
+
+  // Only the extension with the "installed by custodian" flag should have been
+  // installed and enabled.
+  // The extension missing the "installed by custodian" flag is a
+  // supervised user initiated install and hence not allowed.
+  ASSERT_TRUE(extensions[0]);
+  ASSERT_TRUE(extensions[1]);
+  EXPECT_TRUE(registry()->disabled_extensions().Contains(extensions[0]->id()));
+  EXPECT_TRUE(registry()->enabled_extensions().Contains(extensions[1]->id()));
+}
+
+TEST_F(ExtensionServiceTestSupervised,
+       PreinstalledExtensionWithSUInitiatedInstallsEnabled) {
   InitServices(false /* profile_is_supervised */);
+  InitSupervisedUserInitiatedExtensionInstallFeature(true);
 
   // Install an extension.
   base::FilePath path = data_dir().AppendASCII("good.crx");
   const Extension* extension = InstallCRX(path, INSTALL_NEW);
   std::string id = extension->id();
+  // Make sure it's enabled.
+  EXPECT_TRUE(registry()->enabled_extensions().Contains(id));
+
+  MockPermissionRequestCreator* creator = new MockPermissionRequestCreator;
+  supervised_user_service()->AddPermissionRequestCreator(
+      base::WrapUnique(creator));
+  const std::string version("1.0.0.0");
+
+  EXPECT_CALL(*creator, CreateExtensionInstallRequest(
+                            RequestId(good_crx, version), testing::_));
+
+  // Now make the profile supervised.
+  profile()->AsTestingProfile()->SetSupervisedUserId(
+      supervised_users::kChildAccountSUID);
+
+  // The extension should not be enabled anymore.
+  EXPECT_FALSE(registry()->enabled_extensions().Contains(id));
+}
+
+TEST_F(ExtensionServiceTestSupervised,
+       PreinstalledExtensionWithSUInitiatedInstallsDisabled) {
+  InitServices(false /* profile_is_supervised */);
+  InitSupervisedUserInitiatedExtensionInstallFeature(false);
+
+  // Install an extension.
+  base::FilePath path = data_dir().AppendASCII("good.crx");
+  const Extension* extension = InstallCRX(path, INSTALL_NEW);
+  std::string id = extension->id();
+  // Make sure it's enabled.
+  EXPECT_TRUE(registry()->enabled_extensions().Contains(id));
+
+  MockPermissionRequestCreator* creator = new MockPermissionRequestCreator;
+  supervised_user_service()->AddPermissionRequestCreator(
+      base::WrapUnique(creator));
+  const std::string version("1.0.0.0");
+
+  // No request should be sent because supervised user initiated installs
+  // are disabled.
+  EXPECT_CALL(*creator, CreateExtensionInstallRequest(
+                            RequestId(good_crx, version), testing::_))
+      .Times(0);
 
   // Now make the profile supervised.
   profile()->AsTestingProfile()->SetSupervisedUserId(
@@ -1727,8 +1816,8 @@ TEST_F(ExtensionServiceTestSupervised, UpdateWithPermissionIncreaseNoApproval) {
   // Since we don't require the custodian's approval, no permission request
   // should be created.
   const std::string version2("2");
-  EXPECT_CALL(*creator, CreateExtensionUpdateRequest(
-                            UpdateRequestId(id, version2), testing::_))
+  EXPECT_CALL(*creator,
+              CreateExtensionUpdateRequest(RequestId(id, version2), testing::_))
       .Times(0);
   UpdatePermissionsTestExtension(id, version2, DISABLED);
 }
@@ -1749,8 +1838,8 @@ TEST_F(ExtensionServiceTestSupervised,
   std::string id = InstallPermissionsTestExtension();
 
   // Update to a new version with increased permissions.
-  EXPECT_CALL(*creator, CreateExtensionUpdateRequest(
-                            UpdateRequestId(id, version2), testing::_));
+  EXPECT_CALL(*creator, CreateExtensionUpdateRequest(RequestId(id, version2),
+                                                     testing::_));
   UpdatePermissionsTestExtension(id, version2, DISABLED);
 
   // Simulate a custodian approval for re-enabling the extension coming in
@@ -1766,8 +1855,8 @@ TEST_F(ExtensionServiceTestSupervised,
 
   // Attempting to re-enable an old version should result in a permission
   // request for the current version.
-  EXPECT_CALL(*creator, CreateExtensionUpdateRequest(
-                            UpdateRequestId(id, version2), testing::_));
+  EXPECT_CALL(*creator, CreateExtensionUpdateRequest(RequestId(id, version2),
+                                                     testing::_));
 
   SyncChangeList list =
       MakeSyncChangeList(id, specifics, SyncChange::ACTION_UPDATE);
@@ -1795,8 +1884,8 @@ TEST_F(ExtensionServiceTestSupervised,
 
   // Update to a new version with increased permissions.
   const std::string version2("2");
-  EXPECT_CALL(*creator, CreateExtensionUpdateRequest(
-                            UpdateRequestId(id, version2), testing::_));
+  EXPECT_CALL(*creator, CreateExtensionUpdateRequest(RequestId(id, version2),
+                                                     testing::_));
   UpdatePermissionsTestExtension(id, version2, DISABLED);
 
   // Simulate a custodian approval for re-enabling the extension coming in
@@ -1831,8 +1920,8 @@ TEST_F(ExtensionServiceTestSupervised,
 
   // Update to a new version with increased permissions.
   const std::string version2("2");
-  EXPECT_CALL(*creator, CreateExtensionUpdateRequest(
-                            UpdateRequestId(id, version2), testing::_));
+  EXPECT_CALL(*creator, CreateExtensionUpdateRequest(RequestId(id, version2),
+                                                     testing::_));
   UpdatePermissionsTestExtension(id, version2, DISABLED);
 
   // Simulate a custodian approval for re-enabling the extension coming in
@@ -1843,12 +1932,11 @@ TEST_F(ExtensionServiceTestSupervised,
   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);

[Marc Treib, 2016/05/23 15:32:42]

Don't remove this

[mamir, 2016/05/23 19:35:14]

Done.

   ext_specifics->set_version(version3);
 
   // This should *not* result in a new permission request.
-  EXPECT_CALL(*creator, CreateExtensionUpdateRequest(
-                            UpdateRequestId(id, version3), testing::_))
+  EXPECT_CALL(*creator,
+              CreateExtensionUpdateRequest(RequestId(id, version3), testing::_))
       .Times(0);
 
   SyncChangeList list =
@@ -1865,6 +1953,59 @@ TEST_F(ExtensionServiceTestSupervised,
   UpdatePermissionsTestExtension(id, version3, ENABLED);
 }
 
+TEST_F(ExtensionServiceTestSupervised, SupervisedUserInitiatedInstalls) {
+  InitNeedCustodianApprovalFieldTrial(true);
+  InitSupervisedUserInitiatedExtensionInstallFeature(true);
+
+  InitServices(true /* profile_is_supervised */);
+
+  MockPermissionRequestCreator* creator = new MockPermissionRequestCreator;
+  supervised_user_service()->AddPermissionRequestCreator(
+      base::WrapUnique(creator));
+
+  base::FilePath path = data_dir().AppendASCII("good.crx");
+  const std::string version("1.0.0.0");
+
+  EXPECT_CALL(*creator, CreateExtensionInstallRequest(
+                            RequestId(good_crx, version), testing::_));
+
+  // Should be installed but disabled, a request for approval should be sent.
+  const Extension* extension = InstallCRX(path, INSTALL_WITHOUT_LOAD);
+  ASSERT_EQ(extension->id(), good_crx);
+  ASSERT_TRUE(extension);
+  EXPECT_TRUE(registry()->disabled_extensions().Contains(extension->id()));
+  EXPECT_FALSE(registry()->enabled_extensions().Contains(extension->id()));
+
+  // Simulate a custodian approval for enabling the extension coming in
+  // through Sync.
+  sync_pb::EntitySpecifics specifics;
+  sync_pb::ExtensionSpecifics* ext_specifics = specifics.mutable_extension();
+  ext_specifics->set_id(extension->id());
+  ext_specifics->set_enabled(true);
+  ext_specifics->set_disable_reasons(Extension::DISABLE_NONE);
+  ext_specifics->set_version(version);
+
+  SyncChangeList list1 =
+      MakeSyncChangeList(extension->id(), specifics, SyncChange::ACTION_UPDATE);
+
+  extension_sync_service()->ProcessSyncChanges(FROM_HERE, list1);

[Marc Treib, 2016/05/23 15:32:42]

After this, we should make sure that the extension is not enabled yet.
Also add a second test, where the sync changes come in in the other order, and
make sure things work out too.
Or actually, come to think of it: This sync change shouldn't even matter, right?
If so, just make sure that the extension still has the same state, e.g. disable
reasons.

[mamir, 2016/05/23 19:35:14]

Yes this Sync change indeed shouldn't matter.  But (apart from their order)
those steps accurately simulates what happens on the server side. Because the
same approval pipeline is used for approving requests upon permission increase
and upon SU-initiated installs.
i.e. for both requests, the disable reasons as well as approved extensions are
touched.
What do you mean with "just make sure the extension still has the same state,
e.g. disable reason"?

[Marc Treib, 2016/05/24 09:54:57]

But here, the Sync change will actually be a no-op change, since the disable
reason isn't synced. Right?

So, I'm not sure if it makes sense to keep the Extension change, since it's not
actually a change at all. But if we do keep it, then we should make sure it
behaves correctly, i.e. after the change has been processed, the extension is
still disabled with REQUIRE_CUSTODIAN_APPROVAL.

[mamir, 2016/06/03 09:45:54]

Yes, it's not Synced.
I will remove it then.

+
+  std::string key = SupervisedUserSettingsService::MakeSplitSettingKey(
+      supervised_users::kContentPackApprovedExtensions, extension->id());
+  syncer::SyncData sync_data =
+      SupervisedUserSettingsService::CreateSyncDataForSetting(
+          key, base::StringValue(version));
+
+  SyncChangeList list2(
+      1, SyncChange(FROM_HERE, SyncChange::ACTION_ADD, sync_data));
+
+  SupervisedUserSettingsService* supervised_user_settings_service =
+      SupervisedUserSettingsServiceFactory::GetForProfile(profile());
+  supervised_user_settings_service->ProcessSyncChanges(FROM_HERE, list2);
+  // The extension should be enabled now.
+  EXPECT_TRUE(registry()->enabled_extensions().Contains(extension->id()));
+}
+
 TEST_F(ExtensionServiceSyncTest, SyncUninstallByCustodianSkipsPolicy) {
   InitializeEmptyExtensionService();
   extension_sync_service()->MergeDataAndStartSyncing(