New issue
Advanced search Search tips

Issue 842270 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug



Sign in to add a comment

Extensions: PermissionsData should not depend on Extension class

Project Member Reported by rdevlin....@chromium.org, May 11 2018

Issue description

PermissionsData has a handful of methods that take an Extension object.  It seems like these are used for two main reasons:
1. To access data about the extension, like id and Manifest::Location.
2. To access the permissions of the extension.

For 1, we should just pass in the bits we need (e.g., id and location).  2 is simply silly, since the PermissionsData object is accessing the PermissionsData of the extension, which is... itself.
 
It is owned by an extension object. So we should just be able to able to keep an unowned pointer to its owner, the extension.
We could, and that's better (because at least doing the oddities of 2.), but that would still require a cyclic dependency on the extension class.  It'd be nice to avoid that, if we can.
Project Member

Comment 3 by bugdroid1@chromium.org, May 12 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/99b1e5dc7e2baefede3785fec1ec4b8159cfcd82

commit 99b1e5dc7e2baefede3785fec1ec4b8159cfcd82
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Sat May 12 01:20:14 2018

[Extensions] Factor Extension out of PermissionsData::PolicyDelegate

Refactor PermissionsData::PolicyDelegate::CanExecuteScriptOnPage to no
longer rely on being passed an extension. Instead, rename it to
IsRestrictedUrl(), and call it from the method of the same name in
PermissionsData. PermissionsData::IsRestrictedUrl() already checks if
the extension can execute script everywhere, so the check is no longer
necessary in the policy delegate. Additionally, this is ensures that
places checking if an url is restricted also check with the delegate.

Bug:  842270 
Change-Id: Ica4c086d1315f9512cc908542841bf8dee2bedee
Reviewed-on: https://chromium-review.googlesource.com/1055930
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558083}
[modify] https://crrev.com/99b1e5dc7e2baefede3785fec1ec4b8159cfcd82/chrome/renderer/extensions/renderer_permissions_policy_delegate.cc
[modify] https://crrev.com/99b1e5dc7e2baefede3785fec1ec4b8159cfcd82/chrome/renderer/extensions/renderer_permissions_policy_delegate.h
[modify] https://crrev.com/99b1e5dc7e2baefede3785fec1ec4b8159cfcd82/extensions/common/permissions/permissions_data.cc
[modify] https://crrev.com/99b1e5dc7e2baefede3785fec1ec4b8159cfcd82/extensions/common/permissions/permissions_data.h

Project Member

Comment 4 by bugdroid1@chromium.org, May 15 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f2173e44cd6ecde69bb4e2cb3d79d05fcde3fee2

commit f2173e44cd6ecde69bb4e2cb3d79d05fcde3fee2
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Tue May 15 16:41:39 2018

[Extensions] Factor Extension out of CanExecuteScriptEverywhere()

Factor the Extension object out of
PermissionsData::CanExecuteScriptEverywhere(), instead passing in the
extension ID and location directly.

Also clean up a few duplicate calls to the method by locally caching the
result.

Bug:  842270 

Change-Id: I434a3df769d32d9f63e2e77bc393d634dcdba4da
Reviewed-on: https://chromium-review.googlesource.com/1056036
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558746}
[modify] https://crrev.com/f2173e44cd6ecde69bb4e2cb3d79d05fcde3fee2/chrome/browser/extensions/scripting_permissions_modifier.cc
[modify] https://crrev.com/f2173e44cd6ecde69bb4e2cb3d79d05fcde3fee2/chrome/common/extensions/permissions/permissions_data_unittest.cc
[modify] https://crrev.com/f2173e44cd6ecde69bb4e2cb3d79d05fcde3fee2/extensions/browser/api/guest_view/web_view/web_view_internal_api.cc
[modify] https://crrev.com/f2173e44cd6ecde69bb4e2cb3d79d05fcde3fee2/extensions/common/manifest_handlers/content_scripts_handler.cc
[modify] https://crrev.com/f2173e44cd6ecde69bb4e2cb3d79d05fcde3fee2/extensions/common/manifest_handlers/permissions_parser.cc
[modify] https://crrev.com/f2173e44cd6ecde69bb4e2cb3d79d05fcde3fee2/extensions/common/permissions/permissions_data.cc
[modify] https://crrev.com/f2173e44cd6ecde69bb4e2cb3d79d05fcde3fee2/extensions/common/permissions/permissions_data.h
[modify] https://crrev.com/f2173e44cd6ecde69bb4e2cb3d79d05fcde3fee2/extensions/renderer/extension_injection_host.cc
[modify] https://crrev.com/f2173e44cd6ecde69bb4e2cb3d79d05fcde3fee2/extensions/renderer/user_script_set.cc

Project Member

Comment 5 by bugdroid1@chromium.org, May 16 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a81e00522e161fd7f9c495ac1ea4792aef3f3eee

commit a81e00522e161fd7f9c495ac1ea4792aef3f3eee
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Wed May 16 03:33:45 2018

[Extensions] Clean up PermissionsData::IsRestrictedUrl()

Factor out the Extension object from
PermissionsData::IsRestrictedUrl(), just using the extension ID and
location instead. Additionally, make the method non-static, since it
should always be used on a valid PermissionsData object.

