Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(314)

Issue 1442083002: Stop inheriting push notification permissions from regular to incognito (Closed)

Created:
5 years, 1 month ago by johnme
Modified:
5 years ago
CC:
chromium-reviews, msramek+watch_chromium.org, Peter Beverloo, mlamouri+watch-notifications_chromium.org, kcarattini+watch_chromium.org, raymes+watch_chromium.org, mlamouri+watch-permissions_chromium.org, markusheintz_
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Stop inheriting push notification permissions from regular to incognito Normally, permissions granted in regular mode also apply to incognito. This patch makes an exception for the notifications and push messaging permissions, so that only blocks are inherited by incognito (i.e. permission grants are no longer inherited). This is because: - Notifications are unusually dangerous to the incognito privacy protections, for example a minimized window might be revealed to a shoulder surfer by a notification. - Push messaging is not currently supported in incognito, so if permission is granted in regular mode and carries over to incognito, but push is disabled, this would reveal that incognito mode is active. - Even if we later support push in incognito mode, push messaging registrations made in incognito would often expire before any messages are sent (wasting server-side resources), thus it's best if users explicitly opt in to push messaging in incognito. - Notifications and push messaging must behave the same in incognito, since push messaging is auto-granted by notifications permission, so otherwise a website might be able to determine that incognito mode is active by comparing these permissions. BUG=479679, 401439 TBR=finnur@chromium.org Committed: https://crrev.com/80e13f45116ea0baf481171f2b6dadef34fa81b8 Cr-Commit-Position: refs/heads/master@{#363514}

Patch Set 1 #

Total comments: 11

Patch Set 2 : Major rewrite #

Patch Set 3 : Annotate bool parameters #

Total comments: 6

Patch Set 4 : Addressed mvanouwerkerk's review comments #

Total comments: 24

Patch Set 5 : Address review comments #

Total comments: 14

Patch Set 6 : Move GetContentSettingValueAndPatterns to HostContentSettingsMap, and address other review comments #

Total comments: 8

Patch Set 7 : Address raymes' review comments #

Total comments: 2

Patch Set 8 : Address review comments (created TestUtils class) #

Total comments: 4

Patch Set 9 : Address review nits #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+583 lines, -473 lines) Patch
M chrome/browser/content_settings/content_settings_default_provider_unittest.cc View 1 2 3 4 5 6 7 9 chunks +66 lines, -103 lines 0 comments Download
M chrome/browser/content_settings/content_settings_policy_provider_unittest.cc View 1 2 3 4 5 6 7 5 chunks +35 lines, -67 lines 0 comments Download
M chrome/browser/content_settings/content_settings_pref_provider_unittest.cc View 1 2 3 4 5 6 7 10 chunks +52 lines, -100 lines 0 comments Download
M chrome/browser/content_settings/host_content_settings_map_unittest.cc View 1 2 3 4 5 6 7 8 11 chunks +139 lines, -17 lines 0 comments Download
M chrome/browser/extensions/api/content_settings/content_settings_store_unittest.cc View 1 2 3 4 5 6 7 8 2 chunks +2 lines, -1 line 0 comments Download
M components/content_settings/core/browser/content_settings_default_provider.cc View 1 2 3 4 5 6 1 chunk +4 lines, -0 lines 0 comments Download
M components/content_settings/core/browser/content_settings_info.h View 1 2 3 4 2 chunks +16 lines, -1 line 0 comments Download
M components/content_settings/core/browser/content_settings_info.cc View 1 2 3 4 1 chunk +4 lines, -2 lines 0 comments Download
M components/content_settings/core/browser/content_settings_provider_unittest.cc View 1 2 3 4 5 6 7 4 chunks +27 lines, -36 lines 0 comments Download
M components/content_settings/core/browser/content_settings_registry.h View 1 2 3 4 5 6 1 chunk +2 lines, -1 line 0 comments Download
M components/content_settings/core/browser/content_settings_registry.cc View 1 2 3 4 5 6 7 chunks +52 lines, -24 lines 3 comments Download
M components/content_settings/core/browser/content_settings_registry_unittest.cc View 1 2 3 4 1 chunk +6 lines, -0 lines 0 comments Download
M components/content_settings/core/browser/content_settings_utils.h View 1 2 3 4 5 6 1 chunk +0 lines, -18 lines 0 comments Download
M components/content_settings/core/browser/content_settings_utils.cc View 1 2 3 4 5 2 chunks +0 lines, -58 lines 0 comments Download
M components/content_settings/core/browser/host_content_settings_map.h View 1 2 3 4 5 6 7 4 chunks +20 lines, -0 lines 0 comments Download
M components/content_settings/core/browser/host_content_settings_map.cc View 1 2 3 4 5 6 7 4 chunks +88 lines, -10 lines 0 comments Download
M components/content_settings/core/test/content_settings_test_utils.h View 1 2 3 4 5 6 7 1 chunk +38 lines, -18 lines 0 comments Download
M components/content_settings/core/test/content_settings_test_utils.cc View 1 2 3 4 5 6 7 1 chunk +32 lines, -17 lines 0 comments Download

