Supervised Users Initiated Installs v2

chrome/browser/extensions/extension_service_sync_unittest.cc

 // Copyright 2015 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 <stddef.h>
 
 #include <map>
 #include <memory>
 #include <string>
 #include <utility>
 
 #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"
 #include "base/memory/weak_ptr.h"
 #include "base/metrics/field_trial.h"
 #include "base/test/mock_entropy_provider.h"
 #include "chrome/browser/extensions/component_loader.h"
 #include "chrome/browser/extensions/extension_service.h"
 #include "chrome/browser/extensions/extension_service_test_with_install.h"
 #include "chrome/browser/extensions/extension_sync_data.h"
@@ skipping 21 matching lines @@
 #include "extensions/common/value_builder.h"
 #include "sync/api/fake_sync_change_processor.h"
 #include "sync/api/sync_change_processor_wrapper_for_test.h"
 #include "sync/api/sync_data.h"
 #include "sync/api/sync_error_factory_mock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 #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"
+#include "chrome/common/pref_names.h"
 #endif
 
 using extensions::AppSorting;
 using extensions::Extension;
 using extensions::ExtensionPrefs;
 using extensions::ExtensionSyncData;
 using extensions::ExtensionSystem;
 using extensions::Manifest;
 using extensions::PermissionSet;
 using syncer::SyncChange;
 using syncer::SyncChangeList;
+using testing::Mock;
 
 namespace {
 
 const char good0[] = "behllobkkfkfnphdnhnkndlbkcpglgmj";
 const char good2[] = "bjafgdebaacbbbecmhlhpofkepfkgcpa";
 const char good_crx[] = "ldnnhddmnhbkjipkidpdiheffobcpfmf";
 const char page_action[] = "obcimlgaoabeegjmmpldobjndiealpln";
 const char theme2_crx[] = "pjpgmfcmabopnnfonnhmdjglfpjjfkbf";
 
 SyncChangeList MakeSyncChangeList(const std::string& id,
@@ skipping 1484 matching lines @@
     // Group name doesn't matter.
     base::FieldTrialList::CreateFieldTrial(
         "SupervisedUserExtensionPermissionIncrease", "group");
     std::map<std::string, std::string> params;
     params["legacy_supervised_user"] = enabled ? "true" : "false";
     params["child_account"] = enabled ? "true" : "false";
     variations::AssociateVariationParams(
         "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 such that the testing
+    // profile creates a pref service that uses SupervisedUserPrefStore.
+    if (profile_is_supervised) {
+      params.pref_file = base::FilePath();
+    }
     InitializeExtensionService(params);
     StartSyncing(syncer::EXTENSIONS);
 
     supervised_user_service()->SetDelegate(this);
     supervised_user_service()->Init();
   }
 
-  std::string InstallPermissionsTestExtension() {
+  std::string InstallPermissionsTestExtension(bool by_custodian) {
     const std::string version("1");
 
-    const Extension* extension =
-        PackAndInstallCRX(dir_path(version), pem_path(), INSTALL_NEW,
-                          Extension::WAS_INSTALLED_BY_CUSTODIAN);
-    // The extension must now be installed and enabled.
+    int creation_flags = 0;
+    InstallState expected_state = INSTALL_WITHOUT_LOAD;
+    if (by_custodian) {
+      creation_flags = Extension::WAS_INSTALLED_BY_CUSTODIAN;
+      expected_state = INSTALL_NEW;
+    }
+    const Extension* extension = PackAndInstallCRX(
+        dir_path(version), pem_path(), expected_state, creation_flags);
+    // The extension must now be installed.
     EXPECT_TRUE(extension);
-    EXPECT_TRUE(registry()->enabled_extensions().Contains(extension->id()));
+
+    if (by_custodian)
+      EXPECT_TRUE(registry()->enabled_extensions().Contains(extension->id()));
+    else
+      EXPECT_TRUE(registry()->disabled_extensions().Contains(extension->id()));
+
     EXPECT_EQ(version, extension->VersionString());
 
     return extension->id();
   }
 
   void UpdatePermissionsTestExtension(const std::string& id,
                                       const std::string& version,
                                       UpdateState expected_state) {
     PackCRXAndUpdateExtension(id, dir_path(version), pem_path(),
                               expected_state);
     const Extension* extension = registry()->GetInstalledExtension(id);
     ASSERT_TRUE(extension);
     // The version should have been updated.
     EXPECT_EQ(version, extension->VersionString());
   }
 
+  // Simulate a custodian approval for enabling the extension coming in
+  // through Sync by adding the approved version to the map of approved
+  // extensions.  It doesn't change the disable reasons.

[Marc Treib, 2016/06/15 12:31:07]

Is it particularly relevant that this doesn't change disable reasons? If so,
should you check it?

nitty nit: only one space after "."

[mamir, 2016/06/15 17:30:03]

The idea is: this method is responsible only for getting a extension approval
SUSetting sync entity.  It doesn't simulate any change in the extension disable
reasons.
Makes sense?
I have rephrased the comment.

[Marc Treib, 2016/06/16 08:26:54]

Acknowledged.

+  void SimulateCustodianApprovalViaSync(const std::string& extension_id,
+                                        const std::string& version) {
+    std::string key = SupervisedUserSettingsService::MakeSplitSettingKey(
+        supervised_users::kApprovedExtensions, extension_id);
+    syncer::SyncData sync_data =
+        SupervisedUserSettingsService::CreateSyncDataForSetting(
+            key, base::StringValue(version));
+
+    SyncChangeList list(
+        1, SyncChange(FROM_HERE, SyncChange::ACTION_ADD, sync_data));
+
+    SupervisedUserSettingsService* supervised_user_settings_service =
+        SupervisedUserSettingsServiceFactory::GetForProfile(profile());
+    supervised_user_settings_service->ProcessSyncChanges(FROM_HERE, list);
+  }
+
   SupervisedUserService* supervised_user_service() {
     return SupervisedUserServiceFactory::GetForProfile(profile());
   }
 
-  static std::string UpdateRequestId(const std::string& extension_id,
-                                     const std::string& version) {
-    return SupervisedUserService::GetExtensionUpdateRequestId(
+  static std::string RequestId(const std::string& extension_id,
+                               const std::string& version) {
+    return SupervisedUserService::GetExtensionRequestId(
         extension_id, base::Version(version));
   }
 
  private:
   // This prevents the legacy supervised user init code from running.
   bool SetActive(bool active) override { return true; }
 
   base::FilePath base_path() const {
     return data_dir().AppendASCII("permissions_increase");
   }
@@ skipping 12 matching lines @@
   MockPermissionRequestCreator() {}
   ~MockPermissionRequestCreator() override {}
 
   bool IsEnabled() const override { return true; }
 
   void CreateURLAccessRequest(const GURL& url_requested,
                               const SuccessCallback& callback) override {
     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));
 
  private:
   DISALLOW_COPY_AND_ASSIGN(MockPermissionRequestCreator);
 };
 
 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)
   };
 
   // Only the extension with the "installed by custodian" flag should have been
   // installed and enabled.
   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 enabled.
+  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,
+       PreinstalledExtensionWithSUInitiatedInstalls) {
   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);
