Commit 319a7c9a authored by Marijn Kruisselbrink's avatar Marijn Kruisselbrink Committed by Commit Bot

[NativeFS] Refactor permission request dialog for new permission model.

This changes NativeFileSystemPermissionView to not only support asking
for write access, but to also support asking for read access (as well
as asking for read and write access at the same time).

Also changes the origin scoped permission model to go through this new
code path when asking for read access.

This does not hook anything up yet to ask for read and write access at
the same time (that will still DCHECK in the content layer), dealing
with that case will be handled in a follow-up CL.

Bug: 984769
Change-Id: I5fc276b11802e105379ae42e0f6abf84f2c7d81f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2091920Reviewed-by: default avatarOlivier Yiptong <oyiptong@chromium.org>
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#747954}
parent 4620e94a
......@@ -10024,9 +10024,36 @@ Please help our engineers fix this problem. Tell us what happened right before y
<message name="IDS_NATIVE_FILE_SYSTEM_ORIGIN_SCOPED_WRITE_PERMISSION_DIRECTORY_TEXT" desc="Text of the dialog for confirming origin scoped write access to a directory using the Native File System API">
<ph name="ORIGIN">$1<ex>example.com</ex></ph> will be able to edit files in <ph name="FOLDERNAME">$2<ex>My Project</ex></ph> until you close all <ph name="ORIGIN">$1</ph> tabs
</message>
<message name="IDS_NATIVE_FILE_SYSTEM_ORIGIN_SCOPED_READ_PERMISSION_FILE_TEXT" desc="Text of dialog for confirming read access to files using the Native File System API">
<ph name="ORIGIN">$1<ex>example.com</ex></ph> will be able to view <ph name="FILENAME">$2<ex>README.md</ex></ph> until you close all <ph name="ORIGIN">$1</ph> tabs
</message>
<message name="IDS_NATIVE_FILE_SYSTEM_ORIGIN_SCOPED_READ_PERMISSION_DIRECTORY_TEXT" desc="Text of dialog asking user if they intended to share a particular directory">
<ph name="ORIGIN">$1<ex>example.com</ex></ph> will be able to view files in <ph name="FOLDERNAME">$2<ex>My Project</ex></ph> until you close all <ph name="ORIGIN">$1</ph> tabs
</message>
<message name="IDS_NATIVE_FILE_SYSTEM_EDIT_FILE_PERMISSION_TITLE" desc="Title of dialog asking user to confirm giving read and write access to a file using the Native File System API">
Let site edit <ph name="FILE_NAME">$1<ex>README.md</ex></ph>?
</message>
<message name="IDS_NATIVE_FILE_SYSTEM_EDIT_DIRECTORY_PERMISSION_TITLE" desc="Title of dialog asking user to confirm giving read and write access to (files in) a directory using the Native File System API">
Let site edit files?
</message>
<message name="IDS_NATIVE_FILE_SYSTEM_READ_FILE_PERMISSION_TITLE" desc="Title of dialog asking user to confirm giving read access to a file using the Native File System API">
Let site view <ph name="FILE_NAME">$1<ex>README.md</ex></ph>?
</message>
<message name="IDS_NATIVE_FILE_SYSTEM_READ_DIRECTORY_PERMISSION_TITLE" desc="Title of dialog asking user if they intended to share a particular directory">
Let site view files?
</message>
<message name="IDS_NATIVE_FILE_SYSTEM_EDIT_FILE_PERMISSION_ALLOW_TEXT" desc="Text of the 'allow' button in the dialog for confirming write access to a file using the Native File System API">
Edit file
</message>
<message name="IDS_NATIVE_FILE_SYSTEM_EDIT_DIRECTORY_PERMISSION_ALLOW_TEXT" desc="Text of the 'allow' button in the dialog for confirming write access to a directory (and the files in it) using the Native File System API">
Edit files
</message>
<message name="IDS_NATIVE_FILE_SYSTEM_VIEW_FILE_PERMISSION_ALLOW_TEXT" desc="Text of the 'allow' button in the dialog for confirming read access to a file using the Native File System API">
View file
</message>
<message name="IDS_NATIVE_FILE_SYSTEM_VIEW_DIRECTORY_PERMISSION_ALLOW_TEXT" desc="Text of the 'allow' button in the dialog for confirming read access to a directory (and the files in it) using the Native File System API">
View files
</message>
<!-- Native File System usage indicator -->
<message name="IDS_NATIVE_FILE_SYSTEM_WRITE_USAGE_TOOLTIP" desc="Tooltip for the omnibox Native File System API file write access usage indicator.">
......
6800810c482a6773c02b5c677ac82b6f19bd2555
\ No newline at end of file
1a079cf2c36bcdc1485ec3981690223944f591b8
\ No newline at end of file
1a079cf2c36bcdc1485ec3981690223944f591b8
\ No newline at end of file
48d7d2be0c371d05c376b604e46784279bf2e6a1
\ No newline at end of file
5d6fba335cb335d431ceb56b3ba4dcdcd79e27ae
\ No newline at end of file
5d6fba335cb335d431ceb56b3ba4dcdcd79e27ae
\ No newline at end of file
......@@ -11,13 +11,17 @@
#include "components/permissions/switches.h"
#include "content/public/browser/browser_task_traits.h"
bool operator==(
namespace {
bool RequestsAreIdentical(
const NativeFileSystemPermissionRequestManager::RequestData& a,
const NativeFileSystemPermissionRequestManager::RequestData& b) {
return a.origin == b.origin && a.path == b.path &&
a.is_directory == b.is_directory;
a.is_directory == b.is_directory && a.access == b.access;
}
} // namespace
struct NativeFileSystemPermissionRequestManager::Request {
Request(
RequestData data,
......@@ -44,12 +48,12 @@ void NativeFileSystemPermissionRequestManager::AddRequest(
}
// Check if any pending requests are identical to the new request.
if (current_request_ && current_request_->data == data) {
if (current_request_ && RequestsAreIdentical(current_request_->data, data)) {
current_request_->callbacks.push_back(std::move(callback));
return;
}
for (const auto& request : queued_requests_) {
if (request->data == data) {
if (RequestsAreIdentical(request->data, data)) {
request->callbacks.push_back(std::move(callback));
return;
}
......@@ -96,8 +100,7 @@ void NativeFileSystemPermissionRequestManager::DequeueAndShowRequest() {
}
ShowNativeFileSystemPermissionDialog(
current_request_->data.origin, current_request_->data.path,
current_request_->data.is_directory,
current_request_->data,
base::BindOnce(
&NativeFileSystemPermissionRequestManager::OnPermissionDialogResult,
weak_factory_.GetWeakPtr()),
......
......@@ -33,17 +33,33 @@ class NativeFileSystemPermissionRequestManager
public:
~NativeFileSystemPermissionRequestManager() override;
enum class Access {
// Only ask for read access.
kRead,
// Only ask for write access, assuming read access has already been granted.
kWrite,
// Ask for both read and write access.
kReadWrite
};
struct RequestData {
RequestData(const url::Origin& origin,
const base::FilePath& path,
bool is_directory)
: origin(origin), path(path), is_directory(is_directory) {}
bool is_directory,
Access access)
: origin(origin),
path(path),
is_directory(is_directory),
access(access) {}
RequestData(RequestData&&) = default;
RequestData(const RequestData&) = default;
RequestData& operator=(RequestData&&) = default;
RequestData& operator=(const RequestData&) = default;
url::Origin origin;
base::FilePath path;
bool is_directory;
Access access;
};
void AddRequest(
......
......@@ -138,26 +138,16 @@ class OriginScopedNativeFileSystemPermissionContext::PermissionGrantImpl
// Drop fullscreen mode so that the user sees the URL bar.
web_contents->ForSecurityDropFullscreen();
if (type_ == GrantType::kRead) {
if (!is_directory_) {
// TODO(mek): Implement requesting read permissions for files.
RunCallbackAndRecordPermissionRequestOutcome(
std::move(callback), PermissionRequestOutcome::kRequestAborted);
return;
}
NativeFileSystemPermissionRequestManager::Access access =
type_ == GrantType::kRead
? NativeFileSystemPermissionRequestManager::Access::kRead
: NativeFileSystemPermissionRequestManager::Access::kWrite;
// TODO(mek): Handle directory read access prompting in RequestManager.
ShowNativeFileSystemDirectoryAccessConfirmationDialog(
origin_, path_,
base::BindOnce(&PermissionGrantImpl::OnPermissionRequestResult, this,
std::move(callback)),
web_contents);
return;
}
// TODO(mek): We need to somehow deal with the case where both read and
// write access are being asked for at the same time.
request_manager->AddRequest(
{origin_, path_, is_directory_},
{origin_, path_, is_directory_, access},
base::BindOnce(&PermissionGrantImpl::OnPermissionRequestResult, this,
std::move(callback)));
}
......
......@@ -89,7 +89,8 @@ void ShowWritePermissionPromptOnUIThread(
web_contents->ForSecurityDropFullscreen();
request_manager->AddRequest(
{origin, path, is_directory},
{origin, path, is_directory,
NativeFileSystemPermissionRequestManager::Access::kWrite},
base::BindOnce(
[](base::OnceCallback<void(PermissionRequestOutcome outcome,
permissions::PermissionAction result)>
......
......@@ -8,9 +8,7 @@
#if !defined(TOOLKIT_VIEWS)
void ShowNativeFileSystemPermissionDialog(
const url::Origin& origin,
const base::FilePath& path,
bool is_directory,
const NativeFileSystemPermissionRequestManager::RequestData& request,
base::OnceCallback<void(permissions::PermissionAction result)> callback,
content::WebContents* web_contents) {
// There's no dialog version of this available outside views, run callback as
......
......@@ -10,6 +10,7 @@
#include "base/callback.h"
#include "build/build_config.h"
#include "chrome/browser/native_file_system/native_file_system_permission_request_manager.h"
#include "content/public/browser/native_file_system_permission_context.h"
namespace base {
......@@ -31,9 +32,7 @@ class Origin;
// Displays a dialog to ask for write access to the given file or directory for
// the native file system API.
void ShowNativeFileSystemPermissionDialog(
const url::Origin& origin,
const base::FilePath& path,
bool is_directory,
const NativeFileSystemPermissionRequestManager::RequestData& request,
base::OnceCallback<void(permissions::PermissionAction result)> callback,
content::WebContents* web_contents);
......
......@@ -21,16 +21,60 @@
#include "ui/views/controls/label.h"
#include "ui/views/layout/box_layout.h"
namespace {
using AccessType = NativeFileSystemPermissionRequestManager::Access;
int GetMessageText(const NativeFileSystemPermissionView::Request& request) {
if (!base::FeatureList::IsEnabled(
features::kNativeFileSystemOriginScopedPermissions)) {
// With tab scoped permission model this dialog is only used for write
// access.
return request.is_directory
? IDS_NATIVE_FILE_SYSTEM_WRITE_PERMISSION_DIRECTORY_TEXT
: IDS_NATIVE_FILE_SYSTEM_WRITE_PERMISSION_FILE_TEXT;
}
switch (request.access) {
case AccessType::kRead:
return request.is_directory
? IDS_NATIVE_FILE_SYSTEM_ORIGIN_SCOPED_READ_PERMISSION_DIRECTORY_TEXT
: IDS_NATIVE_FILE_SYSTEM_ORIGIN_SCOPED_READ_PERMISSION_FILE_TEXT;
case AccessType::kWrite:
case AccessType::kReadWrite:
// Only difference between write and read-write access dialog is in button
// label and dialog title.
return request.is_directory
? IDS_NATIVE_FILE_SYSTEM_ORIGIN_SCOPED_WRITE_PERMISSION_DIRECTORY_TEXT
: IDS_NATIVE_FILE_SYSTEM_ORIGIN_SCOPED_WRITE_PERMISSION_FILE_TEXT;
}
NOTREACHED();
}
int GetButtonLabel(const NativeFileSystemPermissionView::Request& request) {
switch (request.access) {
case AccessType::kRead:
return request.is_directory
? IDS_NATIVE_FILE_SYSTEM_VIEW_DIRECTORY_PERMISSION_ALLOW_TEXT
: IDS_NATIVE_FILE_SYSTEM_VIEW_FILE_PERMISSION_ALLOW_TEXT;
case AccessType::kWrite:
return IDS_NATIVE_FILE_SYSTEM_WRITE_PERMISSION_ALLOW_TEXT;
case AccessType::kReadWrite:
return request.is_directory
? IDS_NATIVE_FILE_SYSTEM_EDIT_DIRECTORY_PERMISSION_ALLOW_TEXT
: IDS_NATIVE_FILE_SYSTEM_EDIT_FILE_PERMISSION_ALLOW_TEXT;
}
NOTREACHED();
}
} // namespace
NativeFileSystemPermissionView::NativeFileSystemPermissionView(
const url::Origin& origin,
const base::FilePath& path,
bool is_directory,
const Request& request,
base::OnceCallback<void(permissions::PermissionAction result)> callback)
: path_(path), callback_(std::move(callback)) {
: request_(request), callback_(std::move(callback)) {
DialogDelegate::set_button_label(
ui::DIALOG_BUTTON_OK,
l10n_util::GetStringUTF16(
IDS_NATIVE_FILE_SYSTEM_WRITE_PERMISSION_ALLOW_TEXT));
l10n_util::GetStringUTF16(GetButtonLabel(request_)));
auto run_callback = [](NativeFileSystemPermissionView* dialog,
permissions::PermissionAction result) {
......@@ -52,19 +96,10 @@ NativeFileSystemPermissionView::NativeFileSystemPermissionView(
provider->GetDialogInsetsForContentType(views::TEXT, views::TEXT),
provider->GetDistanceMetric(views::DISTANCE_RELATED_CONTROL_VERTICAL)));
if (base::FeatureList::IsEnabled(
features::kNativeFileSystemOriginScopedPermissions)) {
AddChildView(native_file_system_ui_helper::CreateOriginPathLabel(
is_directory
? IDS_NATIVE_FILE_SYSTEM_ORIGIN_SCOPED_WRITE_PERMISSION_DIRECTORY_TEXT
: IDS_NATIVE_FILE_SYSTEM_ORIGIN_SCOPED_WRITE_PERMISSION_FILE_TEXT,
origin, path, CONTEXT_BODY_TEXT_SMALL, /*show_emphasis=*/true));
} else {
AddChildView(native_file_system_ui_helper::CreateOriginPathLabel(
is_directory ? IDS_NATIVE_FILE_SYSTEM_WRITE_PERMISSION_DIRECTORY_TEXT
: IDS_NATIVE_FILE_SYSTEM_WRITE_PERMISSION_FILE_TEXT,
origin, path, CONTEXT_BODY_TEXT_SMALL, /*show_emphasis=*/true));
}
AddChildView(native_file_system_ui_helper::CreateOriginPathLabel(
GetMessageText(request_), request_.origin, request_.path,
CONTEXT_BODY_TEXT_SMALL,
/*show_emphasis=*/true));
}
NativeFileSystemPermissionView::~NativeFileSystemPermissionView() {
......@@ -74,21 +109,41 @@ NativeFileSystemPermissionView::~NativeFileSystemPermissionView() {
}
views::Widget* NativeFileSystemPermissionView::ShowDialog(
const url::Origin& origin,
const base::FilePath& path,
bool is_directory,
const Request& request,
base::OnceCallback<void(permissions::PermissionAction result)> callback,
content::WebContents* web_contents) {
auto delegate = base::WrapUnique(new NativeFileSystemPermissionView(
origin, path, is_directory, std::move(callback)));
auto delegate = base::WrapUnique(
new NativeFileSystemPermissionView(request, std::move(callback)));
return constrained_window::ShowWebModalDialogViews(delegate.release(),
web_contents);
}
base::string16 NativeFileSystemPermissionView::GetWindowTitle() const {
return l10n_util::GetStringFUTF16(
IDS_NATIVE_FILE_SYSTEM_WRITE_PERMISSION_TITLE,
path_.BaseName().LossyDisplayName());
switch (request_.access) {
case AccessType::kRead:
if (request_.is_directory) {
return l10n_util::GetStringUTF16(
IDS_NATIVE_FILE_SYSTEM_READ_DIRECTORY_PERMISSION_TITLE);
} else {
return l10n_util::GetStringFUTF16(
IDS_NATIVE_FILE_SYSTEM_READ_FILE_PERMISSION_TITLE,
request_.path.BaseName().LossyDisplayName());
}
case AccessType::kWrite:
return l10n_util::GetStringFUTF16(
IDS_NATIVE_FILE_SYSTEM_WRITE_PERMISSION_TITLE,
request_.path.BaseName().LossyDisplayName());
case AccessType::kReadWrite:
if (request_.is_directory) {
return l10n_util::GetStringUTF16(
IDS_NATIVE_FILE_SYSTEM_EDIT_DIRECTORY_PERMISSION_TITLE);
} else {
return l10n_util::GetStringFUTF16(
IDS_NATIVE_FILE_SYSTEM_EDIT_FILE_PERMISSION_TITLE,
request_.path.BaseName().LossyDisplayName());
}
}
NOTREACHED();
}
bool NativeFileSystemPermissionView::ShouldShowCloseButton() const {
......@@ -111,11 +166,9 @@ views::View* NativeFileSystemPermissionView::GetInitiallyFocusedView() {
}
void ShowNativeFileSystemPermissionDialog(
const url::Origin& origin,
const base::FilePath& path,
bool is_directory,
const NativeFileSystemPermissionView::Request& request,
base::OnceCallback<void(permissions::PermissionAction result)> callback,
content::WebContents* web_contents) {
NativeFileSystemPermissionView::ShowDialog(origin, path, is_directory,
std::move(callback), web_contents);
NativeFileSystemPermissionView::ShowDialog(request, std::move(callback),
web_contents);
}
......@@ -7,12 +7,9 @@
#include "base/macros.h"
#include "base/strings/string16.h"
#include "chrome/browser/native_file_system/native_file_system_permission_request_manager.h"
#include "ui/views/window/dialog_delegate.h"
namespace base {
class FilePath;
} // namespace base
namespace content {
class WebContents;
} // namespace content
......@@ -21,10 +18,6 @@ namespace permissions {
enum class PermissionAction;
}
namespace url {
class Origin;
} // namespace url
namespace views {
class Widget;
} // namespace views
......@@ -33,6 +26,8 @@ class Widget;
// native file system API.
class NativeFileSystemPermissionView : public views::DialogDelegateView {
public:
using Request = NativeFileSystemPermissionRequestManager::RequestData;
~NativeFileSystemPermissionView() override;
// Shows a dialog asking the user if they want to give write access to the
......@@ -40,9 +35,7 @@ class NativeFileSystemPermissionView : public views::DialogDelegateView {
// |callback| will be called with the users choice, either GRANTED or
// DISMISSED.
static views::Widget* ShowDialog(
const url::Origin& origin,
const base::FilePath& path,
bool is_directory,
const Request& request,
base::OnceCallback<void(permissions::PermissionAction result)> callback,
content::WebContents* web_contents);
......@@ -55,12 +48,10 @@ class NativeFileSystemPermissionView : public views::DialogDelegateView {
private:
NativeFileSystemPermissionView(
const url::Origin& origin,
const base::FilePath& path,
bool is_directory,
const Request& request,
base::OnceCallback<void(permissions::PermissionAction result)> callback);
const base::FilePath path_;
const Request request_;
base::OnceCallback<void(permissions::PermissionAction result)> callback_;
DISALLOW_COPY_AND_ASSIGN(NativeFileSystemPermissionView);
......
......@@ -11,39 +11,55 @@
#include "components/permissions/permission_util.h"
#include "ui/views/controls/button/label_button.h"
using AccessType = NativeFileSystemPermissionRequestManager::Access;
class NativeFileSystemPermissionViewTest : public DialogBrowserTest {
public:
// DialogBrowserTest:
void ShowUi(const std::string& name) override {
base::FilePath path;
url::Origin origin = kTestOrigin;
bool is_directory = false;
NativeFileSystemPermissionView::Request request(
kTestOrigin, base::FilePath(), /*is_directory=*/false,
AccessType::kWrite);
if (name == "LongFileName") {
path = base::FilePath(FILE_PATH_LITERAL(
request.path = base::FilePath(FILE_PATH_LITERAL(
"/foo/bar/Some Really Really Really Really Long File Name.txt"));
} else if (name == "Folder") {
path = base::FilePath(FILE_PATH_LITERAL("/bar/MyProject"));
is_directory = true;
request.path = base::FilePath(FILE_PATH_LITERAL("/bar/MyProject"));
request.is_directory = true;
} else if (name == "LongOrigin") {
path = base::FilePath(FILE_PATH_LITERAL("/foo/README.txt"));
origin =
request.path = base::FilePath(FILE_PATH_LITERAL("/foo/README.txt"));
request.origin =
url::Origin::Create(GURL("https://"
"longextendedsubdomainnamewithoutdashesinord"
"ertotestwordwrapping.appspot.com"));
} else if (name == "FileOrigin") {
path = base::FilePath(FILE_PATH_LITERAL("/foo/README.txt"));
origin = url::Origin::Create(GURL("file:///foo/bar/bla"));
request.path = base::FilePath(FILE_PATH_LITERAL("/foo/README.txt"));
request.origin = url::Origin::Create(GURL("file:///foo/bar/bla"));
} else if (name == "ExtensionOrigin") {
path = base::FilePath(FILE_PATH_LITERAL("/foo/README.txt"));
origin = url::Origin::Create(GURL(
request.path = base::FilePath(FILE_PATH_LITERAL("/foo/README.txt"));
request.origin = url::Origin::Create(GURL(
"chrome-extension://ehoadneljpdggcbbknedodolkkjodefl/capture.html"));
} else if (name == "FolderRead") {
request.path = base::FilePath(FILE_PATH_LITERAL("/bar/MyProject"));
request.is_directory = true;
request.access = AccessType::kRead;
} else if (name == "FolderReadWrite") {
request.path = base::FilePath(FILE_PATH_LITERAL("/bar/MyProject"));
request.is_directory = true;
request.access = AccessType::kReadWrite;
} else if (name == "FileRead") {
request.path = base::FilePath(FILE_PATH_LITERAL("/foo/README.txt"));
request.access = AccessType::kRead;
} else if (name == "FileReadWrite") {
request.path = base::FilePath(FILE_PATH_LITERAL("/foo/README.txt"));
request.access = AccessType::kReadWrite;
} else if (name == "default") {
path = base::FilePath(FILE_PATH_LITERAL("/foo/README.txt"));
request.path = base::FilePath(FILE_PATH_LITERAL("/foo/README.txt"));
} else {
NOTREACHED() << "Unimplemented test: " << name;
}
widget_ = NativeFileSystemPermissionView::ShowDialog(
origin, path, is_directory,
request,
base::BindLambdaForTesting([&](permissions::PermissionAction result) {
callback_called_ = true;
callback_result_ = result;
......@@ -122,3 +138,22 @@ IN_PROC_BROWSER_TEST_F(NativeFileSystemPermissionViewTest,
InvokeUi_ExtensionOrigin) {
ShowAndVerifyUi();
}
IN_PROC_BROWSER_TEST_F(NativeFileSystemPermissionViewTest,
InvokeUi_FolderRead) {
ShowAndVerifyUi();
}
IN_PROC_BROWSER_TEST_F(NativeFileSystemPermissionViewTest,
InvokeUi_FolderReadWrite) {
ShowAndVerifyUi();
}
IN_PROC_BROWSER_TEST_F(NativeFileSystemPermissionViewTest, InvokeUi_FileRead) {
ShowAndVerifyUi();
}
IN_PROC_BROWSER_TEST_F(NativeFileSystemPermissionViewTest,
InvokeUi_FileReadWrite) {
ShowAndVerifyUi();
}
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment