Closed Bug 1451024 Opened 6 years ago Closed 6 years ago

Cookies default settings are not greyed-out while lock a policy is used

Categories

(Firefox :: Enterprise Policies, defect)

61 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 61
Tracking Status
firefox60 --- fixed
firefox61 --- verified

People

(Reporter: Abe_LV, Assigned: bytesized)

References

Details

Attachments

(1 file, 1 obsolete file)

Steps to reproduce:
Screen capture- https://testing-1.tinytake.com/sf/MjQ4ODA1Nl83NDkyNjE0

1. Use this policy:
{
  "policies": {
    "Cookies": {
      "Default": true,
      "ExpireAtSessionEnd": false,
      "AcceptThirdParty": "all",
      "Locked": true
    }
  }
}

2. Go to about:preferences#privacy -> Cookies and Site Data
3. Check if "Keep until" and "Accept third-party cookie and site data" options are greyed-out.

Actual result:
They look active but are not changeable.

Expected result:
Locked options should be greyed out

Note- this is not a blocking bug to the actual implementation. It is a nice to have for future developments or followups.
Flags: needinfo?(felipc)
Blocks: 1429169
No longer blocks: 1429178
Flags: needinfo?(felipc) → needinfo?(ksteuber)
Assignee: nobody → ksteuber
Flags: needinfo?(ksteuber)
Yeah, this looks like a preference bug. Those are weird.
Attachment #8965461 - Flags: review?(jaws)
Comment on attachment 8965461 [details]
Bug 1451024 - Cookie settings should be disabled when the related prefs are locked

https://reviewboard.mozilla.org/r/234210/#review239834

::: browser/components/preferences/in-content/privacy.js:857
(Diff revision 1)
> -    acceptThirdPartyLabel.disabled = acceptThirdPartyMenu.disabled = !acceptCookies;
> +    if (!acceptThirdPartyMenu.disabled) {
> +      acceptThirdPartyMenu.disabled = !acceptCookies;
> +    }

Once this menu gets disabled, how does it ever get enabled again?
Comment on attachment 8965461 [details]
Bug 1451024 - Cookie settings should be disabled when the related prefs are locked

https://reviewboard.mozilla.org/r/234210/#review239834

> Once this menu gets disabled, how does it ever get enabled again?

As discussed on IRC, locking and unlocking prefs is not instantly synced with about:preferences the way that changes to the pref values are. The menu should not need to be enabled once disabled.
Comment on attachment 8965461 [details]
Bug 1451024 - Cookie settings should be disabled when the related prefs are locked

https://reviewboard.mozilla.org/r/234210/#review240724
Attachment #8965461 - Flags: review?(jaws) → review+
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0d049666e764
Cookie settings should be disabled when the related prefs are locked r=jaws
Backed out changeset 0d049666e764 (bug 1451024) for browser chrome failures on browser_privacypane_1.js

Backout: https://hg.mozilla.org/integration/autoland/rev/085c62e04558eea099732b66fbbde71625cbb772

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0d049666e764901e4e7a6ec915caa74f59bcc477&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-resultStatus=pending&filter-resultStatus=running&selectedJob=172733705

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=172733705&repo=autoland&lineNumber=6476

