Fix two bugs with Location when not using ACCESS_FINE_LOCATION #10291

mikelambert · 2016-10-07T21:42:06Z

Fix two bugs with Location when not using ACCESS_FINE_LOCATION:

If we request highAccuracy=false, because we don't have ACCESS_FINE_LOCATION, but we don't have access to the NETWORK_PROVIDER...then the code should not trigger a SecurityException because it fell-back to using GPS_PROVIDER (which we don't have permission for)
If the device is pre-lollipop, and doesn't have a provider, we should detect this properly instead of letting the pre-lollipop code raise a SecurityException.

Unfortunately, SecurityExceptions cannot be caught by the calling JS code. But I am not fixing that one here, instead choosing to fix/obviate the SecurityExceptions altogether.

facebook-github-bot · 2016-10-07T21:42:33Z

By analyzing the blame information on this pull request, we identified @AaaChiuuu and @leeight to be potential reviewers.

leeight · 2016-10-08T14:05:24Z

@satya164 If this PR was merged, can we change the default targetSdkVersion to 23 or not?

mikelambert · 2016-10-08T17:44:18Z

AFAIK targetSdkVersion=23 requires that apps use an dynamic approach to requesting permissions (a la iOS) instead of the old list-it-all-in-AndroidManifest.xml.

Changing the default targetSdkVersion would force that on everyone, so would be a very major change on downstream users.

Maybe I'm missing context, or maybe that's exactly the desired plan? But I'm not sure how that relates to my change here.

facebook-github-bot · 2016-10-18T15:08:41Z

@mikelambert updated the pull request - view changes

facebook-github-bot · 2016-11-10T23:21:39Z

@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

hramos · 2016-11-11T19:56:34Z

This failed to pass some internal tests. Running them again, which is going to take a while...

satya164 · 2016-11-13T15:18:55Z

ReactAndroid/src/main/java/com/facebook/react/modules/location/LocationModule.java

+    // If it's an enabled provider, but we don't have permissions, ignore it
+    int finePermission = ContextCompat.checkSelfPermission(getReactApplicationContext(), android.Manifest.permission.ACCESS_FINE_LOCATION);
+    if (provider.equals(LocationManager.GPS_PROVIDER) && finePermission != PackageManager.PERMISSION_GRANTED) {
+      return null;


Should we call onError callback instead if ignoring it?

The existing error cases above return null, so I was just following that example. I think that's all getValidProvider can do.

It's called from two places.

When the getCurrentPosition call receives null, it will call error.invoke("No available location provider.");

When the startObserving call receives null, it will emit geolocationError and call watchPosition()'s error function.

satya164 · 2016-11-13T15:19:25Z

@leeight what @mikelambert said.

leeight · 2016-11-14T01:45:59Z

Make sense. It's not good time to upgrade to 23. So this PR LGTM.

hramos · 2016-11-14T15:41:20Z

@facebook-github-bot shipit

facebook-github-bot · 2016-11-14T18:48:11Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

mkonicek · 2016-11-14T20:35:05Z

Hey @mikelambert, can you add a test plan as described here? https://github.com/facebook/react-native/blob/master/.github/PULL_REQUEST_TEMPLATE.md

mkonicek · 2016-11-14T20:41:15Z

ReactAndroid/src/main/java/com/facebook/react/modules/location/LocationModule.java

        return null;
      }
    }
+    // If it's an enabled provider, but we don't have permissions, ignore it
+    int finePermission = ContextCompat.checkSelfPermission(getReactApplicationContext(), android.Manifest.permission.ACCESS_FINE_LOCATION);


nit: hasPermission

Why check ACCESS_FINE_LOCATION here?

Not sure exactly what you're asking, but maybe this answers it?

My app was released prior to RN, and I used COARSE but not FINE permissions. I am not ready to block my app upgrades on "new FINE permission" that RN sometimes triggers accidentally, so wanted to avoid that.

This check is to say "If they are trying to use the GPS, but they don't have a FINE permission enabled, then pretend like we don't have a GPS so we don't trigger the permissions errors that crash the app" (These are not returned through onError, but propagate as a native Java exception).

What do you mean by:

used COARSE but not FINE permissions

Does this mean declaring them in the manifest?

"new FINE permission" that RN sometimes triggers accidentally

Why does RN accidentally need a new permission? That seems like a bug.

If they are trying to use the GPS, but they don't have a FINE permission enabled, then pretend like we don't have a GPS

However, if we don't have the fine permission, we may still use Geolocation, correct?

To reply to your comment directly (as opposed to the main thread, where it appears to have been lost):