+
+  Mock::VerifyAndClearExpectations(creator);
+
+  // The extension should not be enabled anymore.
+  EXPECT_FALSE(registry()->enabled_extensions().Contains(id));
+
+  ExtensionPrefs* extension_prefs = ExtensionPrefs::Get(profile());
+  EXPECT_TRUE(extension_prefs->HasDisableReason(
+      id, extensions::Extension::DISABLE_CUSTODIAN_APPROVAL_REQUIRED));
+}
+
+TEST_F(ExtensionServiceTestSupervised,
+       PreinstalledExtensionWithoutSUInitiatedInstalls) {
+  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::_))

[Marc Treib, 2016/06/15 12:31:07]

I'd just use testing::_ instead of an explicit request id - we don't expect any
request at all.

[mamir, 2016/06/15 17:30:03]

Done.

+      .Times(0);
+
+  // 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));
+
+  extensions::ExtensionPrefs* extension_prefs =
+      extensions::ExtensionPrefs::Get(profile());
+  EXPECT_TRUE(extension_prefs->HasDisableReason(
+      id, extensions::Extension::DISABLE_CUSTODIAN_APPROVAL_REQUIRED));
+}
+
+TEST_F(ExtensionServiceTestSupervised,
+       ExtensionApprovalBeforeInstallation) {

[Marc Treib, 2016/06/15 12:31:07]

nit: fits on the previous line I think

[mamir, 2016/06/15 17:30:03]

Done.

+  // This tests the case when the sync entity flagging the extension as approved
+  // arrives before the extension itself is installed.
+  InitServices(true /* profile_is_supervised */);
+  InitSupervisedUserInitiatedExtensionInstallFeature(true);
+
+  MockPermissionRequestCreator* creator = new MockPermissionRequestCreator;
+  supervised_user_service()->AddPermissionRequestCreator(
+      base::WrapUnique(creator));
+
+  std::string id = good_crx;
+  const std::string version("1.0.0.0");

[Marc Treib, 2016/06/15 12:31:07]

nit: Why is one const and the other isn't? (Don't really care which, but please
be consistent)

