search

Karumi / Dexter

Android library that simplifies the process of requesting permissions at runtime.
http://karumi.com
Apache License 2.0
5.23k stars 671 forks source link

Check permissions with PermissionChecker #151

Closed Serchinastico closed 7 years ago

Serchinastico commented 7 years ago

With this PR we are trying to address the issue described #140 where apps targeting SDKs lower than M are granted the permissions even though the user has rejected it through the OS settings. We are now using the newer PermissionChecker class and handling the permissions denied in a more graceful way.

johnwatsondev commented 7 years ago

Hello @Serchinastico I have tested permission-checker branch's code. I didn't approve change the DexterActivity.java file.

Because I found another issue about the special situation what I pointed in #140.

Test Device: Nexus 5 (6.0.1) Test Repo: sample in permission-checker branch build.gradle config: targetSdkVersion 15 Reproduce issue step:

  1. Please disable Contacts permission in the setting.
  2. Open the app and clicked CONTACTS button it will show request permission dialog by system.
  3. After clicked Allow button, the app will restart. (The restart behavior will not appear when targetSdkVersion >= 23)
  4. When I click Back key the app will crash. This is the crash log. Here is the activities hierarchy after restart.

Reproduce issue gif: dexter_sample_crash

I think we shouldn't invoke the requestPermissionsToSystem(Collection<String> permissions) method in DexterInstance.java hastily when the permission is denied and the device is running Android 6.0 or higher (app's targetSdkVersion is 15 ).

BTW: I just tell the user to grant the permission manually in setting for this special situation.

Here is some hint about this.

If the device is running Android 5.1 or lower, or your app's target SDK is 22 or lower: If you list a dangerous permission in your manifest, the user has to grant the permission when they install the app; if they do not grant the permission, the system does not install the app at all.

If the device is running Android 6.0 or higher, and your app's target SDK is 23 or higher: The app has to list the permissions in the manifest, and it must request each dangerous permission it needs while the app is running. The user can grant or deny each permission, and the app can continue to run with limited capabilities even if the user denies a permission request.

Serchinastico commented 7 years ago

Hi @johnwatsondev

First of all, sorry for the huge delay, my fault.

I think the crash you are experiencing was somehow unrelated to this issue and it's just a NPE that should be fixed by now in the most recent release of Dexter. Anyhow, I see your point on avoid system calls when the target SDK is < 23 or the device SDK is < 23, we are also being informed that something isn't going the way it should when we receive the PERMISSION_DENIED_APP_OP response so we are now going to put those permissions in a different impossibleToGrantPermissions list so that we don't request them to the system.

I just did some tests with:

  1. target SDK = 15 & device SDK = 23
  2. target SDK = 23 & device SDK = 22
  3. target SDK = 23 & device SDK = 23

It looks like, with the flow you described, Dexter works just fine, that is, it considers the contacts permission as permanently denied (1 and 2) and the others as granted (1 and 2), or asks for them in 3.

I just updated the PR.

Serchinastico commented 7 years ago

I'm still seeing some issues if the user decides to put the app in background and coming back from the app list but that's something that has been there for a while and therefore we are not changing with this PR

Serchinastico commented 7 years ago

You can find a snapshot version of the library with version 4.1.2-SNAPSHOT in the snapshots maven repository just in case you want to test it out.