Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update targetSdk to 26 and switch to dynamic permissions #325

Closed
shankari opened this issue Feb 28, 2019 · 128 comments
Closed

Update targetSdk to 26 and switch to dynamic permissions #325

shankari opened this issue Feb 28, 2019 · 128 comments

Comments

@shankari
Copy link
Contributor

starting November 1, 2018, updates to apps and games on Google Play will be required to target Android Oreo (API level 26) or higher. After this date, the Play Console will prevent us from submitting new APKs with a targetSdkVersion less than 26.

Supporting apk version 26 requires us to switch to dynamic permissions. This is not a huge issue since we already support dynamic permissions on iOS, but it does require non-trivial implementation and testing time.

It is not a coincidence that the last update of e-mission and emTripLog were updated in Oct 2019.

However, as other groups, notably @jf87 @deepalics0044 @PatGendre want to deploy custom apps to the store, we need to fix this pretty soon.

@shankari
Copy link
Contributor Author

shankari commented Mar 4, 2019

this is a cordova app, so each plugin that we choose to include can have permissions, in addition to the permissions that the native plugins that I wrote. So let's start with a list of all permissions, figure out the plugins that add them (and whether they are really necessary), and classify them into runtime versus compile time.

This is the full list of permissions.

$ grep "uses-permission" platforms/android/AndroidManifest.xml
    <uses-permission android:name="android.permission.INTERNET" />
    <uses-permission android:name="android.permission.GET_ACCOUNTS" />
    <uses-permission android:name="android.permission.WRITE_EXTERNAL_STORAGE" />
    <uses-permission android:name="android.permission.RECEIVE_BOOT_COMPLETED" />
    <uses-permission android:name="android.permission.MANAGE_ACCOUNTS" />
    <uses-permission android:name="com.google.android.gms.permission.ACTIVITY_RECOGNITION" />
    <uses-permission android:name="android.permission.ACCESS_FINE_LOCATION" />
    <uses-permission android:name="android.permission.READ_SYNC_SETTINGS" />
    <uses-permission android:name="android.permission.WRITE_SYNC_SETTINGS" />
    <uses-permission android:name="android.permission.AUTHENTICATE_ACCOUNTS" />
    <uses-permission android:name="android.permission.CAMERA" />
    <uses-permission android:name="android.permission.FLASHLIGHT" />
    <uses-permission android:name="android.permission.WAKE_LOCK" />
    <uses-permission android:name="android.permission.VIBRATE" />
    <uses-permission android:name="com.google.android.c2dm.permission.RECEIVE" />
    <uses-permission android:name="${applicationId}.permission.C2D_MESSAGE" />
    <uses-permission android:name="${applicationId}.permission.PushHandlerActivity" />

@shankari
Copy link
Contributor Author

shankari commented Mar 4, 2019

Permission Plugin Needed? Dangerous
android.permission.INTERNET edu.berkeley.eecs.emission.cordova.serversync, phonegap-plugin-push Yes Normal
android.permission.GET_ACCOUNTS cordova-plugin-email-composer, edu.berkeley.eecs.emission.cordova.auth ? Dangerous
android.permission.WRITE_EXTERNAL_STORAGE cordova-plugin-file, de.appplant.cordova.plugin.local-notification-ios9-fix, edu.berkeley.eecs.emission.cordova.auth, cordova-plugin-x-socialsharing ? Dangerous
android.permission.RECEIVE_BOOT_COMPLETED de.appplant.cordova.plugin.local-notification-ios9-fix, edu.berkeley.eecs.emission.cordova.datacollection Yes Normal
android.permission.MANAGE_ACCOUNTS edu.berkeley.eecs.emission.cordova.auth N/A Removed in API 22+
com.google.android.gms.permission.ACTIVITY_RECOGNITION edu.berkeley.eecs.emission.cordova.datacollection Yes Seems to be normal, but unsure
android.permission.ACCESS_FINE_LOCATION edu.berkeley.eecs.emission.cordova.datacollection Yes Dangerous
android.permission.READ_SYNC_SETTINGS edu.berkeley.eecs.emission.cordova.serversync Yes normal
android.permission.WRITE_SYNC_SETTINGS edu.berkeley.eecs.emission.cordova.serversync Yes normal
android.permission.CAMERA phonegap-plugin-barcodescanner ? Need to check most recent version
android.permission.FLASHLIGHT phonegap-plugin-barcodescanner ? Removed in API level 24
android.permission.WAKE_LOCK phonegap-plugin-push Need to see what the most recent version needs normal
android.permission.VIBRATE phonegap-plugin-push Yes normal
com.google.android.c2dm.permission.RECEIVE phonegap-plugin-push GCM is deprecated ???
${applicationId}.permission.C2D_MESSAGE phonegap-plugin-push GCM is deprecated ???
${applicationId}.permission.C2D_MESSAGE phonegap-plugin-push GCM is deprecated ???

