Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Fenix does not require WRITE_EXTERNAL_STORAGE permission to be granted #2306

Conversation

rpl
Copy link
Member

@rpl rpl commented Aug 25, 2021

Fixes #2304

For Fenix we don't need the WRITE_EXTERNAL_STORAGE permission to be granted:

  • it was required for Fennec because web-ext run was also running it in a separate temporary profile as we do on desktop
  • but Fennec doesn't support that and so the WRITE_EXTERNAL_STORAGE permission isn't really necessary
  • We still need the READ_EXTERNAL_STORAGE permission to have been granted, to allow Fennec to load the xpi file that web-ext run uploads to the android emulator or device

TODO:

  • confirm with Rob that we are both ok with the strategy drafted in this PR (or agree on an alternative one)
  • add a more specific test case for the behavior expected
  • test on an android 11 emulator or device and a recent Fenix and GeckoViewExample builds

@rpl rpl requested a review from Rob--W August 25, 2021 17:30
@rpl
Copy link
Member Author

rpl commented Aug 25, 2021

@Rob--W this is currently mainly a draft of the kind of fix we may use and then even release on npm in a minor version:

the idea is to only make the check for the read permission still mandatory, and instead only run a "non blocking" check for the write permission (which just log a debug message, to avoid confusing the user with a warning message that is unlikely to apply to most users nowdays, but still allow us to get it in the verbose logs if we are investigating an issue reported to us).

Let me know wdyt.

@codecov
Copy link

codecov bot commented Aug 25, 2021

Codecov Report

Merging #2306 (e828938) into master (c884696) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2306   +/-   ##
=======================================
  Coverage   99.88%   99.88%           
=======================================
  Files          32       32           
  Lines        1699     1699           
=======================================
  Hits         1697     1697           
  Misses          2        2           
Impacted Files Coverage Δ
src/extension-runners/firefox-android.js 100.00% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c884696...e828938. Read the comment docs.

@rpl rpl force-pushed the fix/android-11-fenix-write-external-storage-permission branch from 4fb8db1 to e828938 Compare August 30, 2021 11:40
@rpl rpl requested a review from Rob--W August 30, 2021 11:51
// on android versions >= 23 (Android Marshmallow, which is the first version where
// these permissions are optional and have to be granted explicitly).
// Runtime permissions needed to Firefox to be able to access the
// xpi file uploaded to the android device or emulator.
const requiredPermissions = [
'android.permission.READ_EXTERNAL_STORAGE',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just tested, and the read permission isn't even needed to run an extension in Fenix. We can delete lots of code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rob--W have you checked that the permission wasn't actually already granted?
some more details about what STR you actually tested manually would be helpful to assess if that was enough to ensure we are not missing some legit corner case.

Copy link
Member Author

@rpl rpl Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Rob--W also, I would like to release this into a minor or patch version, and so "less changes" would make me feel way less concerned about releasing it in a "non major" new release (than "remove a lot of code").

I would be obviously more than happy about "remove a lot of code" in a separate follow up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filed #2312

@rpl rpl requested a review from Rob--W August 31, 2021 11:02
@rpl rpl merged commit 00f3f6c into mozilla:master Sep 7, 2021
piroor added a commit to piroor/web-ext that referenced this pull request Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can not grant android.permission.WRITE_EXTERNAL_STORAGE to org.mozilla.fenix
2 participants