Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(219)

Issue 348313003: Create withheld permissions (Closed)

Created:
6 years, 6 months ago by Devlin
Modified:
6 years ago
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Project:
chromium
Visibility:
Public.

Description

Add a withheld permissions model to PermissionsData. Withheld permissions are the permissions which were requested by the extension, but not granted due to how dangerous/powerful they are. Currently, these withheld permissions are only used for all hosts, and only behind the scripts_require_action flag. BUG=362353 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=281605

Patch Set 1 : #

Patch Set 2 : Rebase #

Patch Set 3 : Test fix #

Total comments: 62

Patch Set 4 : #

Total comments: 8

Patch Set 5 : #

Patch Set 6 : Kalman's #

Total comments: 2

Patch Set 7 : #

Patch Set 8 : Latest master #

Unified diffs Side-by-side diffs Delta from patch set Stats (+957 lines, -467 lines) Patch
M chrome/browser/extensions/active_script_controller.h View 1 2 3 4 5 6 7 3 chunks +32 lines, -13 lines 0 comments Download
M chrome/browser/extensions/active_script_controller.cc View 1 2 3 4 5 6 7 6 chunks +94 lines, -49 lines 0 comments Download
M chrome/browser/extensions/active_script_controller_browsertest.cc View 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/active_script_controller_unittest.cc View 1 2 3 4 5 6 7 12 chunks +59 lines, -46 lines 0 comments Download
M chrome/browser/extensions/active_tab_permission_granter.cc View 1 2 3 4 5 2 chunks +4 lines, -13 lines 0 comments Download
M chrome/browser/extensions/extension_service.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/extensions/extension_util.cc View 1 2 3 4 5 2 chunks +15 lines, -0 lines 0 comments Download
M chrome/browser/extensions/permissions_updater.h View 1 2 3 4 5 2 chunks +14 lines, -5 lines 0 comments Download
M chrome/browser/extensions/permissions_updater.cc View 1 2 3 4 5 7 chunks +176 lines, -38 lines 0 comments Download
M chrome/browser/extensions/permissions_updater_unittest.cc View 1 2 3 4 5 5 chunks +193 lines, -1 line 0 comments Download
M chrome/browser/ui/webui/extensions/extension_settings_handler.cc View 1 2 3 4 5 6 7 1 chunk +1 line, -3 lines 0 comments Download
M extensions/common/extension_messages.h View 1 2 3 4 5 6 7 8 chunks +38 lines, -16 lines 0 comments Download
M extensions/common/extension_messages.cc View 4 chunks +56 lines, -20 lines 0 comments Download
M extensions/common/permissions/permission_set.cc View 2 chunks +1 line, -29 lines 0 comments Download
M extensions/common/permissions/permissions_data.h View 1 2 3 4 5 8 chunks +56 lines, -18 lines 0 comments Download
M extensions/common/permissions/permissions_data.cc View 1 2 3 4 5 6 chunks +72 lines, -48 lines 0 comments Download
M extensions/common/permissions/permissions_data_unittest.cc View 1 2 3 2 chunks +0 lines, -54 lines 0 comments Download
M extensions/common/url_pattern.h View 1 2 3 1 chunk +7 lines, -0 lines 0 comments Download
M extensions/common/url_pattern.cc View 1 2 3 4 5 6 2 chunks +37 lines, -0 lines 0 comments Download
M extensions/common/user_script.h View 1 2 3 4 5 1 chunk +13 lines, -0 lines 0 comments Download
M extensions/renderer/dispatcher.h View 1 2 1 chunk +1 line, -4 lines 0 comments Download
M extensions/renderer/dispatcher.cc View 1 2 3 4 5 6 7 6 chunks +20 lines, -50 lines 0 comments Download
M extensions/renderer/programmatic_script_injector.h View 1 2 3 4 5 2 chunks +6 lines, -4 lines 0 comments Download
M extensions/renderer/programmatic_script_injector.cc View 1 2 3 4 5 6 7 3 chunks +15 lines, -16 lines 0 comments Download
M extensions/renderer/script_injection.cc View 1 2 3 4 5 6 7 5 chunks +15 lines, -13 lines 0 comments Download
M extensions/renderer/script_injector.h View 1 2 3 4 5 3 chunks +9 lines, -12 lines 0 comments Download
M extensions/renderer/user_script_injector.h View 1 2 3 4 5 2 chunks +6 lines, -4 lines 0 comments Download
M extensions/renderer/user_script_injector.cc View 1 2 3 4 5 3 chunks +13 lines, -6 lines 0 comments Download
M extensions/renderer/user_script_set.cc View 1 2 3 1 chunk +2 lines, -2 lines 0 comments Download

