Closed Bug 1421707 Opened 4 years ago Closed 3 years ago

Implement a system for disabling about: pages via Policy

Categories

(Firefox :: Enterprise Policies, enhancement, P1)

Product:
Firefox
Component:
Enterprise Policies
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- verified

People

(Reporter: bytesized, Assigned: bytesized)

References

Details

Attachments

(3 files, 1 obsolete file)

policies.json
55 bytes, application/json
Details
Blocked about config.png
164.05 KB, image/png
Details
Bug 1421707 - Implement a system for disabling about: pages via Policy
59 bytes, text/x-review-board-request
bzbarsky
: review+
Felipe
: review+
Details 
Many policies require that an about: page (ex: "about:config") be disabled. CCK2 does this by registering a factory with a contract ID that will cause it to return a "disabled page" when that about: page is requested [1][2].

This approach is quite easy to recreate, but I would also like to investigate how difficult it would be to implement something a bit cleaner.


[1] https://github.com/mkaply/cck2wizard/blob/a2979914fce6589cc22fa3b1fd0b46eadfe1ae0d/cck2/modules/CCK2.jsm#L380-L385
[2] https://github.com/mkaply/cck2wizard/blob/a2979914fce6589cc22fa3b1fd0b46eadfe1ae0d/cck2/modules/CCK2.jsm#L1249-L1271
Group: mozilla-employee-confidential
Component: General → Enterprise Policies
The patch posted specifically allows about:config to be blocked, but introduces a mechanism that should block any about page that we want to block. The only pieces missing to block other about pages are schema and policy implementation. The policy implementation can mimic that of the "block_about_config" policy: |manager.disallowFeature("about:_pagename_", true);|.
Attached file policies.json — 
To enable the patch's functionality, two prefs must be added and set:
  browser.policies.enabled = true
  browser.policies.alternatePath = "/path/to/policies.json"

This copy of policies.json enables just the policy to block about:config.
Comment on attachment 8941602 [details]
Bug 1421707 - Implement a system for disabling about: pages via Policy

Felipe, can you take a look at this and tell me what you think?
Attachment #8941602 - Flags: feedback?(felipc)
It should be noted that while the attached patch is (I believe) functionally complete, I am less sure about the user interface component. That is to say, we may want to change the text that the user sees on navigation to "about:config". Right now, the user will see a page that looks something like:

###
#!# Page Blocked
###

   This site has been blocked.

   Access has been disabled by your administrator.
Attachment #8941602 - Flags: review?(felipc)
Felipe, could you take a look at the Policy Manager related parts of my patch? I'm still trying to figure out who should review the rest, so let me know if you have any suggestions.
Blocks: 1429176
Comment on attachment 8941602 [details]
Bug 1421707 - Implement a system for disabling about: pages via Policy

https://reviewboard.mozilla.org/r/211864/#review217890

r+ for the policies part!

We should get someone involved from copywriting to suggest the final strings for the error page, keeping in mind that we want to use this same error msg for normal websites that were blocked by policies too (bug 1429178)

::: browser/components/enterprisepolicies/tests/browser/browser.ini:8
(Diff revision 2)
>    browser.policies.enabled=true
>  support-files =
>    head.js
>    config_simple_policies.json
>    config_broken_json.json
> +  block_about_config.json

please prepend this file with config_ to keep the pattern

::: browser/components/enterprisepolicies/tests/browser/browser_policy_block_about_config.js:13
(Diff revision 2)
> +    let body = content.document.getElementsByTagName("body")[0];
> +    is(body.firstElementChild.id, "errorPageContainer",
> +       "about:config should look like a net error page");

You can replace this trick with checking the content.document.documentURI, as it points to the internal URL that was loaded. See it being used here:

https://searchfox.org/mozilla-central/source/browser/base/content/content.js#244

::: dom/locales/en-US/chrome/appstrings.properties:41
(Diff revision 2)
>  corruptedContentErrorv2=The site at %S has experienced a network protocol violation that cannot be repaired.
>  remoteXUL=This page uses an unsupported technology that is no longer available by default.
>  sslv3Used=The safety of your data on %S could not be guaranteed because it uses SSLv3, a broken security protocol.
>  weakCryptoUsed=The owner of %S has configured their website improperly. To protect your information from being stolen, the connection to this website has not been established.
>  inadequateSecurityError=The website tried to negotiate an inadequate level of security.
> +blockedByPolicy=This site has been blocked.

