New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 689799 link

Starred by 2 users
Status: Started
Owner:
timloh@chromium.org
Cc:
█ raymes@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: All
Pri: 2
Type: Bug
Team-Security-UX



Sign in to add a comment

Permissions/Content Settings Refactoring

Project Member Reported by timloh@chromium.org, Feb 8 2017 
Raymes has written a doc proposing various refactorings in the permissions code: https://docs.google.com/document/d/1F2SXlanlqHFobtCsXpGZBUXu22Df5hB0EKGmrfmNsio/edit#

There are several problems described in the doc:

The first is the state of the PermissionType/ContentSettingsType and PermissionStatus/ContentSetting enums. We would like to use only the latter types within chrome/, converting from the content/ types at the content/ boundary. This will require adding extra values (MIDI, PUSH_MESSAGING) to the ContentSettingsType enum, but will allow us to clean up classes storing both enum types, and make it obvious which type to use where.

The second is about code accessing the HostContentSettingsMap directly instead of via the PermissionsManager. We want to ensure that code querying permission information is using the PermissionsManager as much as possible.

The third is about permission logic in content_settings. We would like to move all permission logic in components/content_settings into chrome/browser/permissions.
Project Member

Comment 1 by bugdroid1@chromium.org, Feb 20 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9a180ad9469617b9477a805f0676b1dbca15f395

commit 9a180ad9469617b9477a805f0676b1dbca15f395
Author: timloh <timloh@chromium.org>
Date: Mon Feb 20 07:15:23 2017

Replace PermissionType in chrome/ with ContentSettingsType

This patch replaces most usages of PermissionType in chrome/ with
ContentSettingsType. This reverts some of the past changes moving in
the opposite direction, cleaning up the side-by-side enum storages of
both PermissionType and ContentSettingsType in several classes, the main
one being PermissionContextBase. Ideally we'd like PermissionManager to
be the only class in chrome/ using PermissionType, but we may continue
to use it in PermissionUmaUtil as the enum value is used directly in the
ContentSettings.PermissionRequested histogram.

This patch (re-)adds a CONTENT_SETTINGS_TYPE_PUSH_MESSAGING enum value.
The enum is converted to the NOTIFICATION enum value in a few places
(NotificationPermissionContext, PermissionContextBase and
PermissionQueueController) so that it doesn't ever reach the
HostContentSettingsMap. This is necessary for classes like
NotificationPermissionContext to work correctly. We are hoping to
further unify the logic of the push messaging and notification
permissions in the future so as to not require a separate enum.

Some PermissionType usages are left untouched for now as they need to
handle PermissionType::MIDI. This permission is always set to true and
doesn't currently have a corresponding ContentSettingsType enum value.
A subsequent patch will add this value to ContentSettingsType.

This patch should make it clear where the different enum types should be
used and reduce the conversions between these types.

BUG=689799
TBR=msramek,iclelland,devlin,xhwang,tommycli,nparker,tapted,mnissler,lfg,jsbell

