[Privacy Hub] Increase scope to stop pinging GLS

Increase the scope of PH Geolocation toggle to also include the IP-based
geolocation. So when users disable the PH location toggle, device will
stop making any geolocation-related requests. All Geolocation clients
should seamlessly pause/resume scheduling on
'kUserGeolocationPermission` user preference updates.

Bug: b:281657093
TEST=out/Default/chromeos_unittests --gtest_filter=*SimpleGeolocation*
TEST=testing/xvfb.py out/Default/unit_tests \
--gtest_filter=*NightLightClientImplTest*
TEST=testing/xvfb.py out/Default/browser_tests \
--gtest_filter=*WizardController*
TEST=testing/xvfb.py out/Default/ash_unittests \
--gtest_filter=*GeolocationControllerTest*
TEST=testing/xvfb.py out/Default/browser_tests \
--gtest_filter=*TimeZoneResolverManager*

Change-Id: I606fcd20d2c7d7f6bb8930391963e33761501122
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4537319
Commit-Queue: Zauri Meshveliani‎ <zauri@chromium.org>
Reviewed-by: Alexander Hendrich <hendrich@chromium.org>
Auto-Submit: Zauri Meshveliani‎ <zauri@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1158753}

ash/system/geolocation/geolocation_controller.cc

@@ -16,6 +16,7 @@
 #include "base/time/clock.h"
 #include "chromeos/ash/components/geolocation/geoposition.h"
 #include "chromeos/ash/components/geolocation/simple_geolocation_provider.h"
+#include "components/prefs/pref_change_registrar.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
 #include "third_party/icu/source/i18n/astro.h"
@@ -79,34 +80,41 @@ void GeolocationController::RegisterProfilePrefs(PrefRegistrySimple* registry) {
 void GeolocationController::AddObserver(Observer* observer) {
   const bool is_first_observer = observers_.empty();
   observers_.AddObserver(observer);
-  if (is_first_observer)
+  if (is_first_observer && IsSystemGeolocationAllowed()) {
     ScheduleNextRequest(base::Seconds(0));
+  }
 }
 
 void GeolocationController::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
-  if (observers_.empty())
+  if (observers_.empty()) {
     timer_->Stop();
+  }
 }
 
 void GeolocationController::TimezoneChanged(const icu::TimeZone& timezone) {
   const std::u16string timezone_id =
       system::TimezoneSettings::GetTimezoneID(timezone);
-  if (current_timezone_id_ == timezone_id)
+  if (current_timezone_id_ == timezone_id) {
     return;
+  }
 
   current_timezone_id_ = timezone_id;
 
-  // On timezone changes, request an immediate geoposition.
-  ScheduleNextRequest(base::Seconds(0));
+  // On timezone changes, request an immediate geoposition if the system
+  // geolocation allows.
+  if (IsSystemGeolocationAllowed()) {
+    ScheduleNextRequest(base::Seconds(0));
+  }
 }
 
 void GeolocationController::SuspendDone(base::TimeDelta sleep_duration) {
-  if (sleep_duration >= kNextRequestDelayAfterSuccess)
+  if (sleep_duration >= kNextRequestDelayAfterSuccess) {
     ScheduleNextRequest(base::Seconds(0));
+  }
 }
 
