Handle runtime permissions in K-9

[philipwhiuk]

Expected behaviour

K-9 should:

• Clearly denote why it uses each permission
• Handle loss of permissions without crashing
• Handle loss of non-critical permissions without loss of core functionality
• Properly display information about each of the permissions it exposes.

Actual behavior

• If you remove the Contacts permission the application crashes.

01-21 19:52:43.872 7635-8133/com.fsck.k9.debug E/AndroidRuntime: FATAL EXCEPTION: AsyncTask #2
   Process: com.fsck.k9.debug, PID: 7635
   java.lang.RuntimeException: An error occurred while executing doInBackground()
       at android.os.AsyncTask$3.done(AsyncTask.java:325)
       at java.util.concurrent.FutureTask.finishCompletion(FutureTask.java:354)
       at java.util.concurrent.FutureTask.setException(FutureTask.java:223)
       at java.util.concurrent.FutureTask.run(FutureTask.java:242)
       at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1133)
       at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:607)
       at java.lang.Thread.run(Thread.java:761)
    Caused by: java.lang.SecurityException: Permission Denial: opening provider com.android.providers.contacts.ContactsProvider2 from ProcessRecord{9aee797 7635:com.fsck.k9.debug/u0a119} (pid=7635, uid=10119) requires android.permission.READ_CONTACTS or android.permission.WRITE_CONTACTS
       at android.os.Parcel.readException(Parcel.java:1684)
       at android.os.Parcel.readException(Parcel.java:1637)
       at android.app.ActivityManagerProxy.getContentProvider(ActivityManagerNative.java:4199)
       at android.app.ActivityThread.acquireProvider(ActivityThread.java:5476)
       at android.app.ContextImpl$ApplicationContentResolver.acquireUnstableProvider(ContextImpl.java:2239)
       at android.content.ContentResolver.acquireUnstableProvider(ContentResolver.java:1517)
       at android.content.ContentResolver.query(ContentResolver.java:516)
       at android.content.ContentResolver.query(ContentResolver.java:474)
       at com.fsck.k9.activity.compose.RecipientLoader.fillContactDataFromQuery(RecipientLoader.java:188)
       at com.fsck.k9.activity.compose.RecipientLoader.loadInBackground(RecipientLoader.java:110)

If you try and save an attachment with no Storage permission

01-21 20:32:12.880 12365-12600/com.fsck.k9.debug E/k9: Error saving attachment
java.io.FileNotFoundException: /storage/emulated/0/download.jpeg (Permission denied)
    at java.io.FileOutputStream.open(Native Method)
    at java.io.FileOutputStream.<init>(FileOutputStream.java:221)
    at java.io.FileOutputStream.<init>(FileOutputStream.java:169)
    at com.fsck.k9.ui.messageview.AttachmentController.writeAttachmentToStorage(AttachmentController.java:151)
    at com.fsck.k9.ui.messageview.AttachmentController.saveAttachmentWithUniqueFileName(AttachmentController.java:141)
    at com.fsck.k9.ui.messageview.AttachmentController.access$900(AttachmentController.java:42)
    at com.fsck.k9.ui.messageview.AttachmentController$SaveAttachmentAsyncTask.doInBackground(AttachmentController.java:348)
    at com.fsck.k9.ui.messageview.AttachmentController$SaveAttachmentAsyncTask.doInBackground(AttachmentController.java:337)
    at android.os.AsyncTask$2.call(AsyncTask.java:305)
    at java.util.concurrent.FutureTask.run(FutureTask.java:237)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1133)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:607)
    at java.lang.Thread.run(Thread.java:761)

• If you view the 'Additional Permissions' screen:

Steps to reproduce

1. (Possibly) Run a version of the app targeting SDK version 23 or higher
2. Remove the contacts permission
3. Try to type an email.

Environment

K-9 Mail version: e238ee5

Android version: 7.1.1

Account type (IMAP, POP3, WebDAV/Exchange): N/A

[philipwhiuk]

Notes on importance:

