agamemnus / cordova-plugin-xapkreader

Easily access Google Play APK expansion file data.
32 stars 55 forks source link

Android 6: May need to actively request WRITE_EXTERNAL_STORAGE permission #94

Closed agwells closed 5 years ago

agwells commented 7 years ago

As I noted in another issue, there's a bug in Android 6 (api 23) and how it handles OBB expansion files. OBBs are downloaded into the app's internal storage space, so they shouldn't require READ_EXTERNAL_STORAGE or WRITE_EXTERNAL_STORAGE permission. But in Android 6, the system does require those permissions from the app, in order to function.

Now, this plugin does add those permissions to the Android Manifest file. But another change in Android 6, is that the user is not automatically prompted to grant all permissions at upgrade/install time. Instead, some permissions are categorized as "dangerous permissions", and the app must prompt for these at runtime. See https://developer.android.com/training/permissions/requesting.html

So taken together, if you install an app with this plugin on Android 6, it fails to be able to read from the expansion file after it's downloaded by Google Play. (And it probably can't download the OBB via the plugin's own downloader code.) In practice, I've had mixed results and it seems to work fine on some devices. That may be down to differences in Android distributions.

The good news is, this only happens if your app's target api version is 23 or higher. So the quick workaround for this is to set your Cordova app's android-targetSdkVersion attribute to 22 or lower. This will cause it to exhibit the older behavior -- prompt the user to approve WRITE_EXTERNAL_STORAGE permission when they install the app.

The more polished fix, would be to sniff whether the app is currently running in api 23, and if so, do the whole rigamarole of checking for, and requesting, the permission.

agamemnus commented 7 years ago

Hmm.

Can you confirm that it is only Android 6 that has the problem and not Android 7, per the bug report?

agwells commented 7 years ago

@agamemnus I don't have access to a physical test device, but I've tested the behavior in Android 6 & Android 7, and it looks like, indeed, this is only in Android 6.

Specifically, here's what I did.

Test case 1:

  1. Compile the app with target api 23
  2. Launch an Android 6 api 23 image in the emulator.
  3. Upload the expansion file into place on the image using adb
  4. Download the APK file onto the image and install it and launch the app

Expected result: Images from the expansion file are visible Actual result: Images from the expansion file are not visible

Test case 2: Same as test case 1, but use an Android 7 (API 24) system image Result: Images from the expansion file are visible.

agwells commented 7 years ago

Hm, looks like for me the api 22 workaround won't be an option, because I've already uploaded a target 23 APK to Google. And once you do that, Google Play will not let you upload a subsequent API with a lower target!

So I may be filing a merge request for the runtime permissions request in a day or two.

agamemnus commented 7 years ago

@agwells: Sorry if it is a silly question: can you confirm the issue still exists with the latest SDK, too? My android-target is 25 and Android platform 6.1.2 on Cordova 6.5.0.

BobAchgill commented 7 years ago

My use case is can't access extSD obb ... Android-target 25 Cordova 6.5.0 On an Android 7 phone

I have been guessing that lacking run time permission is the issue for not being able to access the obb file on tge extsd because of the new run time feature for dangerous permissions android implemented with 23 and higher.

I ran across the same issue (can't access the extsd path) with the Cordova.plugins.diagnostic till I used the run time permissions function provided in the example code. Cordova-diagnostic-plugin-example

From reading the link in the first post it looks like run time permissions for "dangerous" activity is here to stay. Its a feature not a bug. :)

@agamemnus do you have a runtime permission function implemented for this plugin that I can use?

agamemnus commented 7 years ago

Nothing sorry.

The permissions seem to be only for 6 and not 7. I don't really get it. I think Google just decided to forget about and give up on maintaining OBB file compatibility, with all the issues in recent years.

agwells commented 7 years ago

@agwells: Sorry if it is a silly question: can you confirm the issue still exists with the latest SDK, too? My android-target is 25 and Android platform 6.1.2 on Cordova 6.5.0.

Looks like it's still present. I tried building the app with the android-targetSdkVersion in my config.xml set to 25, and it still is broken in Android 6.0 (in the emulator). I believe the OBB permissions bug is present in the Android OS side of things, so that's why raising the app to a higher SDK target can't fix it.

I'm currently working on some code to request the permission at runtime if the device is running Android 6.

