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

Issue 2858013002: PS - Showing permission prompt for activeTab

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 weeks, 6 days ago by Ivan Šandrk
Modified:
1 hour, 9 minutes 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

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

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 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 0 comments 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
Commit queue not available (can’t edit this change).

Messages

Total messages: 75 (55 generated)
Ivan Šandrk
Hey Devlin, just a preliminary review. I'm not sure about the design I should use ...
2 weeks, 6 days 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, ...
2 weeks, 5 days ago (2017-05-05 16:24:27 UTC) #20
Devlin (catching up)
Sorry I didn't get to this sooner. I'm actually a bit confused on this one ...
2 weeks, 4 days 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?
2 weeks, 4 days 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 ...
2 weeks, 2 days 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 ...
2 weeks, 1 day ago (2017-05-09 13:29:29 UTC) #27
Devlin (catching up)
On 2017/05/08 12:56:10, Ivan Šandrk wrote: > Should've posted this earlier, this is the reasoning ...
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 ...
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 ...
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, ...
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: ...
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 ...
2 weeks ago (2017-05-10 11:11:45 UTC) #38
Alexander Alekseev
lgtm
1 week, 5 days ago (2017-05-12 04:33:17 UTC) #42
Devlin (catching up)
I'm still a little dubious on the reasoning behind this - to me, it still ...
1 week, 5 days 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 ...
1 week, 4 days ago (2017-05-12 18:27:44 UTC) #50
Devlin (catching up)
On 2017/05/12 18:27:44, Ivan Šandrk wrote: > > One other concern: what does the permission ...
1 week, 1 day 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 days, 22 hours 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 days, 5 hours ago (2017-05-18 12:20:20 UTC) #59
Devlin (catching up)
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 ...
22 hours, 25 minutes ago (2017-05-23 19:28:04 UTC) #68
Ivan Šandrk
8 hours, 25 minutes ago (2017-05-24 09:29:01 UTC) #71
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 :)
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 650457f06