I have to raise the priority as this is a critical issue for the customer blocking them from launching G Suite deployment next month impacting 3K users.

I can take a look.

Initial testing with the following policy set on Linux:

{
  "OverrideSecurityRestrictionsOnInsecureOrigin": ["http://permission.site/"],
  "NotificationsAllowedForUrls": ["http://permission.site"]
}

This does correctly apply two effects of the OverrideSecurityRestrictionsOnInsecureOrigin policy:

1) Show the neutral (i) security indicator in the omnibox.
2) Treat the origin as a secure context (`isSecureContext` property in JavaScript returns `true`).

If we drop the NotificationsAllowedForUrls policy and try to request permission manually on http://permission.site, we just get a "denied" result, despite the origin being treated as a secure context. +peter@, the OWNER of the notifications module in Blink in case they have any ideas.

Either way, I'll dig into this more next week.

--

Separately, it looks like there may be some content settings state confusion however. The NotificationsAllowedForUrls policy shows "Notifications: Allowed" in Page Info, but is shown as "Blocked to protect your privacy" in chrome://settings/content/siteDetails?site=http%3A%2F%2Fpermission.site

+raymes@ and +msramek@ who are OWNERS of the content settings component where the policies are implemented -- do you have any ideas about the interplay of these policies? Also adding some relevant components.

Thanks for looking into this, Chris! Sounds like it's a regression from Chrome 68, so might be worth a bisect to see if we can pinpoint where this broke.

The issue seems to be Enterprise related, could anyone from enterprise Inhouse team look into this and help in bisecting.

Thanks.!

I ran a bisect and found the revision range for the regression with both policies:

https://chromium.googlesource.com/chromium/src/+log/ba054928d72a33c7693fc26f3bd01b5add5a0226..0dca2c3c5856698dce482a1733ef68beb44f74b2

From those commits, it looks like https://chromium.googlesource.com/chromium/src/+/ae1f9bae4aed3b364aa3fd19fbd482775efa4a83 is the most likely culprit, as it changes how notification permissions are checked. 

peter@ do you have any ideas of what would have changed with this change with regard to these policies?

An initial look at the permissions code [1] does look like it is using content::IsOriginSecure(), which in the browser process does not process entries from the policy allowlist. I'm not familiar enough with the code to know if this is the definitive place where the secure context check is applied browser-side though.

Previously, we had fixed this for security UI (the security indicator in the omnibox) -- see  Bug 869422 . This was scoped to only the security UI due to a pressing enterprise concern around the new "Not Secure" UI for HTTP pages. The underlying issue (that the policy only gets applied to _new_ processes, mainly renderers, due to how command line flags get created from policies). Bug 917107 has some initial discussion about seeing if we can instead handle this for anything above content/ by moving this to content::IsOriginSecure() instead, but there are issues with how to handle prefs at that layer.

If [1] is the right single place to handle permissions checks, then this is in chrome/ and we could instead use the new supplemental origin check code used in  Bug 869422 . That would be an easier fix for all permissions checks, but a more robust/complete fix would be to figure out a way to handle this in content/.

[1] https://cs.chromium.org/chromium/src/chrome/browser/permissions/permission_context_base.cc?l=209&rcl=a8eadaf186f4cec0649428a2c49d6e037040ad91

If my CL indeed broke it, then the permission checks were partial to begin with. Notifications used to have two paths to check the permission status, which was necessary due to a synchronous IPC in a world where we still supported NPAPI. That's been unified since.

All permission checks today happen through the content::PermissionController interface, which ends up calling PermissionManager::GetPermissionStatus(). The check that's now failing indeed is the one that cthomp@ points out. I agree with his assessment that the simple fix would be to check the whitelist there. Maybe a //chrome-level wrapper around IsOriginSecure() could be an interim step?

Lots of this code changed (significantly) since M69 however. Is there a target release for a fix?

As this is a potential blocker for an enterprise customer rolling out the browser to over 3000+ users, the sooner the better.

Looking at the release schedule, it may not be possible to land it in 72, so 73 would be the ideal.

Yes, I agree that your CL breaking it is really just incidental to the underlying root cause (enterprise policies not completely accessible in the browser process).

I agree that the best approach going forward may be to make a content/chrome split and have a chrome/ wrapper that has access to the enterprise policy prefs and can apply them anywhere it is used in chrome/ (and add a note or DEPS tricks to avoid using content::IsOriginSecure() in chrome/).

