Closed Bug 1429177 Opened 4 years ago Closed 3 years ago

Policy: Set network proxy settings (and lock them)

Categories

(Firefox :: Enterprise Policies, defect, P1)

Product:
Firefox
Component:
Enterprise Policies
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox59 --- wontfix
firefox60 + verified
firefox61 --- verified

People

(Reporter: Felipe, Assigned: Felipe)

References

Details

Attachments

(1 file)

Bug 1429177 - Policy: Set network proxy settings.
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)
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: nobody → felipc
Status: NEW → ASSIGNED
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+
(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.
(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
(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)
[Tracking Requested - why for this release]:
Enterprise Policies for ESR
status-firefox59: affected → wontfix
status-firefox60: --- → affected
tracking-firefox60: --- → ?
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)
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
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea1065832cf1
Policy: Set network proxy settings. r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/ea1065832cf1
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
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?
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
Blocks: 1446508
(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)
(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 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+
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
status-firefox61: fixed → verified
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
Depends on: 1455827
You need to log in before you can comment on or make changes to this bug.