@shankari
Copy link
Contributor Author

shankari commented Mar 4, 2019

ok so some obvious fixes are:

As of April 10, 2018, Google has deprecated GCM. The GCM server and client APIs are deprecated and will be removed as soon as April 11, 2019. Migrate GCM apps to Firebase Cloud Messaging (FCM), which inherits the reliable and scalable GCM infrastructure, plus many new features. See the migration guide to learn more.

  • see whether we are really using the file plugin, what for, and whether it needs that permission

@shankari
Copy link
Contributor Author

shankari commented Mar 5, 2019

see whether barcodescanner has a more recent update

We are at 7.1.0; most recent release is at https://github.com/phonegap/phonegap-plugin-barcodescanner/releases/tag/v8.0.1

The changes don't seem significant; we might as well update to address backwards compatibility.
phonegap/phonegap-plugin-barcodescanner@v7.1.0...v8.0.1

The most recent version also has FLASHLIGHT; will file an issue to remove it since it is not needed any more.

Filed phonegap/phonegap-plugin-barcodescanner#769

@shankari
Copy link
Contributor Author

shankari commented Mar 5, 2019

upgrade phonegap-plugin-push

Current 1.x release is v1.11.1
Current 2.x release is v2.2.3
We are at 1.9.2

Trying to move to v2.2.3.
Do I need to switch to a fork with phonegap/phonegap-plugin-push#2369 included?
I think we are fine, because I already split out the android v/s ios tokens and send a data-only message to the android tokens.
https://github.com/e-mission/e-mission-server/blob/master/emission/net/ext_service/push/notify_interface_impl/firebase.py#L116

@shankari
Copy link
Contributor Author

shankari commented Mar 8, 2019

ok so after upgrading, downloading the google-services.json and GoogleService-Info.plist and adding links to them in the config.xml as per the installation instructions, I get the compilation error
https://github.com/phonegap/phonegap-plugin-push/blob/master/docs/INSTALLATION.md

@shankari
Copy link
Contributor Author

shankari commented Mar 8, 2019

This is clearly related to the compatibility library. I checked the support repository against the instructions, and I am at 47+

screen shot 2019-03-07 at 4 43 14 pm

@shankari
Copy link
Contributor Author

shankari commented Mar 8, 2019

Looking at the support plugins included with various plugins,

plugins/cordova-plugin-email-composer/plugin.xml:        <framework src="com.android.support:support-v4:24.1.1+" />
plugins/cordova-plugin-x-socialsharing/plugin.xml:    <framework src="com.android.support:support-v4:24.1.1+" />
plugins/de.appplant.cordova.plugin.local-notification-ios9-fix/plugin.xml:        <framework src="com.android.support:support-v4:+" />
plugins/edu.berkeley.eecs.emission.cordova.unifiedlogger/plugin.xml:    <framework src="com.android.support:support-v4:+" />
plugins/phonegap-plugin-barcodescanner/plugin.xml:    <framework src="com.android.support:support-v4:$ANDROID_SUPPORT_V4_VERSION"/>
plugins/phonegap-plugin-push/plugin.xml:    <framework src="com.android.support:support-v13:26.+"/>

The rest of them are v4 and this is v13. Let's look at the v4 versus v13 documentation.

@shankari
Copy link
Contributor Author

shankari commented Mar 8, 2019

Looking at the NotificationCompat documentation,
https://developer.android.com/reference/android/support/v4/app/NotificationCompat.Builder.html

NotificationCompat.Builder(Context context) was deprecated in API 26.1.0. But that is what this code uses

        mBuilder = new NotificationCompat.Builder(context, channelID);

@shankari
Copy link
Contributor Author

shankari commented Mar 8, 2019

so that code was changed as part of phonegap/phonegap-plugin-push#1949
Apparently, if the app targets sdk 26+ (which we will have to), then notifications won't be received unless we use the new form of the notification builder.

This may imply that we need to update all the other plugins too. Argh!

@shankari
Copy link
Contributor Author

shankari commented Mar 8, 2019