[mamir, 2016/06/15 17:30:03]

Done.

+
+  SimulateCustodianApprovalViaSync(id, version);
+
+  // Now install an extension.
+  base::FilePath path = data_dir().AppendASCII("good.crx");
+  const Extension* extension = InstallCRX(path, INSTALL_NEW);
+
+  // No approval request should be sent.
+  EXPECT_CALL(*creator, CreateExtensionInstallRequest(
+                            RequestId(extension->id(), version), testing::_))
+      .Times(0);
+
+  // Make sure it's enabled.
+  EXPECT_TRUE(registry()->enabled_extensions().Contains(id));
 }
 
 TEST_F(ExtensionServiceTestSupervised, UpdateWithoutPermissionIncrease) {
   InitServices(true /* profile_is_supervised */);
 
   base::FilePath base_path = data_dir().AppendASCII("autoupdate");
   base::FilePath pem_path = base_path.AppendASCII("key.pem");
 
   const Extension* extension =
       PackAndInstallCRX(base_path.AppendASCII("v1"), pem_path, INSTALL_NEW,
@@ skipping 19 matching lines @@
 
 TEST_F(ExtensionServiceTestSupervised, UpdateWithPermissionIncreaseNoApproval) {
   InitNeedCustodianApprovalFieldTrial(false);
 
   InitServices(true /* profile_is_supervised */);
 
   MockPermissionRequestCreator* creator = new MockPermissionRequestCreator;
   supervised_user_service()->AddPermissionRequestCreator(
       base::WrapUnique(creator));
 
-  std::string id = InstallPermissionsTestExtension();
+  std::string id = InstallPermissionsTestExtension(true /* by_custodian */);
 
   // Update to a new version with increased permissions.
   // 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::_))
+                            RequestId(id, version2), testing::_))
       .Times(0);
   UpdatePermissionsTestExtension(id, version2, DISABLED);
 }
 
 TEST_F(ExtensionServiceTestSupervised,
        UpdateWithPermissionIncreaseApprovalOldVersion) {
   InitNeedCustodianApprovalFieldTrial(true);
 
   InitServices(true /* profile_is_supervised */);
 
   MockPermissionRequestCreator* creator = new MockPermissionRequestCreator;
   supervised_user_service()->AddPermissionRequestCreator(
       base::WrapUnique(creator));
 
   const std::string version1("1");
   const std::string version2("2");
 
-  std::string id = InstallPermissionsTestExtension();
+  std::string id = InstallPermissionsTestExtension(true /* by_custodian */);
 
   // Update to a new version with increased permissions.
   EXPECT_CALL(*creator, CreateExtensionUpdateRequest(
-                            UpdateRequestId(id, version2), testing::_));
+                            RequestId(id, version2), testing::_));
   UpdatePermissionsTestExtension(id, version2, DISABLED);
+  Mock::VerifyAndClearExpectations(creator);
 
   // Simulate a custodian approval for re-enabling the extension coming in
   // through Sync, 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(
-                            UpdateRequestId(id, version2), testing::_));
+                            RequestId(id, version2), testing::_));
 
   SyncChangeList list =
       MakeSyncChangeList(id, specifics, SyncChange::ACTION_UPDATE);
 
   extension_sync_service()->ProcessSyncChanges(FROM_HERE, list);
   // The re-enable should be ignored, since the version doesn't match.
   EXPECT_FALSE(registry()->enabled_extensions().Contains(id));
   EXPECT_FALSE(extension_sync_service()->HasPendingReenable(
       id, base::Version(version1)));
   EXPECT_FALSE(extension_sync_service()->HasPendingReenable(
       id, base::Version(version2)));
+  Mock::VerifyAndClearExpectations(creator);
 }
 
 TEST_F(ExtensionServiceTestSupervised,
        UpdateWithPermissionIncreaseApprovalMatchingVersion) {
   InitNeedCustodianApprovalFieldTrial(true);
 
   InitServices(true /* profile_is_supervised */);
 
   MockPermissionRequestCreator* creator = new MockPermissionRequestCreator;
   supervised_user_service()->AddPermissionRequestCreator(
       base::WrapUnique(creator));
 
-  std::string id = InstallPermissionsTestExtension();
+  std::string id = InstallPermissionsTestExtension(true /* by_custodian */);
 
   // Update to a new version with increased permissions.
   const std::string version2("2");
   EXPECT_CALL(*creator, CreateExtensionUpdateRequest(
-                            UpdateRequestId(id, version2), testing::_));
+                            RequestId(id, version2), testing::_));
   UpdatePermissionsTestExtension(id, version2, DISABLED);
