DevTools: Replace .allow-devtools-edit file check with confirmation infobar.

DevTools: Replace .allow-devtools-edit file check with confirmation infobar.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=196706

[vsevik, 2013-04-25 10:25:34]

Pavel could you please have a look on this before I request a security review?

[pfeldman, 2013-04-25 11:06:01]

https://codereview.chromium.org/14081036/diff/5001/chrome/browser/devtools/de...
File chrome/browser/devtools/devtools_file_helper.h (right):

https://codereview.chromium.org/14081036/diff/5001/chrome/browser/devtools/de...
chrome/browser/devtools/devtools_file_helper.h:52: const
base::Callback<void()>&,
you could name them for convenience.

[vsevik, 2013-04-25 12:00:16]

Tom, could you please security review this?

In this patch I am removing .allow-devtools-edit file existence check.
The way file system access from DevTools works now is following:

1) DevTools renderer requests browser to add a file system
(DevToolsHostMsg_AddFileSystem)

2) Browser shows folder selection dialog

3) Once user selects a folder an infobar requesting user to confirm this action
is shown (instead of .allow-devtools-edit file existence check before).

4) After user confirms file system access request the path is persisted in
browser settings.

5) Each time DevTools is open it asks browser for all available file systems
(DevToolsHostMsg_RequestFileSystems) and browser grants access to all paths
saved in browser settings without any additional checks (instead of
.allow-devtools-edit file existence check for them before).

[Tom Sepez, 2013-04-25 18:07:28]

How does one revoke access?  If I make a mistake, I can remove the
.allow-devtools-edit file.  I don't see a way to do this with the new approach.

I'm not sure an infobar is strong enough. There's a class of users that will
click this but would never be willing or savvy enough to put the .allow file
onto their machines.

[pfeldman, 2013-04-25 19:42:17]

On 2013/04/25 18:07:28, Tom Sepez wrote:
> How does one revoke access?  If I make a mistake, I can remove the
> .allow-devtools-edit file.  I don't see a way to do this with the new
approach.
> 

DevTools Settings lists all added FileSystems and allows removing them.

> I'm not sure an infobar is strong enough. There's a class of users that will
> click this but would never be willing or savvy enough to put the .allow file
> onto their machines.

That's where we are asking for guidance. Suggested grant access routine has
following steps:

1) Open DevTools
2) Go to Settings, under Workspace, choose Add File System
3) Browser's file chooser shows up, select folder to grant access to
4) Infobar shows up on browser level, shows warning suggesting that DevTools
requested access there

After that, granted access is persisted in browser settings. It seems like fair
amount of confirmations to me, but it should be balanced across the rest of the
system, hence the question.

[Tom Sepez, 2013-04-25 19:58:45]

LGTM with nit.

https://codereview.chromium.org/14081036/diff/5001/chrome/app/generated_resou...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/14081036/diff/5001/chrome/app/generated_resou...
chrome/app/generated_resources.grd:4230: +        DevTools requests permission
to access the following file system path: $1.
The message ought to be scarier, talking about ensuring that there isn't
sensitive information in that directory.

[vsevik, 2013-04-26 08:59:21]

Pavel, PTAL

[pfeldman, 2013-04-26 09:04:55]

lgtm

https://codereview.chromium.org/14081036/diff/13001/chrome/app/generated_reso...
File chrome/app/generated_resources.grd (right):

https://codereview.chromium.org/14081036/diff/13001/chrome/app/generated_reso...
chrome/app/generated_resources.grd:4237: +        DevTools request full access
to $1.
requests

https://codereview.chromium.org/14081036/diff/13001/chrome/browser/devtools/d...
File chrome/browser/devtools/devtools_file_helper.h (right):

https://codereview.chromium.org/14081036/diff/13001/chrome/browser/devtools/d...
chrome/browser/devtools/devtools_file_helper.h:53: const base::Callback<void()>&
cancel_callback)>
Do we need two callbacks?

[commit-bot: I haz the power, 2013-04-26 10:13:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/vsevik@chromium.org/14081036/18002

[commit-bot: I haz the power, 2013-04-26 12:29:24]

Change committed as 196706