Skip to content

Commit

Permalink
Return errors immediately on read-only roots.
Browse files Browse the repository at this point in the history 
If write-related functions are called on read-only roots, we can return
errors immediately instead of making mojo calls which always fail.

Bug: 956852
Test: Ran unit_tests
Change-Id: I81bbe3aea12f683ecf7dc89db698983d35202a67
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2346101
Reviewed-by: Shuhei Takahashi <nya@chromium.org>
Commit-Queue: Naoki Fukino <fukino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796740}
  • Loading branch information
Naoki Fukino authored and Commit Bot committed Aug 11, 2020
1 parent ca633aa commit b1b0136
Show file tree
Hide file tree
Showing 3 changed files with 161 additions and 0 deletions.
20 changes: 20 additions & 0 deletions chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,10 @@ void ArcDocumentsProviderRoot::ReadDirectory(const base::FilePath& path,
void ArcDocumentsProviderRoot::DeleteFile(const base::FilePath& path,
StatusCallback callback) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
if (read_only_) {
std::move(callback).Run(base::File::FILE_ERROR_ACCESS_DENIED);
return;
}
ResolveToDocumentId(
path,
base::BindOnce(&ArcDocumentsProviderRoot::DeleteFileWithDocumentId,
Expand All @@ -150,6 +154,10 @@ void ArcDocumentsProviderRoot::DeleteFile(const base::FilePath& path,
void ArcDocumentsProviderRoot::CreateFile(const base::FilePath& path,
StatusCallback callback) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
if (read_only_) {
std::move(callback).Run(base::File::FILE_ERROR_ACCESS_DENIED);
return;
}
ResolveToDocumentId(
path, base::BindOnce(
&ArcDocumentsProviderRoot::CreateFileAfterConflictCheck,
Expand All @@ -159,6 +167,10 @@ void ArcDocumentsProviderRoot::CreateFile(const base::FilePath& path,
void ArcDocumentsProviderRoot::CreateDirectory(const base::FilePath& path,
StatusCallback callback) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
if (read_only_) {
std::move(callback).Run(base::File::FILE_ERROR_ACCESS_DENIED);
return;
}
ResolveToDocumentId(
path, base::BindOnce(
&ArcDocumentsProviderRoot::CreateDirectoryAfterConflictCheck,
Expand All @@ -169,6 +181,10 @@ void ArcDocumentsProviderRoot::CopyFileLocal(const base::FilePath& src_path,
const base::FilePath& dest_path,
StatusCallback callback) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
if (read_only_) {
std::move(callback).Run(base::File::FILE_ERROR_ACCESS_DENIED);
return;
}
if (src_path == dest_path) {
std::move(callback).Run(base::File::FILE_ERROR_INVALID_OPERATION);
return;
Expand All @@ -184,6 +200,10 @@ void ArcDocumentsProviderRoot::MoveFileLocal(const base::FilePath& src_path,
const base::FilePath& dest_path,
StatusCallback callback) {
DCHECK_CURRENTLY_ON(BrowserThread::UI);
if (read_only_) {
std::move(callback).Run(base::File::FILE_ERROR_ACCESS_DENIED);
return;
}
if (src_path.DirName() == dest_path.DirName()) {
RenameFileInternal(src_path, dest_path.BaseName().value(),
std::move(callback));
Expand Down
7 changes: 7 additions & 0 deletions chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,27 +86,33 @@ class ArcDocumentsProviderRoot : public ArcFileSystemOperationRunner::Observer {
ReadDirectoryCallback callback);

// Deletes a file/directory at the given path.
//
// - File::FILE_ERROR_NOT_FOUND if |path| does not exist.
// - File::FILE_ERROR_ACCESS_DENIED if this root is read-only.
void DeleteFile(const base::FilePath& path, StatusCallback callback);

// Creates a file at the given path.
//
// This reports following error code via |callback|:
// - File::FILE_ERROR_NOT_FOUND if |path|'s parent directory does not exist.
// - File::FILE_ERROR_EXISTS if a file already exists at |path|.
// - File::FILE_ERROR_ACCESS_DENIED if this root is read-only.
void CreateFile(const base::FilePath& path, StatusCallback callback);

// Creates a directory at the given path.
//
// This reports following error code via |callback|:
// - File::FILE_ERROR_NOT_FOUND if |path|'s parent directory does not exist.
// - File::FILE_ERROR_EXISTS if a file already exists at |path|.
// - File::FILE_ERROR_ACCESS_DENIED if this root is read-only.
void CreateDirectory(const base::FilePath& path, StatusCallback callback);

// Copies a file from |src_path| to |dest_path| inside this root.
//
// This reports following error code via |callback|:
// - File::FILE_ERROR_NOT_FOUND if |src_path| or the parent directory of
// |dest_path| does not exist.
// - File::FILE_ERROR_ACCESS_DENIED if this root is read-only.
void CopyFileLocal(const base::FilePath& src_path,
const base::FilePath& dest_path,
StatusCallback callback);
Expand All @@ -116,6 +122,7 @@ class ArcDocumentsProviderRoot : public ArcFileSystemOperationRunner::Observer {
// This reports following error code via |callback|:
// - File::FILE_ERROR_NOT_FOUND if |src_path| or the parent directory of
// |dest_path| does not exist.
// - File::FILE_ERROR_ACCESS_DENIED if this root is read-only.
void MoveFileLocal(const base::FilePath& src_path,
const base::FilePath& dest_path,
StatusCallback callback);
Expand Down
134 changes: 134 additions & 0 deletions chrome/browser/chromeos/arc/fileapi/arc_documents_provider_root_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,6 +186,10 @@ class ArcDocumentsProviderRootTest : public testing::Test {
ArcFileSystemOperationRunner::GetForBrowserContext(profile_.get()),
kAuthority, kRootSpec.document_id, "", false,
std::vector<std::string>());
read_only_root_ = std::make_unique<ArcDocumentsProviderRoot>(
ArcFileSystemOperationRunner::GetForBrowserContext(profile_.get()),
kAuthority, kRootSpec.document_id, "read-only", true,
std::vector<std::string>());
}

void TearDown() override {
Expand All @@ -206,6 +210,7 @@ class ArcDocumentsProviderRootTest : public testing::Test {
std::unique_ptr<ArcServiceManager> arc_service_manager_;
std::unique_ptr<TestingProfile> profile_;
std::unique_ptr<ArcDocumentsProviderRoot> root_;
std::unique_ptr<ArcDocumentsProviderRoot> read_only_root_;

private:
DISALLOW_COPY_AND_ASSIGN(ArcDocumentsProviderRootTest);
Expand Down Expand Up @@ -639,6 +644,25 @@ TEST_F(ArcDocumentsProviderRootTest, DeleteDirectory) {
base::FilePath(FILE_PATH_LITERAL("dir"))));
}

TEST_F(ArcDocumentsProviderRootTest, DeleteFileOnReadOnlyRoot) {
base::RunLoop run_loop;
read_only_root_->DeleteFile(
base::FilePath(FILE_PATH_LITERAL("dir/photo.jpg")),
base::BindOnce(
[](base::RunLoop* run_loop, base::File::Error error) {
run_loop->Quit();
// An attempt to delete a file on read-only root
// should return FILE_ERROR_ACCESS_DENIED error.
EXPECT_EQ(base::File::FILE_ERROR_ACCESS_DENIED, error);
},
&run_loop));
run_loop.Run();
// dir/photo.jpg should not have been removed.
EXPECT_TRUE(fake_file_system_.DocumentExists(
kAuthority, kRootSpec.document_id,
base::FilePath(FILE_PATH_LITERAL("dir/photo.jpg"))));
}

TEST_F(ArcDocumentsProviderRootTest, CreateFile) {
// Make sure that dir/new.txt doesn't exist.
ASSERT_FALSE(fake_file_system_.DocumentExists(
Expand Down Expand Up @@ -751,6 +775,25 @@ TEST_F(ArcDocumentsProviderRootTest, CreateFileInReadOnlyDirectory) {
base::FilePath(FILE_PATH_LITERAL("dir3/photo.jpg"))));
}

TEST_F(ArcDocumentsProviderRootTest, CreateFileOnReadOnlyRoot) {
base::RunLoop run_loop;
read_only_root_->CreateFile(
base::FilePath(FILE_PATH_LITERAL("dir/new.txt")),
base::BindOnce(
[](base::RunLoop* run_loop, base::File::Error error) {
run_loop->Quit();
// An attempt to create a file on read-only root should return
// FILE_ERROR_ACCESS_DENIED error.
EXPECT_EQ(base::File::FILE_ERROR_ACCESS_DENIED, error);
},
&run_loop));
run_loop.Run();
// The dir/new.txt should not have been created.
EXPECT_FALSE(fake_file_system_.DocumentExists(
kAuthority, kRootSpec.document_id,
base::FilePath(FILE_PATH_LITERAL("dir/new.txt"))));
}

TEST_F(ArcDocumentsProviderRootTest, CreateDirectory) {
// Make sure that directory "dir2" doesn't exist.
ASSERT_FALSE(fake_file_system_.DocumentExists(
Expand Down Expand Up @@ -821,6 +864,25 @@ TEST_F(ArcDocumentsProviderRootTest, CreateDirectoryParentNotFound) {
base::FilePath(FILE_PATH_LITERAL("dir3/new_dir"))));
}

TEST_F(ArcDocumentsProviderRootTest, CreateDirectoryOnReadOnlyRoot) {
base::RunLoop run_loop;
read_only_root_->CreateDirectory(
base::FilePath(FILE_PATH_LITERAL("dir2")),
base::BindOnce(
[](base::RunLoop* run_loop, base::File::Error error) {
run_loop->Quit();
// An attempt to create a directory on read-only root should return
// FILE_ERROR_ACCESS_DENIED error.
EXPECT_EQ(base::File::FILE_ERROR_ACCESS_DENIED, error);
},
&run_loop));
run_loop.Run();
// The dir2 should not have been created.
EXPECT_FALSE(fake_file_system_.DocumentExists(
kAuthority, kRootSpec.document_id,
base::FilePath(FILE_PATH_LITERAL("dir2"))));
}

TEST_F(ArcDocumentsProviderRootTest, CopyFile) {
base::RunLoop run_loop;
root_->CopyFileLocal(
Expand Down Expand Up @@ -893,6 +955,30 @@ TEST_F(ArcDocumentsProviderRootTest, CopyFileDestParentNotFound) {
run_loop.Run();
}

TEST_F(ArcDocumentsProviderRootTest, CopyFileOnReadOnlyRoot) {
base::RunLoop run_loop;
read_only_root_->CopyFileLocal(
base::FilePath(FILE_PATH_LITERAL("dir/photo.jpg")),
base::FilePath(FILE_PATH_LITERAL("dir/photo2.jpg")),
base::BindOnce(
[](base::RunLoop* run_loop, base::File::Error error) {
run_loop->Quit();
// An attempt to copy a directory on read-only root should return
// FILE_ERROR_ACCESS_DENIED error.
EXPECT_EQ(base::File::FILE_ERROR_ACCESS_DENIED, error);
},
&run_loop));
run_loop.Run();
// dir/photo2.jpg should not be created by the copy.
EXPECT_FALSE(fake_file_system_.DocumentExists(
kAuthority, kRootSpec.document_id,
base::FilePath(FILE_PATH_LITERAL("dir/photo2.jpg"))));
// The source file should still be there.
EXPECT_TRUE(fake_file_system_.DocumentExists(
kAuthority, kRootSpec.document_id,
base::FilePath(FILE_PATH_LITERAL("dir/photo.jpg"))));
}

TEST_F(ArcDocumentsProviderRootTest, RenameFile) {
base::RunLoop run_loop;
root_->MoveFileLocal(
Expand Down Expand Up @@ -948,6 +1034,30 @@ TEST_F(ArcDocumentsProviderRootTest, RenameFileNotRenamable) {
base::FilePath(FILE_PATH_LITERAL("dir/no-rename2.jpg"))));
}

TEST_F(ArcDocumentsProviderRootTest, RenameFileOnReadOnlyRoot) {
base::RunLoop run_loop;
read_only_root_->MoveFileLocal(
base::FilePath(FILE_PATH_LITERAL("dir/photo.jpg")),
base::FilePath(FILE_PATH_LITERAL("dir/photo2.jpg")),
base::BindOnce(
[](base::RunLoop* run_loop, base::File::Error error) {
run_loop->Quit();
// An attempt to rename a file on read-only root should return
// FILE_ERROR_ACCESS_DENIED error.
EXPECT_EQ(base::File::FILE_ERROR_ACCESS_DENIED, error);
},
&run_loop));
run_loop.Run();
// dir/no-rename.jpg should still be there.
EXPECT_TRUE(fake_file_system_.DocumentExists(
kAuthority, kRootSpec.document_id,
base::FilePath(FILE_PATH_LITERAL("dir/no-rename.jpg"))));
// dir/no-rename2.jpg shouldn't be there".
EXPECT_FALSE(fake_file_system_.DocumentExists(
kAuthority, kRootSpec.document_id,
base::FilePath(FILE_PATH_LITERAL("dir/no-rename2.jpg"))));
}

TEST_F(ArcDocumentsProviderRootTest, MoveFile) {
base::RunLoop run_loop;
root_->MoveFileLocal(
Expand Down Expand Up @@ -1032,6 +1142,30 @@ TEST_F(ArcDocumentsProviderRootTest, MoveFileDestParentNotFound) {
run_loop.Run();
}

TEST_F(ArcDocumentsProviderRootTest, MoveFileOnReadOnlyRoot) {
base::RunLoop run_loop;
read_only_root_->MoveFileLocal(
base::FilePath(FILE_PATH_LITERAL("dir/photo.jpg")),
base::FilePath(FILE_PATH_LITERAL("photo.jpg")),
base::BindOnce(
[](base::RunLoop* run_loop, base::File::Error error) {
run_loop->Quit();
// An attempt to move a file on read-only root should return
// FILE_ERROR_ACCESS_DENIED error.
EXPECT_EQ(base::File::FILE_ERROR_ACCESS_DENIED, error);
},
&run_loop));
run_loop.Run();
// The destination file should not be created.
EXPECT_FALSE(fake_file_system_.DocumentExists(
kAuthority, kRootSpec.document_id,
base::FilePath(FILE_PATH_LITERAL("photo.jpg"))));
// The source file should not be gone by move.
EXPECT_TRUE(fake_file_system_.DocumentExists(
kAuthority, kRootSpec.document_id,
base::FilePath(FILE_PATH_LITERAL("dir/photo.jpg"))));
}

TEST_F(ArcDocumentsProviderRootTest, WatchChanged) {
int num_called = 0;
auto watcher_callback = base::Bind(
Expand Down

0 comments on commit b1b0136

Please sign in to comment.