1440578 - Policy: "block" under "cookies" should clear already-stored cookies

Status: RESOLVED FIXED

(Firefox :: Enterprise Policies, enhancement)

Description

[YUKI "Piro" Hiroshi]

"cookies" allows to specify policy for newly stored cookies, but already stored cookies are out of control. When the policy is updated to block cookies, already stored cookies should be cleared to avoid problems caused by old values.

Comment 1

[YUKI "Piro" Hiroshi]

Attachment: Clear stored cookies for cookie blocked hosts (WIP)

I'm trying to implement this behavior but the automated test still fails. Is any missing part for the test?

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8956029 [details] [diff] [review]
Clear stored cookies for cookie blocked hosts (WIP)

I didn't run it myself, so I don't know why the test is failing. How is JSON.stringify(cookies) and JSON.stringify(expected) differing?

Can you use the function removeCookiesWithOriginAttributes to avoid the manual iteration?

Also, you should wrap the newly added block to Policies.jsm with the runOnce helper function because we don't want to be doing that on every startup because that would be a performance hit

Comment 3

[:Felipe Gomes (needinfo for replies!)]

(In reply to :Felipe Gomes (needinfo me!) from comment #2)

> Can you use the function removeCookiesWithOriginAttributes to avoid the
> manual iteration?

Looks like you can, by calling Services.cookies.removeCookiesWithOriginAttributes("{}", host);

note: each element of the param.Block array is already an nsIURI object, so you don't need to do a newURI call on it

Comment 4

[YUKI "Piro" Hiroshi]

(In reply to :Felipe Gomes (needinfo me!) from comment #3)
> Looks like you can, by calling
> Services.cookies.removeCookiesWithOriginAttributes("{}", host);
> 
> note: each element of the param.Block array is already an nsIURI object, so
> you don't need to do a newURI call on it

Thank you for reviewing and advises! I misunderstood the spec of param.Block. After some changes the test succeeded as expected.

Comment 5

[YUKI "Piro" Hiroshi]

Attachment: Clear stored cookies for cookie blocked hosts

Finalized patch.

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8956318 [details] [diff] [review]
Clear stored cookies for cookie blocked hosts

Review of attachment 8956318 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks!

I'll just double check with jdm that this is the correct usage for this removeCookiesWithOriginAttributes function.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5521f4a9012d
Policy: "block" under "cookies" should clear already-stored cookies. r=felipc, r=jdm

Comment 8

[Cosmin Sabou [:CosminS]]

Backed out for ESlint failure on browser/components/enterprisepolicies/Policies.jsm

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5521f4a9012d88d6aa399bc16b27ec0013976526&filter-searchStr=bf1813c891520f2ba58ac0e5c348b469a6fe7762&selectedJob=166672000

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=166672000&repo=mozilla-inbound&lineNumber=241

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/b18dbffb418d9d165707e4a18c9da952d5992f4a

Comment 9

[Cosmin Sabou [:CosminS]]

Also please take a look at these failures: https://treeherder.mozilla.org/logviewer.html#?job_id=166673261&repo=mozilla-inbound&lineNumber=2003

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5521f4a9012d88d6aa399bc16b27ec0013976526&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=166673261&filter-searchStr=tv

Comment 10

[YUKI "Piro" Hiroshi]

(In reply to Cosmin Sabou [:cosmin_s] from comment #9)
> Also please take a look at these failures:
> https://treeherder.mozilla.org/logviewer.html#?job_id=166673261&repo=mozilla-
> inbound&lineNumber=2003
> 
> https://treeherder.mozilla.org/#/jobs?repo=mozilla-
> inbound&revision=5521f4a9012d88d6aa399bc16b27ec0013976526&filter-
> resultStatus=testfailed&filter-resultStatus=busted&filter-
> resultStatus=exception&selectedJob=166673261&filter-searchStr=tv

Except eslint errors, I couldn't reproduce the failure with a command line:

----
$ ./mach test browser/components/enterprisepolicies/tests/br
owser/browser_policy_blocked_cookies.js
----

I still cannot find out the reason why it succeeds on my environment but fails on CI...

Comment 11

[YUKI "Piro" Hiroshi]

Attachment: Clear stored cookies for cookie blocked hosts v2

This just fixes eslint erros.

Comment 12

[Tooru Fujisawa [:arai]]

to reproduce TV failure locally, you need to specify --verify after "test".

then, the issue is that runOncePerModification function early-returns because the value is same as previous one, 
that is stored in browser.policies.runOncePerModification..clearCookiesForBlockedHosts.
So, you need to clear the pref in teardown.

Comment 13

[Tooru Fujisawa [:arai]]

(In reply to Tooru Fujisawa [:arai] from comment #12)
> So, you need to clear the pref in teardown.

sorry, this wasn't proper fix.
iiuc the same issue can happen outside of test.
So, the pref should be cleared when "Cookies" or "Block" isn't present for newly set policy.
so that it will be cleared by setupPolicyEngineWithJson("") in testcase

Comment 14

[YUKI "Piro" Hiroshi]

(In reply to Tooru Fujisawa [:arai] from comment #12)
> to reproduce TV failure locally, you need to specify --verify after "test".
> 
> then, the issue is that runOncePerModification function early-returns
> because the value is same as previous one, 
> that is stored in
> browser.policies.runOncePerModification..clearCookiesForBlockedHosts.
> So, you need to clear the pref in teardown.

Thanks a lot! Now I'm updating my patch with clearing of "runOnce" state.

Comment 15

[YUKI "Piro" Hiroshi]

Attachment: Clear stored cookies for cookie blocked hosts v2.1

Update patch. Changes from v1:

 * fix eslint errors.
 * clear "runOnce" state for every test.

Comment 16

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8957217 [details] [diff] [review]
Clear stored cookies for cookie blocked hosts v2.1

>diff --git a/browser/components/enterprisepolicies/Policies.jsm b/browser/components/enterprisepolicies/Policies.jsm
>+
>+  resetRunOnceState: function resetRunOnceState() {
>+    const runOnceBaseKeys = [
>+      "browser.policies.runonce.",
>+      "browser.policies.runOncePerModification."
>+    ];
>+    for (let base of runOnceBaseKeys) {
>+      for (let key of Services.prefs.getChildList(base, {})) {
>+        if (Services.prefs.prefHasUserValue(key))
>+          Services.prefs.clearUserPref(key);
>+      }
>+    }
>+  },

Thanks for doing this!!
Instead of calling it directly from your test, can you call resetRunOnceState from the cleanup function in head.js?
https://searchfox.org/mozilla-central/source/browser/components/enterprisepolicies/tests/browser/head.js#24

Comment 17

[YUKI "Piro" Hiroshi]

(In reply to :Felipe Gomes (needinfo me!) from comment #16)
> Instead of calling it directly from your test, can you call
> resetRunOnceState from the cleanup function in head.js?
> https://searchfox.org/mozilla-central/source/browser/components/
> enterprisepolicies/tests/browser/head.js#24

Yes, I tried that, but it seems to break the test for SetHomepage policy, so I couldn't decide that it should be done in head.js. I need more research for it...

Comment 18

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8957217 [details] [diff] [review]
Clear stored cookies for cookie blocked hosts v2.1

Review of attachment 8957217 [details] [diff] [review]:
-----------------------------------------------------------------

Ok! So let's use this version for now. But if you find the reason for that breakage of the other test, let me know

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7eae9d89985b
Policy: "block" under "cookies" should clear already-stored cookies r=felipc

Comment 20

[YUKI "Piro" Hiroshi]

Attachment: clear "runOnce" state for every test

(In reply to :Felipe Gomes (needinfo me!) from comment #18)
> Ok! So let's use this version for now. But if you find the reason for that
> breakage of the other test, let me know

Finally I found the cause why the test is broken. This is a patch to fix the breakage. I think it should be applied after the current v2.1 patch is land.

Comment 21

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/7eae9d89985b

Comment 22

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8957601 [details] [diff] [review]
clear "runOnce" state for every test

(marking myself for review so I don't forget about it)

Comment 23

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8957601 [details] [diff] [review]
clear "runOnce" state for every test

Review of attachment 8957601 [details] [diff] [review]:
-----------------------------------------------------------------

(I moved this patch to bug 1446508 and gave r+ there)