microg / GmsCore

Free implementation of Play Services
https://microg.org
Apache License 2.0
8.2k stars 1.69k forks source link

Exposure notifications: check for location permission broken #1356

Open alois31 opened 3 years ago

alois31 commented 3 years ago

I originally observed the effects of this issue in Corona Contact Tracing Germany. In short, CCTG shows a "Exposure notifications disabled" warning while still collecting keys (https://codeberg.org/corona-contact-tracing-germany/cwa-android/issues/79). After further investigations, I also observed the reverse issue (no notification and not collecting keys) and came to the conclusion that the issue is in microG. What's happening is the following:

Bubu commented 2 years ago

I'll take a look at this as it;s related to https://github.com/microg/GmsCore/pull/1606. I'm somewhat surprised that no-one complained about this since January :D

alois31 commented 2 years ago

Apparently, the current behavior is correct if microG is installed as a system app. Only for embedded microG usage the check is incorrect. I have not figured out a way of distinguishing the two situations.

Bubu commented 2 years ago

That sounds confusing... and annoying to test :-/.

I have not figured out a way of distinguishing the two situations.

That should be doable by checking if the packagename is the play-service package name.

Bubu commented 2 years ago

As a result, microG shows a warning falsely if it has location permission but location services are disabled (approximately the issue I reported in CCTG), while still collecting keys.

I can't reproduce this: Testing this on an Android 10 device with stock rom without system level microG, I don't see advertisements from the ScannerService when location service is disabled, even though the location permission is granted.

I need to make another test on Android 11 or 12 and see if that's any different there.

Perhaps more severe, it does not show a warning if it does not have location permission but location services are enabled.

This is true but it only allows activating the exposure notification system, if you have granted location permission at that time. You can later manually revoke the permission. Checking for this case can only be really done at phone startup as the service never should be restarting during normal operation, so I can add this check but not sure if it would really make any difference in practice. Maybe it would help the case if the system automatically revokes our permissions.

(one could also schedule a periodic check for this... maybe? :thinking:)

alois31 commented 2 years ago

I can't reproduce this: Testing this on an Android 10 device with stock rom without system level microG, I don't see advertisements from the ScannerService when location service is disabled, even though the location permission is granted.

I need to make another test on Android 11 or 12 and see if that's any different there.

I cannot reproduce this behavior any more either, but I don't know what changed in the meantime (there has been no system upgrade and I cannot spot relevant changes in microG). According to StackOverflow, it is an undocumented "feature" that Bluetooth scanning sometimes requires location services to be enabled and sometimes not. Interestingly, other apps (e.g. BLExplorer) can still see the beacons just fine even with location services off.

This is true but it only allows activating the exposure notification system, if you have granted location permission at that time. You can later manually revoke the permission. Checking for this case can only be really done at phone startup as the service never should be restarting during normal operation, so I can add this check but not sure if it would really make any difference in practice. Maybe it would help the case if the system automatically revokes our permissions.

(one could also schedule a periodic check for this... maybe? thinking)

I would agree that this is a corner case. I had not checked whether exposure notification could be enabled in the first place when location permission is off, but I can confirm that this is not the case. Only when later revoking location permission this point is relevant.

Bubu commented 2 years ago

Made another test on Android 12 with basically the same results as before.

One thing I noticed which might have been a cause of prior confusion: if you do the onboarding with location service enabled and then disable them you'll get scan results for a couple of minutes until the next scan cycle starts. Similarly the other way around, if you start with location services disabled, then enable them you'll have to wait for a while until the first results will show up.

Apart from that, I think the current implementation is correct for all cases.