chrome/browser/extensions/extension_util.cc - Issue 2004043002: Supervised Users Initiated Installs v2

Unified Diff: chrome/browser/extensions/extension_util.cc

Issue 2004043002: Supervised Users Initiated Installs v2 (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@kid_initiated_install

Patch Set: Response to code review by Devlin and fixing a test Created 4 years, 6 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« no previous file with comments | « chrome/browser/extensions/extension_sync_service.cc ('k') | chrome/browser/supervised_user/child_accounts/permission_request_creator_apiary.h » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: chrome/browser/extensions/extension_util.cc

diff --git a/chrome/browser/extensions/extension_util.cc b/chrome/browser/extensions/extension_util.cc

index 4c019be7b86784d0c95a53a011f50a5fb73532be..8ce2f4247deccb7a95f82bf494fdb1570e87f230 100644

--- a/chrome/browser/extensions/extension_util.cc

+++ b/chrome/browser/extensions/extension_util.cc

@@ -257,6 +257,7 @@ void SetWasInstalledByCustodian(const std::string& extension_id,

// 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 t

Marc Treib 2016/06/24 12:46:33 Why does MustRemainDisabled keep it disabled? If w

mamir 2016/06/24 14:43:07 hmm, this is for extension installed by custodian,

Marc Treib 2016/06/24 15:00:59 But it should be called again afterwards, no? That

Devlin 2016/06/24 16:09:39 I would have also thought that calling Reload() wo

mamir 2016/06/27 14:26:59 OK, here is how it works. The way reload works is

On 2016/06/24 16:09:39, Devlin wrote: > On 2016/06/24 15:00:59, Marc Treib wrote: > > On 2016/06/24 14:43:07, mamir wrote: > > > On 2016/06/24 12:46:33, Marc Treib wrote: > > > > On 2016/06/23 11:43:20, mamir wrote: > > > > > 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! > > > > > > > > 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. > > > > > > 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. > > > > 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. > > > > > 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. > > > > 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?

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

}