[Offline pages] Removing permission prompt when accessing offline page

[Offline pages] Removing permission prompt when accessing offline page

Updates ExternalNavigationDelegateImpl to check for an accessed file:// url to be inside of the application's data directory. For files that are, no storage permission prompt is shown, as the application always has access to access such files.

It does not verify whether the file exists.

BUG=565875
Committed: https://crrev.com/6b6c4727f852917070449f683c268d73968d9082
Cr-Commit-Position: refs/heads/master@{#367859}

[fgorski, 2015-12-22 23:12:54]

fgorski@chromium.org changed reviewers:
+ jianli@chromium.org, newt@chromium.org

[fgorski, 2015-12-22 23:12:55]

This patch relaxes the requirement that the URL has to point to an offline page,
it simply checks that we are pointing to and .mhtml file in the right directory.

What we get this way is no need to call native, and idea which I like and which
reflects better how ExternalNavigationHandler was already making that check.

I am open to suggestions on how to make it better, and whether this is the right
approach.

Remember that the goal is to remove the prompt shown when accessing the file
that is already accessible to the application without necessary storage
permission.

[newt (away), 2016-01-04 19:45:06]

https://codereview.chromium.org/1542943006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
(right):

https://codereview.chromium.org/1542943006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:127:
&& url.startsWith("file://" + PathUtils.getDataDirectory(context))
This seems prone to silent breakage if the location of offline URLs changes. Can
you instead just get the offline page directory from the canonical (C++?) method
that provides said directory?

Since this code only runs for file:// URLs, I think a call into native is fine
and probably better than duplicating knowledge about the location on disk of
offline pages.

(Alternatively, can you simply update
ExternalNavigationDelegateImpl.shouldRequestFileAccess() to check whether the
URL points inside PathUtils.getDataDirectory()? Does Chrome need file
permissions for the renderer to access, say,
file:///data/data/com.google.android.apps.chrome/app_chrome/Default/Preferences
? I would expect not.)

[jianli, 2016-01-04 21:42:00]

https://codereview.chromium.org/1542943006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
(right):

https://codereview.chromium.org/1542943006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:127:
&& url.startsWith("file://" + PathUtils.getDataDirectory(context))
On 2016/01/04 19:45:05, newt (OOO until 1-4) wrote:
> This seems prone to silent breakage if the location of offline URLs changes.
Can
> you instead just get the offline page directory from the canonical (C++?)
method
> that provides said directory?
> 
> Since this code only runs for file:// URLs, I think a call into native is fine
> and probably better than duplicating knowledge about the location on disk of
> offline pages.
> 
> (Alternatively, can you simply update
> ExternalNavigationDelegateImpl.shouldRequestFileAccess() to check whether the
> URL points inside PathUtils.getDataDirectory()? Does Chrome need file
> permissions for the renderer to access, say,
>
file:///data/data/com.google.android.apps.chrome/app_chrome/Default/Preferences
> ? I would expect not.)

The name isOfflineUrlForStoragePermissionCheck sounds too restrictive. I don't
think this needs to be limited to storage permission check usage. I think we
want a version used for both C++ and Java that will tell if a given URL is very
likely an offline URL.

I agree with newt. It seems to be better to add more checks in C++'s
MightBeOfflineURL and expose it to Java.

[fgorski, 2016-01-04 23:11:35]

https://codereview.chromium.org/1542943006/diff/1/chrome/android/java/src/org...
File
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java
(right):

https://codereview.chromium.org/1542943006/diff/1/chrome/android/java/src/org...
chrome/android/java/src/org/chromium/chrome/browser/offlinepages/OfflinePageUtils.java:127:
&& url.startsWith("file://" + PathUtils.getDataDirectory(context))
On 2016/01/04 21:42:00, jianli wrote:
> On 2016/01/04 19:45:05, newt (OOO until 1-4) wrote:
> > This seems prone to silent breakage if the location of offline URLs changes.
> Can
> > you instead just get the offline page directory from the canonical (C++?)
> method
> > that provides said directory?

This sits inside of the profile folder and I am not seeing references to
profile/browser context here, so I'd prefer to avoid it.

> > 
> > Since this code only runs for file:// URLs, I think a call into native is
fine
> > and probably better than duplicating knowledge about the location on disk of
> > offline pages.
> > 
> > (Alternatively, can you simply update
> > ExternalNavigationDelegateImpl.shouldRequestFileAccess() to check whether
the
> > URL points inside PathUtils.getDataDirectory()? Does Chrome need file
> > permissions for the renderer to access, say,
> >
>
file:///data/data/com.google.android.apps.chrome/app_chrome/Default/Preferences
> > ? I would expect not.)

I thought of that earlier, but the difficulty there is that we only pass a tab
to the method, and not the url in question. Do you suggest getting it form the
tab somehow or changing the interface to the Delegate?

> 
> The name isOfflineUrlForStoragePermissionCheck sounds too restrictive. I don't
> think this needs to be limited to storage permission check usage. I think we
> want a version used for both C++ and Java that will tell if a given URL is
very
> likely an offline URL.
> 
> I agree with newt. It seems to be better to add more checks in C++'s
> MightBeOfflineURL and expose it to Java.

Let's explore that option deeper (I think we have a basic agreement as to how to
do it, based on our conversation) once I hear from Newton about the other
option.

