Issue 2004043002: Supervised Users Initiated Installs v2

mamir

Description was changed from ========== Fixing style Fixing the tests Fixing the format It compiles ...

4 years, 7 months ago (2016-05-23 14:45:05 UTC) #1

Description was changed from

==========
Fixing style


Fixing the tests


Fixing the format


It compiles now


Adding approved extensions to supervised user settings


Removing "is_approved_by_custodian" flag.


minor fix


SetIsApprovedByCustodian should never unset the flag


Fixing two typos


Response to seventh round of comment by treib


Style


Response to sixth round of comments by treib


FREEZE.unindexed


formating.


Response to Marc fifth round of comments and fixing after rebase


FREEZE.unindexed


git squash commit.

0ab7fe868446c104081cfef0abf046d41df6b958
[WIP - DO NOT SUBMIT] SU-initiated extension installs

patch from issue 1663023003 at patchset 1 (http://crrev.com/1663023003#ps1)

ea7c4d69a7ab6089b8583c94bb920d4ced12f54f
Implementing virtual methods in PermissionRequestCreator

fb12917a27f41879cf9ca0c024c43fd7edc85025
fixing some unit tests

227c994e7c38a6bee14f7e8d5722e1c066cbc346
formatting

e74a7e070bfb4bb99ec8c099f0bf474e5f516fa8
testing kid-initiated installs flow

58984bbac4292608b5810959d55f18240aaf9a02
Fixing style

731f390db51d033d010b85fac71ba25aa29317b7
Hiding supervised user initiaied installs behind a feature

4433c5845b0a7de1595306c027fbee788f4bd312
Fixing test afer the supervised user initiated installs feature flag

ea6d7407345c1673ca69def4c9df6c76c9c9d597
Fixing test afer the supervised user initiated installs feature flag

ee25280c2b98d26fa5b80775bd088d09357aa6b9
Fixing test afer the supervised user initiated installs feature flag

3283afe876e1b0ff51a9a85c736d1f7cca955cc0
is_approved_by_custodian flag instead of disable_reasons bit

7c807fa3ac40e18055c9c3d1adecdbcc5549c453
Adding is_approved_by_custodian flag and keeping the
oldpending_approval_disable_reason

6d4180cfacb7d35cc88928f29493c18630007e2b
fixing more tests

f36ede5c9f9cd499fa8b2a04d5a9e640c5d59d49
All tests fixed

ffb0f9343ffcb99d8faccc7e955cf41ad23adacb
All tests fixed

b660dc67604d8c2a45d3d65c71d3e127bc880dd7
All test actually pass now

ca9dc738567cb2bf77d02af3ab9ca5f3c07bda50
Response to first round of comment from treib@

7a3b274ffcc6a7de9f73cae6e99e9769be60061a
Fixing the style.

ff368483ec97128fce649ef5b8dc88d9a0cbb091
Response to second round of comments from treib@

19b1b6a8eacc6582980e9d018bbf8f702f38aa65
Fixing the style.

1522c76e5d5848e31dd97dfc087e19f6770bdc37
Response to third round of comments from treib@

2866a4c1877f5b80539fed5e368f15826434aee1
Fixing the style.

6c41af51616e7fcffa83d32887046c9f5ef943e4
Response to fourth round of comments from treib@

914747e0f7437a80661b1298c2cee304be43bc13
Fixing the style.

BUG=
==========

to

==========
BUG=
==========

Marc Treib

https://codereview.chromium.org/2004043002/diff/20001/chrome/browser/extensions/extension_service_sync_unittest.cc File chrome/browser/extensions/extension_service_sync_unittest.cc (left): https://codereview.chromium.org/2004043002/diff/20001/chrome/browser/extensions/extension_service_sync_unittest.cc#oldcode1846 chrome/browser/extensions/extension_service_sync_unittest.cc:1846: ext_specifics->set_installed_by_custodian(true); Don't remove this https://codereview.chromium.org/2004043002/diff/20001/chrome/browser/extensions/extension_service_sync_unittest.cc File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2004043002/diff/20001/chrome/browser/extensions/extension_service_sync_unittest.cc#newcode317 ...

4 years, 7 months ago (2016-05-23 15:32:43 UTC) #4

mamir

Thank you Marc for useful comments. I think there is something wrong with the formatting, ...

4 years, 7 months ago (2016-05-23 19:35:15 UTC) #5

Marc Treib

On 2016/05/23 19:35:15, mamir wrote: > Thank you Marc for useful comments. > I think ...

4 years, 7 months ago (2016-05-24 08:28:09 UTC) #6

Marc Treib

https://codereview.chromium.org/2004043002/diff/20001/chrome/browser/extensions/extension_service_sync_unittest.cc File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2004043002/diff/20001/chrome/browser/extensions/extension_service_sync_unittest.cc#newcode1991 chrome/browser/extensions/extension_service_sync_unittest.cc:1991: extension_sync_service()->ProcessSyncChanges(FROM_HERE, list1); On 2016/05/23 19:35:14, mamir wrote: > On ...

4 years, 7 months ago (2016-05-24 09:54:58 UTC) #7

mamir

https://codereview.chromium.org/2004043002/diff/20001/chrome/browser/extensions/extension_service_sync_unittest.cc File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2004043002/diff/20001/chrome/browser/extensions/extension_service_sync_unittest.cc#newcode1991 chrome/browser/extensions/extension_service_sync_unittest.cc:1991: extension_sync_service()->ProcessSyncChanges(FROM_HERE, list1); On 2016/05/24 09:54:57, Marc Treib wrote: > ...

4 years, 6 months ago (2016-06-03 09:45:55 UTC) #8

https://codereview.chromium.org/2004043002/diff/20001/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_sync_unittest.cc (right):

https://codereview.chromium.org/2004043002/diff/20001/chrome/browser/extensio...
chrome/browser/extensions/extension_service_sync_unittest.cc:1991:
extension_sync_service()->ProcessSyncChanges(FROM_HERE, list1);
On 2016/05/24 09:54:57, Marc Treib wrote:
> On 2016/05/23 19:35:14, mamir wrote:
> > On 2016/05/23 15:32:42, Marc Treib wrote:
> > > 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.
> > 
> > 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"?
> 
> 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.

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

https://codereview.chromium.org/2004043002/diff/20001/chrome/browser/supervis...
File chrome/browser/supervised_user/permission_request_creator.h (right):

https://codereview.chromium.org/2004043002/diff/20001/chrome/browser/supervis...
chrome/browser/supervised_user/permission_request_creator.h:31: // Creates a
request to re-enable the extension with the given |id| (composed
On 2016/05/24 09:54:57, Marc Treib wrote:
> On 2016/05/23 19:35:14, mamir wrote:
> > On 2016/05/23 15:32:42, Marc Treib wrote:
> > > Update comment
> > 
> > Which part exactly you aren't happy with?
> 
> The text is exactly the same as for update requests below. In particular,
> "re-enable" doesn't really make sense here.

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/extensio...
File chrome/browser/extensions/extension_service_sync_unittest.cc (right):

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/extensio...
chrome/browser/extensions/extension_service_sync_unittest.cc:1590: // profile to
create a pref service to uses SupervisedUserPrefStore instead.
On 2016/05/24 09:54:57, Marc Treib wrote:
> some comment nits:
> You're not *forcing* the testing profile to do anything by not passing a pref
> file path.
> 
> s/to uses/that uses/ ?
> 
> Also it's not really using the SUPrefStore *instead* of anything.

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/extensio...
chrome/browser/extensions/extension_service_sync_unittest.cc:1690: // While the
extension missing the flag is a supervised user initiated install
On 2016/05/24 09:54:57, Marc Treib wrote:
> not a complete sentence :)
> TBH I'd just drop this comment - in this test, there are no SU-initiated
> installs

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/extensio...
chrome/browser/extensions/extension_service_sync_unittest.cc:1711: // supervised
user initiated install and hence not allowed.
On 2016/05/24 09:54:57, Marc Treib wrote:
> "not allowed" isn't really accurate - installing is allowed, but enabling
isn't

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/extensio...
chrome/browser/extensions/extension_service_sync_unittest.cc:1719:
PreinstalledExtensionWithSUInitiatedInstallsEnabled) {
On 2016/05/24 09:54:57, Marc Treib wrote:
> just PreinstalledExtensionWithSUInitiatedInstalls?

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/extensio...
chrome/browser/extensions/extension_service_sync_unittest.cc:1743:
EXPECT_FALSE(registry()->enabled_extensions().Contains(id));
On 2016/05/24 09:54:57, Marc Treib wrote:
> Also check that it has the proper disable reason?

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/extensio...
chrome/browser/extensions/extension_service_sync_unittest.cc:1747:
PreinstalledExtensionWithSUInitiatedInstallsDisabled) {
On 2016/05/24 09:54:57, Marc Treib wrote:
> PreinstalledExtensionWithoutSUInitiatedInstalls?

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/extensio...
chrome/browser/extensions/extension_service_sync_unittest.cc:1774:
EXPECT_FALSE(registry()->enabled_extensions().Contains(id));
On 2016/05/24 09:54:58, Marc Treib wrote:
> Also here: What should the disable reasons be?

It also should have the same reason DISABLE_CUSTODIAN_APPROVAL_REQUIRED.
The only difference is not approval request is sent.

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/extensio...
chrome/browser/extensions/extension_service_sync_unittest.cc:1980: 
On 2016/05/24 09:54:57, Marc Treib wrote:
> Something I learned recently: You can do
> Mock::VerifyAndClearExpectations(creator);
> to make sure the EXPECT_CALL has actually happened at this point.

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_service.cc:921:
std::map<std::string, base::Version> approved_extensions_map;
On 2016/05/24 09:54:58, Marc Treib wrote:
> Why the separate variable? Can't you just approved_extensions_map_.clear() and
> then add stuff directly into approved_extensions_map_?

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_service.cc:926:
approved_extensions_map[it.key()] = base::Version(version);
On 2016/05/24 09:54:58, Marc Treib wrote:
> Check that the version is valid before using it.

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_service.cc:935: std::string
extension_id = extensions_entry.first;
On 2016/05/24 09:54:58, Marc Treib wrote:
> const std::string& ?

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_service.cc:941:
service->EnableExtension(extension_id);
On 2016/05/24 09:54:58, Marc Treib wrote:
> This will get called even if the extension is already enabled. That *should*
not
> hurt, but is still not very nice.
> Also, if there *are* other disable reasons, you still want to remove
> DISABLE_CUSTODIAN_APPROVAL_REQUIRED. So what I'd do is: remove
> DISABLE_CUSTODIAN_APPROVAL_REQUIRED, then check if any disable reasons are
left.

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_service.cc:1022: 
On 2016/05/24 09:54:58, Marc Treib wrote:
> nit: extra empty line

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_service.cc:1026: if
(extension_prefs->GetInstalledExtensionInfo(id)
On 2016/05/24 09:54:58, Marc Treib wrote:
> Is this check needed?

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_service.cc:1085: const
extensions::Extension* extension,
On 2016/05/24 09:54:58, Marc Treib wrote:
> optional: You could add
> using extensions::Extension;
> above (plus a few more for ExtensionPrefs etc) and save a whole lot of
> "extensions::" here.

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_service.h (right):

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_service.h:228:
ChangesSyncSessionStateOnChangedSettings);
On 2016/05/24 09:54:58, Marc Treib wrote:
> Any reason for adding these unrelated tests here?

Nop :-)
I think they came here during a rebase.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_service.h:329: // BLOCKED:
Extension is blocked if it is not ALLOWED or FORCED
On 2016/05/24 09:54:58, Marc Treib wrote:
> remove "Extension is blocked" to be consistent with the other case
descriptions

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/supervis...
File chrome/browser/supervised_user/supervised_user_service_unittest.cc (right):

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_service_unittest.cc:53: using
extensions::Extension;
On 2016/05/24 09:54:58, Marc Treib wrote:
> This should probably go into the ENABLE_EXTENSIONS block above

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_service_unittest.cc:460:
ExtensionManagementPolicyProviderWithDisabledSUInitiatedInstalls) {
On 2016/05/24 09:54:58, Marc Treib wrote:
> ExtensionManagementPolicyProviderWithoutSUInitiatedInstalls?

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_service_unittest.cc:516:
ExtensionManagementPolicyProviderWithEnabledSUInitiatedInstalls) {
On 2016/05/24 09:54:58, Marc Treib wrote:
> ExtensionManagementPolicyProviderWithSUInitiatedInstalls?

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/supervis...
chrome/browser/supervised_user/supervised_user_service_unittest.cc:540: &reason,
On 2016/05/24 09:54:58, Marc Treib wrote:
> misaligned, also below

Done.

https://codereview.chromium.org/2004043002/diff/20002/chrome/test/base/testin...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/2004043002/diff/20002/chrome/test/base/testin...
chrome/test/base/testing_profile.cc:425: #if defined(ENABLE_SUPERVISED_USERS)
On 2016/05/24 09:54:58, Marc Treib wrote:
> Is it necessary to move this up here?
> It used to happen after
> browser_context_dependency_manager_->CreateBrowserContextServicesForTest,
doing
> it before that seems like it might mess things up...

Yes, it is.
Because CreatePrefServiceForSupervisedUser() requires the
SupervisedUserSettingsService to be initialized (specifically to have a pref
store)

Marc Treib

https://codereview.chromium.org/2004043002/diff/20002/chrome/test/base/testing_profile.cc File chrome/test/base/testing_profile.cc (right): https://codereview.chromium.org/2004043002/diff/20002/chrome/test/base/testing_profile.cc#newcode425 chrome/test/base/testing_profile.cc:425: #if defined(ENABLE_SUPERVISED_USERS) On 2016/06/03 09:45:55, mamir wrote: > On ...

4 years, 6 months ago (2016-06-03 13:24:14 UTC) #9

mamir

https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/supervised_user/supervised_user_service.h File chrome/browser/supervised_user/supervised_user_service.h (right): https://codereview.chromium.org/2004043002/diff/20002/chrome/browser/supervised_user/supervised_user_service.h#newcode430 chrome/browser/supervised_user/supervised_user_service.h:430: // Stores a map from extension_id -> latest approved ...

4 years, 6 months ago (2016-06-06 15:01:37 UTC) #10

Marc Treib

https://codereview.chromium.org/2004043002/diff/20002/chrome/test/base/testing_profile.cc File chrome/test/base/testing_profile.cc (right): https://codereview.chromium.org/2004043002/diff/20002/chrome/test/base/testing_profile.cc#newcode425 chrome/test/base/testing_profile.cc:425: #if defined(ENABLE_SUPERVISED_USERS) On 2016/06/06 15:01:36, mamir wrote: > On ...

4 years, 6 months ago (2016-06-07 10:27:04 UTC) #11

mamir

mamir@chromium.org changed reviewers: + rdevlin.cronin@chromium.org

4 years, 6 months ago (2016-06-07 17:00:06 UTC) #12

mamir

rdevlin.cronin@chromium.org: Please review this CL and give me your feedback. Thank you. https://codereview.chromium.org/2004043002/diff/160001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc ...

4 years, 6 months ago (2016-06-07 17:00:07 UTC) #13

Marc Treib

Please add a CL description and bug number. https://codereview.chromium.org/2004043002/diff/180001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2004043002/diff/180001/chrome/browser/supervised_user/supervised_user_service.cc#newcode285 chrome/browser/supervised_user/supervised_user_service.cc:285: approved_extensions_map_[extension_id] ...

4 years, 6 months ago (2016-06-08 12:08:25 UTC) #14

mamir

Description was changed from ========== BUG= ========== to ========== Supervised users can installs apps and ...

4 years, 6 months ago (2016-06-09 11:35:07 UTC) #15

mamir

https://codereview.chromium.org/2004043002/diff/180001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2004043002/diff/180001/chrome/browser/supervised_user/supervised_user_service.cc#newcode285 chrome/browser/supervised_user/supervised_user_service.cc:285: approved_extensions_map_[extension_id] = version; On 2016/06/08 12:08:24, Marc Treib wrote: ...

4 years, 6 months ago (2016-06-09 12:14:11 UTC) #16

Marc Treib

https://codereview.chromium.org/2004043002/diff/180001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2004043002/diff/180001/chrome/browser/supervised_user/supervised_user_service.cc#newcode285 chrome/browser/supervised_user/supervised_user_service.cc:285: approved_extensions_map_[extension_id] = version; On 2016/06/09 12:14:10, mamir wrote: > ...

4 years, 6 months ago (2016-06-09 13:44:09 UTC) #17

mamir

https://codereview.chromium.org/2004043002/diff/180001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2004043002/diff/180001/chrome/browser/supervised_user/supervised_user_service.cc#newcode285 chrome/browser/supervised_user/supervised_user_service.cc:285: approved_extensions_map_[extension_id] = version; On 2016/06/09 13:44:09, Marc Treib wrote: ...

4 years, 6 months ago (2016-06-09 14:04:40 UTC) #18

https://codereview.chromium.org/2004043002/diff/180001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/2004043002/diff/180001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:285:
approved_extensions_map_[extension_id] = version;
On 2016/06/09 13:44:09, Marc Treib wrote:
> On 2016/06/09 12:14:10, mamir wrote:
> > On 2016/06/08 12:08:24, Marc Treib wrote:
> > > Hm, should probably make sure that the approved version can never get
> smaller?
> > 
> > omm, well, probably yes.
> > But I don't think we should check here.
> > Let's keep this method for always updating the approved version.
> > I will check before calling it.
> 
> Hm, UpdateApprovedExtensionVersion is called in only one place, might as well
> inline it there. That removes the question about where to check the version.
> Also, right now we have UpdateApprovedExtensions and
> UpdateApprovedExtensionVersion - they sound similar, but do very different
> things.

Done.

https://codereview.chromium.org/2004043002/diff/180001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_settings_service.cc (right):

https://codereview.chromium.org/2004043002/diff/180001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_settings_service.cc:172:
dict->SetWithoutPathExpansion(key_suffix, value.release());
On 2016/06/08 12:08:25, Marc Treib wrote:
> Not your doing, but could you use std::move(value) instead of value.release()?
> The raw-pointer version of SetWithoutPathExpansion is deprecated.

Done.

https://codereview.chromium.org/2004043002/diff/220001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/2004043002/diff/220001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:1012: id,
Extension::DISABLE_PERMISSIONS_INCREASE)
On 2016/06/09 13:44:09, Marc Treib wrote:
> wrong indent?

