zoontek / react-native-permissions

An unified permissions API for React Native on iOS, Android and Windows.
MIT License
4.02k stars 828 forks source link

iOS: Cannot request location always if we already have location when in use permission #490

Open tallpants opened 4 years ago

tallpants commented 4 years ago

Bug report

If we have location when in use permission, requesting location always permission always returns denied, probably because of this line: https://github.com/react-native-community/react-native-permissions/blob/4017cbf0861b562bb48888f76cfa7c9a62c68ffc/ios/LocationAlways/RNPermissionHandlerLocationAlways.m#L38

Apple's documentation is pretty clear about how you're allowed to call requestAlwaysAuthorization if the current authorization status is either kCLAuthorizationStatusNotDetermined or kCLAuthorizationStatusAuthorizedWhenInUse. Calling this when we already have when-in-use authorization will prompt the user to upgrade to "always allow" location permissions.

But in the library we're assuming that if we currently have when-in-use authorization, we're not allowed to ask for always authorization.

I tested by manually calling requestAlwaysAuthorization in Objective-C after getting when-in-use authorization from react-native-permissions -- and the upgrade prompt shows up just fine.

This should be an easy fix (as far as I can tell).

Documentation for requestAlwaysAuthorization that explains this: https://developer.apple.com/documentation/corelocation/cllocationmanager/1620551-requestalwaysauthorization?changes=latest_minor&language=objc

Upvote & Fund

Fund with Polar

zoontek commented 4 years ago

@tallpants This library already handle location permission escalation. The line you give is from the check method from RNPermissionHandlerLocationAlways.m, so there no issue here.

A normal permission escalation would be:

  1. check that LOCATION_WHEN_IN_USE is granted and LOCATION_ALWAYS is denied.
  2. request LOCATION_ALWAYS
tallpants commented 4 years ago

@zoontek doing request for LOCATION_ALWAYS will return RESULTS.BLOCKED right now if we have LOCATION_WHEN_IN_USE permission already.

zoontek commented 4 years ago

I just checked it, it's not that easy to fix it.

I explain myself: kCLAuthorizationStatusAuthorizedWhenInUse does not means that LOCATION_ALWAYS is requestable. It means that it might be requestable. It's fixable, but will need a bit of work / test.

tallpants commented 4 years ago

You're right yea it's more complicated than I initially thought. Do you have any ideas on how to approach fixing it? My initial guess is that without some form of persistence (maybe to NSUserDefaults) about whether we've already requested LOCATION_ALWAYS once before in the app, it's not possible to reliably work with the existing API of check and request.

Something like this (pseudocode -- doesn't handle all the other cases):

- (void)checkWithResolver:(RCTPromiseResolveBlock)resolve rejecter:(RCTPromiseRejectBlock)reject {
  CLAuthorizationStatus authorizationStatus = [CLLocationManager authorizationStatus];
  BOOL hasRequestedLocationAlways = [NSUserDefaults standardDefaults] boolForKey:@"RNPermissionHandlerLocationAlways-hasRequested"];

  if (authorizationStatus == kCLAuthorizationStatusAuthorizedWhenInUse && hasRequestedLocationAlways) {
    return resolve(RNPermissionStatusDenied);
  } else {
    return resolve(RNPermissionStatusNotDetermined);
  }
}

... and similary setting the RNPermissionHandlerLocationAlways-hasRequested key in user defaults to true in the request method after calling requestAlwaysAuthorization (since it can only be called once).

Just an idea of course, maybe you can find a nicer way of solving.

zoontek commented 4 years ago

The core already has such helper method (https://github.com/react-native-community/react-native-permissions/blob/master/ios/RNPermissions.m#L324)

It might work, but we have to flag as requested only if the result isn't whenInUse.

rolfb commented 4 years ago

Would love the ability to call requestAlwaysAuthorization twice to get to Always faster too.

mikehardy commented 4 years ago

This is under discussion here as well: https://github.com/transistorsoft/react-native-background-geolocation-android/issues/938#issuecomment-668042805

By you @rolfb :-), just cross-linking here as you've got great research posted in the other issue

