[WIP] Fix starting Foreground services from the background #2658

avazirna · 2023-03-27T21:41:29Z

Summary

This PR resolves a restriction on Android 12+ with starting foreground services while the application is in the background, which is causing ForegroundServiceStartNotAllowedException errors. This is particularly relevant to the CommCare Session, as it needs to remain alive even when the app goes to the background for some time. There are 2 places where this happens:

When CommCare tries to start the CommCare Session Service;
When CommCare tries to send a notification to the user after a successful login;

Steps to reproduce the error

Identity a user who hasn't yet logged into the app (it's easier to reproduce during the first login) - use Clear User Data if needed
Log in to the app
Immediately after logging in, put the app in the background by pressing the home key or the using the menu key to navigate away from CommCare.

Stacktrace

android.app.ForegroundServiceStartNotAllowedException: startForegroundService() not allowed due to mAllowStartForeground false: service org.commcare.dalvik.debug/org.commcare.services.CommCareSessionService
	at android.app.ForegroundServiceStartNotAllowedException$1.createFromParcel(ForegroundServiceStartNotAllowedException.java:54)
	at android.app.ForegroundServiceStartNotAllowedException$1.createFromParcel(ForegroundServiceStartNotAllowedException.java:50)
	at android.os.Parcel.readParcelable(Parcel.java:3346)
	at android.os.Parcel.createExceptionOrNull(Parcel.java:2433)
	at android.os.Parcel.createException(Parcel.java:2422)
	at android.os.Parcel.readException(Parcel.java:2405)
	at android.os.Parcel.readException(Parcel.java:2347)
	at android.app.IActivityManager$Stub$Proxy.startService(IActivityManager.java:6969)
	at java.lang.reflect.Method.invoke(Native Method)
	at leakcanary.ServiceWatcher$install$4$2.invoke(ServiceWatcher.kt:85)
	at java.lang.reflect.Proxy.invoke(Proxy.java:1006)
	at $Proxy3.startService(Unknown Source)
	at android.app.ContextImpl.startServiceCommon(ContextImpl.java:1927)
	at android.app.ContextImpl.startForegroundService(ContextImpl.java:1893)
	at android.content.ContextWrapper.startForegroundService(ContextWrapper.java:798)
	at org.commcare.CommCareApplication.bindUserSessionService(CommCareApplication.java:830)
	at org.commcare.CommCareApplication.startUserSession(CommCareApplication.java:372)
	at org.commcare.tasks.ManageKeyRecordTask.doPostCalloutTask(ManageKeyRecordTask.java:444)
	at org.commcare.network.HttpCalloutTask.doTaskBackground(HttpCalloutTask.java:121)
	at org.commcare.network.HttpCalloutTask.doTaskBackground(HttpCalloutTask.java:34)
	at org.commcare.tasks.templates.CommCareTask.doInBackground(CommCareTask.java:40)
	at android.os.AsyncTask$3.call(AsyncTask.java:394)
	at java.util.concurrent.FutureTask.run(FutureTask.java:264)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1137)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:637)
	at java.lang.Thread.run(Thread.java:1012)
  Caused by: android.os.RemoteException: Remote stack trace:
  	at com.android.server.am.ActiveServices.startServiceLocked(ActiveServices.java:770)
  	at com.android.server.am.ActiveServices.startServiceLocked(ActiveServices.java:678)
  	at com.android.server.am.ActivityManagerService.startService(ActivityManagerService.java:14104)
  	at android.app.IActivityManager$Stub.onTransact(IActivityManager.java:2960)
  	at com.android.server.am.ActivityManagerService.onTransact(ActivityManagerService.java:3036)

Product Description

Safety Assurance

[ X ] I have confidence that this PR will not introduce a regression for the reasons below

Automated test coverage

Safety story

…-fix Fix Zip path traversal vulnerability

shubham1g5

The code itself looks great on it. Though I have a couple points worth mentioning here -

This looks like a fine soultion for the purpose of this hotfix but I think we want to have a more long term strategy here for the next release (2.54). We possibly want to move away from the model of always having a foreground service running with the notification with these new Android restrictions in place. @ctsims I believe this will be a substantial product change for users as they might be accustomed to see the CC logged-in notification. Do you have thoughts on making this change. The trade off here is asking for a new permission to all users (exact alarm) vs retaining the foreground session notification.
Have you been able to replicate this bug and verify that this PR actually fixes it ? Since it looks a bit tricky to repro this bug, it would be nice to have the repro steps mentioned in the description of this PR.

shubham1g5 · 2023-03-28T14:14:44Z