According to eclipse, not.  According to git cl format, yes. ;-)
Done.

Marc Treib

https://codereview.chromium.org/2004043002/diff/240001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2004043002/diff/240001/chrome/browser/supervised_user/supervised_user_service.cc#newcode296 chrome/browser/supervised_user/supervised_user_service.cc:296: service->EnableExtension(extension_id); Ah, so you remove the CUSTODIAN_APPROVAL_REQUIRED reason unconditionally, ...

4 years, 6 months ago (2016-06-09 15:47:36 UTC) #19

mamir

https://codereview.chromium.org/2004043002/diff/240001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2004043002/diff/240001/chrome/browser/supervised_user/supervised_user_service.cc#newcode296 chrome/browser/supervised_user/supervised_user_service.cc:296: service->EnableExtension(extension_id); On 2016/06/09 15:47:36, Marc Treib wrote: > Ah, ...

4 years, 6 months ago (2016-06-09 15:58:36 UTC) #20

Devlin

Still catching up after being away for a couple days, and probably want to take ...

4 years, 6 months ago (2016-06-10 00:09:44 UTC) #21

Still catching up after being away for a couple days, and probably want to take
another look at the tests, but a few things I noticed.

https://codereview.chromium.org/2004043002/diff/260001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service_sync_unittest.cc (right):