agwells commented 7 years ago

Good news, it looks like Cordova has an API to make it easier to request runtime permissions. https://cordova.apache.org/docs/en/latest/guide/platforms/android/plugin.html#android-permissions

samengstrom commented 7 years ago

I added some code to fix this and created a pull request

agwells commented 7 years ago

@samengstrom's PR: https://github.com/agamemnus/cordova-plugin-xapkreader/pull/95

agwells commented 7 years ago

I actually have implemented this as well, but I forgot to come back and issue a PR! :) https://github.com/agwells/cordova-plugin-xapkreader/commit/9d0741ac2fafecfb07b95f2b37fa6101388ade71

I'll file a PR. My code is pretty similar to @samengstrom's, but it has a couple of (I think) minor improvements. It only asks for this permission if you're running API level 23, because the bug was fixed in Android 7+. And it checks for the permission in initialize() rather than startDownloadExpansionIfAvailable(), because you need that permission in order to read the file, not just to download it. Specifically, if you do a clean install of an app with an OBB expansion, Google Play will download the file along with the app; and with this bug it'll be sitting there unreadable unless you have read permissions.

samengstrom commented 7 years ago

@agwells Your code seems good and it makes sense to check for the API level, although I'm not sure if it's strictly necessary because you should have the permission in older API levels already, if correctly specified in the manifest.

I put the permission request in startDownloadExpansionIfAvailable so that you can call it later in the app if you only need to use the expansion in a special case or if you want to do something before downloading it. For example, you might want to display the reason why the app asks for permission. At least for my app it isn't obvious why it needs access to the user's photos etc and the user might be inclined to refuse the permission request, so I should explain that the app actually won't work at all without the permission.

BobAchgill commented 7 years ago

What about the case where I add the OBB for testing by hand and try to test run the app on Android 7? My issue is that that it does not work... and I do not get a popup asking for permission. So does this mean that the app is not finding the OBB because a run time permission is not be asked for and granted??

samengstrom commented 7 years ago

Have you manually pushed the OBB to the device with adb?

BobAchgill commented 7 years ago

I copied it there with filemanger from my windows machine via usb.

samengstrom commented 7 years ago

You can also add the grant permission from Android settings on the device to verify or rule out that the problem has to do with permissions

samengstrom commented 7 years ago

Just did some testing on Android 7.1 (API level 25) and it is able to open the expansion pack that Google Play downloaded even if I refuse the permission.

BobAchgill commented 7 years ago

Thanks! I am at Android 7.0 on my device. I guess I just need to give up on trying to manually test access to my OBB before uploading it to Google Play. Maybe there is some small detail that makes that not possible with how the Cordova plugin is designed.

agamemnus commented 7 years ago

It worked before (before 6). Who knows what is happening now.

BobAchgill commented 7 years ago

Yep... it is probably some small detail I am missing from your instructions. I'll try a test OBB upload and see if I learn something from if the download makes it start working properly. My OBBs are 1.5GB ... that's why I hesitated and preferred to do a manual test first.

samengstrom commented 7 years ago

Confirmed that testing works as before even with the newest code. To push the obb I use: /home/user/Android/Sdk/platform-tools/adb push platforms/android/expansion.obb storage/emulated/obb/com.mycompany.myapp/main.0.com.mycompany.myapp.obb The path storage/emulated/obb/com.mycompany.myapp needs to exist on device (I do a mkdir in adb shell)

samengstrom commented 7 years ago

I did some testing with the latest version and Play store's alpha channel and got some confusing results.

Android 6.0.1 (tested with a Nexus 7 and a Samsung Galaxy Tab S2 9.7) The expansion is downloaded from Play store alongside the app nicely. When I start the app it asks for permission but the expansion pack is loaded even if I reject it. If I then delete the expansion in adb shell, it will download it again when I restart the app. In this version of the code I have to grant permission for the downloader to even start, so I can't currently test what would happen if permissions weren't granted.

Android 7.1.2 (tested with a Nexus 6P) The expansion is downloaded and works without permission prompts, as expected. However it shows a "Downloading assets" dialog on first run and leaves it frozen in the notification area at 0/0 complete. If I delete the expansion it downloads again and works, but the notification remains frozen at the very end (Shows 492.46MB/492.72MB). Also if I delete the expansion directory under obb, the app fails to download and simply says "Error. Download failed.". If I make an empty directory for it, it works.

