Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2499493004:
Communicate ExtensionSettings policy to renderers (Closed)
DescriptionCommunicate ExtensionSettings policy to renderers
-Communicate which hosts are runtime blocked to all renderers
-Blocks host permissions for specific hosts by specific extensions
-Tests via blocking content script injection
-Introduces new test class for use with ExtensionSettings policy
BUG=624649
Review-Url: https://codereview.chromium.org/2499493004
Cr-Commit-Position: refs/heads/master@{#464954}
Committed: https://chromium.googlesource.com/chromium/src/+/c2f02148125c69bdce012802d9a467d725965a93
Patch Set 1 #
Total comments: 25
Patch Set 2 : URLPatternSets use shared memory for IPC. Default scope patterns sent once per renderer. #
Total comments: 21
Patch Set 3 : Add URLPattern effective TLD whitelisting, Switched IPC to UpdatePermissions, Removed shared memor… #
Total comments: 34
Patch Set 4 : Fixed static non-pod, removed default params, fixed formatting, fixed nits, simplified code #
Total comments: 26
Patch Set 5 : Style fixes, prevent heap leak. #
Total comments: 52
Patch Set 6 : Fix effective TLD wildcard bug, move to Leaky LazyInstance in PermissionsData, removed unnecessary … #
Total comments: 50
Patch Set 7 : -Added includes, Seperated setting usage of default policy in PermissionsData, spelling fixes #
Total comments: 6
Patch Set 8 : -Send notification of extension permission changes due to policy, only send individual policy when … #Patch Set 9 : -Forgot to remove comments #
Total comments: 18
Patch Set 10 : -Removed old includes, added lock check to SetUsesDefaultHostRestrictions #
Total comments: 22
Patch Set 11 : -Updating host policy calls function to use default or send override data. Fixes bug sending notifi… #Patch Set 12 : -Make if statments positive, cl format #Patch Set 13 : -Forgot to update copyright date #
Total comments: 2
Patch Set 14 : -Added unit tests for PermissionsUpdater and PermissionsData, Removed unnecessary lock assertion, p… #
Total comments: 63
Patch Set 15 : -Updated unit tests, fixed nits, moved IsRuntimeBlockedHost for safety #
Total comments: 7
Patch Set 16 : Change to a debug check, change function name to reduce confusion. #Patch Set 17 : Removed unnecessary optional_permissions #Patch Set 18 : -fix unittest, manual merge, fix to apease windows compiler #
Total comments: 6
Patch Set 19 : nits #
Total comments: 2
Messages
Total messages: 128 (66 generated)
The CQ bit was checked by nrpeter@google.com 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 ========== -Communicate which hosts are runtime blocked to all renderers -Blocks host permissions for extensions in accordance with ExtensionSettings policy -Tests via blocking content script injection -Introduces new test class for use with ExtensionSettings policy BUG=624649 ========== to ========== -Communicate which hosts are runtime blocked to all renderers -Blocks host permissions for extensions in accordance with ExtensionSettings policy -Tests via blocking content script injection -Introduces new test class for use with ExtensionSettings policy BUG=624649 ==========
nrpeter@chromium.org changed reviewers: + asargent@chromium.org, dcheng@chromium.org
nrpeter@chromium.org changed required reviewers: + asargent@chromium.org, dcheng@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== -Communicate which hosts are runtime blocked to all renderers -Blocks host permissions for extensions in accordance with ExtensionSettings policy -Tests via blocking content script injection -Introduces new test class for use with ExtensionSettings policy BUG=624649 ========== to ========== -Communicate which hosts are runtime blocked to all renderers -Blocks host permissions for extensions in accordance with ExtensionSettings policy -Tests via blocking content script injection -Introduces new test class for use with ExtensionSettings policy Incremental patch, relies on 2492013002 BUG=624649 ==========
nrpeter@chromium.org changed reviewers: + nrpeter@chromium.org
nrpeter@chromium.org changed reviewers: + rdevlin.cronin@chromium.org
The biggest concern here is the one about the size of the lists and IPC messages copying them around. (see inline) https://codereview.chromium.org/2499493004/diff/1/chrome/browser/extensions/c... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/1/chrome/browser/extensions/c... chrome/browser/extensions/content_script_apitest.cc:337: ::switches::kAppsGalleryURL, why this change? (maybe the missing closing brace to the extensions namespace in extension_with_management_policy_apitest.h was causing a compile error here?) https://codereview.chromium.org/2499493004/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_with_management_policy_apitest.h (right): https://codereview.chromium.org/2499493004/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_with_management_policy_apitest.h:23: namespace { no anonymous namespaces in .h files If you want to have a shared version of the AddPattern function from permissions_apitest.cc that is useful for subclasses of ExtensionApiTestWithManagementPolicy, the best way to do that would be to just declare it as static member of the ExtensionApiTestWithManagementPolicy class and have the definition of it in extension_with_management_policy_apitest.cc. On the other hand, if it's still only going to be used in permissions_apitest.cc, just leave it in an anonymous namespace there and skip the declaration here. https://codereview.chromium.org/2499493004/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_with_management_policy_apitest.h:34: class ExtensionApiTestWithManagementPolicy : public ExtensionApiTest { nit: needs brief overview comment about why this class exists / what it's used for https://codereview.chromium.org/2499493004/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_with_management_policy_apitest.h:49: missing matching brace to close the extensions namespace opened at line 21 https://codereview.chromium.org/2499493004/diff/1/extensions/common/extension... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/2499493004/diff/1/extensions/common/extension... extensions/common/extension_messages.h:303: extensions::URLPatternSet runtime_allowed_hosts; Do we have any sort of limitation on the size of these lists? I'm worried about the cost of copying data for this IPC as currently specified if they contain hundreds/thousands or more of entries. If we do want to handle more than a few 10's of entries for these lists, we probably want to rethink the design of how we share/update this data to account for this. (For instance, something like shared memory segments or passing a handle to a file on disk). https://codereview.chromium.org/2499493004/diff/1/extensions/common/extension... extensions/common/extension_messages.h:534: IPC_MESSAGE_CONTROL1(ExtensionMsg_UpdatePolicy, "UpdatePolicy" is a little ambiguous of a name here, since there are a lot of policies that apply to extensions and this message is about a fairly particular one. Also, we could conceivably want to at some point allow users to manually adjust the runtime allowed/blocked hosts lists themselves in non-enterprise managed scenarios, so it would be better to use names that are agnostic of the source of the allowed/blocked lists. How about something like "UpdateAllowedAndBlockedHosts"? https://codereview.chromium.org/2499493004/diff/1/extensions/common/manifest_... File extensions/common/manifest_constants.cc (right): https://codereview.chromium.org/2499493004/diff/1/extensions/common/manifest_... extensions/common/manifest_constants.cc:291: "This page cannot be scripted due to an ExtensionsSettings policy."; In theory, this file should have constants for keys in the manifest and error messages related to static parsing of extension manifest.json files. It looks like a few other messages that don't quite fit this criteria have crept in. Devlin might have a good idea of a better place for it. (These strings are distinct from user visible messages that get translated, since they are only reported to extensions in the chrome.runtime.lastError property of API methods like chrome.tabs.executeScript, etc. that developers can access) https://codereview.chromium.org/2499493004/diff/1/extensions/common/permissio... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/1/extensions/common/permissio... extensions/common/permissions/permissions_data.cc:345: if (runtime_blocked_hosts_unsafe_.MatchesURL(url)) { It seems like one of 2 things should happen here: a) runtime_lock_ is held b) use untime_blocked_hosts() to test against (instead of runtime_blocked_hosts_unsafe_) (same thing below for runtime_allowed_hosts_unsafe)
https://codereview.chromium.org/2499493004/diff/1/extensions/common/manifest_... File extensions/common/manifest_constants.cc (right): https://codereview.chromium.org/2499493004/diff/1/extensions/common/manifest_... extensions/common/manifest_constants.cc:291: "This page cannot be scripted due to an ExtensionsSettings policy."; On 2016/11/23 01:19:23, Antony Sargent wrote: > In theory, this file should have constants for keys in the manifest and error > messages related to static parsing of extension manifest.json files. It looks > like a few other messages that don't quite fit this criteria have crept in. > > Devlin might have a good idea of a better place for it. (These strings are > distinct from user visible messages that get translated, since they are only > reported to extensions in the chrome.runtime.lastError property of API methods > like chrome.tabs.executeScript, etc. that developers can access) Typically, we just put the errors in the files where they're used (so either the API implementation or permissions data). https://codereview.chromium.org/2499493004/diff/1/extensions/common/permissio... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/1/extensions/common/permissio... extensions/common/permissions/permissions_data.cc:91: extension->permissions_data()->IsRuntimeBlockedHost(document_url)) { This isn't the same type of restricted url as the rest here, and this is redundant with the check in CanRunOnPage. https://codereview.chromium.org/2499493004/diff/1/extensions/common/permissio... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/2499493004/diff/1/extensions/common/permissio... extensions/common/permissions/permissions_data.h:86: bool IsRuntimeBlockedHost(const GURL& url) const; Why does this need to be public? https://codereview.chromium.org/2499493004/diff/1/extensions/common/permissio... extensions/common/permissions/permissions_data.h:212: const URLPatternSet& runtime_blocked_hosts() const { ditto https://codereview.chromium.org/2499493004/diff/1/extensions/common/permissio... extensions/common/permissions/permissions_data.h:279: mutable URLPatternSet runtime_blocked_hosts_unsafe_; Do we need this for each extension? It seems like almost all extensions will use the default settings, so we shouldn't need to copy these over for every single item when they're identical. Could we have these *only* if they differ from the default?
zmin@chromium.org changed reviewers: + zmin@chromium.org
https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1490: CHECK(settings); Do we have to use CHECK() here? What will happened if we can't get the settings? Can we use the DCHECK instead? https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1905: /* extensions::PermissionsUpdater(profile()).SetRuntimeBlockedAllowedHosts( Please remove this. https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_with_management_policy_apitest.h (right): https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_with_management_policy_apitest.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. no (c) https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:365: for (RenderProcessHost::iterator i(RenderProcessHost::AllHostsIterator()); Can we use "host_iterator" or some other meaningful name instead of "i" here? https://codereview.chromium.org/2499493004/diff/20001/extensions/common/exten... File extensions/common/extension_messages.cc (right): https://codereview.chromium.org/2499493004/diff/20001/extensions/common/exten... extensions/common/extension_messages.cc:108: CHECK(iter->ReadUInt32(&num_patterns)); Again, CHECK is a very strong assertion. It means Chrome will crash if it failed. Do we have to shutdown Chrome here if this failed? https://codereview.chromium.org/2499493004/diff/20001/extensions/common/permi... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/20001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:389: if (!runtime_allowed_hosts().MatchesURL(url)) Can we do: return runtime_blocked_hosts().MatchesURL(url) && !runtime_allowed_hosts().MatchesURL(url);
https://codereview.chromium.org/2499493004/diff/1/chrome/browser/extensions/c... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/1/chrome/browser/extensions/c... chrome/browser/extensions/content_script_apitest.cc:337: ::switches::kAppsGalleryURL, On 2016/11/23 01:19:23, Antony Sargent wrote: > why this change? (maybe the missing closing brace to the extensions namespace in > extension_with_management_policy_apitest.h was causing a compile error here?) > Done. https://codereview.chromium.org/2499493004/diff/1/chrome/browser/extensions/e... File chrome/browser/extensions/extension_with_management_policy_apitest.h (right): https://codereview.chromium.org/2499493004/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_with_management_policy_apitest.h:23: namespace { On 2016/11/23 01:19:23, Antony Sargent wrote: > no anonymous namespaces in .h files > > If you want to have a shared version of the AddPattern function from > permissions_apitest.cc that is useful for subclasses of > ExtensionApiTestWithManagementPolicy, the best way to do that would be to just > declare it as static member of the ExtensionApiTestWithManagementPolicy class > and have the definition of it in extension_with_management_policy_apitest.cc. > > On the other hand, if it's still only going to be used in > permissions_apitest.cc, just leave it in an anonymous namespace there and skip > the declaration here. Done. https://codereview.chromium.org/2499493004/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_with_management_policy_apitest.h:34: class ExtensionApiTestWithManagementPolicy : public ExtensionApiTest { On 2016/11/23 01:19:23, Antony Sargent wrote: > nit: needs brief overview comment about why this class exists / what it's used > for Done. https://codereview.chromium.org/2499493004/diff/1/chrome/browser/extensions/e... chrome/browser/extensions/extension_with_management_policy_apitest.h:49: On 2016/11/23 01:19:23, Antony Sargent wrote: > missing matching brace to close the extensions namespace opened at line 21 Done. https://codereview.chromium.org/2499493004/diff/1/extensions/common/extension... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/2499493004/diff/1/extensions/common/extension... extensions/common/extension_messages.h:303: extensions::URLPatternSet runtime_allowed_hosts; On 2016/11/23 01:19:23, Antony Sargent wrote: > Do we have any sort of limitation on the size of these lists? I'm worried about > the cost of copying data for this IPC as currently specified if they contain > hundreds/thousands or more of entries. > > If we do want to handle more than a few 10's of entries for these lists, we > probably want to rethink the design of how we share/update this data to account > for this. (For instance, something like shared memory segments or passing a > handle to a file on disk). Updated to use SharedMemoryHandle, this should let us handle huge URLPatternSets. https://codereview.chromium.org/2499493004/diff/1/extensions/common/extension... extensions/common/extension_messages.h:534: IPC_MESSAGE_CONTROL1(ExtensionMsg_UpdatePolicy, On 2016/11/23 01:19:23, Antony Sargent wrote: > "UpdatePolicy" is a little ambiguous of a name here, since there are a lot of > policies that apply to extensions and this message is about a fairly particular > one. > > Also, we could conceivably want to at some point allow users to manually adjust > the runtime allowed/blocked hosts lists themselves in non-enterprise managed > scenarios, so it would be better to use names that are agnostic of the source of > the allowed/blocked lists. > > How about something like "UpdateAllowedAndBlockedHosts"? Done. https://codereview.chromium.org/2499493004/diff/1/extensions/common/manifest_... File extensions/common/manifest_constants.cc (right): https://codereview.chromium.org/2499493004/diff/1/extensions/common/manifest_... extensions/common/manifest_constants.cc:291: "This page cannot be scripted due to an ExtensionsSettings policy."; On 2016/11/23 17:22:56, Devlin wrote: > On 2016/11/23 01:19:23, Antony Sargent wrote: > > In theory, this file should have constants for keys in the manifest and error > > messages related to static parsing of extension manifest.json files. It looks > > like a few other messages that don't quite fit this criteria have crept in. > > > > Devlin might have a good idea of a better place for it. (These strings are > > distinct from user visible messages that get translated, since they are only > > reported to extensions in the chrome.runtime.lastError property of API methods > > like chrome.tabs.executeScript, etc. that developers can access) > > Typically, we just put the errors in the files where they're used (so either the > API implementation or permissions data). Done. https://codereview.chromium.org/2499493004/diff/1/extensions/common/permissio... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/1/extensions/common/permissio... extensions/common/permissions/permissions_data.cc:91: extension->permissions_data()->IsRuntimeBlockedHost(document_url)) { On 2016/11/23 17:22:56, Devlin (ooo until Jan 4) wrote: > This isn't the same type of restricted url as the rest here, and this is > redundant with the check in CanRunOnPage. The idea was to block any manipulation of a webpage protected by this policy in a way similar to the webstore. I was thinking there are some situations where this is called instead of CanRunOnPage. https://codereview.chromium.org/2499493004/diff/1/extensions/common/permissio... extensions/common/permissions/permissions_data.cc:345: if (runtime_blocked_hosts_unsafe_.MatchesURL(url)) { On 2016/11/23 01:19:23, asargent (OOO) wrote: > It seems like one of 2 things should happen here: > > a) runtime_lock_ is held > > b) use untime_blocked_hosts() to test against (instead of > runtime_blocked_hosts_unsafe_) > > (same thing below for runtime_allowed_hosts_unsafe) I may need a little bit of help with this. I tried making sure runtime_lock_ is held and it works in most every test except for when a new extension is added. Would you have 15 minutes to look at the trace of the issue? https://codereview.chromium.org/2499493004/diff/1/extensions/common/permissio... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/2499493004/diff/1/extensions/common/permissio... extensions/common/permissions/permissions_data.h:86: bool IsRuntimeBlockedHost(const GURL& url) const; On 2016/11/23 17:22:56, Devlin wrote: > Why does this need to be public? Not all API calls / events that disclose information about a particular website / tab / url require a host permission. For example, if an extension declares the "webNavigation" permission it can spy on every webpage the user visits. This could easily leak info about new projects, bearer tokens in URLs, etc. In a later CL, we modify webNavigation to not emit events when the navigation is to or from one of the URLs protected by this policy. This is detailed in the go/limit-host-permissions doc that talks about this overall effort. https://codereview.chromium.org/2499493004/diff/1/extensions/common/permissio... extensions/common/permissions/permissions_data.h:212: const URLPatternSet& runtime_blocked_hosts() const { On 2016/11/23 17:22:56, Devlin wrote: > ditto This is used during the renderer_startup_helper to provide an extension's current list of blocked hosts to a new renderer. https://codereview.chromium.org/2499493004/diff/1/extensions/common/permissio... extensions/common/permissions/permissions_data.h:279: mutable URLPatternSet runtime_blocked_hosts_unsafe_; On 2016/11/23 17:22:56, Devlin wrote: > Do we need this for each extension? It seems like almost all extensions will > use the default settings, so we shouldn't need to copy these over for every > single item when they're identical. Could we have these *only* if they differ > from the default? Done. https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1490: CHECK(settings); On 2016/12/22 22:15:39, zmin wrote: > Do we have to use CHECK() here? What will happened if we can't get the settings? > Can we use the DCHECK instead? I figured that if we aren't properly distributing security restrictions we'd want hard fail. Is this not usually the case?
asargent@chromium.org changed required reviewers: - asargent@chromium.org
I've just transferred to another team, so I'm going to remove myself as reviewer and hand this over to Devlin.
asargent@chromium.org changed reviewers: - asargent@chromium.org
https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1490: CHECK(settings); On 2017/01/02 19:57:45, nrpeter wrote: > On 2016/12/22 22:15:39, zmin wrote: > > Do we have to use CHECK() here? What will happened if we can't get the > settings? > > Can we use the DCHECK instead? > > I figured that if we aren't properly distributing security restrictions we'd > want hard fail. Is this not usually the case? Looks like there're some CHECK(settings) in extension_service.cc already so I'm OK with CHECK here. However, is it possible/necessary to have something in between. For example, block everything by default without getting settings properly?
I haven't reviewed completely, but is it necessary to use shared memory for this? https://codereview.chromium.org/2499493004/diff/20001/extensions/common/exten... File extensions/common/extension_messages.cc (right): https://codereview.chromium.org/2499493004/diff/20001/extensions/common/exten... extensions/common/extension_messages.cc:62: base::Pickle& pickle, const extensions::URLPatternSet& pattern_list) { Chromium style does not permit mutable reference parameters.
My first implementation didn't use shared memory and asargent@ suggested I re-write to use shared memory for performance. If using shared memory isn't required I'm happy to rip it out and make everything a bit simpler. I'd just like to make sure we come to a consensus before I spend more time making these changes so I don't waste time. On 2017/01/04 19:38:17, dcheng wrote: > I haven't reviewed completely, but is it necessary to use shared memory for > this? > > https://codereview.chromium.org/2499493004/diff/20001/extensions/common/exten... > File extensions/common/extension_messages.cc (right): > > https://codereview.chromium.org/2499493004/diff/20001/extensions/common/exten... > extensions/common/extension_messages.cc:62: base::Pickle& pickle, const > extensions::URLPatternSet& pattern_list) { > Chromium style does not permit mutable reference parameters.
On 2017/01/04 20:42:37, nrpeter wrote: > My first implementation didn't use shared memory and asargent@ suggested I > re-write to use shared memory for performance. If using shared memory isn't > required I'm happy to rip it out and make everything a bit simpler. I'd just > like to make sure we come to a consensus before I spend more time making these > changes so I don't waste time. > > > On 2017/01/04 19:38:17, dcheng wrote: > > I haven't reviewed completely, but is it necessary to use shared memory for > > this? > > > > > https://codereview.chromium.org/2499493004/diff/20001/extensions/common/exten... > > File extensions/common/extension_messages.cc (right): > > > > > https://codereview.chromium.org/2499493004/diff/20001/extensions/common/exten... > > extensions/common/extension_messages.cc:62: base::Pickle& pickle, const > > extensions::URLPatternSet& pattern_list) { > > Chromium style does not permit mutable reference parameters. It's my strong preference to avoid shared memory for this. How large are these pattern lists? Are they sent a lot? The current code still has to copy twice (once to write it into shared memory, and once to read it out), which is only one fewer copy than would be required by IPC (since you need an extra copy at the end).
On 2017/01/05 06:39:16, dcheng wrote: > On 2017/01/04 20:42:37, nrpeter wrote: > > My first implementation didn't use shared memory and asargent@ suggested I > > re-write to use shared memory for performance. If using shared memory isn't > > required I'm happy to rip it out and make everything a bit simpler. I'd just > > like to make sure we come to a consensus before I spend more time making these > > changes so I don't waste time. > > > > > > On 2017/01/04 19:38:17, dcheng wrote: > > > I haven't reviewed completely, but is it necessary to use shared memory for > > > this? > > > > > > > > > https://codereview.chromium.org/2499493004/diff/20001/extensions/common/exten... > > > File extensions/common/extension_messages.cc (right): > > > > > > > > > https://codereview.chromium.org/2499493004/diff/20001/extensions/common/exten... > > > extensions/common/extension_messages.cc:62: base::Pickle& pickle, const > > > extensions::URLPatternSet& pattern_list) { > > > Chromium style does not permit mutable reference parameters. > > It's my strong preference to avoid shared memory for this. How large are these > pattern lists? Are they sent a lot? The current code still has to copy twice > (once to write it into shared memory, and once to read it out), which is only > one fewer copy than would be required by IPC (since you need an extra copy at > the end). Currently there is no technical control limiting the number of URLPatterns in the policy. In most use cases, they'll be relatively small as most companies don't have too many sensitive sites (under 100). For some larger companies these could be in the thousands due to the fact that URLPattern doesn't allow a wildcard TLD e.g. *://*.example.*/*. This results in policies with the same hostname but modified effective TLD e.g. *://*.example.com/*, *://*.example.org/*, *://*.example.co.uk/* etc. We expect most extensions to use the default policy which is likely to be the largest. The default policy is only sent once per renderer when the renderer starts, or the policy changes. We could add a warning to users in the documentation that a very high number of entries could affect performance. If the extensions team is open to adding effective TLD wildcarding to URLPattern then the number of URLPatterns could be reduced considerably for large companies down to a few dozen in most cases. Effective TLD support would also help with the ever increasing number of TLDs. I could code this as a separate CL delivered in a later update. Adding effective TLD support to URLPattern may have other unintended side effects I'm not aware of, so I'd want to get approval from Devlin before going that route. If we're only getting one less copy with shared memory, I'm not sure its worth the added complexity and security vulnerabilities that can come from shared memory.
On 2017/01/05 18:56:33, nrpeter wrote: > On 2017/01/05 06:39:16, dcheng wrote: > > On 2017/01/04 20:42:37, nrpeter wrote: > > > My first implementation didn't use shared memory and asargent@ suggested I > > > re-write to use shared memory for performance. If using shared memory isn't > > > required I'm happy to rip it out and make everything a bit simpler. I'd just > > > like to make sure we come to a consensus before I spend more time making > these > > > changes so I don't waste time. > > > > > > > > > On 2017/01/04 19:38:17, dcheng wrote: > > > > I haven't reviewed completely, but is it necessary to use shared memory > for > > > > this? > > > > > > > > > > > > > > https://codereview.chromium.org/2499493004/diff/20001/extensions/common/exten... > > > > File extensions/common/extension_messages.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2499493004/diff/20001/extensions/common/exten... > > > > extensions/common/extension_messages.cc:62: base::Pickle& pickle, const > > > > extensions::URLPatternSet& pattern_list) { > > > > Chromium style does not permit mutable reference parameters. > > > > It's my strong preference to avoid shared memory for this. How large are these > > pattern lists? Are they sent a lot? The current code still has to copy twice > > (once to write it into shared memory, and once to read it out), which is only > > one fewer copy than would be required by IPC (since you need an extra copy at > > the end). > > Currently there is no technical control limiting the number of URLPatterns in > the policy. In most use cases, they'll be relatively small as most companies > don't have too many sensitive sites (under 100). For some larger companies these > could be in the thousands due to the fact that URLPattern doesn't allow a > wildcard TLD e.g. *://*.example.*/*. This results in policies with the same > hostname but modified effective TLD e.g. *://*.example.com/*, > *://*.example.org/*, *://*.example.co.uk/* etc. > > We expect most extensions to use the default policy which is likely to be the > largest. The default policy is only sent once per renderer when the renderer > starts, or the policy changes. We could add a warning to users in the > documentation that a very high number of entries could affect performance. > > If the extensions team is open to adding effective TLD wildcarding to URLPattern > then the number of URLPatterns could be reduced considerably for large companies > down to a few dozen in most cases. Effective TLD support would also help with > the ever increasing number of TLDs. I could code this as a separate CL delivered > in a later update. Adding effective TLD support to URLPattern may have other > unintended side effects I'm not aware of, so I'd want to get approval from > Devlin before going that route. > > If we're only getting one less copy with shared memory, I'm not sure its worth > the added complexity and security vulnerabilities that can come from shared > memory. I just looked at the policy we'd use internally, and there are a bit under 700 URLPatterns. I'd assume this would be similar for other large companies so its not quite as bad as the thousands I originally estimated.
On 2017/01/06 21:47:36, nrpeter wrote: > On 2017/01/05 18:56:33, nrpeter wrote: > > On 2017/01/05 06:39:16, dcheng wrote: > > > On 2017/01/04 20:42:37, nrpeter wrote: > > > > My first implementation didn't use shared memory and asargent@ suggested I > > > > re-write to use shared memory for performance. If using shared memory > isn't > > > > required I'm happy to rip it out and make everything a bit simpler. I'd > just > > > > like to make sure we come to a consensus before I spend more time making > > these > > > > changes so I don't waste time. > > > > > > > > > > > > On 2017/01/04 19:38:17, dcheng wrote: > > > > > I haven't reviewed completely, but is it necessary to use shared memory > > for > > > > > this? > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2499493004/diff/20001/extensions/common/exten... > > > > > File extensions/common/extension_messages.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2499493004/diff/20001/extensions/common/exten... > > > > > extensions/common/extension_messages.cc:62: base::Pickle& pickle, const > > > > > extensions::URLPatternSet& pattern_list) { > > > > > Chromium style does not permit mutable reference parameters. > > > > > > It's my strong preference to avoid shared memory for this. How large are > these > > > pattern lists? Are they sent a lot? The current code still has to copy twice > > > (once to write it into shared memory, and once to read it out), which is > only > > > one fewer copy than would be required by IPC (since you need an extra copy > at > > > the end). > > > > Currently there is no technical control limiting the number of URLPatterns in > > the policy. In most use cases, they'll be relatively small as most companies > > don't have too many sensitive sites (under 100). For some larger companies > these > > could be in the thousands due to the fact that URLPattern doesn't allow a > > wildcard TLD e.g. *://*.example.*/*. This results in policies with the same > > hostname but modified effective TLD e.g. *://*.example.com/*, > > *://*.example.org/*, *://*.example.co.uk/* etc. > > > > We expect most extensions to use the default policy which is likely to be the > > largest. The default policy is only sent once per renderer when the renderer > > starts, or the policy changes. We could add a warning to users in the > > documentation that a very high number of entries could affect performance. > > > > If the extensions team is open to adding effective TLD wildcarding to > URLPattern > > then the number of URLPatterns could be reduced considerably for large > companies > > down to a few dozen in most cases. Effective TLD support would also help with > > the ever increasing number of TLDs. I could code this as a separate CL > delivered > > in a later update. Adding effective TLD support to URLPattern may have other > > unintended side effects I'm not aware of, so I'd want to get approval from > > Devlin before going that route. > > > > If we're only getting one less copy with shared memory, I'm not sure its worth > > the added complexity and security vulnerabilities that can come from shared > > memory. > > I just looked at the policy we'd use internally, and there are a bit under 700 > URLPatterns. I'd assume this would be similar for other large companies so its > not quite as bad as the thousands I originally estimated. Sending over hundreds of url patterns makes me sad. If having TLD wildcards saves us from that, I'm fine with having it for internal cases like this. If we can put a cap at how many we can restrict to ensure performance, even better.
This is a little complicated for what it's doing - I'm wondering if we can find some simpler ways. A few high level questions. https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1490: CHECK(settings); On 2017/01/03 19:42:16, zmin wrote: > On 2017/01/02 19:57:45, nrpeter wrote: > > On 2016/12/22 22:15:39, zmin wrote: > > > Do we have to use CHECK() here? What will happened if we can't get the > > settings? > > > Can we use the DCHECK instead? > > > > I figured that if we aren't properly distributing security restrictions we'd > > want hard fail. Is this not usually the case? > > Looks like there're some CHECK(settings) in extension_service.cc already so I'm > OK with CHECK here. > However, is it possible/necessary to have something in between. For example, > block everything by default without getting settings properly? ExtensionManagementFactory is a BCKS - the only way this can fail is if our code is very wrong. DCHECK is sufficient. https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.h:76: void SetRuntimeBlockedAllowedHosts(const Extension* extension, RuntimeBlockedAllowedHosts makes no sense to me :) Is this blocked hosts? Allowed hosts? Hosts that were allowed and now blocked? Blocked and now allowed? A weird combinative state? https://codereview.chromium.org/2499493004/diff/20001/extensions/renderer/dis... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2499493004/diff/20001/extensions/renderer/dis... extensions/renderer/dispatcher.cc:1220: extension->permissions_data()->SetRuntimeBlockedAllowedHosts( Do we need to be able to support runtime extension permission changes? Or could this just happen on startup, in which case we can just send these over as part of the initial extension state?
On 2017/01/09 23:28:16, Devlin wrote: > On 2017/01/06 21:47:36, nrpeter wrote: > > On 2017/01/05 18:56:33, nrpeter wrote: > > > On 2017/01/05 06:39:16, dcheng wrote: > > > > On 2017/01/04 20:42:37, nrpeter wrote: > > > > > My first implementation didn't use shared memory and asargent@ suggested > I > > > > > re-write to use shared memory for performance. If using shared memory > > isn't > > > > > required I'm happy to rip it out and make everything a bit simpler. I'd > > just > > > > > like to make sure we come to a consensus before I spend more time making > > > these > > > > > changes so I don't waste time. > > > > > > > > > > > > > > > On 2017/01/04 19:38:17, dcheng wrote: > > > > > > I haven't reviewed completely, but is it necessary to use shared > memory > > > for > > > > > > this? > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2499493004/diff/20001/extensions/common/exten... > > > > > > File extensions/common/extension_messages.cc (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2499493004/diff/20001/extensions/common/exten... > > > > > > extensions/common/extension_messages.cc:62: base::Pickle& pickle, > const > > > > > > extensions::URLPatternSet& pattern_list) { > > > > > > Chromium style does not permit mutable reference parameters. > > > > > > > > It's my strong preference to avoid shared memory for this. How large are > > these > > > > pattern lists? Are they sent a lot? The current code still has to copy > twice > > > > (once to write it into shared memory, and once to read it out), which is > > only > > > > one fewer copy than would be required by IPC (since you need an extra copy > > at > > > > the end). > > > > > > Currently there is no technical control limiting the number of URLPatterns > in > > > the policy. In most use cases, they'll be relatively small as most companies > > > don't have too many sensitive sites (under 100). For some larger companies > > these > > > could be in the thousands due to the fact that URLPattern doesn't allow a > > > wildcard TLD e.g. *://*.example.*/*. This results in policies with the same > > > hostname but modified effective TLD e.g. *://*.example.com/*, > > > *://*.example.org/*, *://*.example.co.uk/* etc. > > > > > > We expect most extensions to use the default policy which is likely to be > the > > > largest. The default policy is only sent once per renderer when the renderer > > > starts, or the policy changes. We could add a warning to users in the > > > documentation that a very high number of entries could affect performance. > > > > > > If the extensions team is open to adding effective TLD wildcarding to > > URLPattern > > > then the number of URLPatterns could be reduced considerably for large > > companies > > > down to a few dozen in most cases. Effective TLD support would also help > with > > > the ever increasing number of TLDs. I could code this as a separate CL > > delivered > > > in a later update. Adding effective TLD support to URLPattern may have other > > > unintended side effects I'm not aware of, so I'd want to get approval from > > > Devlin before going that route. > > > > > > If we're only getting one less copy with shared memory, I'm not sure its > worth > > > the added complexity and security vulnerabilities that can come from shared > > > memory. > > > > I just looked at the policy we'd use internally, and there are a bit under 700 > > URLPatterns. I'd assume this would be similar for other large companies so its > > not quite as bad as the thousands I originally estimated. > > Sending over hundreds of url patterns makes me sad. If having TLD wildcards > saves us from that, I'm fine with having it for internal cases like this. If we > can put a cap at how many we can restrict to ensure performance, even better. With effective TLD wildcarding I'm fine with a limit, what about 500 URLPatterns?
I'm a fan of simplification as well, I'm going to be in town tomorrow for another meeting. Maybe we could sit down for 30 minutes and go over this together? https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.h:76: void SetRuntimeBlockedAllowedHosts(const Extension* extension, On 2017/01/09 23:30:57, Devlin wrote: > RuntimeBlockedAllowedHosts makes no sense to me :) Is this blocked hosts? > Allowed hosts? Hosts that were allowed and now blocked? Blocked and now > allowed? A weird combinative state? This function updates the list of hosts to be blocked (runtime_blocked_hosts) and the list of exceptions (runtime_allowed_hosts). Since they're only ever updated at the same time I didn't think it made sense to split this into two functions. I'm happy to change the name, maybe something like SetRuntimeHostPolicy? https://codereview.chromium.org/2499493004/diff/20001/extensions/renderer/dis... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2499493004/diff/20001/extensions/renderer/dis... extensions/renderer/dispatcher.cc:1220: extension->permissions_data()->SetRuntimeBlockedAllowedHosts( On 2017/01/09 23:30:57, Devlin wrote: > Do we need to be able to support runtime extension permission changes? Or could > this just happen on startup, in which case we can just send these over as part > of the initial extension state? We do need to support changes to the policy during runtime. There are a number of situations where this is necessary (blacklisting an extension, granting an exception, fixing a policy mistake, etc.).
I'm a fan of simplification as well, I'm going to be in town tomorrow for another meeting. Maybe we could sit down for 30 minutes and go over this together?
On 2017/01/10 17:48:07, nrpeter wrote: > I'm a fan of simplification as well, I'm going to be in town tomorrow for > another meeting. Maybe we could sit down for 30 minutes and go over this > together? (For others as FYI, nrpeter@ and I are meeting this morning to discuss)
On 2017/01/11 15:46:39, Devlin wrote: > On 2017/01/10 17:48:07, nrpeter wrote: > > I'm a fan of simplification as well, I'm going to be in town tomorrow for > > another meeting. Maybe we could sit down for 30 minutes and go over this > > together? > > (For others as FYI, nrpeter@ and I are meeting this morning to discuss) Big take-aways from the meeting - we can go ahead and do wildcard TLD matching, which also means getting rid of shared memory. We can then also impose a limit on the number of specifiable hosts. We can refactor the permissions data a bit as well.
https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1490: CHECK(settings); On 2017/01/09 23:30:57, Devlin wrote: > On 2017/01/03 19:42:16, zmin wrote: > > On 2017/01/02 19:57:45, nrpeter wrote: > > > On 2016/12/22 22:15:39, zmin wrote: > > > > Do we have to use CHECK() here? What will happened if we can't get the > > > settings? > > > > Can we use the DCHECK instead? > > > > > > I figured that if we aren't properly distributing security restrictions we'd > > > want hard fail. Is this not usually the case? > > > > Looks like there're some CHECK(settings) in extension_service.cc already so > I'm > > OK with CHECK here. > > However, is it possible/necessary to have something in between. For example, > > block everything by default without getting settings properly? > > ExtensionManagementFactory is a BCKS - the only way this can fail is if our code > is very wrong. DCHECK is sufficient. Done. https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1905: /* extensions::PermissionsUpdater(profile()).SetRuntimeBlockedAllowedHosts( On 2016/12/22 22:15:39, zmin wrote: > Please remove this. Done. https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/extension_with_management_policy_apitest.h (right): https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/extension_with_management_policy_apitest.h:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2016/12/22 22:15:39, zmin wrote: > no (c) > > https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md... Done. https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/20001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:365: for (RenderProcessHost::iterator i(RenderProcessHost::AllHostsIterator()); On 2016/12/22 22:15:39, zmin wrote: > Can we use "host_iterator" or some other meaningful name instead of "i" here? Done. https://codereview.chromium.org/2499493004/diff/20001/extensions/common/exten... File extensions/common/extension_messages.cc (right): https://codereview.chromium.org/2499493004/diff/20001/extensions/common/exten... extensions/common/extension_messages.cc:62: base::Pickle& pickle, const extensions::URLPatternSet& pattern_list) { On 2017/01/04 19:38:16, dcheng wrote: > Chromium style does not permit mutable reference parameters. Done. https://codereview.chromium.org/2499493004/diff/20001/extensions/common/exten... extensions/common/extension_messages.cc:108: CHECK(iter->ReadUInt32(&num_patterns)); On 2016/12/22 22:15:39, zmin wrote: > Again, CHECK is a very strong assertion. It means Chrome will crash if it > failed. Do we have to shutdown Chrome here if this failed? Done. https://codereview.chromium.org/2499493004/diff/20001/extensions/common/permi... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/20001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:389: if (!runtime_allowed_hosts().MatchesURL(url)) On 2016/12/22 22:15:39, zmin wrote: > Can we do: > return runtime_blocked_hosts().MatchesURL(url) && > !runtime_allowed_hosts().MatchesURL(url); Done.
https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_management_constants.cc (right): https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_management_constants.cc:25: const size_t kMaxItemsURLPatternSet = 500; Didn't we decide on 100 for this? https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_management_unittest.cc (left): https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_management_unittest.cc:128: const URLPatternSet& GetRuntimeBlockedHostsById(const std::string& id) { Why this change? https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:877: extensions::PermissionsUpdater(profile()).SetPolicyHostRestrictions( We seem to be calling this an awful lot. We should instead be able to hook into one or two places. Basically, I'd expect that we can merge SetPolicyHostRestrictions with PermissionsUpdater::InitializePermissions(), which is already called when the extension is added. We will have to update if we find a new policy, but otherwise that should cover all our bases, right? https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_with_management_policy_apitest.h (right): https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_with_management_policy_apitest.h:8: #include "chrome/browser/extensions/api/permissions/permissions_api.h" Surely we don't need quite this many includes? https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:147: std::move(runtime_blocked_hosts), std::move(runtime_allowed_hosts), std::move() casts to a &&, and const && are pretty useless. :) https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.h:12: #include "base/memory/shared_memory.h" needed? https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.h:76: void SetPolicyHostRestrictions(const Extension* extension, See comment in extension_service.cc https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.h:77: const URLPatternSet& runtime_blocked_hosts, don't forget to use git cl format - it's a life-saver. https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.h:106: POLICY nit: trailing comma https://codereview.chromium.org/2499493004/diff/40001/extensions/browser/rend... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2499493004/diff/40001/extensions/browser/rend... extensions/browser/renderer_startup_helper.cc:173: } why this extraneous change? https://codereview.chromium.org/2499493004/diff/40001/extensions/common/exten... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/2499493004/diff/40001/extensions/common/exten... extensions/common/extension_messages.h:313: // shared memory handle. outdated? https://codereview.chromium.org/2499493004/diff/40001/extensions/common/permi... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/40001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:34: URLPatternSet default_runtime_blocked_hosts_unsafe_ = URLPatternSet(); non-pod statics aren't allowed https://codereview.chromium.org/2499493004/diff/40001/extensions/common/permi... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/2499493004/diff/40001/extensions/common/permi... extensions/common/permissions/permissions_data.h:86: bool IsRuntimeBlockedHost(const GURL& url) const; Why do we need to expose this? It should be handled as part of CanAccessPage, etc. Callers should not need to have knowledge of policy-blocked vs user-blocked vs extension-not-requested vs optional vs .... https://codereview.chromium.org/2499493004/diff/40001/extensions/common/url_p... File extensions/common/url_pattern.h (right): https://codereview.chromium.org/2499493004/diff/40001/extensions/common/url_p... extensions/common/url_pattern.h:109: const bool allow_wildcard_effective_tld = false); default parameters aren't allowed by style https://codereview.chromium.org/2499493004/diff/40001/extensions/common/url_p... extensions/common/url_pattern.h:124: // Gets whether to match effective TLD of host(). If false, during a match we This comment doesn't seem quite clear to me by itself. Can you add an example? https://codereview.chromium.org/2499493004/diff/40001/extensions/common/url_p... File extensions/common/url_pattern_unittest.cc (right): https://codereview.chromium.org/2499493004/diff/40001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:455: TEST(URLPatternTest, Match20) { Match20 is a terrible name for a test. ;) (So is Match19, 18, etc, but let's break the pattern!) https://codereview.chromium.org/2499493004/diff/40001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:473: } Do we have tests of the inverse behavior?
https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_management_constants.cc (right): https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_management_constants.cc:25: const size_t kMaxItemsURLPatternSet = 500; On 2017/01/26 22:47:39, Devlin wrote: > Didn't we decide on 100 for this? Quite right, forgot to reduce after testing. https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_management_unittest.cc (left): https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_management_unittest.cc:128: const URLPatternSet& GetRuntimeBlockedHostsById(const std::string& id) { On 2017/01/26 22:47:39, Devlin wrote: > Why this change? It was a syntax issue I didn't detect earlier by compiling unit_tests. This function isn't actually necessary since RuntimeBlockedHosts is only ever queried via ID. I went ahead and removed this function and simplified GetRuntimeBlockedHosts. https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:877: extensions::PermissionsUpdater(profile()).SetPolicyHostRestrictions( On 2017/01/26 22:47:39, Devlin wrote: > We seem to be calling this an awful lot. We should instead be able to hook into > one or two places. > > Basically, I'd expect that we can merge SetPolicyHostRestrictions with > PermissionsUpdater::InitializePermissions(), which is already called when the > extension is added. We will have to update if we find a new policy, but > otherwise that should cover all our bases, right? The call under CheckManagementPolicy handles the situation where the policy changes and the renderers need the new policy so I think it makes sense to leave it. The calls under OnLoadedInstalledExtensions, AddExtension, EnableExtension were unnecessary and I removed them. The AddExtension call is covered by RendererStartupHelper::InitializeProcess. The EnableExtension and OnLoadedInstalledExtensions is covered by RendererStartupHelper::OnExtensionLoaded which already communicates the permissions to all existing renderers. https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/extension_with_management_policy_apitest.h (right): https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/extension_with_management_policy_apitest.h:8: #include "chrome/browser/extensions/api/permissions/permissions_api.h" On 2017/01/26 22:47:39, Devlin wrote: > Surely we don't need quite this many includes? Good catch, I forgot to remove these after refactoring. https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:147: std::move(runtime_blocked_hosts), std::move(runtime_allowed_hosts), On 2017/01/26 22:47:39, Devlin wrote: > std::move() casts to a &&, and const && are pretty useless. :) Done. https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.h:12: #include "base/memory/shared_memory.h" On 2017/01/26 22:47:39, Devlin wrote: > needed? Done. https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.h:76: void SetPolicyHostRestrictions(const Extension* extension, On 2017/01/26 22:47:39, Devlin wrote: > See comment in extension_service.cc Done. https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.h:77: const URLPatternSet& runtime_blocked_hosts, On 2017/01/26 22:47:39, Devlin wrote: > don't forget to use git cl format - it's a life-saver. Done. https://codereview.chromium.org/2499493004/diff/40001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.h:106: POLICY On 2017/01/26 22:47:39, Devlin wrote: > nit: trailing comma Done. https://codereview.chromium.org/2499493004/diff/40001/extensions/browser/rend... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2499493004/diff/40001/extensions/browser/rend... extensions/browser/renderer_startup_helper.cc:173: } On 2017/01/26 22:47:40, Devlin wrote: > why this extraneous change? Done. https://codereview.chromium.org/2499493004/diff/40001/extensions/common/exten... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/2499493004/diff/40001/extensions/common/exten... extensions/common/extension_messages.h:313: // shared memory handle. On 2017/01/26 22:47:40, Devlin wrote: > outdated? Done. https://codereview.chromium.org/2499493004/diff/40001/extensions/common/permi... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/40001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:34: URLPatternSet default_runtime_blocked_hosts_unsafe_ = URLPatternSet(); On 2017/01/26 22:47:40, Devlin wrote: > non-pod statics aren't allowed Done. https://codereview.chromium.org/2499493004/diff/40001/extensions/common/permi... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/2499493004/diff/40001/extensions/common/permi... extensions/common/permissions/permissions_data.h:86: bool IsRuntimeBlockedHost(const GURL& url) const; On 2017/01/26 22:47:40, Devlin wrote: > Why do we need to expose this? It should be handled as part of CanAccessPage, > etc. Callers should not need to have knowledge of policy-blocked vs > user-blocked vs extension-not-requested vs optional vs .... Done, made this private. https://codereview.chromium.org/2499493004/diff/40001/extensions/common/url_p... File extensions/common/url_pattern.h (right): https://codereview.chromium.org/2499493004/diff/40001/extensions/common/url_p... extensions/common/url_pattern.h:109: const bool allow_wildcard_effective_tld = false); On 2017/01/26 22:47:40, Devlin wrote: > default parameters aren't allowed by style Done, switched to overloading. https://codereview.chromium.org/2499493004/diff/40001/extensions/common/url_p... extensions/common/url_pattern.h:124: // Gets whether to match effective TLD of host(). If false, during a match we On 2017/01/26 22:47:40, Devlin wrote: > This comment doesn't seem quite clear to me by itself. Can you add an example? Done. https://codereview.chromium.org/2499493004/diff/40001/extensions/common/url_p... File extensions/common/url_pattern_unittest.cc (right): https://codereview.chromium.org/2499493004/diff/40001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:455: TEST(URLPatternTest, Match20) { On 2017/01/26 22:47:40, Devlin wrote: > Match20 is a terrible name for a test. ;) (So is Match19, 18, etc, but let's > break the pattern!) Done. https://codereview.chromium.org/2499493004/diff/40001/extensions/common/url_p... extensions/common/url_pattern_unittest.cc:473: } On 2017/01/26 22:47:40, Devlin wrote: > Do we have tests of the inverse behavior? When you say inverse behavior, do you mean a test that doesn't have an effective TLD wildcard (Match3)? Or do you mean a test of Parse that enables effective TLD wildcarding but doesn't use it? I'm not quite clear what you're asking for here.
https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_management.cc (right): https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:241: const Extension* extension) const { Pass by const reference if it's never null? *shrug* https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_management_internal.cc (right): https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_management_internal.cc:120: LOG(WARNING) << "Exceeded maximum number of URL match patterns (" DLOG(WARNING) https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_with_management_policy_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_with_management_policy_apitest.cc:36: for (auto it = request_log_.begin(); it != request_log_.end(); ++it) { Prefer range-based for loops. https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_with_management_policy_apitest.h (right): https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_with_management_policy_apitest.h:8: #include "chrome/browser/extensions/api/permissions/permissions_api.h" A lot of these headers look like they should be in th e.cc https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:334: if (event_name != NULL) nullptr, or just if (event_name) https://codereview.chromium.org/2499493004/diff/60001/extensions/common/permi... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/60001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:185: const URLPatternSet runtime_blocked_hosts, Nit: & https://codereview.chromium.org/2499493004/diff/60001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:200: new URLPatternSet(default_runtime_blocked_hosts); Will this leak the original heap allocation, if it's already assigned? https://codereview.chromium.org/2499493004/diff/60001/extensions/common/url_p... File extensions/common/url_pattern.h (right): https://codereview.chromium.org/2499493004/diff/60001/extensions/common/url_p... extensions/common/url_pattern.h:110: const bool allow_wildcard_effective_tld); Nit: it's preferable to have explicitly named enum values instead of a bool when the meaning isn't clear from the method name https://codereview.chromium.org/2499493004/diff/60001/extensions/renderer/dis... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2499493004/diff/60001/extensions/renderer/dis... extensions/renderer/dispatcher.cc:1084: URLPatternSet runtime_blocked_hosts = param.runtime_blocked_hosts; const URLPatternSet& here and below (the current implementation will copy twice instead of just once) https://codereview.chromium.org/2499493004/diff/60001/extensions/renderer/dis... extensions/renderer/dispatcher.cc:1201: UpdateBindings(""); Nit: std::string() rather than ""
https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_management.cc (right): https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:241: const Extension* extension) const { On 2017/02/06 07:05:29, dcheng wrote: > Pass by const reference if it's never null? *shrug* Everything else in extension_management that passes an extension uses const *. Would we rather have consistency or switch this to const &? I'm fine either way, just let me know. https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_management_internal.cc (right): https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_management_internal.cc:120: LOG(WARNING) << "Exceeded maximum number of URL match patterns (" On 2017/02/06 07:05:29, dcheng wrote: > DLOG(WARNING) Why would we want to change this to a DLOG? We want this warning to display in the release version as well. This indicates to an administrator that the policy they're using is invalid. https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_with_management_policy_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_with_management_policy_apitest.cc:36: for (auto it = request_log_.begin(); it != request_log_.end(); ++it) { On 2017/02/06 07:05:29, dcheng wrote: > Prefer range-based for loops. Done. https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_with_management_policy_apitest.h (right): https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_with_management_policy_apitest.h:8: #include "chrome/browser/extensions/api/permissions/permissions_api.h" On 2017/02/06 07:05:29, dcheng wrote: > A lot of these headers look like they should be in th e.cc Done. https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:334: if (event_name != NULL) On 2017/02/06 07:05:29, dcheng wrote: > nullptr, or just if (event_name) Done. https://codereview.chromium.org/2499493004/diff/60001/extensions/common/permi... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/60001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:185: const URLPatternSet runtime_blocked_hosts, On 2017/02/06 07:05:29, dcheng wrote: > Nit: & Done. https://codereview.chromium.org/2499493004/diff/60001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:200: new URLPatternSet(default_runtime_blocked_hosts); On 2017/02/06 07:05:29, dcheng wrote: > Will this leak the original heap allocation, if it's already assigned? Quite correct since I can't use unique_ptr::reset due to static POD requirements. C++ isn't my main language, so a double check if this fix would be much appreciated as I don't want to introduce a UAF. https://codereview.chromium.org/2499493004/diff/60001/extensions/common/url_p... File extensions/common/url_pattern.h (right): https://codereview.chromium.org/2499493004/diff/60001/extensions/common/url_p... extensions/common/url_pattern.h:110: const bool allow_wildcard_effective_tld); On 2017/02/06 07:05:29, dcheng wrote: > Nit: it's preferable to have explicitly named enum values instead of a bool when > the meaning isn't clear from the method name Done. https://codereview.chromium.org/2499493004/diff/60001/extensions/renderer/dis... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2499493004/diff/60001/extensions/renderer/dis... extensions/renderer/dispatcher.cc:1084: URLPatternSet runtime_blocked_hosts = param.runtime_blocked_hosts; On 2017/02/06 07:05:29, dcheng wrote: > const URLPatternSet& here and below (the current implementation will copy twice > instead of just once) Done. https://codereview.chromium.org/2499493004/diff/60001/extensions/renderer/dis... extensions/renderer/dispatcher.cc:1201: UpdateBindings(""); On 2017/02/06 07:05:29, dcheng wrote: > Nit: std::string() rather than "" Done. FYI: Everywhere else in the file that we use UpdateBindings with an empty string, it uses "".
https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_management.cc (right): https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_management.cc:241: const Extension* extension) const { On 2017/02/06 22:53:15, nrpeter wrote: > On 2017/02/06 07:05:29, dcheng wrote: > > Pass by const reference if it's never null? *shrug* > > Everything else in extension_management that passes an extension uses const *. > Would we rather have consistency or switch this to const &? I'm fine either way, > just let me know. I guess I'll defer to devlin on this one, but that's my preference (even if it means it'll take some time to converge on a consistent style here). https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... File chrome/browser/extensions/extension_management_internal.cc (right): https://codereview.chromium.org/2499493004/diff/60001/chrome/browser/extensio... chrome/browser/extensions/extension_management_internal.cc:120: LOG(WARNING) << "Exceeded maximum number of URL match patterns (" On 2017/02/06 22:53:15, nrpeter wrote: > On 2017/02/06 07:05:29, dcheng wrote: > > DLOG(WARNING) > > Why would we want to change this to a DLOG? We want this warning to display in > the release version as well. This indicates to an administrator that the policy > they're using is invalid. In general, having release logging should be extremely rare since it costs binary size. I'll defer to devlin on this one as well, but it's a reasonable point. https://codereview.chromium.org/2499493004/diff/60001/extensions/common/permi... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/60001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:200: new URLPatternSet(default_runtime_blocked_hosts); On 2017/02/06 22:53:15, nrpeter wrote: > On 2017/02/06 07:05:29, dcheng wrote: > > Will this leak the original heap allocation, if it's already assigned? > > Quite correct since I can't use unique_ptr::reset due to static POD > requirements. > > C++ isn't my main language, so a double check if this fix would be much > appreciated as I don't want to introduce a UAF. Since URLPatternSet is assignable, I would recommend something along these lines: GetDefaultRuntimeBlockedHosts() = default_runtime_blocked_hosts; with: URLPatternSet& GetDefaultRuntimeBlockedHosts() { CR_DEFINE_STATIC_LOCAL(URLPatternSet, patterns, ()); return patterns; }; https://codereview.chromium.org/2499493004/diff/60001/extensions/renderer/dis... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2499493004/diff/60001/extensions/renderer/dis... extensions/renderer/dispatcher.cc:1201: UpdateBindings(""); On 2017/02/06 22:53:15, nrpeter wrote: > On 2017/02/06 07:05:29, dcheng wrote: > > Nit: std::string() rather than "" > > Done. > > FYI: Everywhere else in the file that we use UpdateBindings with an empty > string, it uses "". Yeah, it's more about long-term consistency here. "" is a bit sad; with libstdc++, you get a heap allocation... to hold an empty string. With SSO, it's a bit better, but it's even cheaper to not memcpy an empty string to begin with.
nrpeter@google.com changed reviewers: + pastarmovj@chromium.org
https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/content_script_apitest.cc:680: // Test that optional permissions blocked by enterprise policy will be denied What optional permissions? https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (left): https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:874: nit: remove extraneous change https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1463: remove extraneous change https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/extension_with_management_policy_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_with_management_policy_apitest.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/extension_with_management_policy_apitest.h (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_with_management_policy_apitest.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. not 2016 anymore https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:158: // Keep track of runtime blocked and hosts for this extension in the browser which extension? https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:302: content::NotificationService::current()->Notify( We should still notify the rest of chrome when we change permissions. https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.h:82: // SetPolicyHostRestrictions. This is the Deault scope "*" of I don't follow the last sentence here.... https://codereview.chromium.org/2499493004/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/policy_blocked/background.js (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/policy_blocked/background.js:5: // Give 2 seconds to inject JS, if no response hasn't injected This is inherently flaky - there is no guarantee that js will have injected in 2 seconds. Instead, we should cause a visible change to the page (e.g. setting a variable on window, modifying the title, etc), wait for a deterministic point after which we know the content script would have injected, and then check it. https://codereview.chromium.org/2499493004/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/policy_blocked/background.js:8: }, 2000); <random comment spot because I can't comment in inject.html> looks like inject.html is empty? https://codereview.chromium.org/2499493004/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/policy_blocked/inject.js (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/policy_blocked/inject.js:6: chrome.test.notifyFail("Injected into page protected by policy"); single quotes in js https://codereview.chromium.org/2499493004/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/policy_blocked/manifest.json (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/policy_blocked/manifest.json:7: "scripts": ["background.js"] use 2-space indentation in json https://codereview.chromium.org/2499493004/diff/80001/extensions/common/permi... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/80001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:34: URLPatternSet* default_runtime_blocked_hosts_unsafe_ = nullptr; the trailing _ is used for member variables; this shouldn't have one. https://codereview.chromium.org/2499493004/diff/80001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:34: URLPatternSet* default_runtime_blocked_hosts_unsafe_ = nullptr; I think I'd prefer these to be stored in a struct with a (leaky) LazyInstance. https://codereview.chromium.org/2499493004/diff/80001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:159: const URLPatternSet& PermissionsData::runtime_blocked_hosts() const { We should check thread access here. https://codereview.chromium.org/2499493004/diff/80001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:398: return runtime_blocked_hosts().MatchesURL(url) && we can use the sets directly here. https://codereview.chromium.org/2499493004/diff/80001/extensions/common/permi... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/2499493004/diff/80001/extensions/common/permi... extensions/common/permissions/permissions_data.h:225: static const URLPatternSet& default_runtime_blocked_hosts(); These should only be used for serialization during message passing, right? Can we make the method end in something like "for_serialization()" so that's clear, and add a comment to use the utility methods above (e.g. CanAccessPage())? https://codereview.chromium.org/2499493004/diff/80001/extensions/common/permi... extensions/common/permissions/permissions_data.h:298: mutable URLPatternSet runtime_blocked_hosts_unsafe_; runtime_blocked_hosts sounds weird - maybe policy_blocked_hosts? https://codereview.chromium.org/2499493004/diff/80001/extensions/common/url_p... File extensions/common/url_pattern.cc (right): https://codereview.chromium.org/2499493004/diff/80001/extensions/common/url_p... extensions/common/url_pattern.cc:282: host_components.size() > 1 && host_components[1] == "*") { Won't this fail on something like maps.google.*, where host_components[1] == google? (Should this be a test case?) https://codereview.chromium.org/2499493004/diff/80001/extensions/common/url_p... File extensions/common/url_pattern.h (right): https://codereview.chromium.org/2499493004/diff/80001/extensions/common/url_p... extensions/common/url_pattern.h:113: // for the effective TLD, make |allow_wildcard_effective_tld| true. allow_wildcard_effective_tld doesn't exist. :) https://codereview.chromium.org/2499493004/diff/80001/extensions/common/url_p... extensions/common/url_pattern.h:116: const ParseOptions parse_options); no need to pass an enum as const https://codereview.chromium.org/2499493004/diff/80001/extensions/common/url_p... extensions/common/url_pattern.h:131: // Gets whether host() contains an effective TLD. If false, durring s/durring/during https://codereview.chromium.org/2499493004/diff/80001/extensions/renderer/dis... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2499493004/diff/80001/extensions/renderer/dis... extensions/renderer/dispatcher.cc:1224: runtime_blocked_hosts = params.runtime_blocked_hosts; won't params.runtime_blocked_hosts be empty if params.uses_default_policy_host_restrictions is true? So can't we just do SetPolicyHostRestrictions(params.blocked, params.allowed, params.uses_default)?
https://codereview.chromium.org/2499493004/diff/60001/extensions/common/permi... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/60001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:200: new URLPatternSet(default_runtime_blocked_hosts); On 2017/02/07 07:47:12, dcheng wrote: > On 2017/02/06 22:53:15, nrpeter wrote: > > On 2017/02/06 07:05:29, dcheng wrote: > > > Will this leak the original heap allocation, if it's already assigned? > > > > Quite correct since I can't use unique_ptr::reset due to static POD > > requirements. > > > > C++ isn't my main language, so a double check if this fix would be much > > appreciated as I don't want to introduce a UAF. > > Since URLPatternSet is assignable, I would recommend something along these > lines: > GetDefaultRuntimeBlockedHosts() = default_runtime_blocked_hosts; > > with: > URLPatternSet& GetDefaultRuntimeBlockedHosts() { > CR_DEFINE_STATIC_LOCAL(URLPatternSet, patterns, ()); > return patterns; > }; Devlin requested this to be a Leaky LazyInstance and I've updated the code to do that. Does that work for you? https://codereview.chromium.org/2499493004/diff/60001/extensions/renderer/dis... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2499493004/diff/60001/extensions/renderer/dis... extensions/renderer/dispatcher.cc:1201: UpdateBindings(""); On 2017/02/07 07:47:12, dcheng wrote: > On 2017/02/06 22:53:15, nrpeter wrote: > > On 2017/02/06 07:05:29, dcheng wrote: > > > Nit: std::string() rather than "" > > > > Done. > > > > FYI: Everywhere else in the file that we use UpdateBindings with an empty > > string, it uses "". > > Yeah, it's more about long-term consistency here. "" is a bit sad; with > libstdc++, you get a heap allocation... to hold an empty string. > > With SSO, it's a bit better, but it's even cheaper to not memcpy an empty string > to begin with. Done. https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/content_script_apitest.cc:680: // Test that optional permissions blocked by enterprise policy will be denied On 2017/02/14 23:17:09, Devlin wrote: > What optional permissions? This test is not needed as MAYBE_ContentScriptPermissionApi covers the same situations. https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/extension_service.cc (left): https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:874: On 2017/02/14 23:17:09, Devlin wrote: > nit: remove extraneous change Done. https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_service.cc:1463: On 2017/02/14 23:17:09, Devlin wrote: > remove extraneous change Done. https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/extension_with_management_policy_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_with_management_policy_apitest.cc:1: // Copyright (c) 2016 The Chromium Authors. All rights reserved. On 2017/02/14 23:17:09, Devlin wrote: > no (c) Done. https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/extension_with_management_policy_apitest.h (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/extension_with_management_policy_apitest.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2017/02/14 23:17:09, Devlin wrote: > not 2016 anymore Done. https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:158: // Keep track of runtime blocked and hosts for this extension in the browser On 2017/02/14 23:17:10, Devlin wrote: > which extension? This applies to multiple extensions, I've updated the note to reflect this. https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:302: content::NotificationService::current()->Notify( On 2017/02/14 23:17:10, Devlin wrote: > We should still notify the rest of chrome when we change permissions. For privacy reasons, we avoid disclosing the list of sites that were added or removed from this list. We could send a blank message to Chrome saying permissions were updated, but not including what was updated. I'm not sure how helpful that would be though... thoughts? https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.h:82: // SetPolicyHostRestrictions. This is the Deault scope "*" of On 2017/02/14 23:17:10, Devlin wrote: > I don't follow the last sentence here.... Removed as it wasn't strictly necessary. https://codereview.chromium.org/2499493004/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/policy_blocked/background.js (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/policy_blocked/background.js:5: // Give 2 seconds to inject JS, if no response hasn't injected On 2017/02/14 23:17:10, Devlin wrote: > This is inherently flaky - there is no guarantee that js will have injected in 2 > seconds. > > Instead, we should cause a visible change to the page (e.g. setting a variable > on window, modifying the title, etc), wait for a deterministic point after which > we know the content script would have injected, and then check it. This case is already tested under a different test, removed it. https://codereview.chromium.org/2499493004/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/policy_blocked/background.js:8: }, 2000); On 2017/02/14 23:17:10, Devlin wrote: > <random comment spot because I can't comment in inject.html> looks like > inject.html is empty? Yes, I just needed a valid page to browse to so code could be injected. This test was removed however. https://codereview.chromium.org/2499493004/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/policy_blocked/inject.js (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/policy_blocked/inject.js:6: chrome.test.notifyFail("Injected into page protected by policy"); On 2017/02/14 23:17:10, Devlin wrote: > single quotes in js Test was not needed. https://codereview.chromium.org/2499493004/diff/80001/chrome/test/data/extens... File chrome/test/data/extensions/api_test/content_scripts/policy_blocked/manifest.json (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/test/data/extens... chrome/test/data/extensions/api_test/content_scripts/policy_blocked/manifest.json:7: "scripts": ["background.js"] On 2017/02/14 23:17:10, Devlin wrote: > use 2-space indentation in json Test removed https://codereview.chromium.org/2499493004/diff/80001/extensions/common/permi... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/80001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:34: URLPatternSet* default_runtime_blocked_hosts_unsafe_ = nullptr; On 2017/02/14 23:17:10, Devlin wrote: > the trailing _ is used for member variables; this shouldn't have one. Done. https://codereview.chromium.org/2499493004/diff/80001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:34: URLPatternSet* default_runtime_blocked_hosts_unsafe_ = nullptr; On 2017/02/14 23:17:10, Devlin wrote: > I think I'd prefer these to be stored in a struct with a (leaky) LazyInstance. Done. https://codereview.chromium.org/2499493004/diff/80001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:159: const URLPatternSet& PermissionsData::runtime_blocked_hosts() const { On 2017/02/14 23:17:10, Devlin wrote: > We should check thread access here. Done. https://codereview.chromium.org/2499493004/diff/80001/extensions/common/permi... extensions/common/permissions/permissions_data.cc:398: return runtime_blocked_hosts().MatchesURL(url) && On 2017/02/14 23:17:10, Devlin wrote: > we can use the sets directly here. If we did, we'd end up duplicating the code to check whether we're using default restrictions or per-extension restrictions. I'd rather not duplicate code if it can be avoided. https://codereview.chromium.org/2499493004/diff/80001/extensions/common/permi... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/2499493004/diff/80001/extensions/common/permi... extensions/common/permissions/permissions_data.h:225: static const URLPatternSet& default_runtime_blocked_hosts(); On 2017/02/14 23:17:10, Devlin wrote: > These should only be used for serialization during message passing, right? Can > we make the method end in something like "for_serialization()" so that's clear, > and add a comment to use the utility methods above (e.g. CanAccessPage())? This has two uses 1. For serialization 2. For use within utility methods Since this method is used within utility methods, I'm not sure adding for_serialization to the name makes sense. I've added comments to these methods to indicate they shouldn't be used directly, is that good enough? https://codereview.chromium.org/2499493004/diff/80001/extensions/common/permi... extensions/common/permissions/permissions_data.h:298: mutable URLPatternSet runtime_blocked_hosts_unsafe_; On 2017/02/14 23:17:10, Devlin wrote: > runtime_blocked_hosts sounds weird - maybe policy_blocked_hosts? Done. https://codereview.chromium.org/2499493004/diff/80001/extensions/common/url_p... File extensions/common/url_pattern.cc (right): https://codereview.chromium.org/2499493004/diff/80001/extensions/common/url_p... extensions/common/url_pattern.cc:282: host_components.size() > 1 && host_components[1] == "*") { On 2017/02/14 23:17:10, Devlin wrote: > Won't this fail on something like maps.google.*, where host_components[1] == > google? (Should this be a test case?) Yes, updated. Added as a test case as well. https://codereview.chromium.org/2499493004/diff/80001/extensions/common/url_p... File extensions/common/url_pattern.h (right): https://codereview.chromium.org/2499493004/diff/80001/extensions/common/url_p... extensions/common/url_pattern.h:113: // for the effective TLD, make |allow_wildcard_effective_tld| true. On 2017/02/14 23:17:10, Devlin wrote: > allow_wildcard_effective_tld doesn't exist. :) Done. https://codereview.chromium.org/2499493004/diff/80001/extensions/common/url_p... extensions/common/url_pattern.h:116: const ParseOptions parse_options); On 2017/02/14 23:17:10, Devlin wrote: > no need to pass an enum as const Done. https://codereview.chromium.org/2499493004/diff/80001/extensions/common/url_p... extensions/common/url_pattern.h:131: // Gets whether host() contains an effective TLD. If false, durring On 2017/02/14 23:17:10, Devlin wrote: > s/durring/during Done. https://codereview.chromium.org/2499493004/diff/80001/extensions/renderer/dis... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2499493004/diff/80001/extensions/renderer/dis... extensions/renderer/dispatcher.cc:1224: runtime_blocked_hosts = params.runtime_blocked_hosts; On 2017/02/14 23:17:10, Devlin wrote: > won't params.runtime_blocked_hosts be empty if > params.uses_default_policy_host_restrictions is true? > > So can't we just do SetPolicyHostRestrictions(params.blocked, params.allowed, > params.uses_default)? We could, but SetPolicyHostRestrictions has to acquire a lock before proceeding. I figured it'd be better performance wise to avoid that if possible. https://codereview.chromium.org/2499493004/diff/100001/extensions/common/url_... File extensions/common/url_pattern_unittest.cc (right): https://codereview.chromium.org/2499493004/diff/100001/extensions/common/url_... extensions/common/url_pattern_unittest.cc:474: URLPattern pattern_sub(kAllSchemes); Tests the bug rdcronin@ identified in url_pattern.cc
https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:302: content::NotificationService::current()->Notify( On 2017/03/22 23:47:39, nrpeter wrote: > On 2017/02/14 23:17:10, Devlin wrote: > > We should still notify the rest of chrome when we change permissions. > > For privacy reasons, we avoid disclosing the list of sites that were added or > removed from this list. We could send a blank message to Chrome saying > permissions were updated, but not including what was updated. I'm not sure how > helpful that would be though... thoughts? ... I'm confused. Chrome is responsible for enforcing these permissions, and trivially and requisitely knows what they are. Any object in chrome that *really* wanted to know could, hypothetically, continuously poll ExtensionManagement and look for changes. What exactly are we trying to prevent? https://codereview.chromium.org/2499493004/diff/80001/extensions/renderer/dis... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2499493004/diff/80001/extensions/renderer/dis... extensions/renderer/dispatcher.cc:1224: runtime_blocked_hosts = params.runtime_blocked_hosts; On 2017/03/22 23:47:39, nrpeter wrote: > On 2017/02/14 23:17:10, Devlin wrote: > > won't params.runtime_blocked_hosts be empty if > > params.uses_default_policy_host_restrictions is true? > > > > So can't we just do SetPolicyHostRestrictions(params.blocked, params.allowed, > > params.uses_default)? > > We could, but SetPolicyHostRestrictions has to acquire a lock before proceeding. > I figured it'd be better performance wise to avoid that if possible. We unconditionally call SetPolicyHostRestrictions(), though, don't we? It's not conditional based on the if on line 1223. So all we're doing here is creating an extra URLPatternSet and causing a potentially-expensive copy when there are policy restrictions. Or am I missing something? https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/content_script_apitest.cc:472: #define MAYBE_ContentScriptPermissionsApi DISABLED_ContentScriptPermissionsApi We probably shouldn't be relying on a test that's disabled on windows. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_management.cc (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_management.cc:242: return (settings_by_id_.find(extension->id()) == settings_by_id_.end()); parens unnecessary https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_with_management_policy_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.cc:14: void ExtensionApiTestWithManagementPolicy::SetUpInProcessBrowserTestFixture() { Why does this need to be done in SetUpInProcessBrowserTestFixture() instead of SetUpOnMainThread()? https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.cc:38: for (auto it : request_log_) { this results in a copy for each entry; prefer const auto& https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.cc:42: return false; This would be simpler as: return std::find_if(request_log_.begin(), request_log_.end(), [test_host](const ManagementPolicyRequestLog& log) { return log.host == test_host; }) != request_log_.end(); https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_with_management_policy_apitest.h (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.h:11: struct ManagementPolicyRequestLog { Any reason this can't be a nested struct in ExtensionApiTestWithManagementPolicy? https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.h:12: std::string all_headers; iwyu string https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.h:27: bool BrowsedTo(const char* test_host); Are these methods needed? https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.h:32: std::vector<ManagementPolicyRequestLog> request_log_; iwyu vector https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.h:33: }; DISALLOW_COPY_AND_ASSIGN https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:141: const URLPatternSet& runtime_blocked_hosts, See comment in permissions_data.h, but same thing here - have two options to either a) set explicit hosts or b) set to use defaults. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:286: // Policy isn't exposed via JS API. What is this comment referring to? https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.h:74: // This is the individual scope of ExtensionSettings. This line isn't really necessary. https://codereview.chromium.org/2499493004/diff/100001/extensions/DEPS File extensions/DEPS (right): https://codereview.chromium.org/2499493004/diff/100001/extensions/DEPS#newcode50 extensions/DEPS:50: "+chrome/browser/extensions/extension_with_management_policy_apitest.h", Why do we need this? https://codereview.chromium.org/2499493004/diff/100001/extensions/common/exte... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/2499493004/diff/100001/extensions/common/exte... extensions/common/extension_messages.h:306: // Contians URLPatternSets defining which URLs an extension may not interact s/Contians/Contains https://codereview.chromium.org/2499493004/diff/100001/extensions/common/perm... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/2499493004/diff/100001/extensions/common/perm... extensions/common/permissions/permissions_data.h:105: const bool is_default_runtime_blocked_allowed_hosts) const; To me, it seems like this shouldn't take three parameters, and instead should be two functions: SetPolicyHostRestrictions( const URLPatternSet& runtime_blocked_hosts, const URLPatternSet& runtime_allowed_hosts); SetUsesDefaultHostRestrictions(bool uses_default_restrictions); Otherwise, what do we pass in the blocked/allowed hosts when uses default is true? If we pass in the defaults, then it results in massive memory waste - each extension storing a copy of the defaults - and makes it easier to lead to bugs (if we update the defaults, we'd need to also update extensions). If we pass in empty sets, then we didn't need to pass in anything. We should prefer one of the two routes - pass in the hosts if they are specified, or else just use the defaults. https://codereview.chromium.org/2499493004/diff/100001/extensions/common/perm... extensions/common/permissions/permissions_data.h:109: // per-extnsion basis with SetPolicyHostRestrictions. per-extension https://codereview.chromium.org/2499493004/diff/100001/extensions/renderer/di... File extensions/renderer/dispatcher.h (right): https://codereview.chromium.org/2499493004/diff/100001/extensions/renderer/di... extensions/renderer/dispatcher.h:196: void OnUpdatePolicyHostRestrictions( This method doesn't appear to exist?
https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:302: content::NotificationService::current()->Notify( On 2017/03/29 21:36:49, Devlin wrote: > On 2017/03/22 23:47:39, nrpeter wrote: > > On 2017/02/14 23:17:10, Devlin wrote: > > > We should still notify the rest of chrome when we change permissions. > > > > For privacy reasons, we avoid disclosing the list of sites that were added or > > removed from this list. We could send a blank message to Chrome saying > > permissions were updated, but not including what was updated. I'm not sure how > > helpful that would be though... thoughts? > > ... I'm confused. Chrome is responsible for enforcing these permissions, and > trivially and requisitely knows what they are. Any object in chrome that > *really* wanted to know could, hypothetically, continuously poll > ExtensionManagement and look for changes. What exactly are we trying to > prevent? I'm not worried about another part of Chrome viewing this list. We'd like to avoid disclosing the list of URLPatterns to extensions via permissions.onAdded and permissions.onRemoved. IIUC, those events are triggered by this code, is that not the case? https://codereview.chromium.org/2499493004/diff/80001/extensions/renderer/dis... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2499493004/diff/80001/extensions/renderer/dis... extensions/renderer/dispatcher.cc:1224: runtime_blocked_hosts = params.runtime_blocked_hosts; On 2017/03/29 21:36:49, Devlin wrote: > On 2017/03/22 23:47:39, nrpeter wrote: > > On 2017/02/14 23:17:10, Devlin wrote: > > > won't params.runtime_blocked_hosts be empty if > > > params.uses_default_policy_host_restrictions is true? > > > > > > So can't we just do SetPolicyHostRestrictions(params.blocked, > params.allowed, > > > params.uses_default)? > > > > We could, but SetPolicyHostRestrictions has to acquire a lock before > proceeding. > > I figured it'd be better performance wise to avoid that if possible. > > We unconditionally call SetPolicyHostRestrictions(), though, don't we? It's not > conditional based on the if on line 1223. So all we're doing here is creating > an extra URLPatternSet and causing a potentially-expensive copy when there are > policy restrictions. Or am I missing something? Done. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/content_script_apitest.cc:472: #define MAYBE_ContentScriptPermissionsApi DISABLED_ContentScriptPermissionsApi On 2017/03/29 21:36:49, Devlin wrote: > We probably shouldn't be relying on a test that's disabled on windows. Looks like this was disabled ~4 years ago on Windows for flakiness. It could be that this is no longer flaky. Could we mark this from DISABLED_ to FLAKY_ and gather a few days worth of data? Or is there another way of determining flakiness? We should probably have permission tests on Windows anyways. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_management.cc (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_management.cc:242: return (settings_by_id_.find(extension->id()) == settings_by_id_.end()); On 2017/03/29 21:36:49, Devlin wrote: > parens unnecessary Done. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_with_management_policy_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.cc:14: void ExtensionApiTestWithManagementPolicy::SetUpInProcessBrowserTestFixture() { On 2017/03/29 21:36:49, Devlin wrote: > Why does this need to be done in SetUpInProcessBrowserTestFixture() instead of > SetUpOnMainThread()? This was moved from permissions_apitest.cc since we are adding additional capabilities. I just copied what was there since it works. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.cc:38: for (auto it : request_log_) { On 2017/03/29 21:36:49, Devlin wrote: > this results in a copy for each entry; prefer const auto& Done. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.cc:42: return false; On 2017/03/29 21:36:49, Devlin wrote: > This would be simpler as: > return std::find_if(request_log_.begin(), request_log_.end(), [test_host](const > ManagementPolicyRequestLog& log) { return log.host == test_host; }) != > request_log_.end(); Done. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_with_management_policy_apitest.h (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.h:11: struct ManagementPolicyRequestLog { On 2017/03/29 21:36:49, Devlin wrote: > Any reason this can't be a nested struct in > ExtensionApiTestWithManagementPolicy? Done. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.h:12: std::string all_headers; On 2017/03/29 21:36:49, Devlin wrote: > iwyu string Done. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.h:27: bool BrowsedTo(const char* test_host); On 2017/03/29 21:36:50, Devlin wrote: > Are these methods needed? Since I removed the redundant test they are no longer used in this CL. They are however used in the next 5 CLs that use this as a diffbase. I can remove this and put it in the later CLs if necessary. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.h:32: std::vector<ManagementPolicyRequestLog> request_log_; On 2017/03/29 21:36:49, Devlin wrote: > iwyu vector Done. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.h:33: }; On 2017/03/29 21:36:49, Devlin wrote: > DISALLOW_COPY_AND_ASSIGN Done. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:141: const URLPatternSet& runtime_blocked_hosts, On 2017/03/29 21:36:50, Devlin wrote: > See comment in permissions_data.h, but same thing here - have two options to > either a) set explicit hosts or b) set to use defaults. Done. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:286: // Policy isn't exposed via JS API. On 2017/03/29 21:36:50, Devlin wrote: > What is this comment referring to? AFAIK, notification here are exposed to extensions via permissions.onAdded and permissions.onRemoved. We don't want the URLPatterns in this policy to be exposed to an extension, so we don't send a noficiation. Please let me know if this understanding is incorrect. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.h:74: // This is the individual scope of ExtensionSettings. On 2017/03/29 21:36:50, Devlin wrote: > This line isn't really necessary. Done. https://codereview.chromium.org/2499493004/diff/100001/extensions/DEPS File extensions/DEPS (right): https://codereview.chromium.org/2499493004/diff/100001/extensions/DEPS#newcode50 extensions/DEPS:50: "+chrome/browser/extensions/extension_with_management_policy_apitest.h", On 2017/03/29 21:36:50, Devlin wrote: > Why do we need this? We need to use this in tests, isn't this necessary to do so? https://codereview.chromium.org/2499493004/diff/100001/extensions/common/exte... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/2499493004/diff/100001/extensions/common/exte... extensions/common/extension_messages.h:306: // Contians URLPatternSets defining which URLs an extension may not interact On 2017/03/29 21:36:50, Devlin wrote: > s/Contians/Contains Done. https://codereview.chromium.org/2499493004/diff/100001/extensions/common/perm... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/2499493004/diff/100001/extensions/common/perm... extensions/common/permissions/permissions_data.h:105: const bool is_default_runtime_blocked_allowed_hosts) const; On 2017/03/29 21:36:50, Devlin wrote: > To me, it seems like this shouldn't take three parameters, and instead should be > two functions: > SetPolicyHostRestrictions( > const URLPatternSet& runtime_blocked_hosts, > const URLPatternSet& runtime_allowed_hosts); > SetUsesDefaultHostRestrictions(bool uses_default_restrictions); > > Otherwise, what do we pass in the blocked/allowed hosts when uses default is > true? If we pass in the defaults, then it results in massive memory waste - > each extension storing a copy of the defaults - and makes it easier to lead to > bugs (if we update the defaults, we'd need to also update extensions). If we > pass in empty sets, then we didn't need to pass in anything. We should prefer > one of the two routes - pass in the hosts if they are specified, or else just > use the defaults. Done. https://codereview.chromium.org/2499493004/diff/100001/extensions/common/perm... extensions/common/permissions/permissions_data.h:109: // per-extnsion basis with SetPolicyHostRestrictions. On 2017/03/29 21:36:50, Devlin wrote: > per-extension Done. https://codereview.chromium.org/2499493004/diff/100001/extensions/renderer/di... File extensions/renderer/dispatcher.h (right): https://codereview.chromium.org/2499493004/diff/100001/extensions/renderer/di... extensions/renderer/dispatcher.h:196: void OnUpdatePolicyHostRestrictions( On 2017/03/29 21:36:50, Devlin wrote: > This method doesn't appear to exist? Done. Forgot to remove after we folded this into OnUpdatePermissions.
(just responding, will take another look tomorrow) https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:302: content::NotificationService::current()->Notify( On 2017/03/30 00:06:05, nrpeter wrote: > On 2017/03/29 21:36:49, Devlin wrote: > > On 2017/03/22 23:47:39, nrpeter wrote: > > > On 2017/02/14 23:17:10, Devlin wrote: > > > > We should still notify the rest of chrome when we change permissions. > > > > > > For privacy reasons, we avoid disclosing the list of sites that were added > or > > > removed from this list. We could send a blank message to Chrome saying > > > permissions were updated, but not including what was updated. I'm not sure > how > > > helpful that would be though... thoughts? > > > > ... I'm confused. Chrome is responsible for enforcing these permissions, and > > trivially and requisitely knows what they are. Any object in chrome that > > *really* wanted to know could, hypothetically, continuously poll > > ExtensionManagement and look for changes. What exactly are we trying to > > prevent? > > I'm not worried about another part of Chrome viewing this list. We'd like to > avoid disclosing the list of URLPatterns to extensions via permissions.onAdded > and permissions.onRemoved. IIUC, those events are triggered by this code, is > that not the case? Nope, events are dispatched from the DispatchEvent() method. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/content_script_apitest.cc:472: #define MAYBE_ContentScriptPermissionsApi DISABLED_ContentScriptPermissionsApi On 2017/03/30 00:06:05, nrpeter wrote: > On 2017/03/29 21:36:49, Devlin wrote: > > We probably shouldn't be relying on a test that's disabled on windows. > > Looks like this was disabled ~4 years ago on Windows for flakiness. It could be > that this is no longer flaky. Could we mark this from DISABLED_ to FLAKY_ and > gather a few days worth of data? Or is there another way of determining > flakiness? We should probably have permission tests on Windows anyways. We don't have FLAKY_ tests on chrome; only disabled or enabled. We can try flipping this to enabled on windows in a separate CL, but it would be good to try running it locally first on a windows machine to see if it consistently passes. Another option is adding another browser test. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_with_management_policy_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.cc:14: void ExtensionApiTestWithManagementPolicy::SetUpInProcessBrowserTestFixture() { On 2017/03/30 00:06:05, nrpeter wrote: > On 2017/03/29 21:36:49, Devlin wrote: > > Why does this need to be done in SetUpInProcessBrowserTestFixture() instead of > > SetUpOnMainThread()? > > This was moved from permissions_apitest.cc since we are adding additional > capabilities. I just copied what was there since it works. Let's try moving it to SetUpOnMainThread(), which is probably the better place for it. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_with_management_policy_apitest.h (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.h:27: bool BrowsedTo(const char* test_host); On 2017/03/30 00:06:06, nrpeter wrote: > On 2017/03/29 21:36:50, Devlin wrote: > > Are these methods needed? > > Since I removed the redundant test they are no longer used in this CL. They are > however used in the next 5 CLs that use this as a diffbase. I can remove this > and put it in the later CLs if necessary. It's usually preferable to only add stuff as we need it, so saving it for another CL would be nice, if doable. It ensures that, if those CLs change, we don't accidentally leave this unused method in. https://codereview.chromium.org/2499493004/diff/100001/extensions/DEPS File extensions/DEPS (right): https://codereview.chromium.org/2499493004/diff/100001/extensions/DEPS#newcode50 extensions/DEPS:50: "+chrome/browser/extensions/extension_with_management_policy_apitest.h", On 2017/03/30 00:06:06, nrpeter wrote: > On 2017/03/29 21:36:50, Devlin wrote: > > Why do we need this? > > We need to use this in tests, isn't this necessary to do so? Only if the tests are in the //extensions layer, which it doesn't seem like any of these are.
It'd also be good to add a few simple unittests to permissions data and permissions updater. https://codereview.chromium.org/2499493004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/2499493004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.h:77: bool is_default); As mentioned here [1], we should either set explicit hosts *or* use defaults. [1] https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... https://codereview.chromium.org/2499493004/diff/120001/extensions/common/exte... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/2499493004/diff/120001/extensions/common/exte... extensions/common/extension_messages.h:312: bool is_default_policy_blocked_allowed_hosts; s/is_default/uses_default https://codereview.chromium.org/2499493004/diff/120001/extensions/common/perm... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/2499493004/diff/120001/extensions/common/perm... extensions/common/permissions/permissions_data.h:315: mutable URLPatternSet policy_blocked_hosts_unsafe; add trailing _
https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/80001/chrome/browser/extensio... chrome/browser/extensions/permissions_updater.cc:302: content::NotificationService::current()->Notify( On 2017/03/30 00:33:52, Devlin wrote: > On 2017/03/30 00:06:05, nrpeter wrote: > > On 2017/03/29 21:36:49, Devlin wrote: > > > On 2017/03/22 23:47:39, nrpeter wrote: > > > > On 2017/02/14 23:17:10, Devlin wrote: > > > > > We should still notify the rest of chrome when we change permissions. > > > > > > > > For privacy reasons, we avoid disclosing the list of sites that were added > > or > > > > removed from this list. We could send a blank message to Chrome saying > > > > permissions were updated, but not including what was updated. I'm not sure > > how > > > > helpful that would be though... thoughts? > > > > > > ... I'm confused. Chrome is responsible for enforcing these permissions, > and > > > trivially and requisitely knows what they are. Any object in chrome that > > > *really* wanted to know could, hypothetically, continuously poll > > > ExtensionManagement and look for changes. What exactly are we trying to > > > prevent? > > > > I'm not worried about another part of Chrome viewing this list. We'd like to > > avoid disclosing the list of URLPatterns to extensions via permissions.onAdded > > and permissions.onRemoved. IIUC, those events are triggered by this code, is > > that not the case? > > Nope, events are dispatched from the DispatchEvent() method. Done. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/content_script_apitest.cc:472: #define MAYBE_ContentScriptPermissionsApi DISABLED_ContentScriptPermissionsApi On 2017/03/30 00:33:53, Devlin wrote: > On 2017/03/30 00:06:05, nrpeter wrote: > > On 2017/03/29 21:36:49, Devlin wrote: > > > We probably shouldn't be relying on a test that's disabled on windows. > > > > Looks like this was disabled ~4 years ago on Windows for flakiness. It could > be > > that this is no longer flaky. Could we mark this from DISABLED_ to FLAKY_ and > > gather a few days worth of data? Or is there another way of determining > > flakiness? We should probably have permission tests on Windows anyways. > > We don't have FLAKY_ tests on chrome; only disabled or enabled. We can try > flipping this to enabled on windows in a separate CL, but it would be good to > try running it locally first on a windows machine to see if it consistently > passes. Another option is adding another browser test. I opted for the second option, creating another browser test. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_with_management_policy_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.cc:14: void ExtensionApiTestWithManagementPolicy::SetUpInProcessBrowserTestFixture() { On 2017/03/30 00:33:53, Devlin wrote: > On 2017/03/30 00:06:05, nrpeter wrote: > > On 2017/03/29 21:36:49, Devlin wrote: > > > Why does this need to be done in SetUpInProcessBrowserTestFixture() instead > of > > > SetUpOnMainThread()? > > > > This was moved from permissions_apitest.cc since we are adding additional > > capabilities. I just copied what was there since it works. > > Let's try moving it to SetUpOnMainThread(), which is probably the better place > for it. I tried moving this, but the policy connector isn't created by the time I need to set a policy service. [6560:6560:0331/142415:FATAL:browser_policy_connector_base.cc(119)] Check failed: !g_created_policy_service. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_with_management_policy_apitest.h (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.h:27: bool BrowsedTo(const char* test_host); On 2017/03/30 00:33:53, Devlin wrote: > On 2017/03/30 00:06:06, nrpeter wrote: > > On 2017/03/29 21:36:50, Devlin wrote: > > > Are these methods needed? > > > > Since I removed the redundant test they are no longer used in this CL. They > are > > however used in the next 5 CLs that use this as a diffbase. I can remove this > > and put it in the later CLs if necessary. > > It's usually preferable to only add stuff as we need it, so saving it for > another CL would be nice, if doable. It ensures that, if those CLs change, we > don't accidentally leave this unused method in. Done. https://codereview.chromium.org/2499493004/diff/100001/extensions/DEPS File extensions/DEPS (right): https://codereview.chromium.org/2499493004/diff/100001/extensions/DEPS#newcode50 extensions/DEPS:50: "+chrome/browser/extensions/extension_with_management_policy_apitest.h", On 2017/03/30 00:33:53, Devlin wrote: > On 2017/03/30 00:06:06, nrpeter wrote: > > On 2017/03/29 21:36:50, Devlin wrote: > > > Why do we need this? > > > > We need to use this in tests, isn't this necessary to do so? > > Only if the tests are in the //extensions layer, which it doesn't seem like any > of these are. Done. https://codereview.chromium.org/2499493004/diff/120001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/2499493004/diff/120001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.h:77: bool is_default); On 2017/03/30 21:07:27, Devlin wrote: > As mentioned here [1], we should either set explicit hosts *or* use defaults. > > [1] > https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... Done. https://codereview.chromium.org/2499493004/diff/120001/extensions/common/exte... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/2499493004/diff/120001/extensions/common/exte... extensions/common/extension_messages.h:312: bool is_default_policy_blocked_allowed_hosts; On 2017/03/30 21:07:27, Devlin wrote: > s/is_default/uses_default Done. https://codereview.chromium.org/2499493004/diff/120001/extensions/common/perm... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/2499493004/diff/120001/extensions/common/perm... extensions/common/permissions/permissions_data.h:315: mutable URLPatternSet policy_blocked_hosts_unsafe; On 2017/03/30 21:07:27, Devlin wrote: > add trailing _ Done.
ipc lg but a few questions that I had when I was trying to understand how the pieces fit together https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... chrome/browser/extensions/content_script_apitest.cc:16: #include "chrome/browser/extensions/extension_management_constants.h" I'm not sure this is used. https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... chrome/browser/extensions/content_script_apitest.cc:28: #include "components/policy/core/common/mock_configuration_policy_provider.h" Ditto https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:325: extension->permissions_data()->policy_allowed_hosts(); This part makes it seem either-or. https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.h:81: bool uses_default_restrictions); Out of curiosity, is this supposed to be mutually exclusive with SetPolicyHostRestrictions? I'm kind of wondering if this should be: SetPolicyHostRestrictions(const Extension*, ...) and SetUseDefaultHostRestructions(const Extension*) and the latter just clears the overridden list as well. https://codereview.chromium.org/2499493004/diff/160001/extensions/common/perm... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/160001/extensions/common/perm... extensions/common/permissions/permissions_data.cc:190: policy_blocked_hosts_unsafe_ = runtime_blocked_hosts; I'm having trouble understanding why it's sometimes OK to not grab the lock, but sometimes it's important to grab it.
https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... chrome/browser/extensions/content_script_apitest.cc:28: #include "components/policy/core/common/mock_configuration_policy_provider.h" On 2017/03/31 23:48:05, dcheng wrote: > Ditto Done. https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:325: extension->permissions_data()->policy_allowed_hosts(); On 2017/03/31 23:48:06, dcheng wrote: > This part makes it seem either-or. Yes, an extension either uses the default, or provides its own list of blocked and allowed hosts. If the extension uses a default policy, we don't need to send an individual policy. Either way, we need to signal if its using the default, otherwise it could have been previously set to individual. https://codereview.chromium.org/2499493004/diff/160001/extensions/common/perm... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/160001/extensions/common/perm... extensions/common/permissions/permissions_data.cc:190: policy_blocked_hosts_unsafe_ = runtime_blocked_hosts; On 2017/03/31 23:48:06, dcheng wrote: > I'm having trouble understanding why it's sometimes OK to not grab the lock, but > sometimes it's important to grab it. Good point, added the check to SetUsesDefaultHostRestrictions. runtime_lock_ is part of the class and not available to static methods like SetDefaultPolicyHostRestrictions. Should I create a second static lock for setting / reading the default host restrictions?
https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:325: extension->permissions_data()->policy_allowed_hosts(); On 2017/04/01 00:19:02, nrpeter wrote: > On 2017/03/31 23:48:06, dcheng wrote: > > This part makes it seem either-or. > > Yes, an extension either uses the default, or provides its own list of blocked > and allowed hosts. > > If the extension uses a default policy, we don't need to send an individual > policy. Either way, we need to signal if its using the default, otherwise it > could have been previously set to individual. Right, that's fine: what I'm saying is that calling SetPolicyHostRestrictions() should implicitly set "use default host restrictions" to false. And thus, SetUsesDefaultHostRestrictions() just unconditionally sets "use default host restrictions" to true, and doesn't need the bool param. https://codereview.chromium.org/2499493004/diff/160001/extensions/common/perm... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/160001/extensions/common/perm... extensions/common/permissions/permissions_data.cc:190: policy_blocked_hosts_unsafe_ = runtime_blocked_hosts; On 2017/04/01 00:19:02, nrpeter wrote: > On 2017/03/31 23:48:06, dcheng wrote: > > I'm having trouble understanding why it's sometimes OK to not grab the lock, > but > > sometimes it's important to grab it. > > Good point, added the check to SetUsesDefaultHostRestrictions. > > runtime_lock_ is part of the class and not available to static methods like > SetDefaultPolicyHostRestrictions. Should I create a second static lock for > setting / reading the default host restrictions? I don't really know, but my question is more about PermissionsData::policy_allowed_hosts(), etc: we return a reference to a URLPatternSet, but access doesn't appear to be protected. I'm wondering why this is OK.
https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.h (right): https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.h:81: bool uses_default_restrictions); On 2017/03/31 23:48:06, dcheng wrote: > Out of curiosity, is this supposed to be mutually exclusive with > SetPolicyHostRestrictions? > > I'm kind of wondering if this should be: > > SetPolicyHostRestrictions(const Extension*, ...) > > and > > SetUseDefaultHostRestructions(const Extension*) > > and the latter just clears the overridden list as well. The default policy applies to all extensions by default. e.g. block all extensions from accessing *.google.com The default policy can be overridden per-extension e.g. lpcaedmchfhocbbapmcbpinfpgnhiddi CAN access *.google.com In an effort to not send redundant data, we send the default policy once (SetDefaultPolicyHostRestrictions) per renderer. If an extension defines its own policy to override the default, we need to indicate this (SetUsesDefaultHostRestrictions). Keep in mind, policies can change over time, so an individual policy can be removed and the extension must revert back to the default policy. If we've indicated an extension specific policy, we must send the details (SetPolicyHostRestrictions)
On 2017/04/01 00:28:28, nrpeter wrote: > https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... > File chrome/browser/extensions/permissions_updater.h (right): > > https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... > chrome/browser/extensions/permissions_updater.h:81: bool > uses_default_restrictions); > On 2017/03/31 23:48:06, dcheng wrote: > > Out of curiosity, is this supposed to be mutually exclusive with > > SetPolicyHostRestrictions? > > > > I'm kind of wondering if this should be: > > > > SetPolicyHostRestrictions(const Extension*, ...) > > > > and > > > > SetUseDefaultHostRestructions(const Extension*) > > > > and the latter just clears the overridden list as well. > > The default policy applies to all extensions by default. > e.g. block all extensions from accessing *.google.com > > The default policy can be overridden per-extension > e.g. lpcaedmchfhocbbapmcbpinfpgnhiddi CAN access *.google.com > > In an effort to not send redundant data, we send the default policy once > (SetDefaultPolicyHostRestrictions) per renderer. > > If an extension defines its own policy to override the default, we need to > indicate this (SetUsesDefaultHostRestrictions). Keep in mind, policies can > change over time, so an individual policy can be removed and the extension must > revert back to the default policy. > > If we've indicated an extension specific policy, we must send the details > (SetPolicyHostRestrictions) Right, I understand all this - I don't see how this is in conflict with what I said: the bool parameter seems redundant in the function calls. If we add per-extension overrides, we don't want to use the default policy host restrictions. If these are removed, then we call SetUseDefaultPolicyHostRestrictions(extension), which sets the bool back to true, and clears any per-extension hosts.
https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/content_script_apitest.cc (left): https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... chrome/browser/extensions/content_script_apitest.cc:473: extensions::PermissionsRequestFunction::SetAutoConfirmForTests(true); This will break this test... https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... chrome/browser/extensions/extension_service.cc:1223: extensions::PermissionsUpdater(profile()).SetUsesDefaultHostRestrictions( this should be in an else. As I've mentioned here [1], [2], [3], and related to dcheng's comment, we should call *either* a method to set explicit host restrictions *or* a method to set an extension to use the defaults; not both. [1] https://codereview.chromium.org/2499493004/diff/100001/extensions/common/perm... [2] https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... [3] https://codereview.chromium.org/2499493004/diff/120001/chrome/browser/extensi... https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (left): https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:247: if (changed.IsEmpty()) Why move the early return? https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:158: uses_default_restrictions); If we don't notify the renderer, how do we update extension permissions? (This also indicates there's a gap in our testing; please see my comment from March 30 requesting more unit tests) https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:360: params.default_policy_blocked_hosts = default_runtime_blocked_hosts; This performs extra copies (because I don't think the compiler is smart enough to implicitly move these into the IPC message). Prefer reusing the same struct, as we do in the other notification method. https://codereview.chromium.org/2499493004/diff/180001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2499493004/diff/180001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:385: "../browser/extensions/extension_with_management_policy_apitest.cc", Is this an interactive ui test? https://codereview.chromium.org/2499493004/diff/180001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:4950: "../browser/extensions/extension_with_management_policy_apitest.cc", Is this a performance browser test? https://codereview.chromium.org/2499493004/diff/180001/extensions/browser/ren... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2499493004/diff/180001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:22: #include "ipc/ipc_message_attachment_set.h" needed? https://codereview.chromium.org/2499493004/diff/180001/extensions/common/perm... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/180001/extensions/common/perm... extensions/common/permissions/permissions_data.cc:414: if (IsRuntimeBlockedHost(document_url)) { Isn't this checked as a part of IsRestrictedUrl below? https://codereview.chromium.org/2499493004/diff/180001/extensions/renderer/di... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2499493004/diff/180001/extensions/renderer/di... extensions/renderer/dispatcher.cc:1088: extension->permissions_data()->SetUsesDefaultHostRestrictions( see comment in extension_service.cc https://codereview.chromium.org/2499493004/diff/180001/extensions/renderer/di... extensions/renderer/dispatcher.cc:1227: extension->permissions_data()->SetUsesDefaultHostRestrictions( see comment in extension_service.cc
https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_with_management_policy_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.cc:14: void ExtensionApiTestWithManagementPolicy::SetUpInProcessBrowserTestFixture() { On 2017/03/31 21:43:35, nrpeter wrote: > On 2017/03/30 00:33:53, Devlin wrote: > > On 2017/03/30 00:06:05, nrpeter wrote: > > > On 2017/03/29 21:36:49, Devlin wrote: > > > > Why does this need to be done in SetUpInProcessBrowserTestFixture() > instead > > of > > > > SetUpOnMainThread()? > > > > > > This was moved from permissions_apitest.cc since we are adding additional > > > capabilities. I just copied what was there since it works. > > > > Let's try moving it to SetUpOnMainThread(), which is probably the better place > > for it. > > I tried moving this, but the policy connector isn't created by the time I need > to set a policy service. > [6560:6560:0331/142415:FATAL:browser_policy_connector_base.cc(119)] Check > failed: !g_created_policy_service. Did you put the call before or after the call to ExtensionApiTest::SetUpOnMainThread()?
On 2017/04/03 15:58:21, Devlin wrote: > https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... > File chrome/browser/extensions/extension_with_management_policy_apitest.cc > (right): > > https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... > chrome/browser/extensions/extension_with_management_policy_apitest.cc:14: void > ExtensionApiTestWithManagementPolicy::SetUpInProcessBrowserTestFixture() { > On 2017/03/31 21:43:35, nrpeter wrote: > > On 2017/03/30 00:33:53, Devlin wrote: > > > On 2017/03/30 00:06:05, nrpeter wrote: > > > > On 2017/03/29 21:36:49, Devlin wrote: > > > > > Why does this need to be done in SetUpInProcessBrowserTestFixture() > > instead > > > of > > > > > SetUpOnMainThread()? > > > > > > > > This was moved from permissions_apitest.cc since we are adding additional > > > > capabilities. I just copied what was there since it works. > > > > > > Let's try moving it to SetUpOnMainThread(), which is probably the better > place > > > for it. > > > > I tried moving this, but the policy connector isn't created by the time I need > > to set a policy service. > > [6560:6560:0331/142415:FATAL:browser_policy_connector_base.cc(119)] Check > > failed: !g_created_policy_service. > > Did you put the call before or after the call to > ExtensionApiTest::SetUpOnMainThread()? After the call to SetUpOnMainThread.
On 2017/04/01 00:24:07, dcheng wrote: > https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... > File chrome/browser/extensions/permissions_updater.cc (right): > > https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... > chrome/browser/extensions/permissions_updater.cc:325: > extension->permissions_data()->policy_allowed_hosts(); > On 2017/04/01 00:19:02, nrpeter wrote: > > On 2017/03/31 23:48:06, dcheng wrote: > > > This part makes it seem either-or. > > > > Yes, an extension either uses the default, or provides its own list of blocked > > and allowed hosts. > > > > If the extension uses a default policy, we don't need to send an individual > > policy. Either way, we need to signal if its using the default, otherwise it > > could have been previously set to individual. > > Right, that's fine: what I'm saying is that calling SetPolicyHostRestrictions() > should implicitly set "use default host restrictions" to false. > > And thus, SetUsesDefaultHostRestrictions() just unconditionally sets "use > default host restrictions" to true, and doesn't need the bool param. > > https://codereview.chromium.org/2499493004/diff/160001/extensions/common/perm... > File extensions/common/permissions/permissions_data.cc (right): > > https://codereview.chromium.org/2499493004/diff/160001/extensions/common/perm... > extensions/common/permissions/permissions_data.cc:190: > policy_blocked_hosts_unsafe_ = runtime_blocked_hosts; > On 2017/04/01 00:19:02, nrpeter wrote: > > On 2017/03/31 23:48:06, dcheng wrote: > > > I'm having trouble understanding why it's sometimes OK to not grab the lock, > > but > > > sometimes it's important to grab it. > > > > Good point, added the check to SetUsesDefaultHostRestrictions. > > > > runtime_lock_ is part of the class and not available to static methods like > > SetDefaultPolicyHostRestrictions. Should I create a second static lock for > > setting / reading the default host restrictions? > > I don't really know, but my question is more about > PermissionsData::policy_allowed_hosts(), etc: we return a reference to a > URLPatternSet, but access doesn't appear to be protected. I'm wondering why this > is OK. I was copying active_permissions() from permissions_data.h. Both are used to populate extension data from outside permissions_data and return references. Should I made the reference returning version private, and create a public version that returns a non-referenced URLPatternSet?
https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_with_management_policy_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.cc:14: void ExtensionApiTestWithManagementPolicy::SetUpInProcessBrowserTestFixture() { On 2017/04/03 15:58:21, Devlin wrote: > On 2017/03/31 21:43:35, nrpeter wrote: > > On 2017/03/30 00:33:53, Devlin wrote: > > > On 2017/03/30 00:06:05, nrpeter wrote: > > > > On 2017/03/29 21:36:49, Devlin wrote: > > > > > Why does this need to be done in SetUpInProcessBrowserTestFixture() > > instead > > > of > > > > > SetUpOnMainThread()? > > > > > > > > This was moved from permissions_apitest.cc since we are adding additional > > > > capabilities. I just copied what was there since it works. > > > > > > Let's try moving it to SetUpOnMainThread(), which is probably the better > place > > > for it. > > > > I tried moving this, but the policy connector isn't created by the time I need > > to set a policy service. > > [6560:6560:0331/142415:FATAL:browser_policy_connector_base.cc(119)] Check > > failed: !g_created_policy_service. > > Did you put the call before or after the call to > ExtensionApiTest::SetUpOnMainThread()? I put the call after. https://codereview.chromium.org/2499493004/diff/160001/extensions/common/perm... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/160001/extensions/common/perm... extensions/common/permissions/permissions_data.cc:190: policy_blocked_hosts_unsafe_ = runtime_blocked_hosts; On 2017/04/01 00:24:07, dcheng wrote: > On 2017/04/01 00:19:02, nrpeter wrote: > > On 2017/03/31 23:48:06, dcheng wrote: > > > I'm having trouble understanding why it's sometimes OK to not grab the lock, > > but > > > sometimes it's important to grab it. > > > > Good point, added the check to SetUsesDefaultHostRestrictions. > > > > runtime_lock_ is part of the class and not available to static methods like > > SetDefaultPolicyHostRestrictions. Should I create a second static lock for > > setting / reading the default host restrictions? > > I don't really know, but my question is more about > PermissionsData::policy_allowed_hosts(), etc: we return a reference to a > URLPatternSet, but access doesn't appear to be protected. I'm wondering why this > is OK. I was copying active_permissions() from permissions_data.h which also returns a reference and is used to populate permissions data from the browser process to renderers. https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/content_script_apitest.cc (left): https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... chrome/browser/extensions/content_script_apitest.cc:473: extensions::PermissionsRequestFunction::SetAutoConfirmForTests(true); On 2017/04/03 15:52:23, Devlin wrote: > This will break this test... Done. https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... chrome/browser/extensions/extension_service.cc:1223: extensions::PermissionsUpdater(profile()).SetUsesDefaultHostRestrictions( On 2017/04/03 15:52:23, Devlin wrote: > this should be in an else. As I've mentioned here [1], [2], [3], and related to > dcheng's comment, we should call *either* a method to set explicit host > restrictions *or* a method to set an extension to use the defaults; not both. > > [1] > https://codereview.chromium.org/2499493004/diff/100001/extensions/common/perm... > [2] > https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... > [3] > https://codereview.chromium.org/2499493004/diff/120001/chrome/browser/extensi... Done. https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (left): https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:247: if (changed.IsEmpty()) On 2017/04/03 15:52:23, Devlin wrote: > Why move the early return? Done. https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:158: uses_default_restrictions); On 2017/04/03 15:52:23, Devlin wrote: > If we don't notify the renderer, how do we update extension permissions? > > (This also indicates there's a gap in our testing; please see my comment from > March 30 requesting more unit tests) Fixed this issue, but I hear the need for more unit tests. I'll start working on those, but want to get these fixes addressed first. https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:360: params.default_policy_blocked_hosts = default_runtime_blocked_hosts; On 2017/04/03 15:52:23, Devlin wrote: > This performs extra copies (because I don't think the compiler is smart enough > to implicitly move these into the IPC message). Prefer reusing the same struct, > as we do in the other notification method. Done. https://codereview.chromium.org/2499493004/diff/180001/chrome/test/BUILD.gn File chrome/test/BUILD.gn (right): https://codereview.chromium.org/2499493004/diff/180001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:385: "../browser/extensions/extension_with_management_policy_apitest.cc", On 2017/04/03 15:52:23, Devlin wrote: > Is this an interactive ui test? Was attempting to offer this everywhere extension_apitest was. Since we don't have interactive_ui tests using this yet (later CLs) I'll remove it. https://codereview.chromium.org/2499493004/diff/180001/chrome/test/BUILD.gn#n... chrome/test/BUILD.gn:4950: "../browser/extensions/extension_with_management_policy_apitest.cc", On 2017/04/03 15:52:23, Devlin wrote: > Is this a performance browser test? Was attempting to offer this everywhere extension_apitest was. Since we don't have performance tests using this yet (later CLs) I'll remove it. https://codereview.chromium.org/2499493004/diff/180001/extensions/browser/ren... File extensions/browser/renderer_startup_helper.cc (right): https://codereview.chromium.org/2499493004/diff/180001/extensions/browser/ren... extensions/browser/renderer_startup_helper.cc:22: #include "ipc/ipc_message_attachment_set.h" On 2017/04/03 15:52:23, Devlin wrote: > needed? Done. https://codereview.chromium.org/2499493004/diff/180001/extensions/common/perm... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/180001/extensions/common/perm... extensions/common/permissions/permissions_data.cc:414: if (IsRuntimeBlockedHost(document_url)) { On 2017/04/03 15:52:23, Devlin wrote: > Isn't this checked as a part of IsRestrictedUrl below? Done. https://codereview.chromium.org/2499493004/diff/180001/extensions/renderer/di... File extensions/renderer/dispatcher.cc (right): https://codereview.chromium.org/2499493004/diff/180001/extensions/renderer/di... extensions/renderer/dispatcher.cc:1088: extension->permissions_data()->SetUsesDefaultHostRestrictions( On 2017/04/03 15:52:23, Devlin wrote: > see comment in extension_service.cc Done. https://codereview.chromium.org/2499493004/diff/180001/extensions/renderer/di... extensions/renderer/dispatcher.cc:1227: extension->permissions_data()->SetUsesDefaultHostRestrictions( On 2017/04/03 15:52:23, Devlin wrote: > see comment in extension_service.cc Done.
Hey Nick, I made it part way through another round of reviews here, but it looks like some of the comments you marked as done weren't actually done. Can you please make sure that all comments are addressed (either by performing the requested action or by asking a question if more information is needed), and reping when the patch set is ready? https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_with_management_policy_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.cc:14: void ExtensionApiTestWithManagementPolicy::SetUpInProcessBrowserTestFixture() { On 2017/04/03 22:35:48, nrpeter wrote: > On 2017/04/03 15:58:21, Devlin wrote: > > On 2017/03/31 21:43:35, nrpeter wrote: > > > On 2017/03/30 00:33:53, Devlin wrote: > > > > On 2017/03/30 00:06:05, nrpeter wrote: > > > > > On 2017/03/29 21:36:49, Devlin wrote: > > > > > > Why does this need to be done in SetUpInProcessBrowserTestFixture() > > > instead > > > > of > > > > > > SetUpOnMainThread()? > > > > > > > > > > This was moved from permissions_apitest.cc since we are adding > additional > > > > > capabilities. I just copied what was there since it works. > > > > > > > > Let's try moving it to SetUpOnMainThread(), which is probably the better > > place > > > > for it. > > > > > > I tried moving this, but the policy connector isn't created by the time I > need > > > to set a policy service. > > > [6560:6560:0331/142415:FATAL:browser_policy_connector_base.cc(119)] Check > > > failed: !g_created_policy_service. > > > > Did you put the call before or after the call to > > ExtensionApiTest::SetUpOnMainThread()? > > I put the call after. Try putting it before. https://codereview.chromium.org/2499493004/diff/160001/extensions/common/perm... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/160001/extensions/common/perm... extensions/common/permissions/permissions_data.cc:190: policy_blocked_hosts_unsafe_ = runtime_blocked_hosts; On 2017/04/03 22:35:48, nrpeter wrote: > On 2017/04/01 00:24:07, dcheng wrote: > > On 2017/04/01 00:19:02, nrpeter wrote: > > > On 2017/03/31 23:48:06, dcheng wrote: > > > > I'm having trouble understanding why it's sometimes OK to not grab the > lock, > > > but > > > > sometimes it's important to grab it. > > > > > > Good point, added the check to SetUsesDefaultHostRestrictions. > > > > > > runtime_lock_ is part of the class and not available to static methods like > > > SetDefaultPolicyHostRestrictions. Should I create a second static lock for > > > setting / reading the default host restrictions? > > > > I don't really know, but my question is more about > > PermissionsData::policy_allowed_hosts(), etc: we return a reference to a > > URLPatternSet, but access doesn't appear to be protected. I'm wondering why > this > > is OK. > > I was copying active_permissions() from permissions_data.h which also returns a > reference and is used to populate permissions data from the browser process to > renderers. It's only safe to grab them directly (using the *unsafe_ members) when we've already grabbed the lock. e.g., CanRunOnPage() is only ever called from other functions which acquire the lock; therefore it's safe to use the members without first acquiring it. We should make sure that we're abiding by this (so we need to acquire the lock in SetUsesDefaultHostRestrictions, etc.) https://codereview.chromium.org/2499493004/diff/240001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/240001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:289: if (changed.IsEmpty() && event_type != POLICY) https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... This doesn't look done?
Wasn't sure where to put the error message, so I put it in constants. If you'd prefer a different location please let me know and I'll happily move it. Thanks for catching that mistake Devlin, I reviewed all the comments you and dcheng made in the past month and found no other comments marked "done" that were not. https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_with_management_policy_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.cc:14: void ExtensionApiTestWithManagementPolicy::SetUpInProcessBrowserTestFixture() { On 2017/04/04 16:29:10, Devlin wrote: > On 2017/04/03 22:35:48, nrpeter wrote: > > On 2017/04/03 15:58:21, Devlin wrote: > > > On 2017/03/31 21:43:35, nrpeter wrote: > > > > On 2017/03/30 00:33:53, Devlin wrote: > > > > > On 2017/03/30 00:06:05, nrpeter wrote: > > > > > > On 2017/03/29 21:36:49, Devlin wrote: > > > > > > > Why does this need to be done in SetUpInProcessBrowserTestFixture() > > > > instead > > > > > of > > > > > > > SetUpOnMainThread()? > > > > > > > > > > > > This was moved from permissions_apitest.cc since we are adding > > additional > > > > > > capabilities. I just copied what was there since it works. > > > > > > > > > > Let's try moving it to SetUpOnMainThread(), which is probably the better > > > place > > > > > for it. > > > > > > > > I tried moving this, but the policy connector isn't created by the time I > > need > > > > to set a policy service. > > > > [6560:6560:0331/142415:FATAL:browser_policy_connector_base.cc(119)] > Check > > > > failed: !g_created_policy_service. > > > > > > Did you put the call before or after the call to > > > ExtensionApiTest::SetUpOnMainThread()? > > > > I put the call after. > > Try putting it before. Tried putting it before, and I got the same error message. Any other ideas? If this is important I can find someone from the Chrome enterprise team to help me debug. https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... chrome/browser/extensions/content_script_apitest.cc:16: #include "chrome/browser/extensions/extension_management_constants.h" On 2017/03/31 23:48:06, dcheng wrote: > I'm not sure this is used. Done. https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/160001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:325: extension->permissions_data()->policy_allowed_hosts(); On 2017/04/01 00:24:07, dcheng wrote: > On 2017/04/01 00:19:02, nrpeter wrote: > > On 2017/03/31 23:48:06, dcheng wrote: > > > This part makes it seem either-or. > > > > Yes, an extension either uses the default, or provides its own list of blocked > > and allowed hosts. > > > > If the extension uses a default policy, we don't need to send an individual > > policy. Either way, we need to signal if its using the default, otherwise it > > could have been previously set to individual. > > Right, that's fine: what I'm saying is that calling SetPolicyHostRestrictions() > should implicitly set "use default host restrictions" to false. > > And thus, SetUsesDefaultHostRestrictions() just unconditionally sets "use > default host restrictions" to true, and doesn't need the bool param. I've SetPolicyHostRestrictions to automatically set whether it uses the default or not and updated UsesDefaultPolicyHostRestrictions as well. https://codereview.chromium.org/2499493004/diff/160001/extensions/common/perm... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/160001/extensions/common/perm... extensions/common/permissions/permissions_data.cc:190: policy_blocked_hosts_unsafe_ = runtime_blocked_hosts; On 2017/04/04 16:29:10, Devlin wrote: > On 2017/04/03 22:35:48, nrpeter wrote: > > On 2017/04/01 00:24:07, dcheng wrote: > > > On 2017/04/01 00:19:02, nrpeter wrote: > > > > On 2017/03/31 23:48:06, dcheng wrote: > > > > > I'm having trouble understanding why it's sometimes OK to not grab the > > lock, > > > > but > > > > > sometimes it's important to grab it. > > > > > > > > Good point, added the check to SetUsesDefaultHostRestrictions. > > > > > > > > runtime_lock_ is part of the class and not available to static methods > like > > > > SetDefaultPolicyHostRestrictions. Should I create a second static lock for > > > > setting / reading the default host restrictions? > > > > > > I don't really know, but my question is more about > > > PermissionsData::policy_allowed_hosts(), etc: we return a reference to a > > > URLPatternSet, but access doesn't appear to be protected. I'm wondering why > > this > > > is OK. > > > > I was copying active_permissions() from permissions_data.h which also returns > a > > reference and is used to populate permissions data from the browser process to > > renderers. > > It's only safe to grab them directly (using the *unsafe_ members) when we've > already grabbed the lock. e.g., CanRunOnPage() is only ever called from other > functions which acquire the lock; therefore it's safe to use the members without > first acquiring it. > > We should make sure that we're abiding by this (so we need to acquire the lock > in SetUsesDefaultHostRestrictions, etc.) FYI: We are acquiring the lock in SetUsesDefaultHostRestrictions in a later patchset https://codereview.chromium.org/2499493004/diff/240001/extensions/common/perm... https://codereview.chromium.org/2499493004/diff/240001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/240001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:289: if (changed.IsEmpty() && event_type != POLICY) On 2017/04/04 16:29:11, Devlin wrote: > https://codereview.chromium.org/2499493004/diff/180001/chrome/browser/extensi... > > This doesn't look done? Thanks for the reminder. I've moved it back to the original location. https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... File chrome/common/extensions/permissions/permissions_data_unittest.cc (right): https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:234: EXPECT_TRUE( I ran git cl format, and this happened.... Do you want me to revert these? https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:249: extension, SocketPermissionRequest::TCP_CONNECT, "www.example.com", 80)); Ditto https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:274: "239.255.255.250", 1900)); Ditto https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:628: LoadManifestStrict("script_and_capture", "extension_regular_all.json"); Ditto https://codereview.chromium.org/2499493004/diff/260001/extensions/common/perm... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/260001/extensions/common/perm... extensions/common/permissions/permissions_data.cc:398: return policy_blocked_hosts().MatchesURL(url) && Only place IsRuntimeBlockedHost is call is IsRestrictedUrl, which also calls active_permissions() which doesn't have an assertion check. Also IsRuntimeBlockedHost is private. Didn't need the check.
Nice! A bunch of nits, but most of these are pretty easy to resolve. Thanks for adding the tests! https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/extension_with_management_policy_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/100001/chrome/browser/extensi... chrome/browser/extensions/extension_with_management_policy_apitest.cc:14: void ExtensionApiTestWithManagementPolicy::SetUpInProcessBrowserTestFixture() { On 2017/04/05 23:13:26, nrpeter wrote: > On 2017/04/04 16:29:10, Devlin wrote: > > On 2017/04/03 22:35:48, nrpeter wrote: > > > On 2017/04/03 15:58:21, Devlin wrote: > > > > On 2017/03/31 21:43:35, nrpeter wrote: > > > > > On 2017/03/30 00:33:53, Devlin wrote: > > > > > > On 2017/03/30 00:06:05, nrpeter wrote: > > > > > > > On 2017/03/29 21:36:49, Devlin wrote: > > > > > > > > Why does this need to be done in > SetUpInProcessBrowserTestFixture() > > > > > instead > > > > > > of > > > > > > > > SetUpOnMainThread()? > > > > > > > > > > > > > > This was moved from permissions_apitest.cc since we are adding > > > additional > > > > > > > capabilities. I just copied what was there since it works. > > > > > > > > > > > > Let's try moving it to SetUpOnMainThread(), which is probably the > better > > > > place > > > > > > for it. > > > > > > > > > > I tried moving this, but the policy connector isn't created by the time > I > > > need > > > > > to set a policy service. > > > > > [6560:6560:0331/142415:FATAL:browser_policy_connector_base.cc(119)] > > Check > > > > > failed: !g_created_policy_service. > > > > > > > > Did you put the call before or after the call to > > > > ExtensionApiTest::SetUpOnMainThread()? > > > > > > I put the call after. > > > > Try putting it before. > > Tried putting it before, and I got the same error message. Any other ideas? If > this is important I can find someone from the Chrome enterprise team to help me > debug. Let's table it for now. I'd like to get to the bottom of it, but we don't need to hold up this CL on it. https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/content_script_apitest.cc:488: PermissionsRequestFunction::SetAutoConfirmForTests(true); needed? https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/content_script_apitest.cc:489: PermissionsRequestFunction::SetIgnoreUserGestureForTests(true); needed? https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/extension_management.h (right): https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/extension_management.h:126: // GetRuntimeBlockedHosts. This should only be used to initialize a new We can probably remove the bit about "this should only be used to initialize a new renderer" here - these are also used to initialize permissions data for extensions. https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/extension_service.cc:1227: extensions::PermissionsUpdater(profile()).SetDefaultPolicyHostRestrictions( nit: let's put this call first, so that existing renderers get the set of default policy hosts *before* messages that extensions use them. https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:143: // Keep track of runtime blocked and hosts for this extension in the browser nit: remove this comment. Trying to document how every other class will use permissions_data() is a losing battle. https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:165: // individual policy. We'll pull from here when a new renderer is created. ditto https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:338: // Trigger the onAdded and onRemoved events in the extension. Add comment: // We explicitly don't do this for policy-related events. https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater_unittest.cc (right): https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater_unittest.cc:369: { A scope that contains the whole test isn't very useful. :) https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater_unittest.cc:376: ListBuilder required_permissions; It'd probably be more interesting to construct an extension that could normally access a restricted site, e.g. one with *://*/*. https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater_unittest.cc:378: CreateExtensionWithOptionalPermissions(optional_permissions.Build(), these could be inlined, e.g. Create...(ListBuilder().Build() https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater_unittest.cc:393: EXPECT_FALSE(extension->permissions_data() These checks are testing permissions data, not permissions updater. They don't belong in this test. Instead, we should just be checking using PermissionsData::GetPageAccess() https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater_unittest.cc:492: // Set an empty individual policy, should not affect defualt policy. typo: default https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... File chrome/common/extensions/permissions/permissions_data_unittest.cc (right): https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:234: EXPECT_TRUE( On 2017/04/05 23:13:26, nrpeter wrote: > I ran git cl format, and this happened.... > > Do you want me to revert these? nah, git cl format changes are fine, as long as they're areas that your CL touches. https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:285: scoped_refptr<const Extension> policy_extension = Let's make new tests for this. Unit tests are cheap, and it ensures we don't accidentally lose test coverage by extending an existing case. https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:822: EXPECT_TRUE(CaptureOnly(extension.get(), https_url)); These variables (https_url, https_url_diff_subdomain) aren't very descriptive for this particular test case (which is more interested in hosts). Let's make new ones to make reading this easier. https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:830: // Individual poilcy example.google.com BLOCKED. typo: policy https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:835: // Tests that URL is allowed if same URL in blocked and allowed. Use proper grammar in comments, e.g. "Tests that an extension is allowed access to a given URL if that URL is in both the blocked and allowed sets of the policy." (Goes for everywhere) https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G... https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:930: // Having chrome://favicon/* should not give you chrome://* A lot of this test seems duplicative. Can we either a) isolate the interesting parts, or b) explain why we're retesting? https://codereview.chromium.org/2499493004/diff/260001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/content_scripts/policy/background.js (right): https://codereview.chromium.org/2499493004/diff/260001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/policy/background.js:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. no (c) https://codereview.chromium.org/2499493004/diff/260001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/policy/background.js:5: var assertEq = chrome.test.assertEq; needed? https://codereview.chromium.org/2499493004/diff/260001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/policy/background.js:6: var assertTrue = chrome.test.assertTrue; needed? https://codereview.chromium.org/2499493004/diff/260001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/policy/background.js:11: var testTabId; needed? https://codereview.chromium.org/2499493004/diff/260001/extensions/common/cons... File extensions/common/constants.h (right): https://codereview.chromium.org/2499493004/diff/260001/extensions/common/cons... extensions/common/constants.h:223: // Error message when enterprise policy blocks scripting of webpage comment needs punctuation (though it doesn't really belong here, see later comments). https://codereview.chromium.org/2499493004/diff/260001/extensions/common/perm... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/260001/extensions/common/perm... extensions/common/permissions/permissions_data.cc:37: }; nitty nit: \n https://codereview.chromium.org/2499493004/diff/260001/extensions/common/perm... extensions/common/permissions/permissions_data.cc:104: *error = extension_misc::kPolicyBlockedScripting; If this is only used here, let's just add it as a constant in the anonymous namespace of this file. https://codereview.chromium.org/2499493004/diff/260001/extensions/common/url_... File extensions/common/url_pattern.h (right): https://codereview.chromium.org/2499493004/diff/260001/extensions/common/url_... extensions/common/url_pattern.h:135: // If this is false host() would be google nitty nit: my orderly persona requires a ':' after both true and false in this comment (or neither) :) https://codereview.chromium.org/2499493004/diff/260001/extensions/common/url_... extensions/common/url_pattern.h:264: // default and only false if the the pattern's host ends with ".*" nit: This isn't false if the patterns host ends with ., but rather is only false if ALLOW_WILDCARD_FOR_EFFECTIVE_TLD is true. (It's only *applicable* if the pattern's host ends with .*, which is also good to mention!). Can you update this?
the ipc bits largely look fine. one comment wrt thread-safety and making it more obvious when things are safe though https://codereview.chromium.org/2499493004/diff/160001/extensions/common/perm... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/160001/extensions/common/perm... extensions/common/permissions/permissions_data.cc:190: policy_blocked_hosts_unsafe_ = runtime_blocked_hosts; On 2017/04/04 16:29:10, Devlin wrote: > On 2017/04/03 22:35:48, nrpeter wrote: > > On 2017/04/01 00:24:07, dcheng wrote: > > > On 2017/04/01 00:19:02, nrpeter wrote: > > > > On 2017/03/31 23:48:06, dcheng wrote: > > > > > I'm having trouble understanding why it's sometimes OK to not grab the > > lock, > > > > but > > > > > sometimes it's important to grab it. > > > > > > > > Good point, added the check to SetUsesDefaultHostRestrictions. > > > > > > > > runtime_lock_ is part of the class and not available to static methods > like > > > > SetDefaultPolicyHostRestrictions. Should I create a second static lock for > > > > setting / reading the default host restrictions? > > > > > > I don't really know, but my question is more about > > > PermissionsData::policy_allowed_hosts(), etc: we return a reference to a > > > URLPatternSet, but access doesn't appear to be protected. I'm wondering why > > this > > > is OK. > > > > I was copying active_permissions() from permissions_data.h which also returns > a > > reference and is used to populate permissions data from the browser process to > > renderers. > > It's only safe to grab them directly (using the *unsafe_ members) when we've > already grabbed the lock. e.g., CanRunOnPage() is only ever called from other > functions which acquire the lock; therefore it's safe to use the members without > first acquiring it. > > We should make sure that we're abiding by this (so we need to acquire the lock > in SetUsesDefaultHostRestrictions, etc.) Can we use runtime_lock_.AssertAquired() to make this clear?
https://codereview.chromium.org/2499493004/diff/160001/extensions/common/perm... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/160001/extensions/common/perm... extensions/common/permissions/permissions_data.cc:190: policy_blocked_hosts_unsafe_ = runtime_blocked_hosts; On 2017/04/07 06:57:19, dcheng wrote: > On 2017/04/04 16:29:10, Devlin wrote: > > On 2017/04/03 22:35:48, nrpeter wrote: > > > On 2017/04/01 00:24:07, dcheng wrote: > > > > On 2017/04/01 00:19:02, nrpeter wrote: > > > > > On 2017/03/31 23:48:06, dcheng wrote: > > > > > > I'm having trouble understanding why it's sometimes OK to not grab the > > > lock, > > > > > but > > > > > > sometimes it's important to grab it. > > > > > > > > > > Good point, added the check to SetUsesDefaultHostRestrictions. > > > > > > > > > > runtime_lock_ is part of the class and not available to static methods > > like > > > > > SetDefaultPolicyHostRestrictions. Should I create a second static lock > for > > > > > setting / reading the default host restrictions? > > > > > > > > I don't really know, but my question is more about > > > > PermissionsData::policy_allowed_hosts(), etc: we return a reference to a > > > > URLPatternSet, but access doesn't appear to be protected. I'm wondering > why > > > this > > > > is OK. > > > > > > I was copying active_permissions() from permissions_data.h which also > returns > > a > > > reference and is used to populate permissions data from the browser process > to > > > renderers. > > > > It's only safe to grab them directly (using the *unsafe_ members) when we've > > already grabbed the lock. e.g., CanRunOnPage() is only ever called from other > > functions which acquire the lock; therefore it's safe to use the members > without > > first acquiring it. > > > > We should make sure that we're abiding by this (so we need to acquire the lock > > in SetUsesDefaultHostRestrictions, etc.) > > Can we use runtime_lock_.AssertAquired() to make this clear? Added the lock assertion check as requested. https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/content_script_apitest.cc (right): https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/content_script_apitest.cc:488: PermissionsRequestFunction::SetAutoConfirmForTests(true); On 2017/04/07 00:40:26, Devlin wrote: > needed? Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/content_script_apitest.cc:489: PermissionsRequestFunction::SetIgnoreUserGestureForTests(true); On 2017/04/07 00:40:26, Devlin wrote: > needed? Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/extension_management.h (right): https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/extension_management.h:126: // GetRuntimeBlockedHosts. This should only be used to initialize a new On 2017/04/07 00:40:26, Devlin wrote: > We can probably remove the bit about "this should only be used to initialize a > new renderer" here - these are also used to initialize permissions data for > extensions. Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/extension_service.cc (right): https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/extension_service.cc:1227: extensions::PermissionsUpdater(profile()).SetDefaultPolicyHostRestrictions( On 2017/04/07 00:40:26, Devlin wrote: > nit: let's put this call first, so that existing renderers get the set of > default policy hosts *before* messages that extensions use them. Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:143: // Keep track of runtime blocked and hosts for this extension in the browser On 2017/04/07 00:40:26, Devlin wrote: > nit: remove this comment. Trying to document how every other class will use > permissions_data() is a losing battle. Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:165: // individual policy. We'll pull from here when a new renderer is created. On 2017/04/07 00:40:26, Devlin wrote: > ditto Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:338: // Trigger the onAdded and onRemoved events in the extension. On 2017/04/07 00:40:26, Devlin wrote: > Add comment: > // We explicitly don't do this for policy-related events. Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater_unittest.cc (right): https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater_unittest.cc:369: { On 2017/04/07 00:40:26, Devlin wrote: > A scope that contains the whole test isn't very useful. :) Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater_unittest.cc:376: ListBuilder required_permissions; On 2017/04/07 00:40:26, Devlin wrote: > It'd probably be more interesting to construct an extension that could normally > access a restricted site, e.g. one with *://*/*. Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater_unittest.cc:378: CreateExtensionWithOptionalPermissions(optional_permissions.Build(), On 2017/04/07 00:40:26, Devlin wrote: > these could be inlined, e.g. Create...(ListBuilder().Build() Since I added required permissions inlining may not make sense now. https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater_unittest.cc:393: EXPECT_FALSE(extension->permissions_data() On 2017/04/07 00:40:26, Devlin wrote: > These checks are testing permissions data, not permissions updater. They don't > belong in this test. Instead, we should just be checking using > PermissionsData::GetPageAccess() Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater_unittest.cc:492: // Set an empty individual policy, should not affect defualt policy. On 2017/04/07 00:40:26, Devlin wrote: > typo: default Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... File chrome/common/extensions/permissions/permissions_data_unittest.cc (right): https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:234: EXPECT_TRUE( On 2017/04/07 00:40:27, Devlin wrote: > On 2017/04/05 23:13:26, nrpeter wrote: > > I ran git cl format, and this happened.... > > > > Do you want me to revert these? > > nah, git cl format changes are fine, as long as they're areas that your CL > touches. Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:249: extension, SocketPermissionRequest::TCP_CONNECT, "www.example.com", 80)); On 2017/04/05 23:13:27, nrpeter wrote: > Ditto Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:274: "239.255.255.250", 1900)); On 2017/04/05 23:13:27, nrpeter wrote: > Ditto Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:285: scoped_refptr<const Extension> policy_extension = On 2017/04/07 00:40:27, Devlin wrote: > Let's make new tests for this. Unit tests are cheap, and it ensures we don't > accidentally lose test coverage by extending an existing case. Moved out of IsRestrictedUrl to this is no longer necessary. https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:628: LoadManifestStrict("script_and_capture", "extension_regular_all.json"); On 2017/04/05 23:13:27, nrpeter wrote: > Ditto Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:822: EXPECT_TRUE(CaptureOnly(extension.get(), https_url)); On 2017/04/07 00:40:26, Devlin wrote: > These variables (https_url, https_url_diff_subdomain) aren't very descriptive > for this particular test case (which is more interested in hosts). Let's make > new ones to make reading this easier. Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:830: // Individual poilcy example.google.com BLOCKED. On 2017/04/07 00:40:27, Devlin wrote: > typo: policy Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:830: // Individual poilcy example.google.com BLOCKED. On 2017/04/07 00:40:27, Devlin wrote: > typo: policy Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:835: // Tests that URL is allowed if same URL in blocked and allowed. On 2017/04/07 00:40:27, Devlin wrote: > Use proper grammar in comments, e.g. "Tests that an extension is allowed access > to a given URL if that URL is in both the blocked and allowed sets of the > policy." (Goes for everywhere) > > https://google.github.io/styleguide/cppguide.html#Punctuation,_Spelling_and_G... Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/common/extensio... chrome/common/extensions/permissions/permissions_data_unittest.cc:930: // Having chrome://favicon/* should not give you chrome://* On 2017/04/07 00:40:27, Devlin wrote: > A lot of this test seems duplicative. Can we either a) isolate the interesting > parts, or b) explain why we're retesting? Good point, all the tests after the component test aren't needed. https://codereview.chromium.org/2499493004/diff/260001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/content_scripts/policy/background.js (right): https://codereview.chromium.org/2499493004/diff/260001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/policy/background.js:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/04/07 00:40:27, Devlin wrote: > no (c) Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/policy/background.js:5: var assertEq = chrome.test.assertEq; On 2017/04/07 00:40:27, Devlin wrote: > needed? Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/policy/background.js:6: var assertTrue = chrome.test.assertTrue; On 2017/04/07 00:40:27, Devlin wrote: > needed? Done. https://codereview.chromium.org/2499493004/diff/260001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/policy/background.js:11: var testTabId; On 2017/04/07 00:40:27, Devlin wrote: > needed? Done. https://codereview.chromium.org/2499493004/diff/260001/extensions/common/cons... File extensions/common/constants.h (right): https://codereview.chromium.org/2499493004/diff/260001/extensions/common/cons... extensions/common/constants.h:223: // Error message when enterprise policy blocks scripting of webpage On 2017/04/07 00:40:27, Devlin wrote: > comment needs punctuation (though it doesn't really belong here, see later > comments). Fixed punctuation. Responded to the issue of location in later comments. https://codereview.chromium.org/2499493004/diff/260001/extensions/common/perm... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/260001/extensions/common/perm... extensions/common/permissions/permissions_data.cc:37: }; On 2017/04/07 00:40:27, Devlin wrote: > nitty nit: \n Done. https://codereview.chromium.org/2499493004/diff/260001/extensions/common/perm... extensions/common/permissions/permissions_data.cc:104: *error = extension_misc::kPolicyBlockedScripting; On 2017/04/07 00:40:27, Devlin wrote: > If this is only used here, let's just add it as a constant in the anonymous > namespace of this file. Its also used in the permissions_data_unittest. https://codereview.chromium.org/2499493004/diff/260001/extensions/common/url_... File extensions/common/url_pattern.h (right): https://codereview.chromium.org/2499493004/diff/260001/extensions/common/url_... extensions/common/url_pattern.h:135: // If this is false host() would be google On 2017/04/07 00:40:27, Devlin wrote: > nitty nit: my orderly persona requires a ':' after both true and false in this > comment (or neither) :) Done. https://codereview.chromium.org/2499493004/diff/260001/extensions/common/url_... extensions/common/url_pattern.h:264: // default and only false if the the pattern's host ends with ".*" On 2017/04/07 00:40:27, Devlin wrote: > nit: This isn't false if the patterns host ends with ., but rather is only false > if ALLOW_WILDCARD_FOR_EFFECTIVE_TLD is true. (It's only *applicable* if the > pattern's host ends with .*, which is also good to mention!). Can you update > this? Done.
IPC still lgtm (and thanks for adding the locking assertions) https://codereview.chromium.org/2499493004/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/280001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:297: CHECK_EQ(POLICY, event_type); Unrelated (since this is inherited from the past code) but why is this CHECK instead of, say, DCHECK? https://codereview.chromium.org/2499493004/diff/280001/extensions/common/perm... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/2499493004/diff/280001/extensions/common/perm... extensions/common/permissions/permissions_data.h:296: const URLPatternSet& policy_blocked_hosts_() const; Nit: this function needs a different name -- the difference between this and policy_blocked_hosts() is too subtle. (I suggested PolicyBlockedHostsUnsafe) https://codereview.chromium.org/2499493004/diff/280001/extensions/common/perm... extensions/common/permissions/permissions_data.h:299: const URLPatternSet& policy_allowed_hosts_() const; Ditto
I think this is good. Can you do a dry-run through the CQ and see if there any test failures that would change things, before I stamp? https://codereview.chromium.org/2499493004/diff/280001/chrome/test/data/exten... File chrome/test/data/extensions/api_test/content_scripts/policy/manifest.json (right): https://codereview.chromium.org/2499493004/diff/280001/chrome/test/data/exten... chrome/test/data/extensions/api_test/content_scripts/policy/manifest.json:6: "optional_permissions": ["http://*/*"], needed?
https://codereview.chromium.org/2499493004/diff/280001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/280001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:297: CHECK_EQ(POLICY, event_type); On 2017/04/13 22:07:40, dcheng wrote: > Unrelated (since this is inherited from the past code) but why is this CHECK > instead of, say, DCHECK? Done. https://codereview.chromium.org/2499493004/diff/280001/extensions/common/perm... File extensions/common/permissions/permissions_data.h (right): https://codereview.chromium.org/2499493004/diff/280001/extensions/common/perm... extensions/common/permissions/permissions_data.h:296: const URLPatternSet& policy_blocked_hosts_() const; On 2017/04/13 22:07:40, dcheng wrote: > Nit: this function needs a different name -- the difference between this and > policy_blocked_hosts() is too subtle. > > (I suggested PolicyBlockedHostsUnsafe) Done. https://codereview.chromium.org/2499493004/diff/280001/extensions/common/perm... extensions/common/permissions/permissions_data.h:299: const URLPatternSet& policy_allowed_hosts_() const; On 2017/04/13 22:07:40, dcheng wrote: > Ditto Done.
The CQ bit was checked by nrpeter@google.com 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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nrpeter@google.com 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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
The CQ bit was checked by nrpeter@google.com 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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nrpeter@google.com 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: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #20 (id:380001) has been deleted
Patchset #19 (id:360001) has been deleted
Patchset #18 (id:340001) has been deleted
The CQ bit was checked by nrpeter@google.com 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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #18 (id:400001) has been deleted
The CQ bit was checked by nrpeter@google.com to run a CQ dry run
Description was changed from ========== -Communicate which hosts are runtime blocked to all renderers -Blocks host permissions for extensions in accordance with ExtensionSettings policy -Tests via blocking content script injection -Introduces new test class for use with ExtensionSettings policy Incremental patch, relies on 2492013002 BUG=624649 ========== to ========== -Communicate which hosts are runtime blocked to all renderers -Blocks host permissions for extensions in accordance with ExtensionSettings policy -Tests via blocking content script injection -Introduces new test class for use with ExtensionSettings policy BUG=624649 ==========
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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Patchset #18 (id:420001) has been deleted
The CQ bit was checked by nrpeter@google.com 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: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by nrpeter@google.com 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: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
Patchset #18 (id:440001) has been deleted
Patchset #18 (id:460001) has been deleted
The CQ bit was checked by nrpeter@google.com 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.
All tests pass on CQ dry run. Are we good for LGTM?
lgtm https://codereview.chromium.org/2499493004/diff/480001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (left): https://codereview.chromium.org/2499493004/diff/480001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:259: DCHECK_EQ(0, init_flag_ & INIT_FLAG_TRANSIENT); this shouldn't change. https://codereview.chromium.org/2499493004/diff/480001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/480001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:358: DCHECK((init_flag_ & INIT_FLAG_TRANSIENT) == 0); nit: prefer DCHECK_EQ https://codereview.chromium.org/2499493004/diff/480001/extensions/common/perm... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/480001/extensions/common/perm... extensions/common/permissions/permissions_data.cc:422: if ((extension->location() != Manifest::COMPONENT) && nit: no parens
One other thing: your commit message is the description of the CL, so be sure to include the title. i.e., the description itself should be: <Title> <Description> BUG=<bug> (preferably each wrapped to ~72 chars)
Description was changed from ========== -Communicate which hosts are runtime blocked to all renderers -Blocks host permissions for extensions in accordance with ExtensionSettings policy -Tests via blocking content script injection -Introduces new test class for use with ExtensionSettings policy BUG=624649 ========== to ========== Communicate ExtensionSettings policy to renderers -Communicate which hosts are runtime blocked to all renderers -Blocks host permissions for extensions in accordance with ExtensionSettings policy -Tests via blocking content script injection -Introduces new test class for use with ExtensionSettings policy BUG=624649 ==========
Description was changed from ========== Communicate ExtensionSettings policy to renderers -Communicate which hosts are runtime blocked to all renderers -Blocks host permissions for extensions in accordance with ExtensionSettings policy -Tests via blocking content script injection -Introduces new test class for use with ExtensionSettings policy BUG=624649 ========== to ========== Communicate ExtensionSettings policy to renderers -Communicate which hosts are runtime blocked to all renderers -Blocks host permissions for specific hosts by specific extensions -Tests via blocking content script injection -Introduces new test class for use with ExtensionSettings policy BUG=624649 ==========
The CQ bit was checked by nrpeter@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org, rdevlin.cronin@chromium.org Link to the patchset: https://codereview.chromium.org/2499493004/#ps500001 (title: "nits")
https://codereview.chromium.org/2499493004/diff/480001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (left): https://codereview.chromium.org/2499493004/diff/480001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:259: DCHECK_EQ(0, init_flag_ & INIT_FLAG_TRANSIENT); On 2017/04/17 16:29:38, Devlin wrote: > this shouldn't change. Thanks, missed that during the merge. https://codereview.chromium.org/2499493004/diff/480001/chrome/browser/extensi... File chrome/browser/extensions/permissions_updater.cc (right): https://codereview.chromium.org/2499493004/diff/480001/chrome/browser/extensi... chrome/browser/extensions/permissions_updater.cc:358: DCHECK((init_flag_ & INIT_FLAG_TRANSIENT) == 0); On 2017/04/17 16:29:38, Devlin wrote: > nit: prefer DCHECK_EQ Done. https://codereview.chromium.org/2499493004/diff/480001/extensions/common/perm... File extensions/common/permissions/permissions_data.cc (right): https://codereview.chromium.org/2499493004/diff/480001/extensions/common/perm... extensions/common/permissions/permissions_data.cc:422: if ((extension->location() != Manifest::COMPONENT) && On 2017/04/17 16:29:38, Devlin wrote: > nit: no parens Done.
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": 500001, "attempt_start_ts": 1492449465750030,
"parent_rev": "bf32bc709c97515485b41f48c783dab5c5d0d30f", "commit_rev":
"c2f02148125c69bdce012802d9a467d725965a93"}
Message was sent while issue was closed.
Description was changed from ========== Communicate ExtensionSettings policy to renderers -Communicate which hosts are runtime blocked to all renderers -Blocks host permissions for specific hosts by specific extensions -Tests via blocking content script injection -Introduces new test class for use with ExtensionSettings policy BUG=624649 ========== to ========== Communicate ExtensionSettings policy to renderers -Communicate which hosts are runtime blocked to all renderers -Blocks host permissions for specific hosts by specific extensions -Tests via blocking content script injection -Introduces new test class for use with ExtensionSettings policy BUG=624649 Review-Url: https://codereview.chromium.org/2499493004 Cr-Commit-Position: refs/heads/master@{#464954} Committed: https://chromium.googlesource.com/chromium/src/+/c2f02148125c69bdce012802d9a4... ==========
Message was sent while issue was closed.
Committed patchset #19 (id:500001) as https://chromium.googlesource.com/chromium/src/+/c2f02148125c69bdce012802d9a4...
Message was sent while issue was closed.
vabr@chromium.org changed reviewers: + vabr@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/2499493004/diff/500001/extensions/common/exte... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/2499493004/diff/500001/extensions/common/exte... extensions/common/extension_messages.h:298: bool uses_default_policy_host_restrictions; Do the POD fields like uses_default_policy_host_restrictions and uses_default_policy_blocked_allowed_hosts below need an initial value, either here or in all constructors? Is that what MSan is complaining about in https://crbug.com/712641?
Message was sent while issue was closed.
A revert of this CL (patchset #19 id:500001) has been created in https://codereview.chromium.org/2820333003/ by vabr@chromium.org. The reason for reverting is: This likely broke MSan. Details in BUG=712641.
Message was sent while issue was closed.
https://codereview.chromium.org/2499493004/diff/500001/extensions/common/exte... File extensions/common/extension_messages.h (right): https://codereview.chromium.org/2499493004/diff/500001/extensions/common/exte... extensions/common/extension_messages.h:298: bool uses_default_policy_host_restrictions; On 2017/04/18 14:17:48, vabr (Chromium) wrote: > Do the POD fields like uses_default_policy_host_restrictions and > uses_default_policy_blocked_allowed_hosts below need an initial value, either > here or in all constructors? > > Is that what MSan is complaining about in https://crbug.com/712641 I think msan is complaining about the policy_blocked_hosts and policy_allowed_hosts. I only initialize those when use_default_policy_host_restrictions is true. On the receiving end, we also look at the boolean to see whether we will use the URLPatternSets. The issue is that policy_blocked_hosts and policy_allowed_hosts get written to the message regardless. I'll either have to conditionally write them to the message (yuck) or send empty URLPatternSet objects (wasteful). Maybe Devlin has another idea about how to do this efficiently.
Message was sent while issue was closed.
On 2017/04/18 15:30:40, nrpeter wrote: > https://codereview.chromium.org/2499493004/diff/500001/extensions/common/exte... > File extensions/common/extension_messages.h (right): > > https://codereview.chromium.org/2499493004/diff/500001/extensions/common/exte... > extensions/common/extension_messages.h:298: bool > uses_default_policy_host_restrictions; > On 2017/04/18 14:17:48, vabr (Chromium) wrote: > > Do the POD fields like uses_default_policy_host_restrictions and > > uses_default_policy_blocked_allowed_hosts below need an initial value, either > > here or in all constructors? > > > > Is that what MSan is complaining about in https://crbug.com/712641 > > I think msan is complaining about the policy_blocked_hosts and > policy_allowed_hosts. I only initialize those when > use_default_policy_host_restrictions is true. On the receiving end, we also look > at the boolean to see whether we will use the URLPatternSets. The issue is that > policy_blocked_hosts and policy_allowed_hosts get written to the message > regardless. > > I'll either have to conditionally write them to the message (yuck) or send empty > URLPatternSet objects (wasteful). Maybe Devlin has another idea about how to do > this efficiently. To me, it looks like it's complaining about the uses_default_policy_blocked_allowed_hosts, which is only set in one constructor. It needs to be set in all (or initialized to a default value in the struct definition).
Message was sent while issue was closed.
Patchset #20 (id:520001) has been deleted |