[fgorski, 2016-01-05 21:50:04]

Netwon, I applied your suggestion. PTAL

This approach to the problem minimizes the changes. The only drawback is that
the delegate implementation is not tested at all.

[newt (away), 2016-01-05 22:32:37]

lgtm

https://codereview.chromium.org/1542943006/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java
(right):

https://codereview.chromium.org/1542943006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java:289:
// If the url points inside of Chromium's data directory no permissions are
necessary.
I'd add a comment that this is needed for offline pages in particular.

[jianli, 2016-01-06 00:40:37]

lgtm

Please update the issue description.

https://codereview.chromium.org/1542943006/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java
(right):

https://codereview.chromium.org/1542943006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java:289:
// If the url points inside of Chromium's data directory no permissions are
necessary.
nit: add comma after 'directory'

[fgorski, 2016-01-06 17:52:20]

Description was changed from

==========
[Offline pages] Removing permission prompt when accessing offline page

Adds a method to OfflinePageUtils that verifies that url:
* Points to a file with .mhtml extension
* That is located in application data directory
* And is in Offline Pages/archives folder

It does not verify whether the file exists.

BUG=565875
==========

to

==========
[Offline pages] Removing permission prompt when accessing offline page

Updates ExternalNavigationDelegateImpl to check for an accessed file:// url to
be inside of the application's data directory. For files that are, no storage
permission prompt is shown, as the application always has access to access such
files.

It does not verify whether the file exists.

BUG=565875
==========

[fgorski, 2016-01-06 17:52:55]

All addressed. Thanks for review.

https://codereview.chromium.org/1542943006/diff/20001/chrome/android/java/src...
File
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java
(right):

https://codereview.chromium.org/1542943006/diff/20001/chrome/android/java/src...
chrome/android/java/src/org/chromium/chrome/browser/externalnav/ExternalNavigationDelegateImpl.java:289:
// If the url points inside of Chromium's data directory no permissions are
necessary.
On 2016/01/06 00:40:37, jianli wrote:
> nit: add comma after 'directory'

Done.

[fgorski, 2016-01-06 17:53:03]

The CQ bit was checked by fgorski@chromium.org

[fgorski, 2016-01-06 17:53:04]

The patchset sent to the CQ was uploaded after l-g-t-m from jianli@chromium.org,
newt@chromium.org
Link to the patchset: https://codereview.chromium.org/1542943006/#ps40001
(title: "Fixing a comment based on CR")

[commit-bot: I haz the power, 2016-01-06 17:54:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1542943006/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1542943006/40001

[commit-bot: I haz the power, 2016-01-06 18:18:31]

Description was changed from

==========
[Offline pages] Removing permission prompt when accessing offline page

Updates ExternalNavigationDelegateImpl to check for an accessed file:// url to
be inside of the application's data directory. For files that are, no storage
permission prompt is shown, as the application always has access to access such
files.

It does not verify whether the file exists.

BUG=565875
==========

to

==========
[Offline pages] Removing permission prompt when accessing offline page

Updates ExternalNavigationDelegateImpl to check for an accessed file:// url to
be inside of the application's data directory. For files that are, no storage
permission prompt is shown, as the application always has access to access such
files.

It does not verify whether the file exists.

BUG=565875
==========

[commit-bot: I haz the power, 2016-01-06 18:18:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-06 18:19:38]

Description was changed from

==========
[Offline pages] Removing permission prompt when accessing offline page

Updates ExternalNavigationDelegateImpl to check for an accessed file:// url to
be inside of the application's data directory. For files that are, no storage
permission prompt is shown, as the application always has access to access such
files.

It does not verify whether the file exists.

BUG=565875
==========

to

==========
[Offline pages] Removing permission prompt when accessing offline page

Updates ExternalNavigationDelegateImpl to check for an accessed file:// url to
be inside of the application's data directory. For files that are, no storage
permission prompt is shown, as the application always has access to access such
files.

It does not verify whether the file exists.

BUG=565875

Committed: https://crrev.com/6b6c4727f852917070449f683c268d73968d9082
Cr-Commit-Position: refs/heads/master@{#367859}
==========

[commit-bot: I haz the power, 2016-01-06 18:19:39]

Patchset 3 (id:??) landed as
https://crrev.com/6b6c4727f852917070449f683c268d73968d9082
Cr-Commit-Position: refs/heads/master@{#367859}