+  Mock::VerifyAndClearExpectations(creator);
 
   // Simulate a custodian approval for re-enabling the extension coming in
   // through Sync.
   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(version2);
 
   SyncChangeList list =
       MakeSyncChangeList(id, specifics, SyncChange::ACTION_UPDATE);
 
   extension_sync_service()->ProcessSyncChanges(FROM_HERE, list);
   // The extension should have gotten re-enabled.
   EXPECT_TRUE(registry()->enabled_extensions().Contains(id));
 }
 
 TEST_F(ExtensionServiceTestSupervised,
        UpdateWithPermissionIncreaseApprovalNewVersion) {
   InitNeedCustodianApprovalFieldTrial(true);
 
   InitServices(true /* profile_is_supervised */);
 
   MockPermissionRequestCreator* creator = new MockPermissionRequestCreator;
   supervised_user_service()->AddPermissionRequestCreator(
       base::WrapUnique(creator));
 
-  std::string id = InstallPermissionsTestExtension();
+  std::string id = InstallPermissionsTestExtension(true /* by_custodian */);
 
   // Update to a new version with increased permissions.
   const std::string version2("2");
   EXPECT_CALL(*creator, CreateExtensionUpdateRequest(
-                            UpdateRequestId(id, version2), testing::_));
+                            RequestId(id, version2), testing::_));
   UpdatePermissionsTestExtension(id, version2, DISABLED);
+  Mock::VerifyAndClearExpectations(creator);
 
   // Simulate a custodian approval for re-enabling the extension coming in
   // through Sync. Set a newer version than we have installed.
   const std::string version3("3");
   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(version3);
 
   // This should *not* result in a new permission request.
   EXPECT_CALL(*creator, CreateExtensionUpdateRequest(
-                            UpdateRequestId(id, version3), testing::_))
+                            RequestId(id, version3), testing::_))
       .Times(0);
 
   SyncChangeList list =
       MakeSyncChangeList(id, specifics, SyncChange::ACTION_UPDATE);
 
   extension_sync_service()->ProcessSyncChanges(FROM_HERE, list);
   // The re-enable should be delayed until the extension is updated to the
   // matching version.
   EXPECT_FALSE(registry()->enabled_extensions().Contains(id));
   EXPECT_TRUE(extension_sync_service()->HasPendingReenable(
       id, base::Version(version3)));
 
   // Update to the matching version. Now the extension should get enabled.
   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()));