Review-Url: https://codereview.chromium.org/2675483002
Cr-Commit-Position: refs/heads/master@{#451574}

[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/background_sync/background_sync_permission_context.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/browsing_data/browsing_data_remover_impl_unittest.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/chromeos/attestation/platform_verification_flow.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/extensions/service_worker_apitest.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/geolocation/geolocation_infobar_delegate_android.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/geolocation/geolocation_permission_context.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/geolocation/geolocation_permission_context_unittest.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/media/midi_permission_context.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/media/midi_permission_infobar_delegate_android.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/media/protected_media_identifier_infobar_delegate_android.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/media/protected_media_identifier_permission_context.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/media/webrtc/media_permission.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/media/webrtc/media_stream_device_permission_context.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/media/webrtc/media_stream_device_permission_context.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/media/webrtc/media_stream_device_permission_context_unittest.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/media/webrtc/media_stream_devices_controller.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/media/webrtc/media_stream_devices_controller.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/media/webrtc/media_stream_devices_controller_browsertest.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/media/webrtc/media_stream_infobar_delegate_android.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/notifications/message_center_settings_controller_unittest.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/notifications/notification_permission_context.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/notifications/notification_permission_context.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/notifications/notification_permission_context_unittest.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/notifications/notification_permission_infobar_delegate.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/notifications/notification_permission_infobar_delegate.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/notifications/notifier_state_tracker.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/notifications/platform_notification_service_impl.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/delegation_tracker.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/delegation_tracker.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/delegation_tracker_unittest.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_blacklist_client.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_blacklist_client.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_context_base.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_context_base.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_context_base_unittest.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_decision_auto_blocker.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_decision_auto_blocker.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_decision_auto_blocker_unittest.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_dialog_delegate.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_dialog_delegate.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_infobar_delegate.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_infobar_delegate.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_manager.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_manager.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_queue_controller.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_queue_controller.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_queue_controller_unittest.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_request.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_request_impl.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_request_impl.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_request_manager_browsertest.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_request_manager_test_api.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_request_manager_test_api.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_uma_util.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_uma_util.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_util.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/permissions/permission_util.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/plugins/flash_download_interception.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/plugins/flash_permission_context.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/plugins/flash_temporary_permission_tracker_unittest.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/push_messaging/push_messaging_service_impl.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/push_messaging/push_messaging_service_impl.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/safe_browsing/permission_reporter.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/safe_browsing/permission_reporter.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/safe_browsing/permission_reporter_unittest.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/safe_browsing/ping_manager.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/storage/durable_storage_permission_context.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/ui/cocoa/website_settings/permission_bubble_cocoa_interactive_uitest.mm
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/ui/website_settings/website_settings.cc
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/chrome/browser/ui/website_settings/website_settings_ui.h
[modify] https://crrev.com/9a180ad9469617b9477a805f0676b1dbca15f395/components/content_settings/core/common/content_settings_types.h
Project Member

Comment 2 by bugdroid1@chromium.org, Feb 23 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a5284b9c11fb616ff61bc3343584409cf1e558fa

commit a5284b9c11fb616ff61bc3343584409cf1e558fa
Author: timloh <timloh@chromium.org>
Date: Thu Feb 23 06:12:21 2017

Remove redundant GetPermissionType call in ~ScopedRevocationReporter

The GetPermissionType call in ~ScopedRevocationReporter checks whether
the given type has an associated PermissionType, but doesn't make use of
the PermissionType value. The guarded function, PermissionRevoked, has a
stricter check (notifications, geolocation, mic or camera), so there's
no reason to actually call GetPermissionType.

BUG=689799

Review-Url: https://codereview.chromium.org/2707383004
Cr-Commit-Position: refs/heads/master@{#452416}

[modify] https://crrev.com/a5284b9c11fb616ff61bc3343584409cf1e558fa/chrome/browser/permissions/permission_util.cc
Project Member

Comment 3 by bugdroid1@chromium.org, Feb 23 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/592d7328de7543536c9928a5b0d11e3a97379c70

commit 592d7328de7543536c9928a5b0d11e3a97379c70
Author: timloh <timloh@chromium.org>
Date: Thu Feb 23 07:23:54 2017

Make PermissionManager use ContentSettingsType internally more

This patch adds the MIDI permission to ContentSettingsType so that the
PermissionManager can use ContentSettingsType internally more. The inner
classes of it, PendingRequest and Subscription, are updated to store a
vector<ContentSettingsType> and a ContentSettingsType respectively.

As the added MIDI permission is always set to allow, we simply maintain
the existing design of having the PermissionManager always resolve it
and not pass it down into the HostContentSettingsMap.

BUG=689799

Review-Url: https://codereview.chromium.org/2714603002
Cr-Commit-Position: refs/heads/master@{#452426}

[modify] https://crrev.com/592d7328de7543536c9928a5b0d11e3a97379c70/chrome/browser/permissions/permission_manager.cc
[modify] https://crrev.com/592d7328de7543536c9928a5b0d11e3a97379c70/chrome/browser/permissions/permission_manager.h
[modify] https://crrev.com/592d7328de7543536c9928a5b0d11e3a97379c70/chrome/browser/permissions/permission_uma_util.cc
[modify] https://crrev.com/592d7328de7543536c9928a5b0d11e3a97379c70/chrome/browser/permissions/permission_uma_util.h
[modify] https://crrev.com/592d7328de7543536c9928a5b0d11e3a97379c70/chrome/browser/permissions/permission_util.cc
[modify] https://crrev.com/592d7328de7543536c9928a5b0d11e3a97379c70/chrome/browser/permissions/permission_util.h
[modify] https://crrev.com/592d7328de7543536c9928a5b0d11e3a97379c70/components/content_settings/core/common/content_settings_types.h
Project Member

Comment 4 by bugdroid1@chromium.org, Mar 1 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c6911808d9c3d3abbbc59829a63459c63d1645dc

commit c6911808d9c3d3abbbc59829a63459c63d1645dc
Author: timloh <timloh@chromium.org>
Date: Wed Mar 01 05:37:03 2017

Use ContentSetting in chrome/ instead of PermissionStatus

This patch updates the PermissionManager so that it's methods exposed to
chrome/ use ContentSetting instead of PermissionStatus. This is to avoid
ContentSetting values being converted to PermissionStatus values and
back again, and makes it clearer which type should be used in chrome/.

The GetPermissionStatus is changed to return a PermissionResult instead
of a PermissionStatus. This will allow us to, for example, surface
denial reasons in website settings.

BUG=689799
TBR=peter,tommycli,xhwang,lfg,devlin,mnissler

Review-Url: https://codereview.chromium.org/2713083003
Cr-Commit-Position: refs/heads/master@{#453855}

[modify] https://crrev.com/c6911808d9c3d3abbbc59829a63459c63d1645dc/chrome/browser/chromeos/attestation/platform_verification_flow.cc
[modify] https://crrev.com/c6911808d9c3d3abbbc59829a63459c63d1645dc/chrome/browser/extensions/service_worker_apitest.cc
[modify] https://crrev.com/c6911808d9c3d3abbbc59829a63459c63d1645dc/chrome/browser/geolocation/geolocation_permission_context_unittest.cc
[modify] https://crrev.com/c6911808d9c3d3abbbc59829a63459c63d1645dc/chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.cc
[modify] https://crrev.com/c6911808d9c3d3abbbc59829a63459c63d1645dc/chrome/browser/guest_view/web_view/chrome_web_view_permission_helper_delegate.h
[modify] https://crrev.com/c6911808d9c3d3abbbc59829a63459c63d1645dc/chrome/browser/media/webrtc/media_permission.cc
[modify] https://crrev.com/c6911808d9c3d3abbbc59829a63459c63d1645dc/chrome/browser/notifications/message_center_settings_controller_unittest.cc
[modify] https://crrev.com/c6911808d9c3d3abbbc59829a63459c63d1645dc/chrome/browser/notifications/notification_permission_context_unittest.cc
[modify] https://crrev.com/c6911808d9c3d3abbbc59829a63459c63d1645dc/chrome/browser/notifications/notifier_state_tracker.cc
[modify] https://crrev.com/c6911808d9c3d3abbbc59829a63459c63d1645dc/chrome/browser/notifications/platform_notification_service_impl.cc
[modify] https://crrev.com/c6911808d9c3d3abbbc59829a63459c63d1645dc/chrome/browser/notifications/platform_notification_service_interactive_uitest.cc
[modify] https://crrev.com/c6911808d9c3d3abbbc59829a63459c63d1645dc/chrome/browser/permissions/permission_manager.cc
[modify] https://crrev.com/c6911808d9c3d3abbbc59829a63459c63d1645dc/chrome/browser/permissions/permission_manager.h
[modify] https://crrev.com/c6911808d9c3d3abbbc59829a63459c63d1645dc/chrome/browser/permissions/permission_manager_unittest.cc
[modify] https://crrev.com/c6911808d9c3d3abbbc59829a63459c63d1645dc/chrome/browser/plugins/flash_download_interception.cc
[modify] https://crrev.com/c6911808d9c3d3abbbc59829a63459c63d1645dc/chrome/browser/push_messaging/push_messaging_service_impl.cc
[modify] https://crrev.com/c6911808d9c3d3abbbc59829a63459c63d1645dc/chrome/browser/push_messaging/push_messaging_service_impl.h

Comment 5 by est...@chromium.org, Nov 10 2017

Labels: Hotlist-EnamelAndFriendsFixIt

Comment 6 by est...@chromium.org, Feb 18 2018

Labels: -Hotlist-EnamelAndFriendsFixIt
►

Sign in to add a comment