https://codereview.chromium.org/2004043002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:1735:
InstallCRX(path2, INSTALL_NEW, Extension::WAS_INSTALLED_BY_CUSTODIAN)};
nitty nit: I think style would be to have closing bracket on a newline (if the
opening bracket isn't on the same line as an item).

https://codereview.chromium.org/2004043002/diff/260001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/2004043002/diff/260001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:254: callback, 0);
git cl format

https://codereview.chromium.org/2004043002/diff/260001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:291: // from
service->EnableExtension() below) will add it back if necessary.
This is true, but is pretty inefficient.  Is there a reason to not do something
like:
EnableExtensionIfAllowed(id) {
  if (GetExtensionState() in [BLOCKED, REQUIRE_APPROVAL])
    return;  // Extension must remain disabled
  if (!prefs->HasDisableReason(APPROVAL_REQUIRED))
    return;  // Disabled for other reasons
  prefs->RemoveDisableReason(APPROVAL_REQUIRED);
  if (!prefs->GetDisableReasons())
    service->EnableExtension()
}

It seems a little more straight forward, and less likely to cause something
wacky with the intersection of policy providers.

As an example of something wacky to avoid, what happens if:
Extension A is disabled with approval required
We call into here and remove the disable reason
We enter EnableExtension()
Another PolicyProvider (let's say blacklist) is asked *first* and says the
extension must remain disabled.

The extension will be disabled with reason blacklist, but not reason approval
required.  Obviously, slim chance of this happening (it would have to be added
to the blacklist after being disabled, etc), but removing disable reasons and
hoping we catch it just sounds risky to me in general.

https://codereview.chromium.org/2004043002/diff/260001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:546:
extensions::ExtensionRegistry::Get(profile)->AddObserver(this);
Don't we need to remove this when it's dtored?  (Note: ScopedObservers are great
for this.)

mamir

Thank you very much Devlin for your useful feedback/comments. https://codereview.chromium.org/2004043002/diff/260001/chrome/browser/extensions/extension_service_sync_unittest.cc File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2004043002/diff/260001/chrome/browser/extensions/extension_service_sync_unittest.cc#newcode1735 chrome/browser/extensions/extension_service_sync_unittest.cc:1735: ...

4 years, 6 months ago (2016-06-10 11:04:46 UTC) #22

Thank you very much Devlin for your useful feedback/comments.

https://codereview.chromium.org/2004043002/diff/260001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service_sync_unittest.cc (right):

https://codereview.chromium.org/2004043002/diff/260001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:1735:
InstallCRX(path2, INSTALL_NEW, Extension::WAS_INSTALLED_BY_CUSTODIAN)};
On 2016/06/10 00:09:44, Devlin wrote:
> nitty nit: I think style would be to have closing bracket on a newline (if the
> opening bracket isn't on the same line as an item).

Done.

https://codereview.chromium.org/2004043002/diff/260001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/2004043002/diff/260001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:254: callback, 0);
On 2016/06/10 00:09:44, Devlin wrote:
> git cl format

git cl format is fine with this part.
Do you have any specific concern?

https://codereview.chromium.org/2004043002/diff/260001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:291: // from
service->EnableExtension() below) will add it back if necessary.
On 2016/06/10 00:09:44, Devlin wrote:
> This is true, but is pretty inefficient.  Is there a reason to not do
something
> like:
> EnableExtensionIfAllowed(id) {
>   if (GetExtensionState() in [BLOCKED, REQUIRE_APPROVAL])
>     return;  // Extension must remain disabled
>   if (!prefs->HasDisableReason(APPROVAL_REQUIRED))
>     return;  // Disabled for other reasons
>   prefs->RemoveDisableReason(APPROVAL_REQUIRED);
>   if (!prefs->GetDisableReasons())
>     service->EnableExtension()
> }
> 
> It seems a little more straight forward, and less likely to cause something
> wacky with the intersection of policy providers.
> 
> As an example of something wacky to avoid, what happens if:
> Extension A is disabled with approval required
> We call into here and remove the disable reason
> We enter EnableExtension()
> Another PolicyProvider (let's say blacklist) is asked *first* and says the
> extension must remain disabled.
> 
> The extension will be disabled with reason blacklist, but not reason approval
> required.  Obviously, slim chance of this happening (it would have to be added
> to the blacklist after being disabled, etc), but removing disable reasons and
> hoping we catch it just sounds risky to me in general.

Done.

https://codereview.chromium.org/2004043002/diff/260001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:546:
extensions::ExtensionRegistry::Get(profile)->AddObserver(this);
On 2016/06/10 00:09:44, Devlin wrote:
> Don't we need to remove this when it's dtored?  (Note: ScopedObservers are
great
> for this.)

Done.

Devlin

Mostly just nits https://codereview.chromium.org/2004043002/diff/280001/chrome/browser/extensions/extension_service_sync_unittest.cc File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2004043002/diff/280001/chrome/browser/extensions/extension_service_sync_unittest.cc#newcode1777 chrome/browser/extensions/extension_service_sync_unittest.cc:1777: extensions::ExtensionPrefs* extension_prefs = no extensions:: prefix ...

4 years, 6 months ago (2016-06-13 15:37:23 UTC) #23

Devlin

Please also update your CL description: [Title] [Description] BUG=[bug] Each wrapped to 72 chars. I ...

4 years, 6 months ago (2016-06-13 15:38:15 UTC) #24

mamir

Description was changed from ========== Supervised users can installs apps and extensions and send to ...

4 years, 6 months ago (2016-06-13 16:18:09 UTC) #25

mamir

https://codereview.chromium.org/2004043002/diff/280001/chrome/browser/extensions/extension_service_sync_unittest.cc File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2004043002/diff/280001/chrome/browser/extensions/extension_service_sync_unittest.cc#newcode1777 chrome/browser/extensions/extension_service_sync_unittest.cc:1777: extensions::ExtensionPrefs* extension_prefs = On 2016/06/13 15:37:22, Devlin wrote: > ...

4 years, 6 months ago (2016-06-13 17:06:26 UTC) #26

Marc Treib

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervised_user/supervised_user_service.cc#newcode53 chrome/browser/supervised_user/supervised_user_service.cc:53: #include "extensions/browser/extension_registry.h" This needs to go into the #ifdef ...

4 years, 6 months ago (2016-06-14 13:53:35 UTC) #27

Devlin

Apart from Marc's comments, I don't see anything here. But I'll take another look once ...

4 years, 6 months ago (2016-06-15 00:25:11 UTC) #28

mamir

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervised_user/supervised_user_service.cc#newcode53 chrome/browser/supervised_user/supervised_user_service.cc:53: #include "extensions/browser/extension_registry.h" On 2016/06/14 13:53:34, Marc Treib wrote: > ...

4 years, 6 months ago (2016-06-15 09:40:11 UTC) #29

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:53: #include
"extensions/browser/extension_registry.h"
On 2016/06/14 13:53:34, Marc Treib wrote:
> This needs to go into the #ifdef ENABLE_EXTENSIONS below

Done.

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:75: #include
"chrome/browser/supervised_user/supervised_user_service_factory.h"
On 2016/06/14 13:53:34, Marc Treib wrote:
> This, OTOH, can go out into the regular includes. (Not feeling too strongly,
but
> IMO it's less confusing that way)

Done.

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:90: using
extensions::ExtensionSystem;
On 2016/06/14 13:53:34, Marc Treib wrote:
> These also need to go into an #ifdef

Done.

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:286: const Extension*
extension = registry_->GetInstalledExtension(extension_id);
On 2016/06/14 13:53:34, Marc Treib wrote:
> Can extension be null? Seems like it could be, if we get an approval before
it's
> actually installed/loaded?
> (If that's correct, then our test coverage is clearly not where it should be
yet
> ;P)

Fixed and added a proper unit test ;-)
Done!

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:304: ==
Extension::DISABLE_NONE) {
On 2016/06/14 13:53:34, Marc Treib wrote:
> Is this correctly aligned?

Done.

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:306: // properly
enable the extension if possible.
On 2016/06/14 13:53:34, Marc Treib wrote:
> This comment seems out of place now - we've already checked that our
> ManagementPolicy implementation doesn't want to keep it disabled.

The comment is intended to refer to other ManagementPolicies.
Still seems out of place?

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:1082:
extension_it->second != *(extension.version())) {
On 2016/06/14 13:53:34, Marc Treib wrote:
> nit: parens around extension.version() aren't necessary

Done.

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:1158: *reason =
Extension::DISABLE_PERMISSIONS_INCREASE;
On 2016/06/14 13:53:34, Marc Treib wrote:
> You probably also need to set error here?

Done.

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:1162: *reason =
Extension::DISABLE_CUSTODIAN_APPROVAL_REQUIRED;
On 2016/06/14 13:53:34, Marc Treib wrote:
> Hm, this is confusing. Can we ever get here if SU-initiated installs are not
> enabled?

Not really, because if SU-initiated installs are not enabled, the extensions
will be blocked and "UserMayLoad" will return false.
(I assume MustRemainDisabled is called only for loaded extensions)

Marc Treib

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervised_user/supervised_user_service.cc#newcode304 chrome/browser/supervised_user/supervised_user_service.cc:304: == Extension::DISABLE_NONE) { On 2016/06/15 09:40:11, mamir wrote: > ...

4 years, 6 months ago (2016-06-15 12:31:08 UTC) #30

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:304: ==
Extension::DISABLE_NONE) {
On 2016/06/15 09:40:11, mamir wrote:
> On 2016/06/14 13:53:34, Marc Treib wrote:
> > Is this correctly aligned?
> 
> Done.

git cl formatted now? Still looks weird, but oh well.

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:306: // properly
enable the extension if possible.
On 2016/06/15 09:40:11, mamir wrote:
> On 2016/06/14 13:53:34, Marc Treib wrote:
> > This comment seems out of place now - we've already checked that our
> > ManagementPolicy implementation doesn't want to keep it disabled.
> 
> The comment is intended to refer to other ManagementPolicies.
> Still seems out of place?

Yes, because it's not really relevant - we don't expect any interference from
other policies. Of course it could still happen, but that's just a property of
EnableExtension. I don't see a reason to specifically call it out here.

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:1162: *reason =
Extension::DISABLE_CUSTODIAN_APPROVAL_REQUIRED;
On 2016/06/15 09:40:11, mamir wrote:
> On 2016/06/14 13:53:34, Marc Treib wrote:
> > Hm, this is confusing. Can we ever get here if SU-initiated installs are not
> > enabled?
> 
> Not really, because if SU-initiated installs are not enabled, the extensions
> will be blocked and "UserMayLoad" will return false.
> (I assume MustRemainDisabled is called only for loaded extensions)

If it can't happen, then the code shouldn't handle it. Add a DCHECK instead?
(Not sure in what circumstances MustRemainDisabled can be called..)

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service_sync_unittest.cc (right):

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:1640: //
extensions.  It doesn't change the disable reasons.
Is it particularly relevant that this doesn't change disable reasons? If so,
should you check it?

nitty nit: only one space after "."

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:1802:
RequestId(good_crx, version), testing::_))
I'd just use testing::_ instead of an explicit request id - we don't expect any
request at all.

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:1819:
ExtensionApprovalBeforeInstallation) {
nit: fits on the previous line I think

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:1830: const
std::string version("1.0.0.0");
nit: Why is one const and the other isn't? (Don't really care which, but please
be consistent)

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:2111:
UpdateSUInitiatedInstallWithPermissionIncrease) {
This test name suggests that it's the counterpart to the above one, but the code
is actually very different. Why?

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:2118: const
std::string version("1");
Also here: please try to be consistent with consts

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:2130:
EXPECT_FALSE(registry()->enabled_extensions().Contains(id));
Also check the disable reasons?

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:2131: }
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).

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:287:
extensions::ExtensionRegistry* registry_ =
This should be registry (no trailing underscore).
optional: You could add a "using" declaration above, so you don't have to say
"extensions::", like for the other things in that namespace

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:1167: if (error)
nit: braces if the body doesn't fit on one line.

