Mask DENIED status for storage-access permission in overrides [chromium/src : main]
Jalon Thomas (Gerrit)
Jalon Thomas added 4 comments![]( "Open in Gerrit")
Jalon ThomasYou could also consider adding to chrome/browser/storage_access_api/api_browsertest.cc, to make sure that this really causes the JS-visible change we intend it to.
Added a test here. I used QueryPermission as the sole check, but let me know if there's any other methods I should use as well.
Masks DENIED status for storage-access permission in overridesJalon ThomasNit: git commit titles should be phrased as commands (imperative mood). (https://chris.beams.io/git-commit#imperative, linked from https://chromium.googlesource.com/chromium/src/+/main/docs/contributing.md#Uploading-a-change-for-review)
```suggestion
Mask DENIED status for storage-access permission in overrides
```
Done
# 'prompt' when queried to prevent any attempt at retaliating against users Jalon ThomasPlease fix this WARNING reported by Trailing Whitespace: Please remove the trailing whitespace.
Please remove the trailing whitespace.
Done
if (permission == blink::PermissionType::STORAGE_ACCESS_GRANT &&
*status == PermissionStatus::DENIED) {
return std::make_optional(PermissionResult(
PermissionStatus::ASK, PermissionStatusSource::UNSPECIFIED));
}
return std::make_optional(
PermissionResult(*status, PermissionStatusSource::UNSPECIFIED));
}Jalon ThomasPersonal preference: there's a bit of duplication here, I'd factor that out into a single return statement for simplicity. You can use an additional mutable variable to help with that (since the map is immutable in this method).
```suggestion
PermissionStatus adjusted_status = *status;
if (permission == blink::PermissionType::STORAGE_ACCESS_GRANT &&
*status == PermissionStatus::DENIED) {
adjusted_status = PermissionStatus::ASK;
}return std::make_optional(
PermissionResult(adjusted_status, PermissionStatusSource::UNSPECIFIED));
}
```
Used this suggestion.
Related details
- Chris Fredrickson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Fredrickson (Gerrit)
Chris Fredrickson voted and added 3 comments![]( "Open in Gerrit")
Votes added by Chris Fredrickson
| Code-Review | +1 |
3 comments
NavigateFrameTo(kHostB, "/echoheader?cookie");We're not depending on the `echoheader` logic in this test, so we can simplify this.
```suggestion
NavigateFrameTo(kHostB, "/empty.html");
```
devtools_client.SendCommandSync(
"Browser.setPermission",
base::Value::Dict()
.Set("setting", "granted")
.Set("origin", kOriginA)
.Set("embeddedOrigin", kOriginB)
.Set("permission",
base::Value::Dict().Set("name", "storage-access")));
// Ensure that we granted the permission.
EXPECT_EQ(QueryPermission(GetFrame()), "granted");
This part is already tested in `PermissionGrantedViaDevtools`, so normally I'd say we should skip it here. But in this case, it's nice to show that the two calls to `QueryPermission` give different results, so we know for sure that we're manipulating the right permission.
So to make it clearer that the "denied" part is the part this test cares about, maybe we consider these lines the "setup"?
```suggestion
devtools_client.SendCommandSync(
"Browser.setPermission",
base::Value::Dict()
.Set("setting", "granted")
.Set("origin", kOriginA)
.Set("embeddedOrigin", kOriginB)
.Set("permission",
base::Value::Dict().Set("name", "storage-access")));
// Ensure that we granted the permission.
ASSERT_EQ(QueryPermission(GetFrame()), "granted");
```
Related details
- Jalon Thomas
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Fredrickson (Gerrit)
Chris Fredrickson added 1 comment![]( "Open in Gerrit")
// Ensure that we granted the permission.Nit: this comment doesn't add any information, so I'd remove it. https://testing.googleblog.com/2017/07/code-health-to-comment-or-not-to-comment.html
Jalon Thomas (Gerrit)
Jalon Thomas added 3 comments![]( "Open in Gerrit")
We're not depending on the `echoheader` logic in this test, so we can simplify this.
```suggestion
NavigateFrameTo(kHostB, "/empty.html");
```
Done
Nit: this comment doesn't add any information, so I'd remove it. https://testing.googleblog.com/2017/07/code-health-to-comment-or-not-to-comment.html
Done
devtools_client.SendCommandSync(
"Browser.setPermission",
base::Value::Dict()
.Set("setting", "granted")
.Set("origin", kOriginA)
.Set("embeddedOrigin", kOriginB)
.Set("permission",
base::Value::Dict().Set("name", "storage-access")));
// Ensure that we granted the permission.
EXPECT_EQ(QueryPermission(GetFrame()), "granted");
This part is already tested in `PermissionGrantedViaDevtools`, so normally I'd say we should skip it here. But in this case, it's nice to show that the two calls to `QueryPermission` give different results, so we know for sure that we're manipulating the right permission.
So to make it clearer that the "denied" part is the part this test cares about, maybe we consider these lines the "setup"?
```suggestion
devtools_client.SendCommandSync(
"Browser.setPermission",
base::Value::Dict()
.Set("setting", "granted")
.Set("origin", kOriginA)
.Set("embeddedOrigin", kOriginB)
.Set("permission",
base::Value::Dict().Set("name", "storage-access")));
Jalon Thomas// Ensure that we granted the permission.
ASSERT_EQ(QueryPermission(GetFrame()), "granted");```
Makes sense!
Related details
- Chris Fredrickson
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Fredrickson (Gerrit)
Chris Fredrickson voted Code-Review+1![]( "Open in Gerrit")
- Jalon Thomas
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Fredrickson (Gerrit)
Chris Fredrickson added 1 comment![]( "Open in Gerrit")
return std::make_optional(
PermissionResult(adjusted_status, PermissionStatusSource::UNSPECIFIED));Optional: now that this is no longer in the ternary expression, you can remove the `std::make_optional` call and just return the PermissionResult directly, relying on the implicit conversion to optional. (That also might end up being faster due to copy elision; a C++ expert would know for sure.)
```suggestion
return PermissionResult(adjusted_status, PermissionStatusSource::UNSPECIFIED);
```
Related details
- Elias Klim
- Jalon Thomas
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Elias Klim (Gerrit)
Elias Klim added 1 comment![]( "Open in Gerrit")
AFAIU, these `overrides` are for test only, in other words I do not see how real users would be impacted. It is still a good idea to match specs, I just want to better understand if this was mainly an inconvenience or a real privacy-related issue?
Related details
- Chris Fredrickson
- Jalon Thomas
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Elias Klim (Gerrit)
Elias Klim voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Chris Fredrickson
- Jalon Thomas
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jalon Thomas (Gerrit)
Jalon Thomas added 1 comment![]( "Open in Gerrit")
AFAIU, these `overrides` are for test only, in other words I do not see how real users would be impacted. It is still a good idea to match specs, I just want to better understand if this was mainly an inconvenience or a real privacy-related issue?
That's right, but we need `overrides` to match the spec behavior for web platform tests. For example, this is [the test](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/storage-access-api/resources/permissions-iframe.https.html;l=32) that is fixed with this change. So, making this change is necessary to pass the tests, and it aligns with the actual SAA code [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/storage_access_api/storage_access_grant_permission_context.cc;l=608) because when an override is set, it gets used, instead of calling into `GetContentSettingStatusInternal`. Does that make sense?
Related details
- Chris Fredrickson
- Elias Klim
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chris Fredrickson (Gerrit)
Chris Fredrickson voted Code-Review+1![]( "Open in Gerrit")
- Elias Klim
- Jalon Thomas
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Elias Klim (Gerrit)
Elias Klim added 1 comment![]( "Open in Gerrit")
Jalon ThomasAFAIU, these `overrides` are for test only, in other words I do not see how real users would be impacted. It is still a good idea to match specs, I just want to better understand if this was mainly an inconvenience or a real privacy-related issue?
That's right, but we need `overrides` to match the spec behavior for web platform tests. For example, this is [the test](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/external/wpt/storage-access-api/resources/permissions-iframe.https.html;l=32) that is fixed with this change. So, making this change is necessary to pass the tests, and it aligns with the actual SAA code [here](https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/storage_access_api/storage_access_grant_permission_context.cc;l=608) because when an override is set, it gets used, instead of calling into `GetContentSettingStatusInternal`. Does that make sense?
- Alex N. Jose
- Jalon Thomas
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex N. Jose (Gerrit)
Alex N. Jose added 1 comment![]( "Open in Gerrit")
+ kvitekp@ to reviewer since crrev.com/c/7076875. LGTM from a chromedriver test update pov.
Related details
- Jalon Thomas
- Peter Kvitek
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alex N. Jose (Gerrit)
Alex N. Jose voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Jalon Thomas
- Peter Kvitek
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Peter Kvitek (Gerrit)
Peter Kvitek voted and added 1 comment![]( "Open in Gerrit")
Votes added by Peter Kvitek
| Code-Review | +1 |
1 comment
lgtm
Related details
- Jalon Thomas
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jalon Thomas (Gerrit)
Jalon Thomas voted Commit-Queue+2![]( "Open in Gerrit")
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Mask DENIED status for storage-access permission in overrides
Permission overrides should align with the Storage Access API spec
calling for avoiding exposure of rejections to prevent any attempt at
retaliating against users who would reject a prompt:
https://privacycg.github.io/storage-access/#permissions-integration
- M chrome/browser/storage_access_api/api_browsertest.cc
- M chrome/test/chromedriver/test/run_py_tests.py
- M content/browser/permissions/permission_controller_impl_unittest.cc
- M content/browser/permissions/permission_overrides.cc
- M content/browser/permissions/permission_overrides_unittest.cc
- D third_party/blink/web_tests/external/wpt/storage-access-api/storage-access-permission.sub.https.window-expected.txt
Code-Review: +1 by Peter Kvitek, +1 by Elias Klim, +1 by Alex N. Jose, +1 by Chris Fredrickson
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |