Closed
Bug 1429177
Opened 5 years ago
Closed 5 years ago
Policy: Set network proxy settings (and lock them)
Categories
(Firefox :: Enterprise Policies, defect, P1)
Firefox
Enterprise Policies
Tracking
()
VERIFIED
FIXED
Firefox 61
People
(Reporter: Felipe, Assigned: Felipe)
References
Details
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
mixedpuppy
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
This is probably one of the most complicated policies, at least schema-wise. We need to see what are all the settings needed to be controlled by sysadmins, and how to expose them through the policies.json file. From early investigation: - network.proxy.* - Some extra config may be necessary for local proxy autoconfig (https://github.com/mkaply/cck2wizard/blob/54c388ceb2d1a749d63362b1eabf4a675793aafd/content/finish.js#L479-L499)
Comment 1•5 years ago
|
||
Freddy pointed us to Patrick McManus or Jason Duell for network proxy settings expertise. "Once we have an architecture draft and an initial patch, we're happy to provide some time-boxed security/code reviews for the policies you find most crucial to the success of the project. Just let us know."
Assignee | ||
Updated•5 years ago
|
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Comment hidden (mozreview-request) |
Comment 3•5 years ago
|
||
mozreview-review |
Comment on attachment 8959325 [details] Bug 1429177 - Policy: Set network proxy settings. https://reviewboard.mozilla.org/r/228164/#review234236 Are policies not able to set a default but allow override? Seems like a limitation. just nits, but my one concern is the comment in connection.js. r+ with verifying/explaining that change is ok ::: browser/components/enterprisepolicies/helpers/ProxyPolicies.jsm:93 (Diff revision 1) > + setProxyHostAndPort("http", param.HTTPProxy); > + > + // network.proxy.share_proxy_settings is a UI feature, not handled by the > + // network code. That pref only controls if the checkbox is checked, and > + // then we must manually set the other values. > + if (param.UseHTTPProxyForAllProtocols) { likewise, it seems like the settings after this block should not be done if this flag is set. not sure it matters, but it does seem wrong. test_proxy_addresses indicates that the later params would not get set, but it's non-obvious here. ::: browser/components/enterprisepolicies/schemas/policies-schema.json:268 (Diff revision 1) > > + "Proxy": { > + "description": "Configure Proxy settings.", > + "first_available": "60.0", > + > + "type": "object", Would it be ok to make this match the proxyConfig object? I'm not sure how the policies schema's are hooked up, whether you can use "$ref": "browserSettings.ProxyConfig". Hrm. that wouldn't be consistent with other policies schema entries. :( ::: browser/components/preferences/connection.js:264 (Diff revision 1) > function setInputsDisabledState(isControlled) { > let disabled = isLocked || isControlled; > for (let element of gConnectionsDialog.getProxyControls()) { > element.disabled = disabled; > } > - if (!isControlled) { > + if (!isLocked) { shouldn't this be !disabled ? It looks like this used to be prevented if an extension was controlling the pref, but with this change it appears that is no longer the case.
Attachment #8959325 -
Flags: review?(mixedpuppy) → review+
Assignee | ||
Comment 4•5 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #3) > Comment on attachment 8959325 [details] > Bug 1429177 - Policy: Set network proxy settings. > > https://reviewboard.mozilla.org/r/228164/#review234236 > > Are policies not able to set a default but allow override? Seems like a > limitation. Some policies do that, but it depends on the policy. And some offer both options (locked and unlocked). But the unlocked ones are more about setting UI customizations (showing menubar/toolbar etc). It's more work to support both styles, so I'll leave that for the future if people ask for it. I imagine most sysadmins would prefer to have this locked so that users don't mess with it. > ::: browser/components/enterprisepolicies/helpers/ProxyPolicies.jsm:93 > (Diff revision 1) > > + setProxyHostAndPort("http", param.HTTPProxy); > > + > > + // network.proxy.share_proxy_settings is a UI feature, not handled by the > > + // network code. That pref only controls if the checkbox is checked, and > > + // then we must manually set the other values. > > + if (param.UseHTTPProxyForAllProtocols) { > > likewise, it seems like the settings after this block should not be done if > this flag is set. not sure it matters, but it does seem wrong. > test_proxy_addresses indicates that the later params would not get set, but > it's non-obvious here. This is on purpose. From my understanding of the connection.js code, I believe the webextensions code is not working properly with the httpProxyAll option. The checkbox is only used by UI: when it's clicked, it fills the other proxies with the same information (which is the same that this code is doing). The pref is only used by the UI to memorize that the checkbox was checked. > > ::: browser/components/enterprisepolicies/schemas/policies-schema.json:268 > (Diff revision 1) > > > > + "Proxy": { > > + "description": "Configure Proxy settings.", > > + "first_available": "60.0", > > + > > + "type": "object", > > Would it be ok to make this match the proxyConfig object? I'm not sure how > the policies schema's are hooked up, whether you can use "$ref": > "browserSettings.ProxyConfig". Hrm. that wouldn't be consistent with other > policies schema entries. :( Yeah, the Policies validator uses some different code, which doesn't support the $ref. I couldn't match the parameters letter-by-letter because the standard in the Windows GPO world is to have things starting with a capital letter. So, due to this, I ended up choosing slightly more descriptive names. > > ::: browser/components/preferences/connection.js:264 > (Diff revision 1) > > function setInputsDisabledState(isControlled) { > > let disabled = isLocked || isControlled; > > for (let element of gConnectionsDialog.getProxyControls()) { > > element.disabled = disabled; > > } > > - if (!isControlled) { > > + if (!isLocked) { > > shouldn't this be !disabled ? It looks like this used to be prevented if an > extension was controlling the pref, but with this change it appears that is > no longer the case. That's a good point. In practice it can't be controlled and not locked, so the end result is the same, but !disabled is indeed more semantically correct. I'll make this change.
Comment 5•5 years ago
|
||
(In reply to :Felipe Gomes (needinfo me!) from comment #4) > (In reply to Shane Caraveo (:mixedpuppy) from comment #3) > > Comment on attachment 8959325 [details] > > Bug 1429177 - Policy: Set network proxy settings. > > > > https://reviewboard.mozilla.org/r/228164/#review234236 > > > > Are policies not able to set a default but allow override? Seems like a > > limitation. > > Some policies do that, but it depends on the policy. And some offer both > options (locked and unlocked). But the unlocked ones are more about setting > UI customizations (showing menubar/toolbar etc). It's more work to support > both styles, so I'll leave that for the future if people ask for it. I > imagine most sysadmins would prefer to have this locked so that users don't > mess with it. We've received a lot of commentary on proxy api, and it does seem a lot of organizations rely on proxy extensions to do stuff. Whether they would want to combine locked down prefs with that is another question, but they might want to provide default prefs for these settings. Can you post a followup bug and ni mkaply and ericjung about whether this extra work is worthwhile?
Flags: needinfo?(felipc)
Pushed by felipc@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6663b286d68c Policy: Set network proxy settings. r=mixedpuppy
Assignee | ||
Comment 7•5 years ago
|
||
(In reply to Shane Caraveo (:mixedpuppy) from comment #5) > We've received a lot of commentary on proxy api, and it does seem a lot of > organizations rely on proxy extensions to do stuff. Whether they would want > to combine locked down prefs with that is another question, but they might > want to provide default prefs for these settings. Can you post a followup > bug and ni mkaply and ericjung about whether this extra work is worthwhile? Ok, it was much simpler than I imagined, so I ended up supporting the non-locked version in this bug. As I was already passing the custom setPref function as parameter, it was just a matter of passing one that doesn't lock the pref. Mkaply was landing such a function with bug 1433870, so I just used that. It was a change of just a couple of lines, so I carried over the r+.
Flags: needinfo?(felipc)
Assignee | ||
Comment 8•5 years ago
|
||
[Tracking Requested - why for this release]: Enterprise Policies for ESR
Comment 9•5 years ago
|
||
Backed out changeset 6663b286d68c (bug 1429177) for Mochitest leaks on ProxyAutoConfig Log of the fail: https://treeherder.mozilla.org/logviewer.html#?job_id=168665272&repo=mozilla-inbound&lineNumber=4032 Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/b1120103af20267499acb35dbfa5a6faf4ffd8c4 Push that got backed out: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6663b286d68c09979af181f3ca9d779c89b271f2
Flags: needinfo?(felipc)
Comment hidden (Intermittent Failures Robot) |
Assignee | ||
Comment 11•5 years ago
|
||
The backout was due to a leak of a ProxyAutoConfig object. I imagine it's because the test left the proxy mode on autoconfig, so I'm gonna make it reset to the original setting. However, it's not clear that that will fix the problem: it might be that just passing through the autoconfig mode triggers this leak. In both cases, that's probably a legit leak (unrelated to this code). I'll see how to avoid it in the thest and if a bug for it needs to be filed.
Flags: needinfo?(felipc)
Assignee | ||
Comment 12•5 years ago
|
||
So the test harness itself changes some of the proxy parameters (type and autoconfig url), so messing with them during tests causes problems. I changed the test to not directly test the API exposed by ProxyPolicies.jsm to avoid doing a real pref change. Tests are green (https://treeherder.mozilla.org/#/jobs?repo=try&revision=f1170c2a09d15891340202e6106e2c1bc58f38da&selectedJob=168874599), so I'm gonna reland
Comment 13•5 years ago
|
||
Pushed by felipc@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea1065832cf1 Policy: Set network proxy settings. r=mixedpuppy
Comment 14•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/ea1065832cf1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Updated•5 years ago
|
Assignee | ||
Comment 15•5 years ago
|
||
Comment on attachment 8959325 [details] Bug 1429177 - Policy: Set network proxy settings. Approval Request Comment [Feature/Bug causing the regression]: Policy: proxy settings [User impact if declined]: no policy to configure proxy [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: QA is finishing verification [Needs manual test from QE? If yes, steps to reproduce]: - [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: limited to the use of this policy [String changes made/needed]: none
Attachment #8959325 -
Flags: approval-mozilla-beta?
Comment 16•5 years ago
|
||
We tested this on nightly with JSON policy format and it is verified as fixed. Firefox network proxy can be set by this policy. We will retest with admx format when available. Test cases and runs are avaliable here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760
Comment 17•5 years ago
|
||
(In reply to :Felipe Gomes (needinfo me!) from comment #4) > > ::: browser/components/preferences/connection.js:264 > > (Diff revision 1) > > > function setInputsDisabledState(isControlled) { > > > let disabled = isLocked || isControlled; > > > for (let element of gConnectionsDialog.getProxyControls()) { > > > element.disabled = disabled; > > > } > > > - if (!isControlled) { > > > + if (!isLocked) { > > > > shouldn't this be !disabled ? It looks like this used to be prevented if an > > extension was controlling the pref, but with this change it appears that is > > no longer the case. > > That's a good point. In practice it can't be controlled and not locked, so > the end result is the same, but !disabled is indeed more semantically > correct. I'll make this change. It looks like this landed as "!isLocked" rather than "!disabled" after all?
Flags: needinfo?(felipc)
Assignee | ||
Comment 18•5 years ago
|
||
(In reply to Julien Cristau [:jcristau] from comment #17) > > That's a good point. In practice it can't be controlled and not locked, so > > the end result is the same, but !disabled is indeed more semantically > > correct. I'll make this change. > > It looks like this landed as "!isLocked" rather than "!disabled" after all? Ah. good catch, sorry I didn't notice that. As mentioned in comment 4, they mean the same thing in practice. So, in order to simplify the uplift, I'd prefer to uplift the same version that landed, and file a follow-up bug to change that to !disabled.
Flags: needinfo?(felipc)
Comment 19•5 years ago
|
||
Comment on attachment 8959325 [details] Bug 1429177 - Policy: Set network proxy settings. thanks, let's get this one on beta then.
Attachment #8959325 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 20•5 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/8df92d67493b
Flags: in-testsuite+
Comment 21•4 years ago
|
||
We retest this on Nighty using adm files as well and it is verified as fixed. Test cases and runs are avaliable here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760
Comment 22•4 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
•