[Extensions] Add option to show per-extension site settings in dialog [chromium/src : main]
Kelvin Jiang (Gerrit)
Attention is currently required from: Demetrios Papadopoulos, Devlin Cronin.
Kelvin Jiang would like Demetrios Papadopoulos and Devlin Cronin to review this change.
[Extensions] Add option to show per-extension site settings in dialog
This CL adds a list of extensions that have specified at least one
site matching the site in the edit permissions dialog. This list is
shown when the "customize per extension" option is selected, and if
the user clicks save on the dialog, then the site is removed from any
user specified site sets.
If the site in the dialog is a host, then assume we're editing for http
and https schemes.
If the site contains a wildcard (*.<eTLD+1>, then the radio
button group is not shown as such sites can only be specified by
extensions (and not in any user specified site sets).
Note: there is a discrepancy between the extension count vs #
extensions in the dialog for a : the count shows the #
extensions that can run on all subdomains for an eTLD+1 vs the list of
extensions in the dialog shows extensions that can run on at least one
subdomain for an eTLD+1. This may need to be revisited later
Screenshots: https://imgur.com/a/khBmXat
Bug: 1253673
Change-Id: Idba1bb97de66481d615b224c2d4a3d90cf37a945
---
M chrome/app/extensions_strings.grdp
A chrome/app/extensions_strings_grdp/IDS_EXTENSIONS_EDIT_SITE_PERMISSIONS_CUSTOMIZE_PER_EXTENSION.png.sha1
A chrome/app/extensions_strings_grdp/IDS_EXTENSIONS_SITE_PERMISSIONS_ALWAYS_ON_ALL_SITES.png.sha1
A chrome/app/extensions_strings_grdp/IDS_EXTENSIONS_SITE_PERMISSIONS_ALWAYS_ON_THIS_SITE.png.sha1
A chrome/app/extensions_strings_grdp/IDS_EXTENSIONS_SITE_PERMISSIONS_ON_CLICK.png.sha1
M chrome/browser/extensions/api/developer_private/developer_private_api.cc
M chrome/browser/resources/extensions/manager.html
M chrome/browser/resources/extensions/service.ts
M chrome/browser/resources/extensions/site_permissions.html
M chrome/browser/resources/extensions/site_permissions.ts
M chrome/browser/resources/extensions/site_permissions_by_site.html
M chrome/browser/resources/extensions/site_permissions_by_site.ts
M chrome/browser/resources/extensions/site_permissions_edit_permissions_dialog.html
M chrome/browser/resources/extensions/site_permissions_edit_permissions_dialog.ts
M chrome/browser/resources/extensions/site_permissions_list.html
M chrome/browser/resources/extensions/site_permissions_list.ts
M chrome/browser/resources/extensions/site_permissions_site_group.html
M chrome/browser/resources/extensions/site_permissions_site_group.ts
M chrome/browser/resources/extensions/site_settings_mixin.ts
M chrome/browser/ui/webui/extensions/extensions_ui.cc
M chrome/test/data/webui/extensions/site_permissions_by_site_test.ts
M chrome/test/data/webui/extensions/site_permissions_edit_permissions_dialog_test.ts
M chrome/test/data/webui/extensions/site_permissions_site_group_test.ts
M chrome/test/data/webui/extensions/test_service.ts
M tools/typescript/definitions/developer_private.d.ts
25 files changed, 473 insertions(+), 55 deletions(-)
To view, visit change 3897553. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Demetrios Papadopoulos, Devlin Cronin.
Patch set 6:Commit-Queue +1
Demetrios Papadopoulos (Gerrit)
Attention is currently required from: Devlin Cronin, Kelvin Jiang.
11 comments:
Commit Message:
Note: there is a discrepancy between the extension count vs #
extensions in the dialog for a : the count shows the #
This sentence is a bit hard to understand. Can you replace "#" with plain English instead? Also what does "a :" mean here?
File chrome/browser/resources/extensions/site_permissions_edit_permissions_dialog.html:
Patch Set #6, Line 50:
<div slot="header">Is the cr-radio-group part of the header so that it is always visible? If so, maybe add a comment to make it more obvious?
Patch Set #6, Line 69:
<template is="dom-if" if="[[showExtensionSiteAccessData_(siteSet_)]]">How does the dialog look like when this is false? Should this dom-if be moved outside slot=body?
Patch Set #6, Line 79:
="true"`disabled` is a boolean attribute and therefore does not take a value. Its presence or absense determines whether the element is disabled. Here it incorrectly gets a string value of "true" instead. Let's remove
File chrome/browser/resources/extensions/site_permissions_edit_permissions_dialog.ts:
Patch Set #6, Line 90:
HostAccess_Rename to hostAccessEnum_
static get observers() {
return ['onExtensionsUpdated_(extensions)', 'onSiteSetUpdated_(siteSet_)'];
}observers() should be used for multi-property obesrvers. Here since two single-property observers are registered, these better belongto each Polymer property's definition instead.
s/url/URL
Is this a full proof way to check? (Leaving this up to you and Devlin to check, just pointing out that it might be insufficient)
Nit: Unnecessary parentheses.
Patch Set #6, Line 182:
chrome.developerPrivate.SiteSet.EXTENSION_SPECIFIEDSince you've added a shorter alias for this, let's use it here (and elsewhere).
File chrome/browser/resources/extensions/site_permissions_list.ts:
Here and elsewhere where a new `extensions` Polymer property is added: Need to declare `extensions` as a member variable as well (so that the exact Array type can be documented).
To view, visit change 3897553. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Demetrios Papadopoulos, Kelvin Jiang.
11 comments:
Patchset:
Thanks, Kelvin!
Thanks, Kelvin!
File chrome/browser/extensions/api/developer_private/developer_private_api.cc:
Patch Set #6, Line 2471:
->GenerateInstalledExtensionsSet(This change might warrant a comment in the CL description
File chrome/browser/resources/extensions/service.ts:
function(resolve) {
chrome.developerPrivate.getMatchingExtensionsForSite(site, resolve);
});nit: I think webui style is prefer arrow functions
File chrome/browser/resources/extensions/site_permissions_edit_permissions_dialog.ts:
Patch Set #6, Line 130:
Returns if `this.site` is a full url or just a hostnit: returns what if it's a full url or just a host?
Probably prefer "Returns true if `this.site` is a full URL (as opposed to just a host)."
Is this a full proof way to check? (Leaving this up to you and Devlin to check, just pointing out th […]
Thanks for flagging, Demetrios. What happens for e.g. file schemes on windows operating systems?
Patch Set #6, Line 142:
if (siteSet !== EXTENSION_SPECIFIED) {add a comment why
Patch Set #6, Line 146:
isSiteFullUrl_this assumes that if it's not a full site, it's a (valid) host. Can we add an assert to isSiteFullUrl() to verify that?
for (const {id, name, iconUrl} of extensions) {
extensionsIdToInfo.set(id, {name, iconUrl});
}This is all we ever do with `extensions`. Would it make sense to have the property be this map (constructed within a setter of this class) rather than constructing it here each time?
Patch Set #6, Line 156:
if (extensionsIdToInfo.has(id)) {When would extensionsIdToInfo not have `id`? (Add a comment)
Patch Set #6, Line 209:
return this.site.includes('*');is it guaranteed a site never includes '*' in the path or scheme, and only includes it in the host?
To view, visit change 3897553. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Demetrios Papadopoulos, Kelvin Jiang.
Kelvin Jiang uploaded patch set #7 to this change.
[Extensions] Add option to show per-extension site settings in dialog
This CL adds a list of extensions that have specified at least one
site matching the site in the edit permissions dialog. This list is
shown when the "customize per extension" option is selected, and if
the user clicks save on the dialog, then the site is removed from any
user specified site sets.
If the site in the dialog is a host, then assume we're editing for http
and https schemes.
If the site contains a wildcard (*.<eTLD+1>, then the radio
button group is not shown as such sites can only be specified by
extensions (and not in any user specified site sets).
The developerPrivate.getMatchingExtensionsForSite API method has been
changed to iterate over the same set of extensions that
developerPrivate.getExtensionsInfo iterates over, which means blocked
extensions will no longer be considered in that method.
Note: there is a discrepancy between the extension count vs the
number of extensions in the dialog for a site that specifies matching
all subdomains such as "*.google.com: the count shows the number of
25 files changed, 479 insertions(+), 55 deletions(-)
To view, visit change 3897553. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Demetrios Papadopoulos, Kelvin Jiang.
Kelvin Jiang uploaded patch set #8 to this change.
[Extensions] Add option to show per-extension site settings in dialog
This CL adds a list of extensions that have specified at least one
site matching the site in the edit permissions dialog. This list is
shown when the "customize per extension" option is selected, and if
the user clicks save on the dialog, then the site is removed from any
user specified site sets.
If the site in the dialog is a host, then assume we're editing for http
and https schemes.
If the site contains a wildcard (*.<eTLD+1>, then the radio
button group is not shown as such sites can only be specified by
extensions (and not in any user specified site sets).
The developerPrivate.getMatchingExtensionsForSite API method has been
changed to iterate over the same set of extensions that
developerPrivate.getExtensionsInfo iterates over, which means blocked
extensions will no longer be considered in that method.
Note: there is a discrepancy between the extension count vs the
number of extensions in the dialog for a site that specifies matching
all subdomains such as "*.google.com: the count shows the number of
extensions that can run on all subdomains for an eTLD+1 vs the list of
extensions in the dialog shows extensions that can run on at least one
subdomain for an eTLD+1. This may need to be revisited later
Screenshots: https://imgur.com/a/Sa4nPDV
Kelvin Jiang (Gerrit)
Attention is currently required from: Demetrios Papadopoulos, Devlin Cronin.
Patch set 9:Commit-Queue +1
19 comments:
Commit Message:
Note: there is a discrepancy between the extension count vs #
extensions in the dialog for a : the count shows the #
This sentence is a bit hard to understand. […]
done, updated the description
File chrome/browser/extensions/api/developer_private/developer_private_api.cc:
Patch Set #6, Line 2471:
->GenerateInstalledExtensionsSet(This change might warrant a comment in the CL description
Done
File chrome/browser/resources/extensions/service.ts:
function(resolve) {
chrome.developerPrivate.getMatchingExtensionsForSite(site, resolve);
});nit: I think webui style is prefer arrow functions
Done (for this one), changing the rest of this file to use arrows will be done in a follow up
File chrome/browser/resources/extensions/site_permissions_edit_permissions_dialog.html:
Patch Set #6, Line 50:
<div slot="header">
Is the cr-radio-group part of the header so that it is always visible? If so, maybe add a comment to […]
it's part of the header so that if the dialog's body exceeds a certain height, the body scrolls but the header will stay in place (see screenshots)
Patch Set #6, Line 69:
<template is="dom-if" if="[[showExtensionSiteAccessData_(siteSet_)]]">How does the dialog look like when this is false? Should this dom-if be moved outside slot=body?
The dialog would just have the cr-radio-group in that case, though there is a slightly larger space between the bottom of the radio group and the save/cancel buttons (the body slot container has a minimum height, and the body slot gets added to the dialog anyway, not sure how I'd override this?)
Patch Set #6, Line 79:
="true"
`disabled` is a boolean attribute and therefore does not take a value. […]
Done
File chrome/browser/resources/extensions/site_permissions_edit_permissions_dialog.ts:
Patch Set #6, Line 90:
HostAccess_Rename to hostAccessEnum_
Done
static get observers() {
return ['onExtensionsUpdated_(extensions)', 'onSiteSetUpdated_(siteSet_)'];
}
observers() should be used for multi-property obesrvers. […]
Done
Patch Set #6, Line 130:
Returns if `this.site` is a full url or just a host
nit: returns what if it's a full url or just a host? […]
Done
s/url/URL
Done
Thanks for flagging, Demetrios. What happens for e.g. […]
oh... perhaps I can change this to `isSiteHostOnly` and test against a regex that only captures the host and optionally, the includes subdomain specifier "*."
Patch Set #6, Line 142:
if (siteSet !== EXTENSION_SPECIFIED) {add a comment why
Done, it's to avoid a redundant API call if the extensions won't be displayed anyway
Patch Set #6, Line 146:
isSiteFullUrl_
this assumes that if it's not a full site, it's a (valid) host. […]
I changed isSiteFullUrl() to isSiteHostOnly() which matches against a regex (and if not, assumes it's a full site). Does that work for an assert?
Re: full site: are there other "full sites" that can be legal here other than something with a scheme + host? (and, do windows file schemes use backslashes \ )?
^So far, in the all sites/permissions by sites page, we're using only hosts, and in the site permissions/user specified sites page, we're using origins (scheme + host). If this is the case for a while then I can reuse a regex to test for a full site (and assert that it matches), otherwise, this will need to be revisited: an API method to check for a site on developerPrivate was mentioned before, and I could call that method as the dialog is initialized since the dialog's site shouldn't change.
WDYT?
Nit: Unnecessary parentheses.
Done
for (const {id, name, iconUrl} of extensions) {
extensionsIdToInfo.set(id, {name, iconUrl});
}
This is all we ever do with `extensions`. […]
Done
Patch Set #6, Line 156:
if (extensionsIdToInfo.has(id)) {When would extensionsIdToInfo not have `id`? (Add a comment)
I think this was added to make the typescript compiler not freak out.
Actually, this shouldn't happen since ids in extensionsIdToInfo should be a superset of matchingExtensionsInfo, so I changed this to an assert instead
Patch Set #6, Line 182:
chrome.developerPrivate.SiteSet.EXTENSION_SPECIFIEDSince you've added a shorter alias for this, let's use it here (and elsewhere).
Done
Patch Set #6, Line 209:
return this.site.includes('*');is it guaranteed a site never includes '*' in the path or scheme, and only includes it in the host?
yes, because
- if it's a full site, then this dialog would be opened from the site permissions page which only shows user specified sites, which are origins
- if it's not, then the site would either be just a host, or *.<etld+1>
I'll change this to "*." and add a comment on how we can just use that to identify subdomains
File chrome/browser/resources/extensions/site_permissions_list.ts:
Here and elsewhere where a new `extensions` Polymer property is added: Need to declare `extensions` […]
Done
To view, visit change 3897553. To unsubscribe, or for help writing mail filters, visit settings.
Demetrios Papadopoulos (Gerrit)
Attention is currently required from: Devlin Cronin, Kelvin Jiang.
Kelvin Jiang has uploaded this change for review.
25 files changed, 507 insertions(+), 56 deletions(-)
Demetrios Papadopoulos (Gerrit)
Attention is currently required from: Devlin Cronin, Kelvin Jiang.
9 comments:
Patchset:
+johntlee: See question about alt="" below.
File chrome/browser/resources/extensions/site_permissions_edit_permissions_dialog.html:
Patch Set #9, Line 76:
alt=""Should this have something like `role=presentation` instead?
+johntlee to conlult.
File chrome/browser/resources/extensions/site_permissions_edit_permissions_dialog.ts:
this.delegate.getMatchingExtensionsForSite(siteToCheck)
.then(matchingExtensionsInfo => {
const extensionSiteAccessData: ExtensionSiteAccessInfo[] = [];
matchingExtensionsInfo.forEach(({id, siteAccess}) => {
assert(this.extensionsIdToInfo_.has(id));
const {name, iconUrl} = this.extensionsIdToInfo_.get(id)!;
extensionSiteAccessData.push({id, name, iconUrl, siteAccess});
});
this.extensionSiteAccessData_ = extensionSiteAccessData;
});
}Nit: Use async/await to make this more readable?
File chrome/test/data/webui/extensions/site_permissions_edit_permissions_dialog_test.ts:
Patch Set #9, Line 121:
const submitNit: Unnecessary intermediate var?
Patch Set #9, Line 166:
await delegate.whenCalled('getMatchingExtensionsForSite');Let's assert on the value that was passed to getMatchingExtensionsForSite() (here and elsewhere)?
Not needed?
Patch Set #9, Line 170:
HTMLSelectElementNot needed.
Patch Set #9, Line 198:
HTMLSelectElementNot needed.
File tools/typescript/definitions/developer_private.d.ts:
export function getMatchingExtensionsForSite(
site: string,
callback: (result: MatchingExtensionInfo[]) => void): void;Optional: How about using the Promise-based version instead (also see https://crbug.com/1368302).
IIUC, you could declare the Promise-based API here, and use it from the calling code, as it is already supported by the backend. Then you don't need to wrap this with a wrapper to convert it to Promise-based.
To view, visit change 3897553. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Demetrios Papadopoulos, Devlin Cronin.
Patch set 10:Commit-Queue +1
7 comments:
File chrome/browser/resources/extensions/site_permissions_edit_permissions_dialog.ts:
this.delegate.getMatchingExtensionsForSite(siteToCheck)
.then(matchingExtensionsInfo => {
const extensionSiteAccessData: ExtensionSiteAccessInfo[] = [];
matchingExtensionsInfo.forEach(({id, siteAccess}) => {
assert(this.extensionsIdToInfo_.has(id));
const {name, iconUrl} = this.extensionsIdToInfo_.get(id)!;
extensionSiteAccessData.push({id, name, iconUrl, siteAccess});
});
this.extensionSiteAccessData_ = extensionSiteAccessData;
});
}Nit: Use async/await to make this more readable?
Done
File chrome/test/data/webui/extensions/site_permissions_edit_permissions_dialog_test.ts:
Patch Set #9, Line 121:
const submitNit: Unnecessary intermediate var?
Done
Patch Set #9, Line 166:
await delegate.whenCalled('getMatchingExtensionsForSite');Let's assert on the value that was passed to getMatchingExtensionsForSite() (here and elsewhere)?
Done
Patch Set #9, Line 170:
HTMLSelectElementNot needed.
Done
Not needed?
Done
Patch Set #9, Line 198:
HTMLSelectElementNot needed.
Done
File tools/typescript/definitions/developer_private.d.ts:
export function getMatchingExtensionsForSite(
site: string,
callback: (result: MatchingExtensionInfo[]) => void): void;
Optional: How about using the Promise-based version instead (also see https://crbug.com/1368302). […]
isn't this just the typescript interface definitions based on what's in https://source.chromium.org/chromium/chromium/src/+/main:chrome/common/extensions/api/developer_private.idl ?
To view, visit change 3897553. To unsubscribe, or for help writing mail filters, visit settings.
Demetrios Papadopoulos (Gerrit)
Attention is currently required from: Devlin Cronin, Kelvin Jiang.
1 comment:
File tools/typescript/definitions/developer_private.d.ts:
export function getMatchingExtensionsForSite(
site: string,
callback: (result: MatchingExtensionInfo[]) => void): void;
isn't this just the typescript interface definitions based on what's in https://source.chromium. […]
See all the `supportsPromises` tokens in the link above. The C++ implementation supports both Promise-based and callback-based APIs at the same time. It is up to us to pick which one to use, and the Promise-based API is preferable.
To view, visit change 3897553. To unsubscribe, or for help writing mail filters, visit settings.
Demetrios Papadopoulos (Gerrit)
Attention is currently required from: Devlin Cronin, John Lee, Kelvin Jiang.
Demetrios Papadopoulos would like John Lee to review this change authored by Kelvin Jiang.
25 files changed, 513 insertions(+), 58 deletions(-)
Demetrios Papadopoulos (Gerrit)
Attention is currently required from: Devlin Cronin, John Lee, Kelvin Jiang.
Patch set 10:Code-Review +1
3 comments:
Patchset:
LGTM.
File chrome/browser/resources/extensions/site_permissions_edit_permissions_dialog.html:
Patch Set #9, Line 76:
alt=""
Should this have something like `role=presentation` instead? […]
Moving to reviewer list. Please see question above.
File tools/typescript/definitions/developer_private.d.ts:
export function getMatchingExtensionsForSite(
site: string,
callback: (result: MatchingExtensionInfo[]) => void): void;
See all the `supportsPromises` tokens in the link above. […]
Ping ^. See a similar CL that adds the Promise-based version at [1]
To view, visit change 3897553. To unsubscribe, or for help writing mail filters, visit settings.
John Lee (Gerrit)
Attention is currently required from: Demetrios Papadopoulos, Devlin Cronin, Kelvin Jiang.
1 comment:
File chrome/browser/resources/extensions/site_permissions_edit_permissions_dialog.html:
Patch Set #9, Line 76:
alt=""
Moving to reviewer list. Please see question above.
`alt=""` allegedly has more widespread screenreader support. This should be fine.
To view, visit change 3897553. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Demetrios Papadopoulos, Devlin Cronin, John Lee.
Patch set 11:Commit-Queue +1
3 comments:
Patchset:
+Devlin, mind taking another look?
File chrome/browser/resources/extensions/site_permissions_edit_permissions_dialog.html:
Patch Set #9, Line 76:
alt=""
`alt=""` allegedly has more widespread screenreader support. This should be fine.
Thanks John! (resolving)
File tools/typescript/definitions/developer_private.d.ts:
export function getMatchingExtensionsForSite(
site: string,
callback: (result: MatchingExtensionInfo[]) => void): void;
Ping ^. See a similar CL that adds the Promise-based version at [1] […]
Done
To view, visit change 3897553. To unsubscribe, or for help writing mail filters, visit settings.
Devlin Cronin (Gerrit)
Attention is currently required from: Demetrios Papadopoulos, John Lee, Kelvin Jiang.
Patch set 11:Code-Review +1
3 comments:
Patchset:
LGTM % one comment. Thanks, Kelvin!
LGTM % one comment. Thanks, Kelvin!
File chrome/browser/resources/extensions/site_permissions_edit_permissions_dialog.ts:
Patch Set #11, Line 49:
'([a-z0-9\\.-]+\\.[a-z0-9]+|localhost)' +Two concerns with this regex:
- This wouldn't work for TLDs that are valid destinations, e.g. "ai" (http://ai).
- This also doesn't allow for any ports — could we have a host-only pattern that includes a port, if it were specifically added or visited by the user?
A somewhat tedious, but I *think* accurate, check here would be to compare the start of the url against all allowed schemes for extensions (see URLPattern for the ones that are allowed) followed by the scheme separator. e.g., something like:
const SCHEMES = [
'http',
'https',
...
];function isSiteHostOnly_(site) {
return SCHEMES.some(scheme => site.startsWith(scheme + '://'));
}That would probably work?
To view, visit change 3897553. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Demetrios Papadopoulos, John Lee.
Patch set 12:Commit-Queue +2
1 comment:
File chrome/browser/resources/extensions/site_permissions_edit_permissions_dialog.ts:
Patch Set #11, Line 49:
'([a-z0-9\\.-]+\\.[a-z0-9]+|localhost)' +
Two concerns with this regex: […]
Done
To view, visit change 3897553. To unsubscribe, or for help writing mail filters, visit settings.
Kelvin Jiang (Gerrit)
Attention is currently required from: Demetrios Papadopoulos, John Lee, Kelvin Jiang.
To view, visit change 3897553. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
11 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: chrome/browser/resources/extensions/site_permissions_edit_permissions_dialog.ts
Insertions: 22, Deletions: 17.
@@ -35,19 +35,23 @@
const EXTENSION_SPECIFIED = chrome.developerPrivate.SiteSet.EXTENSION_SPECIFIED;
-// A RegExp to roughly match a single hostname, and optionally the includes
-// subdomains specifier.
-// exec'ing() this RegExp will match the following groups:
-// 0: Full matched string.
-// 1: Match subdomains ('*.').
-// 2: Hostname (e.g., 'example.com').
-const hostPatternRegExp = new RegExp(
- '^' +
- // Include subdomains specifier; optional.
- '(\\*\\.)?' +
- // Hostname or localhost, required.
- '([a-z0-9\\.-]+\\.[a-z0-9]+|localhost)' +
- '$');
+// A list of possible schemes that can be specified by extension host
+// permissions. This is derived from URLPattern::SchemeMasks.
+const VALID_SCHEMES = [
+ '*',
+ 'http',
+ 'https',
+ 'file',
+ 'ftp',
+ 'chrome',
+ 'chrome-extension',
+ 'filesystem',
+ 'ftp',
+ 'ws',
+ 'wss',
+ 'data',
+ 'uuid-in-package',
+];
const SitePermissionsEditPermissionsDialogElementBase =
I18nMixin(PolymerElement);
@@ -147,11 +151,12 @@
this.updateExtensionSiteAccessData_(siteSet);
}
- // Returns true if this.site is a just a host. If not, assume the site is a
- // full URL. Different components that use this dialog may supply either a URL
- // or just a host.
+ // Returns true if this.site is a just a host by checking whether or not it
+ // starts with a valid scheme. If not, assume the site is a full URL.
+ // Different components that use this dialog may supply either a URL or just a
+ // host.
private isSiteHostOnly_(): boolean {
- return hostPatternRegExp.test(this.site);
+ return !VALID_SCHEMES.some(scheme => this.site.startsWith(`${scheme}://`));
}
// Fetches all extensions that have requested access to `this.site` along with
```
[Extensions] Add option to show per-extension site settings in dialog
This CL adds a list of extensions that have specified at least one
site matching the site in the edit permissions dialog. This list is
shown when the "customize per extension" option is selected, and if
the user clicks save on the dialog, then the site is removed from any
user specified site sets.
If the site in the dialog is a host, then assume we're editing for http
and https schemes.
If the site contains a wildcard (*.<eTLD+1>, then the radio
button group is not shown as such sites can only be specified by
extensions (and not in any user specified site sets).
The developerPrivate.getMatchingExtensionsForSite API method has been
changed to iterate over the same set of extensions that
developerPrivate.getExtensionsInfo iterates over, which means blocked
extensions will no longer be considered in that method.
Note: there is a discrepancy between the extension count vs the
number of extensions in the dialog for a site that specifies matching
all subdomains such as "*.google.com: the count shows the number of
extensions that can run on all subdomains for an eTLD+1 vs the list of
extensions in the dialog shows extensions that can run on at least one
subdomain for an eTLD+1. This may need to be revisited later
Screenshots: https://imgur.com/a/Sa4nPDV
Bug: 1253673
Change-Id: Idba1bb97de66481d615b224c2d4a3d90cf37a945
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3897553
Reviewed-by: Demetrios Papadopoulos <dpa...@chromium.org>
Reviewed-by: Devlin Cronin <rdevlin...@chromium.org>
Commit-Queue: Kelvin Jiang <kelvi...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1053387}
25 files changed, 520 insertions(+), 58 deletions(-)