Then I made an update to the APK only (with no changes to the code) to see how it behaves when the expansion is already on the device and it worked nicely on the Samsung (with no expansion download when I rejected the downloader). On the Nexus 7 I did the update but accepted the permission and now the downloader opened but got stuck at 0. After restarting the app the downloader didn't open and everything worked.

On Android 7.1 the downloader opened and got stuck at 0. After restarting the app the downloader didn't open and everything worked. So this was the same as with Android 6 when you grant the permission.

So the permissions don't seem to be the problem, but the downloader getting stuck. I could not find anything useful in logcat. I will do more investigating, because the user experience isn't great if you have to kill and restart the app after an update.

agamemnus commented 7 years ago

That's not good. We don't want fixes to cause even more problems!!!

agwells commented 7 years ago

@samengstrom https://github.com/agamemnus/cordova-plugin-xapkreader/issues/94#issuecomment-296045801

Oh, you have a good point about providing an opportunity for the app to explain the reason for the permissions. Google even advises doing that, in their instructions about using "unsafe" permissions. I didn't bother with that part because my client was looking for the quickest solution possible, and it's an app being used by members of an organization rather than random app store customers, so they're less likely to question these things. But for everyone else it's a good idea.

My only concern about doing it in startDownloadExpansion() is that in my testing I found you could wind up with this scenario:

  1. Clean install of the app
  2. As part of the clean install, Google Play downloads the OBB alongside the app, and saves it with incorrect permissions.
  3. Launch app for the first time.

In that scenario, the file will already be present, so you shouldn't need to enter the downloading section of the code. But, you also don't have the right permissions to read it yet. In my testing, this resulted in the app not wanting to re-download the OBB, but also not being able to access any of its contents.

Maybe the solution, then, is to check the permissions immediately before downloading, and also immediately before launching the service provider.

I'm not sure if it's strictly necessary because you should have the permission in older API levels already, if correctly specified in the manifest.

Well, there are three use-cases:

  1. API level < Android 6.0: App will ask for all permissions in the manifest, at install or upgrade.
  2. API level == Android 6.0: App needs file READ/WRITE permission, and it must be requested interactively
  3. API level > Android 6.0: App doesn't need file READ/WRITE permission (because the OBB file is correctly marked as the app's "internal" storage)

That's why I only ask for the permission if the API level is exactly 23. As it mentions in the Google bug report, and you noticed from testing, you don't need that permission to use the OBB file in Android 7+. So, since it's a scary permission, I only ask for it if you actually need it.

agwells commented 7 years ago

@samengstrom https://github.com/agamemnus/cordova-plugin-xapkreader/issues/94#issuecomment-296153382

The expansion is downloaded and works without permission prompts, as expected. However it shows a "Downloading assets" dialog on first run and leaves it frozen in the notification area at 0/0 complete. If I delete the expansion it downloads again and works, but the notification remains frozen at the very end (Shows 492.46MB/492.72MB). Also if I delete the expansion directory under obb, the app fails to download and simply says "Error. Download failed.". If I make an empty directory for it, it works.

Oh, if the downloader is showing MB, does that mean you're testing that commit number I posted? https://github.com/agwells/cordova-plugin-xapkreader/commit/9d0741ac2fafecfb07b95f2b37fa6101388ade71

That commit is from my prod branch, which contains code not present in the agamemnus 6.5.0 branch. Specifically, it's hacked to display progress in MB only. (I've filed a pull request for that, which is modified to turn MB/% into an optional setting instead of being hard-coded to MB: https://github.com/agamemnus/cordova-plugin-xapkreader/pull/73 ). I had noticed, in my own testing, that the downloader sometimes didn't go away properly after download had finished. It sounds like it maybe has further issues in Android 7.

The pull request I filed for this issue ( https://github.com/agamemnus/cordova-plugin-xapkreader/issues/96 ) doesn't have those changes. I cherry-picked the permissions code on top of the current agamemnus 6.5.0 branch.

So, just to make sure we're on the same page, do you see the same issues if you use the branch https://github.com/agamemnus/cordova-plugin-xapkreader/commits/cordova-6.5.0 ? :)

agamemnus commented 7 years ago

I didn't realize you had updated that to make it an option. I just accepted the pull request.

agwells commented 7 years ago

@agamemnus Lol, well, apparently it may cause issues with the download progress notifier not closing properly. ;)

But maybe we can file a separate issue report for that. And I find, in practice, the download progress notifier is rarely used, because Google Play will try its hardest to download the OBB file at the same time as the app's install or upgrade.

agamemnus commented 7 years ago

By the way, I have been trying to keep the latest 5.3.1-auto and 6.5 the same.

Why would changing the display to MB cause issues? Hmm.

agamemnus commented 7 years ago

What if you remove this part:

if (mProgressDialog.getMax() != totalMB) {
 mProgressDialog.setMax(totalMB);
}
agwells commented 7 years ago

Why would changing the display to MB cause issues? Hmm.

Good point, I don't see anything in my patch that immediately seems like it would cause that problem. Maybe it was an earlier code change? Unfortunately I don't have any time to dig into it further right now. :(

agwells commented 7 years ago

For anyone who wants to investigate that download-progress-notifier issue, here's a little hack I discovered, to make it quicker to test OBB downloader code without having to upload new versions of the app to Google Play and wait for them to roll out.

  1. Install the app on your test device via Google Play.
  2. If Google Play downloaded the OBB along with the app, manually delete the OBB from your device.
  3. Launch the app. (Force-quit it first if it's already running.) This should trigger the downloader to get the OBB from Google Play. Importantly, this means the app will retrieve a valid Google Play download URL and store it in its internal preferences.
  4. Now, compile a new build of the app locally, but make sure to give it the same build number as the version you just installed via Play (in Cordova, you can use android-versionCode in config.xml for this).
  5. Force-install the new build onto your test device via adb: adb install -r android-release.apk
  6. Delete the OBB file (or partially downloaded OBB file) from step 3.
  7. Re-launch the app.

If all goes well, the new build you just installed on the test device will launch, keeping the same local data as the store build, and it'll successfully start downloading the OBB file! You can repeat this over and over with new test builds. Google Play download URLs are temporary, so after a few hours it'll stop working and you'll need to reinstall from the Play store and repeat the process above.

I'm pretty sure this is a bug, so it may not even work in newer Android versions. ;)

samengstrom commented 7 years ago

Just to clarify, my testing was done with code in https://github.com/agamemnus/cordova-plugin-xapkreader/commits/cordova-6.5.0 and not @agwells branch. The downloader in the app does not show MB, but the one in the notification area does. Provided by Google Play perhaps?

The weird part regarding permissions in my testing was that in Android 6 the expansion was opened correctly even though I rejected the permission. So did just asking for permissions make it work, or does it not need to ask for permissions at all? I'll do more testing on Thursday.

agamemnus commented 7 years ago

Rip this thread.

samengstrom commented 7 years ago

Sorry, I forgot to post updates. The app still works even if I reject permission but I haven't tried removing the permission check completely.

As a workaround to the startup problem after update, I disabled the downloader (!) by setting auto_download to false. I haven't yet found a case where Google Play would not download the expansion for the user and if it's deleted somehow later the user can reinstall the app so I'm currently not starting the downloader anywhere in the app.

fonograph commented 7 years ago

Looking at https://issuetracker.google.com/issues/37075181 (and my own experience), the bug doesn't actually appear to be consistently fixed on Android 7 -- it's occurring for me on Android 7 / Samsung S7, targeting API 25. Switching the API check from == 23 to >= 23 so that the app requested the permission fixed the problem for me.

agamemnus commented 7 years ago

It is ironic that Google has pretty much given up supporting obb files when they just released a new policy in May prohibiting "executable code" from being installed via a source other than Google Play. Of course, we can just pretend that it's lawyer speak (it is), but one way to interpret this is that updates other than via obb files aren't allowed anymore.

Sad.

ashleygwinnell commented 7 years ago

Having the same issue as @fonograph. Samsung S6 and other devices (mostly Samsung ones) still need permissions. I've changed to check for >= 23 locally and this requests the permission now. I now just need a signal to catch in JS when the permission is granted/denied (because the page continues to load in the background and crashes because of no permissions) but that may be beyond the scope of this plugin?

agamemnus commented 7 years ago

It's not really (not beyond the scope). But this is all very annoying. It seems there is chaos here. Not sure what the best comprehensive solution is. >:(