This prevents updating beyond SDK 22 ( #2108 ) which prevents fixing #781 and #857

One PR ( #1860 ) started looking into this but will only be a small part in fixing the whole thing.

[philipwhiuk]

The three commits I have so far cover:

• Saving attachments (requests Storage if necessary)
• Saving settings (requests Storage if necessary)
• Reading contacts (requests Storage if necessary) for displaying email (checks for access only)

Next I need to look at the RecipientSelectView stuff for reading contacts while writing email.

If anyone has any feedback about when we should request Contacts access please leave your comments.

[cketti]

Switching to runtime permissions is a serious change in the user experience. I think we should figure out how to do this properly before attempting to write any code. This is something for the design overhaul, not something we do in between.

I think the only two permissions we need to worry about are contacts and storage permission. I'm not sure yet what the best time to request the contacts permission is. For an email app it's quite useful to have, but not essential. Asking for it during onboarding might seem strange to users because it's not used right away. Maybe we wait until the user starts composing a message.

Ideally, I want to get rid of the storage permission (on newer Android versions). I believe the storage access framework should make this possible for all use cases other than moving the account database to external storage. Since nowadays "external storage" really is on internal storage anyway, I think we can get rid of that "feature", too.

[gdt]

For me, the UX problem with permissions (in general) is that it's hard to make clear why the permission is requested and the consequences. For contacts, I agree that technically one could use K-9 without contacts access, but that seems like a fringe case even to me, because email contents seem more likely to be sensitive than contacts, and if one doesn't trust K-9 to behave, there's no hope. But, I could see presenting a screen that says "K-9 is about to request access to contacts, which will be used to let you choose who to mail to and [other]. If you do not permit access to contacts, K-9 will still work, but you will have to type all email addresses exactly, with no autocompletion". Then a "continue" button to launch the permissions dialog.

I'm not 100% clear on storage, but one thing K-9 does that I think is important to retain is to export settings to external storage. (I realize "external" is often internal, but to me the point is that this storage can be read by other apps so you can copy files off easily, e.g. using syncthing to sync the entire external storage to a regular computer.) But, it seems fine to defer asking for permission to do that until it is needed.

[cketti]

Exporting settings using the storage access framework (SAF) will allow you to select the destination. That includes device storage, device-external storage and everything else that hooks into SAF, e.g. Google Drive.

[philipwhiuk]

I'm all in favour of improving use of storage. But if we have to perfect our use of storage in order to update the targetSdkVersion (which we have to do to solve Doze) then that feels very broken to me.

The commits I have make the app not crash with runtime permissions set. They ask for Storage permissions sensibly to enable the way it currently behaves to work and they don't bother asking for Contacts permissions yet. I figure we make the app ask for contact permissions on upgrade and then we open an issue up to make better use of the storage architecture.

Leaving the app stuck on 22 (and breaking core functionality like sync) until we get round to handling a very low priority issue (for me at least) of exporting settings seems absurd.

But, I could see presenting a screen that says "K-9 is about to request access to contacts, which will be used to let you choose who to mail to and [other]. If you do not permit access to contacts, K-9 will still work, but you will have to type all email addresses exactly, with no autocompletion". Then a "continue" button to launch the permissions dialog.

That's pretty much the advice of the Android documentation. Course this makes this issue even larger (and so less likely be to resolved). I'm much more in favour of incremental improvements that make the app better each time and can lead to rapid user feedback than massive sweeping changes that take ages to get released and then still have to be iterated on anyway following user feedback. We can go back and add these screens later.

[ArchangeGabriel]

I think that Contacts is something that should be asked upon first opening of the app (either after installation or upgrade). And I also agree with @gdt with this message, but as long as this doesn’t go into oblivion, I think @philipwhiuk is right that releasing a first version making use of runtime permissions and keep that message for later is the way to go.

[Valodim]

Is there a reason we can't have runtime permissions even while targetting api level < 23? Users can revoke permissions, and we can handle them in the more clear cases (like saving attachments) even if we don't fully support them in all places yet. That'll allow us to handle some more situations than we do right now, and yield better feedback for those implementations.

[cketti]

I don't understand what you're asking. Runtime permissions can't be used without targetSdkVersion=23 or higher.

[Valodim]

I'm refering to this comment in the docs:

Note: Beginning with Android 6.0 (API level 23), users can revoke permissions from any app at any time, even if the app targets a lower API level. You should test your app to verify that it behaves properly when it's missing a needed permission, regardless of what API level your app targets.

which sounds like we can check permissions even with lower sdk, we just don't have to? Maybe not.

[cketti]

That'll trigger a compatibility behavior. E.g. when querying contacts you'll get an empty list, rather than a SecurityException.

[Olf0]

Please mind that "runtime permission management" (the ability for users to retract permissions granted upon installation of an app) is also relevant for API levels < 26:
While "stock" Android exposes this feature since Android 8 (IIRC), it has been offered in CyanogenMod / LineageOS since long ago (IIRC since API level 20) and (WRT contacts) in AlienDalvik (API levels < 20; as @cketti and I discovered while analysing issue #3498).

Thus please make the proper handling of retracted permissions available on all API levels supported by K-9 Mail.

[rugk]

Yes, but AFAIK in Cyanogenmod/LineageOS apps cannot influence/work with the system there, i.e. there are no extra/special APIs for that. It's just you can pay attention your app does not crash when a permission is not there, which you might expect, but I guess that works.

[Olf0]

Yes, but AFAIK in Cyanogenmod/LineageOS apps cannot influence/work with the system there, i.e. there are no extra/special APIs for that.

Ack.
IIRC an access is simply denied, even though the specific permission for it has been requested and granted upon installation.

It's just you can pay attention your app does not crash when a permission is not there, which you might expect, but I guess that works.

Yeah, it would be really nice if K-9 Mail would do that (instead of crashing)!

[rugk]

IIRC an access is simply denied, even though the specific permission for it has been requested and granted upon installation.

Exactly.

Yeah, it would be really nice if K-9 Mail would do that (instead of crashing)!

Does it do so?

[Olf0]

@rugk, yes, see issue #3498 and the original report by @philipwhiuk above, in the section Actual Behavior.

[fat-tire]

I haven't looked at the state of the current build or touched k9 for months but if I remember correctly I had fixed this back in January and was rejected. See commits here.

[cketti]

The UX of requesting permissions can certainly be improved. But I think we have the functionality covered now. Closing this issue.