Closed
Bug 1421707
Opened 6 years ago
Closed 6 years ago
Implement a system for disabling about: pages via Policy
Categories
(Firefox :: Enterprise Policies, enhancement, P1)
Firefox
Enterprise Policies
Tracking
()
VERIFIED
FIXED
Firefox 60
Tracking | Status | |
---|---|---|
firefox60 | --- | verified |
People
(Reporter: bytesized, Assigned: bytesized)
References
Details
Attachments
(3 files, 1 obsolete file)
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
Updated•6 years ago
|
Group: mozilla-employee-confidential
Component: General → Enterprise Policies
Updated•6 years ago
|
Blocks: policies-mvp
Comment hidden (mozreview-request) |
Assignee | ||
Comment 3•6 years ago
|
||
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);|.
Assignee | ||
Comment 4•6 years ago
|
||
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.
Assignee | ||
Comment 5•6 years ago
|
||
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)
Assignee | ||
Comment 6•6 years ago
|
||
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.
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8941602 -
Flags: review?(felipc)
Assignee | ||
Comment 8•6 years ago
|
||
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.
Comment 9•6 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 10•6 years ago
|
||
mozreview-review-reply |
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.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•6 years ago
|
||
@bz I had a hard time picking a reviewer for this patch. Let me know if you would prefer that I ask someone else.
Assignee | ||
Updated•6 years ago
|
Attachment #8941602 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•6 years ago
|
Attachment #8941602 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 13•6 years ago
|
||
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 14•6 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Assignee | ||
Comment 16•6 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Updated•6 years ago
|
Attachment #8941602 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•6 years ago
|
Attachment #8941602 -
Flags: review?(bzbarsky)
Comment 17•6 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Assignee | ||
Comment 19•6 years ago
|
||
@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 20•6 years ago
|
||
mozreview-review |
Comment on attachment 8941602 [details] Bug 1421707 - Implement a system for disabling about: pages via Policy https://reviewboard.mozilla.org/r/211864/#review219044
Assignee | ||
Comment 22•6 years ago
|
||
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)
Assignee | ||
Comment 23•6 years ago
|
||
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 hidden (mozreview-request) |
Comment 25•6 years ago
|
||
mozreview-review |
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]
Assignee | ||
Comment 26•6 years ago
|
||
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)
Comment 27•6 years ago
|
||
Yes, following file style for the moment seems like the right thing to me.
Flags: needinfo?(bzbarsky)
Comment 28•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment 30•6 years ago
|
||
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!
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Attachment #8941602 -
Attachment is obsolete: true
Updated•6 years ago
|
Priority: -- → P1
Comment 32•6 years ago
|
||
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)
Assignee | ||
Updated•6 years ago
|
Attachment #8948729 -
Flags: review?(felipc)
Attachment #8948729 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 33•6 years ago
|
||
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 34•6 years ago
|
||
mozreview-review |
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 35•6 years ago
|
||
mozreview-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 36•6 years ago
|
||
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)
Comment 37•6 years ago
|
||
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.
Comment 38•6 years ago
|
||
(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
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 42•6 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment 44•6 years ago
|
||
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
Comment 45•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/769fbce64db8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Comment 46•6 years ago
|
||
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.
Comment 47•6 years ago
|
||
(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.
Comment 48•6 years ago
|
||
(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.
Comment 49•6 years ago
|
||
Thanks Brian. And yes, as you underlined the button "Contact Admin" is opt in.
Comment 50•6 years ago
|
||
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.
Comment 51•6 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•