Skip to content

Commit

Permalink
Use Geolocation System Permission in Permissions API on macOS.
Browse files Browse the repository at this point in the history
Currently the Permissions API is only using the site level permission.
This is confusing when the site level is allowed and the system level
permission is denied. The Permission API will say you are allowed to
but the request says permission denied. This change creates a
GeolocationPermissionContextMac which subscribes to system permission
updates and injects them where needed.

Bug: 1200933
Change-Id: I2c5bdc9785ce3481d3d78bf4523ffab967789af8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2877775
Commit-Queue: James Hollyer <jameshollyer@chromium.org>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Balazs Engedy <engedy@chromium.org>
Reviewed-by: Clark DuVall <cduvall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#892317}
  • Loading branch information
jameshollyergoogle authored and Chromium LUCI CQ committed Jun 14, 2021
1 parent c6499b8 commit 0fa6eca
Show file tree
Hide file tree
Showing 8 changed files with 283 additions and 19 deletions.
24 changes: 18 additions & 6 deletions chrome/browser/permissions/permission_manager_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,13 @@
#else
#include "chrome/browser/geolocation/geolocation_permission_context_delegate.h"
#include "chrome/browser/web_applications/components/file_handling_permission_context.h"
#if defined(OS_MAC)
#include "chrome/browser/browser_process.h"
#include "chrome/browser/browser_process_platform_part.h"
#include "components/permissions/contexts/geolocation_permission_context_mac.h"
#else
#include "components/permissions/contexts/geolocation_permission_context.h"
#endif
#include "components/permissions/contexts/nfc_permission_context.h"
#endif