According to this
https://stackoverflow.com/questions/45462666/notificationcompat-builder-deprecated-in-android-o
we may be able to set the channel id separately instead. But we still need to set the channelId...

@shankari
Copy link
Contributor Author

shankari commented Mar 8, 2019

opening the NotificationCompat class and checking to see where it is from, I find
~/Library/Android/sdk/extras/android/m2repository/com/android/support/support-compat/25.3.1/support-compat-25.3.1-sources.jar

why isn't the project using version 26 or 27 instead?

@shankari
Copy link
Contributor Author

shankari commented Mar 8, 2019

Aha! If I look at the SDK manager #325 (comment), it does have the Android Support Repository at 47.0.0 installed, but the Android Support Library is only 23.1.0. The example in the installation instructions does not have an entry for the Android Support Library.

Hm, let's try uninstalling the library first...

@shankari
Copy link
Contributor Author

shankari commented Mar 8, 2019

If I uninstall the library, the entry disappears, but the error persists. Checking the compat directory, I still see

ls ~/Library/Android/sdk/extras/android/m2repository/com/android/support/support-compat/
24.2.0			25.1.1			maven-metadata.xml
24.2.1			25.2.0			maven-metadata.xml.md5
25.0.0			25.3.0			maven-metadata.xml.sha1
25.0.1			25.3.1
25.1.0			26.0.0-alpha1

And I can't see any support library entries so that I can install the latest version. Apparently, this is a known issue - e.g. https://stackoverflow.com/questions/12382555/android-support-library-manual-download

@shankari
Copy link
Contributor Author

shankari commented Mar 8, 2019

digging a bit further, the new method is was added in version 26.1.0
https://developer.android.com/reference/android/support/v4/app/NotificationCompat.Builder.html#notificationcompat.builder_1

According to the libraries tab in android studio, we have three sets of support libraries.
https://stackoverflow.com/questions/25233510/how-to-find-the-version-of-library-from-gradle-dependency-android-studio

Gradle__com_android_support_support_v4_25_3_1_aar.xml
Gradle__com_android_support_support_v13_23_4_0_aar.xml
Gradle__com_android_support_support_v13_26_1_0_aar.xml

But I don't actually see the version 26.1.0 version in .../android/m2repository/com/android/support/support-v13/maven-metadata.xml

but it is in ~/.gradle/caches/modules-2/files-2.1/com.android.support/support-v13/26.1.0/. Why does gradle pick the one in maven instead of the one in gradle?

Because for some of them, the sources are in the local library.

    <SOURCES>
      <root url="jar://$USER_HOME$/Library/Android/sdk/extras/android/m2repository/com/android/support/support-compat/25.3.1/support-compat-25.3.1-sources.jar!/" />
    </SOURCES>

I'm going to try upgrading gradle since I keep getting Android Studio warnings about it. Although I am kind of paranoid about making the change since it is not easily reversible.

The warning says

To take advantage of the latest features, improvements, and security fixes, we strongly recommend that you update the Android Gradle plugin to version 3.3.1 and Gradle to version 4.10.1.

@shankari
Copy link
Contributor Author

shankari commented Mar 8, 2019

I figured it out and it was really annoying. Basically, in edu.berkeley.eecs.emission.cordova.auth/embase-openid-config.gradle, we set

configurations.all {
    resolutionStrategy.force 'com.android.support:support-v4:25.3.1'
}

This was what was forcing us to use 25.3.1 although 26.1.0 was available. I changed that to

configurations.all {
    resolutionStrategy.force 'com.android.support:support-v13:26.1.0'
}

Next, I ran into a multi-dex error, caused by a conflict between

    compile "com.google.android.gms:play-services-location:10.2+"
    compile "com.google.firebase:firebase-messaging:11.0.1"

There is not much of an explanation for the bump
phonegap/phonegap-plugin-push@576135b

After making both of them be the same version, the build succeeds. So the next step is to figure out whether we should downgrade firebase-messaging or upgrading the location services. I am tempted to upgrade the location services, although I should remember to update the motion activity code to handle the new field names...

@shankari
Copy link
Contributor Author

shankari commented Mar 8, 2019

finally getting back to this after dealing with other issues all morning. we should start with upgrading location services since:

  • we control the location stuff, and moving forward is only likely to make it more accurate
  • we don't own the push plugin and it was updated around 2 years ago, so it is unclear how it will work with the older version.

@shankari
Copy link
Contributor Author

shankari commented Mar 8, 2019

see whether we are really using the file plugin, what for, and whether it needs that permission

The file plugin is used at