Messages

Total messages: 50 (14 generated)
johnme
peter: please review chrome/browser/notifications msramek: please review */content_settings Thanks!
5 years, 1 month ago (2015-11-13 18:12:48 UTC) #2
msramek
LGTM. Regarding the statement "dangerous to the incognito privacy protections [...] revealed to a shoulder ...
5 years, 1 month ago (2015-11-16 12:40:45 UTC) #3
Peter Beverloo
https://codereview.chromium.org/1442083002/diff/1/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc File chrome/browser/content_settings/content_settings_pref_provider_unittest.cc (right): https://codereview.chromium.org/1442083002/diff/1/chrome/browser/content_settings/content_settings_pref_provider_unittest.cc#newcode534 chrome/browser/content_settings/content_settings_pref_provider_unittest.cc:534: PrefProvider normal_provider(&prefs, false); nit: may be worth annotating the ...
5 years, 1 month ago (2015-11-16 17:14:32 UTC) #4
raymes
Sorry for chiming in late. I'm a bit concerned that this approach will lead to ...
5 years, 1 month ago (2015-11-17 00:16:00 UTC) #6
raymes
I also noticed the discussion on crbug.com/542081 but it seems like consensus wasn't reached there. ...
5 years, 1 month ago (2015-11-17 02:20:01 UTC) #7
msramek
Hmm, sorry, I should have given some background too. Raymes, I agree with everything you ...
5 years, 1 month ago (2015-11-17 10:37:56 UTC) #8
raymes
On 2015/11/17 10:37:56, msramek wrote: > Hmm, sorry, I should have given some background too. ...
5 years, 1 month ago (2015-11-18 01:48:34 UTC) #9
johnme
Ok, I've done a complete rewrite (several, in fact) of this patch, addressing all of ...
5 years ago (2015-11-26 17:31:54 UTC) #10
Michael van Ouwerkerk
Small drive-by review, not official. https://codereview.chromium.org/1442083002/diff/40001/components/content_settings/core/browser/content_settings_utils.cc File components/content_settings/core/browser/content_settings_utils.cc (right): https://codereview.chromium.org/1442083002/diff/40001/components/content_settings/core/browser/content_settings_utils.cc#newcode159 components/content_settings/core/browser/content_settings_utils.cc:159: return value; I'm wondering ...
5 years ago (2015-11-26 18:51:06 UTC) #11
johnme
Addressed mvanouwerkerk's review comments - thanks :) https://codereview.chromium.org/1442083002/diff/40001/components/content_settings/core/browser/content_settings_utils.cc File components/content_settings/core/browser/content_settings_utils.cc (right): https://codereview.chromium.org/1442083002/diff/40001/components/content_settings/core/browser/content_settings_utils.cc#newcode159 components/content_settings/core/browser/content_settings_utils.cc:159: return value; ...
5 years ago (2015-11-27 14:12:29 UTC) #14
msramek
Not LGTM, as a lot has changed since my last L-G-T-M and we need to ...
5 years ago (2015-11-27 16:30:05 UTC) #15
raymes
https://codereview.chromium.org/1442083002/diff/1/components/content_settings/core/browser/content_settings_pref.cc File components/content_settings/core/browser/content_settings_pref.cc (right): https://codereview.chromium.org/1442083002/diff/1/components/content_settings/core/browser/content_settings_pref.cc#newcode248 components/content_settings/core/browser/content_settings_pref.cc:248: // aren't inherited from regular to incognito. Their value_map_ ...
5 years ago (2015-11-30 00:28:46 UTC) #16
raymes
And thanks for making the changes! :)
5 years ago (2015-11-30 00:29:02 UTC) #17
johnme
Addressed review comments - thanks! https://codereview.chromium.org/1442083002/diff/1/components/content_settings/core/browser/content_settings_pref.cc File components/content_settings/core/browser/content_settings_pref.cc (right): https://codereview.chromium.org/1442083002/diff/1/components/content_settings/core/browser/content_settings_pref.cc#newcode248 components/content_settings/core/browser/content_settings_pref.cc:248: // aren't inherited from ...
5 years ago (2015-11-30 14:56:58 UTC) #18
raymes
https://codereview.chromium.org/1442083002/diff/80001/components/content_settings/core/browser/content_settings_utils.cc File components/content_settings/core/browser/content_settings_utils.cc (right): https://codereview.chromium.org/1442083002/diff/80001/components/content_settings/core/browser/content_settings_utils.cc#newcode172 components/content_settings/core/browser/content_settings_utils.cc:172: } What I was suggesting is to move this ...
5 years ago (2015-12-01 03:19:29 UTC) #19
msramek
Thanks for the attention to detail, John :-) LGTM % nits. https://codereview.chromium.org/1442083002/diff/60001/chrome/browser/content_settings/host_content_settings_map_unittest.cc File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): ...
5 years ago (2015-12-01 13:41:43 UTC) #20
johnme
Addressed review comments, hopefully - thanks :) https://codereview.chromium.org/1442083002/diff/60001/chrome/browser/content_settings/host_content_settings_map_unittest.cc File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1442083002/diff/60001/chrome/browser/content_settings/host_content_settings_map_unittest.cc#newcode684 chrome/browser/content_settings/host_content_settings_map_unittest.cc:684: HostContentSettingsMap* otr_map1 ...
5 years ago (2015-12-02 15:13:17 UTC) #21
raymes
Thanks John. Sorry for the multiple review rounds, thanks for being patient. I had a ...
5 years ago (2015-12-03 01:56:49 UTC) #22
johnme
Replied to/addressed review comments - thanks! https://codereview.chromium.org/1442083002/diff/80001/components/content_settings/core/browser/content_settings_utils.cc File components/content_settings/core/browser/content_settings_utils.cc (right): https://codereview.chromium.org/1442083002/diff/80001/components/content_settings/core/browser/content_settings_utils.cc#newcode172 components/content_settings/core/browser/content_settings_utils.cc:172: } On 2015/12/03 ...
5 years ago (2015-12-03 19:06:40 UTC) #23
raymes
Thanks! https://codereview.chromium.org/1442083002/diff/80001/components/content_settings/core/browser/content_settings_utils.cc File components/content_settings/core/browser/content_settings_utils.cc (right): https://codereview.chromium.org/1442083002/diff/80001/components/content_settings/core/browser/content_settings_utils.cc#newcode172 components/content_settings/core/browser/content_settings_utils.cc:172: } I'm confused though - AFAICT the code ...
5 years ago (2015-12-03 22:19:42 UTC) #24
johnme
Addressed review comments - thanks! https://codereview.chromium.org/1442083002/diff/80001/components/content_settings/core/browser/content_settings_utils.cc File components/content_settings/core/browser/content_settings_utils.cc (right): https://codereview.chromium.org/1442083002/diff/80001/components/content_settings/core/browser/content_settings_utils.cc#newcode172 components/content_settings/core/browser/content_settings_utils.cc:172: } On 2015/12/03 22:19:42, ...
5 years ago (2015-12-04 17:14:32 UTC) #25
raymes
lgtm! Thank you for helping make the code better :) https://codereview.chromium.org/1442083002/diff/140001/chrome/browser/content_settings/host_content_settings_map_unittest.cc File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1442083002/diff/140001/chrome/browser/content_settings/host_content_settings_map_unittest.cc#newcode741 ...
5 years ago (2015-12-07 03:02:30 UTC) #26
johnme
Done - thanks! https://codereview.chromium.org/1442083002/diff/140001/chrome/browser/content_settings/host_content_settings_map_unittest.cc File chrome/browser/content_settings/host_content_settings_map_unittest.cc (right): https://codereview.chromium.org/1442083002/diff/140001/chrome/browser/content_settings/host_content_settings_map_unittest.cc#newcode741 chrome/browser/content_settings/host_content_settings_map_unittest.cc:741: ContentSettingsPattern::FromString("[*.]example.com"); On 2015/12/07 03:02:30, raymes wrote: ...
5 years ago (2015-12-07 11:28:51 UTC) #27
Peter Beverloo
lgtm Would you please file a bug for the purpose of "disable notifications in incognito" ...
5 years ago (2015-12-07 13:51:19 UTC) #28
msramek
https://codereview.chromium.org/1442083002/diff/160001/components/content_settings/core/browser/content_settings_registry.cc File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1442083002/diff/160001/components/content_settings/core/browser/content_settings_registry.cc#newcode230 components/content_settings/core/browser/content_settings_registry.cc:230: CONTENT_SETTING_ASK, WebsiteSettingsInfo::SYNCABLE, On 2015/12/07 13:51:19, Peter Beverloo wrote: > ...
5 years ago (2015-12-07 15:05:42 UTC) #29
msramek
https://codereview.chromium.org/1442083002/diff/160001/components/content_settings/core/browser/content_settings_registry.cc File components/content_settings/core/browser/content_settings_registry.cc (right): https://codereview.chromium.org/1442083002/diff/160001/components/content_settings/core/browser/content_settings_registry.cc#newcode230 components/content_settings/core/browser/content_settings_registry.cc:230: CONTENT_SETTING_ASK, WebsiteSettingsInfo::SYNCABLE, On 2015/12/07 15:05:42, msramek wrote: > On ...
5 years ago (2015-12-07 15:15:45 UTC) #30
johnme
TBR=finnur for mechanical change to chrome/browser/extensions/api/content_settings/content_settings_store_unittest.cc
5 years ago (2015-12-07 15:35:22 UTC) #33
johnme
On 2015/12/07 13:51:19, Peter Beverloo wrote: > lgtm > > Would you please file a ...
5 years ago (2015-12-07 15:36:43 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1442083002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1442083002/160001
5 years ago (2015-12-07 15:37:27 UTC) #37
commit-bot: I haz the power
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/151019)
5 years ago (2015-12-07 16:04:22 UTC) #39
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1442083002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1442083002/160001
5 years ago (2015-12-07 16:20:26 UTC) #41
commit-bot: I haz the power
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_rel_ng/builds/151410)
5 years ago (2015-12-07 16:26:53 UTC) #43
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1442083002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1442083002/160001
5 years ago (2015-12-07 17:45:58 UTC) #45
commit-bot: I haz the power
Committed patchset #9 (id:160001)
5 years ago (2015-12-07 18:19:04 UTC) #47
commit-bot: I haz the power
Patchset 9 (id:??) landed as https://crrev.com/80e13f45116ea0baf481171f2b6dadef34fa81b8 Cr-Commit-Position: refs/heads/master@{#363514}
5 years ago (2015-12-07 18:20:13 UTC) #49
Finnur
5 years ago (2015-12-07 18:22:40 UTC) #50
Message was sent while issue was closed.
content_settings_store_unittest.cc LGTM

Powered by Google App Engine
This is Rietveld 408576698