mamir

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervised_user/supervised_user_service.cc#newcode304 chrome/browser/supervised_user/supervised_user_service.cc:304: == Extension::DISABLE_NONE) { On 2016/06/15 12:31:07, Marc Treib wrote: ...

4 years, 6 months ago (2016-06-15 17:30:03 UTC) #31

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:304: ==
Extension::DISABLE_NONE) {
On 2016/06/15 12:31:07, Marc Treib wrote:
> On 2016/06/15 09:40:11, mamir wrote:
> > On 2016/06/14 13:53:34, Marc Treib wrote:
> > > Is this correctly aligned?
> > 
> > Done.
> 
> git cl formatted now? Still looks weird, but oh well.

Yup :-)

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:306: // properly
enable the extension if possible.
On 2016/06/15 12:31:07, Marc Treib wrote:
> On 2016/06/15 09:40:11, mamir wrote:
> > On 2016/06/14 13:53:34, Marc Treib wrote:
> > > This comment seems out of place now - we've already checked that our
> > > ManagementPolicy implementation doesn't want to keep it disabled.
> > 
> > The comment is intended to refer to other ManagementPolicies.
> > Still seems out of place?
> 
> Yes, because it's not really relevant - we don't expect any interference from
> other policies. Of course it could still happen, but that's just a property of
> EnableExtension. I don't see a reason to specifically call it out here.

