Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(27)

Issue 2858013002: PS - Showing permission prompt for activeTab (Closed)

Created:
6 months, 2 weeks ago by Ivan Šandrk
Modified:
5 months, 3 weeks ago
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

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/+/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
Unified diffs Side-by-side diffs Delta from patch set Stats (+484 lines, -47 lines) Patch
M chrome/app/generated_resources.grd View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/BUILD.gn View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +3 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos.h View 1 2 3 4 5 6 1 chunk +39 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +32 lines, -0 lines 0 comments Download
A chrome/browser/chromeos/extensions/active_tab_permission_granter_delegate_chromeos_unittest.cc View 1 2 3 4 5 6 7 1 chunk +85 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/extensions/device_local_account_management_policy_provider.cc View 1 1 chunk +3 lines, -1 line 0 comments Download
M chrome/browser/chromeos/extensions/public_session_permission_helper.h View 1 2 3 2 chunks +10 lines, -2 lines 0 comments Download
M chrome/browser/chromeos/extensions/public_session_permission_helper.cc View 1 2 3 4 5 6 7 8 9 10 9 chunks +60 lines, -10 lines 0 comments Download
M chrome/browser/chromeos/extensions/public_session_permission_helper_unittest.cc View 1 2 3 4 5 6 7 6 chunks +66 lines, -3 lines 0 comments Download
M chrome/browser/chromeos/login/users/chrome_user_manager_impl.cc View 1 2 3 3 chunks +17 lines, -13 lines 0 comments Download
M chrome/browser/extensions/active_tab_permission_granter.h View 1 2 3 4 5 6 7 8 9 1 chunk +13 lines, -0 lines 0 comments Download
M chrome/browser/extensions/active_tab_permission_granter.cc View 1 2 3 4 5 6 7 8 9 10 3 chunks +19 lines, -2 lines 0 comments Download
M chrome/browser/extensions/active_tab_unittest.cc View 1 2 3 4 5 6 7 8 9 4 chunks +114 lines, -0 lines 1 comment Download
M chrome/browser/extensions/extension_install_prompt.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/extension_install_prompt.cc View 1 2 3 4 5 6 7 8 9 10 11 3 chunks +3 lines, -7 lines 0 comments Download
M chrome/browser/ui/cocoa/extensions/extension_install_view_controller_unittest.mm View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +6 lines, -6 lines 0 comments Download
M chrome/browser/ui/views/extensions/extension_install_dialog_view_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -2 lines 0 comments Download
M components/user_manager/user.h View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -0 lines 0 comments Download
M components/user_manager/user_manager_base.h View 1 2 3 4 5 6 7 8 1 chunk +4 lines, -0 lines 0 comments Download

Messages

