Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1200833004:
Apps&Extensions for Supervised Users: send permission request on outdated re-enables (Closed)
|
Created:
5 years, 6 months ago by Marc Treib Modified:
5 years, 2 months ago Reviewers:
Devlin CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionApps&Extensions for Supervised Users: send permission request on outdated re-enables
Also add a test to verify that re-enabling works as expected
BUG=503106
Committed: https://crrev.com/40d3ad9f1201248f2eaa169c5d6b528bc42afbb4
Cr-Commit-Position: refs/heads/master@{#355106}
Patch Set 1 #Patch Set 2 : . #
Total comments: 33
Patch Set 3 : review1 #Patch Set 4 : Revived! #Patch Set 5 : rebase #
Total comments: 18
Patch Set 6 : review (easy parts) #Patch Set 7 : tests! #
Total comments: 1
Dependent Patchsets: Messages
Total messages: 21 (3 generated)
treib@chromium.org changed reviewers: + kalman@chromium.org
PTAL! Some more context on the bug. https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/app_sync_bundle.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/app_sync_bundle.cc:126: ProcessSyncChange(it->second); Before this CL, the pending_sync_data_ was applied only during MergeDataAndStartSyncing, i.e. it only handled changes that happened before Sync was up and running. I think that was a bug that just happened to have no visible effects (see other comments below), but I might be missing something..? https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1687: extension->id(), Extension::DISABLE_PERMISSIONS_INCREASE)) { This is really unrelated; I just discovered the duplicate requests in the new test. https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_bundle.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_bundle.cc:128: ProcessSyncChange(it->second); It sucks that this code is duplicated in app_sync_bundle and extension_sync_bundle - in fact, the two are almost identical % AppSyncData vs. ExtensionSyncData. Maybe the base SyncData class should be a template... https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:534: return false; Actually, I think the same behavior should apply to all users, not just supervised ones: Defer the re-enabling until we're on the new version, since the old version might have permissions that the user never approved. WDYT? (Note that there already is an "is this outdated" check below, but it happens after everything has been applied.) https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:614: return false; This is a bit weird: Returning "false" here causes the ExtensionSyncData to be stored in pending_sync_data_ (in App/ExtensionSyncBundle), but before this CL, it would never be used again if Sync is already running. I guess things worked only because all the changes have been applied already at this point?
This is a hard change to get my head around. I need to come back to it tomorrow morning I think, but perhaps you could clarify some things for me. https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/app_sync_bundle.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/app_sync_bundle.cc:126: ProcessSyncChange(it->second); On 2015/06/22 15:35:18, Marc Treib wrote: > Before this CL, the pending_sync_data_ was applied only during > MergeDataAndStartSyncing, i.e. it only handled changes that happened before Sync > was up and running. I think that was a bug that just happened to have no visible > effects (see other comments below), but I might be missing something..? I'm struggling with this. Is SyncChangeIfNeeded the right place to be doing this? This gets called when we try to *generate* a sync change, right? Why would that involve pushing out these pending sync changes? https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1687: extension->id(), Extension::DISABLE_PERMISSIONS_INCREASE)) { On 2015/06/22 15:35:18, Marc Treib wrote: > This is really unrelated; I just discovered the duplicate requests in the new > test. Should the check be on line 1672-3 instead? https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:494: const Extension* extension = extension_service_->GetInstalledExtension(id); Can we save a reference to ExtensionRegistry (say "registry") up here, use registry->GetInstalledExtension(id) instead of this, and use registry->enabled_extensions()->Contains(id) below? Just a tiny bit cleaner. https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:534: return false; On 2015/06/22 15:35:18, Marc Treib wrote: > Actually, I think the same behavior should apply to all users, not just > supervised ones: Defer the re-enabling until we're on the new version, since the > old version might have permissions that the user never approved. WDYT? > (Note that there already is an "is this outdated" check below, but it happens > after everything has been applied.) I agree this isn't necessarily supervised-user specific, and it would be good to separate that out. However I guess it could never happen outside supervised users, because the sync change to push the update to the extension could only ever come before a sync change to grant it permissions? Now... ok, you're going to need to refresh my memory. How are supervised users different? https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:614: return false; On 2015/06/22 15:35:18, Marc Treib wrote: > This is a bit weird: Returning "false" here causes the ExtensionSyncData to be > stored in pending_sync_data_ (in App/ExtensionSyncBundle), but before this CL, > it would never be used again if Sync is already running. I guess things worked > only because all the changes have been applied already at this point? Bleh. This is too complicated.
Thanks for your comments! I've tried to explain things better below. I think a big chunk of the complexity comes from the quirks of extension syncing though... https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/app_sync_bundle.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/app_sync_bundle.cc:126: ProcessSyncChange(it->second); On 2015/06/22 21:03:10, kalman wrote: > On 2015/06/22 15:35:18, Marc Treib wrote: > > Before this CL, the pending_sync_data_ was applied only during > > MergeDataAndStartSyncing, i.e. it only handled changes that happened before > Sync > > was up and running. I think that was a bug that just happened to have no > visible > > effects (see other comments below), but I might be missing something..? > > I'm struggling with this. Is SyncChangeIfNeeded the right place to be doing > this? This gets called when we try to *generate* a sync change, right? Why would > that involve pushing out these pending sync changes? My reasoning is this: At some previous point, we decided that we can't apply this sync change right now (e.g. because the installed extension is outdated), and instead queued it up as pending. Now here, we're telling the ExtensionSyncService "this extension has changed", so this is a good time to try applying the change again. Maybe "SyncChangeIfNeeded" should be renamed to "NotifyExtensionChanged" or something like that? https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1687: extension->id(), Extension::DISABLE_PERMISSIONS_INCREASE)) { On 2015/06/22 21:03:10, kalman wrote: > On 2015/06/22 15:35:18, Marc Treib wrote: > > This is really unrelated; I just discovered the duplicate requests in the new > > test. > > Should the check be on line 1672-3 instead? Hmm.. maybe? Lines 1673 and 1677 are irrelevant in this case (it already has the disable reason, and so is necessarily also disabled already), but I don't know about the "AutoDisable" histogram. What is that even supposed to mean? https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:494: const Extension* extension = extension_service_->GetInstalledExtension(id); On 2015/06/22 21:03:10, kalman wrote: > Can we save a reference to ExtensionRegistry (say "registry") up here, use > registry->GetInstalledExtension(id) instead of this, and use > registry->enabled_extensions()->Contains(id) below? Just a tiny bit cleaner. Sure, done. https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:534: return false; On 2015/06/22 21:03:10, kalman wrote: > On 2015/06/22 15:35:18, Marc Treib wrote: > > Actually, I think the same behavior should apply to all users, not just > > supervised ones: Defer the re-enabling until we're on the new version, since > the > > old version might have permissions that the user never approved. WDYT? > > (Note that there already is an "is this outdated" check below, but it happens > > after everything has been applied.) > > I agree this isn't necessarily supervised-user specific, and it would be good to > separate that out. > > However I guess it could never happen outside supervised users, because the sync > change to push the update to the extension could only ever come before a sync > change to grant it permissions? Now... ok, you're going to need to refresh my > memory. How are supervised users different? But the extension update could have arrived first, and then a Sync update from another client that hasn't seen the new version yet, right? Not very likely, but still. Supervised users are different in that the Sync change to re-enable does not come from another Chrome instance, but instead comes from their custodian, via a web dashboard that calls a Sync RPC. That generates a Sync change that sets the "enabled" bit to true, and the version to whatever the custodian saw and approved. We can't guarantee that what the custodian saw is actually what we have installed here, so we need to check that the versions match. https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:614: return false; On 2015/06/22 21:03:10, kalman wrote: > On 2015/06/22 15:35:18, Marc Treib wrote: > > This is a bit weird: Returning "false" here causes the ExtensionSyncData to be > > stored in pending_sync_data_ (in App/ExtensionSyncBundle), but before this CL, > > it would never be used again if Sync is already running. I guess things worked > > only because all the changes have been applied already at this point? > > Bleh. This is too complicated. No argument here...
Slowly getting used to this code. https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/app_sync_bundle.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/app_sync_bundle.cc:126: ProcessSyncChange(it->second); On 2015/06/23 10:22:25, Marc Treib wrote: > On 2015/06/22 21:03:10, kalman wrote: > > On 2015/06/22 15:35:18, Marc Treib wrote: > > > Before this CL, the pending_sync_data_ was applied only during > > > MergeDataAndStartSyncing, i.e. it only handled changes that happened before > > Sync > > > was up and running. I think that was a bug that just happened to have no > > visible > > > effects (see other comments below), but I might be missing something..? > > > > I'm struggling with this. Is SyncChangeIfNeeded the right place to be doing > > this? This gets called when we try to *generate* a sync change, right? Why > would > > that involve pushing out these pending sync changes? > > My reasoning is this: At some previous point, we decided that we can't apply > this sync change right now (e.g. because the installed extension is outdated), > and instead queued it up as pending. > Now here, we're telling the ExtensionSyncService "this extension has changed", > so this is a good time to try applying the change again. > Maybe "SyncChangeIfNeeded" should be renamed to "NotifyExtensionChanged" or > something like that? That doesn't sound like it follows to me. Perhaps there should be a DCHECK in here that there is no pending sync change for an Extension, but actually pushing out that change - that should happen when the conditions that caused it to skip in the first place are fulfilled. I.e. when we know that ExtensionSyncService::ProcessExtensionSyncDataHelper will return true. The funky thing is that ProcessExtensionSyncDataHelper does a whole bunch of work before deciding that it can apply the change. Calling back into it like this (calling ProcessSyncChange again) will cause all that work to happen again, at which point it again may return false (I don't see why not). That seems ... wrong. In fact if that comment is to believed "Returns false if the changes were not completely applied and need to be tried again later" and something *does* try again later... that is exactly the same problem already. Basically this is all very hard to follow. ExtensionSyncService and *SyncBundle jump between each other and I lose track of what the role is supposed to be. Argh. https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1677: extension_prefs_->SetExtensionState(extension->id(), Extension::DISABLED); I wonder if this should actually go on line 1697, if (disable_reasons != Extension::DISABLE_NONE) { prefs_->AddDisableReasons(); prefs_->SetExtensionState(); } seems odd to be doing very similar work in 2 different places. But, that basically defines this entire file and all the sync stuff. We can deal with this later. https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1687: extension->id(), Extension::DISABLE_PERMISSIONS_INCREASE)) { On 2015/06/23 10:22:25, Marc Treib wrote: > On 2015/06/22 21:03:10, kalman wrote: > > On 2015/06/22 15:35:18, Marc Treib wrote: > > > This is really unrelated; I just discovered the duplicate requests in the > new > > > test. > > > > Should the check be on line 1672-3 instead? > > Hmm.. maybe? Lines 1673 and 1677 are irrelevant in this case (it already has the > disable reason, and so is necessarily also disabled already), but I don't know > about the "AutoDisable" histogram. What is that even supposed to mean? Ohh right. I think at this point the prefs contain the old status of the extension, so the AutoDisable thing means that if the extension isn't already disabled, we're about to, and that is reported. Perfectly normal. So I think the code as you have it is fine, however, I'd change it to use DidExtensionEscalatePermissions instead of HasDisableReason, for consistency. https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:534: return false; On 2015/06/23 10:22:25, Marc Treib wrote: > On 2015/06/22 21:03:10, kalman wrote: > > On 2015/06/22 15:35:18, Marc Treib wrote: > > > Actually, I think the same behavior should apply to all users, not just > > > supervised ones: Defer the re-enabling until we're on the new version, since > > the > > > old version might have permissions that the user never approved. WDYT? > > > (Note that there already is an "is this outdated" check below, but it > happens > > > after everything has been applied.) > > > > I agree this isn't necessarily supervised-user specific, and it would be good > to > > separate that out. > > > > However I guess it could never happen outside supervised users, because the > sync > > change to push the update to the extension could only ever come before a sync > > change to grant it permissions? Now... ok, you're going to need to refresh my > > memory. How are supervised users different? > > But the extension update could have arrived first, and then a Sync update from > another client that hasn't seen the new version yet, right? Not very likely, but > still. > > Supervised users are different in that the Sync change to re-enable does not > come from another Chrome instance, but instead comes from their custodian, via a > web dashboard that calls a Sync RPC. That generates a Sync change that sets the > "enabled" bit to true, and the version to whatever the custodian saw and > approved. We can't guarantee that what the custodian saw is actually what we > have installed here, so we need to check that the versions match. Ok, right. I think it only makes sense for supervised users. In the normal case there is no way we could be getting a sync change to enable an extension before it's actually been installed at that version. Could you explain that? https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:541: // re-enable request to the custodian. Is this actually necessary? I would have expected there to be code elsewhere, somewhere, that sees an update and makes an update request.
https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/app_sync_bundle.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/app_sync_bundle.cc:126: ProcessSyncChange(it->second); On 2015/06/24 00:25:10, kalman wrote: > On 2015/06/23 10:22:25, Marc Treib wrote: > > On 2015/06/22 21:03:10, kalman wrote: > > > On 2015/06/22 15:35:18, Marc Treib wrote: > > > > Before this CL, the pending_sync_data_ was applied only during > > > > MergeDataAndStartSyncing, i.e. it only handled changes that happened > before > > > Sync > > > > was up and running. I think that was a bug that just happened to have no > > > visible > > > > effects (see other comments below), but I might be missing something..? > > > > > > I'm struggling with this. Is SyncChangeIfNeeded the right place to be doing > > > this? This gets called when we try to *generate* a sync change, right? Why > > would > > > that involve pushing out these pending sync changes? > > > > My reasoning is this: At some previous point, we decided that we can't apply > > this sync change right now (e.g. because the installed extension is outdated), > > and instead queued it up as pending. > > Now here, we're telling the ExtensionSyncService "this extension has changed", > > so this is a good time to try applying the change again. > > Maybe "SyncChangeIfNeeded" should be renamed to "NotifyExtensionChanged" or > > something like that? > > That doesn't sound like it follows to me. Perhaps there should be a DCHECK in > here that there is no pending sync change for an Extension, but actually pushing > out that change - that should happen when the conditions that caused it to skip > in the first place are fulfilled. > > I.e. when we know that ExtensionSyncService::ProcessExtensionSyncDataHelper will > return true. Well, in the current state, ProcessExtensionSyncDataHelper gives us no clue why it said "false" before, so "retry whenever something changed" is kind of the best we can do. But maybe we should change that :) > The funky thing is that ProcessExtensionSyncDataHelper does a whole bunch of > work before deciding that it can apply the change. Calling back into it like > this (calling ProcessSyncChange again) will cause all that work to happen again, > at which point it again may return false (I don't see why not). That seems ... > wrong. In fact if that comment is to believed "Returns false if the changes were > not completely applied and need to be tried again later" and something *does* > try again later... that is exactly the same problem already. Yeah, that's kind of messed up: Right now, *nothing ever tries again later* (if Sync is up already). Anyway, I noticed that my changes right now wouldn't work reliably: The MarkPendingAppSynced call below will remove the pending Sync change, whether is was applied or not. So I guess some more thought is required here... > Basically this is all very hard to follow. ExtensionSyncService and *SyncBundle > jump between each other and I lose track of what the role is supposed to be. > Argh. Indeed. https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1687: extension->id(), Extension::DISABLE_PERMISSIONS_INCREASE)) { On 2015/06/24 00:25:10, kalman wrote: > On 2015/06/23 10:22:25, Marc Treib wrote: > > On 2015/06/22 21:03:10, kalman wrote: > > > On 2015/06/22 15:35:18, Marc Treib wrote: > > > > This is really unrelated; I just discovered the duplicate requests in the > > new > > > > test. > > > > > > Should the check be on line 1672-3 instead? > > > > Hmm.. maybe? Lines 1673 and 1677 are irrelevant in this case (it already has > the > > disable reason, and so is necessarily also disabled already), but I don't know > > about the "AutoDisable" histogram. What is that even supposed to mean? > > Ohh right. I think at this point the prefs contain the old status of the > extension, so the AutoDisable thing means that if the extension isn't already > disabled, we're about to, and that is reported. Perfectly normal. > > So I think the code as you have it is fine, however, I'd change it to use > DidExtensionEscalatePermissions instead of HasDisableReason, for consistency. Hm. DidExtensionEscalatePermissions checks for DISABLE_PERMISSIONS_INCREASE | DISABLE_REMOTE_INSTALL. Now DISABLE_REMOTE_INSTALL shouldn't ever be set for a supervised user, but still... https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:534: return false; On 2015/06/24 00:25:11, kalman wrote: > On 2015/06/23 10:22:25, Marc Treib wrote: > > On 2015/06/22 21:03:10, kalman wrote: > > > On 2015/06/22 15:35:18, Marc Treib wrote: > > > > Actually, I think the same behavior should apply to all users, not just > > > > supervised ones: Defer the re-enabling until we're on the new version, > since > > > the > > > > old version might have permissions that the user never approved. WDYT? > > > > (Note that there already is an "is this outdated" check below, but it > > happens > > > > after everything has been applied.) > > > > > > I agree this isn't necessarily supervised-user specific, and it would be > good > > to > > > separate that out. > > > > > > However I guess it could never happen outside supervised users, because the > > sync > > > change to push the update to the extension could only ever come before a > sync > > > change to grant it permissions? Now... ok, you're going to need to refresh > my > > > memory. How are supervised users different? > > > > But the extension update could have arrived first, and then a Sync update from > > another client that hasn't seen the new version yet, right? Not very likely, > but > > still. > > > > Supervised users are different in that the Sync change to re-enable does not > > come from another Chrome instance, but instead comes from their custodian, via > a > > web dashboard that calls a Sync RPC. That generates a Sync change that sets > the > > "enabled" bit to true, and the version to whatever the custodian saw and > > approved. We can't guarantee that what the custodian saw is actually what we > > have installed here, so we need to check that the versions match. > > Ok, right. I think it only makes sense for supervised users. In the normal case > there is no way we could be getting a sync change to enable an extension before > it's actually been installed at that version. Could you explain that? I still think the same thing can happen for regular users in some (unlikely) circumstances. Say another device has updated to the new version and sends Sync updates. On this device, the web store is unreachable/slow, so we're still on the old version. https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:541: // re-enable request to the custodian. On 2015/06/24 00:25:10, kalman wrote: > Is this actually necessary? I would have expected there to be code elsewhere, > somewhere, that sees an update and makes an update request. Hm, I'd need to remove the "don't send duplicate permission requests" check that I just added in extension_service.cc again. Then we would not need to send requests here, at the cost of some extraneous requests. But those shouldn't hurt, so maybe that's the better option overall.
https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/app_sync_bundle.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/app_sync_bundle.cc:126: ProcessSyncChange(it->second); > Well, in the current state, ProcessExtensionSyncDataHelper gives us no clue why > it said "false" before, so "retry whenever something changed" is kind of the > best we can do. But maybe we should change that :) Indeed. It wouldn't be too hard of a transformation though it does mildly terrify me that what should be an innocuous change... well, who knows. > Yeah, that's kind of messed up: Right now, *nothing ever tries again later* (if > Sync is up already). I followed the path up from GetPendingData() and it ends up in ExtensionSyncService::MergeDataAndStartSyncing() where those pending changes are applied, and which is called when sync is set up. Does that count as "trying again" or is there something else going wrong? https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1687: extension->id(), Extension::DISABLE_PERMISSIONS_INCREASE)) { > Hm. DidExtensionEscalatePermissions checks for DISABLE_PERMISSIONS_INCREASE | > DISABLE_REMOTE_INSTALL. Now DISABLE_REMOTE_INSTALL shouldn't ever be set for a > supervised user, but still... From purely an interface perspective - without looking at the code or figuring out its implications - it seems like calling DidExtensionEscalatePermissions is the right thing to do? From an implementation perspective, what *will* (or should) happen if DISABLE_REMOTE_INSTALL is set for a supervised user? https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:534: return false; > > Ok, right. I think it only makes sense for supervised users. In the normal > case > > there is no way we could be getting a sync change to enable an extension > before > > it's actually been installed at that version. Could you explain that? > > I still think the same thing can happen for regular users in some (unlikely) > circumstances. Say another device has updated to the new version and sends Sync > updates. On this device, the web store is unreachable/slow, so we're still on > the old version. Nit: it's omaha not the webstore. Also the situation you describe is pretty common, sync is faster than omaha, but perhaps I'm misinterpreting you. Or confused you :-) But... ok, I think given that sync coalesces updates with the same key (in this case the key is the extension ID I think), an update event could get squashed by an enabled event. In fact in that case shouldn't we be updating the extension then? ARGH. https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:541: // re-enable request to the custodian. On 2015/06/24 11:50:57, Marc Treib wrote: > On 2015/06/24 00:25:10, kalman wrote: > > Is this actually necessary? I would have expected there to be code elsewhere, > > somewhere, that sees an update and makes an update request. > > Hm, I'd need to remove the "don't send duplicate permission requests" check that > I just added in extension_service.cc again. Then we would not need to send > requests here, at the cost of some extraneous requests. But those shouldn't > hurt, so maybe that's the better option overall. Mm I don't understand why you'd need to remove that check to have this work, but now that I read this code again, I think what you're written isn't wrong: drop the change if it's for an older version. I think it can happen outside supervised users though - just like you said, more common because there are 2 users involved not 1. But I think you could simplify this (insofar as pulling out of supervised users) but basically saying: if any sync change happens for an older version, we should drop it. That prevents rolling back extensions but wow if that ever needs to happen we're in serious trouble all over the place.
Nothing to review right now. I'll try cleaning up this whole mess (i.e. properly retry pending changes once the version matches) and then ping you when a new patchset is available. https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/app_sync_bundle.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/app_sync_bundle.cc:126: ProcessSyncChange(it->second); On 2015/06/24 20:47:38, kalman wrote: > > Well, in the current state, ProcessExtensionSyncDataHelper gives us no clue > why > > it said "false" before, so "retry whenever something changed" is kind of the > > best we can do. But maybe we should change that :) > > Indeed. It wouldn't be too hard of a transformation though it does mildly > terrify me that what should be an innocuous change... well, who knows. I'll take a shot at it. Let's see where I'll end up :) > > Yeah, that's kind of messed up: Right now, *nothing ever tries again later* > (if > > Sync is up already). > > I followed the path up from GetPendingData() and it ends up in > ExtensionSyncService::MergeDataAndStartSyncing() where those pending changes are > applied, and which is called when sync is set up. Does that count as "trying > again" or is there something else going wrong? IIUC MergeDataAndStartSyncing is called only once in the lifetime of the service, and that's when Sync starts up. So if we queue up any pending changes after Sync has started up, then those will never be applied. https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1687: extension->id(), Extension::DISABLE_PERMISSIONS_INCREASE)) { On 2015/06/24 20:47:38, kalman wrote: > > Hm. DidExtensionEscalatePermissions checks for DISABLE_PERMISSIONS_INCREASE | > > DISABLE_REMOTE_INSTALL. Now DISABLE_REMOTE_INSTALL shouldn't ever be set for a > > supervised user, but still... > > From purely an interface perspective - without looking at the code or figuring > out its implications - it seems like calling DidExtensionEscalatePermissions is > the right thing to do? From an implementation perspective, what *will* (or > should) happen if DISABLE_REMOTE_INSTALL is set for a supervised user? Hm. Right now, if the extension doesn't come from the custodian (installed_by_custodian flag), then it'll be blocked anyway, so I guess it doesn't matter. So, done. https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:541: // re-enable request to the custodian. On 2015/06/24 20:47:38, kalman wrote: > On 2015/06/24 11:50:57, Marc Treib wrote: > > On 2015/06/24 00:25:10, kalman wrote: > > > Is this actually necessary? I would have expected there to be code > elsewhere, > > > somewhere, that sees an update and makes an update request. > > > > Hm, I'd need to remove the "don't send duplicate permission requests" check > that > > I just added in extension_service.cc again. Then we would not need to send > > requests here, at the cost of some extraneous requests. But those shouldn't > > hurt, so maybe that's the better option overall. > > Mm I don't understand why you'd need to remove that check to have this work, but > now that I read this code again, I think what you're written isn't wrong: drop > the change if it's for an older version. > > I think it can happen outside supervised users though - just like you said, more > common because there are 2 users involved not 1. But I think you could simplify > this (insofar as pulling out of supervised users) but basically saying: if any > sync change happens for an older version, we should drop it. That prevents > rolling back extensions but wow if that ever needs to happen we're in serious > trouble all over the place. Wait, rolling back extensions isn't possible anyway, right? Omaha only ever gives us the latest version AFAIK..?
Cool, ping when you're done. https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:541: // re-enable request to the custodian. > Wait, rolling back extensions isn't possible anyway, right? Omaha only ever > gives us the latest version AFAIK..? Yes, I just meant the case when an update from sync comes for an older version. Obviously Chrome can't do much with it because Omaha only keeps the latest version - and nor should it anyway, because there is a never version. My point was just that rather than trying to write any special code at all, we should simply have a check "if (sync_change.version < current_version) return" or the equivalent. or does this code need to be special to handle the cross-managed-and-supervised user flow?
Alright, I'll need to do this in stages... the plan is: 1. Cleanup/code deduplication. 2. Logic changes for all users (defer applying sync updates until the version matches). 3. Special supervised user code (this CL). Part 1 will be coming your way soon. https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1200833004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:541: // re-enable request to the custodian. On 2015/06/29 22:59:04, kalman wrote: > > > Wait, rolling back extensions isn't possible anyway, right? Omaha only ever > > gives us the latest version AFAIK..? > > Yes, I just meant the case when an update from sync comes for an older version. > Obviously Chrome can't do much with it because Omaha only keeps the latest > version - and nor should it anyway, because there is a never version. > > My point was just that rather than trying to write any special code at all, we > should simply have a check "if (sync_change.version < current_version) return" > or the equivalent. I'm totally happy to do this for all users, and have one less "if(supervised)" :) > or does this code need to be special to handle the cross-managed-and-supervised > user flow? For SUs, we still need to send a permission request *somewhere*, and I'm not sure yet if there's a better place for that than here. But having special code for that is still way better than changing the sync behavior for SUs.
On 2015/06/30 08:32:37, Marc Treib wrote: > Alright, I'll need to do this in stages... the plan is: > 1. Cleanup/code deduplication. > 2. Logic changes for all users (defer applying sync updates until the version > matches). > 3. Special supervised user code (this CL). Sorry, on behalf of past/present/future Extensions team, for the big yak shave :-(
treib@chromium.org changed reviewers: + rdevlin.cronin@chromium.org - kalman@chromium.org
PTAL!
https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_service_unittest.cc:7196: public ExtensionServiceTest, public testing::WithParamInterface<bool> {}; Parameterized tests?!?!? What is this black magic?? ... nifty. https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_service_unittest.cc:7257: if (!need_custodian_approval) What has this succeeded in testing in this case? Would it be more valuable to check other branches and see the expected outcome? (What *is* the expected outcome?) https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_service_unittest.cc:7305: // The re-enable should be delayed until the extension is updated to the Do we have a test for it being enabled when the version does match? https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_service_unittest.cc:7311: path = base_path.AppendASCII("v3"); nit: inline path (everywhere) https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:155: return up.version.Equals(version) && up.grant_permissions_and_reenable; Nit: up? Do you pu? Either way, I prefer short, full names to hard-to-decipher acronyms. So, in this case, maybe 'pending'. https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.h (right): https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.h:48: bool HasPendingReenable(const std::string& id, Nit: method comments https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_service.cc:114: LOG_IF(WARNING, !success) << "Failed sending update request for " << id; LOGs are annoying. We never read them, except when it's something random that happens and no one understands it. When can this happen? If it shouldn't (ever), (D)CHECK. If it can and we care about metrics/knowing about it, UMA it. If it can and we just don't know what to do, but feel bad doing nothing, make it a VLOG. https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_service.cc:266: extension_id + ":" + version.GetString())); nitty nit: prefer base::StringPrintf for times like this. https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_service.h (right): https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_service.h:120: // Same as above, but without a callback, for when the caller doesn't care Some of this comment seems unnecessary - I think the "for when the caller doesn't care about the result" is implied by the lack of callback. ;) How about just "Same as above, but without a callback, just logging errors on failure."
Comments addressed, except for the missing test, which will follow shortly :) https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/extension_service_unittest.cc (right): https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_service_unittest.cc:7196: public ExtensionServiceTest, public testing::WithParamInterface<bool> {}; On 2015/10/16 02:40:24, Devlin wrote: > Parameterized tests?!?!? What is this black magic?? > > ... nifty. In this case, it's arguably more trouble than it's worth... but oh well. It works :) https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_service_unittest.cc:7257: if (!need_custodian_approval) On 2015/10/16 02:40:24, Devlin wrote: > What has this succeeded in testing in this case? Would it be more valuable to > check other branches and see the expected outcome? (What *is* the expected > outcome?) The extension was updated but disabled, and (in particular) a permission request was sent iff we need custodian approval. What "other branches"? https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_service_unittest.cc:7305: // The re-enable should be delayed until the extension is updated to the On 2015/10/16 02:40:24, Devlin wrote: > Do we have a test for it being enabled when the version does match? Good point, we don't :D I'll add one and ping you again. (I think I'll have to split this test into multiple ones. Oh well.) https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_service_unittest.cc:7311: path = base_path.AppendASCII("v3"); On 2015/10/16 02:40:24, Devlin wrote: > nit: inline path (everywhere) Done. https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.cc:155: return up.version.Equals(version) && up.grant_permissions_and_reenable; On 2015/10/16 02:40:24, Devlin wrote: > Nit: up? Do you pu? Either way, I prefer short, full names to hard-to-decipher > acronyms. So, in this case, maybe 'pending'. Done. https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/extension_sync_service.h (right): https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_sync_service.h:48: bool HasPendingReenable(const std::string& id, On 2015/10/16 02:40:24, Devlin wrote: > Nit: method comments Done. https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_service.cc (right): https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_service.cc:114: LOG_IF(WARNING, !success) << "Failed sending update request for " << id; On 2015/10/16 02:40:24, Devlin wrote: > LOGs are annoying. We never read them, except when it's something random that > happens and no one understands it. When can this happen? > > If it shouldn't (ever), (D)CHECK. If it can and we care about metrics/knowing > about it, UMA it. If it can and we just don't know what to do, but feel bad > doing nothing, make it a VLOG. It can happen when the server we talk to is down, etc. I think it's rare enough that it won't be annoying, but eh. It's now a VLOG. https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_service.cc:266: extension_id + ":" + version.GetString())); On 2015/10/16 02:40:24, Devlin wrote: > nitty nit: prefer base::StringPrintf for times like this. Done. https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/supervis... File chrome/browser/supervised_user/supervised_user_service.h (right): https://codereview.chromium.org/1200833004/diff/80001/chrome/browser/supervis... chrome/browser/supervised_user/supervised_user_service.h:120: // Same as above, but without a callback, for when the caller doesn't care On 2015/10/16 02:40:24, Devlin wrote: > Some of this comment seems unnecessary - I think the "for when the caller > doesn't care about the result" is implied by the lack of callback. ;) > > How about just "Same as above, but without a callback, just logging errors on > failure." Done.
Missing test is here now! I had to split up that parameterized test. Now there's separate ones for "don't need custodian approval", and "need custodian approval" x old/matching/new version. Good thing too, since the splitting revealed a bug that was hidden by having multiple tests in one... PTAL! https://codereview.chromium.org/1200833004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/extension_sync_service.cc (right): https://codereview.chromium.org/1200833004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/extension_sync_service.cc:360: if (is_privilege_increase && version_compare_result > 0 && This was that bug - I was generating a permission request in exactly the wrong case. It didn't show in the test since I was creating both and old and a new approval...
LGTM
The CQ bit was checked by treib@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1200833004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1200833004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/40d3ad9f1201248f2eaa169c5d6b528bc42afbb4 Cr-Commit-Position: refs/heads/master@{#355106} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