-bool GeolocationController::IsPreciseGeolocationAllowed() const {
+bool GeolocationController::IsSystemGeolocationAllowed() const {
   // TODO(b/276715041): Refactor the `SimpleGeolocationProvider` class to
   // eliminate the `Shell`-dependency of this class.
   Shell* const shell = Shell::Get();
@@ -134,6 +142,18 @@ void GeolocationController::OnActiveUserPrefServiceChanged(
   LoadCachedGeopositionIfNeeded();
 }
 
+void GeolocationController::OnSystemGeolocationPermissionChanged(bool enabled) {
+  // Drop all pending requests when system geolocation is toggled OFF.
+  if (!enabled) {
+    timer_->Stop();
+    return;
+  }
+
+  // System geolocation toggled ON, post an immediate new geolocation request to
+  // resume continuous scheduling.
+  ScheduleNextRequest(base::Seconds(0));
+}
+
 // static
 base::TimeDelta
 GeolocationController::GetNextRequestDelayAfterSuccessForTesting() {
@@ -157,6 +177,13 @@ void GeolocationController::SetCurrentTimezoneIdForTesting(
 void GeolocationController::OnGeoposition(const Geoposition& position,
                                           bool server_error,
                                           const base::TimeDelta elapsed) {
+  if (!IsSystemGeolocationAllowed() || observers_.empty()) {
+    // The request might come after the user disabled the system geolocation
+    // access or if all observers unsubscribed, in which case we should stop
+    // processing the geolocation responses.
+    return;
+  }
+
   if (server_error || !position.Valid() ||
       elapsed > kGeolocationRequestTimeout) {
     VLOG(1) << "Failed to get a valid geoposition. Trying again later.";
@@ -208,14 +235,22 @@ base::Time GeolocationController::GetNow() const {
 }
 
 void GeolocationController::ScheduleNextRequest(base::TimeDelta delay) {
+  // Drop all pending geolocation requests while system permission is
+  // denied. Toggling system geolocation ON will trigger a fresh geolocation
+  // request.
+  if (!IsSystemGeolocationAllowed()) {
+    return;
+  }
+
   timer_->Start(FROM_HERE, delay, this,
                 &GeolocationController::RequestGeoposition);
 }
 
 void GeolocationController::NotifyGeopositionChange(
     bool possible_change_in_timezone) {
-  for (Observer& observer : observers_)
+  for (Observer& observer : observers_) {
     observer.OnGeopositionChanged(possible_change_in_timezone);
+  }
 }
 
 void GeolocationController::RequestGeoposition() {

ash/system/geolocation/geolocation_controller.h

@@ -20,6 +20,7 @@
 #include "chromeos/dbus/power/power_manager_client.h"
 #include "services/network/public/cpp/shared_url_loader_factory.h"
 
+class PrefChangeRegistrar;
 class PrefRegistrySimple;
 class PrefService;
 
@@ -78,6 +79,12 @@ class ASH_EXPORT GeolocationController
   static GeolocationController* Get();
   static void RegisterProfilePrefs(PrefRegistrySimple* registry);
 
+  // This class should respect the system geolocation permission. When the
+  // permission is disabled, no requests should be dispatched and no responses
+  // processed.
+  // Called from `ash::Preferences::ApplyPreferences()`.
+  void OnSystemGeolocationPermissionChanged(bool enabled);
+
   const base::OneShotTimer& timer() const { return *timer_; }
 
   const std::u16string& current_timezone_id() const {
@@ -94,7 +101,7 @@ class ASH_EXPORT GeolocationController
   void SuspendDone(base::TimeDelta sleep_duration) override;
 
   // SimpleGeolocationProvider::Delegate:
-  bool IsPreciseGeolocationAllowed() const override;
+  bool IsSystemGeolocationAllowed() const override;
 
   // SessionObserver:
   void OnActiveUserPrefServiceChanged(PrefService* pref_service) override;
@@ -168,6 +175,7 @@ class ASH_EXPORT GeolocationController
 
   // May be null if a user has not logged in yet.
   raw_ptr<PrefService> active_user_pref_service_ = nullptr;
+  std::unique_ptr<PrefChangeRegistrar> registrar_;
 
   // The IP-based geolocation provider.
   SimpleGeolocationProvider provider_;

ash/system/geolocation/geolocation_controller_unittest.cc

@@ -67,6 +67,29 @@ base::Time ToUTCTime(base::StringPiece utc_time_str) {
   return time;
 }
 
+class FakeGeolocationController : public GeolocationController {
+ public:
+  explicit FakeGeolocationController(
+      scoped_refptr<network::SharedURLLoaderFactory> factory)
+      : GeolocationController(factory) {}
+
+  // Proxy method to call the `OnGeoposition()` callback directly, without
+  // waiting for the server response. Need this to test scheduler behavior.
+  void ImitateGeopositionReceived() {
+    Geoposition fake_pos;
+    fake_pos.latitude = kTestLatitude1;
+    fake_pos.longitude = kTestLongitude1;
+    fake_pos.status = Geoposition::STATUS_OK;
+    fake_pos.accuracy = 10;
+    fake_pos.timestamp = base::Time::Now();
+
+    GeolocationController::OnGeoposition(fake_pos, false, base::Seconds(1));
+  }
+
+  // TODO(b/286233027): Override `RequestGeolocation()` to fake the server
+  // communication.
+};
+
 // Base test fixture.
 class GeolocationControllerTest : public AshTestBase {
  public:
@@ -82,7 +105,7 @@ class GeolocationControllerTest : public AshTestBase {
   void SetUp() override {
     AshTestBase::SetUp();
     CreateTestUserSessions();
-    controller_ = std::make_unique<GeolocationController>(
+    controller_ = std::make_unique<FakeGeolocationController>(
         static_cast<scoped_refptr<network::SharedURLLoaderFactory>>(
             base::MakeRefCounted<TestGeolocationUrlLoaderFactory>()));
 
@@ -109,7 +132,7 @@ class GeolocationControllerTest : public AshTestBase {
     AshTestBase::TearDown();
   }
 
-  GeolocationController* controller() const { return controller_.get(); }
+  FakeGeolocationController* controller() const { return controller_.get(); }
   base::SimpleTestClock* test_clock() { return &test_clock_; }
   base::OneShotTimer* timer_ptr() const { return timer_ptr_; }
   const Geoposition& position() const { return position_; }
@@ -156,8 +179,14 @@ class GeolocationControllerTest : public AshTestBase {
     factory_->set_position(position_);
   }
 
+  void UpdateUserGeolocationPermission(bool enabled) {
+    PrefService* pref_service =
+        Shell::Get()->session_controller()->GetPrimaryUserPrefService();
+    pref_service->SetBoolean(ash::prefs::kUserGeolocationAllowed, enabled);
+  }
+
  private:
-  std::unique_ptr<GeolocationController> controller_;
+  std::unique_ptr<FakeGeolocationController> controller_;
   base::SimpleTestClock test_clock_;
   raw_ptr<base::OneShotTimer, ExperimentalAsh> timer_ptr_;
   raw_ptr<TestGeolocationUrlLoaderFactory, ExperimentalAsh> factory_;
@@ -276,6 +305,75 @@ TEST_F(GeolocationControllerTest, TimezoneChanges) {
   EXPECT_TRUE(timer_ptr()->IsRunning());
 }
 
+TEST_F(GeolocationControllerTest, SystemGeolocationPermissionChanges) {
+  EXPECT_FALSE(timer_ptr()->IsRunning());
+
+  GeolocationControllerObserver observer;
+  controller()->AddObserver(&observer);
+  EXPECT_EQ(0, observer.position_received_num());
+
+  FireTimerToFetchGeoposition();
+  EXPECT_EQ(1, observer.position_received_num());
+  EXPECT_TRUE(timer_ptr()->IsRunning());
+
+  // Disable system geo permission. Scheduling should stop.
+  controller()->OnSystemGeolocationPermissionChanged(false);
+  EXPECT_FALSE(timer_ptr()->IsRunning());
+
+  // Re-enabling the system geo permission, should resume scheduling.
+  controller()->OnSystemGeolocationPermissionChanged(true);
+  EXPECT_TRUE(timer_ptr()->IsRunning());
+}
+
+TEST_F(GeolocationControllerTest, StopSchedulingWhileResponseIsComing) {
+  EXPECT_FALSE(timer_ptr()->IsRunning());
+
+  // This will start scheduling.
+  GeolocationControllerObserver observer;
+  controller()->AddObserver(&observer);
+
+  // Fire Geolocation request.
+  timer_ptr()->FireNow();
+  EXPECT_FALSE(timer_ptr()->IsRunning());
+
+  // Disable user geolocation permission, this should stop scheduling.
+  UpdateUserGeolocationPermission(false);
+  controller()->OnSystemGeolocationPermissionChanged(false);
+  EXPECT_FALSE(timer_ptr()->IsRunning());
+
+  // Simulate server response and check it didn't trigger scheduling to
+  // continue.
+  controller()->ImitateGeopositionReceived();
+  EXPECT_FALSE(timer_ptr()->IsRunning());
+
+  // Re-enable user geolocation permission, this should resume scheduling.
+  UpdateUserGeolocationPermission(true);
+  controller()->OnSystemGeolocationPermissionChanged(true);
+  EXPECT_TRUE(timer_ptr()->IsRunning());
+}
+
+TEST_F(GeolocationControllerTest, StopSchedulingWhenObserverListIsEmpty) {
+  EXPECT_FALSE(timer_ptr()->IsRunning());
+
+  // Add the first observer. This will kick off scheduling.
+  GeolocationControllerObserver observer;
+  controller()->AddObserver(&observer);
+
+  // Fire Geolocation request.
+  timer_ptr()->FireNow();
+
+  // Unsubscribe the only observer.
+  controller()->RemoveObserver(&observer);
+
+  // Simulate the server response and check it didn't resume the scheduler.
+  controller()->ImitateGeopositionReceived();
+  EXPECT_FALSE(timer_ptr()->IsRunning());
+
+  // Add the observer back, scheduling should resume.
+  controller()->AddObserver(&observer);
+  EXPECT_TRUE(timer_ptr()->IsRunning());
+}
+
 // Tests obtaining sunset/sunrise time when there is no valid geoposition, for
 // example, due to lack of connectivity.
 TEST_F(GeolocationControllerTest, SunsetSunriseDefault) {

ash/system/privacy_hub/geolocation_privacy_switch_controller.cc

@@ -45,7 +45,6 @@ void GeolocationPrivacySwitchController::OnActiveUserPrefServiceChanged(
           &GeolocationPrivacySwitchController::OnPreferenceChanged,
           base::Unretained(this)));
   UpdateNotification();
-  // TODO(zauri): Set 0-state
 }
 
 void GeolocationPrivacySwitchController::TrackGeolocationAttempted(
@@ -84,8 +83,6 @@ std::vector<std::u16string> GeolocationPrivacySwitchController::GetActiveApps(
 }
 
 void GeolocationPrivacySwitchController::OnPreferenceChanged() {
-  // TODO(zauri): This is a stub code. Sync the state with
-  // SimpleGeolocationProvider.
   const bool geolocation_state = pref_change_registrar_->prefs()->GetBoolean(
       prefs::kUserGeolocationAllowed);
   DLOG(ERROR) << "Privacy Hub: Geolocation switch state = "

ash/system/privacy_hub/privacy_hub_controller.cc

@@ -16,9 +16,11 @@ PrivacyHubController::~PrivacyHubController() = default;
 // static
 void PrivacyHubController::RegisterLocalStatePrefs(
     PrefRegistrySimple* registry) {
+  // TODO(b/286526469): Sync this pref with the device owner's location
+  // permission `kUserGeolocationAllowed`.
   registry->RegisterIntegerPref(
       prefs::kDeviceGeolocationAllowed,
-      static_cast<int>(PrivacyHubController::AccessLevel::kDisallowed));
+      static_cast<int>(PrivacyHubController::AccessLevel::kAllowed));
 }
 
 // static

chrome/browser/ash/BUILD.gn

@@ -2121,6 +2121,8 @@ source_set("ash") {
     "network_change_manager_client.h",
     "night_light/night_light_client.cc",
     "night_light/night_light_client.h",
+    "night_light/night_light_client_impl.cc",
+    "night_light/night_light_client_impl.h",
     "note_taking_controller_client.cc",
     "note_taking_controller_client.h",
     "note_taking_helper.cc",
@@ -5419,7 +5421,7 @@ source_set("unit_tests") {
     "net/system_proxy_manager_unittest.cc",
     "net/traffic_counters_handler_unittest.cc",
     "network_change_manager_client_unittest.cc",
-    "night_light/night_light_client_unittest.cc",
+    "night_light/night_light_client_impl_unittest.cc",
     "note_taking_helper_unittest.cc",
     "notifications/debugd_notification_handler_unittest.cc",
     "notifications/deprecation_notification_controller_unittest.cc",