rolfb commented 4 years ago

Cheers, @mikehardy!

The other one is private though :-)

mikehardy commented 4 years ago

Ah true enough - it affects the public version, but either way may result in knowledge here :)

rolfb commented 4 years ago

It seems like you have to explicitly request WhenInUse first, then you can request Always directly after. If you request Always first and get While in use (as read from device settings) you can't request Always to trigger the "Change to always" alert and will have to wait for the system to trigger it. This is probably clear to most, just wanted to summarize.

jdegger commented 4 years ago

For reference, the background-geolocation-android private repo owner @christocracy posted this as a solution:

docs clearly state it must either be undetermined or when in use when requesting.

The code added to make this work is the || clause.

if ((status == kCLAuthorizationStatusNotDetermined) || ((lastStatus == kCLAuthorizationStatusAuthorizedWhenInUse) && ([desiredRequest isEqualToString:ALWAYS_STRING]) )) {
  .
  .
  .
}

I have tested by commenting-out the check for kCLAuthorizationStatusNotDetermined in this repo and this seems to work fine.

tallpants commented 4 years ago

@jdegger that'll work for the initial request -- but to keep the API consistent we also need to track manually when we've asked for it already and return DENIED -- because requestAlwaysAuthorization will just not do anything if it's called a second time.

christocracy commented 4 years ago

we also need to track manually when we've asked for it already

That's correct. The updated logic in background-geolocationplugin keeps track of a config.didRequestUpgradeLocationAuthorization, stored in NSUseDefaults.

Note that if user resets location authorization to Ask Next Time, we can once again request an authorization upgrade from WhenInUse -> Always.

BOOL isRequestUpgradeAlways = NO;

if (status == kCLAuthorizationStatusNotDetermined) {
    // Reset upgrade-to-always state in UserDefaults
    config.didRequestUpgradeLocationAuthorization = NO;
} else if ((status == kCLAuthorizationStatusAuthorizedWhenInUse) && ([desiredRequest isEqualToString:ALWAYS_STRING]) && !config.didRequestUpgradeLocationAuthorization ) {
    isRequestUpgradeAlways = YES;
}

if ((status == kCLAuthorizationStatusNotDetermined) || isRequestUpgradeAlways ) {
    // Update upgrade-to-always state in UserDefaults
    if (isRequestUpgradeAlways) {
        config.didRequestUpgradeLocationAuthorization = YES;
    }
    // Request authorization
    .
    .
    .
}
elkinjosetm commented 4 years ago

Is there any update on this issue?

zoontek commented 4 years ago

I personally don't have any time to work on open source these days. For react-native-permissions, iOS 14 / Android 11 support will probably be the main focus area, so this task will probably receive a low priority.

christocracy commented 4 years ago

