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.

Sign up for GitHub

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

Jump to bottom

[Geolocation] Provide option to request specific location permissions #15067

Closed

yatchoi wants to merge 2 commits into facebook:master from yatchoi:ios/authorization-fix

yatchoi commented Jul 18, 2017

Context

On IOS, the React Native Geolocation library will make system permission requests based on the capability of the app as defined in Info.plist.

This PR allows the option for feature developers to specify the exact level of location permission request to make, by passing an argument in as part of the options parameter. This is important in scenarios where user privacy and permission regarding location-based features needs to be specifically scoped.

This will prevent situations where apps will ask users for background location permission, even though certain features may only require location in the foreground.

Changes

Add locationAuthorizationIOS as an option in GeoOptions
Expose Geolocation.IOS_AUTHORIZATIONS as a String enum
Add locationAuthorization as an argument for beginLocationUpdatesWithDesiredAccuracy
Make location requests either based on the option, or fall back to the old Info.plist approach

Test Plan

Tested in simulator, using an app with Background Location capability, called beginLocationUpdatesWithDesiredAccuracywith various authorizations requested.

Contributor

facebook-github-bot commented Jul 18, 2017

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot added the GH Review: review-needed label

eslint-bot reviewed

View reviewed changes

Libraries/Geolocation/Geolocation.js Outdated

		@@ -186,4 +187,8 @@ var Geolocation = {
		}

eslint-bot Jul 18, 2017

null: Parsing error: Unexpected token, expected ;

189 |
190 | Geolocation.IOS_AUTHORIZATIONS = {};

191 | Geolocation.IOS_AUTHORIZATIONS.WHEN_IN_USE: "locationAuthorizationWhenInUse";
| ^
192 | Geolocation.IOS_AUTHORIZATIONS.ALWAYS: "locationAuthorizationAlways";
193 |
194 | module.exports = Geolocation;

eslint-bot reviewed

View reviewed changes

Libraries/Geolocation/Geolocation.js Outdated

		@@ -186,4 +187,8 @@ var Geolocation = {
		}

eslint-bot Jul 18, 2017

Unexpected token :


          [Geolocation] Provide option to request specific location permissions

0d424a4

Fix options

Spacing

[Geolocation] Fix build

[Geolocation] Fix npm flow error

[Geolocation] Fix lint

yatchoi force-pushed the ios/authorization-fix branch from 939afdc to 0d424a4 Compare

July 18, 2017 20:36

eslint-bot reviewed

View reviewed changes

Libraries/Geolocation/Geolocation.js Outdated

@@ @@ -186,4 +187,8 @@ var Geolocation = { @@
                 }
               };
+              Geolocation.IOS_AUTHORIZATIONS = {};
+              Geolocation.IOS_AUTHORIZATIONS.WHEN_IN_USE = "locationAuthorizationWhenInUse";

eslint-bot Jul 18, 2017

quotes: Strings must use singlequote.

eslint-bot reviewed

View reviewed changes

Libraries/Geolocation/Geolocation.js Outdated

		@@ -186,4 +187,8 @@ var Geolocation = {
		}
		};

eslint-bot Jul 18, 2017

property IOS_AUTHORIZATIONS Property not found in object literal

eslint-bot reviewed

View reviewed changes

Libraries/Geolocation/Geolocation.js Outdated

@@ @@ -186,4 +187,8 @@ var Geolocation = { @@
                 }
               };
+              Geolocation.IOS_AUTHORIZATIONS = {};

eslint-bot Jul 18, 2017

quotes: Strings must use singlequote.

eslint-bot reviewed

View reviewed changes

Libraries/Geolocation/Geolocation.js

@@ @@ -31,6 +31,7 @@ type GeoOptions = { @@
                 maximumAge: number,
                 enableHighAccuracy: bool,
                 distanceFilter: number,
+                locationAuthorizationIOS: string,
               }
               /**

eslint-bot Jul 23, 2017

null: Parsing error: Unexpected token, expected ;

188 | };
189 |

190 | Geolocation.IOS_AUTHORIZATIONS: Object = {};
| ^
191 | Geolocation.IOS_AUTHORIZATIONS.WHEN_IN_USE = 'locationAuthorizationWhenInUse';
192 | Geolocation.IOS_AUTHORIZATIONS.ALWAYS = 'locationAuthorizationAlways';
193 |

eslint-bot reviewed

View reviewed changes

Libraries/Geolocation/Geolocation.js

@@ @@ -31,6 +31,7 @@ type GeoOptions = { @@
                 maximumAge: number,
                 enableHighAccuracy: bool,
                 distanceFilter: number,
+                locationAuthorizationIOS: string,
               }
               /**

eslint-bot Jul 23, 2017

Unexpected token :


          [Geolocation] Fix linter issues

92f4db7

[Geolocation] Fix linter issues

yatchoi force-pushed the ios/authorization-fix branch from 7dfe30e to 92f4db7 Compare

July 23, 2017 15:34

facebook-github-bot added the CLA Signed label

Contributor

facebook-github-bot commented Jul 23, 2017

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

ngandhy mentioned this pull request

getCurrentPosition Prompts the "Always Location" permission instead of the "On Use" permission #11822

Closed

Contributor

rigdern commented Aug 27, 2017

There's a second PR for fixing this problem (#15096). We should figure out which one to take. I believe the primary difference between these PRs is:

This one tweaks Geolocation such that you can tell it which permissions to request.
PR iOS: Geolocation: Allow skipping of permission prompts #15096 tweaks Geolocation such that you can tell it not to request permissions on your behalf. Instead, the app has to request permissions on its own.

I prefer the solution used in my PR (#15096). It's consistent with the Android implementation of Geolocation which doesn't request any permissions. Instead, apps are expected to use the PermissionsAndroid module to request the needed permissions. I think we should go in a similar direction for iOS. React Native should eventually have a cross-platform Permissions module that an app can use to request any permission it wants. I like that this gives apps a lot of flexibility in that they get complete control over their permission requests. My team has used this approach in our app and it has worked well for us.

@yatchoi do you have any thoughts on this?

Adam Comella
Microsoft Corp.

rigdern mentioned this pull request

iOS: Geolocation: Allow skipping of permission prompts #15096

Closed

Contributor

facebook-github-bot commented Sep 24, 2017

@yatchoi I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

stale bot commented Nov 23, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Maybe the issue has been fixed in a recent release, or perhaps it is not affecting a lot of people. If you think this issue should definitely remain open, please let us know why. Thank you for your contributions.

stale bot added the Stale label

stale bot closed this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Stale