search

medic / cht-android

A native Android container for Community Health Toolkit (CHT) applications
GNU Affero General Public License v3.0
25 stars 49 forks source link

Unresolvable SMS permission prompt when submitting reports for CHT < 3.9 #322

Open jkuester opened 1 year ago

jkuester commented 1 year ago

To recreate the behavior

Basically, the sending report as SMS functionality is getting triggered unintentionally and this is inadvertently triggering the SMS permission request which fails because the app does not have SEND_SMS permission enabled.

Details

Due to https://github.com/medic/cht-core/issues/6162, submitting a report in cht-android for CHT versions < 3.9.0 will always trigger an SMS record for the report to be sent regardless of the xml2sms configuration for the form. In older versions of cht-android, if your brand was not configured with the SEND_SMS this send-attempt would always cause an error that was handled silently by the app (but visible in the adb logs).

In cht-android 1.0.0, we updated handling for the SEND_SMS permission to properly request permission from the user when trying to send a report via sms. BUT this only works when the SEND_SMS permission has been configured for the app brand. The idea is that to send reports via sms this is what is needed:

  1. Publish cht-android app with SEND_SMS permission registered
  2. Configure form with xml2sms value
  3. User approves permission request when prompted by app (should happen the first time the app tries to send a report via sms).

Unfortunately, when connecting to a < 3.9.0 CHT server, #2 is bypassed (because of the above-linked issue). Then, if #1 is not done (since presumably you do not want to use the xml2sms feature), the app cannot actually request the permission from the user in #3. When that logic fails in #3, it falls back to re-directing the user to the Android settings to manually set the permission for the app. However, in this case, without #1, it is actually impossible to set the permission (and no prompt is shown to the user for how to proceed). This will continue to happen each time a report is submitted.

Workaround

The good news is that I have identified a workaround that does not require updates to cht-core or cht-android. To avoid experiencing this issue when using cht-android 1.0.0+ to connect to a CHT server that is < 3.9.0, you can set the xml2sms configuration on all your forms to "xml2sms": "false". (It needs to be the string "false" and not just the boolean primitive false.) This will prevent the behavior from https://github.com/medic/cht-core/issues/6162 and new reports submitted for those forms will NOT trigger the SMS workflow at all.

Kymoraa commented 1 year ago

Thank you for looking into this and sharing this feedback @jkuester It is really useful and helpful

jkuester commented 1 year ago

I realized I did not mention this in the original description, but one cht-android code change I have thought of that should address this issue would be to add a check to the SMS permission request workflow to short-circuit the process _if the app has not registered the SEND_SMS permission._ To even be able to prompt the user for permission to send SMS messages, the app has to have set SEND_SMS in the Manifest. If SEND_SMS, there is no point in even trying to request permission from the user (or re-direct to the Android settings) because the user cannot even manually grant the permission without SEND_SMS.

This code change would be beneficial not only because it would improve compatibility with CHT < 3.9, but also it would help make it easier to debug SMS report issues in the future! If someone is trying to configure xml2sms for some forms, but has forgotten to include SEND_SMS on their cht-android brand they will encounter this behavior even on CHT >= 3.9 (and it will probably not be obvious what it is that they are missing). If we had a check for SEND_SMS, we could at least log a coherent message saying that the SMS workflow was triggered but the permission was missing.

Kymoraa commented 1 year ago

Thanks for this update @jkuester This will still require a re-creation and re-distribution of the APKs, right? Between this and the solution to update all forms json and push the configuration changes which one is a more long term and fail proof way to go?

jkuester commented 1 year ago

@Kymoraa correct. Taking advantage of any changes made in cht-android would require re-distribution of the APKs.

Honestly, either of the SEND_SMS apk fix or the xml2sms workaround should reliably address this issue going forwards. The only disadvantage of the xml2sms workaround that I have been able to think of is that you have to make sure to include this config in any new forms that get added to the server. Not a big effort, but does require some awareness in the long-term (but this only has to be done until the server is upgraded to 3.9+). I imagine that we will probably want to include the SEND_SMS apk fix in cht-android one way or the other. So if your current 1.x apk has not been widely distributed yet, you could just update your apk and then not have to think about this again. However, if it is going to be a large effort to re-distribute a new APK, the xml2sms workaround probably makes more sense.

Kymoraa commented 1 year ago

That makes sense @jkuester A follow up question: If we later upgrade to a version greater than v3.9 e.g. v4.x, will we need to revert the xml2sms workaround and or set it to "true" for all the workflows?

jkuester commented 1 year ago

Technically no, you would not need to make any changes to the xml2sms configuration when upgrading. Having "xml2sms": "false" will continue to signal that there is no sms message to send. In >=3.9 this will behave the same as just not having any xml2sms configuration set at all.