app/AndroidManifest.xml

@@ -28,7 +28,7 @@
    <uses-permission android:name="android.permission.RECORD_AUDIO" />
    <uses-permission android:name="android.permission.CAMERA" />
    <uses-permission android:name="android.permission.FOREGROUND_SERVICE" />
-
+    <uses-permission android:name="android.permission.SCHEDULE_EXACT_ALARM" />


This permission can be revoked by the user or system at any time and we need to check if the permission is granted before using it otherwise direct user to grant this permission. reference

shubham1g5 · 2023-03-28T14:16:07Z

app/src/org/commcare/services/CommCareSessionService.java

+            this.startForeground(NOTIFICATION, notificationBuilder.build());
+    }
+
+    private boolean isAppInTheBackground() {


avazirna · 2023-03-28T15:17:17Z

Have you been able to replicate this bug and verify that this PR actually fixes it ? Since it looks a bit tricky to repro this bug, it would be nice to have the repro steps mentioned in the description of this PR.

@shubham1g5 Yes, it's not that difficult to reproduce, especially when the user is logging in for the first time. So, clearing user data, logging in and immediately sending the app to the background is one way to make it happen. Let me add those steps to the description.

ctsims · 2023-03-28T19:51:30Z

Hey @shubham1g5

This looks like a fine soultion for the purpose of this hotfix but I think we want to have a more long term strategy here for the next release (2.54). We possibly want to move away from the model of always having a foreground service running with the notification with these new Android restrictions in place. @ctsims I believe this will be a substantial product change for users as they might be accustomed to see the CC logged-in notification. Do you have thoughts on making this change. The trade off here is asking for a new permission to all users (exact alarm) vs retaining the foreground session notification.

Can you start a spec doc outlining this transition, including the motivations and underlying changes to Android's permissions framework that are making it necessary?

It's definitely a very big transition to move to a model where we don't have a persistent service, because the persistent service is the mechanism we use to maintain a persistent in-memory-only footprint for the decrypted database keys without Android killing the process. Without the service, many things could happen which would require the user to re-enter their password constantly, which is something we'd like to actively avoid since most of our users are provided CommCare as the main or sole function of the phone.

It's plausible there are new mechanisms we can take advantage of for that purpose, though. We already have it on the docket to explore other ways to manage other sensitive tokens outside of process memory, so we should possibly explore these concepts together.

shubham1g5 · 2023-03-29T00:00:27Z

because the persistent service is the mechanism we use to maintain a persistent in-memory-only footprint for the decrypted database keys without Android killing the process.

Ahh I didn't realise that's the primary reason for the session service. I will see if I can find alternatives here and list them out in another doc.

shubham1g5

The code looks good to me overall except a few comments and I do think this solution will work out for this issue in short term atleast. And I am happy for this to go out as a hotfix but would like QA to test this workflow in detail before we do so.

Although I do think that the better approach here would be to get rid of foreground service altogether instead of adding another permission that user needs to grant with this special request framing.

shubham1g5 · 2023-06-12T16:42:28Z

app/src/org/commcare/CommCareApplication.java

+     * running in the background. This method leverages AlarmManager to initiate the Service
+     */
+    @RequiresApi(api = Build.VERSION_CODES.O)
+    private void startForegroundServiceWithAlarmManager() {


This method should check for the alarm permission is granted ( canScheduleExactAlarms()) before scheduling the alarm.

shubham1g5 · 2023-06-12T17:06:50Z

app/src/org/commcare/utils/Permissions.java

+        }
+    }
+
+    // TODO: In addition to scrolling to where CommCare app is, we should highlight the item


Does the page this intent send us to not CC specific and user need to locate CC from a list of apps ?

shubham1g5 · 2023-06-12T17:08:09Z

app/src/org/commcare/utils/Permissions.java

+    }
+
+    private static boolean shouldRequestSpecialPermission(AppCompatActivity activity, String specialPerm) {
+        if (!requestingSpecialAppPermission) {


what would cause requestingSpecialAppPermission to be true here ?

shubham1g5 · 2023-06-12T17:23:30Z

app/src/org/commcare/services/CommCareSessionService.java

-        //Send the notification.
-        this.startForeground(NOTIFICATION, notificationBuilder.build());
+        //Send the notification
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.S && CommCareApplication.isAppInBackground())


Have you been able to repro the crash with the foreground notification here ? I would think that since the foreground service has already started the app will always be in the foreground.

shubham1g5 · 2023-06-12T17:24:50Z

app/src/org/commcare/services/CommCareSessionService.java

+                PendingIntent.FLAG_IMMUTABLE);
+
+        AlarmManager alarmmanager = (AlarmManager) getSystemService(ALARM_SERVICE);
+        alarmmanager.setExactAndAllowWhileIdle(AlarmManager.RTC_WAKEUP, Calendar.getInstance().getTimeInMillis(), pendingIntent);