Total messages: 103 (67 generated)
Ivan Šandrk
Hey Devlin, just a preliminary review. I'm not sure about the design I should use ...
6 months, 2 weeks ago (2017-05-03 18:13:54 UTC) #6
Ivan Šandrk
Using a delegate implementation now. I think I prefer PS3 version over PS2. Devlin, Drew, ...
6 months, 2 weeks ago (2017-05-05 16:24:27 UTC) #20
Devlin
Sorry I didn't get to this sooner. I'm actually a bit confused on this one ...
6 months, 2 weeks ago (2017-05-05 18:26:07 UTC) #23
Ivan Šandrk
You're totally right, completely forgot about that. Mattias, can you chime in please?
6 months, 2 weeks ago (2017-05-05 18:42:18 UTC) #25
Ivan Šandrk
Should've posted this earlier, this is the reasoning behind the choice: Allows access to web ...
6 months, 2 weeks ago (2017-05-08 12:56:10 UTC) #26
Andrew T Wilson (Slow)
lgtm https://codereview.chromium.org/2858013002/diff/60001/chrome/browser/chromeos/extensions/public_session_permission_helper.h File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2858013002/diff/60001/chrome/browser/chromeos/extensions/public_session_permission_helper.h#newcode51 chrome/browser/chromeos/extensions/public_session_permission_helper.h:51: // Callback can be null (permission_helper::RequestResolvedCallback()). What does ...
6 months, 2 weeks ago (2017-05-09 13:29:29 UTC) #27
Devlin
On 2017/05/08 12:56:10, Ivan Šandrk wrote: > Should've posted this earlier, this is the reasoning ...
6 months, 2 weeks ago (2017-05-09 22:02:05 UTC) #28
Andrew T Wilson (Slow)
On 2017/05/09 22:02:05, Devlin (OOO until May 12) wrote: > On 2017/05/08 12:56:10, Ivan Šandrk ...
6 months, 2 weeks ago (2017-05-10 09:32:33 UTC) #29
Mattias Nissler (ping if slow)
On 2017/05/10 09:32:33, Andrew T Wilson (Slow) wrote: > On 2017/05/09 22:02:05, Devlin (OOO until ...
6 months, 2 weeks ago (2017-05-10 09:39:32 UTC) #30
Ivan Šandrk
https://codereview.chromium.org/2858013002/diff/60001/chrome/browser/chromeos/extensions/public_session_permission_helper.h File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2858013002/diff/60001/chrome/browser/chromeos/extensions/public_session_permission_helper.h#newcode51 chrome/browser/chromeos/extensions/public_session_permission_helper.h:51: // Callback can be null (permission_helper::RequestResolvedCallback()). On 2017/05/09 13:29:29, ...
6 months, 2 weeks ago (2017-05-10 10:01:28 UTC) #33
Andrew T Wilson (Slow)
https://codereview.chromium.org/2858013002/diff/80001/chrome/browser/extensions/active_tab_permission_granter.cc File chrome/browser/extensions/active_tab_permission_granter.cc (right): https://codereview.chromium.org/2858013002/diff/80001/chrome/browser/extensions/active_tab_permission_granter.cc#newcode107 chrome/browser/extensions/active_tab_permission_granter.cc:107: if (permissions_data->HasWithheldImpliedAllHosts() || I'd still probably change this to: ...
6 months, 2 weeks ago (2017-05-10 10:33:49 UTC) #34
Ivan Šandrk
https://codereview.chromium.org/2858013002/diff/80001/chrome/browser/extensions/active_tab_permission_granter.cc File chrome/browser/extensions/active_tab_permission_granter.cc (right): https://codereview.chromium.org/2858013002/diff/80001/chrome/browser/extensions/active_tab_permission_granter.cc#newcode107 chrome/browser/extensions/active_tab_permission_granter.cc:107: if (permissions_data->HasWithheldImpliedAllHosts() || On 2017/05/10 10:33:49, Andrew T Wilson ...
6 months, 2 weeks ago (2017-05-10 11:11:45 UTC) #38
Alexander Alekseev
lgtm
6 months, 1 week ago (2017-05-12 04:33:17 UTC) #42
Devlin
I'm still a little dubious on the reasoning behind this - to me, it still ...
6 months, 1 week ago (2017-05-12 14:56:45 UTC) #45
Ivan Šandrk
> One other concern: what does the permission prompt show? We don't have an active ...
6 months, 1 week ago (2017-05-12 18:27:44 UTC) #50
Devlin
On 2017/05/12 18:27:44, Ivan Šandrk wrote: > > One other concern: what does the permission ...
6 months, 1 week ago (2017-05-15 20:51:12 UTC) #53
Ivan Šandrk
Devlin, thanks for the tip. Ptal! Mattias: ptal at chrome/app/generated_resources.grd (that's the warning that will ...
6 months ago (2017-05-17 19:10:21 UTC) #56
Ivan Šandrk
Added a comment. https://codereview.chromium.org/2858013002/diff/160001/chrome/browser/extensions/extension_install_prompt.cc File chrome/browser/extensions/extension_install_prompt.cc (left): https://codereview.chromium.org/2858013002/diff/160001/chrome/browser/extensions/extension_install_prompt.cc#oldcode171 chrome/browser/extensions/extension_install_prompt.cc:171: install_permissions.is_showing_details.clear(); On 2017/05/17 19:10:21, Ivan Šandrk ...
6 months ago (2017-05-18 12:20:20 UTC) #59
Devlin
https://codereview.chromium.org/2858013002/diff/160001/chrome/browser/extensions/extension_install_prompt.cc File chrome/browser/extensions/extension_install_prompt.cc (left): https://codereview.chromium.org/2858013002/diff/160001/chrome/browser/extensions/extension_install_prompt.cc#oldcode171 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 ...
6 months ago (2017-05-23 19:28:04 UTC) #68
Ivan Šandrk
https://codereview.chromium.org/2858013002/diff/160001/chrome/browser/extensions/extension_install_prompt.cc File chrome/browser/extensions/extension_install_prompt.cc (left): https://codereview.chromium.org/2858013002/diff/160001/chrome/browser/extensions/extension_install_prompt.cc#oldcode171 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: > ...
6 months ago (2017-05-24 09:29:01 UTC) #71
Devlin
lgtm I'm still a bit dubious about requiring a dialog here, but y'all are the ...
5 months, 4 weeks ago (2017-05-25 20:52:58 UTC) #78
Mattias Nissler (ping if slow)
https://codereview.chromium.org/2858013002/diff/260001/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2858013002/diff/260001/chrome/app/generated_resources.grd#newcode3593 chrome/app/generated_resources.grd:3593: Read and change all your data on the current ...
5 months, 3 weeks ago (2017-05-26 13:44:47 UTC) #79
Ivan Šandrk
https://codereview.chromium.org/2858013002/diff/260001/chrome/app/generated_resources.grd File chrome/app/generated_resources.grd (right): https://codereview.chromium.org/2858013002/diff/260001/chrome/app/generated_resources.grd#newcode3593 chrome/app/generated_resources.grd:3593: Read and change all your data on the current ...
5 months, 3 weeks ago (2017-05-26 14:04:23 UTC) #80
Devlin
https://codereview.chromium.org/2858013002/diff/260001/chrome/browser/chromeos/extensions/public_session_permission_helper.h File chrome/browser/chromeos/extensions/public_session_permission_helper.h (right): https://codereview.chromium.org/2858013002/diff/260001/chrome/browser/chromeos/extensions/public_session_permission_helper.h#newcode63 chrome/browser/chromeos/extensions/public_session_permission_helper.h:63: bool PermissionAllowed(const Extension* extension, On 2017/05/26 14:04:22, Ivan Šandrk ...
5 months, 3 weeks ago (2017-05-26 14:49:52 UTC) #81
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2858013002/280001
5 months, 3 weeks ago (2017-05-26 15:53:40 UTC) #84
commit-bot: I haz the power
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/448761)
5 months, 3 weeks ago (2017-05-26 16:02:14 UTC) #86
Ivan Šandrk
Avi, ptal at chrome/browser/ui/cocoa/extensions/extension_install_view_controller_unittest.mm
5 months, 3 weeks ago (2017-05-26 16:11:34 UTC) #88
Avi (ping after 24h)
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
5 months, 3 weeks ago (2017-05-26 16:13:09 UTC) #89
Ivan Šandrk
On 2017/05/26 16:13:09, Avi (ping after 24h) wrote: > On 2017/05/26 16:11:34, Ivan Šandrk wrote: ...
5 months, 3 weeks ago (2017-05-26 16:14:42 UTC) #90
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2858013002/280001
5 months, 3 weeks ago (2017-05-26 16:15:19 UTC) #92
commit-bot: I haz the power
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/builds/224600)
5 months, 3 weeks ago (2017-05-26 18:15:02 UTC) #94
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2858013002/280001
5 months, 3 weeks ago (2017-05-26 20:30:37 UTC) #96
commit-bot: I haz the power
Committed patchset #14 (id:280001) as https://chromium.googlesource.com/chromium/src/+/98a3e4a19ea5cbfe16f3486b14e458df441c394f
5 months, 3 weeks ago (2017-05-26 21:15:19 UTC) #99
findit-for-me
Findit (https://goo.gl/kROfz5) identified this CL at revision 475128 as the culprit for failures in the ...
5 months, 3 weeks ago (2017-05-27 00:58:08 UTC) #100
Lei Zhang
https://codereview.chromium.org/2858013002/diff/280001/chrome/browser/extensions/active_tab_unittest.cc File chrome/browser/extensions/active_tab_unittest.cc (right): https://codereview.chromium.org/2858013002/diff/280001/chrome/browser/extensions/active_tab_unittest.cc#newcode493 chrome/browser/extensions/active_tab_unittest.cc:493: free(ActiveTabPermissionGranter::SetPlatformDelegate(nullptr)); This should be a delete. Have you considered ...
5 months, 3 weeks ago (2017-05-27 01:19:42 UTC) #102
Lei Zhang
5 months, 3 weeks ago (2017-05-27 01:26:01 UTC) #103
Message was sent while issue was closed.
https://codereview.chromium.org/2907953002

Powered by Google App Engine
This is Rietveld efc10ee0f