Done.

https://codereview.chromium.org/2004043002/diff/300001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:1162: *reason =
Extension::DISABLE_CUSTODIAN_APPROVAL_REQUIRED;
On 2016/06/15 12:31:07, Marc Treib wrote:
> On 2016/06/15 09:40:11, mamir wrote:
> > On 2016/06/14 13:53:34, Marc Treib wrote:
> > > Hm, this is confusing. Can we ever get here if SU-initiated installs are
not
> > > enabled?
> > 
> > Not really, because if SU-initiated installs are not enabled, the extensions
> > will be blocked and "UserMayLoad" will return false.
> > (I assume MustRemainDisabled is called only for loaded extensions)
> 
> If it can't happen, then the code shouldn't handle it. Add a DCHECK instead?
> (Not sure in what circumstances MustRemainDisabled can be called..)

Sorry, I was wrong.
It can actually reach this part for pre-installed extensions if SU-initiated
installs aren't enabled.

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service_sync_unittest.cc (right):

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:1640: //
extensions.  It doesn't change the disable reasons.
On 2016/06/15 12:31:07, Marc Treib wrote:
> Is it particularly relevant that this doesn't change disable reasons? If so,
> should you check it?
> 
> nitty nit: only one space after "."

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.

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:1802:
RequestId(good_crx, version), testing::_))
On 2016/06/15 12:31:07, Marc Treib wrote:
> I'd just use testing::_ instead of an explicit request id - we don't expect
any
> request at all.

Done.

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:1819:
ExtensionApprovalBeforeInstallation) {
On 2016/06/15 12:31:07, Marc Treib wrote:
> nit: fits on the previous line I think

Done.

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:1830: const
std::string version("1.0.0.0");
On 2016/06/15 12:31:07, Marc Treib wrote:
> nit: Why is one const and the other isn't? (Don't really care which, but
please
> be consistent)

Done.

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:2111:
UpdateSUInitiatedInstallWithPermissionIncrease) {
On 2016/06/15 12:31:07, Marc Treib wrote:
> This test name suggests that it's the counterpart to the above one, but the
code
> is actually very different. Why?

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.)

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:2118: const
std::string version("1");
On 2016/06/15 12:31:07, Marc Treib wrote:
> Also here: please try to be consistent with consts

Done.

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:2130:
EXPECT_FALSE(registry()->enabled_extensions().Contains(id));
On 2016/06/15 12:31:07, Marc Treib wrote:
> Also check the disable reasons?

Done.

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:2131: }
On 2016/06/15 12:31:07, Marc Treib wrote:
> 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).

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.

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:287:
extensions::ExtensionRegistry* registry_ =
On 2016/06/15 12:31:07, Marc Treib wrote:
> This should be registry (no trailing underscore).
> optional: You could add a "using" declaration above, so you don't have to say
> "extensions::", like for the other things in that namespace

Done.

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:1167: if (error)
On 2016/06/15 12:31:07, Marc Treib wrote:
> nit: braces if the body doesn't fit on one line.

Done.

Marc Treib

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensions/extension_service_sync_unittest.cc File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensions/extension_service_sync_unittest.cc#newcode1640 chrome/browser/extensions/extension_service_sync_unittest.cc:1640: // extensions. It doesn't change the disable reasons. On ...

4 years, 6 months ago (2016-06-16 08:26:54 UTC) #32

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service_sync_unittest.cc (right):

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:1640: //
extensions.  It doesn't change the disable reasons.
On 2016/06/15 17:30:03, mamir wrote:
> On 2016/06/15 12:31:07, Marc Treib wrote:
> > Is it particularly relevant that this doesn't change disable reasons? If so,
> > should you check it?
> > 
> > nitty nit: only one space after "."
> 
> 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.