Bug:  842270 
Change-Id: Ib1d9e74674bba2a9835398457d817613c024e6e6
Reviewed-on: https://chromium-review.googlesource.com/1057440
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558958}
[modify] https://crrev.com/a81e00522e161fd7f9c495ac1ea4792aef3f3eee/chrome/browser/extensions/api/debugger/debugger_api.cc
[modify] https://crrev.com/a81e00522e161fd7f9c495ac1ea4792aef3f3eee/chrome/browser/extensions/api/tabs/tabs_api.cc
[modify] https://crrev.com/a81e00522e161fd7f9c495ac1ea4792aef3f3eee/chrome/common/extensions/permissions/permissions_data_unittest.cc
[modify] https://crrev.com/a81e00522e161fd7f9c495ac1ea4792aef3f3eee/extensions/common/permissions/permissions_data.cc
[modify] https://crrev.com/a81e00522e161fd7f9c495ac1ea4792aef3f3eee/extensions/common/permissions/permissions_data.h

Project Member

Comment 6 by bugdroid1@chromium.org, May 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/5cb43783f38b420093d5a58c4e369db9552618b5

commit 5cb43783f38b420093d5a58c4e369db9552618b5
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Thu May 17 20:14:41 2018

[Extensions] Factor Extension out of PermissionsData GetAccess methods

Factor the Extension object out of the following methods:
- PermissionsData::GetPageAccess()
- PermissionsData::CanAccessPage()
- PermissionsData::GetContentScriptAccess()
- PermissionsData::CanRunContentScriptOnPage()
- PermissionsData::CanCaptureVisiblePage()

In all these cases, the extension passed in should correspond to
the extension that owns the PermissionsData object; thus, we can
just use the data associated with the PermissionsData. This makes
the method more clear, avoids a potential bug where the passed-in
extension could be different than the owning extension, and removes
another dependency of PermissionsData on the Extension class.

Bug:  842270 
Change-Id: I6254d1d292e333ac9781c06d89c661a4881eb67c
Reviewed-on: https://chromium-review.googlesource.com/1061588
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559654}
[modify] https://crrev.com/5cb43783f38b420093d5a58c4e369db9552618b5/chrome/browser/extensions/active_tab_unittest.cc
[modify] https://crrev.com/5cb43783f38b420093d5a58c4e369db9552618b5/chrome/browser/extensions/api/automation_internal/automation_internal_api.cc
[modify] https://crrev.com/5cb43783f38b420093d5a58c4e369db9552618b5/chrome/browser/extensions/api/tabs/tabs_api.cc
[modify] https://crrev.com/5cb43783f38b420093d5a58c4e369db9552618b5/chrome/browser/extensions/extension_action_runner.cc
[modify] https://crrev.com/5cb43783f38b420093d5a58c4e369db9552618b5/chrome/browser/extensions/permissions_updater_unittest.cc
[modify] https://crrev.com/5cb43783f38b420093d5a58c4e369db9552618b5/chrome/browser/extensions/scripting_permissions_modifier_unittest.cc
[modify] https://crrev.com/5cb43783f38b420093d5a58c4e369db9552618b5/chrome/common/extensions/extension_unittest.cc
[modify] https://crrev.com/5cb43783f38b420093d5a58c4e369db9552618b5/chrome/common/extensions/manifest_tests/extension_manifests_chromepermission_unittest.cc
[modify] https://crrev.com/5cb43783f38b420093d5a58c4e369db9552618b5/chrome/common/extensions/permissions/permissions_data_unittest.cc
[modify] https://crrev.com/5cb43783f38b420093d5a58c4e369db9552618b5/chrome/renderer/extensions/chrome_extensions_renderer_client.cc
[modify] https://crrev.com/5cb43783f38b420093d5a58c4e369db9552618b5/chrome/renderer/extensions/renderer_permissions_policy_delegate_unittest.cc
[modify] https://crrev.com/5cb43783f38b420093d5a58c4e369db9552618b5/extensions/browser/api/web_request/web_request_permissions.cc
[modify] https://crrev.com/5cb43783f38b420093d5a58c4e369db9552618b5/extensions/common/permissions/permissions_data.cc
[modify] https://crrev.com/5cb43783f38b420093d5a58c4e369db9552618b5/extensions/common/permissions/permissions_data.h
[modify] https://crrev.com/5cb43783f38b420093d5a58c4e369db9552618b5/extensions/renderer/extension_injection_host.cc

Project Member

Comment 7 by bugdroid1@chromium.org, May 17 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/861126a203278fa4d5092e72f7748923f8678375

commit 861126a203278fa4d5092e72f7748923f8678375
Author: Devlin Cronin <rdevlin.cronin@chromium.org>
Date: Thu May 17 21:48:50 2018

[Extensions] Finish factoring Extension out of PermissionsData

Fully factor the Extension class out of PermissionsData, breaking the
dependency cycle. Instead, pass in the necessary members directly to the
constructor.

Also clean up other unnecessary includes.

Bug:  842270 
Change-Id: Id73c549ff39b51ef4a457612853b5ea0d62aab69
Reviewed-on: https://chromium-review.googlesource.com/1064873
Commit-Queue: Devlin <rdevlin.cronin@chromium.org>
Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#559692}
[modify] https://crrev.com/861126a203278fa4d5092e72f7748923f8678375/extensions/common/extension.cc
[modify] https://crrev.com/861126a203278fa4d5092e72f7748923f8678375/extensions/common/permissions/permissions_data.cc
[modify] https://crrev.com/861126a203278fa4d5092e72f7748923f8678375/extensions/common/permissions/permissions_data.h

Status: Fixed (was: Started)
Our work here is done.

Sign in to add a comment