Restore the iframe behavior of notification permissions

Restore the iframe behavior of notification permissions

Unlike other permissions, granting notifications for a given domain
will grant them even if such domain happens to be an iframe of
an embedder. Most other permissions are granted only for a combination of
iframe + embedder.

This reverts an unintentional behavioral change, and moves
us back to the situation we currently ship in stable while
we figure out if it is a good idea or not to change the behavior.

BUG=416645
Committed: https://crrev.com/bdfadb831ade3952a192d2487daebdf73df71ed7
Cr-Commit-Position: refs/heads/master@{#296372}

[Miguel Garcia, 2014-09-23 15:44:00]

miguelg@chromium.org changed reviewers:
+ bauerb@chromium.org, peter@chromium.org

[Miguel Garcia, 2014-09-23 15:44:01]

This is an unfortunate finding that notification permissions behave differently
with regards to iframe behaviour (partially because of the way it is implemented
through a sync IPC). Please reach back to me if you need more context on the
change.

[Peter Beverloo, 2014-09-23 15:58:47]

lgtm % 3 things.

  (1) Comments.
  (2) Please clarify in the description that this reverts an unintentional
behavioral change, and moves us back to the situation we currently ship in
stable.
  (3) Please file a bug because we need to reconsider this behavior.

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_queue_controller.cc (right):

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/permission_queue_controller.cc:373: if (type_ ==
CONTENT_SETTINGS_TYPE_NOTIFICATIONS) {
Can you file a bug about removing this (and other) exceptions, and reconsidering
the permission story we have about notifications?

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/permission_queue_controller.cc:375:
ContentSettingsPattern::FromURLNoWildcard(
nit: indent with four spaces.

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:317: // will grant
it even if it is embedded as an iframe in other domains.
This is phrased a bit ambiguously, what about the following?

Unlike other permission types, granting the notification permission to the
origin of an embedded frame will persist when the origin is loaded autonomously,
or as part of another embedding origin.

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:328: 
nit: only a single blank line.

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.h (right):

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.h:138: virtual void
UpdateContentSetting(
nit: we should be able to format this code as:

virtual void UpdateContentSetting(const GURL& requesting_origin,
                                  const GURL& embedder_origin,
                                  bool allowed) OVERRIDE;

[Bernhard Bauer, 2014-09-23 16:13:02]

LGTM with some more nits:

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.h (right):

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.h:101: // Store the
decided permission as a content setting.
Please add a comment that explains why this is virtual.

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_queue_controller.cc (right):

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/permission_queue_controller.cc:377:
ContentSettingsPattern::Wildcard(),
This is the only difference to the call below, right? If so, consider pulling
this out into a variable and merging the two calls.

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:317: // will grant
it even if it is embedded as an iframe in other domains.
On 2014/09/23 15:58:47, Peter Beverloo wrote:
> This is phrased a bit ambiguously, what about the following?
> 
> Unlike other permission types, granting the notification permission to the
> origin of an embedded frame will persist when the origin is loaded
autonomously,
> or as part of another embedding origin.

I would point out in the comment that the embedding origin simply doesn't
matter. Permissions are granted purely based on the requesting iframe origin,
which is why we override this method to ignore |embedder_origin|.

Also, the bikeshed should be red.

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:322: if (allowed)
Braces can only be omitted for single-line bodies.

[Miguel Garcia, 2014-09-23 16:53:38]

miguelg@chromium.org changed reviewers:
+ dewittj@chromium.org

[Miguel Garcia, 2014-09-23 16:53:39]

+dewittj for browser/notifications/ OWNERS

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_context_base.h (right):

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/permission_context_base.h:101: // Store the
decided permission as a content setting.
On 2014/09/23 16:13:01, Bernhard Bauer wrote:
> Please add a comment that explains why this is virtual.

Done.

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/content_s...
File chrome/browser/content_settings/permission_queue_controller.cc (right):

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/permission_queue_controller.cc:373: if (type_ ==
CONTENT_SETTINGS_TYPE_NOTIFICATIONS) {
On 2014/09/23 15:58:47, Peter Beverloo wrote:
> Can you file a bug about removing this (and other) exceptions, and
reconsidering
> the permission story we have about notifications?

Done. (crbug.com/416894)

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/permission_queue_controller.cc:375:
ContentSettingsPattern::FromURLNoWildcard(
On 2014/09/23 15:58:46, Peter Beverloo wrote:
> nit: indent with four spaces.

Done.

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/content_s...
chrome/browser/content_settings/permission_queue_controller.cc:377:
ContentSettingsPattern::Wildcard(),
On 2014/09/23 16:13:02, Bernhard Bauer wrote:
> This is the only difference to the call below, right? If so, consider pulling
> this out into a variable and merging the two calls.

Done.

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.cc (right):

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:317: // will grant
it even if it is embedded as an iframe in other domains.
On 2014/09/23 16:13:02, Bernhard Bauer wrote:
> On 2014/09/23 15:58:47, Peter Beverloo wrote:
> > This is phrased a bit ambiguously, what about the following?
> > 
> > Unlike other permission types, granting the notification permission to the
> > origin of an embedded frame will persist when the origin is loaded
> autonomously,
> > or as part of another embedding origin.
> 
> I would point out in the comment that the embedding origin simply doesn't
> matter. Permissions are granted purely based on the requesting iframe origin,
> which is why we override this method to ignore |embedder_origin|.
> 
> Also, the bikeshed should be red.

Done.

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:317: // will grant
it even if it is embedded as an iframe in other domains.
On 2014/09/23 15:58:47, Peter Beverloo wrote:
> This is phrased a bit ambiguously, what about the following?
> 
> Unlike other permission types, granting the notification permission to the
> origin of an embedded frame will persist when the origin is loaded
autonomously,
> or as part of another embedding origin.

Acknowledged.

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:322: if (allowed)
On 2014/09/23 16:13:02, Bernhard Bauer wrote:
> Braces can only be omitted for single-line bodies.

Done.

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.cc:328: 
On 2014/09/23 15:58:47, Peter Beverloo wrote:
> nit: only a single blank line.

Done.

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/notificat...
File chrome/browser/notifications/desktop_notification_service.h (right):

https://codereview.chromium.org/593153002/diff/20001/chrome/browser/notificat...
chrome/browser/notifications/desktop_notification_service.h:138: virtual void
UpdateContentSetting(
On 2014/09/23 15:58:47, Peter Beverloo wrote:
> nit: we should be able to format this code as:
> 
> virtual void UpdateContentSetting(const GURL& requesting_origin,
>                                   const GURL& embedder_origin,
>                                   bool allowed) OVERRIDE;

Done.

[Miguel Garcia, 2014-09-24 07:48:08]

miguelg@chromium.org changed reviewers:
+ mukai@chromium.org

[Miguel Garcia, 2014-09-24 07:48:32]

Friendly ping on this review. I have also added Jun in case dewittj is OOO

[Jun Mukai, 2014-09-24 08:15:13]

I am not sure why notification is so special.
I didn't realize this case but not-handling embedded frame scenario sounds like
a bug on notification code. Am I wrong?

[Jun Mukai, 2014-09-24 08:24:09]

If you recognize this solution is not ideal but simply want to change the
behavior back to the previous version for the time being, then this CL lgtm.

However please add a TODO(mukai) at
DesktopNotificationService::UpdateContentSetting to consider the embedded frame
case more properly.

[Miguel Garcia, 2014-09-24 08:27:36]

On 2014/09/24 08:24:09, Jun Mukai wrote:
> If you recognize this solution is not ideal but simply want to change the
> behavior back to the previous version for the time being, then this CL lgtm.
> 
> However please add a TODO(mukai) at
> DesktopNotificationService::UpdateContentSetting to consider the embedded
frame
> case more properly.

Thanks Jun I'll add the TODO and CC you on crbug.com/416894 which has some more
information and where we can decide a final solution for this.

[Miguel Garcia, 2014-09-24 08:35:32]

The CQ bit was checked by miguelg@chromium.org

[commit-bot: I haz the power, 2014-09-24 08:36:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/593153002/60001

[commit-bot: I haz the power, 2014-09-24 09:20:35]

Committed patchset #4 (id:60001) as 4ee487ba9a071f9742061e3814a752787a207394

[commit-bot: I haz the power, 2014-09-24 09:21:14]

Patchset 4 (id:??) landed as
https://crrev.com/bdfadb831ade3952a192d2487daebdf73df71ed7
Cr-Commit-Position: refs/heads/master@{#296372}