Skip to content

Commit

Permalink
[Privacy Hub] Geolocation manager for CrOS
Browse files Browse the repository at this point in the history
Extending the geolocation manager permission functionality to Chrome OS.

Bug: b/257906959
Low-Coverage-Reason: Low coverage is due to 1 uncovered line which is
actually covered by a lacros browser test.

Change-Id: Ia6ad5e2efb3b05802d5adbcac236357bfa699cce
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4014144
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Reviewed-by: Hidehiko Abe <hidehiko@chromium.org>
Commit-Queue: Jan Láník <janlanik@google.com>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1103304}
  • Loading branch information
Jan Lanik authored and Chromium LUCI CQ committed Feb 9, 2023
1 parent 1964b2d commit 7653a0b
Show file tree
Hide file tree
Showing 16 changed files with 511 additions and 5 deletions.
5 changes: 4 additions & 1 deletion chrome/browser/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -2412,6 +2412,7 @@ static_library("browser") {
"//services/cert_verifier:lib",
"//services/data_decoder/public/cpp",
"//services/device/public/cpp:device_features",
"//services/device/public/cpp/geolocation",
"//services/device/public/cpp/serial:switches",
"//services/device/public/cpp/usb",
"//services/device/public/mojom",
Expand Down Expand Up @@ -5327,6 +5328,8 @@ static_library("browser") {
"lacros/force_installed_tracker_lacros.h",
"lacros/fullscreen_controller_client_lacros.cc",
"lacros/fullscreen_controller_client_lacros.h",
"lacros/geolocation/system_geolocation_source_lacros.cc",
"lacros/geolocation/system_geolocation_source_lacros.h",
"lacros/identity_manager_lacros.cc",
"lacros/identity_manager_lacros.h",
"lacros/lacros_extension_apps_controller.cc",
Expand Down Expand Up @@ -5558,6 +5561,7 @@ static_library("browser") {
"//components/reporting/metrics:metrics_data_collection",
"//components/reporting/proto:metric_data_proto",
"//components/webapk:proto",
"//services/device/public/cpp/geolocation",
"//third_party/smhasher:murmurhash2",
"//ui/wm/public",
]
Expand Down Expand Up @@ -5972,7 +5976,6 @@ static_library("browser") {
"//components/remote_cocoa/browser:browser",
"//sandbox/mac:seatbelt",
"//sandbox/policy",
"//services/device/public/cpp/geolocation",
"//services/video_capture/public/mojom:constants",
"//third_party/crashpad/crashpad/client",
"//third_party/google_toolbox_for_mac",
Expand Down
4 changes: 4 additions & 0 deletions chrome/browser/ash/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -1299,6 +1299,8 @@ source_set("ash") {
"fusebox/fusebox_server.h",
"game_mode/game_mode_controller.cc",
"game_mode/game_mode_controller.h",
"geolocation/system_geolocation_source.cc",
"geolocation/system_geolocation_source.h",
"guest_os/guest_id.cc",
"guest_os/guest_id.h",
"guest_os/guest_os_diagnostics_builder.cc",
Expand Down Expand Up @@ -3995,6 +3997,7 @@ source_set("ash") {
"//services/accessibility:buildflags",
"//services/audio/public/cpp",
"//services/data_decoder/public/mojom",
"//services/device/public/cpp/geolocation",
"//services/device/public/cpp/usb",
"//services/metrics/public/cpp:gen_ukm_builders",
"//services/network:network_service",
Expand Down Expand Up @@ -5151,6 +5154,7 @@ source_set("unit_tests") {
"game_mode/game_mode_controller_for_borealis_unittest.cc",
"game_mode/testing/game_mode_controller_test_base.cc",
"game_mode/testing/game_mode_controller_test_base.h",
"geolocation/system_geolocation_source_unittest.cc",
"guest_os/guest_id_unittest.cc",
"guest_os/guest_os_diagnostics_builder_unittest.cc",
"guest_os/guest_os_external_protocol_handler_unittest.cc",
Expand Down
3 changes: 3 additions & 0 deletions chrome/browser/ash/crosapi/prefs_ash.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ const std::string& GetProfilePrefNameForPref(mojom::PrefPath path) {
profile_prefpath_to_name({
{mojom::PrefPath::kAccessibilitySpokenFeedbackEnabled,
ash::prefs::kAccessibilitySpokenFeedbackEnabled},
{mojom::PrefPath::kGeolocationAllowed,
ash::prefs::kUserGeolocationAllowed},
{mojom::PrefPath::kQuickAnswersEnabled,
quick_answers::prefs::kQuickAnswersEnabled},
{mojom::PrefPath::kQuickAnswersConsentStatus,
Expand Down Expand Up @@ -239,6 +241,7 @@ absl::optional<PrefsAsh::State> PrefsAsh::GetState(mojom::PrefPath path) {
return State{local_state_, &local_state_registrar_, false,
metrics::prefs::kMetricsReportingEnabled};
case mojom::PrefPath::kAccessibilitySpokenFeedbackEnabled:
case mojom::PrefPath::kGeolocationAllowed:
case mojom::PrefPath::kQuickAnswersEnabled:
case mojom::PrefPath::kQuickAnswersConsentStatus:
case mojom::PrefPath::kQuickAnswersDefinitionEnabled:
Expand Down
78 changes: 78 additions & 0 deletions chrome/browser/ash/geolocation/system_geolocation_source.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "chrome/browser/ash/geolocation/system_geolocation_source.h"

#include <utility>

#include "ash/constants/ash_pref_names.h"
#include "ash/session/session_controller_impl.h"
#include "ash/shell.h"
#include "base/check.h"
#include "base/functional/callback_helpers.h"
#include "components/prefs/pref_change_registrar.h"
#include "components/prefs/pref_service.h"
#include "services/device/public/cpp/geolocation/geolocation_manager.h"

namespace ash {

SystemGeolocationSource::SystemGeolocationSource()
: permission_update_callback_(base::DoNothing()) {
DCHECK(Shell::Get());
DCHECK(Shell::Get()->session_controller());
observer_.Observe(Shell::Get()->session_controller());
PrefService* last_active_user_pref_service =
Shell::Get()->session_controller()->GetLastActiveUserPrefService();
if (last_active_user_pref_service) {
OnActiveUserPrefServiceChanged(last_active_user_pref_service);
}
}

SystemGeolocationSource::~SystemGeolocationSource() = default;

// static
std::unique_ptr<device::GeolocationManager>
SystemGeolocationSource::CreateGeolocationManagerOnAsh() {
return std::make_unique<device::GeolocationManager>(
std::make_unique<SystemGeolocationSource>());
}

void SystemGeolocationSource::RegisterPermissionUpdateCallback(
PermissionUpdateCallback callback) {
permission_update_callback_ = std::move(callback);
if (pref_change_registrar_) {
OnPrefChanged(prefs::kUserGeolocationAllowed);
}
}

void SystemGeolocationSource::OnActiveUserPrefServiceChanged(
PrefService* pref_service) {
// Subscribing to pref changes.
pref_change_registrar_ = std::make_unique<PrefChangeRegistrar>();
pref_change_registrar_->Init(pref_service);
// value might have changed, hence we trigger the update function
OnPrefChanged(prefs::kUserGeolocationAllowed);
pref_change_registrar_->Add(
prefs::kUserGeolocationAllowed,
base::BindRepeating(&SystemGeolocationSource::OnPrefChanged,
base::Unretained(this)));
}

void SystemGeolocationSource::OnPrefChanged(const std::string& pref_name) {
DCHECK_EQ(pref_name, prefs::kUserGeolocationAllowed);
DCHECK(pref_change_registrar_);
// Get the actual permission status from CrOS by directly accessing pref
// service.
device::LocationSystemPermissionStatus status =
device::LocationSystemPermissionStatus::kNotDetermined;

PrefService* pref_service = pref_change_registrar_->prefs();
if (pref_service) {
status = pref_service->GetBoolean(prefs::kUserGeolocationAllowed)
? device::LocationSystemPermissionStatus::kAllowed
: device::LocationSystemPermissionStatus::kDenied;
}
permission_update_callback_.Run(status);
}
} // namespace ash
54 changes: 54 additions & 0 deletions chrome/browser/ash/geolocation/system_geolocation_source.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef CHROME_BROWSER_ASH_GEOLOCATION_SYSTEM_GEOLOCATION_SOURCE_H_
#define CHROME_BROWSER_ASH_GEOLOCATION_SYSTEM_GEOLOCATION_SOURCE_H_

#include <memory>
#include <string>

#include "ash/public/cpp/session/session_controller.h"
#include "ash/public/cpp/session/session_observer.h"
#include "base/scoped_observation.h"
#include "services/device/public/cpp/geolocation/system_geolocation_source.h"

namespace device {
class GeolocationManager;
}

class PrefService;
class PrefChangeRegistrar;

namespace ash {

// The SystemGeolocationSource is responsible for listening to geolocation
// permissions from the operation system and allows the
// device::GeolocationManager to access it in a platform agnostic manner. This
// concrete implementation is to be used within the Ash browser.
class SystemGeolocationSource : public device::SystemGeolocationSource,
public SessionObserver {
public:
SystemGeolocationSource();
~SystemGeolocationSource() override;

static std::unique_ptr<device::GeolocationManager>
CreateGeolocationManagerOnAsh();

// device::SystemGeolocationSource:
void RegisterPermissionUpdateCallback(
PermissionUpdateCallback callback) override;

private:
// SessionObserver:
void OnActiveUserPrefServiceChanged(PrefService* pref_service) override;

void OnPrefChanged(const std::string& pref_name);

PermissionUpdateCallback permission_update_callback_;
std::unique_ptr<PrefChangeRegistrar> pref_change_registrar_;
base::ScopedObservation<SessionController, SessionObserver> observer_{this};
};

} // namespace ash
#endif // CHROME_BROWSER_ASH_GEOLOCATION_SYSTEM_GEOLOCATION_SOURCE_H_
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// Copyright 2023 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "ash/system/privacy_hub/camera_privacy_switch_controller.h"

#include <utility>
#include <vector>

#include "ash/constants/ash_pref_names.h"
#include "ash/session/session_controller_impl.h"
#include "ash/shell.h"
#include "ash/test/ash_test_base.h"
#include "base/test/repeating_test_future.h"
#include "base/test/scoped_feature_list.h"
#include "chrome/browser/ash/geolocation/system_geolocation_source.h"
#include "components/prefs/pref_service.h"

namespace ash {

class SystemGeolocationSourceTests : public AshTestBase {
protected:
SystemGeolocationSourceTests()
: AshTestBase(base::test::TaskEnvironment::TimeSource::MOCK_TIME) {
scoped_feature_list_.InitAndEnableFeature(ash::features::kCrosPrivacyHub);
}

// AshTestBase:
void SetUp() override { AshTestBase::SetUp(); }

void SetUserPref(bool allowed) {
Shell::Get()->session_controller()->GetActivePrefService()->SetBoolean(
prefs::kUserGeolocationAllowed, allowed);
}

base::test::ScopedFeatureList scoped_feature_list_;
};

TEST_F(SystemGeolocationSourceTests, PermissionUpdate) {
SystemGeolocationSource source;
base::test::RepeatingTestFuture<device::LocationSystemPermissionStatus>
status;

source.RegisterPermissionUpdateCallback(status.GetCallback());

// Initial value should be to allow.
EXPECT_EQ(device::LocationSystemPermissionStatus::kAllowed, status.Take());

// Change user settings to deny and check that the callback is called.
SetUserPref(false);
EXPECT_EQ(device::LocationSystemPermissionStatus::kDenied, status.Take());

// Change user settings back to allowedy and check that the callback is
// called.
SetUserPref(true);
EXPECT_EQ(device::LocationSystemPermissionStatus::kAllowed, status.Take());
}

} // namespace ash
11 changes: 11 additions & 0 deletions chrome/browser/browser_process_platform_part_chromeos.cc
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "chrome/browser/ui/startup/startup_browser_creator.h"
#include "chrome/browser/ui/tabs/tab_strip_model.h"
#include "components/custom_handlers/protocol_handler_registry.h"
#include "services/device/public/cpp/geolocation/geolocation_manager.h"
#include "ui/base/page_transition_types.h"
#include "ui/base/window_open_disposition.h"

Expand All @@ -40,6 +41,16 @@ BrowserProcessPlatformPartChromeOS::BrowserRestoreObserver::
BrowserList::AddObserver(this);
}

device::GeolocationManager*
BrowserProcessPlatformPartChromeOS::geolocation_manager() {
return geolocation_manager_.get();
}

void BrowserProcessPlatformPartChromeOS::SetGeolocationManager(
std::unique_ptr<device::GeolocationManager> mgr) {
geolocation_manager_ = std::move(mgr);
}

BrowserProcessPlatformPartChromeOS::BrowserRestoreObserver::
~BrowserRestoreObserver() {
BrowserList::RemoveObserver(this);
Expand Down
4 changes: 4 additions & 0 deletions chrome/browser/browser_process_platform_part_chromeos.h
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ class BrowserProcessPlatformPartChromeOS

~BrowserProcessPlatformPartChromeOS() override;

device::GeolocationManager* geolocation_manager();
void SetGeolocationManager(std::unique_ptr<device::GeolocationManager>);

protected:
// Returns true if we can restore URLs for `profile`. Restoring URLs should
// only be allowed for regular signed-in users. This is currently virtual as
Expand Down Expand Up @@ -67,6 +70,7 @@ class BrowserProcessPlatformPartChromeOS
base::CallbackListSubscription on_session_restored_callback_subscription_;
};

std::unique_ptr<device::GeolocationManager> geolocation_manager_;
BrowserRestoreObserver browser_restore_observer_;
};

Expand Down
10 changes: 10 additions & 0 deletions chrome/browser/lacros/chrome_browser_main_extra_parts_lacros.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "base/feature_list.h"
#include "build/chromeos_buildflags.h"
#include "chrome/browser/browser_process.h"
#include "chrome/browser/browser_process_platform_part.h"
#include "chrome/browser/chromeos/reporting/metric_reporting_manager_lacros.h"
#include "chrome/browser/chromeos/tablet_mode/tablet_mode_page_behavior.h"
#include "chrome/browser/chromeos/video_conference/video_conference_manager_client.h"
Expand All @@ -22,6 +23,7 @@
#include "chrome/browser/lacros/field_trial_observer.h"
#include "chrome/browser/lacros/force_installed_tracker_lacros.h"
#include "chrome/browser/lacros/fullscreen_controller_client_lacros.h"
#include "chrome/browser/lacros/geolocation/system_geolocation_source_lacros.h"
#include "chrome/browser/lacros/lacros_extension_apps_controller.h"
#include "chrome/browser/lacros/lacros_extension_apps_publisher.h"
#include "chrome/browser/lacros/lacros_file_system_provider.h"
Expand Down Expand Up @@ -49,6 +51,7 @@
#include "chromeos/startup/browser_params_proxy.h"
#include "components/arc/common/intent_helper/arc_icon_cache_delegate.h"
#include "extensions/common/features/feature_session_type.h"
#include "services/device/public/cpp/geolocation/geolocation_manager.h"
#include "ui/views/controls/views_text_services_context_menu_chromeos.h"

namespace {
Expand Down Expand Up @@ -240,6 +243,10 @@ void ChromeBrowserMainExtraPartsLacros::PostProfileInit(
crosapi::ViewsTextServicesContextMenuLacros>(menu_model,
textfield);
}));

DCHECK(!g_browser_process->platform_part()->geolocation_manager());
g_browser_process->platform_part()->SetGeolocationManager(
SystemGeolocationSourceLacros::CreateGeolocationManagerOnLacros());
}

void ChromeBrowserMainExtraPartsLacros::PostMainMessageLoopRun() {
Expand All @@ -249,4 +256,7 @@ void ChromeBrowserMainExtraPartsLacros::PostMainMessageLoopRun() {
// Must be destroyed before |kiosk_session_service_->app_session_->profile_|
// is destroyed.
kiosk_session_service_.reset();

// Initialized in PreProfileInit.
g_browser_process->platform_part()->SetGeolocationManager(nullptr);
}

0 comments on commit 7653a0b

Please sign in to comment.