Expand All @@ -66,17 +72,23 @@ permissions::PermissionManager::PermissionContextMap CreatePermissionContexts(
std::make_unique<permissions::MidiPermissionContext>(profile);
permission_contexts[ContentSettingsType::NOTIFICATIONS] =
std::make_unique<NotificationPermissionContext>(profile);
#if !defined(OS_ANDROID)
permission_contexts[ContentSettingsType::GEOLOCATION] =
std::make_unique<permissions::GeolocationPermissionContext>(
profile,
std::make_unique<GeolocationPermissionContextDelegate>(profile));
#else
#if defined(OS_ANDROID)
permission_contexts[ContentSettingsType::GEOLOCATION] =
std::make_unique<permissions::GeolocationPermissionContextAndroid>(
profile,
std::make_unique<GeolocationPermissionContextDelegateAndroid>(
profile));
#elif defined(OS_MAC)
permission_contexts[ContentSettingsType::GEOLOCATION] =
std::make_unique<permissions::GeolocationPermissionContextMac>(
profile,
std::make_unique<GeolocationPermissionContextDelegate>(profile),
g_browser_process->platform_part()->geolocation_manager());
#else
permission_contexts[ContentSettingsType::GEOLOCATION] =
std::make_unique<permissions::GeolocationPermissionContext>(
profile,
std::make_unique<GeolocationPermissionContextDelegate>(profile));
#endif
#if defined(OS_CHROMEOS) || defined(OS_ANDROID) || defined(OS_WIN)
permission_contexts[ContentSettingsType::PROTECTED_MEDIA_IDENTIFIER] =
Expand Down
6 changes: 5 additions & 1 deletion chrome/test/base/testing_browser_process_platform_part.cc
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,11 @@

TestingBrowserProcessPlatformPart::TestingBrowserProcessPlatformPart() {
#if defined(OS_MAC)
geolocation_manager_ = std::make_unique<device::FakeGeolocationManager>();
auto fake_geolocation_manager =
std::make_unique<device::FakeGeolocationManager>();
fake_geolocation_manager->SetSystemPermission(
device::LocationSystemPermissionStatus::kAllowed);
geolocation_manager_ = std::move(fake_geolocation_manager);
#endif
}

Expand Down
9 changes: 9 additions & 0 deletions components/permissions/BUILD.gn
Original file line number Diff line number Diff line change
Expand Up @@ -108,13 +108,20 @@ source_set("permissions") {
"//content/public/browser",
"//device/bluetooth",
"//device/bluetooth/public/cpp",
"//services/device/public/cpp/geolocation",
"//services/metrics/public/cpp:ukm_builders",
"//sql",
"//third_party/blink/public/common",
"//third_party/sqlite",
"//ui/base",
"//url",
]
if (is_mac) {
sources += [
"contexts/geolocation_permission_context_mac.cc",
"contexts/geolocation_permission_context_mac.h",
]
}
if (is_android) {
sources += [
"android/android_permission_util.cc",
Expand Down Expand Up @@ -180,6 +187,7 @@ source_set("test_support") {
"//components/ukm/content",
"//components/vector_icons",
"//content/public/browser",
"//services/device/public/cpp:test_support",
"//testing/gmock",
"//testing/gtest",
"//ui/gfx",
Expand Down Expand Up @@ -224,6 +232,7 @@ source_set("unit_tests") {
"//components/variations",
"//components/webrtc",
"//content/test:test_support",
"//services/device/public/cpp:test_support",
"//sql",
"//sql:test_support",
"//testing/gtest",
Expand Down
1 change: 1 addition & 0 deletions components/permissions/contexts/DEPS
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,5 @@ include_rules = [
"+services/device/public",
"+third_party/blink/public/common",
"+third_party/blink/public/mojom",
"+services/device/public/cpp/geolocation",
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// Copyright 2021 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "components/permissions/contexts/geolocation_permission_context_mac.h"

#include "content/public/browser/browser_thread.h"
#include "services/device/public/cpp/geolocation/location_system_permission_status.h"

namespace permissions {
GeolocationPermissionContextMac::GeolocationPermissionContextMac(
content::BrowserContext* browser_context,
std::unique_ptr<Delegate> delegate,
device::GeolocationManager* geolocation_manager)
: GeolocationPermissionContext(browser_context, std::move(delegate)) {
DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
system_permission_observers_ = geolocation_manager->GetObserverList();
system_permission_observers_->AddObserver(this);
system_permission_ = geolocation_manager->GetSystemPermission();
}

GeolocationPermissionContextMac::~GeolocationPermissionContextMac() {
system_permission_observers_->RemoveObserver(this);
}

ContentSetting GeolocationPermissionContextMac::GetPermissionStatusInternal(
content::RenderFrameHost* render_frame_host,
const GURL& requesting_origin,
const GURL& embedding_origin) const {
auto site_permission =
GeolocationPermissionContext::GetPermissionStatusInternal(
render_frame_host, requesting_origin, embedding_origin);
if (site_permission != ContentSetting::CONTENT_SETTING_ALLOW)
return site_permission;

switch (system_permission_) {
case LocationSystemPermissionStatus::kNotDetermined:
return ContentSetting::CONTENT_SETTING_ASK;
case LocationSystemPermissionStatus::kDenied:
return ContentSetting::CONTENT_SETTING_BLOCK;
case LocationSystemPermissionStatus::kAllowed:
return ContentSetting::CONTENT_SETTING_ALLOW;
}
}

void GeolocationPermissionContextMac::OnSystemPermissionUpdated(
LocationSystemPermissionStatus new_status) {
system_permission_ = new_status;
for (permissions::Observer& obs : permission_observers_) {
obs.OnPermissionChanged(ContentSettingsPattern::Wildcard(),
ContentSettingsPattern::Wildcard(),
ContentSettingsType::GEOLOCATION);
}
}

} // namespace permissions
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// Copyright 2021 The Chromium Authors. All rights reserved.
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#ifndef COMPONENTS_PERMISSIONS_CONTEXTS_GEOLOCATION_PERMISSION_CONTEXT_MAC_H_
#define COMPONENTS_PERMISSIONS_CONTEXTS_GEOLOCATION_PERMISSION_CONTEXT_MAC_H_

// The flow for geolocation permissions on macOS needs to take into account
// the system geolocation settings so it differs from the other platforms. It
// works as follows.
// GeolocationPermissionContextMac::RequestPermission intercepts the flow
// and proceeds to check the system location permission.
// If enabled, it proceeds with the per site flow via
// GeolocationPermissionContext (which will check per site permissions, create
// infobars, etc.).
//
// It also fires OnPermissionChanged when the effective permission state may
// have changed when the system permission is updated.

#include "components/permissions/contexts//geolocation_permission_context.h"
#include "services/device/public/cpp/geolocation/geolocation_manager.h"

namespace permissions {

using device::LocationSystemPermissionStatus;

class GeolocationPermissionContextMac
: public GeolocationPermissionContext,
public device::GeolocationManager::PermissionObserver {
public:
GeolocationPermissionContextMac(
content::BrowserContext* browser_context,
std::unique_ptr<Delegate> delegate,
device::GeolocationManager* geolocation_manager);
~GeolocationPermissionContextMac() override;

GeolocationPermissionContextMac(const GeolocationPermissionContextMac&) =
delete;
GeolocationPermissionContextMac& operator=(
const GeolocationPermissionContextMac&) = delete;

private:
// GeolocationPermissionContext:
ContentSetting GetPermissionStatusInternal(
content::RenderFrameHost* render_frame_host,
const GURL& requesting_origin,
const GURL& embedding_origin) const override;

// device::GeolocationManager::PermissionObserver:
void OnSystemPermissionUpdated(
LocationSystemPermissionStatus new_status) override;

LocationSystemPermissionStatus system_permission_ =
LocationSystemPermissionStatus::kNotDetermined;

scoped_refptr<device::GeolocationManager::PermissionObserverList>
system_permission_observers_;

// Must be the last member, to ensure that it will be destroyed first, which
// will invalidate weak pointers.
base::WeakPtrFactory<GeolocationPermissionContextMac> weak_factory_{this};
};

} // namespace permissions

#endif // COMPONENTS_PERMISSIONS_CONTEXTS_GEOLOCATION_PERMISSION_CONTEXT_MAC_H_

0 comments on commit 0fa6eca

Please sign in to comment.