www//js/services.js:            parentDir = cordova.file.dataDirectory+"../LocalDatabase";
www//js/recent.js:            parentDir = cordova.file.dataDirectory+"../LocalDatabase";

and they are both for emailing logs/data out from the app, to determine the location of the database file on iOS. So changes in android permissions should not affect us.

services.js

  this.emailLog = function() {
....
        if (ionic.Platform.isIOS()) {
            alert("You must have the mail app on your phone configured with an email address. Otherwise, this won't work");
            parentDir = cordova.file.dataDirectory+"../LocalDatabase";
        }
        alert("Going to email database from "+parentDir+"/loggerDB");
  }

recent.js

$scope.emailCache = function() {
    if (ionic.Platform.isIOS()) {
        alert("You must have the mail app on your phone configured with an email address. Otherwise, this won't work");
        parentDir = cordova.file.dataDirectory+"../LocalDatabase";
    }
    alert("Going to email database from "+parentDir+"/userCacheDB");

}

We are using version 6.0.1. Current version is 6.0.1.
It supports dynamic permissions already https://github.com/apache/cordova-plugin-file/blob/6.0.1/src/android/FileUtils.java.

So to recap:

  • we don't use this on android
  • even if we did, the call that we make doesn't require permissions
  • even if it did, they are dynamic already

So as long as we handle errors in the javascript correctly, we should be good.

@shankari
Copy link
Contributor Author

shankari commented Mar 8, 2019

cordova-plugin-email-composer

required android.permission.GET_ACCOUNTS in our version 0.8.15, but the current version 0.9.2 does not have it any more. https://github.com/katzer/cordova-plugin-email-composer/blob/0.9.2/plugin.xml

It looks like it uses the standard intent instead. So let's upgrade that.

Fails because of

/Users/shankari/e-mission/e-mission-base/platforms/android/src/de/appplant/cordova/emailcomposer/Impl.java:83: error: diamond operator is not supported in -source 1.6
        List<Intent> targets = new ArrayList<>();
                                             ^
  (use -source 7 or higher to enable diamond operator)
1 error

Also, it looks like the app doesn't automatically ask for permissions; instead the user has to ask them https://github.com/katzer/cordova-plugin-email-composer/tree/0.9.2#permissions and add the permission to the manifest.

And in 0.8.15, the plugin actually did so automatically. https://github.com/katzer/cordova-plugin-email-composer/tree/0.8.15#permissions

So it seems like for our use case, it is actually safer to stick with 0.8.15. Checking for the reason for the change to make sure this is not a boneheaded decision.

This is the commit. No reason specified, no link to any issue. I also don't see any issues related to the change by searching. Going to retain the current version.

It looks like the plugin also supports the app:// URL, not just for android but for iOS. We should try using that instead of our current workaround with files, and then we may be able to remove our dependency on the file plugin.

@shankari
Copy link
Contributor Author

shankari commented Mar 9, 2019

ok, so now I'm ready to make changes to our plugins. So far, I have:
edu.berkeley.eecs.emission.cordova.auth/

edu.berkeley.eecs.emission.cordova.comm/
edu.berkeley.eecs.emission.cordova.datacollection/
- change the google services version to 11.0.1 to be consistent with firebase
- implement runtime permissions for ACCESS_FINE_LOCATION

edu.berkeley.eecs.emission.cordova.serversync/
edu.berkeley.eecs.emission.cordova.settings/
edu.berkeley.eecs.emission.cordova.transitionnotify/
edu.berkeley.eecs.emission.cordova.unifiedlogger/
edu.berkeley.eecs.emission.cordova.usercache/

Investigate further:

  • de.appplant.cordova.plugin.local-notification-ios9-fix (WRITE_EXTERNAL_STORAGE)
  • cordova-plugin-x-socialsharing (WRITE_EXTERNAL_STORAGE)

@shankari
Copy link
Contributor Author

shankari commented Mar 9, 2019

Further investigations:

  • de.appplant.cordova.plugin.local-notification-ios9-fix does define WRITE_EXTERNAL_STORAGE.
    • checking for getExternal, we find getUriFromAsset which calls context.getExternalCacheDir(). This does not need any permissions starting in KITKAT, which is API 19. Our minSdkVersion is 18. I will retain that permission for now to maintain our minSdkVersion compatibility. If it is API 18, it will not use runtime permissions and everything will still work. If it is higher APIs, the permission is not needed anyway.
    • the most recent version of the base fork (0.9.0-beta3) is in beta. It does not define any permissions. Unclear how it works on older APIs.
    • going to leave this unchanged for now since it is not explicitly broken
  • cordova-plugin-x-socialsharing does define WRITE_EXTERNAL_STORAGE
    • checking for File in the android code, I see
      • .getExternalFilesDir which no longer requires permissions
      • getAssets which never did
      • we are at 5.2.1, latest is 5.4.4. Current version also includes WRITE_EXTERNAL_STORAGE

