search

expo / config-plugins

Out-of-tree Expo config plugins for packages that haven't adopted the config plugin system yet.
427 stars 91 forks source link

@config-plugins/react-native-ble-plx broken for some Android phones #149

Open marchingband opened 1 year ago

marchingband commented 1 year ago

Summary

Some android phones do not allow Bluetooth Scan.

The issue is with permissions, although it's so confusing, and so poorly documented, that I havn't got to the bottom of it yet. There is clearly a more complicated story with permissions then this plugin is handling, for various versions of Android, and various vendor specific tweaks. If the plugin can't handle the details, then a clear explanation of the issue needs to be added to the readme. I have an app in the Google Play Store with users reporting issues, and negative ratings piling up, despite testing my app on dozens of android devices, on many different Android versions.

If I add ACCESS_BACKGROUND_LOCATION to my app.json under android.permissions, then Google Play Store has a very serious issue with that, and requires me to disclose this information to the user before the permissions is invoked. Such a disclosure is not something I have ever seen in an app, despite many apps using bluetooth, so clearly something is off. Disclosing that an app is accessing users background location for no reason is not a good look, as the app does not have any need of users location.

here is a list so far of users devices that deny the scan:

thank you!!!

Config Plugin

@config-plugins/react-native-ble-plx

What platform(s) does this occur on?

Android

SDK Version

45

Reproducible demo

Any bluetooth app that scans

marchingband commented 1 year ago

This build does not have any extra permissions in app.json, android.permissions is not used at all. So this is leaving the permissions up to the plugin. It also does not have "isBackgroundEnabled" or "neverForLocation" set, so they are at the default values for those fields.

marchingband commented 1 year ago

some explanation of the complexity of the issue here: https://github.com/polarofficial/polar-ble-sdk/issues/246#issuecomment-1058869184

marchingband commented 1 year ago

It seems like very fine-grained manifest permissions are required for particular sdk versions and android versions, which isn't something that is possible in Expo using the android.permissions field, as far as I can tell. Likewise it seems very specific runtime permissions must be requested for particular Android versions and sdk versions. This all varies depending on what the application actually needs, and how these permissions are used. Again I feel like a detailed guide is needed, if this plugin is going to give expo users BLE without sending them into a world of pain when they release the application into the very heterogeneous Android ecosystem.

marchingband commented 1 year ago

I also have in my code

        let res = await PermissionsAndroid.requestMultiple(
            [
                PermissionsAndroid.PERMISSIONS.BLUETOOTH_SCAN,
                PermissionsAndroid.PERMISSIONS.ACCESS_COARSE_LOCATION,
                PermissionsAndroid.PERMISSIONS.ACCESS_FINE_LOCATION,
                PermissionsAndroid.PERMISSIONS.ACCESS_BACKGROUND_LOCATION,
                PermissionsAndroid.PERMISSIONS.READ_EXTERNAL_STORAGE,
                PermissionsAndroid.PERMISSIONS.WRITE_EXTERNAL_STORAGE,
            ]
        )

Some Android SDK versions reject some of these permissions (because they don't exist in that SDK). Is it possible that this rejection triggers some devices to then revoke Bluetooth usage?

marchingband commented 1 year ago

Anyone able to help?

olach commented 1 year ago

Same problem here. No solution found.

marchingband commented 1 year ago

I made the following changes and it works well, but I had to add a very intense popup at boot, which is getting more negative reviews. This is not the 'correct' way to do it, Android docs are very clear that the app should not request more permissions then what it needs for a specific SDK, and it is possible that this code crashes on some devices (we are getting more crashes now then in the previous version which did not request these permissions).

The popup says "This app requires location data to enable Bluetooth discovery and ensure compatibility with all Android devices, even when the app is closed or not in use". It runs before any permissions are requested, this text is taken from the Play Store email explaining the rejection of apps that do not have the popup. They call it a 'prominent disclosure'. My app does not use any location data, so this is very confusing indeed.

app.json:

    "android": {
      "permissions" : [ 
        "ACCESS_COARSE_LOCATION",
        "ACCESS_FINE_LOCATION",
        "ACCESS_BACKGROUND_LOCATION",
        "BLUETOOTH",
        "BLUETOOTH_ADMIN",
        "BLUETOOTH_SCAN",
        "BLUETOOTH_CONNECT",
        "READ_EXTERNAL_STORAGE",
        "WRITE_EXTERNAL_STORAGE"
      ]
    },
    "plugins": [
      [
        "@config-plugins/react-native-ble-plx",
        {
          "neverForLocation": true,
          "isBackgroundEnabled": true,
        }
      ]
    ]

app.js:

        let res = await PermissionsAndroid.requestMultiple(
            [
                PermissionsAndroid.PERMISSIONS.ACCESS_FINE_LOCATION,
                PermissionsAndroid.PERMISSIONS.BLUETOOTH_CONNECT,
                PermissionsAndroid.PERMISSIONS.BLUETOOTH_SCAN,
                PermissionsAndroid.PERMISSIONS.READ_EXTERNAL_STORAGE,
                PermissionsAndroid.PERMISSIONS.WRITE_EXTERNAL_STORAGE,            ]
        )
marchingband commented 1 year ago

I tried to use this code to request specific runtime permissions on various SDKs, but there were some devices (Nokia g10) which would still reject scanning. This code is taken from the links I shared above.

        const api = Device.platformApiLevel
        console.log("API level " + api)
        if (api >= 29) {
            if (api >= 31) {
                await PermissionsAndroid.request(PermissionsAndroid.PERMISSIONS.BLUETOOTH_SCAN)
            } else {
                await PermissionsAndroid.request(PermissionsAndroid.PERMISSIONS.ACCESS_FINE_LOCATION)
            }
        } else {
            await PermissionsAndroid.request(PermissionsAndroid.PERMISSIONS.ACCESS_COARSE_LOCATION)
        }
        let res = await PermissionsAndroid.requestMultiple(
            [
                PermissionsAndroid.PERMISSIONS.READ_EXTERNAL_STORAGE,
                PermissionsAndroid.PERMISSIONS.WRITE_EXTERNAL_STORAGE,
            ]
        )
olach commented 1 year ago

Thank you! This seems to fix the problem on my test device at least. Just need to test on a few more devices. And write better content to inform the user before the popups show up.

marchingband commented 1 year ago

It would be great to hear from the maintainers of this plugin (if there are any) with some kind of a roadmap on this. Permissions are supposed to be managed by the plugin under Expo, or at least that's my impression?

olach commented 1 year ago

Yes, that is my impression too.

I have not yet been able to publish a working update to my app yet. Your example did help me to get it working locally. But when submitting the aab-archive to Google Play Console, I get this error instead:

Duplicate declarations of permission android.permission.ACCESS_COARSE_LOCATION with different maxSdkVersions

So I guess I have to experiment some more to get this to work.

olach commented 1 year ago

I did manage to create a new version that was accepted by Google Play Console.

"android": {
      "permissions" : [ 
        "BLUETOOTH",
        "BLUETOOTH_ADMIN",
        "BLUETOOTH_SCAN",
        "BLUETOOTH_CONNECT"
      ]
    },
    "plugins": [
      [
        "@config-plugins/react-native-ble-plx",
        {
          "isBackgroundEnabled": true,
        }
      ]
    ]

Previously I had ACCESS_COARSE_LOCATION & ACCESS_FINE_LOCATION declared, but I guess the plugin adds those permissions automatically. So I removed those. Seems to be working, but need to test this on new and old devices.

marchingband commented 1 year ago

Interesting. I found that newer devices needed BACKGROUND_LOCATION. This permission is the one that requires the prominent disclosure. If it's not needed then that is great news.

olach commented 1 year ago

I did look into the source code to try understand what this config plugin does. I also checked the generated AndroidManifest.xml file.

With no special permissions specified in app.config.json, and with isBackgroundEnabled: true the following entries is added to the manifest file:

<uses-permission-sdk-23 android:name="android.permission.ACCESS_COARSE_LOCATION"/>
<uses-permission-sdk-23 android:name="android.permission.ACCESS_FINE_LOCATION"/>
<uses-permission android:name="android.permission.BLUETOOTH"/>
<uses-permission android:name="android.permission.BLUETOOTH_ADMIN"/>
<uses-permission android:name="android.permission.BLUETOOTH_CONNECT"/>
<uses-permission android:name="android.permission.BLUETOOTH_SCAN"/>
<uses-feature android:name="android.hardware.bluetooth_le" android:required="true"/>

Comparing this with the instructions at the Android Bluetooth docs I can see that it is mostly correct. But some issues exists.

1. BLUETOOTH and BLUETOOTH_ADMIN should be set with the maxSdkVersion:

<!-- Request legacy Bluetooth permissions on older devices. -->
<uses-permission android:name="android.permission.BLUETOOTH" android:maxSdkVersion="30" />
<uses-permission android:name="android.permission.BLUETOOTH_ADMIN" android:maxSdkVersion="30" />

2. BLUETOOTH_ADVERTISE is missing. This settings should be defined if the mode setting is set to peripheral.

3. The isBackgroundEnabled setting is confusing. This setting adds this line:

<uses-feature android:name="android.hardware.bluetooth_le" android:required="true"/>

But this line only specifies that the app requires Bluetooth LE. And is not shown in the store if your device doesn't support this. This line should not be added because of a background setting.

The setting sounds like the app needs permissions to use Bluetooth in the background. But the ACCESS_BACKGROUND_LOCATION permission is missing from the generated manifest.

4. This plugin sets some default values to the manifest, but the plugin doesn't let the full control over the default settings or how they should be overridden.

Some examples:

4a. I initially specified ACCESS_FINE_LOCATION in my app.config.json because my app need location data. This Bluetooth Config Plugins adds the same permission, so it is duplicated:

<uses-permission-sdk-23 android:name="android.permission.ACCESS_FINE_LOCATION"/>
<uses-permission android:name="android.permission.ACCESS_FINE_LOCATION"/>

This made the Google Play Console reject my app, because of the duplicated entries. To solve this, I had to remove my manually added permission, and let the plugin add this location setting. It works, but feels wrong to let a Bluetooth plugin set permissions for other responsibilities of the app. The plugin should handle this and don't create duplicates.

4b. Another scenario is if the app should only use Bluetooth, and no location/gps. For Android 12, only Bluetooth permissions is needed. But to support older devices, the location permission must be set.

If we don't want the neverForLocation setting (because some BLE beacons gets filtered), then the location permission needs to be set with a custom maxSdkVersion setting like this:

<uses-permission
    android:name="android.permission.ACCESS_COARSE_LOCATION"
    android:maxSdkVersion="30" />
<uses-permission
    android:name="android.permission.ACCESS_FINE_LOCATION"
    android:maxSdkVersion="30" />

Bottom line: I guess to improve this config plugin, we need more control over the generated manifest. Either by allowing the developer to override the default settings, or by having more config options.

@EvanBacon Any comments on this?

kimmyyyyyyyyyyyyy commented 1 year ago

Hi can you share a hello world with the React native BLe i cant install it into my project, i just learned how to use custom packages but this one fails. my email is thomasmuaddimusic@gmail.com

EvanBacon commented 1 year ago

@olach thanks for the in-depth analysis. Would you be interested in opening a PR addressing some of these issues?

kimmyyyyyyyyyyyyy commented 1 year ago

hey evan can you please share a simple app.js with @config/react-native-ble-plx, would be amazing, i dont want to hire someone overseas. just connected to the package, i've tried for 10 hrs to install it didnt work, i got other plug ins to work

olach commented 1 year ago

@kimmyyyyyyyyyyyyy If you look in the source code you will find example apps for the packages: https://github.com/expo/config-plugins/tree/main/apps/react-native-ble-plx

And remember you need to build the app (production build or dev client) to be able to use these native plugins. It will not work in the Expo Go app.

olach commented 1 year ago

@olach thanks for the in-depth analysis. Would you be interested in opening a PR addressing some of these issues?

@EvanBacon I might be able to create a PR for some of the simpler fixes, but I'm not able to do this in the near future because of holidays, other projects etc.

But a bigger question is whether or not the plugin should set these permissions automatically or not.

Not everyone using this Bluetooth plugin will use/need the same permissions. Therefore a configuration option is needed. But instead of documenting and explaining for the user what each configuration option is doing, why not simply explain for the user what kind of permissions is needed. And how to set them manually in the plugin section in app.json.

This would of course need some enhancement for how permissions is added in app.json. Right now, it's not possible to specify max/min Android sdk versions for example.

marchingband commented 1 year ago

@EvanBacon us there any hope of us getting the ability to add max/min Android sdk versions to the permissions in app.json? @olach did you happen to take a crack at this? Would a separate build plugin that takes some config, and generates the correct permissions accordingly, be a reasonable solution?

marchingband commented 1 year ago

@EvanBacon The main issue I am facing is that when I add ACCESS_BACKGROUND_LOCATION, which is required (see https://developer.android.com/guide/topics/connectivity/bluetooth/permissions) then the Play Store demands that I place a "prominent disclosure" in my app. This looks really bad, because my app does not access location. My assumption is that, if I could add the max/min to specify "Android 10 (API level 29) or Android 11", then this disclosure would not be needed. Is there any chance someone could help me clarify this, and/or be willing to help me draft a plugin that would accomplish it?

olach commented 1 year ago

@marchingband Sorry, I did not have time to create a PR for this. And for various reasons I'm not working on this specific app anymore. Hopefully someone else can create a PR to fix these issues.