@zoontek I'm the author of react-native-background-geolocation. Requesting Always permission with Android 11 / targetSdkVersion 30 is really tricky.

  1. Manifest.permission.ACCESS_BACKGROUND_LOCATION must be requested on its own (cannot be combined with ACCESS_COARSE_LOCATION / ACCESS_FINE_LOCATION.
  2. Android offers a method to determine if the user has already been asked for ACCESS_BACKGROUND_PERMISSION :
    /// Returns true if the Dialog should be shown
    @TargetApi(29)
    public boolean shouldShow(Activity activity) {
        if (activity == null || misDialogActive.get()) return false;
        return ActivityCompat.shouldShowRequestPermissionRationale(activity, Manifest.permission.ACCESS_BACKGROUND_LOCATION);
    }
  3. When above method returns true, it's your responsibility to compose a custom dialog explaining why the app requires Allow all the time permission. For background-geolocation, I've created a new config option backgroundPermissionRationale for customizing the text-elements on the dialog.
backgroundPermissionRationale: {
  title: "Allow access to this device's location in the background?",
  message: "In order to X, Y and Z, please enable 'Allow all the time permission",
  okButton: "Change to Allow all the time"
}

In the background-geolocation library, I show a Dialog instance with custom layout styled to look similar to the Android System Dialog:

christocracy commented 4 years ago

@zoontek Maybe another way to handle this without your library producing a native Android Dialog would be to offer an event like onRationalize(permission), where the develop would take full responsibility for producing their own RN dialog and re-executing their permission-request.

zoontek commented 4 years ago

@christocracy I personally hate apps that request Always permission because, with the exception of some apps that use location extensively, it's often not required except to track users.

Thanks for the informations, I will check this when I will have some time (we just launched our product, I'm really busy these days 😅). I'll try to choose the easiest solution, but I'm afraid of these changes.

mikehardy commented 4 years ago

@zoontek I agree with you in general and in spirit, however I have one of those apps that nevertheless does something useful with the always permission, in a transparent + delete-able and even optional way, such that if it's allowed it could help the user, but we respect that people hate it (since normally they receive no value for their data). Which is just to say, sure, but...sometimes there's reasons, so I'd love to see the feature. Recognizing I may need to code it up :-), although I'm pretty busy myself at the moment

christocracy commented 4 years ago

I personally hate apps that request Always

My plugin was originally designed for tracking first-responders in disaster-zones (hurricanes, earthquakes), where life depended on it. It is ideally suited for fleet-tracking use-cases (eg: Jogging App, Delivery service app, Taxi / Trucking apps).

I make no moral judgements on my customers' usage of Always authorization though I do let those who I find using it in Social app that it's not a good idea.

I welcome these strict new permission authorization mechanisms introduced by both Google and iOS.

zoontek commented 4 years ago

@christocracy @mikehardy Guys it's not against you, I also worked on several apps that required background geolocation, it's just that I see this feature abused :)

I will try to add a light API to handle these cases, but not an ultra complete one with X fancy options 😅

tallpants commented 3 years ago

@zoontek would you be willing to accept a PR that fixes only the iOS permission escalation issue (where you can't request location always authorization after you've been granted location when in use authorization) -- which is what the original issue was about, and then we can track the problems on Android and a new API for extra cases and so on in a different issue?

I'm hoping we can merge it and release a minor version since it's technically a bug with the library, instead of saving it for 3.0.

tallpants commented 3 years ago

PR opened for this here: https://github.com/zoontek/react-native-permissions/pull/529

taylorkline commented 3 years ago

I have ran into this issue as well. We have two use-cases in our app, one that only needs whenInUse auth, and another, rare use-case that requires always.

Apple's own guidelines state,

Request authorization only when your user needs location services to perform a task in your app. If it's not clear to the user why your app is using location services, the user may deny your request.

Therefore this current bug does not allow us React Native users to follow Apple's guidelines.

I acknowledge that @zoontek doesn't have time to prioritize this issue right now, but wanted to make a note here.

tallpants commented 3 years ago

@taylorkline we'd all like to see this fixed but for now you'll have to consider this a missing feature. I'd suggest using the changes in my PR linked above along with patch-package in the meantime. Realistically I think this'll only be fixed in the next major release because it's not possible to fit this permission into the existing API.

mkraina commented 3 years ago

Hi guys, I've came up with possible solution proposed here in @tallpants PR So far it works just fine for me, hopefully our QA won't find out any flaws with this here is the react-native-permissions+3.0.0.patch generated via patch-package (using changes from the PR)

index 29c9075..c7e2f7e 100644
--- a/node_modules/react-native-permissions/ios/LocationAlways/RNPermissionHandlerLocationAlways.m
+++ b/node_modules/react-native-permissions/ios/LocationAlways/RNPermissionHandlerLocationAlways.m
@@ -35,7 +35,13 @@ - (void)checkWithResolver:(void (^ _Nonnull)(RNPermissionStatus))resolve
       return resolve(RNPermissionStatusNotDetermined);
     case kCLAuthorizationStatusRestricted:
       return resolve(RNPermissionStatusRestricted);
-    case kCLAuthorizationStatusAuthorizedWhenInUse:
+    case kCLAuthorizationStatusAuthorizedWhenInUse: {
+        BOOL requestedBefore = [RNPermissions isFlaggedAsRequested:[[self class] handlerUniqueId]];
+        if (requestedBefore) {
+            return resolve(RNPermissionStatusDenied);
+        }
+        return resolve(RNPermissionStatusNotDetermined);
+    }
     case kCLAuthorizationStatusDenied:
       return resolve(RNPermissionStatusDenied);
     case kCLAuthorizationStatusAuthorizedAlways:
@@ -48,7 +54,11 @@ - (void)requestWithResolver:(void (^ _Nonnull)(RNPermissionStatus))resolve
   if (![CLLocationManager locationServicesEnabled]) {
     return resolve(RNPermissionStatusNotAvailable);
   }
-  if ([CLLocationManager authorizationStatus] != kCLAuthorizationStatusNotDetermined) {
+
+  CLAuthorizationStatus authorizationStatus = [CLLocationManager authorizationStatus];
+  BOOL requestedBefore = [RNPermissions isFlaggedAsRequested:[[self class] handlerUniqueId]];
+
+  if (authorizationStatus != kCLAuthorizationStatusNotDetermined && (authorizationStatus == kCLAuthorizationStatusAuthorizedWhenInUse && requestedBefore)) {
     return [self checkWithResolver:resolve rejecter:reject];
   }

@@ -57,14 +67,35 @@ - (void)requestWithResolver:(void (^ _Nonnull)(RNPermissionStatus))resolve

   _locationManager = [CLLocationManager new];
   [_locationManager setDelegate:self];
+
+  [[NSNotificationCenter defaultCenter] addObserver:self
+                                        selector:@selector(handleAppStateDidChange:)
+                                        name:UIApplicationDidBecomeActiveNotification
+                                        object:nil];
+    
   [_locationManager requestAlwaysAuthorization];
+  [RNPermissions flagAsRequested:[[self class] handlerUniqueId]];
 }

-- (void)locationManager:(CLLocationManager *)manager didChangeAuthorizationStatus:(CLAuthorizationStatus)status {
-  if (status != kCLAuthorizationStatusNotDetermined) {
-    [_locationManager setDelegate:nil];
+- (void)updatePermission {
+    if(_resolve == nil || _reject == nil) return;
     [self checkWithResolver:_resolve rejecter:_reject];
-  }
+    [_locationManager setDelegate:nil];
+    _resolve = nil;
+    _reject = nil;
+    [[NSNotificationCenter defaultCenter] removeObserver:self name:UIApplicationDidBecomeActiveNotification object:nil];
+}
+
+- (void)locationManager:(CLLocationManager *)manager didChangeAuthorizationStatus:(CLAuthorizationStatus)status {
+    if (status != kCLAuthorizationStatusNotDetermined && status != kCLAuthorizationStatusAuthorizedWhenInUse) {
+        [self updatePermission];
+    }
+}
+
+- (void)handleAppStateDidChange:(NSNotification *)notification {
+    if([notification.name isEqualToString:UIApplicationDidBecomeActiveNotification]) {
+        [self updatePermission];
+    }
 }

 @end
tallpants commented 2 years ago

@zoontek we're revisiting this at our org -- the PR I made for this was left hanging because of your concern over a promise that might be unresolved indefinitely (which was definitely not ideal I agree).

We're willing to do the work to switch to the UIApplicationDidBecomeActiveNotification approach suggested by so many others here (as well as used by Expo in their location module) if you'd be willing to review / merge and release it -- we're happy to be responsible for maintaining that code. Does that work for you?

zoontek commented 2 years ago

@tallpants Yes, that could do the trick!

tallpants commented 2 years ago

@zoontek here's the new PR!

https://github.com/zoontek/react-native-permissions/pull/716

tallpants commented 2 years ago

ping @zoontek -- just following up on this!

KaiValar commented 4 months ago

Just fyi, the guys of Expo can do this job with their package location, maybe you can look how they did it https://github.com/expo/expo/tree/master/packages/expo-location