Acknowledged.

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:2111:
UpdateSUInitiatedInstallWithPermissionIncrease) {
On 2016/06/15 17:30:03, mamir wrote:
> On 2016/06/15 12:31:07, Marc Treib wrote:
> > This test name suggests that it's the counterpart to the above one, but the
> code
> > is actually very different. Why?
> 
> 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.)
> 

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.

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:2131: }
On 2016/06/15 17:30:03, mamir wrote:
> On 2016/06/15 12:31:07, Marc Treib wrote:
> > 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).
> 
> 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.

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

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensions/extension_service_sync_unittest.cc File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensions/extension_service_sync_unittest.cc#newcode2111 chrome/browser/extensions/extension_service_sync_unittest.cc:2111: UpdateSUInitiatedInstallWithPermissionIncrease) { On 2016/06/16 08:26:54, Marc Treib wrote: > ...

4 years, 6 months ago (2016-06-17 15:03:18 UTC) #33

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service_sync_unittest.cc (right):

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:2111:
UpdateSUInitiatedInstallWithPermissionIncrease) {
On 2016/06/16 08:26:54, Marc Treib wrote:
> On 2016/06/15 17:30:03, mamir wrote:
> > On 2016/06/15 12:31:07, Marc Treib wrote:
> > > This test name suggests that it's the counterpart to the above one, but
the
> > code
> > > is actually very different. Why?
> > 
> > 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.)
> > 
> 
> 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.

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.

https://codereview.chromium.org/2004043002/diff/320001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:2131: }
On 2016/06/16 08:26:54, Marc Treib wrote:
> On 2016/06/15 17:30:03, mamir wrote:
> > On 2016/06/15 12:31:07, Marc Treib wrote:
> > > 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).
> > 
> > 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.
> 
> 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.

Done.

https://codereview.chromium.org/2004043002/diff/360001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service_sync_unittest.cc (left):

https://codereview.chromium.org/2004043002/diff/360001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:1690:
base::FilePath base_path = data_dir().AppendASCII("autoupdate");
As discussed offline with Marc, we decided to unify tests with/without
permission increase for both custodian installed and kid-initiated installs. 
Hence touching this part of the code for custodian-installed extensions.

Marc Treib

https://codereview.chromium.org/2004043002/diff/360001/chrome/browser/extensions/extension_service_sync_unittest.cc File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2004043002/diff/360001/chrome/browser/extensions/extension_service_sync_unittest.cc#newcode1644 chrome/browser/extensions/extension_service_sync_unittest.cc:1644: int creation_flags = 0; Ah, so this will now ...

4 years, 6 months ago (2016-06-17 16:55:13 UTC) #34

mamir

https://codereview.chromium.org/2004043002/diff/360001/chrome/browser/extensions/extension_service_sync_unittest.cc File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2004043002/diff/360001/chrome/browser/extensions/extension_service_sync_unittest.cc#newcode1644 chrome/browser/extensions/extension_service_sync_unittest.cc:1644: int creation_flags = 0; On 2016/06/17 16:55:12, Marc Treib ...

4 years, 6 months ago (2016-06-19 12:40:14 UTC) #35

Marc Treib

https://codereview.chromium.org/2004043002/diff/360001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2004043002/diff/360001/chrome/browser/supervised_user/supervised_user_service.cc#newcode302 chrome/browser/supervised_user/supervised_user_service.cc:302: if (state == ExtensionState::BLOCKED || On 2016/06/19 12:40:14, mamir ...

4 years, 6 months ago (2016-06-20 12:44:19 UTC) #36

mamir

https://codereview.chromium.org/2004043002/diff/360001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2004043002/diff/360001/chrome/browser/supervised_user/supervised_user_service.cc#newcode302 chrome/browser/supervised_user/supervised_user_service.cc:302: if (state == ExtensionState::BLOCKED || On 2016/06/20 12:44:18, Marc ...

4 years, 6 months ago (2016-06-20 12:56:53 UTC) #37

Marc Treib

Looking good now! One last comment :) https://codereview.chromium.org/2004043002/diff/400001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2004043002/diff/400001/chrome/browser/supervised_user/supervised_user_service.cc#newcode301 chrome/browser/supervised_user/supervised_user_service.cc:301: if (state ...

4 years, 6 months ago (2016-06-20 15:48:07 UTC) #38

mamir

https://codereview.chromium.org/2004043002/diff/400001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2004043002/diff/400001/chrome/browser/supervised_user/supervised_user_service.cc#newcode301 chrome/browser/supervised_user/supervised_user_service.cc:301: if (state == ExtensionState::FORCED) On 2016/06/20 15:48:06, Marc Treib ...

4 years, 6 months ago (2016-06-20 15:56:55 UTC) #39

Devlin

https://codereview.chromium.org/2004043002/diff/420001/chrome/browser/extensions/extension_service_sync_unittest.cc File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2004043002/diff/420001/chrome/browser/extensions/extension_service_sync_unittest.cc#newcode1620 chrome/browser/extensions/extension_service_sync_unittest.cc:1620: EXPECT_TRUE(registry()->disabled_extensions().Contains(extension->id())); nit: to be thorough, maybe also check disabled ...

4 years, 6 months ago (2016-06-20 17:32:29 UTC) #40

mamir

https://codereview.chromium.org/2004043002/diff/420001/chrome/browser/extensions/extension_service_sync_unittest.cc File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2004043002/diff/420001/chrome/browser/extensions/extension_service_sync_unittest.cc#newcode1620 chrome/browser/extensions/extension_service_sync_unittest.cc:1620: EXPECT_TRUE(registry()->disabled_extensions().Contains(extension->id())); On 2016/06/20 17:32:28, Devlin wrote: > nit: to ...

4 years, 6 months ago (2016-06-21 11:00:44 UTC) #42

mamir

mamir@chromium.org changed reviewers: + phajdan.jr@chromium.org

4 years, 6 months ago (2016-06-21 15:28:39 UTC) #43

mamir

phajdan.jr@chromium.org: Please review changes in chrome/test/base/testing_profile.cc chrome/test/base/testing_profile.h chrome/test/DEPS

4 years, 6 months ago (2016-06-21 15:28:40 UTC) #44

mamir

mamir@chromium.org changed reviewers: + holte@chromium.org

4 years, 6 months ago (2016-06-21 15:31:51 UTC) #45

mamir

Hi holte@chromium.org Please, review changes in tools/metrics/histograms/histograms.xml

4 years, 6 months ago (2016-06-21 15:33:24 UTC) #46

Devlin

lgtm https://codereview.chromium.org/2004043002/diff/440001/chrome/browser/extensions/extension_service_sync_unittest.cc File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2004043002/diff/440001/chrome/browser/extensions/extension_service_sync_unittest.cc#newcode1615 chrome/browser/extensions/extension_service_sync_unittest.cc:1615: const base::FilePath& base_path = data_dir().AppendASCII("autoupdate"); nit: these shouldn't ...

4 years, 6 months ago (2016-06-21 16:54:38 UTC) #47

mamir

Hi Marc and Devlin, Please, have a another look after rebasing to the other CL ...

4 years, 6 months ago (2016-06-23 11:43:20 UTC) #50

mamir

The CQ bit was checked by mamir@chromium.org to run a CQ dry run

4 years, 6 months ago (2016-06-23 13:22:11 UTC) #51

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2004043002/560001

4 years, 6 months ago (2016-06-23 13:22:20 UTC) #52

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 6 months ago (2016-06-23 13:38:42 UTC) #53

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/157416)

4 years, 6 months ago (2016-06-23 13:38:44 UTC) #54