So I think we are fine wrt follow ups even without updating anything.

@shankari
Copy link
Contributor Author

shankari commented Mar 9, 2019

Made all changes in #325 (comment) except implementing runtime permissions. Tried to rebuild. Ran into persistent error that I don't remember.

Removed android platform, re-added android platform.

Now getting failure with

AGPBI: {"kind":"error","text":"error: resource android:attr/ttcIndex not found.","sources":[{"file":"/Users/shankari/.gradle/caches/transforms-1/files-1.1/appcompat-v7-25.3.1.aar/ac54b5ada41f1975b65c69e36aa22b4e/res/values/values.xml","position":{"startLine":202,"startColumn":4,"startOffset":24572,"endColumn":68,"endOffset":24636}}],"original":"","tool":"AAPT"}
/Users/shankari/e-mission/e-mission-base/platforms/android/build/intermediates/incremental/mergeDebugResources/merged.dir/values/values.xml:267: error: resource android:attr/fontVariationSettings not found.
/Users/shankari/e-mission/e-mission-base/platforms/android/build/intermediates/incremental/mergeDebugResources/merged.dir/values/values.xml:267: error: resource android:attr/ttcIndex not found.

which is the same as https://stackoverflow.com/questions/49171052/error-android-resource-linking-failed-aapt2-27-0-3-daemon-0

The issue seems to be the use of version 27.1.0 of the support libraries. But how did it work when I just edited the gradle file? Maybe things weren't cleaned enough until I removed and re-added the values?

At any rate, the appcompat dependency is loaded from

+--- com.auth0.android:jwtdecode:1.1.1
|    +--- com.google.code.gson:gson:2.7 -> 2.8.5
|    \--- com.android.support:appcompat-v7:25.3.1
|         +--- com.android.support:support-annotations:25.3.1 -> 26.1.0
|         +--- com.android.support:support-v4:25.3.1 (*)
|         +--- com.android.support:support-vector-drawable:25.3.1
|         |    +--- com.android.support:support-annotations:25.3.1 -> 26.1.0
|         |    \--- com.android.support:support-compat:25.3.1 (*)
|         \--- com.android.support:animated-vector-drawable:25.3.1
|              \--- com.android.support:support-vector-drawable:25.3.1 (*)

which is included in the auth library <framework src="com.auth0.android:jwtdecode:1.1.1"/>
I guess that is why we had to specify the gradle file?

But why do we even need the com.auth0.android:jwtdecode framework? that doesn't seem to be a dependency of AppAuth-Android...

@shankari
Copy link
Contributor Author

shankari commented Mar 9, 2019

But why do we even need the com.auth0.android:jwtdecode framework? that doesn't seem to be a dependency of AppAuth-Android...

No, but we use it to decode the JWT and extract the email in our code.
https://github.com/e-mission/cordova-jwt-auth/blob/master/src/android/OpenIDAuth.java#L207

Switched to the most recent version of com.auth0.android:jwtdecode and forced the use of appcompat v27. still didn't help. Then I found this entry which indicated that one of the included libraries had been compiled with API 28.
https://stackoverflow.com/a/54125070/4040267

So I grepped around to figure out which library this was in my case.

$ grep -r android:fontVariationSettings ~/.gradle/caches/transforms-1/files-1.1/
/Users/shankari/.gradle/caches/transforms-1/files-1.1//support-compat-28.0.0.aar/18ae3a5455fcd714f8848baafe002aa3/res/values/values.xml:        <attr name="android:fontVariationSettings"/>
/Users/shankari/.gradle/caches/transforms-1/files-1.1//support-compat-28.0.0-rc02.aar/40fcdc5b24df8850d365b77b72a4f7f6/res/values/values.xml:        <attr name="android:fontVariationSettings"/>

and it was support-compat. checking the dependencies, we see what is going on.
Basically, since we specified that the v4 support libraries were 24.1.1+, gradle picked version 28.