+  Mock::VerifyAndClearExpectations(creator);
+
+  SimulateCustodianApprovalViaSync(extension->id(), version);
+
+  // The extension should be enabled now.
+  EXPECT_TRUE(registry()->enabled_extensions().Contains(extension->id()));
+}
+
+TEST_F(ExtensionServiceTestSupervised,
+       UpdateSUInitiatedInstallWithoutPermissionIncrease) {
+  InitNeedCustodianApprovalFieldTrial(true);
+  InitSupervisedUserInitiatedExtensionInstallFeature(true);
+
+  InitServices(true /* profile_is_supervised */);
+
+  base::FilePath base_path = data_dir().AppendASCII("autoupdate");
+  base::FilePath pem_path = base_path.AppendASCII("key.pem");
+
+  const Extension* extension = PackAndInstallCRX(
+      base_path.AppendASCII("v1"), pem_path, INSTALL_WITHOUT_LOAD);
+
+  ASSERT_TRUE(extension);
+  EXPECT_FALSE(registry()->enabled_extensions().Contains(extension->id()));
+
+  SimulateCustodianApprovalViaSync(extension->id(), extension->VersionString());
+
+  // The extension should be enabled now.
+  EXPECT_TRUE(registry()->enabled_extensions().Contains(extension->id()));
+
+  // Save the id, as the extension object will be destroyed during updating.
+  std::string id = extension->id();
+
+  const base::Version old_version = *extension->version();
+
+  // Update to a new version.
+  PackCRXAndUpdateExtension(id, base_path.AppendASCII("v2"), pem_path, ENABLED);
+
+  // The extension should still be there and enabled.
+  extension = registry()->enabled_extensions().GetByID(id);
+  ASSERT_TRUE(extension);
+  // The version should have increased.
+  EXPECT_EQ(1, extension->version()->CompareTo(old_version));
+
+  // Check that the approved version has been updated in the prefs as well.
+  // Prefs are updated via Sync.  If the prefs are updated, then the new
+  // approved version has been pushed to Sync as well.
+  std::string approved_version;
+  PrefService* pref_service = profile()->GetPrefs();
+  const base::DictionaryValue* approved_extensions =
+      pref_service->GetDictionary(prefs::kSupervisedUserApprovedExtensions);
+  approved_extensions->GetStringWithoutPathExpansion(id, &approved_version);
+
+  EXPECT_EQ(0,
+            extension->version()->CompareTo(base::Version(approved_version)));
+}
+
+TEST_F(ExtensionServiceTestSupervised,
+       UpdateSUInitiatedInstallWithPermissionIncrease) {

[Marc Treib, 2016/06/15 12:31:07]

This test name suggests that it's the counterpart to the above one, but the code
is actually very different. Why?

[mamir, 2016/06/15 17:30:03]

Because they test different flows.
The first tests this flow:
1- kid initiates an install.
2- custodian approves it.
3- extension updates without permission increase.
4- approved version automatically updates.

The second tests this flow:
1- kid initiates an install.
2- custodian approves it.
3- extension updates with permission increase.
4- extension is disabled.
The rest of the flow is already thoroughly covered in multiple other test cases
(matching version, non-matching version ...etc.)

[Marc Treib, 2016/06/16 08:26:54]

So, steps 1 and 2 are identical, and step 3 is identical except for the content
of the extension. Yet the code looks very different. That's confusing at best.

[mamir, 2016/06/17 15:03:18]

As discussed offline, in order to unify both tests, I have to touch the 
existing similar tests for custodian installed extension.
I will unify all test for custodian installed and supervised user initiated
installed for both no permission increase and permission increase.

+  InitNeedCustodianApprovalFieldTrial(true);
+  InitSupervisedUserInitiatedExtensionInstallFeature(true);
+
+  InitServices(true /* profile_is_supervised */);
+
+  std::string id = InstallPermissionsTestExtension(false /* by_custodian */);
+  const std::string version("1");

[Marc Treib, 2016/06/15 12:31:07]

Also here: please try to be consistent with consts

[mamir, 2016/06/15 17:30:03]

Done.

+
+  SimulateCustodianApprovalViaSync(id, version);
+
+  // The extension should be enabled now.
+  EXPECT_TRUE(registry()->enabled_extensions().Contains(id));
+
+  const std::string version2("2");
+
+  UpdatePermissionsTestExtension(id, version2, DISABLED);
+
+  // The extension should be disabled.
+  EXPECT_FALSE(registry()->enabled_extensions().Contains(id));

[Marc Treib, 2016/06/15 12:31:07]

Also check the disable reasons?

[mamir, 2016/06/15 17:30:03]

Done.

+}

[Marc Treib, 2016/06/15 12:31:07]

Please extend the test with an incoming approval after the update and make sure
the extension gets re-enabled. Also extend the test (or add new ones) for other
cases:
- Approval with mismatching version (extension should remain disabled).
- Approval comes in before the extension update (extension should be temporarily
disabled in the meantime).

[mamir, 2016/06/15 17:30:03]

But this would be duplicating other tests covering the same situation for
cusotidan installed extension.
IIUC, the only difference is in the initial steps of how we reach this stage of
an extension disabled due to permission increase.

[Marc Treib, 2016/06/16 08:26:54]

The way that approval and re-enabling work are very different for
custodian-installed vs. SU-initiated, so the tests should cover both. In
particular, for SU-initiated installs, there are a few variations that simply
don't exist in the custodian-installed case, e.g. whether the extension update
or the approval arrives first.

[mamir, 2016/06/17 15:03:17]

Done.

+
 TEST_F(ExtensionServiceSyncTest, SyncUninstallByCustodianSkipsPolicy) {
   InitializeEmptyExtensionService();
   extension_sync_service()->MergeDataAndStartSyncing(
       syncer::EXTENSIONS, syncer::SyncDataList(),
       base::WrapUnique(new syncer::FakeSyncChangeProcessor()),
       base::WrapUnique(new syncer::SyncErrorFactoryMock()));
 
   // Install two extensions.
   base::FilePath path1 = data_dir().AppendASCII("good.crx");
   base::FilePath path2 = data_dir().AppendASCII("good2048.crx");
@@ skipping 176 matching lines @@
         break;
       }
     }
   }
   EXPECT_TRUE(found_delete);
 
   // Make sure there is one extension, and there are no more apps.
   EXPECT_EQ(1u, extensions_processor.data().size());
   EXPECT_TRUE(apps_processor.data().empty());
 }