mamir

The CQ bit was checked by mamir@chromium.org to run a CQ dry run

4 years, 6 months ago (2016-06-23 14:46:03 UTC) #55

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2004043002/570001

4 years, 6 months ago (2016-06-23 14:46:17 UTC) #56

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 6 months ago (2016-06-23 15:01:21 UTC) #58

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_android_rel_ng/builds/92590)

4 years, 6 months ago (2016-06-23 15:01:23 UTC) #59

mamir

The CQ bit was checked by mamir@chromium.org to run a CQ dry run

4 years, 6 months ago (2016-06-23 15:15:00 UTC) #60

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2004043002/590001

4 years, 6 months ago (2016-06-23 15:15:10 UTC) #61

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 6 months ago (2016-06-23 15:31:48 UTC) #62

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: android_compile_dbg on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_compile_dbg/builds/86181)

4 years, 6 months ago (2016-06-23 15:31:50 UTC) #63

mamir

The CQ bit was checked by mamir@chromium.org to run a CQ dry run

4 years, 6 months ago (2016-06-24 10:11:37 UTC) #64

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2004043002/630001

4 years, 6 months ago (2016-06-24 10:11:57 UTC) #65

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 6 months ago (2016-06-24 10:55:33 UTC) #66

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/191127)

4 years, 6 months ago (2016-06-24 10:55:35 UTC) #67

Marc Treib

https://codereview.chromium.org/2004043002/diff/630001/chrome/browser/extensions/extension_util.cc File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2004043002/diff/630001/chrome/browser/extensions/extension_util.cc#newcode260 chrome/browser/extensions/extension_util.cc:260: service->EnableExtension(extension_id); ? I'm pretty sure we don't want to ...

4 years, 6 months ago (2016-06-24 10:58:55 UTC) #68

Marc Treib

https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensions/extension_util.cc File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensions/extension_util.cc#newcode239 chrome/browser/extensions/extension_util.cc:239: const Extension* extension = registry->GetInstalledExtension(extension_id); nit: Move getting registry ...

4 years, 6 months ago (2016-06-24 12:46:33 UTC) #69

mamir

https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensions/extension_util.cc File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensions/extension_util.cc#newcode239 chrome/browser/extensions/extension_util.cc:239: const Extension* extension = registry->GetInstalledExtension(extension_id); On 2016/06/24 12:46:33, Marc ...

4 years, 6 months ago (2016-06-24 14:43:07 UTC) #70

https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensi...
File chrome/browser/extensions/extension_util.cc (right):

https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensi...
chrome/browser/extensions/extension_util.cc:239: const Extension* extension =
registry->GetInstalledExtension(extension_id);
On 2016/06/24 12:46:33, Marc Treib wrote:
> nit: Move getting registry and extension down to where you actually need it.

Done.

https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensi...
chrome/browser/extensions/extension_util.cc:243: // If the
installed_by_custodian flag is reset, do nothing.
On 2016/06/24 12:46:33, Marc Treib wrote:
> Inaccurate, you *do* do something.

Done.

https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensi...
chrome/browser/extensions/extension_util.cc:260:
service->EnableExtension(extension_id);
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. 
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.

https://codereview.chromium.org/2004043002/diff/630001/chrome/browser/extensi...
File chrome/browser/extensions/extension_util.cc (right):

https://codereview.chromium.org/2004043002/diff/630001/chrome/browser/extensi...
chrome/browser/extensions/extension_util.cc:260:
service->EnableExtension(extension_id);
On 2016/06/24 10:58:55, Marc Treib wrote:
> ?
> I'm pretty sure we don't want to do this.

For the record, discussed in patchset 29

https://codereview.chromium.org/2004043002/diff/630001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/2004043002/diff/630001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:1125: // upon
extension update if it doesn't require extra permission.
On 2016/06/24 10:58:55, Marc Treib wrote:
> nit: This isn't really accurate anymore. It's also responsible for
> disabling/re-enabling the extension as required. I'd just reformulate it to
just
> refer to extension updates.

Done.

https://codereview.chromium.org/2004043002/diff/630001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:1134: // new
permissions, we update the approved_version and re-enable it.
On 2016/06/24 10:58:55, Marc Treib wrote:
> nit: I'd remove the "and re-enable it" part (and maybe add a comment to the
> ChangeExtensionStateIfNecessary call below)

Done.

https://codereview.chromium.org/2004043002/diff/630001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:1166:
ChangeExtensionStateIfNecessary(extensions_entry.first);
On 2016/06/24 10:58:55, Marc Treib wrote:
> Ah, one more thing here: If an approval was removed, we might have to disable
> the extension, but here we only update the ones that still have approvals
> (matching or not).

Well-spotted, thank you!
Done and I have updated the tests to cover this as well.

https://codereview.chromium.org/2004043002/diff/630001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:1175: // When it gets
installed, GetExtensionState() will return ALLOWED, hence
On 2016/06/24 10:58:55, Marc Treib wrote:
> IMO this isn't really accurate, GetExtensionState might return anything. I'd
> just say that things will be handled after installation.

Done.

https://codereview.chromium.org/2004043002/diff/630001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:1182:
ExtensionSystem::Get(profile_)->extension_service();
On 2016/06/24 10:58:55, Marc Treib wrote:
> nit: You could move both of these to later in the method. (Obsolete if you use
a
> switch)

Done.

https://codereview.chromium.org/2004043002/diff/630001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:1187: if (state ==
ExtensionState::BLOCKED || state == ExtensionState::FORCED)
On 2016/06/24 10:58:55, Marc Treib wrote:
> nit: This might be easier to read as a switch instead of a series of ifs?

Done.

mamir

The CQ bit was checked by mamir@chromium.org to run a CQ dry run

4 years, 6 months ago (2016-06-24 14:43:31 UTC) #71

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2004043002/650001

4 years, 6 months ago (2016-06-24 14:43:54 UTC) #72

Marc Treib

https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensions/extension_util.cc File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensions/extension_util.cc#newcode260 chrome/browser/extensions/extension_util.cc:260: service->EnableExtension(extension_id); On 2016/06/24 14:43:07, mamir wrote: > On 2016/06/24 ...

4 years, 6 months ago (2016-06-24 15:00:59 UTC) #73

https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensi...
File chrome/browser/extensions/extension_util.cc (right):

https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensi...
chrome/browser/extensions/extension_util.cc:260:
service->EnableExtension(extension_id);
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.

Marc Treib

https://codereview.chromium.org/2004043002/diff/650001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2004043002/diff/650001/chrome/browser/supervised_user/supervised_user_service.cc#newcode1158 chrome/browser/supervised_user/supervised_user_service.cc:1158: for (auto imap : approved_extensions_map_) nit: what's "imap"? Also, ...

4 years, 6 months ago (2016-06-24 15:04:05 UTC) #74

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 6 months ago (2016-06-24 15:46:14 UTC) #75

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/191256)

4 years, 6 months ago (2016-06-24 15:46:16 UTC) #76

Devlin

https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensions/extension_util.cc File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensions/extension_util.cc#newcode260 chrome/browser/extensions/extension_util.cc:260: service->EnableExtension(extension_id); On 2016/06/24 15:00:59, Marc Treib wrote: > On ...

4 years, 6 months ago (2016-06-24 16:09:40 UTC) #77

https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensi...
File chrome/browser/extensions/extension_util.cc (right):

https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensi...
chrome/browser/extensions/extension_util.cc:260:
service->EnableExtension(extension_id);
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?