Default RN apps request FINE permissions, and so RN apps declaring FINE do not have any issues. My app only declares COARSE, and so wanted to fix RN for my use case.

I am calling navigator.geolocation.getCurrentPosition(..., {enableHighAccuracy: false}) to avoid using the GPS and just rely on the NETWORK provider.

To your question: yes, by and large the Geolocation library works just fine. Unfortunately, if there is a problem with the NETWORK provider not being enabled, then the RN code attempts to fall back to using the GPS provider.

And if this GPS provider is enabled, my app will crash with an unhandled Java exception, because it only declares COARSE permission. I am merely checking for FINE permission before using the GPS provider, to avoid this crash.

mikelambert · 2016-11-21T20:41:04Z

And @mkonicek, just to clarify in what you want in a test plan, you're looking for an explanation of what I've specifically tested to verify that this change works? (The tests Travis/CI tests are failing, but that seems independent of my change, so I hope you're not talking about that.)

lacker · 2016-12-14T23:54:35Z

Better than a test plan would just be a unit test that covers this. It seems like you should be able to put one in the Android unit tests.

lacker · 2017-02-08T21:47:44Z

Any thoughts on adding a test?

mkonicek · 2017-03-15T22:19:00Z

Sorry I'm so slow at reviewing but I realized I still don't fully understand why the fine location check is needed. I added some questions explaining (hopefully clearly?) why I don't understand it.

facebook-github-bot

@hramos has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

cpojer · 2019-02-21T07:14:54Z

Hello everyone! @mikelambert, thank you for sending this pull requests almost 2.5 years ago (WHAT!?). I'm deeply sorry we never got around to merging it, even though you provided tests.

I wanna make sure we implement your change, finally, so I took the liberty to rebase your PR but without the changes for Android Lollipop. The reasoning behind this is that so much time has passed that merging this now would just increase technical debt.

I did however include the other fix, as well as the test you wrote. Now, let's see if I can get this landed :)

pull-bot · 2019-02-21T07:15:17Z

	Messages
📖	📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.
📖	📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.
📖	📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: `[CATEGORY] [TYPE] - Message`.

Generated by 🚫 dangerJS against 2e0e2e3

…upport (That's outdated now).

facebook-github-bot

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

…upport (That's outdated now).

facebook-github-bot

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

react-native-bot · 2019-02-21T10:30:06Z

@mikelambert merged commit f32dc63 into facebook:master.

cpojer · 2019-02-21T10:35:28Z

Note I removed the test from this PR to land the change. I couldn’t get our internal build system to work with it and it couldn’t locate the DummyLocationProvider. I also couldn’t find any documentation on how to make it work. If somebody knows how to do it, I’m happy to commit the test.

Summary: Fix two bugs with Location when not using ACCESS_FINE_LOCATION: - If we request highAccuracy=false, because we don't have ACCESS_FINE_LOCATION, but we don't have access to the NETWORK_PROVIDER...then the code should not trigger a SecurityException because it fell-back to using GPS_PROVIDER (which we don't have permission for) - If the device is pre-lollipop, and doesn't have a provider, we should detect this properly instead of letting the pre-lollipop code raise a SecurityException. Unfortunately, SecurityExceptions cannot be caught by the calling JS code. But I am not fixing that one here, instead choosing to fix/obviate the SecurityExceptions altogether. Pull Request resolved: #10291 Differential Revision: D4163659 Pulled By: cpojer fbshipit-source-id: 18bb4ee7401bc4eac4fcc97341fc2b3a2a0957c9

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Oct 7, 2016

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 8, 2016

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Oct 18, 2016

satya164 reviewed Nov 13, 2016

View reviewed changes

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Nov 14, 2016

mkonicek reviewed Nov 14, 2016

View reviewed changes

mkonicek added the GH Review: needs-revision label Nov 14, 2016

mkonicek added Needs more information and removed GH Review: accepted labels Mar 15, 2017

react-native-bot added Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. Ran Commands One of our bots successfully processed a command. labels Mar 16, 2018

janicduplessis removed the Android 🤖 label Mar 19, 2018

react-native-bot added Missing Test Plan This PR appears to be missing a test plan. and removed ❔Needs More Information labels May 15, 2018

jslz mentioned this pull request Jul 11, 2018

Android app crashed on request location permission zoontek/react-native-permissions#213

Closed

facebook-github-bot reviewed Jul 19, 2018

View reviewed changes

cpojer force-pushed the location branch from bfbc75b to 12369ef Compare February 21, 2019 07:13

cpojer force-pushed the location branch from 12369ef to f9b9864 Compare February 21, 2019 07:16

cpojer added a commit to mikelambert/react-native that referenced this pull request Feb 21, 2019

Re-roll the changes made in facebook#10291 but without the Lollipop s…

f9b9864

…upport (That's outdated now).

cpojer force-pushed the location branch from f9b9864 to 8ddf0a8 Compare February 21, 2019 07:19

cpojer pushed a commit to mikelambert/react-native that referenced this pull request Feb 21, 2019

Re-roll the changes made in facebook#10291 but without the Lollipop s…

8ddf0a8

…upport (That's outdated now).

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Feb 21, 2019

facebook-github-bot approved these changes Feb 21, 2019

View reviewed changes

Re-roll the changes made in facebook#10291 but without the Lollipop s…

2e0e2e3

…upport (That's outdated now).

cpojer force-pushed the location branch from 8ddf0a8 to 2e0e2e3 Compare February 21, 2019 07:26

facebook-github-bot approved these changes Feb 21, 2019

View reviewed changes

facebook-github-bot closed this in f32dc63 Feb 21, 2019

facebook locked as resolved and limited conversation to collaborators Feb 21, 2019

react-native-bot added the Merged This PR has been merged. label Feb 21, 2019

hramos removed Import Failed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 23, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix two bugs with Location when not using ACCESS_FINE_LOCATION #10291

Fix two bugs with Location when not using ACCESS_FINE_LOCATION #10291

mikelambert commented Oct 7, 2016

facebook-github-bot commented Oct 7, 2016

leeight commented Oct 8, 2016

mikelambert commented Oct 8, 2016

facebook-github-bot commented Oct 18, 2016

facebook-github-bot commented Nov 10, 2016

hramos commented Nov 11, 2016

satya164 Nov 13, 2016 •

edited

Loading

mikelambert Nov 13, 2016 •

edited

Loading

satya164 commented Nov 13, 2016

leeight commented Nov 14, 2016

hramos commented Nov 14, 2016

facebook-github-bot commented Nov 14, 2016

mkonicek commented Nov 14, 2016

mkonicek Nov 14, 2016

mkonicek Nov 14, 2016

mikelambert Nov 21, 2016

mkonicek Mar 15, 2017

mikelambert Jun 1, 2017

mikelambert commented Nov 21, 2016

lacker commented Dec 14, 2016

lacker commented Feb 8, 2017

mkonicek commented Mar 15, 2017

facebook-github-bot left a comment

cpojer commented Feb 21, 2019

pull-bot commented Feb 21, 2019 •

edited

Loading

facebook-github-bot left a comment

facebook-github-bot left a comment

react-native-bot commented Feb 21, 2019

cpojer commented Feb 21, 2019

Fix two bugs with Location when not using ACCESS_FINE_LOCATION #10291

Fix two bugs with Location when not using ACCESS_FINE_LOCATION #10291

Conversation

mikelambert commented Oct 7, 2016

facebook-github-bot commented Oct 7, 2016

leeight commented Oct 8, 2016

mikelambert commented Oct 8, 2016

facebook-github-bot commented Oct 18, 2016

facebook-github-bot commented Nov 10, 2016

hramos commented Nov 11, 2016

satya164 Nov 13, 2016 • edited Loading

Choose a reason for hiding this comment

mikelambert Nov 13, 2016 • edited Loading

Choose a reason for hiding this comment

satya164 commented Nov 13, 2016

leeight commented Nov 14, 2016

hramos commented Nov 14, 2016

facebook-github-bot commented Nov 14, 2016

mkonicek commented Nov 14, 2016

mkonicek Nov 14, 2016

Choose a reason for hiding this comment

mkonicek Nov 14, 2016

Choose a reason for hiding this comment

mikelambert Nov 21, 2016

Choose a reason for hiding this comment

mkonicek Mar 15, 2017

Choose a reason for hiding this comment

mikelambert Jun 1, 2017

Choose a reason for hiding this comment

mikelambert commented Nov 21, 2016

lacker commented Dec 14, 2016

lacker commented Feb 8, 2017

mkonicek commented Mar 15, 2017

facebook-github-bot left a comment

Choose a reason for hiding this comment

cpojer commented Feb 21, 2019

pull-bot commented Feb 21, 2019 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

react-native-bot commented Feb 21, 2019

cpojer commented Feb 21, 2019

satya164 Nov 13, 2016 •

edited

Loading

mikelambert Nov 13, 2016 •

edited

Loading

pull-bot commented Feb 21, 2019 •

edited

Loading