should check for canScheduleExactAlarms() before scheduling this.

shubham1g5 · 2023-06-12T17:30:23Z

I am happy for this to go out as a hotfix

Is it also possible to link the firebase crash in the PR description so that we can see how frequent it is to decide whether it should be hotfixed or not ?

avazirna · 2023-07-28T15:29:47Z

Pausing this for now until we release version 2.54. Changing the base of this PR to master.

avazirna and others added 15 commits January 19, 2023 14:17

Add hotfix version to AndroidManifest

042416e

Automated hotfix version bump

e1dddc5

Check entry path against destination path

2a8b2ea

Update message related to potential path traversal attack

8dcda3b

Merge pull request #2630 from dimagi/zip-path-traversal-vulnerability…

a287cf2

…-fix Fix Zip path traversal vulnerability

Add skeleton to trigger foreground service from the background

7aa1d18

Add broadcast receiver to start foreground component

f4a500b

Set alarm to start foreground service

59c8e36

Set alarm to trigger notification

d956bf7

Add permission to schedule exact alarms

2e8d5b5

Start foreground component from receiver

66851fc

Start foreground components from receiver

81a0ae9

Monitor app state

4da5b61

Lint

4182b49

Refactor

843620d

shubham1g5 requested changes Mar 28, 2023

View reviewed changes

avazirna added 9 commits March 29, 2023 10:36

Lint

e361721

Refactor

7136d8d

Add structure to check special permissions

3fb9c5b

Update Special app permissions

13fb705

Check Schedule Exact Alarm permission

223a3a3

Request Schedule exact alarms permission

9f62356

Handle refusal to grant special permission

d039ebd

Refactor

c8d761b

Add flag to control special permission request

1f67b3b

shubham1g5 requested changes Jun 12, 2023

View reviewed changes

avazirna changed the title ~~Fix starting Foreground services from the background~~ [WIP] Fix starting Foreground services from the background Jul 28, 2023

avazirna changed the base branch from commcare_2.53 to master July 28, 2023 15:22

avazirna added this to the 2.55 milestone Sep 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Fix starting Foreground services from the background #2658

[WIP] Fix starting Foreground services from the background #2658

avazirna commented Mar 27, 2023 •

edited

shubham1g5 left a comment •

edited

shubham1g5 Mar 28, 2023

shubham1g5 Mar 28, 2023

avazirna commented Mar 28, 2023 •

edited

ctsims commented Mar 28, 2023

shubham1g5 commented Mar 29, 2023

shubham1g5 left a comment

shubham1g5 Jun 12, 2023

shubham1g5 Jun 12, 2023

shubham1g5 Jun 12, 2023

shubham1g5 Jun 12, 2023

shubham1g5 Jun 12, 2023

shubham1g5 commented Jun 12, 2023

avazirna commented Jul 28, 2023

[WIP] Fix starting Foreground services from the background #2658

Are you sure you want to change the base?

[WIP] Fix starting Foreground services from the background #2658

Conversation

avazirna commented Mar 27, 2023 • edited

Summary

Steps to reproduce the error

Stacktrace

Product Description

Safety Assurance

Automated test coverage

Safety story

shubham1g5 left a comment • edited

Choose a reason for hiding this comment

shubham1g5 Mar 28, 2023

Choose a reason for hiding this comment

shubham1g5 Mar 28, 2023

Choose a reason for hiding this comment

avazirna commented Mar 28, 2023 • edited

ctsims commented Mar 28, 2023

shubham1g5 commented Mar 29, 2023

shubham1g5 left a comment

Choose a reason for hiding this comment

shubham1g5 Jun 12, 2023

Choose a reason for hiding this comment

shubham1g5 Jun 12, 2023

Choose a reason for hiding this comment

shubham1g5 Jun 12, 2023

Choose a reason for hiding this comment

shubham1g5 Jun 12, 2023

Choose a reason for hiding this comment

shubham1g5 Jun 12, 2023

Choose a reason for hiding this comment

shubham1g5 commented Jun 12, 2023

avazirna commented Jul 28, 2023

avazirna commented Mar 27, 2023 •

edited

shubham1g5 left a comment •

edited

avazirna commented Mar 28, 2023 •

edited