https://codereview.chromium.org/2004043002/diff/650001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service_sync_unittest.cc (right):

https://codereview.chromium.org/2004043002/diff/650001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:1644: const
base::Version version("1");
Define this closer to its use, or maybe just inline it.

https://codereview.chromium.org/2004043002/diff/650001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:1893:
EXPECT_FALSE(registry()->enabled_extensions().Contains(id));
Maybe make a helper function IsDisabledForApproval(id) that checks the
registry's disabled set and the disable reason.

https://codereview.chromium.org/2004043002/diff/650001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:2209: EXPECT_EQ(0,
I think we can just EXPECT_EQ(base::Version(approved_version),
extension->version())

https://codereview.chromium.org/2004043002/diff/650001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/2004043002/diff/650001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:1018: const
std::string& id = extension.id();
just inline id

https://codereview.chromium.org/2004043002/diff/650001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:1072: bool
may_not_uninstall = (result == ExtensionState::FORCED);
no need for parens

mamir

https://codereview.chromium.org/2004043002/diff/650001/chrome/browser/extensions/extension_service_sync_unittest.cc File chrome/browser/extensions/extension_service_sync_unittest.cc (right): https://codereview.chromium.org/2004043002/diff/650001/chrome/browser/extensions/extension_service_sync_unittest.cc#newcode1644 chrome/browser/extensions/extension_service_sync_unittest.cc:1644: const base::Version version("1"); On 2016/06/24 16:09:40, Devlin wrote: > ...

4 years, 5 months ago (2016-06-27 11:48:54 UTC) #78

Marc Treib

https://codereview.chromium.org/2004043002/diff/650001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2004043002/diff/650001/chrome/browser/supervised_user/supervised_user_service.cc#newcode1158 chrome/browser/supervised_user/supervised_user_service.cc:1158: for (auto imap : approved_extensions_map_) On 2016/06/27 11:48:54, mamir ...

4 years, 5 months ago (2016-06-27 11:56:35 UTC) #79

mamir

https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensions/extension_util.cc File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensions/extension_util.cc#newcode260 chrome/browser/extensions/extension_util.cc:260: service->EnableExtension(extension_id); On 2016/06/24 16:09:39, Devlin wrote: > On 2016/06/24 ...

4 years, 5 months ago (2016-06-27 14:26:59 UTC) #80

https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensi...
File chrome/browser/extensions/extension_util.cc (right):

https://codereview.chromium.org/2004043002/diff/560001/chrome/browser/extensi...
chrome/browser/extensions/extension_util.cc:260:
service->EnableExtension(extension_id);
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

https://codereview.chromium.org/2004043002/diff/650001/chrome/browser/supervi...
File chrome/browser/supervised_user/supervised_user_service.cc (right):

https://codereview.chromium.org/2004043002/diff/650001/chrome/browser/supervi...
chrome/browser/supervised_user/supervised_user_service.cc:1158: for (auto imap :
approved_extensions_map_)
On 2016/06/27 11:56:35, Marc Treib wrote:
> On 2016/06/27 11:48:54, mamir wrote:
> > On 2016/06/24 15:04:05, Marc Treib wrote:
> > > nit: what's "imap"? Also, const&
> > 
> > Done.
> 
> nit^2: it's not an iterator, so ".._it" is kinda misleading

Done.

https://codereview.chromium.org/2004043002/diff/670001/chrome/browser/extensi...
File chrome/browser/extensions/extension_service_sync_unittest.cc (right):

https://codereview.chromium.org/2004043002/diff/670001/chrome/browser/extensi...
chrome/browser/extensions/extension_service_sync_unittest.cc:1703:
EXPECT_FALSE(registry()->enabled_extensions().Contains(extension_id));
On 2016/06/27 11:56:35, Marc Treib wrote:
> I find this super confusing to read... it shouldn't be enabled either way, so
> pull that out of the if? And if it's not expected to be loaded, shouldn't you
> check registry->GetInstalledExtension?

Modified as discussed offline.
Done!

Marc Treib

LGTM with some more nits! https://codereview.chromium.org/2004043002/diff/710001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2004043002/diff/710001/chrome/browser/supervised_user/supervised_user_service.cc#newcode1081 chrome/browser/supervised_user/supervised_user_service.cc:1081: // Only extensions that ...

4 years, 5 months ago (2016-06-27 14:32:59 UTC) #81

mamir

https://codereview.chromium.org/2004043002/diff/710001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2004043002/diff/710001/chrome/browser/supervised_user/supervised_user_service.cc#newcode1081 chrome/browser/supervised_user/supervised_user_service.cc:1081: // Only extensions that requires approval should be disabled. ...

4 years, 5 months ago (2016-06-27 14:58:14 UTC) #82

mamir

The CQ bit was checked by mamir@chromium.org to run a CQ dry run

4 years, 5 months ago (2016-06-27 14:58:36 UTC) #83

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2004043002/730001

4 years, 5 months ago (2016-06-27 14:58:56 UTC) #84

Marc Treib

https://codereview.chromium.org/2004043002/diff/710001/chrome/browser/supervised_user/supervised_user_service.cc File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/2004043002/diff/710001/chrome/browser/supervised_user/supervised_user_service.cc#newcode1082 chrome/browser/supervised_user/supervised_user_service.cc:1082: // Blocked extensions should be loaded all together, and ...

4 years, 5 months ago (2016-06-27 15:05:46 UTC) #85

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 5 months ago (2016-06-27 15:42:37 UTC) #86

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/192133)

4 years, 5 months ago (2016-06-27 15:42:38 UTC) #87

mamir

The patchset sent to the CQ was uploaded after l-g-t-m from holte@chromium.org, phajdan.jr@chromium.org, treib@chromium.org, rdevlin.cronin@chromium.org ...

4 years, 5 months ago (2016-06-27 23:08:45 UTC) #90

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2004043002/770001

4 years, 5 months ago (2016-06-27 23:09:28 UTC) #91

commit-bot: I haz the power

The CQ bit was unchecked by commit-bot@chromium.org

4 years, 5 months ago (2016-06-27 23:20:47 UTC) #92

commit-bot: I haz the power

Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/208057)

4 years, 5 months ago (2016-06-27 23:20:50 UTC) #93

mamir

mamir@chromium.org changed reviewers: + pavely@chromium.org

4 years, 5 months ago (2016-06-28 08:45:22 UTC) #94

mamir

Hi Pavel, I need your LGTM because I added a sync dependency to chrome/test/DEPS. Thank ...

4 years, 5 months ago (2016-06-28 08:45:24 UTC) #95

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2004043002/770001

4 years, 5 months ago (2016-06-28 18:56:54 UTC) #99

commit-bot: I haz the power

Description was changed from ========== Supervised users initiated extensions installs Supervised users can install apps ...

4 years, 5 months ago (2016-06-28 22:18:10 UTC) #100

commit-bot: I haz the power

Description was changed from ========== Supervised users initiated extensions installs Supervised users can install apps ...

4 years, 5 months ago (2016-06-28 22:20:02 UTC) #102

commit-bot: I haz the power

4 years, 5 months ago (2016-06-28 22:20:04 UTC) #103

Message was sent while issue was closed.

Patchset 40 (id:??) landed as
https://crrev.com/e960964b7ea2caff5289d4a2f40488af7613389d
Cr-Commit-Position: refs/heads/master@{#402570}

Issue 2004043002: Supervised Users Initiated Installs v2 (Closed)