+--- com.android.support:support-v4:24.1.1+ -> 28.0.0
|    +--- com.android.support:support-compat:28.0.0
|    |    +--- com.android.support:support-annotations:28.0.0
|    |    +--- com.android.support:collections:28.0.0
|    |    |    \--- com.android.support:support-annotations:28.0.0

We can't really fix the versioning (24.1.1+) by changing to (24.1.1), since some are from other plugins.

plugins//cordova-plugin-email-composer/plugin.xml:        <framework src="com.android.support:support-v4:24.1.1+" />

so we fix by setting both the v4 and v13 support library versions in jwtauth. Should really move to cordova-android-support-gradle-release instead of piggy-backing on auth.
https://stackoverflow.com/a/49200782/4040267

*** Build succeeded!! ***

Now just need to fix dynamic permissions, and we are done!

@deepalics0044
Copy link

any update on this? Before launching the app we need to pull from master and build it again.

@shankari
Copy link
Contributor Author

After the weekend, let's get back to testing this. First failure: the broadcast receiver doesn't receive any broadcasts. I tested this with adding breakpoints in the code and we send the broadcast but don't receive it. A brief search finds this breaking change in API 26.
https://developer.android.com/guide/components/broadcast-exceptions

We can fix this in one of three ways:

  • Apps can continue to register for explicit broadcasts in their manifests.
  • Apps can use Context.registerReceiver() at runtime to register a receiver for any broadcast, whether implicit or explicit.
  • Broadcasts that require a signature permission are exempted from this restriction, since these broadcasts are only sent to apps that are signed with the same certificate, not to all the apps on the device.

The original thought behind defining implicit broadcasts was that other apps could receive them and perform actions, which would make it easier to build interoperable apps. However, we have since moved away from that approach, and towards plugins that can be directly incorporated into other apps. Note that we are getting pushback for the two step process already; it is likely to be even less likely that people will want to actually install two separate apps.

So barring a strong use case for multiple cooperating apps, I am going to go with an explicit broadcast, configured with the package name

@shankari
Copy link
Contributor Author

So barring a strong use case for multiple cooperating apps, I am going to go with an explicit broadcast, configured with the package name

Changing the intent creation to specify the packagename of the app works.

        i.setPackage(ctxt.getPackageName());

need to figure out where we should put that for the long-term though. Not sure that all classes should import TripDiaryStateMachineReceiver, but also not sure that we should copy that code around everywhere.

Once this is resolved, the next error is the expected one for permissions.

    java.lang.SecurityException: Client must have ACCESS_FINE_LOCATION permission to request PRIORITY_HIGH_ACCURACY locations.
        at edu.berkeley.eecs.emission.cordova.tracker.location.actions.GeofenceActions.readAndReturnCurrentLocation(GeofenceActions.java:114)
        at edu.berkeley.eecs.emission.cordova.tracker.location.actions.GeofenceActions.create(GeofenceActions.java:76)
        at edu.berkeley.eecs.emission.cordova.tracker.location.TripDiaryStateMachineService$3.run(TripDiaryStateMachineService.java:499)
        at java.lang.Thread.run(Thread.java:764)
2019-03-11 13:56:38.230 10661-10661/edu.berkeley.eecs.embase D/CordovaActivity: Paused the activity.

We could:

  • check the ACCESS_FINE_LOCATION in the service, before we perform any actions
  • check the ACCESS_FINE_LOCATION permission in the actions

But the challenge is that both hasPermission and requestPermission are currently defined in the cordova interface (e.g. cordova.hasPermission), which is not really available from the broadcast receiver or the services. Even the PermissionHelper class that they created for backwards compatibility uses a CordovaPlugin class as a parameter.

So I think that the plugin should actually do all the checking, and in the service and actions, we should just generate a tracking error if we don't have permissions, and move to the start state, just like when location services are turned off and on. The good news is that we should be able to re-use a lot of the same logic in the FSM.

Before we commit to this, let's ensure that passing in the cordova interface or the plugin doesn't really work.

@shankari
Copy link
Contributor Author

Before we commit to this, let's ensure that passing in the cordova interface or the plugin doesn't really work.

Actually I don't see how I can even pass it in. The broadcast receiver is defined in the manifest and launched automatically by the system, not the cordova plugin. So there is no plugin in the interface that we can pass in. Note also that we shouldn't really check the permissions unless the user has consented.

@shankari
Copy link
Contributor Author

It seems like the overall correct design is to check for permission errors in the actions, which is where we perform the dangerous operations. That will allow each action to check for its own permission. However, if an action forgets, we should still have a backup in the service. And given that there is only one dangerous permission that we really need, that will make it much simpler than spreading the checks across a bunch of places.

