1451061 - Review Permissions usage for Android 8 behaviour changes

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Jan Henning [:JanH]]

As per https://developer.android.com/about/versions/oreo/android-8.0-changes.html#rmp, once targetting API26, requesting a certain runtime permission no longer implicitly grants all other permissions within that group [1].

So if we want to both read and write a file on the phone's storage, it's no longer enough to request just the WRITE_EXTERNAL_STORAGE - we need to request the READ_EXTERNAL_STORAGE permission as well.

So we need to review all places where we're requesting runtime permissions and make sure we're requesting the permission(s) that is/are actually required.
E.g. at https://hg.mozilla.org/mozilla-central/annotate/d75d996016dcf325c2db2ed8a47af512d07ffacd/mobile/android/chrome/content/browser.js#l4160 we're actually interested in *reading* the file.

Apart from the issue of read vs. write permissions, the other likely candidate might be ACCESS_COARSE_LOCATION vs. ACCESS_FINE_LOCATION.

[1] Although if a permission within the same group has already been granted, then currently Android won't prompt and automatically grant other permissions within the same group when requested.

Comment 1

[Jan Henning [:JanH]]

Although on the other hand the documentation for READ_EXTERNAL_STORAGE says that "Any app that declares the WRITE_EXTERNAL_STORAGE permission is implicitly granted this permission.", so we might be safe in that case after all.

Comment 2

[Andrei Lazar]

Attachment: Bug 1451061 - Review Permissions usage for Android 8 behaviour changes

Added reading permission as a safety measure for any future android updates.

Review commit: https://reviewboard.mozilla.org/r/250626/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250626/

Comment 3

[Code Review Bot [:reviewbot]]

Comment on attachment 8984822 [details]
Bug 1451061 - Review Permissions usage for Android 8 behaviour changes

https://reviewboard.mozilla.org/r/250626/#review256902


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: mobile/android/modules/RuntimePermissions.jsm:20
(Diff revision 1)
>  const ACCESS_COARSE_LOCATION = "android.permission.ACCESS_COARSE_LOCATION";
>  const ACCESS_FINE_LOCATION = "android.permission.ACCESS_FINE_LOCATION";
>  const CAMERA = "android.permission.CAMERA";
>  const RECORD_AUDIO = "android.permission.RECORD_AUDIO";
>  const WRITE_EXTERNAL_STORAGE = "android.permission.WRITE_EXTERNAL_STORAGE";
> +const READ_EXTERNAL_STORAGE = ""android.permission.WRITE_EXTERNAL_STORAGE";

Error: Parsing error: unexpected token android [eslint]

Comment 4

[Andrei Lazar]

Comment on attachment 8984822 [details]
Bug 1451061 - Review Permissions usage for Android 8 behaviour changes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250626/diff/1-2/

Comment 5

[Andrei Lazar]

Hello Jan, regarding ACCESS_COARSE_LOCATION vs. ACCESS_FINE_LOCATION situation, as per (https://developer.android.com/reference/android/Manifest.permission.html#ACCESS_COARSE_LOCATION) and (https://developer.android.com/reference/android/Manifest.permission.html#ACCESS_FINE_LOCATION), there are no extra details specified in the documentation, so my presumption is that you have to request permission for either one you use, so here we are covered.

Regarding other permissions, we use only specific permissions (camera and audio record), and we already request permission when it is needed.

Thank you!

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8984822 [details]
Bug 1451061 - Review Permissions usage for Android 8 behaviour changes

https://reviewboard.mozilla.org/r/250626/#review258450

::: mobile/android/modules/RuntimePermissions.jsm:20
(Diff revision 2)
>  const ACCESS_COARSE_LOCATION = "android.permission.ACCESS_COARSE_LOCATION";
>  const ACCESS_FINE_LOCATION = "android.permission.ACCESS_FINE_LOCATION";
>  const CAMERA = "android.permission.CAMERA";
>  const RECORD_AUDIO = "android.permission.RECORD_AUDIO";
>  const WRITE_EXTERNAL_STORAGE = "android.permission.WRITE_EXTERNAL_STORAGE";
> +const READ_EXTERNAL_STORAGE = "android.permission.WRITE_EXTERNAL_STORAGE";

Should be "android.permission.READ_EXTERNAL_STORAGE"

Comment 7

[Andrei Lazar]

Comment on attachment 8984822 [details]
Bug 1451061 - Review Permissions usage for Android 8 behaviour changes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250626/diff/2-3/

Comment 8

[Jan Henning [:JanH]]

Comment on attachment 8984822 [details]
Bug 1451061 - Review Permissions usage for Android 8 behaviour changes

https://reviewboard.mozilla.org/r/250626/#review261806

Comment 9

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 1451061 - Review Permissions usage for Android 8 behaviour changes

Added reading permission as a safety measure for any future android updates.

***

Review commit: https://reviewboard.mozilla.org/r/256950/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256950/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f257217209b
Review Permissions usage for Android 8 behaviour changes r=jchen,JanH

Comment 11

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/9f257217209b