Add a notification about auto-granted access to file systems.

Add a notification about auto-granted access to file systems.

For auto-launched kiosk apps permissions are granted automatically, without a
user consent. In such case a notification will be shown though.

TEST=unit_tests: *FileSystemApiConsentProviderTest*
BUG=440674
Committed: https://crrev.com/232bdea945b40f2db4ae674f870cd8ce9f32075d
Cr-Commit-Position: refs/heads/master@{#324169}

[mtomasz, 2015-03-26 10:54:29]

mtomasz@chromium.org changed reviewers:
+ benwells@chromium.org, jww@chromium.org

[mtomasz, 2015-03-26 10:54:46]

@benwells: PTAL.
@jww: PTAL at the approach.

Thanks.

[jww, 2015-03-26 20:14:59]

A couple of questions, but generally looks right from a security perspective.

https://codereview.chromium.org/1032313002/diff/1/chrome/app/chromeos_strings...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/1032313002/diff/1/chrome/app/chromeos_strings...
chrome/app/chromeos_strings.grdp:6142: Full access to <ph
name="VOLUME_NAME">$1<ex>FooBar 4GB</ex> has been granted.</ph>
Instead of "Full access," which sounds ambiguous, can you rewrite as "read and
write"? We've been aiming to use "read and write" whenever possible in
extensions instead of "access".

https://codereview.chromium.org/1032313002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/1032313002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/file_system/file_system_api.cc:322: } else if
(is_auto_launched) {
How does this work with non-auto launched apps that have had the permission
granted previously? Does the permission persist, and if so, shouldn't we give a
notification in that case as well?

[mtomasz, 2015-03-26 23:24:11]

https://codereview.chromium.org/1032313002/diff/1/chrome/app/chromeos_strings...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/1032313002/diff/1/chrome/app/chromeos_strings...
chrome/app/chromeos_strings.grdp:6142: Full access to <ph
name="VOLUME_NAME">$1<ex>FooBar 4GB</ex> has been granted.</ph>
On 2015/03/26 20:14:59, jww wrote:
> Instead of "Full access," which sounds ambiguous, can you rewrite as "read and
> write"? We've been aiming to use "read and write" whenever possible in
> extensions instead of "access".

Is "Read and write access to XXX has been granted." OK?

https://codereview.chromium.org/1032313002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/1032313002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/file_system/file_system_api.cc:322: } else if
(is_auto_launched) {
On 2015/03/26 20:14:59, jww wrote:
> How does this work with non-auto launched apps that have had the permission
> granted previously? Does the permission persist, and if so, shouldn't we give
a
> notification in that case as well?

For non-auto kiosk apps, we show the confirmation dialog and it can access the
file system as long as the kiosk app keeps the FileSystem object. Calling
chrome.fileSystem.requestFileSystem() would show the confirmation dialog again.

[mtomasz, 2015-03-27 05:01:38]

https://codereview.chromium.org/1032313002/diff/1/chrome/app/chromeos_strings...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/1032313002/diff/1/chrome/app/chromeos_strings...
chrome/app/chromeos_strings.grdp:6142: Full access to <ph
name="VOLUME_NAME">$1<ex>FooBar 4GB</ex> has been granted.</ph>
On 2015/03/26 23:24:11, mtomasz wrote:
> On 2015/03/26 20:14:59, jww wrote:
> > Instead of "Full access," which sounds ambiguous, can you rewrite as "read
and
> > write"? We've been aiming to use "read and write" whenever possible in
> > extensions instead of "access".
> 
> Is "Read and write access to XXX has been granted." OK?

Done.

[jww, 2015-03-27 15:59:12]

This lgtm from a security review perspective.

https://codereview.chromium.org/1032313002/diff/1/chrome/browser/extensions/a...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/1032313002/diff/1/chrome/browser/extensions/a...
chrome/browser/extensions/api/file_system/file_system_api.cc:322: } else if
(is_auto_launched) {
On 2015/03/26 at 23:24:11, mtomasz wrote:
> On 2015/03/26 20:14:59, jww wrote:
> > How does this work with non-auto launched apps that have had the permission
> > granted previously? Does the permission persist, and if so, shouldn't we
give a
> > notification in that case as well?
> 
> For non-auto kiosk apps, we show the confirmation dialog and it can access the
file system as long as the kiosk app keeps the FileSystem object. Calling
chrome.fileSystem.requestFileSystem() would show the confirmation dialog again.

I see. So it doesn't persist, and every time the app is launched, it will have
to rerequest the filesystem. SGTM.

https://codereview.chromium.org/1032313002/diff/20001/chrome/app/chromeos_str...
File chrome/app/chromeos_strings.grdp (right):

https://codereview.chromium.org/1032313002/diff/20001/chrome/app/chromeos_str...
chrome/app/chromeos_strings.grdp:6139: Read and write access to <ph
name="VOLUME_NAME">$1<ex>FooBar 4GB</ex> has been granted.</ph>
Great, thanks!

[benwells, 2015-03-30 01:40:15]

FYI, as this change depends on the one that introduces ConsentProvider I'll
review once that has settled.

[benwells, 2015-04-01 22:35:20]

On 2015/03/30 01:40:15, benwells wrote:
> FYI, as this change depends on the one that introduces ConsentProvider I'll
> review once that has settled.

I'll have a look once this has been rebased.

[mtomasz, 2015-04-02 10:12:55]

@benwells: Rebased and ready to review. PTAL. Thanks.

[mtomasz, 2015-04-06 06:17:42]

On 2015/04/02 10:12:55, mtomasz wrote:
> @benwells: Rebased and ready to review. PTAL. Thanks.

@benwells: Friendly ping.

[benwells, 2015-04-07 08:31:33]

lgtm with some nits.

https://codereview.chromium.org/1032313002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/1032313002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:304: // If a
whitelisted component, then no need to ask user.
Nit: s/ask/ask or inform the/

https://codereview.chromium.org/1032313002/diff/60001/chrome/browser/extensio...
File
chrome/browser/extensions/api/file_system/request_file_system_notification.cc
(right):

https://codereview.chromium.org/1032313002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/request_file_system_notification.cc:28:
// TODO(mtomasz): Take into account DPI.
You don't need this TODO, it is already taken into account by the underlying
IconImage stuff.

https://codereview.chromium.org/1032313002/diff/60001/chrome/browser/extensio...
File
chrome/browser/extensions/api/file_system/request_file_system_notification.h
(right):

https://codereview.chromium.org/1032313002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/request_file_system_notification.h:48:
void SetAppImage(const std::string& id, const gfx::ImageSkia& image) override;
Nit: this can be private

[mtomasz, 2015-04-08 00:30:46]

https://codereview.chromium.org/1032313002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/api/file_system/file_system_api.cc (right):

https://codereview.chromium.org/1032313002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/file_system_api.cc:304: // If a
whitelisted component, then no need to ask user.
On 2015/04/07 08:31:33, benwells wrote:
> Nit: s/ask/ask or inform the/

Done.

https://codereview.chromium.org/1032313002/diff/60001/chrome/browser/extensio...
File
chrome/browser/extensions/api/file_system/request_file_system_notification.cc
(right):

https://codereview.chromium.org/1032313002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/request_file_system_notification.cc:28:
// TODO(mtomasz): Take into account DPI.
On 2015/04/07 08:31:33, benwells wrote:
> You don't need this TODO, it is already taken into account by the underlying
> IconImage stuff.

Done.

https://codereview.chromium.org/1032313002/diff/60001/chrome/browser/extensio...
File
chrome/browser/extensions/api/file_system/request_file_system_notification.h
(right):

https://codereview.chromium.org/1032313002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/api/file_system/request_file_system_notification.h:48:
void SetAppImage(const std::string& id, const gfx::ImageSkia& image) override;
On 2015/04/07 08:31:33, benwells wrote:
> Nit: this can be private

Done.

[mtomasz, 2015-04-08 00:30:50]

The CQ bit was checked by mtomasz@chromium.org

[mtomasz, 2015-04-08 00:30:50]

The patchset sent to the CQ was uploaded after l-g-t-m from jww@chromium.org,
benwells@chromium.org
Link to the patchset: https://codereview.chromium.org/1032313002/#ps80001
(title: "Fixed.")

[commit-bot: I haz the power, 2015-04-08 00:31:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1032313002/80001

[commit-bot: I haz the power, 2015-04-08 01:22:25]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2015-04-08 01:23:21]

Patchset 5 (id:??) landed as
https://crrev.com/232bdea945b40f2db4ae674f870cd8ce9f32075d
Cr-Commit-Position: refs/heads/master@{#324169}