Add DeveloperToolsAvailabilityAllowlist and DeveloperToolsAvailabilityBlocklist policies. [chromium/src : main]
Michał Kaczmarczyk (Gerrit)
Message from Michał Kaczmarczyk![]( "Open in Gerrit")
Set Ready For Review
Related details
- Dana Fried
- Dominique Fauteux-Chapleau
- Michał Kaczmarczyk
- Owen Min
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried added 1 comment![]( "Open in Gerrit")
1392: DeveloperToolsAvailabilityAllowlistDoes yaml support IfChange/ThenChange?
Related details
- Michał Kaczmarczyk
- Owen Min
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michał Kaczmarczyk (Gerrit)
Message from Michał Kaczmarczyk![]( "Open in Gerrit")
- Dana Fried
- Dominique Fauteux-Chapleau
- Michał Kaczmarczyk
- Owen Min
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michał Kaczmarczyk (Gerrit)
Michał Kaczmarczyk added 8 comments![]( "Open in Gerrit")
registry->RegisterListPref(Michał KaczmarczykCan you register the pref together with all other devtools prefs?
Done
const char kDeveloperToolsAvailabilityAllowlist[] =Michał KaczmarczykYou should avoid define policy pref under components/policy/core as they are designed for policy component itself.
Please find a better place (maybe in chrome/browser/devtools) for pref name
Done
1392: DeveloperToolsAvailabilityAllowlistMichał KaczmarczykDoes yaml support IfChange/ThenChange?
Done
When the <ph name="DEVELOPER_TOOLS_AVAILABILITY_POLICY_NAME">DeveloperToolsAvailability</ph> policy is set to 2 (DeveloperToolsDisallowed), this policy specifies a list of developer tools that are allowed to be opened. When <ph name="DEVELOPER_TOOLS_AVAILABILITY_POLICY_NAME">DeveloperToolsAvailability</ph> is set to any other value, this policy has no effect.Michał KaczmarczykI'm not actually sure if this is how we want the policy to behave or not, we should ask in crbug.com/442892562
In any case we should also call out that the policy interacts with `DeveloperToolsAvailabilityBlocklist`
Done
URL patterns are matched against the URL of the page being inspected. If a match is found, the DevTools are blocked. For information on the URL format, see https://support.google.com/chrome/a?p=url_blocklist_filter_format.Michał KaczmarczykDoes Chrome devtools an official product name? Does it have translation?
If not, we should use a placeholder(`<ph>`) to avoid translation.
Also, please say Chrome DevTools instead of DevTools to be more specific.
Done
This policy is limited to 1,000 entries.Michał KaczmarczykCan you mention the behavior for urls don't match any of the patterns? Will they fallback to the enum policy?
Please do so for Allowlist policy as well.
Done
- file://components/policy/OWNERSMichał KaczmarczykDeveloperToolsAvailability is owned by the extension team. Any chance they also own the two new ones here? (You can still be the human owner)
Done
Related details
- Dana Fried
- Owen Min
- Theresa Sullivan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Theresa Sullivan (Gerrit)
Theresa Sullivan added 1 comment![]( "Open in Gerrit")
Hi, am I just on here for //chrome/browser/devtools/devtools_window.cc?
If so, prefer sending to closest owners file; https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/devtools/OWNERS
Related details
- Dana Fried
- Michał Kaczmarczyk
- Owen Min
Michał Kaczmarczyk (Gerrit)
Michał Kaczmarczyk added 1 comment![]( "Open in Gerrit")
Hi, am I just on here for //chrome/browser/devtools/devtools_window.cc?
If so, prefer sending to closest owners file; https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/devtools/OWNERS
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried voted Code-Review+1![]( "Open in Gerrit")
- Michał Kaczmarczyk
- Owen Min
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominique Fauteux-Chapleau (Gerrit)
Dominique Fauteux-Chapleau voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Michał Kaczmarczyk
- Owen Min
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michał Kaczmarczyk (Gerrit)
Message from Michał Kaczmarczyk![]( "Open in Gerrit")
- Dana Fried
- Dominique Fauteux-Chapleau
- Michał Kaczmarczyk
- Owen Min
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michał Kaczmarczyk (Gerrit)
Michał Kaczmarczyk voted Auto-Submit+1![]( "Open in Gerrit")
- Dana Fried
- Owen Min
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michał Kaczmarczyk (Gerrit)
Michał Kaczmarczyk added 2 comments![]( "Open in Gerrit")
Michał KaczmarczykI deleted the parts about DeveloperToolsAvailability. I am waiting for Niamh explanation of how these policies are supposed to work.
Michał KaczmarczykI deleted the parts about DeveloperToolsAvailability. I am waiting for Niamh explanation of how these policies are supposed to work.
Done
Michał KaczmarczykLGTM overall
Since I'm only commenting on the policy text you should add enterprise-p...@google.com as a reviewer, they own the code and know more than me about conventions for user documentation.
Done
Related details
- Dana Fried
- Dominique Fauteux-Chapleau
- Owen Min
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominique Fauteux-Chapleau (Gerrit)
Dominique Fauteux-Chapleau voted Code-Review+1![]( "Open in Gerrit")
- Dana Fried
- Michał Kaczmarczyk
- Owen Min
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Michał Kaczmarczyk
- Owen Min
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Owen Min (Gerrit)
Owen Min added 8 comments![]( "Open in Gerrit")
This setting also turns off keyboard shortcuts and menu or context menu entries to open developer tools or the JavaScript console.Can you please fix the indent here and use two spaces for all lines?
Thanks.
caption: List of URL patterns for which DevTools are allowed to be opened```<ph name="CHROME_DEVTOOLS_NAME">Chrome DevTools</ph>```
When the <ph name="DEVELOPER_TOOLS_AVAILABILITY_POLICY_NAME">DeveloperToolsAvailability</ph> policy is set to 2 (DeveloperToolsDisallowed) this policy has no effect. When <ph name="DEVELOPER_TOOLS_AVAILABILITY_POLICY_NAME">DeveloperToolsAvailability</ph> is set to any other value, this policy specifies a list of URL patterns for which <ph name="DEVELOPER_TOOLS_NAME">Chrome DevTools</ph> are allowed to be opened. This policy can be used to allow <ph name="DEVELOPER_TOOLS_NAME">Chrome DevTools</ph> on specific URLs, even when they are generally blocked by the <ph name="DEVELOPER_TOOLS_AVAILABILITY_BLOCKLIST_POLICY_NAME">DeveloperToolsAvailabilityBlocklist</ph> policy.When DeveloperToolsAvailability is set to disallowed, any reason this policy can't still be used to provide per-url exceptions?
Personally, I found current approach is a bit complicated. Instead, I would recommend one of the following approach
1) If DeveloperToolsAvailability is set, ignore the allow and block list.
2) If allow or block list is set, ignore the DeveloperToolsAvailability
3) 3 policies can co-exist, when a url matches neither allow nor blocklist, use DeveloperToolsAvailability as fallback.
Personally, I prefer the 3rd option as this is a common design pattern for list+enum combination.
However, if you want to choose 1) or 2) or the current option (disable lists when set to 2). Please make sure you print error message to ignored policies with policy handler.
If you choose option 3, then I would recommend a doc like
```
Unmatched URLs will follow the DeveloperToolsAvailability policy.
```
And move it to a later position, after the "URL pattern invalid" section.
The <ph name="DEVELOPER_TOOLS_AVAILABILITY_ALLOWLIST_POLICY_NAME">DeveloperToolsAvailabilityAllowlist</ph> policy takes precedence over <ph name="DEVELOPER_TOOLS_AVAILABILITY_BLOCKLIST_POLICY_NAME">DeveloperToolsAvailabilityBlocklist</ph>..... for URLs that match both lists.
If URL patterns are invalid, they are ignored and it follows the <ph name="DEVELOPER_TOOLS_AVAILABILITY_POLICY_NAME">DeveloperToolsAvailability</ph> policy.`they are ignored and follow the ...`
Also URL pattern won't follow DeveloperToolsAvailability, URL will.
So I think it's better to simply say
```
Invalid URL patterns will be ignored.
```
And maybe
```
Unmatched URLs will follow the DeveloperToolsAvailability policy.
```
Of course, the last sentence depends on your answer of the comment I left above.
Will these two new policies affect extension or web app's developer tool?
If the <ph name="DEVELOPER_TOOLS_AVAILABILITY_POLICY_NAME">DeveloperToolsAvailability</ph> policy is set to 2 (DeveloperToolsDisallowed), all <ph name="DEVELOPER_TOOLS_NAME">Chrome DevTools</ph> are disabled, and this policy has no effect. For any other value, this policy blocks access to <ph name="DEVELOPER_TOOLS_NAME">Chrome DevTools</ph>.I don't think this paragraph is necessary. But please see my other comments above first.
Also, if this is kept, please move it to the end of the description.
"policy_pref_mapping_tests": [Can you use `simple_policy_pref_mapping_test`?
Related details
- Michał Kaczmarczyk
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michał Kaczmarczyk (Gerrit)
Message from Michał Kaczmarczyk![]( "Open in Gerrit")
- Dana Fried
- Dominique Fauteux-Chapleau
- Michał Kaczmarczyk
- Owen Min
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominique Fauteux-Chapleau (Gerrit)
Dominique Fauteux-Chapleau added 1 comment![]( "Open in Gerrit")
<<<<<<< PATCH SET (f5e7c4a7dd968da36fe3dc01c35dfca2c82c23d3 Add DeveloperToolsAvailabilityAllowlist and DeveloperToolsAv)
label="ExtensionForceInstallWithMinorPolicyViolationsEnabled"/>
<int value="1396" label="DeveloperToolsAvailabilityAllowlist"/>
<int value="1397" label="DeveloperToolsAvailabilityBlocklist"/>
||||||| BASE (55eb5b3eb58c1cfcd23517bdaa858fc35f840c9f [//cc] Remove PlaybackImageProvider::RasterMode::kGpu)
label="ExtensionForceInstallWithMinorPolicyViolationsEnabled"/>
=======
label="ExtensionForceInstallWithNonMalwareViolationsEnabled"/>
<int value="1396" label="CacheEncryptionEnabled"/>
>>>>>>> BASE (7063aa93d1d0143973fe46f26985501d5579f10a Roll ANGLE from ae33b59db8e2 to 621fe27d9522 (1 revision))Please fix this ERROR reported by Conflict Markers: Complete set of diff3 style conflict markers found. If this is a false alarm, ad...
Complete set of diff3 style conflict markers found. If this is a false alarm, add IGNORE_MERGE_CONFLICT_CHECK==<reason> to your commit message.
Related details
- Michał Kaczmarczyk
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Michał Kaczmarczyk (Gerrit)
Michał Kaczmarczyk voted and added 9 comments![]( "Open in Gerrit")
Votes added by Michał Kaczmarczyk
| Auto-Submit | +1 |
| Commit-Queue | +1 |
9 comments
This setting also turns off keyboard shortcuts and menu or context menu entries to open developer tools or the JavaScript console.Can you please fix the indent here and use two spaces for all lines?
Thanks.
Done
caption: List of URL patterns for which DevTools are allowed to be opened```<ph name="CHROME_DEVTOOLS_NAME">Chrome DevTools</ph>```
Done
When the <ph name="DEVELOPER_TOOLS_AVAILABILITY_POLICY_NAME">DeveloperToolsAvailability</ph> policy is set to 2 (DeveloperToolsDisallowed) this policy has no effect. When <ph name="DEVELOPER_TOOLS_AVAILABILITY_POLICY_NAME">DeveloperToolsAvailability</ph> is set to any other value, this policy specifies a list of URL patterns for which <ph name="DEVELOPER_TOOLS_NAME">Chrome DevTools</ph> are allowed to be opened. This policy can be used to allow <ph name="DEVELOPER_TOOLS_NAME">Chrome DevTools</ph> on specific URLs, even when they are generally blocked by the <ph name="DEVELOPER_TOOLS_AVAILABILITY_BLOCKLIST_POLICY_NAME">DeveloperToolsAvailabilityBlocklist</ph> policy.When DeveloperToolsAvailability is set to disallowed, any reason this policy can't still be used to provide per-url exceptions?
Personally, I found current approach is a bit complicated. Instead, I would recommend one of the following approach
1) If DeveloperToolsAvailability is set, ignore the allow and block list.
2) If allow or block list is set, ignore the DeveloperToolsAvailability
3) 3 policies can co-exist, when a url matches neither allow nor blocklist, use DeveloperToolsAvailability as fallback.
Personally, I prefer the 3rd option as this is a common design pattern for list+enum combination.
However, if you want to choose 1) or 2) or the current option (disable lists when set to 2). Please make sure you print error message to ignored policies with policy handler.If you choose option 3, then I would recommend a doc like
```
Unmatched URLs will follow the DeveloperToolsAvailability policy.
```And move it to a later position, after the "URL pattern invalid" section.
Done
The <ph name="DEVELOPER_TOOLS_AVAILABILITY_ALLOWLIST_POLICY_NAME">DeveloperToolsAvailabilityAllowlist</ph> policy takes precedence over <ph name="DEVELOPER_TOOLS_AVAILABILITY_BLOCKLIST_POLICY_NAME">DeveloperToolsAvailabilityBlocklist</ph>..... for URLs that match both lists.
Done
If URL patterns are invalid, they are ignored and it follows the <ph name="DEVELOPER_TOOLS_AVAILABILITY_POLICY_NAME">DeveloperToolsAvailability</ph> policy.`they are ignored and follow the ...`
Also URL pattern won't follow DeveloperToolsAvailability, URL will.
So I think it's better to simply say
```
Invalid URL patterns will be ignored.
```And maybe
```
Unmatched URLs will follow the DeveloperToolsAvailability policy.
```Of course, the last sentence depends on your answer of the comment I left above.
Done
Will these two new policies affect extension or web app's developer tool?
Done
If the <ph name="DEVELOPER_TOOLS_AVAILABILITY_POLICY_NAME">DeveloperToolsAvailability</ph> policy is set to 2 (DeveloperToolsDisallowed), all <ph name="DEVELOPER_TOOLS_NAME">Chrome DevTools</ph> are disabled, and this policy has no effect. For any other value, this policy blocks access to <ph name="DEVELOPER_TOOLS_NAME">Chrome DevTools</ph>.I don't think this paragraph is necessary. But please see my other comments above first.
Also, if this is kept, please move it to the end of the description.
Done
Can you use `simple_policy_pref_mapping_test`?
Done
<<<<<<< PATCH SET (f5e7c4a7dd968da36fe3dc01c35dfca2c82c23d3 Add DeveloperToolsAvailabilityAllowlist and DeveloperToolsAv)
label="ExtensionForceInstallWithMinorPolicyViolationsEnabled"/>
<int value="1396" label="DeveloperToolsAvailabilityAllowlist"/>
<int value="1397" label="DeveloperToolsAvailabilityBlocklist"/>
||||||| BASE (55eb5b3eb58c1cfcd23517bdaa858fc35f840c9f [//cc] Remove PlaybackImageProvider::RasterMode::kGpu)
label="ExtensionForceInstallWithMinorPolicyViolationsEnabled"/>
=======
label="ExtensionForceInstallWithNonMalwareViolationsEnabled"/>
<int value="1396" label="CacheEncryptionEnabled"/>
>>>>>>> BASE (7063aa93d1d0143973fe46f26985501d5579f10a Roll ANGLE from ae33b59db8e2 to 621fe27d9522 (1 revision))Please fix this ERROR reported by Conflict Markers: Complete set of diff3 style conflict markers found. If this is a false alarm, ad...
Complete set of diff3 style conflict markers found. If this is a false alarm, add IGNORE_MERGE_CONFLICT_CHECK==<reason> to your commit message.
- Dana Fried
- Owen Min
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dominique Fauteux-Chapleau (Gerrit)
Dominique Fauteux-Chapleau voted Code-Review+1![]( "Open in Gerrit")
- Dana Fried
- Michał Kaczmarczyk
- Owen Min
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dana Fried (Gerrit)
Dana Fried voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Michał Kaczmarczyk
- Owen Min
Code-Coverage
Code-Owners
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |