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

Handle permission denied in geolocation service #22535

Closed
3 tasks done
LaurelOlson opened this issue Dec 6, 2018 · 5 comments · Fixed by michalchudziak/react-native-geolocation#1
Closed
3 tasks done
Labels
API: Geolocation Bug Help Wanted :octocat: Issues ideal for external contributors. Platform: Android Android applications. Resolution: Locked This issue was locked by the bot.

Comments

@LaurelOlson
Copy link

Environment

React Native Environment Info:
    System:
      OS: macOS High Sierra 10.13.6
      CPU: x64 Intel(R) Core(TM) i7-4770HQ CPU @ 2.20GHz
      Memory: 29.59 MB / 16.00 GB
      Shell: 5.3 - /bin/zsh
    Binaries:
      Node: 10.6.0 - /usr/local/bin/node
      Yarn: 1.7.0 - /usr/local/bin/yarn
      npm: 6.1.0 - /usr/local/bin/npm
      Watchman: 4.7.0 - /usr/local/bin/watchman
    SDKs:
      iOS SDK:
        Platforms: iOS 11.3, macOS 10.13, tvOS 11.3, watchOS 4.3
      Android SDK:
        Build Tools: 23.0.1, 23.0.3, 25.0.0, 25.0.2, 25.0.3, 26.0.1, 27.0.3, 28.0.0
        API Levels: 23, 24, 25, 26, 27
    IDEs:
      Android Studio: 3.2 AI-181.5540.7.32.5056338
      Xcode: 9.3.1/9E501 - /usr/bin/xcodebuild
    npmPackages:
      @storybook/react-native: 3.4.11 => 3.4.11
      @types/react: 16.4.14 => 16.4.14
      @types/react-native: 0.55.22 => 0.55.22
      react: 16.5.2 => 16.5.2
      react-native: 0.57.2 => 0.57.2
    npmGlobalPackages:
      react-native-cli: 2.0.1
      react-native-git-upgrade: 0.2.7

Description

On Android, if the user denies location permissions, the getCurrentPosition function in Geolocation.js does not call the geo_error callback as expected.

As per the docs regarding permissions for android SDK 23+, access must be granted for certain permissions. In the case of the location permission, the RN Geolocation service handles this in the getCurrentPosition function (with some issues). These issues could be avoided by doing the android permission request yourself but it seems like this should either be fixed or removed from the geolocation service code to avoid confusion.

It appears the correct fix would involve moving the android permission check/request code into the android LocationModule.java file, as the rest of the error handling knowledge live within the java code and this is how it is done in iOS.

We have added a temporary fix for this internally by patching getCurrentPosition so it calls the geo_error callback if permission is denied (see below) but as mentioned, this does not seem to be the proper place for this code.

Figured I would open the issue in case anyone else is running into a similar issue, and with the hopes that this will eventually get properly fixed.

export const getCurrentPosition = async function(
  geo_success: Function,
  geo_error?: Function,
  geo_options?: GeoOptions
) {
  invariant(typeof geo_success === 'function', 'Must provide a valid geo_success callback.')
  let hasPermission = true
  // Supports Android's new permission model. For Android older devices,
  // it's always on.
  if (Platform.OS === 'android' && Platform.Version >= 23) {
    hasPermission = await PermissionsAndroid.check(
      PermissionsAndroid.PERMISSIONS.ACCESS_FINE_LOCATION
    )
    if (!hasPermission) {
      const status = await PermissionsAndroid.request(
        PermissionsAndroid.PERMISSIONS.ACCESS_FINE_LOCATION
      )
      hasPermission = status === PermissionsAndroid.RESULTS.GRANTED
      
      // the patch:
      if (!hasPermission) {
        geo_error({ code: PERMISSION_DENIED_ERROR_CODE })
      }
    }
  }
  if (hasPermission) {
    RCTLocationObserver.getCurrentPosition(geo_options || {}, geo_success, geo_error || logError)
  }
}

Reproducible Demo

Create a react native app and request location via the Geolocation servie. If location is denied on android, the geo_error callback will not be triggered as it is on iOS.

@hramos hramos changed the title [Android] - handle permission denied in geolocation service Handle permission denied in geolocation service Dec 14, 2018
@hramos hramos added the Help Wanted :octocat: Issues ideal for external contributors. label Dec 14, 2018
Jyrno42 added a commit to Jyrno42/react-native that referenced this issue Dec 26, 2018
Jyrno42 added a commit to Jyrno42/react-native that referenced this issue Dec 26, 2018
@Jyrno42
Copy link
Contributor

Jyrno42 commented Dec 26, 2018

Working on this

Jyrno42 added a commit to Jyrno42/react-native that referenced this issue Dec 31, 2018
Jyrno42 added a commit to Jyrno42/react-native that referenced this issue Jan 2, 2019
…ts the Promise

fixes facebook#22535

Moves the permission request logic on Android M and above to Native side to mimic IOS behavior.

Changelog:
----------

[Android] [Fixed] - Ensure permission denied in geolocation.getCurrentPosition rejects the promise

Test Plan:
----------

Verified via my test project which requests location via Geolocation service. If the permission request
is denied by the user the promise is rejected as well.

https://github.com/Jyrno42/rn-geoloctest
Jyrno42 added a commit to Jyrno42/react-native that referenced this issue Jan 2, 2019
…ts the Promise

fixes facebook#22535

Moves the permission request logic on Android M and above to Native side to mimic IOS behavior.

Changelog:
----------

[Android] [Fixed] - Ensure permission denied in geolocation.getCurrentPosition rejects the promise

Test Plan:
----------

Verified via my test project which requests location via Geolocation service. If the permission request
is denied by the user the promise is rejected as well.

https://github.com/Jyrno42/rn-geoloctest
@hramos hramos removed the 🔶APIs label Jan 24, 2019
Jyrno42 added a commit to Jyrno42/react-native that referenced this issue Jan 25, 2019
…ts the Promise

fixes facebook#22535

Moves the permission request logic on Android M and above to Native side to mimic IOS behavior.

Changelog:
----------

[Android] [Fixed] - Ensure permission denied in geolocation.getCurrentPosition rejects the promise

Test Plan:
----------

Verified via my test project which requests location via Geolocation service. If the permission request
is denied by the user the promise is rejected as well.

https://github.com/Jyrno42/rn-geoloctest
@hramos hramos removed the Bug Report label Feb 6, 2019
Jyrno42 added a commit to Jyrno42/react-native that referenced this issue Feb 8, 2019
…ts the Promise

fixes facebook#22535

Moves the permission request logic on Android M and above to Native side to mimic IOS behavior.

Changelog:
----------

[Android] [Fixed] - Ensure permission denied in geolocation.getCurrentPosition rejects the promise

Test Plan:
----------

Verified via my test project which requests location via Geolocation service. If the permission request
is denied by the user the promise is rejected as well.

https://github.com/Jyrno42/rn-geoloctest
@Pandazaur
Copy link

Pandazaur commented Mar 15, 2019

Quick Workaround I'm using:

function getLocation(): Promise<Position, PositionError> {
    return new Promise(async (resolve, reject) => {
        if (Platform.OS === 'android') {
            // https://github.com/facebook/react-native/issues/22535
            const permission = await PermissionsAndroid.request(PermissionsAndroid.PERMISSIONS.ACCESS_FINE_LOCATION)

            if (permission === PermissionsAndroid.RESULTS.GRANTED) {
                return navigator.geolocation.getCurrentPosition(resolve, reject)
            } else {
                reject(new Error('Refused the permission ACCESS_FINE_LOCATION'))
            }
        } else {
            navigator.geolocation.getCurrentPosition(resolve, reject)
        }
    })
}

Jyrno42 added a commit to Jyrno42/react-native-geolocation that referenced this issue Mar 30, 2019
…ts the Promise

fixes facebook/react-native#22535

Moves the permission request logic on Android M and above to Native side to mimic IOS behavior.

Changelog:
----------

[Android] [Fixed] - Ensure permission denied in geolocation.getCurrentPosition rejects the promise

Test Plan:
----------

Was originally reviewed and accepted as facebook/react-native#22843 as part
of core react-native. That PR was verified via my test project which requested location via
Geolocation service. If the permission request was denied by the user the promise was rejected as well.

https://github.com/Jyrno42/rn-geoloctest
@Jyrno42
Copy link
Contributor

Jyrno42 commented Mar 30, 2019

This issue can probably be closed once GeoLocation has been removed from core RN. The fix for this bug has been filed in the new repository as: michalchudziak/react-native-geolocation#1

@janicduplessis
Copy link
Contributor

Thanks @Jyrno42, Looking at your PR now!

@chudinhbka
Copy link

Good job. Working for me

@facebook facebook locked as resolved and limited conversation to collaborators Mar 30, 2020
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Mar 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
API: Geolocation Bug Help Wanted :octocat: Issues ideal for external contributors. Platform: Android Android applications. Resolution: Locked This issue was locked by the bot.
Projects
None yet
7 participants