Ensure Permission before requesting location

Previously we asked for location regardless of permission status and
trusted macOS to handle the request properly. The causes problems in
unsigned builds and puts to much trust into the platform. This change
ensures that permission has been granted before making the request.

Bug: 1131777
Change-Id: I5e629661bdd058ee2c8d0ca71dee6be7ce1d51ff
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2429572Reviewed-by: Reilly Grant <reillyg@chromium.org>
Commit-Queue: James Hollyer <jameshollyer@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810714}

services/device/geolocation/core_location_provider.h

@@ -29,6 +29,8 @@ class CoreLocationProvider : public LocationProvider {
   const mojom::Geoposition& GetPosition() override;
   void OnPermissionGranted() override;
 
+  void SystemLocationPermissionGranted();
+  void SystemLocationPermissionDenied();
   void DidUpdatePosition(CLLocation* location);
   void SetManagerForTesting(CLLocationManager* location_manager);
 
@@ -37,6 +39,8 @@ class CoreLocationProvider : public LocationProvider {
   base::scoped_nsobject<LocationDelegate> delegate_;
   mojom::Geoposition last_position_;
   LocationProviderUpdateCallback callback_;
+  bool has_permission_ = false;
+  bool provider_start_attemped_ = false;
   base::WeakPtrFactory<CoreLocationProvider> weak_ptr_factory_{this};
 };
 

services/device/geolocation/core_location_provider.mm

@@ -31,11 +31,17 @@
   if (!provider_)
     return;
   if (@available(macOS 10.12.0, *)) {
-    if (status == kCLAuthorizationStatusAuthorizedAlways)
-      provider_->OnPermissionGranted();
+    if (status == kCLAuthorizationStatusAuthorizedAlways) {
+      provider_->SystemLocationPermissionGranted();
+    } else {
+      provider_->SystemLocationPermissionDenied();
+    }
   } else {
-    if (status == kCLAuthorizationStatusAuthorized)
-      provider_->OnPermissionGranted();
+    if (status == kCLAuthorizationStatusAuthorized) {
+      provider_->SystemLocationPermissionGranted();
+    } else {
+      provider_->SystemLocationPermissionDenied();
+    }
   }
 }
 
@@ -73,7 +79,15 @@ void CoreLocationProvider::StartProvider(bool high_accuracy) {
     location_manager_.get().desiredAccuracy = kCLLocationAccuracyHundredMeters;
   }
 
-  [location_manager_ startUpdatingLocation];
+  // macOS guarantees that didChangeAuthorization will be called at least once
+  // with the initial authorization status. Therefore this variable will be
+  // updated regardless of whether that authorization status has recently
+  // changed.
+  if (has_permission_) {
+    [location_manager_ startUpdatingLocation];
+  } else {
+    provider_start_attemped_ = true;
+  }
 }
 
 void CoreLocationProvider::StopProvider() {
@@ -88,6 +102,18 @@ void CoreLocationProvider::OnPermissionGranted() {
   // Nothing to do here.
 }
 
+void CoreLocationProvider::SystemLocationPermissionGranted() {
+  has_permission_ = true;
+  if (provider_start_attemped_) {
+    [location_manager_ startUpdatingLocation];
+    provider_start_attemped_ = false;
+  }
+}
+
+void CoreLocationProvider::SystemLocationPermissionDenied() {
+  has_permission_ = false;
+}
+
 void CoreLocationProvider::DidUpdatePosition(CLLocation* location) {
   // The error values in CLLocation correlate exactly to our error values.
   last_position_.latitude = location.coordinate.latitude;

services/device/geolocation/core_location_provider_unittest.mm

@@ -24,6 +24,7 @@
 
 // Utility functions.
 - (void)fakeUpdatePosition:(CLLocation*)test_location;
+- (void)fakeUpdatePermission:(CLAuthorizationStatus)status;
 @end
 
 @implementation FakeCLLocationManager
@@ -51,6 +52,10 @@
   [_delegate locationManager:(id)self didUpdateLocations:@[ testLocation ]];
 }
 
+- (void)fakeUpdatePermission:(CLAuthorizationStatus)status {
+  [_delegate locationManager:(id)self didChangeAuthorizationStatus:status];
+}
+
 @end
 
 namespace device {
@@ -92,6 +97,13 @@ TEST_F(CoreLocationProviderTest, CreateDestroy) {
 
 TEST_F(CoreLocationProviderTest, StartAndStopUpdating) {
   InitializeProvider();
+  if (@available(macOS 10.12.0, *)) {
+    [fake_location_manager_
+        fakeUpdatePermission:kCLAuthorizationStatusAuthorizedAlways];
+  } else {
+    [fake_location_manager_
+        fakeUpdatePermission:kCLAuthorizationStatusAuthorized];
+  }
   provider_->StartProvider(/*high_accuracy=*/true);
   EXPECT_TRUE(IsUpdating());
   EXPECT_EQ([fake_location_manager_ desiredAccuracy], kCLLocationAccuracyBest);
@@ -100,8 +112,39 @@ TEST_F(CoreLocationProviderTest, StartAndStopUpdating) {
   provider_.reset();
 }
 
+TEST_F(CoreLocationProviderTest, DontStartUpdatingIfPermissionDenied) {
+  InitializeProvider();
+  [fake_location_manager_ fakeUpdatePermission:kCLAuthorizationStatusDenied];
+  provider_->StartProvider(/*high_accuracy=*/true);
+  EXPECT_FALSE(IsUpdating());
+}
+
+TEST_F(CoreLocationProviderTest, DontStartUpdatingUntilPermissionGranted) {
+  InitializeProvider();
+  provider_->StartProvider(/*high_accuracy=*/true);
+  EXPECT_FALSE(IsUpdating());
+  [fake_location_manager_ fakeUpdatePermission:kCLAuthorizationStatusDenied];
+  EXPECT_FALSE(IsUpdating());
+  if (@available(macOS 10.12.0, *)) {
+    [fake_location_manager_
+        fakeUpdatePermission:kCLAuthorizationStatusAuthorizedAlways];
+  } else {
+    [fake_location_manager_
+        fakeUpdatePermission:kCLAuthorizationStatusAuthorized];
+  }
+  EXPECT_TRUE(IsUpdating());
+  provider_.reset();
+}
+
 TEST_F(CoreLocationProviderTest, GetPositionUpdates) {
   InitializeProvider();
+  if (@available(macOS 10.12.0, *)) {
+    [fake_location_manager_
+        fakeUpdatePermission:kCLAuthorizationStatusAuthorizedAlways];
+  } else {
+    [fake_location_manager_
+        fakeUpdatePermission:kCLAuthorizationStatusAuthorized];
+  }
   provider_->StartProvider(/*high_accuracy=*/true);
   EXPECT_TRUE(IsUpdating());
   EXPECT_EQ([fake_location_manager_ desiredAccuracy], kCLLocationAccuracyBest);