|
|
Created:
3 years, 7 months ago by Ivan Šandrk Modified:
3 years, 7 months ago Reviewers:
Alexander Alekseev, Devlin, Mattias Nissler (ping if slow), Ivan Sandrk, Andrew T Wilson (Slow), Avi (use Gerrit), Lei Zhang CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPS - Showing permission prompt for activeTab
In Public Sessions, apps and extensions are force-installed by admin policy so the user does not get a chance to review the permissions for these apps. This is not acceptable from a security standpoint, so we:
- show a permission prompt the first time an extension tries to use activeTab permission (and deny that first request).
BUG=717945
Review-Url: https://codereview.chromium.org/2858013002
Cr-Commit-Position: refs/heads/master@{#475128}
Committed: https://chromium.googlesource.com/chromium/src/+/98a3e4a19ea5cbfe16f3486b14e458df441c394f
Patch Set 1 #Patch Set 2 : Using a platform specific delegate for the check #
Total comments: 2
Patch Set 3 : Adapted PublicSessionPermissionHelper to fit this use case #
Total comments: 6
Patch Set 4 : Nits, function for setting up delegates #
Total comments: 2
Patch Set 5 : active_tab_granted check used also for AllHosts #Patch Set 6 : Rebase #
Total comments: 11
Patch Set 7 : Devlin's suggestions #Patch Set 8 : Added permission message for activeTab; partial tests #
Total comments: 4
Patch Set 9 : Test - delegate is set and works #
Total comments: 8
Patch Set 10 : SetPlatformDelegate returns old delegate #Patch Set 11 : Rename to AddPermissions, git cl format, nits #Patch Set 12 : Rebase #Patch Set 13 : Update a unittest (.mm) #
Total comments: 7
Patch Set 14 : Changed permission prompt message #
Total comments: 1
Messages
Total messages: 103 (67 generated)
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== PS - Showing permission prompt for activeTab In Public Sessions, apps and extensions are force-installed by admin policy so the user does not get a chance to review the permissions for these apps. This is not acceptable from a security standpoint, so we: - show a permission prompt the first time an extension tries to use activeTab permission. BUG=717945 ========== to ========== PS - Showing permission prompt for activeTab In Public Sessions, apps and extensions are force-installed by admin policy so the user does not get a chance to review the permissions for these apps. This is not acceptable from a security standpoint, so we: - show a permission prompt the first time an extension tries to use activeTab permission (and deny the request). BUG=717945 ==========
Description was changed from ========== PS - Showing permission prompt for activeTab In Public Sessions, apps and extensions are force-installed by admin policy so the user does not get a chance to review the permissions for these apps. This is not acceptable from a security standpoint, so we: - show a permission prompt the first time an extension tries to use activeTab permission (and deny the request). BUG=717945 ========== to ========== PS - Showing permission prompt for activeTab In Public Sessions, apps and extensions are force-installed by admin policy so the user does not get a chance to review the permissions for these apps. This is not acceptable from a security standpoint, so we: - show a permission prompt the first time an extension tries to use activeTab permission (and deny that first request). BUG=717945 ==========
isandrk@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
Hey Devlin, just a preliminary review. I'm not sure about the design I should use here, should I also use some kind of a delegate?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:40001) has been deleted
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
isandrk@chromium.org changed reviewers: + alemate@chromium.org, atwilson@chromium.org
Using a delegate implementation now. I think I prefer PS3 version over PS2. Devlin, Drew, ptal! Alexander, ptal at chrome/browser/chromeos/login/users! https://codereview.chromium.org/2858013002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos.cc (right): https://codereview.chromium.org/2858013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos.cc:49: permission_helper::HandlePermissionRequest( Should I add a brief comment here explaining the mechanics? https://codereview.chromium.org/2858013002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right): https://codereview.chromium.org/2858013002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:858: extensions::ActiveTabPermissionGranter::SetPlatformDelegate( At this point might as well break out this delegate initialization code into its own function. WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Sorry I didn't get to this sooner. I'm actually a bit confused on this one - activeTab is only granted when the user explicitly invokes an extension on the page, indicating they trust it and are okay sharing the page with it. We don't even have a permission warning for activeTab, so there's nothing that the user hasn't had a chance to review. Why do we need one for public sessions?
isandrk@chromium.org changed reviewers: + mnissler@chromium.org
You're totally right, completely forgot about that. Mattias, can you chime in please?
Should've posted this earlier, this is the reasoning behind the choice: Allows access to web contents in response to user gesture. Note that this doesn't trigger a permission warning on install though, so blocking is somewhat at odds with the spirit of the API - however I presume the API design assumes user-installed extensions, which we don't have here.
lgtm https://codereview.chromium.org/2858013002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2858013002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/public_session_permission_helper.h:51: // Callback can be null (permission_helper::RequestResolvedCallback()). What does it mean if it's null? Does it still prompt (but doesn't invoke the callback when the prompt is interacted with)? https://codereview.chromium.org/2858013002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right): https://codereview.chromium.org/2858013002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:859: new extensions::ActiveTabPermissionGranterDelegateChromeOS); This comment is fine as-is, but it's basically a copy-paste of the previous comment. You could probably make a single comment that describes both pieces of delegate setting and make this code slightly easier to read. https://codereview.chromium.org/2858013002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/active_tab_permission_granter.cc (right): https://codereview.chromium.org/2858013002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/active_tab_permission_granter.cc:107: if (permissions_data->HasWithheldImpliedAllHosts() || Why isn't HasWithheldImpliedAllHosts() subject to active_tab_granted as well?
On 2017/05/08 12:56:10, Ivan Šandrk wrote: > Should've posted this earlier, this is the reasoning behind the choice: > > Allows access to web contents in response to user gesture. Note that this > doesn't trigger a permission warning on install though, so blocking is somewhat > at odds with the spirit of the API - however I presume the API design assumes > user-installed extensions, which we don't have here. The user gesture is in the invocation of the extension itself, though (e.g., through clicking on the extension icon), not just through any user gesture. Even though the user didn't install the extension, the user chose to invoke the extension on that web contents. Is that not indication enough that the user trusts the extension on that web page, and, if not, why is it significantly different than the activeTab permission in general (which is permission-warning-less)? Extension installation *is* an indication of trust (which is why we grant some nuisance-style APIs without permission warnings), but for activeTab we've decided that the invocation, not the installation, is the signal of trust, so this seems a bit weird.
On 2017/05/09 22:02:05, Devlin (OOO until May 12) wrote: > On 2017/05/08 12:56:10, Ivan Šandrk wrote: > > Should've posted this earlier, this is the reasoning behind the choice: > > > > Allows access to web contents in response to user gesture. Note that this > > doesn't trigger a permission warning on install though, so blocking is > somewhat > > at odds with the spirit of the API - however I presume the API design assumes > > user-installed extensions, which we don't have here. > > The user gesture is in the invocation of the extension itself, though (e.g., > through clicking on the extension icon), not just through any user gesture. > Even though the user didn't install the extension, the user chose to invoke the > extension on that web contents. Is that not indication enough that the user > trusts the extension on that web page, and, if not, why is it significantly > different than the activeTab permission in general (which is > permission-warning-less)? Extension installation *is* an indication of trust > (which is why we grant some nuisance-style APIs without permission warnings), > but for activeTab we've decided that the invocation, not the installation, is > the signal of trust, so this seems a bit weird. I don't have a strong opinion here, although in the case of public sessions it's not clear that the user understands the ramification of clicking the browser action button since they didn't install the extension. I guess the fear is that I could create a button that looks friendly/harmless/useful, but actually scrapes info from your current tab and uploads it. activeTab permission gives you pretty intensive permissions, right (can scrape cookies, etc?) I suspect that the permission grant for activeTab isn't precisely the button press - it's the button + installation. Without the installation, the user doesn't really know what the button does so reducing the privileges of the initial button press seems potentially useful.
On 2017/05/10 09:32:33, Andrew T Wilson (Slow) wrote: > On 2017/05/09 22:02:05, Devlin (OOO until May 12) wrote: > > On 2017/05/08 12:56:10, Ivan Šandrk wrote: > > > Should've posted this earlier, this is the reasoning behind the choice: > > > > > > Allows access to web contents in response to user gesture. Note that this > > > doesn't trigger a permission warning on install though, so blocking is > > somewhat > > > at odds with the spirit of the API - however I presume the API design > assumes > > > user-installed extensions, which we don't have here. > > > > The user gesture is in the invocation of the extension itself, though (e.g., > > through clicking on the extension icon), not just through any user gesture. > > Even though the user didn't install the extension, the user chose to invoke > the > > extension on that web contents. Is that not indication enough that the user > > trusts the extension on that web page, and, if not, why is it significantly > > different than the activeTab permission in general (which is > > permission-warning-less)? Extension installation *is* an indication of trust > > (which is why we grant some nuisance-style APIs without permission warnings), > > but for activeTab we've decided that the invocation, not the installation, is > > the signal of trust, so this seems a bit weird. > > I don't have a strong opinion here, although in the case of public sessions it's > not clear that the user understands the ramification of clicking the browser > action button since they didn't install the extension. I guess the fear is that > I could create a button that looks friendly/harmless/useful, but actually > scrapes info from your current tab and uploads it. activeTab permission gives > you pretty intensive permissions, right (can scrape cookies, etc?) > > I suspect that the permission grant for activeTab isn't precisely the button > press - it's the button + installation. Without the installation, the user > doesn't really know what the button does so reducing the privileges of the > initial button press seems potentially useful. Drew summarizes our thinking very nicely. I'm not sure whether a permission prompt is the right UX, but I don't have any compelling alternative suggestions either. This is further complicated by the fact that depending on the nature of the extension, it might be totally harmless (and warning the user that they're exposing their data to the admin would be inaccurate).
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2858013002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2858013002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/extensions/public_session_permission_helper.h:51: // Callback can be null (permission_helper::RequestResolvedCallback()). On 2017/05/09 13:29:29, Andrew T Wilson (Slow) wrote: > What does it mean if it's null? Does it still prompt (but doesn't invoke the > callback when the prompt is interacted with)? Updated comment. And both correct - prompts but doesn't invoke the callback. https://codereview.chromium.org/2858013002/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc (right): https://codereview.chromium.org/2858013002/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc:859: new extensions::ActiveTabPermissionGranterDelegateChromeOS); On 2017/05/09 13:29:29, Andrew T Wilson (Slow) wrote: > This comment is fine as-is, but it's basically a copy-paste of the previous > comment. You could probably make a single comment that describes both pieces of > delegate setting and make this code slightly easier to read. Put them all in a separate function, does it look better now? https://codereview.chromium.org/2858013002/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/active_tab_permission_granter.cc (right): https://codereview.chromium.org/2858013002/diff/60001/chrome/browser/extensio... chrome/browser/extensions/active_tab_permission_granter.cc:107: if (permissions_data->HasWithheldImpliedAllHosts() || On 2017/05/09 13:29:29, Andrew T Wilson (Slow) wrote: > Why isn't HasWithheldImpliedAllHosts() subject to active_tab_granted as well? It's unrelated to activeTab functionality. Afaict it's used to temporarily withhold all_urls host permissions. I mean they are related, but in the sense that either of them can grant the extension access to the current tab/page (activeTab needs to be invoked, while all_urls just gives blanket access to all pages). We block all_urls in public sessions.
https://codereview.chromium.org/2858013002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/active_tab_permission_granter.cc (right): https://codereview.chromium.org/2858013002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/active_tab_permission_granter.cc:107: if (permissions_data->HasWithheldImpliedAllHosts() || I'd still probably change this to: if (active_tab_granted && (permissions_data->HasWithheldImpliedAllHosts() || permissions_data->HasAPIPermission(kActiveTab))) { } No reason to exclude AllHosts from the active_tab_granted check esp since we block AllHosts in public sessions. Safer to do it this way.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
https://codereview.chromium.org/2858013002/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/active_tab_permission_granter.cc (right): https://codereview.chromium.org/2858013002/diff/80001/chrome/browser/extensio... chrome/browser/extensions/active_tab_permission_granter.cc:107: if (permissions_data->HasWithheldImpliedAllHosts() || On 2017/05/10 10:33:49, Andrew T Wilson (Slow) wrote: > I'd still probably change this to: > > if (active_tab_granted && > (permissions_data->HasWithheldImpliedAllHosts() || > permissions_data->HasAPIPermission(kActiveTab))) { > } > > No reason to exclude AllHosts from the active_tab_granted check esp since we > block AllHosts in public sessions. Safer to do it this way. Done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
lgtm
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I'm still a little dubious on the reasoning behind this - to me, it still seems like if the user clicks on the extension to have it run on the page, it should be implicit it will access the page. But I'll defer to y'all. One other concern: what does the permission prompt show? We don't have an active tab permission, so do any warnings appear? And, of course, tests would be nice. :) https://codereview.chromium.org/2858013002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos.cc (right): https://codereview.chromium.org/2858013002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos.cc:24: if (!profiles::IsPublicSession() || Do we set the delegate if it's not a public session? Or can this be a DCHECK? Do we need to worry about the user logging out and it no longer being a public session? https://codereview.chromium.org/2858013002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos.cc:25: chromeos::DeviceLocalAccountManagementPolicyProvider::IsWhitelisted( Would it make sense to put this check in the permission helper? Or does the whitelist only apply to certain permissions? https://codereview.chromium.org/2858013002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/active_tab_permission_granter.cc (right): https://codereview.chromium.org/2858013002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/active_tab_permission_granter.cc:115: if (permissions_data->HasAPIPermission(APIPermission::kTabCapture)) do we want to gate this on whether or not we grant it as well? (We might be safe because kTabCapture might not be whitelisted, but that seems like a tenuous guarantee.) I think it might be cleaner to just do: if (g_delegate && !g_delegate->ActiveTabPermissionGranted()) return; // Delegate refused permission. https://codereview.chromium.org/2858013002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/active_tab_permission_granter.h (right): https://codereview.chromium.org/2858013002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/active_tab_permission_granter.h:39: // Platform specific check whether the user granted activeTab permission. I think rather than "whether the user granted activeTab", this should be something like "whether the activeTab permission is allowed". Other delegates might gate this on something other than user dialogs. https://codereview.chromium.org/2858013002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/active_tab_permission_granter.h:40: virtual bool ActiveTabPermissionGranted( "ActiveTabPermissionGranted" is a strange name for a method that determines *if* activeTab should be granted. Maybe "ShouldGrantActiveTab"?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
> One other concern: what does the permission prompt show? We don't have an active tab permission, so do any warnings appear? Thanks for raising this, I was being a featherbrain, and I didn't even look at the prompt -https://screenshot.googleplex.com/1oN1zLqULEp.png Would you be ok with me adding another rule to the following for activeTab permission? https://cs.chromium.org/chromium/src/chrome/common/extensions/permissions/chr... Or do you maybe have other ideas? > And, of course, tests would be nice. :) Working on it :) https://codereview.chromium.org/2858013002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos.cc (right): https://codereview.chromium.org/2858013002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos.cc:24: if (!profiles::IsPublicSession() || On 2017/05/12 14:56:45, Devlin (catching up) wrote: > Do we set the delegate if it's not a public session? Or can this be a DCHECK? Nope. I guess I'm the kind of person that likes redundancy, but I can make it a DCHECK (or even completely remove it). Done. > Do we need to worry about the user logging out and it no longer being a public > session? Nope. The whole browser process is terminated at the end of logout, everything goes away. https://codereview.chromium.org/2858013002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos.cc:25: chromeos::DeviceLocalAccountManagementPolicyProvider::IsWhitelisted( On 2017/05/12 14:56:45, Devlin (catching up) wrote: > Would it make sense to put this check in the permission helper? Or does the > whitelist only apply to certain permissions? Whitelisted extensions should have unlimited access. Also a good idea, I've made the changes. https://codereview.chromium.org/2858013002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/active_tab_permission_granter.cc (right): https://codereview.chromium.org/2858013002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/active_tab_permission_granter.cc:115: if (permissions_data->HasAPIPermission(APIPermission::kTabCapture)) On 2017/05/12 14:56:45, Devlin (catching up) wrote: > do we want to gate this on whether or not we grant it as well? (We might be > safe because kTabCapture might not be whitelisted, but that seems like a tenuous > guarantee.) > > I think it might be cleaner to just do: > if (g_delegate && !g_delegate->ActiveTabPermissionGranted()) > return; // Delegate refused permission. kTabCapture is already dealt with[1], so I'd keep it as is here (so this only affects the activeTab permission). [1] chrome/browser/media/public_session_tab_capture_access_handler.cc https://codereview.chromium.org/2858013002/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/active_tab_permission_granter.h (right): https://codereview.chromium.org/2858013002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/active_tab_permission_granter.h:39: // Platform specific check whether the user granted activeTab permission. On 2017/05/12 14:56:45, Devlin (catching up) wrote: > I think rather than "whether the user granted activeTab", this should be > something like "whether the activeTab permission is allowed". Other delegates > might gate this on something other than user dialogs. Good point, done. https://codereview.chromium.org/2858013002/diff/120001/chrome/browser/extensi... chrome/browser/extensions/active_tab_permission_granter.h:40: virtual bool ActiveTabPermissionGranted( On 2017/05/12 14:56:45, Devlin (catching up) wrote: > "ActiveTabPermissionGranted" is a strange name for a method that determines *if* > activeTab should be granted. Maybe "ShouldGrantActiveTab"? Another good one, done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/12 18:27:44, Ivan Šandrk wrote: > > One other concern: what does the permission prompt show? We don't have an > active tab permission, so do any warnings appear? > Thanks for raising this, I was being a featherbrain, and I didn't even look at > the prompt -https://screenshot.googleplex.com/1oN1zLqULEp.png > > Would you be ok with me adding another rule to the following for activeTab > permission? > https://cs.chromium.org/chromium/src/chrome/common/extensions/permissions/chr... That won't work, because it would add the warning for installing new extensions - which is definitely something we don't want. > Or do you maybe have other ideas? Hmm... this is tricky. Unless I can convince y'all it's not necessary and that our stance on activeTab should translate to public sessions. :D Really, that would simplify a lot... For one thing, this part, but for another there's two other concerns I have (in addition to the discontinuity between PS and non-PS). 1. The UX is a bit tricky here. The user clicks on the extension action to invoke it, and we pop up a dialog saying "Extension foo needs to be able to run on the page", the user hits "OK", and then... nothing happens, right? The user needs to then hit the extension *again* to make it work, since we auto-rejected the first request for activeTab. I'd think that, to a user, that would look like a bug. We could potentially mitigate that by delaying the dispatch of the user action to the extension until the dialog has been dealt with, but that's a *lot* more complexity... 2. It's hard to say how extensions will deal with this. For example, if an extension injects a script in its browserAction.onClicked handler, that script will fail - possibly sending the extension into a bizarre state. But maybe that's less of a concern - I'm not sure if our stance is that extensions running in PS should account for different behaviors (likely to some extent, they'll have to). Assuming that's still a no-go (one last attempt never hurt! ;)), there's not a *great* solution to this. The best I could come up with would be to introduce a AddCustomWarnings()-type method to the ExtensionInstallPrompt. That would probably work and be reasonably simple, though it is a bit of a shame IMHO. > > And, of course, tests would be nice. :) > Working on it :) https://codereview.chromium.org/2858013002/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos.cc (right): https://codereview.chromium.org/2858013002/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos.cc:24: if (!profiles::IsPublicSession() || On 2017/05/12 18:27:44, Ivan Šandrk wrote: > On 2017/05/12 14:56:45, Devlin (catching up) wrote: > > Do we set the delegate if it's not a public session? Or can this be a DCHECK? > Nope. I guess I'm the kind of person that likes redundancy, but I can make it a > DCHECK (or even completely remove it). Done. > > > Do we need to worry about the user logging out and it no longer being a public > > session? > Nope. The whole browser process is terminated at the end of logout, everything > goes away. We should be careful with redundancy when it means guarding against cases that should never happen. That's where DCHECKs come in handy - they are a) some small assurance that it doesn't happen (at least on the bots), and b) documentation of the guarantees we think we have. But otherwise, this redundancy could potentially indicate a bug, so it's good to catch. Of course, like most things, it's a balance (and very likely, subjective). :)
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Devlin, thanks for the tip. Ptal! Mattias: ptal at chrome/app/generated_resources.grd (that's the warning that will be shown for activeTab permission to the users). And I know, more tests... They will come :) https://codereview.chromium.org/2858013002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_install_prompt.cc (left): https://codereview.chromium.org/2858013002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_install_prompt.cc:171: install_permissions.is_showing_details.clear(); Didn't really see a reason for them to be cleared here, so I removed this part since it was clearing the permissions I set on the prompt before calling ShowDialog(). If that's not acceptable, I'll find another way :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Added a comment. https://codereview.chromium.org/2858013002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_install_prompt.cc (left): https://codereview.chromium.org/2858013002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_install_prompt.cc:171: install_permissions.is_showing_details.clear(); On 2017/05/17 19:10:21, Ivan Šandrk wrote: > Didn't really see a reason for them to be cleared here, so I removed this part > since it was clearing the permissions I set on the prompt before calling > ShowDialog(). If that's not acceptable, I'll find another way :) Or I can also do a "renaming" operation, s/SetPermissions/AddPermissions/ :) (none of the uses of this function relies on the set part, apart from it being used to add the permissions once)
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2858013002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_install_prompt.cc (left): https://codereview.chromium.org/2858013002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_install_prompt.cc:171: install_permissions.is_showing_details.clear(); On 2017/05/18 12:20:20, Ivan Šandrk wrote: > On 2017/05/17 19:10:21, Ivan Šandrk wrote: > > Didn't really see a reason for them to be cleared here, so I removed this part > > since it was clearing the permissions I set on the prompt before calling > > ShowDialog(). If that's not acceptable, I'll find another way :) > > Or I can also do a "renaming" operation, s/SetPermissions/AddPermissions/ :) > > (none of the uses of this function relies on the set part, apart from it being > used to add the permissions once) I think renaming to AddPermissions would make this more clear. https://codereview.chromium.org/2858013002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos.cc (right): https://codereview.chromium.org/2858013002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos.cc:28: if (already_handled) { nit: return already_handled && permission_helper::PermissionAllowed(extension, APIPermission::kActiveTab); https://codereview.chromium.org/2858013002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2858013002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:178: if (!PermissionCheckNeeded(extension)) return !PermissionCheckNeeded(extension) || allowed_permission_set_.ContainsID(permission); https://codereview.chromium.org/2858013002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/active_tab_permission_granter.cc (right): https://codereview.chromium.org/2858013002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/active_tab_permission_granter.cc:95: free(g_delegate); Why do we need this? Just for ASAN? https://codereview.chromium.org/2858013002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/active_tab_permission_granter.cc:109: g_delegate->ShouldGrantActiveTab(extension, web_contents()); Is this git cl format'd?
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2858013002/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/extension_install_prompt.cc (left): https://codereview.chromium.org/2858013002/diff/160001/chrome/browser/extensi... chrome/browser/extensions/extension_install_prompt.cc:171: install_permissions.is_showing_details.clear(); On 2017/05/23 19:28:03, Devlin (catching up) wrote: > On 2017/05/18 12:20:20, Ivan Šandrk wrote: > > On 2017/05/17 19:10:21, Ivan Šandrk wrote: > > > Didn't really see a reason for them to be cleared here, so I removed this > part > > > since it was clearing the permissions I set on the prompt before calling > > > ShowDialog(). If that's not acceptable, I'll find another way :) > > > > Or I can also do a "renaming" operation, s/SetPermissions/AddPermissions/ :) > > > > (none of the uses of this function relies on the set part, apart from it being > > used to add the permissions once) > > I think renaming to AddPermissions would make this more clear. Done. https://codereview.chromium.org/2858013002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos.cc (right): https://codereview.chromium.org/2858013002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos.cc:28: if (already_handled) { On 2017/05/23 19:28:03, Devlin (catching up) wrote: > nit: > return already_handled && > permission_helper::PermissionAllowed(extension, > APIPermission::kActiveTab); Done. https://codereview.chromium.org/2858013002/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.cc (right): https://codereview.chromium.org/2858013002/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.cc:178: if (!PermissionCheckNeeded(extension)) On 2017/05/23 19:28:03, Devlin (catching up) wrote: > return !PermissionCheckNeeded(extension) || > allowed_permission_set_.ContainsID(permission); Done. https://codereview.chromium.org/2858013002/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/active_tab_permission_granter.cc (right): https://codereview.chromium.org/2858013002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/active_tab_permission_granter.cc:95: free(g_delegate); On 2017/05/23 19:28:03, Devlin (catching up) wrote: > Why do we need this? Just for ASAN? Yes https://codereview.chromium.org/2858013002/diff/180001/chrome/browser/extensi... chrome/browser/extensions/active_tab_permission_granter.cc:109: g_delegate->ShouldGrantActiveTab(extension, web_contents()); On 2017/05/23 19:28:04, Devlin (catching up) wrote: > Is this git cl format'd? Nope, now it is :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...) win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by isandrk@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm I'm still a bit dubious about requiring a dialog here, but y'all are the public sessions gurus, so deferring to you. :) https://codereview.chromium.org/2858013002/diff/260001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2858013002/diff/260001/chrome/app/generated_r... chrome/app/generated_resources.grd:3593: Read and change all your data on the current website This could be a little misleading. IIUC, we show the prompt once per extension, right? So envision the flow: user clicks extension on example.com sees prompt "Read and change all your data on the current website" * current website is example.com accepts navigates to google.com clicks extension * extension has access to google.com, even though the prompt said "current website" Maybe we should rephrase this to something like: "Read and change all your data on the current website when activated" or similar? Unfortunately, "activated" sounds kinda weird. Not sure what the best string would be. https://codereview.chromium.org/2858013002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos.cc (right): https://codereview.chromium.org/2858013002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos.cc:23: bool already_handled = permission_helper::HandlePermissionRequest( if you take the other comment, this could become return HandlePermissionRequest() == ALLOWED; https://codereview.chromium.org/2858013002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2858013002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:63: bool PermissionAllowed(const Extension* extension, I wonder if instead of this, we should have HandlePermissionRequest() return an enum of { ALLOWED, BLOCKED, PENDING }. Is it ever expected that a caller would check PermissionAllowed() without HandlePermissionRequest? If not, it could be nice to bundle them and save the extra lookup.
https://codereview.chromium.org/2858013002/diff/260001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2858013002/diff/260001/chrome/app/generated_r... chrome/app/generated_resources.grd:3593: Read and change all your data on the current website On 2017/05/25 20:52:57, Devlin (catching up) wrote: > This could be a little misleading. IIUC, we show the prompt once per extension, > right? So envision the flow: > user clicks extension on http://example.com > sees prompt "Read and change all your data on the current website" > * current website is http://example.com > accepts > navigates to http://google.com > clicks extension > * extension has access to http://google.com, even though the prompt said "current > website" > > Maybe we should rephrase this to something like: > "Read and change all your data on the current website when activated" or > similar? Unfortunately, "activated" sounds kinda weird. Not sure what the best > string would be. Would it make sense to say "all information within this tab"? (Assuming that's accurate and we're generally OK with mentioning "tab" as a technical term in UI)
https://codereview.chromium.org/2858013002/diff/260001/chrome/app/generated_r... File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2858013002/diff/260001/chrome/app/generated_r... chrome/app/generated_resources.grd:3593: Read and change all your data on the current website On 2017/05/26 13:44:47, Mattias Nissler (ping if slow) wrote: > On 2017/05/25 20:52:57, Devlin (catching up) wrote: > > This could be a little misleading. IIUC, we show the prompt once per > extension, > > right? So envision the flow: > > user clicks extension on http://example.com > > sees prompt "Read and change all your data on the current website" > > * current website is http://example.com > > accepts > > navigates to http://google.com > > clicks extension > > * extension has access to http://google.com, even though the prompt said > "current > > website" > > > > Maybe we should rephrase this to something like: > > "Read and change all your data on the current website when activated" or > > similar? Unfortunately, "activated" sounds kinda weird. Not sure what the > best > > string would be. > > Would it make sense to say "all information within this tab"? (Assuming that's > accurate and we're generally OK with mentioning "tab" as a technical term in UI) I think it's not restricted to only this tab. The first time you invoke you get the permission prompt and then the activeTab is active for the current website. If you navigate away or use a different tab, activeTab isn't granted for the new page/tab, but invoking it won't cause any new permission prompts. I think Devlin's suggestion is pretty good actually, and I'd go with that one: "Read and change all your data on the current website when invoked" https://codereview.chromium.org/2858013002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2858013002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:63: bool PermissionAllowed(const Extension* extension, On 2017/05/25 20:52:57, Devlin (catching up) wrote: > I wonder if instead of this, we should have HandlePermissionRequest() return an > enum of { ALLOWED, BLOCKED, PENDING }. Is it ever expected that a caller would > check PermissionAllowed() without HandlePermissionRequest? If not, it could be > nice to bundle them and save the extra lookup. Quite a good idea, but one problem - HandlePermissionRequest() can have two requested permissions via a PermissionIDSet, so it's kinda troubling to return the correct enum sequence. I could change the input parameter to a vector, but that would require some refactoring around the codebase so I'd prefer not to do it in this CL.
https://codereview.chromium.org/2858013002/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2858013002/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/extensions/public_session_permission_helper.h:63: bool PermissionAllowed(const Extension* extension, On 2017/05/26 14:04:22, Ivan Šandrk wrote: > On 2017/05/25 20:52:57, Devlin (catching up) wrote: > > I wonder if instead of this, we should have HandlePermissionRequest() return > an > > enum of { ALLOWED, BLOCKED, PENDING }. Is it ever expected that a caller > would > > check PermissionAllowed() without HandlePermissionRequest? If not, it could > be > > nice to bundle them and save the extra lookup. > > Quite a good idea, but one problem - HandlePermissionRequest() can have two > requested permissions via a PermissionIDSet, so it's kinda troubling to return > the correct enum sequence. I could change the input parameter to a vector, but > that would require some refactoring around the codebase so I'd prefer not to do > it in this CL. Fair enough; this is fine. :)
The CQ bit was checked by isandrk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from atwilson@chromium.org, alemate@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2858013002/#ps280001 (title: "Changed permission prompt message")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_presub...)
isandrk@chromium.org changed reviewers: + avi@chromium.org
Avi, ptal at chrome/browser/ui/cocoa/extensions/extension_install_view_controller_unittest.mm
On 2017/05/26 16:11:34, Ivan Šandrk wrote: > Avi, ptal at > chrome/browser/ui/cocoa/extensions/extension_install_view_controller_unittest.mm rubber stamp lgtm
On 2017/05/26 16:13:09, Avi (ping after 24h) wrote: > On 2017/05/26 16:11:34, Ivan Šandrk wrote: > > Avi, ptal at > > > chrome/browser/ui/cocoa/extensions/extension_install_view_controller_unittest.mm > > rubber stamp lgtm That was extremely fast, thanks!
The CQ bit was checked by isandrk@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by isandrk@google.com
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 280001, "attempt_start_ts": 1495830579834680, "parent_rev": "5827cd0085ff673f5d89192761a4112e07dbb20c", "commit_rev": "98a3e4a19ea5cbfe16f3486b14e458df441c394f"}
Message was sent while issue was closed.
Description was changed from ========== PS - Showing permission prompt for activeTab In Public Sessions, apps and extensions are force-installed by admin policy so the user does not get a chance to review the permissions for these apps. This is not acceptable from a security standpoint, so we: - show a permission prompt the first time an extension tries to use activeTab permission (and deny that first request). BUG=717945 ========== to ========== PS - Showing permission prompt for activeTab In Public Sessions, apps and extensions are force-installed by admin policy so the user does not get a chance to review the permissions for these apps. This is not acceptable from a security standpoint, so we: - show a permission prompt the first time an extension tries to use activeTab permission (and deny that first request). BUG=717945 Review-Url: https://codereview.chromium.org/2858013002 Cr-Commit-Position: refs/heads/master@{#475128} Committed: https://chromium.googlesource.com/chromium/src/+/98a3e4a19ea5cbfe16f3486b14e4... ==========
Message was sent while issue was closed.
Committed patchset #14 (id:280001) as https://chromium.googlesource.com/chromium/src/+/98a3e4a19ea5cbfe16f3486b14e4...
Message was sent while issue was closed.
Findit (https://goo.gl/kROfz5) identified this CL at revision 475128 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb...
Message was sent while issue was closed.
thestig@chromium.org changed reviewers: + thestig@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2858013002/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/active_tab_unittest.cc (right): https://codereview.chromium.org/2858013002/diff/280001/chrome/browser/extensi... chrome/browser/extensions/active_tab_unittest.cc:493: free(ActiveTabPermissionGranter::SetPlatformDelegate(nullptr)); This should be a delete. Have you considered unique_ptrs?
Message was sent while issue was closed.
https://codereview.chromium.org/2907953002 |