Ok, so here's the final plan:

  • in the plugin, if the user has consented, we check for permissions on init (similar to checking for google play services initialization)
  • we won't get to the service unless the user has consented, so we again check for permissions in the service before connecting to google play services. If we don't have permissions, we generate a notification, but don't request permissions since we don't have a UI for the user to consent to.

@shankari
Copy link
Contributor Author

first going to convert all broadcasts to explicit broadcasts to keep the commits clean

Converted everything in the tracker package
Screen Shot 2019-03-11 at 3 43 35 PM

Fixed the one use in the transition notify package

        genericTransitionIntent.setPackage(context.getPackageName());

@shankari
Copy link
Contributor Author

@shankari
Copy link
Contributor Author

I have updated the master branches on both the e-mission (e-mission/e-mission-phone#544) and emTripLog (e-mission/e-mission-base#17) repositories with the plugin versions upto the patch release.

@deepalics0044 @ipsita0012 @PatGendre @MythriNagaraju @jf87 you can pull and re-build your apks now if you like. Note that by default, updating config.xml (such as by pulling these changes) DOES NOT UPDATE cordova plugins (https://stackoverflow.com/questions/40268029/updating-cordova-plugins-according-to-config-xml)

Since the update involves lots of plugins, the stackoverflow workaround is too onerous. The steps I've been following are:

bin/configure_xml_and_json.js cordovabuild (if using e-mission-phone as the repo)
rm -rf platforms
rm -rf plugins
cordova platform add android (instead of cordova prepare)
cordova plugin list (compare the versions to the ones in config.xml)
cordova build android

Note that the changes in this issue only affect android, so adding the android platform alone is sufficient. If you haven't pulled recent changes to iOS, you might want to update and rebuild ios (e.g. cordova platform add ios) as well

Given the length of this issue, if you have any problems with building, please file a new issue that links to this one

Good luck!

@deepalics0044
Copy link

Thank you! Okay let's get started. :)

@shankari
Copy link
Contributor Author

@deepalics0044 thanks to the Indian team for testing this release. Please continue testing, check in with your testers periodically, and report any issues proactively. We will all thank you!

@PatGendre
Copy link
Contributor

Hi, we updated the phone app, it is not working as expected, on the simulator and on my android4 phone.

  1. When starting the app I am not asked my consent for app permissions.
  2. the tracking button in the profile behaves bizarrely ; when toggled e.g. from the tracking state, it displays that the tracking has been stopped but the button stays on the tracking position ; moreover, the tracking icon on the top bar isn"t displayed. When toggling a second time, the tracking button goes the the off position but the message says the tracking is now on and the tracking icon is displayed on the top bar.
    Tell what further tests I should do or complementary info you need.

@shankari
Copy link
Contributor Author

shankari commented Apr 5, 2019

@PatGendre first, does this work in the published app (emTripLog)? Comparing against that will help determine whether the issue is with the rebuilt app or with the platform in general.

Also, which emulator (api 23, api 26, api 28?) are you using?

@PatGendre
Copy link
Contributor

@shankari Ok, I installed already the emtriplog app connected to the Sydney server (with the mode/purpose feature), a few days ago, and it worked.
But I've just reinstalled it and the behavior seems similar : I am not asked for permission consent when lauching the app, and the Tracking toggle button in the profile seems to be behave like I have described (message saying starting tracking when I switch off the button, and no location icon on the top Android bar)....
I ask Yann for the api version of the emulator.

@shankari
Copy link
Contributor Author

shankari commented Apr 5, 2019

@PatGendre so the only change from 1.2.0 (which I assume was the version you installed earlier) to 1.2.1 was to change the notification priority to low to turn off the buzzing, and some text strings, neither of which should affect the permissions.

And on android4 (< 5.1.1), you should never have been prompted for the permissions anyway because it doesn't support runtime permissions (https://developer.android.com/guide/topics/permissions/overview#install-time_requests_android_511_and_below)

So I'm confused. Can you clarify what you mean by

I am not asked for permission consent

I think that is probably the real issue. The toggle behavior is just confusing because the FSM is in the start state.

@PatGendre
Copy link
Contributor

Ok thanks so for the permissions that's normal that I am only asked for permissions when installing the app and not later.
I'll test a little more to understand how works the tracking button.

@shankari
Copy link
Contributor Author

shankari commented Apr 5, 2019

so when you are checking the tracking button, can you also check the FSM state (in Developer Zone)?
I bet it is in start state. Do you have location services turned on?

@PatGendre
Copy link
Contributor

yes it is in wainting_for_trip_start
the location services are turned on, the tracking is on, when I toggle tracking, the message is "local transition.start.tracking" and the tracking button stays on (green). When I toggle again, the message is "local transition.stop.tracking and the tracking button is still on (green) and the location icon disappears. When I toggle again, at last the tracking turns off. And if I toggle again, the button works but the message is saying stop when location is put on and vice-versa. And the location icon doesn't appear again.
Then I tried to activate/deactivate twice the android location service, and when I toggle again the tracking service in e-mission profile, the location icon on the top bar finally reappears but it is kind of erratic.

By the way I have another question related to location service. When I activate it, I am prompted wether I want to activate enhanced location services. Is the e-mission tracking influenced when I choose to activate these? (which I usually do)

@shankari
Copy link
Contributor Author

shankari commented Apr 5, 2019

when I toggle tracking, the message is "local transition.start.tracking" and the tracking button stays on (green). When I toggle again, the message is "local transition.stop.tracking and the tracking button is still on (green) and the location icon disappears. When I toggle again, at last the tracking turns off. And if I toggle again, the button works but the message is saying stop when location is put on and vice-versa. And the location icon doesn't appear again.

Let me investigate this. As you can see from the testing done, I focused on testing the toggle of the location services (from the settings) instead of the button in the e-mission UI. This may be a regression, and I think I might even have a rough idea of why it is happening.

@shankari
Copy link
Contributor Author

shankari commented Apr 5, 2019

By the way I have another question related to location service. When I activate it, I am prompted wether I want to activate enhanced location services. Is the e-mission tracking influenced when I choose to activate these? (which I usually do)

By default, e-mission uses geofencing for the tracking off/on, which requires the enhanced services.

If you configure your version of the platform to not duty cycle (currently testable by editing the settings in the developer zone), then it does not require geofencing. But we use the fused API (that combines WiFi with GPS) by default even for ongoing location tracking so I think that we will always require enhanced services.

There is currently no setting that allows you to use the non-fused API, and I don't think that there will ever be. IIRC on Android P, you cannot even choose basic v/s enhanced; it is always enhanced.

@PatGendre
Copy link
Contributor

Thanks for the explanation on the enhanced location services.
As for toggling the location services settings, I noticed also that if I switched them off, the state remains in "waiting_for_trip_start" state (even if I refresh the profile); the state is modified by toggling the Tracking button, though.
Moreover, I don't know how I can make the location icon displayed on the top bar. Sometimes it is displayed, sometimes not; maybe does it depend on whether I am leaving the geofence? (because I did not test this)

@PatGendre
Copy link
Contributor

PatGendre commented Apr 8, 2019

Also, which emulator (api 23, api 26, api 28?) are you using?

The emulator uses API 27

@shankari
Copy link
Contributor Author

shankari commented Apr 8, 2019

wrt #325 (comment)
this was because the plugins were not updated (@PatGendre can you confirm?)

To reiterate, it is NOT sufficient to just pull and get an updated config.xml. You have to ensure that the plugins are updated as well - see (see #325 (comment))

I will investigate the location tracking button in the e-mission UI today.

@shankari
Copy link
Contributor Author

shankari commented Apr 8, 2019

Experimented with the location tracking button and it appears to be a UI issue.

Could it be related to #239
No, that was fixed in e-mission/e-mission-phone@16a666d#diff-d05c4e64121a9abba5740e74e37c3688

Filed new issue #367 to avoid cluttering this one.

@PatGendre
Copy link
Contributor

@shankari : thanks a lot, I tested the #367 issue with the updated emtriplog UI and the Tracking button works perfectly well :-) We will update the UI (and update the plugins!).
I still do not understand how the location settings interact with the tracking icon (and button) but that is another question.

@shankari
Copy link
Contributor Author

I am going to close this since @PatGendre and @MythriNagaraju have both run the rebuilt android app with no major issues. If we find anything later, we can just open new issues.

@jf87
Copy link
Contributor

jf87 commented May 13, 2019

@shankari @PatGendre

It seems for me Android motion activity data is not anymore collected since that update. Is this just an issue on my side or do you observe the same? Thanks :-)

@shankari
Copy link
Contributor Author

You need to upgrade the server as well (e-mission/e-mission-server#642).

@jf87
Copy link
Contributor

jf87 commented May 14, 2019

Thanks a lot @shankari ! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants