Collect: Adding the Runtime Permissions for the Android 6.0 and above

[nikhilbalyan]

Problem description

For Android 6.0 and below the permissions were asked at the time of installation of the application but with the new improvements the user is asked about the permission at the run time of the application. And we have not handled the permissions for Android 6.0 and above which could lead into a crash

Steps to reproduce the problem

Inside the ImageWebViewWidget class we have handled camera permissions for the versions below Android 6.0 and not for the Android 6.0 and above

Expected behavior

Add permissions check for Android 6.0 and above as well

Other information

https://developer.android.com/training/permissions/requesting.html

I would love to fix this issue. @lognaturel

Permissions that need addressing:

• [x] read external storage #2119
• [x] write external storage #2119
• [x] camera #2320
• [x] access fine location #2358
• [x] access coarse location #2358
• [x] record audio #2367
• [x] read phone state #2506
• [x] get accounts #2474
• [x] SMS #2474

Only permissions listed as dangerous in https://developer.android.com/guide/topics/permissions/overview#permission-groups need to be asked for explicitly so access wifi state, internet, access network state, wake lock don't need any changes

Removed in API 23: use credentials, manage accounts

[nikhilbalyan]

Right now the statement is in try/catch hence a probable crash can be prevented for Android 6.0 and above

[lognaturel]

@nikhilbalyan Please go ahead for camera permissions. I think you'll need to do the same for ImageWidget.

If you'd like to address other cases of this problem, please submit a different pull request for each category of classes the problem occurs in (for example "mapping" is a category).

[lognaturel]

And thanks for filing this important issue!

[lognaturel]

And actually, I also have a question for you. My understanding was that it was possible to do it the "old way" and still ask for all permissions up front if targetSDKVersion was under 23 (which it is). Is that not the case? Have you verified that the crash does in fact happen?

In general, runtime permissions are great. ODK Collect is kind of special, though, in that inexperienced enumerators are often the ones using it. If, for some reason, the survey manager didn't think to trigger all the runtime permission asks and grant them, it could lead to a lot of confusion.

[nikhilbalyan]

@lognaturel We need to put a check if the version of the target sdk is below 23 then we wont ask for permissions and the permissions asked at the time of installation will be fine as explained here http://stackoverflow.com/a/33163206/5230044.

Moreover it will ask for the permissions only first time the user will be using collect. However if the user clears the application data, application data somehow gets deleted or disables the permission manually which they granted to the application earlier then the application will ask again for that permission at the time of usage.

The permissions will be triggered only at the time of usage all of them can't be triggered at once unless the survey manager deliberately does that by going to settings.

And myphone is running on Android 5.1 and i tried a lot to get the Android 6.0 and above but can't arrange that. Although in another project i had seen that application crashes if we don't handle the Android runtime permissions for 6.0 and above.

[lognaturel]

@nikhilbalyan That post you linked to says "when you target API 23 and the app is running on an Android 6.0+ device". So yes, I think you said it right, the only impact right now is if a user explicitly disables the permission. And I believe all permissions will still be asked for at the beginning until we increase the targetSDKVersion. So this is important to do but not critical to get in the next release, I would say.

I have a device running Android 6.0.1. I went to app preferences and revoked access to storage and location. When I revoked the first one, it gave me a warning saying that because the app was targeting an old Android version, it might not work properly.

Then when I launched Collect it gave me an error about not being able to access the SD Card and shut down right away. So that's not ideal! But it did require me to click through a warning and I think it's unlikely typical users would revoke permissions. That said, storage access would be a good one of these permissions to prioritize since revoking it makes the app unusable.

For location, GeoPoint just gave me an endless process dialog.

[nikhilbalyan]

@lognaturel you said it right normal user wouldn't revoke the permission. That said user will definetly be prompted to give the runtime permission for the very first time they will be using the feature as per Android Runtime guidelines we can't escape the first time Runtime permission check(In older version that is asked at installation hence there is no hassle).

And as far as GeoPoint is concerned that also requires the GPS access in the fusedLocationAPI Google detects the location based on the mobile network, cell Tower. Not going into more details since that will be handled in #508 but again in the callback of fusedlocationAPI while getting longitude and latittude Android studio asks us to implement a runtime permission check for android 6.0 and above.

[nikhilbalyan]

@lognaturel yeah you are right our target sdk is 22. so i think either we increase our target sdk and then implement runtime permission either there is no benefit of implementing runtime permission.

But yeah it can be beneficial when the application will be running on API 6 or above device since device with those configuration have the option to revoke permission but the device with API level below 6 don't have the option to revoke permission.

[shobhitagarwal1612]

@lognaturel We can create a small utility class which contains all permission checks being used in the app.
Then use the same class everywhere and handling the permission check results in onRequestPermissionsResult()

[yanokwa]

I wanted to revisit this issue because the deadline is coming up in November!

So I think what has to happen here for a first PR is that we wrap every call to the SD card with a runtime permission check. We do this first because that's the permission that every Collect user will need on launch. Then a second PR would wrap everything that touches the camera, then the microphone, then geo, etc.

Then when we eventually bump our SDK number, we can ship a form that exercises all the various permissions so supervisors can set up devices without requiring enumerators to decide on permissions. We might also be able to do that prompting per form so that each form that requires certain permissions can do ask for those permissions on the first launch of that form.

@lognaturel is this inline with what you were thinking at https://github.com/opendatakit/collect/issues/644#issuecomment-287210300?

[jd-alexander]

Hey everyone! I started working on an implementation for runtime storage permissions and I realized that before any READ_EXTERNAL_STORAGE or READ_EXTERNAL_STORAGE permissions can be requested, ActivityLogger is being instantiated and lifecycle methods of activities and others are making calls to it. I was thinking of putting a permissions check inside the open method and also moving loggingEnabled into the open method as well, as trying to create a file without the permissions would cause issues.
public ActivityLogger(String deviceId) { this.deviceId = deviceId; loggingEnabled = new File(Collect.LOG_PATH, ENABLE_LOGGING).exists(); open(); }

What do you guys think of this approach? All logging methods make a call to isOpen() so if the unavailability of the permissions cause the database to not be open it should allow for a lazy instantiation of the database where the logger could be opened when the permission is granted in one of the activities that are performing the grants on entry. Let me know your thoughts :)

[yanokwa]

I'm surprised that ActivityLogger is the first thing that touches the SD card. I thought the splash screen activity would be the first. Regardless, this sounds like a reasonable approach to me! What do you think, @grzesiek2010?

Just as a heads up, ActivityLogger will probably be deprecated soon (https://github.com/opendatakit/collect/issues/1395). I don't think it changes the approach but wanted to mention it in case it does...

[grzesiek2010]

When it comes to "touching the SD card" I think ActivityLogger doesn't touch it at all because we don't create its database... https://github.com/opendatakit/collect/blob/master/collect_app/src/main/java/org/odk/collect/android/database/ActivityLogger.java#L127 return is always called if you install a new app.

[jd-alexander]

You are right @grzesiek2010. I just saw the how the loggingEnabled works. So the ActivityLogger won't create the database until OdkDirs creates the LOG path. So my next question is, should I start the ActivityLogger once the path has been created? I am currently doing that in my code now but would disable if it isn't necessary.

[grzesiek2010]

but that LOG_PATH is never created looks like it works just for backward compatibility, I mean if that file exists then ok we will collect logs if not then nothing happens.

[getodk-bot]

Hello @jd-alexander, you have been unassigned from this issue because you have not updated this issue or any referenced pull requests for over 10 days.

You can reclaim this issue or claim any other issue by commenting @opendatakit-bot claim on that issue.

Thanks for your contributions, and hope to see you again soon!

[lognaturel]

👏 Storage permissions complete, thank you @jd-alexander!!

The good news is that I think this was the toughest one. I've updated the original issue so we can keep track of which ones are addressed.

[jd-alexander]

Here are some points I wanted share from a meeting I had with @lognaturel @grzesiek2010 about the ongoing permissions implementation.

• All the permissions boilerplate code is encapsulated in the PermissionUtils with the help of a permissions library called Dexter. This design decision was made because permission boilerplate can clutter the widgets and activities that need to interface with them and most of these classes already have alot of responsibilities so putting this all in a centralized place keeps the permissions code encapsulated from the rest of the app while sharing a simple api to check and grant permissions.

• Along with implementing permission checks and grants there are several changes that have been made across the different api versions, and one that has impacted the way how things are done in the Collect app is the need for permissions to share the uri to internal files to external apps eg. Camera. I currently hve a PR opened that will address that. The solution is basically a FileProvider that acts as the gateway to the files.

• When implementing storage permissions one of the main lessons I learnt was to check all the activity lifecycle methods and how they interact with the permissions implementation I was putting in place because sometimes a check might work well in onCreate but then the permission wasn't covered for onResume and the api gets consumed before the grant which causes a permission denied crash.So doing these checks and also just navigating to the Settings menu and disabling the permission after granting it helps to test these edge cases.

[lognaturel]

2506 adds the last permissions and bumps the target API to 23.