Supervised Users Initiated Installs v2

chrome/browser/extensions/extension_util.cc

 // Copyright 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 "chrome/browser/extensions/extension_util.h"
 
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/metrics/field_trial.h"
 #include "base/values.h"
@@ skipping 218 matching lines @@
 void SetWasInstalledByCustodian(const std::string& extension_id,
                                 content::BrowserContext* context,
                                 bool installed_by_custodian) {
   if (installed_by_custodian == WasInstalledByCustodian(extension_id, context))
     return;
 
   ExtensionPrefs::Get(context)->UpdateExtensionPref(
       extension_id, kWasInstalledByCustodianPrefName,
       installed_by_custodian ? new base::FundamentalValue(true) : nullptr);
   ExtensionRegistry* registry = ExtensionRegistry::Get(context);
   const Extension* extension = registry->GetInstalledExtension(extension_id);

[Marc Treib, 2016/06/24 12:46:33]

nit: Move getting registry and extension down to where you actually need it.

[mamir, 2016/06/24 14:43:07]

Done.

   ExtensionService* service =
       ExtensionSystem::Get(context)->extension_service();
 
   // If the installed_by_custodian flag is reset, do nothing.

[Marc Treib, 2016/06/24 12:46:33]

Inaccurate, you *do* do something.

[mamir, 2016/06/24 14:43:07]

Done.

   if (!installed_by_custodian) {
     // If installed_by_custodian changes to false, the extension may need to
     // be unloaded now.
     service->ReloadExtension(extension_id);
     return;
   }
 
   // If it is already enabled, do nothing.
   if (registry->enabled_extensions().Contains(extension_id))
     return;
 
   // If the extension is not loaded, it may need to be reloaded.
   // Example is a pre-installed extension that was unloaded when a
   // supervised user flag has been received.
   if (!extension) {
     service->ReloadExtension(extension_id);
+    service->EnableExtension(extension_id);

[mamir, 2016/06/23 11:43:20]

Marc, Devlin,
I had to add this call because the test
ExtensionServiceTestSupervised.DelegatedAndPreinstalledExtensionIsSUFirst 
was failing.

The reason is we now added MustRemainDisabled call back to Superviseduser
Service, which is returning false before reloading the extension, and doesn't
get called during reloading the extension.
So even after reloading, the extension stayed disabled.

Is it fine like that?  I don't think so.
Advise, please!

[Marc Treib, 2016/06/24 12:46:33]

Why does MustRemainDisabled keep it disabled? If we have a matching approval,
then the state should be ExtensionState::ALLOWED and everything should be fine?
In any case, just unconditionally enabling here is definitely not the way to go.

[mamir, 2016/06/24 14:43:07]

hmm, this is for extension installed by custodian, right?
So it should actually return FORCED.

After adding log statements, MustRemainDisabled  is called before calling
SetWasInstalledByCustodian.  And hence it reads the old value and returns true. 
This flags the extension to stay disabled even after we reload it.

I completely agree that we cannot just unconditionally enable it.  And I hence
asked for input in case you have better ways in mind.  Otherwise, I will keep
investigating. 

[Marc Treib, 2016/06/24 15:00:59]

But it should be called again afterwards, no? That's the whole reason we're
calling ReloadExtensions here after changing the pref.
Clearly we're missing something, but from the top of my head I can't tell what.

[Devlin, 2016/06/24 16:09:39]

I would have also thought that calling Reload() would put it in the proper
state.  Have you checked that the supervised user service is hit during the
reload call, or if not, why not?

[mamir, 2016/06/27 14:26:59]

OK, here is how it works.
The way reload works is it disables the extension if required.
But doesn't do the opposite.
This is done at InstalledLoader::Load.  If the extension is enabled, and the
policy::MustRemainDisabled is true, it disables the extension, not the other way
around.
enableExtension fixed it because it does exactly what we need, if the extension
is disabled and the policy::MustRemainDisabled is false, it enables it.
However, after further investigation I think the issue is we both unloaded and
disabled BLOCKED extensions when we got the information this is a Supervised
User who isn't allowed to install apps.
Instead, we should only unload it, not disable it.
To support this hypothesis, if we disable the extension, we need to assign it a
disable reasons.
What would be a suitable disable reason for a BLOCKED extension?
So, I adjusted the code and added a comment in the
SUService::MustRemainDisabled.
All tests pass now

   }
 }
 
 bool WasInstalledByCustodian(const std::string& extension_id,
                              content::BrowserContext* context) {
   bool installed_by_custodian = false;
   ExtensionPrefs* prefs = ExtensionPrefs::Get(context);
   prefs->ReadPrefAsBoolean(extension_id, kWasInstalledByCustodianPrefName,
                            &installed_by_custodian);
   return installed_by_custodian;
@@ skipping 169 matching lines @@
   base::FieldTrialList::FindFullName(
       kSupervisedUserExtensionPermissionIncreaseFieldTrialName);
   std::string value = variations::GetVariationParamValue(
       kSupervisedUserExtensionPermissionIncreaseFieldTrialName,
       profile->IsChild() ? "child_account" : "legacy_supervised_user");
   return value == "true";
 }
 
 }  // namespace util
 }  // namespace extensions