[task 2018-04-09T20:23:53.705Z] 20:23:53     INFO - TEST-PASS | browser/components/preferences/in-content/tests/browser_privacypane_1.js | keepCookiesUntil should be disabled - 
[task 2018-04-09T20:23:53.706Z] 20:23:53     INFO - Buffered messages finished
[task 2018-04-09T20:23:53.708Z] 20:23:53     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_privacypane_1.js | acceptThirdPartyLabel should not be disabled - Got true, expected false
[task 2018-04-09T20:23:53.709Z] 20:23:53     INFO - Stack trace:
[task 2018-04-09T20:23:53.710Z] 20:23:53     INFO - chrome://mochikit/content/browser-test.js:test_is:1280
[task 2018-04-09T20:23:53.712Z] 20:23:53     INFO - chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/privacypane_tests_perwindow.js:test_dependent_cookie_elements/expect_disabled/<:133
[task 2018-04-09T20:23:53.713Z] 20:23:53     INFO - chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/privacypane_tests_perwindow.js:expect_disabled:132
[task 2018-04-09T20:23:53.715Z] 20:23:53     INFO - chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/privacypane_tests_perwindow.js:test_dependent_cookie_elements:144
[task 2018-04-09T20:23:53.716Z] 20:23:53     INFO - chrome://mochitests/content/browser/browser/components/preferences/in-content/tests/privacypane_tests_perwindow.js:runTestOnPrivacyPrefPane:11
[task 2018-04-09T20:23:53.717Z] 20:23:53     INFO - Not taking screenshot here: see the one that was previously logged
[task 2018-04-09T20:23:53.719Z] 20:23:53     INFO - TEST-UNEXPECTED-FAIL | browser/components/preferences/in-content/tests/browser_privacypane_1.js | acceptThirdPartyMenu should not be disabled - Got true, expected false
Flags: needinfo?(ksteuber)
I am now able to reproduce this bug. It appears to show up only in debug builds. Working on a fix now.
Flags: needinfo?(ksteuber)
My mistake. I can, in fact, reproduce this in a release build in addition to a debug build. Still working on a fix.
Attachment #8965461 - Attachment is obsolete: true
Attachment #8966770 - Flags: review?(jaws)
jaws, it looks like you were right when you suspected that being unable to re-enable those controls would be a problem. As I said then, it does not result in any problems as far as enterprise policies are concerned, but it looks like it causes problems when cookies are fully disabled, then re-enabled.

This patch should address that. I'm not crazy about basically adding an extra mechanism for disabling controls with locked prefs, but I think that this solution is acceptable.
Comment on attachment 8966770 [details]
Bug 1451024 - Cookie settings should be disabled when the related prefs are locked

https://reviewboard.mozilla.org/r/235454/#review241892

::: browser/components/preferences/in-content/privacy.js:862
(Diff revision 1)
>  
> -    acceptThirdPartyLabel.disabled = acceptThirdPartyMenu.disabled = !acceptCookies;
> +    acceptThirdPartyLabel.disabled = acceptThirdPartyMenu.disabled = controlsDisabled;
>  
>      let privateBrowsing = Preferences.get("browser.privatebrowsing.autostart").value;
> -    keepUntil.disabled = menu.disabled = privateBrowsing || !acceptCookies;
> +    let cookieExpirationLocked = Services.prefs.prefIsLocked("network.cookie.lifetimePolicy");
> +    controlsDisabled = privateBrowsing || !acceptCookies || cookieExpirationLocked;

This changes the meaning of `controlsDisabled`. I'd prefer if you introduce a new variable so future code doesn't have to worry about the potentially changing value of this. Then you can declear `controlsDisabled` as a `const`.
Attachment #8966770 - Flags: review?(jaws) → review+
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/37ef105b62dc
Cookie settings should be disabled when the related prefs are locked r=jaws
https://hg.mozilla.org/mozilla-central/rev/37ef105b62dc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
We tested this on latest nightly with adm and JSON policies and it is verified as fixed.
Settings are now greyed out when the lock option is used in the policy.

Test cases and runs are here- https://testrail.stage.mozaws.net/index.php?/plans/view/8760
Status: RESOLVED → VERIFIED
Comment on attachment 8966770 [details]
Bug 1451024 - Cookie settings should be disabled when the related prefs are locked

Approval Request Comment
[Feature/Bug causing the regression]: Enterprise Policy Engine
[User impact if declined]: When cookie settings are locked, the controls will not correctly appear to be disabled
[Is this code covered by automated tests?]: Included in patch
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Testing being done by Abe
[List of other uplifts needed for the feature/fix]: Related bugs already uplifted.
[Is the change risky?]: Minimal Risk
[Why is the change risky/not risky?]: Changes behavior of about:preferences control to the standard behavior - When related pref is locked, control should be disabled.
[String changes made/needed]: None
Attachment #8966770 - Flags: approval-mozilla-beta?
Comment on attachment 8966770 [details]
Bug 1451024 - Cookie settings should be disabled when the related prefs are locked

fix for looking cookie settings by policy, approved for 60.0b14
Attachment #8966770 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.