This should be do-able and merge-able to M-72 if we just land it for the permissions call site. Then, in a followup I could move all other chrome/ uses of content::IsOriginSecure() to use the wrapper instead.

This would mean that the policy would be applied in renderer processes and in chrome/ code, but not in any code in content/. This seems reasonable from the point of what enterprise policies should be (a Chrome concept rather than a content layer concept), but there may be other places where users of the policy might expect it to influence the content layer. Maybe I can later audit uses in content/ to see if there's anything that falls under the umbrella of the policy.

Last comment from the customer:

It seems the problem begin with the vesion 69 of Chrome. I set a desktop with the setting compliant with version 68 of Chrome and, I make Update with the packages ou gave me and notifications failed since the Version 69 (With OverrideSecurityRestrictionsOnInsecureOrigin policy set with *.<domain>) . With Version 70, 71 and 72 (beta 72.0.3626.53) we have the same results. 

In Version 69, 70, 71 and 72 (beta 72.0.3626.53) when we use Chrome --unsafely-treat-insecure-origin-as-secure flag, notifications works. But, on the same desktop, when we use the policy OverrideSecurityRestrictionsOnInsecureOrigin for *.<domain>, notifications aren't displayed.

An initial implementation of the generic chrome/ wrapper for this is in crrev.com/c/1417840. This can be used for anything in chrome/* that can provide a PrefService reference for the current Profile. I think that this may be a simple enough change to merge back to M-72.

That said...

There are still a number of places where content/ relies on content::IsOriginSecure() and can then have inconsistent results from higher layer features. Since the original intent of the enterprise policy was to allow secure-only web features, it feels weird that there may be browser-side-of-content places where the policy does not apply.

The following is a proposal for how we might be able to apply the policy at a much lower level. I think we might want to instead make content::IsOriginSecure() the canonical function (for everywhere, again), and add a new method to ContentBrowserClient/ChromeContentBrowserClient to do the following:

1. Add ContentBrowserClient::GetSecureOriginAndPatterns(BrowserContext* context) -- this seems potentially useful enough for other embedders to justify its inclusion here.
2. Implement ChromeContentBrowserClient::GetSecureOriginAndPatterns() which pulls and parses the Prefs (derive a Profile from BrowserContext, then pulls the Pref if it has been set by command line or policy).
3. content::IsWhitelistedAsSecureOrigin() can now call GetContentClient()->GetSecureOriginsAndPatterns() to get a list that the embedder can augment with policies/prefs as it sees fit.

ContentClient already is used for AddAdditionalSchemes [1]. This would essentially expand what is currently a static initialized early in startup (|origins|, accessible by GetSecureOriginsAndPatterns() in content/common/urls_schemes.h [2]) and have it go through ContentBrowserClient entirely to allow the embedder to include details from policy/prefs.

However, this may require that the call-site pass in a BrowserContext to the function, which is definitely a change in expectations. Thinking about call sites that would be affected and might not be able to pass a BrowserContext necessary:

* Stuff in components/ (such as components/payments/content/origin_security_checker.cc).
* Stuff in content/common (such as content/common/content_security_policy/csp_context.cc).
* Stuff in content/renderer like content/renderer/pepper/*
* content/shell/common/shell_origin_trial_policy.cc calls it (this is an override of a Blink interface).

It could be "allowable" to pass a null BrowserContext for places that do not have it (or keep the old IsOriginSecure(url::Origin) API for those). Places like components/payments could pass this through via their delegates (which might just return null and then we can handle it as before).

Then we can rip out all of the "above content" wrappers for this that currently exist (which includes the above linked CL).

[1] https://cs.chromium.org/chromium/src/content/public/common/content_client.h?l=151&rcl=12533689c127d188719970bd56ce30d34f3bdff4, implemented in ChromeContentClient here https://cs.chromium.org/chromium/src/chrome/common/chrome_content_client.cc?l=605&rcl=47edf766d78f27a57d061622220473cc5d3891eb
[2] https://cs.chromium.org/chromium/src/content/common/url_schemes.cc?l=43&rcl=04e7d6d99a84b4c3ed3062993d05688f32ca02e4

cc vogelheim who originally implemented the policy (iirc) in case he has thoughts on #17

Please provide ETA for the fix as the customer is facing deadline soon of a major G Suite deployment, and the issue impacts them significantly. 

The fix is currently being reviewed. Hopefully the fix will land on trunk later today and go to Canary for tomorrow. On verifying the fix in Canary I will request a merge to M-72. Then the fix will be available when M-72 goes to Stable next Tuesday.