where does this string show up?

I suggest changing "site" to "page".
Attachment #8941602 - Flags: review?(felipc) → review+
Comment on attachment 8941602 [details]
Bug 1421707 - Implement a system for disabling about: pages via Policy

https://reviewboard.mozilla.org/r/211864/#review217890

> where does this string show up?
> 
> I suggest changing "site" to "page".

It shows up on the net error page between the title and the long description.
@bz I had a hard time picking a reviewer for this patch. Let me know if you would prefer that I ask someone else.
Attachment #8941602 - Flags: review?(bzbarsky)
Attachment #8941602 - Flags: review?(bzbarsky)
I think it would make sense for this patch to remove blocked about: pages from about:about. I am going to add that functionality and re-request review.
Comment on attachment 8941602 [details]
Bug 1421707 - Implement a system for disabling about: pages via Policy

https://reviewboard.mozilla.org/r/211864/#review218048

::: browser/base/content/aboutNetError.xhtml:312
(Diff revision 3)
>  
>          var event = new CustomEvent("AboutNetErrorLoad", {bubbles: true});
>          document.dispatchEvent(event);
>  
> -        if (err == "inadequateSecurityError") {
> +        if (err == "inadequateSecurityError" || err == "blockedByPolicy") {
>            // Remove the "Try again" button for HTTP/2 inadequate security as it

This comment should get updated.

::: browser/locales/en-US/chrome/overrides/appstrings.properties:43
(Diff revision 3)
>  corruptedContentErrorv2=The site at %S has experienced a network protocol violation that cannot be repaired.
>  remoteXUL=This page uses an unsupported technology that is no longer available by default in Firefox.
>  ## LOCALIZATION NOTE (sslv3Used) - Do not translate "%S".
>  sslv3Used=Firefox cannot guarantee the safety of your data on %S because it uses SSLv3, a broken security protocol.
>  inadequateSecurityError=The website tried to negotiate an inadequate level of security.
> +blockedByPolicy=This page has been blocked.

Might as well give more info about what it has been blocked by?

::: docshell/base/nsAboutRedirector.cpp:191
(Diff revision 3)
>          aLoadInfo->SetResultPrincipalURI(tempURI);
>        }
>  
>        tempChannel->SetOriginalURI(aURI);
>  
> +      nsCOMPtr<nsIEnterprisePolicies> policyManager =

Why is this in the docshell about: redirector.  That won't work for blocking things like about:preferences (which is implemented in browser/components/about/AboutRedirector.cpp), and I assume we do in fact want to block them, right?

Might be better to put this in the about: protocol handler...

Alternately, we could hack the "get an about: module" bits to return something appropriate here, which would possibly help with about:about too, maybe.  Not sure.

::: docshell/resources/content/netError.xhtml:182
(Diff revision 3)
>            var secOverride = document.getElementById("securityOverrideDiv");
>            secOverride.remove();
>          }
>  
> -        if (err == "inadequateSecurityError") {
> +        if (err == "inadequateSecurityError" || err == "blockedByPolicy") {
>            // Remove the "Try again" button for HTTP/2 inadequate security as it

Again, fix comment.

::: dom/locales/en-US/chrome/appstrings.properties:41
(Diff revision 3)
>  corruptedContentErrorv2=The site at %S has experienced a network protocol violation that cannot be repaired.
>  remoteXUL=This page uses an unsupported technology that is no longer available by default.
>  sslv3Used=The safety of your data on %S could not be guaranteed because it uses SSLv3, a broken security protocol.
>  weakCryptoUsed=The owner of %S has configured their website improperly. To protect your information from being stolen, the connection to this website has not been established.
>  inadequateSecurityError=The website tried to negotiate an inadequate level of security.
> +blockedByPolicy=This page has been blocked.

By what?
Attachment #8941602 - Flags: review-
Comment on attachment 8941602 [details]
Bug 1421707 - Implement a system for disabling about: pages via Policy

https://reviewboard.mozilla.org/r/211864/#review218048

> Might as well give more info about what it has been blocked by?

My intention is to get someone involved from copywriting to suggest the final strings for the error page, but I will change the string anyways to make it a bit more informative.
Attachment #8941602 - Flags: review?(bzbarsky)
Attachment #8941602 - Flags: review?(bzbarsky)
Comment on attachment 8941602 [details]
Bug 1421707 - Implement a system for disabling about: pages via Policy

https://reviewboard.mozilla.org/r/211864/#review218316

::: netwerk/protocol/about/nsAboutProtocolHandler.cpp:198
(Diff revision 4)
> +                nsAutoCString normalizedURL;
> +                normalizedURL.AssignLiteral("about:");
> +                normalizedURL.Append(path);
> +                bool aboutPageAllowed;
> +                rv2 = policyManager->IsAllowed(normalizedURL, &aboutPageAllowed);
> +                if (NS_SUCCEEDED(rv2) && !aboutPageAllowed) {

We don't want to fail-closed here?

::: netwerk/protocol/about/nsAboutProtocolHandler.cpp:199
(Diff revision 4)
> +                normalizedURL.AssignLiteral("about:");
> +                normalizedURL.Append(path);
> +                bool aboutPageAllowed;
> +                rv2 = policyManager->IsAllowed(normalizedURL, &aboutPageAllowed);
> +                if (NS_SUCCEEDED(rv2) && !aboutPageAllowed) {
> +                    rv = NS_ERROR_BLOCKED_BY_POLICY;

This is a bit different from creating and then canceling the channel.  I would vaguely prefer we did this via the "create and cancel" approach, which would let us do more of this async later if desired without changing observables.
Attachment #8941602 - Flags: review?(bzbarsky) → review+
@bz Could you just take a quick look at the way that I changed |nsAboutProtocolHandler::NewChannel2| and let me know if it looks ok?
Flags: needinfo?(bzbarsky)
Comment on attachment 8941602 [details]
Bug 1421707 - Implement a system for disabling about: pages via Policy

https://reviewboard.mozilla.org/r/211864/#review219044
Those changes look good, yes.
Flags: needinfo?(bzbarsky)
So when we were talking about error text before, Felipe suggested that we get someone involved from copywriting to suggest the final strings for the error page. I would agree with this as the strings that I wrote are a bit rough. I haven't needed to request this sort of help before though, so I have some questions:

Since this patch is ready, is now the time to have someone write these strings?

How do I go about requesting help with this? Is there someone in particular that I should ask?
Flags: needinfo?(felipc)
Flags: needinfo?(ddurst)
It sounds like we may be getting a UX person assigned to this project soon that would be able to address these issues.
Flags: needinfo?(felipc)
Flags: needinfo?(ddurst)
Comment on attachment 8941602 [details]
Bug 1421707 - Implement a system for disabling about: pages via Policy

https://reviewboard.mozilla.org/r/211864/#review221110


Static analysis found 2 defects in this patch.
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: dom/browser-element/BrowserElementChildPreload.js:1517
(Diff revision 6)
>              return;
>            case Cr.NS_ERROR_CORRUPTED_CONTENT :
>              sendAsyncMsg('error', { type: 'corruptedContentErrorv2' });
>              return;
> +          case Cr.NS_ERROR_BLOCKED_BY_POLICY :
> +            sendAsyncMsg('error', { type: 'blockedByPolicy' });

Error: Strings must use doublequote. [eslint: quotes]

::: dom/browser-element/BrowserElementChildPreload.js:1517
(Diff revision 6)
>              return;
>            case Cr.NS_ERROR_CORRUPTED_CONTENT :
>              sendAsyncMsg('error', { type: 'corruptedContentErrorv2' });
>              return;
> +          case Cr.NS_ERROR_BLOCKED_BY_POLICY :
> +            sendAsyncMsg('error', { type: 'blockedByPolicy' });

Error: Strings must use doublequote. [eslint: quotes]
The changes requested by :reviewbot above would follow our global style better, but it would not match the surrounding code. I believe that it makes more sense to ignore eslint here and follow the local style.

@bz - As my reviewer for this, can you confirm that this is what I should do? Thanks!
Flags: needinfo?(bzbarsky)
Yes, following file style for the moment seems like the right thing to me.
Flags: needinfo?(bzbarsky)
Hello Amin,

for about pages that are blocked through policies, we're hoping to use the same about:neterror page that is used in various other situations, as we can customize the strings and we don't have to maintain a separate page and styling that has to be updated from time to time.

Here's a screenshot of what it looks like right now as implemented by this patch. We're looking to get feedback on the strings. The warning icon can also be replaced if necessary (e.g. that's where the Dino images go for the offline and DNS error cases)
Attachment #8946379 - Flags: feedback?(aalhazwani)
Blocks: 1429123
Hey Felipe,

I reached out to our content team! They are going to provide the necessary strings while I will try a couple of different icons for the visual aspect of the page!
Attachment #8941602 - Attachment is obsolete: true
Priority: -- → P1
Hi Amin,

This is blocked on UX and strings.  Do you have an ETA?  These Policy fixes need to land ASAP to ride with Fx 60.  Policy is the main blockers for Fx 60 release.
Flags: needinfo?(aalhazwani)
Attachment #8948729 - Flags: review?(felipc)
Attachment #8948729 - Flags: review?(bzbarsky)
Ug, sorry. The updates that I have made to this patch somehow caused it to lose its r+. The changes were purely rebases and changes to names, so there should be no need to spend much time looking at the new version.
Comment on attachment 8948729 [details]
Bug 1421707 - Implement a system for disabling about: pages via Policy

https://reviewboard.mozilla.org/r/218118/#review225898
Attachment #8948729 - Flags: review?(bzbarsky) → review+
Comment on attachment 8948729 [details]
Bug 1421707 - Implement a system for disabling about: pages via Policy

https://reviewboard.mozilla.org/r/218118/#review225906
Attachment #8948729 - Flags: review?(felipc) → review+
Comment on attachment 8946379 [details]
Blocked about config.png

Hey Felipe, sorry for the late response but I've been waiting for a feedback from the content team. The page looks good, using the already existing styles of about: pages is a smart solution. The only minor thing that I would suggest is to replace the icon with the forbidden icon that you can find here https://design.firefox.com/icons/viewer/#forbidden. The fill color of the icon should be Grey 90 a80 https://github.com/FirefoxUX/design-tokens/blob/master/photon-colors/photon-colors.css#L70

Moreover if possible I would add a "Learn more" link that points to a SUMO (or other) resource where we give more information around policies and enterprise.

Nice to have, again considering resources and time, would also be a "Contact Admin..." button that allows ESR users to directly contact their policy manager administrator.

More details here https://mozilla.invisionapp.com/share/QWFUIA593BH#/279275422_Enterprise_Edition (at the bottom)

I know that Brian Jones would like to let administrator customize the texts but we would need anyway default texts. Needinfo him to provide the default title,  description and button label.
Flags: needinfo?(aalhazwani) → needinfo?(brjones)
Attachment #8946379 - Flags: feedback?(aalhazwani)
Hey all - discussed this with Kirk, we are landing the original blocked page [1] first. Then we'll update text & icons in a follow up, and also look at the Learn more and admin button requests. Motivation here is to get this functionality landed so we can start testing with it. We already have reviews so the work here can land now.
(In reply to Jim Mathies [:jimm] from comment #37)
> Hey all - discussed this with Kirk, we are landing the original blocked page
> [1] first. Then we'll update text & icons in a follow up, and also look at
> the Learn more and admin button requests. Motivation here is to get this
> functionality landed so we can start testing with it. We already have
> reviews so the work here can land now.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8946379
Blocks: 1438243
Moved needinfo to bug 1438243
Flags: needinfo?(brjones)
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s b8821db4b1430a92c99b78660e3e7d875d7eed6b -d 7080e442652d: rebasing 447347:b8821db4b143 "Bug 1421707 - Implement a system for disabling about: pages via Policy r=bz,Felipe" (tip)
merging browser/components/enterprisepolicies/Policies.jsm
merging browser/components/enterprisepolicies/schemas/policies-schema.json
merging browser/components/enterprisepolicies/tests/browser/browser.ini
merging docshell/base/nsDocShell.cpp
merging dom/browser-element/BrowserElementChildPreload.js
warning: conflicts while merging browser/components/enterprisepolicies/tests/browser/browser.ini! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/769fbce64db8
Implement a system for disabling about: pages via Policy r=bz,Felipe
https://hg.mozilla.org/mozilla-central/rev/769fbce64db8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
We tested this disabling ‘about:config’ policy using JSON file. We verified this manually as fixed.  
When "BlockAboutConfig" policy is in use, about:config becomes blocked. It also provides information to the user when blocked.

Test steps and runs are available here: https://testrail.stage.mozaws.net/index.php?/plans/view/7734

The bug will also be retested with ADMX files when it is ready for testing.
(In reply to Amin Al Hazwani [:amin] (Firefox UX) from comment #36)
> Comment on attachment 8946379 [details]
> Blocked about config.png
> 
> Hey Felipe, sorry for the late response but I've been waiting for a feedback
> from the content team. The page looks good, using the already existing
> styles of about: pages is a smart solution. The only minor thing that I
> would suggest is to replace the icon with the forbidden icon that you can
> find here https://design.firefox.com/icons/viewer/#forbidden. The fill color
> of the icon should be Grey 90 a80
> https://github.com/FirefoxUX/design-tokens/blob/master/photon-colors/photon-
> colors.css#L70
> 
> Moreover if possible I would add a "Learn more" link that points to a SUMO
> (or other) resource where we give more information around policies and
> enterprise.
> 
> Nice to have, again considering resources and time, would also be a "Contact
> Admin..." button that allows ESR users to directly contact their policy
> manager administrator.
> 
> More details here
> https://mozilla.invisionapp.com/share/QWFUIA593BH#/
> 279275422_Enterprise_Edition (at the bottom)
> 
> I know that Brian Jones would like to let administrator customize the texts
> but we would need anyway default texts. Needinfo him to provide the default
> title,  description and button label.

Amin, I think our wires are crossed. I've been commenting in the Google doc. I'm busy for the next 2 hours, but can insert strings here later today.
(In reply to Amin Al Hazwani [:amin] (Firefox UX) from comment #36)
> Comment on attachment 8946379 [details]
> Blocked about config.png
> 
> Hey Felipe, sorry for the late response but I've been waiting for a feedback
> from the content team. The page looks good, using the already existing
> styles of about: pages is a smart solution. The only minor thing that I
> would suggest is to replace the icon with the forbidden icon that you can
> find here https://design.firefox.com/icons/viewer/#forbidden. The fill color
> of the icon should be Grey 90 a80
> https://github.com/FirefoxUX/design-tokens/blob/master/photon-colors/photon-
> colors.css#L70
> 
> Moreover if possible I would add a "Learn more" link that points to a SUMO
> (or other) resource where we give more information around policies and
> enterprise.
> 
> Nice to have, again considering resources and time, would also be a "Contact
> Admin..." button that allows ESR users to directly contact their policy
> manager administrator.
> 
> More details here
> https://mozilla.invisionapp.com/share/QWFUIA593BH#/
> 279275422_Enterprise_Edition (at the bottom)
> 
> I know that Brian Jones would like to let administrator customize the texts
> but we would need anyway default texts. Needinfo him to provide the default
> title,  description and button label.

The string would be:
Blocked Page
Your organization has blocked access to this page or website.

I don't think a link to a SUMO page makes sense. We can't know/predict all the reasons a company or school or whatever might block access. We also can't offer any meaningful help or remediation and we shouldn't be in the business of speaking for a company's sys admins. Don't think we should include a "Contact Admin" button either (unless it's turned off and invisible by default). It's not up to us to give users a direct route to an organization's IT folks. Those are reasons I like the idea of letting the org customize a message, with nothing but the generic blocked access message visible by default.
Thanks Brian. And yes, as you underlined the button "Contact Admin" is opt in.
After thinking more about it, I don't think a "Contact Admin" button makes sense. We seem to be assuming the sys admin is the person making the decision about what is blocked and what isn't. In cases where an organization uses a "generic" white/blacklist, that might be true. Bur other organizations customize access, and it isn't likely the sys admin makes the decision about what is allowed and what isn't. This page is fundamentally different than other in-product messaging opportunities. We're not in a position to offer guidance or remediation; we're simply reflecting an organization's decision about a specific page. Thanks.
We retested this on beta builds[FX60] with ADM and JSON policy formats and it is verified as fixed.

Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760
Status: RESOLVED → VERIFIED
status-firefox60: fixed → verified
You need to log in before you can comment on or make changes to this bug.