Android 6 Runtime Permissions
ClosedPublic

Authored by nicolasfella on May 15 2017, 9:42 PM.

Details

Summary

This is an early draft for supporting the Android 6 Permission System. Please test a lot and give feedback as many things can be improved

Test Plan

Ran Unit-Tests

Diff Detail

Repository
R225 KDE Connect - Android application
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
nicolasfella created this revision.May 15 2017, 9:42 PM
nicolasfella edited the summary of this revision. (Show Details)May 15 2017, 9:46 PM
nicolasfella edited the test plan for this revision. (Show Details)
nicolasfella added a reviewer: KDE Connect.

Thanks for starting to work on this! I find your approach to not be very user-friendly, though. Since the app only requests the permission after receiving a packet from the computer, if the user is not looking at the phone, it's easy to miss the popup.

I think that we can do something more similar to the Notifications plugin: check if we have the permission when the plugin is instantiated (that is: after pairing) and if it's not, the initialization function returns false and we will list it on the main screen as "disabled/not working plugins". Then, when the user taps the plugin we show a popup explaining the reason we couldn't load the plugin (we don't have enough permissions) and from there we open the permission request. When you have given permissions, the plugin can load safely.

Can you check how it is done for the Notifications plugin and try something similar?

albertvaka added a comment.EditedMay 17 2017, 2:18 PM

I actually started writing something:

https://commits.kde.org/kdeconnect-android/869814ad8726ad0fd9e51dfeedcbff74251aad3e

But I never even tested if it compiles... Was just trying to get an idea out of it.

Thanks for starting to work on this! I find your approach to not be very user-friendly, though. Since the app only requests the permission after receiving a packet from the computer, if the user is not looking at the phone, it's easy to miss the popup.

I think that we can do something more similar to the Notifications plugin: check if we have the permission when the plugin is instantiated (that is: after pairing) and if it's not, the initialization function returns false and we will list it on the main screen as "disabled/not working plugins". Then, when the user taps the plugin we show a popup explaining the reason we couldn't load the plugin (we don't have enough permissions) and from there we open the permission request. When you have given permissions, the plugin can load safely.

Can you check how it is done for the Notifications plugin and try something similar?

Actually it works this way: When the Main Activity is started the user is asked to grant the Permissions for Storage, Contacts, Phone and SMS. When something happens that needs a permission it checks whether we have this permission. If not, nothing happens. You're right, thats not very user friendly because the user doesn't know why some features are not working when he/she declines a permission. I think that not loading a plugin because of lack of permission is not wise as some plugins are useful even without them (e.g. the SharePlugin can still share/receive URLs without the storage permission).

Our first task should be identifying all the spots where certain permissions are required and make the programm not to crash when a permission is not granted.
Then we should notify the user if some features are disabled because of lack of permission

Maybe we can add a function to the Plugin class like "boolean hasAllPermissions()" and if we return false from there, we make the plugin appear in a list of "permissions needed" in the Device Activity. My goal is to put it in a very visible place so users find it even if they don't give the permissions the first time they are asked for them. What do you think?

Sounds good. I would separate between required permissions and optional permission. E.g. Telephony Permission is obviously required for the telephony plugin, but access to contacts would be optional since the telephony plugin can work without. If one required permission isn't granted we don't load the plugin and display a message like you did and provide explanation. If an optional permission isn't granted we load the plugin, but features are somewhat limited. Then we display the limited features similarly.

Optional permissions could be:
Contacts in Telephony and SMS, if not granted we display the Phone Number instead
Storage Access in Share as URLs can be shared without Storage permission

I implemented your suggestion with a little change. I separated between Plugins that could not be loaded because of lack of permission and Plugins that could not be loaded because of other errors and have separate lists. This way, it is more clear to the user. Maybe we should add an option to hide the list in case the user really doesn't want to grant a permission. This separation also makes it easier for Plugin developers. They only have to list the desired permissions and the Plugin class takes care of requesting them. This way, getErrorDialog() is only used for real errors and getPermissionExplanationDialog() for Permissions. Plugins can provide detailed explanation why permissions are necessary by setting permissionExplanation, otherwise a generic explanation is used.

What do you think?

I've just found time to compile the app with your patch and it looks great! Sorry for the delay. We will need to add a custom explanation for every plugin, but we can do that easily by overriding the getPermissionExplanationDialog(). Nicely done!

I've seen that you added a function getOptionalPermissions() that you are not using, though. Is this work in progress and you plan to use it as part of this same code review? It would be nice to have a second list of "plugins that can load, but could do more stuff if you gave extra permissions". However, we can leave this out for a second iteration and merge the current patch, because it's already functional. Feel free to tell me if you want to do more changes to this patch or if it's ready to merge :)

src/org/kde/kdeconnect/Plugins/SharePlugin/SharePlugin.java
211

Good catch :)

src/org/kde/kdeconnect/Plugins/TelephonyPlugin/TelephonyPlugin.java
291

This plugin only needs to RECEIVE_SMS, but not send them.

Right now I think it's ready to merge. I will implement optional permission when I find the time to. Overriding getPermissionExplanationDialog should not be necessary. Normally it would be sufficient to set permissionExplanation to a new value

albertvaka accepted this revision.May 31 2017, 1:31 PM
This revision is now accepted and ready to land.May 31 2017, 1:31 PM
Closed by commit R225:0b83cfe06d26: Implement Android 6 Runtime Permissions (authored by nicolasfella, committed by Albert Vaca <albertvaca@joinverse.com>). · Explain WhyMay 31 2017, 1:53 PM
This revision was automatically updated to reflect the committed changes.