Messages

Total messages: 24 (0 generated)
Devlin
Haven't finished trybots yet, but I think this should be reasonably stable. Feel free to ...
6 years, 5 months ago (2014-06-27 17:18:16 UTC) #1
not at google - send to devlin
I like this change. https://codereview.chromium.org/348313003/diff/100001/chrome/browser/extensions/active_script_controller.cc File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/348313003/diff/100001/chrome/browser/extensions/active_script_controller.cc#newcode48 chrome/browser/extensions/active_script_controller.cc:48: ->ShouldWarnAllHosts(); don't you need to ...
6 years, 5 months ago (2014-06-27 23:24:35 UTC) #2
not at google - send to devlin
a couple more comments I thought of over the weekend. https://codereview.chromium.org/348313003/diff/100001/chrome/browser/extensions/active_script_controller.cc File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/348313003/diff/100001/chrome/browser/extensions/active_script_controller.cc#newcode190 ...
6 years, 5 months ago (2014-06-30 14:37:04 UTC) #3
Devlin
https://codereview.chromium.org/348313003/diff/100001/chrome/browser/extensions/active_script_controller.cc File chrome/browser/extensions/active_script_controller.cc (right): https://codereview.chromium.org/348313003/diff/100001/chrome/browser/extensions/active_script_controller.cc#newcode48 chrome/browser/extensions/active_script_controller.cc:48: ->ShouldWarnAllHosts(); On 2014/06/27 23:24:33, kalman wrote: > don't you ...
6 years, 5 months ago (2014-06-30 17:06:11 UTC) #4
Devlin
Refactor done. The only concern I have now is that withholding the permissions isn't done ...
6 years, 5 months ago (2014-06-30 20:28:58 UTC) #5
not at google - send to devlin
looking good. https://codereview.chromium.org/348313003/diff/100001/chrome/browser/extensions/active_tab_permission_granter.cc File chrome/browser/extensions/active_tab_permission_granter.cc (right): https://codereview.chromium.org/348313003/diff/100001/chrome/browser/extensions/active_tab_permission_granter.cc#newcode52 chrome/browser/extensions/active_tab_permission_granter.cc:52: permissions_data->HasWithheldAllHosts()) { On 2014/06/30 17:06:09, Devlin wrote: ...
6 years, 5 months ago (2014-07-01 00:28:36 UTC) #6
not at google - send to devlin
https://codereview.chromium.org/348313003/diff/100001/extensions/common/extension_messages.h File extensions/common/extension_messages.h (right): https://codereview.chromium.org/348313003/diff/100001/extensions/common/extension_messages.h#newcode209 extensions/common/extension_messages.h:209: struct ExtensionMsg_PermissionSetStruct { On 2014/07/01 00:28:35, kalman wrote: > ...
6 years, 5 months ago (2014-07-01 02:56:51 UTC) #7
Devlin
https://codereview.chromium.org/348313003/diff/100001/chrome/browser/extensions/active_tab_permission_granter.cc File chrome/browser/extensions/active_tab_permission_granter.cc (right): https://codereview.chromium.org/348313003/diff/100001/chrome/browser/extensions/active_tab_permission_granter.cc#newcode52 chrome/browser/extensions/active_tab_permission_granter.cc:52: permissions_data->HasWithheldAllHosts()) { On 2014/07/01 00:28:35, kalman wrote: > On ...
6 years, 5 months ago (2014-07-01 16:27:06 UTC) #8
not at google - send to devlin
https://codereview.chromium.org/348313003/diff/100001/chrome/browser/extensions/active_tab_permission_granter.cc File chrome/browser/extensions/active_tab_permission_granter.cc (right): https://codereview.chromium.org/348313003/diff/100001/chrome/browser/extensions/active_tab_permission_granter.cc#newcode52 chrome/browser/extensions/active_tab_permission_granter.cc:52: permissions_data->HasWithheldAllHosts()) { On 2014/07/01 16:27:05, Devlin wrote: > On ...
6 years, 5 months ago (2014-07-01 17:02:12 UTC) #9
not at google - send to devlin
lgtm after comments apart from the IPC stuff, which was resolved offline
6 years, 5 months ago (2014-07-01 17:34:42 UTC) #10
Devlin
https://codereview.chromium.org/348313003/diff/100001/chrome/browser/extensions/active_tab_permission_granter.cc File chrome/browser/extensions/active_tab_permission_granter.cc (right): https://codereview.chromium.org/348313003/diff/100001/chrome/browser/extensions/active_tab_permission_granter.cc#newcode52 chrome/browser/extensions/active_tab_permission_granter.cc:52: permissions_data->HasWithheldAllHosts()) { On 2014/07/01 17:02:11, kalman wrote: > On ...
6 years, 5 months ago (2014-07-01 18:34:08 UTC) #11
Devlin
+Justin for extension_messages.
6 years, 5 months ago (2014-07-01 18:35:52 UTC) #12
jschuh
ipc security lgtm with one nit https://codereview.chromium.org/348313003/diff/220001/extensions/common/extension_messages.h File extensions/common/extension_messages.h (right): https://codereview.chromium.org/348313003/diff/220001/extensions/common/extension_messages.h#newcode317 extensions/common/extension_messages.h:317: IPC_STRUCT_MEMBER(std::string, extension_id) fix ...
6 years, 5 months ago (2014-07-02 18:27:25 UTC) #13
Devlin
https://codereview.chromium.org/348313003/diff/220001/extensions/common/extension_messages.h File extensions/common/extension_messages.h (right): https://codereview.chromium.org/348313003/diff/220001/extensions/common/extension_messages.h#newcode317 extensions/common/extension_messages.h:317: IPC_STRUCT_MEMBER(std::string, extension_id) On 2014/07/02 18:27:25, Justin Schuh wrote: > ...
6 years, 5 months ago (2014-07-02 23:04:22 UTC) #14
Devlin
The CQ bit was checked by rdevlin.cronin@chromium.org
6 years, 5 months ago (2014-07-07 19:00:47 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/348313003/280001
6 years, 5 months ago (2014-07-07 19:01:08 UTC) #16
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_aosp on tryserver.chromium ...
6 years, 5 months ago (2014-07-07 20:58:58 UTC) #17
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 5 months ago (2014-07-07 21:07:17 UTC) #18
commit-bot: I haz the power
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_compile_rel/builds/20117) android_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/203323)
6 years, 5 months ago (2014-07-07 21:07:20 UTC) #19
Devlin
The CQ bit was checked by rdevlin.cronin@chromium.org
6 years, 5 months ago (2014-07-07 21:47:55 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rdevlin.cronin@chromium.org/348313003/280001
6 years, 5 months ago (2014-07-07 21:49:07 UTC) #21
commit-bot: I haz the power
Change committed as 281605
6 years, 5 months ago (2014-07-07 23:26:32 UTC) #22
robwu
Devlin, after this CL, PermissionsData::CanRunContentScriptOnPage seems to be unused (dead), at least at HEAD. $ ...
6 years ago (2014-12-11 23:58:37 UTC) #23
Devlin
6 years ago (2014-12-12 00:26:07 UTC) #24
Message was sent while issue was closed.
On 2014/12/11 23:58:37, robwu wrote:
> Devlin, after this CL, PermissionsData::CanRunContentScriptOnPage seems to be
> unused (dead), at least at HEAD.
> 
> $ grep -r CanRunContentScriptOnPage --exclude-dir=third_party/
--exclude-dir=out
> extensions/common/permissions/permissions_data.cc:bool
> PermissionsData::CanRunContentScriptOnPage(const Extension* extension,
> extensions/common/permissions/permissions_data.h:  bool
> CanRunContentScriptOnPage(const Extension* extension,
> extensions/common/permissions/permissions_data.h:  // Like
> CanRunContentScriptOnPage, but also takes withheld permissions into
> chrome/common/extensions/extension_unittest.cc: 
> EXPECT_FALSE(extension->permissions_data()->CanRunContentScriptOnPage(
> chrome/common/extensions/extension_unittest.cc: 
> EXPECT_TRUE(extension->permissions_data()->CanRunContentScriptOnPage(
> chrome/common/extensions/extension_unittest.cc: 
> EXPECT_FALSE(extension->permissions_data()->CanRunContentScriptOnPage(
> chrome/common/extensions/extension_unittest.cc: 
> EXPECT_TRUE(extension->permissions_data()->CanRunContentScriptOnPage(
> chrome/common/extensions/extension_unittest.cc: 
> EXPECT_FALSE(extension->permissions_data()->CanRunContentScriptOnPage(
> chrome/common/extensions/extension_unittest.cc: 
> EXPECT_FALSE(extension->permissions_data()->CanRunContentScriptOnPage(
> 
> Will you delete this dead code, or are you going to use it in the future?

I think that, at one point, CanRunContentScriptOnPage was used in the renderer. 
In the time since, this code has been updated to use the superior
GetContentScriptAccess(), so it looks like yes, CanRunContentScriptOnPage can be
deleted (with the unit tests being updated to use GetContentScriptAccess() ==
ALLOWED/DENIED).

I'll add it to my to-do list, but feel free to beat me to it :)

Powered by Google App Engine
This is Rietveld 408576698