[FSA] Update `queryPermission()` to include persistent permission state.
Previously, even if a handle has persistent permission,
`queryPermission()` could return `PermissionStatus::ASK`; only when
`requestPermission()` is called, the persistent permission status would
be retrieved for auto-granting. However, this could be a confusing
behavior to API users, as the persistence of permission is
implementation detail.
`FileSystemAccessPermissionGrant.GetStatus()` is now updated to read
from persisted grant objects and to return the combined state between
active and persisted grants. This method is used not only for
`queryPermission()` API, but also in various places internally such as
returning child handles from a parent directory handle.
Instead of introducing a new `GetPersistedStatus()`, the existing
`GetStatus()` is updated to account for persistent permission.
Additionally, a new private method `GetActivePermissionStatus()` is
introduced to replace the former usage of `GetStatus()` within
`ChromeFileSystemAccessPermissionContext`.
Bug: 1011533
Change-Id: Ica5ca4f74395c6ccf66235f022b8ce345440b9d2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4989830
Commit-Queue: Daseul Lee <dslee@chromium.org>
Reviewed-by: Christine Smith <christinesm@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1217870}
diff --git a/chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc b/chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc
index 3849975..adbc96e4 100644
--- a/chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc
+++ b/chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc
@@ -546,8 +546,21 @@
// FileSystemAccessPermissionGrant:
PermissionStatus GetStatus() override {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+ // TODO(crbug.com/1011533): Determine if this should return denied for
+ // guard block, and how ancestor permission should be handled.
+ if (status_ == PermissionStatus::ASK &&
+ context_->CanAutoGrantViaPersistentPermission(origin_, path_,
+ handle_type_, type_)) {
+ return PermissionStatus::GRANTED;
+ }
return status_;
}
+
+ PermissionStatus GetActivePermissionStatus() {
+ DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+ return status_;
+ }
+
base::FilePath GetPath() override {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
return path_;
@@ -562,8 +575,8 @@
// Check if a permission request has already been processed previously. This
// check is done first because we don't want to reset the status of a
// permission if it has already been granted.
- if (GetStatus() != PermissionStatus::ASK || !context_) {
- if (GetStatus() == PermissionStatus::GRANTED) {
+ if (GetActivePermissionStatus() != PermissionStatus::ASK || !context_) {
+ if (GetActivePermissionStatus() == PermissionStatus::GRANTED) {
SetStatus(PermissionStatus::GRANTED,
PersistedPermissionOptions::kUpdatePersistedPermission);
}
@@ -829,7 +842,8 @@
return;
}
- DCHECK_EQ(entry_it->second->GetStatus(), PermissionStatus::GRANTED);
+ DCHECK_EQ(entry_it->second->GetActivePermissionStatus(),
+ PermissionStatus::GRANTED);
auto* const grant_impl = entry_it->second;
grant_impl->SetPath(new_path);
@@ -1205,7 +1219,8 @@
break;
}
- if (existing_grant->GetStatus() == PermissionStatus::GRANTED) {
+ if (existing_grant->GetActivePermissionStatus() ==
+ PermissionStatus::GRANTED) {
ScheduleUsageIconUpdate();
}
@@ -1291,7 +1306,8 @@
break;
}
- if (existing_grant->GetStatus() == PermissionStatus::GRANTED) {
+ if (existing_grant->GetActivePermissionStatus() ==
+ PermissionStatus::GRANTED) {
ScheduleUsageIconUpdate();
}
@@ -1334,7 +1350,8 @@
auto it = active_permissions_map_.find(origin);
if (it != active_permissions_map_.end()) {
for (const auto& grant : it->second.read_grants) {
- if (grant.second->GetStatus() == PermissionStatus::GRANTED) {
+ if (grant.second->GetActivePermissionStatus() ==
+ PermissionStatus::GRANTED) {
auto value = grant.second->AsValue();
// Persisted permissions include both read and write information in
@@ -1343,7 +1360,8 @@
auto file_path = grant.first;
auto write_grant_it = it->second.write_grants.find(file_path);
if (write_grant_it != it->second.write_grants.end() &&
- write_grant_it->second->GetStatus() == PermissionStatus::GRANTED) {
+ write_grant_it->second->GetActivePermissionStatus() ==
+ PermissionStatus::GRANTED) {
value.Set(kPermissionWritableKey, true);
}
@@ -1838,14 +1856,16 @@
if (origin_it != active_permissions_map_.end()) {
OriginState& origin_state = origin_it->second;
for (auto& read_grant : origin_state.read_grants) {
- if (read_grant.second->GetStatus() == PermissionStatus::GRANTED) {
+ if (read_grant.second->GetActivePermissionStatus() ==
+ PermissionStatus::GRANTED) {
read_grant.second->SetStatus(
PermissionStatus::GRANTED,
PersistedPermissionOptions::kUpdatePersistedPermission);
}
}
for (auto& write_grant : origin_state.write_grants) {
- if (write_grant.second->GetStatus() == PermissionStatus::GRANTED) {
+ if (write_grant.second->GetActivePermissionStatus() ==
+ PermissionStatus::GRANTED) {
write_grant.second->SetStatus(
PermissionStatus::GRANTED,
PersistedPermissionOptions::kUpdatePersistedPermission);
@@ -1914,7 +1934,8 @@
auto it = active_permissions_map_.find(origin);
if (it != active_permissions_map_.end()) {
return base::ranges::any_of(it->second.read_grants, [&](const auto& grant) {
- return grant.second->GetStatus() == PermissionStatus::GRANTED;
+ return grant.second->GetActivePermissionStatus() ==
+ PermissionStatus::GRANTED;
});
}
@@ -1943,7 +1964,8 @@
if (it != active_permissions_map_.end()) {
return base::ranges::any_of(
it->second.write_grants, [&](const auto& grant) {
- return grant.second->GetStatus() == PermissionStatus::GRANTED;
+ return grant.second->GetActivePermissionStatus() ==
+ PermissionStatus::GRANTED;
});
}
@@ -2304,7 +2326,7 @@
parent = parent.DirName()) {
auto i = relevant_grants.find(parent);
if (i != relevant_grants.end() && i->second &&
- i->second->GetStatus() == PermissionStatus::GRANTED) {
+ i->second->GetActivePermissionStatus() == PermissionStatus::GRANTED) {
return true;
}
}
@@ -2612,7 +2634,7 @@
// be granted but won't be visible in any UI because the permission context
// isn't tracking them anymore.
if (grant_it == grants.end()) {
- DCHECK_EQ(PermissionStatus::DENIED, grant->GetStatus());
+ DCHECK_EQ(PermissionStatus::DENIED, grant->GetActivePermissionStatus());
return;
}
diff --git a/chrome/browser/file_system_access/chrome_file_system_access_permission_context_unittest.cc b/chrome/browser/file_system_access/chrome_file_system_access_permission_context_unittest.cc
index 7e656b1..4f4c175a 100644
--- a/chrome/browser/file_system_access/chrome_file_system_access_permission_context_unittest.cc
+++ b/chrome/browser/file_system_access/chrome_file_system_access_permission_context_unittest.cc
@@ -1059,11 +1059,7 @@
EXPECT_TRUE(permission_context()->HasExtendedPermissionForTesting(
kTestOrigin, kTestPath, HandleType::kFile, GrantType::kWrite));
- grant.reset();
- // Permission should not be granted for `kOpen`.
- grant = permission_context()->GetWritePermissionGrant(
- kTestOrigin, kTestPath, HandleType::kFile, UserAction::kOpen);
- EXPECT_EQ(grant->GetStatus(), PermissionStatus::ASK);
+ permission_context()->RevokeActiveGrantsForTesting(kTestOrigin, kTestPath);
// Permission should be auto-granted here via the persisted permission.
base::test::TestFuture<PermissionRequestOutcome> future;
@@ -1141,7 +1137,6 @@
// Revoke the active grant, but not the persisted permission. The granted
// object for the given origin is not revoked.
permission_context()->RevokeActiveGrantsForTesting(kTestOrigin);
- EXPECT_EQ(grant->GetStatus(), PermissionStatus::ASK);
ASSERT_THAT(
permission_context()->GetExtendedPersistedObjectsForTesting(kTestOrigin),
testing::SizeIs(1));
@@ -2041,25 +2036,6 @@
}
TEST_F(ChromeFileSystemAccessPermissionContextTest,
- PersistedPermission_RevokeOnlyActiveGrants) {
- permission_context()->SetOriginHasExtendedPermissionForTesting(kTestOrigin);
- auto grant = permission_context()->GetWritePermissionGrant(
- kTestOrigin, kTestPath, HandleType::kFile, UserAction::kSave);
- EXPECT_EQ(grant->GetStatus(), PermissionStatus::GRANTED);
- // Revoke active grant, but not the persisted permission.
- permission_context()->RevokeActiveGrantsForTesting(kTestOrigin);
- EXPECT_EQ(grant->GetStatus(), PermissionStatus::ASK);
- EXPECT_TRUE(permission_context()->HasExtendedPermissionForTesting(
- kTestOrigin, kTestPath, HandleType::kFile, GrantType::kWrite));
-
- ChromeFileSystemAccessPermissionContext::Grants grants =
- permission_context()->ConvertObjectsToGrants(
- permission_context()->GetGrantedObjects(kTestOrigin));
- std::vector<base::FilePath> expected_res = {kTestPath};
- EXPECT_EQ(grants.file_write_grants, expected_res);
-}
-
-TEST_F(ChromeFileSystemAccessPermissionContextTest,
PersistedPermission_RevokeGrantByFilePath) {
auto grant = permission_context()->GetWritePermissionGrant(
kTestOrigin, kTestPath, HandleType::kFile, UserAction::kSave);
@@ -2080,15 +2056,7 @@
auto grant = permission_context()->GetWritePermissionGrant(
kTestOrigin, kTestPath, HandleType::kFile, UserAction::kSave);
EXPECT_EQ(grant->GetStatus(), PermissionStatus::GRANTED);
- grant.reset();
- EXPECT_TRUE(permission_context()->HasExtendedPermissionForTesting(
- kTestOrigin, kTestPath, HandleType::kFile, GrantType::kWrite));
-
- // After reset, the grant should be cleared, and the new grant request should
- // be in the `ASK` state.
- grant = permission_context()->GetWritePermissionGrant(
- kTestOrigin, kTestPath, HandleType::kFile, UserAction::kOpen);
- EXPECT_EQ(grant->GetStatus(), PermissionStatus::ASK);
+ permission_context()->RevokeActiveGrantsForTesting(kTestOrigin, kTestPath);
SetDefaultContentSettingValue(ContentSettingsType::FILE_SYSTEM_WRITE_GUARD,
CONTENT_SETTING_BLOCK);
