Closed Bug 1465950 Opened 6 years ago Closed 6 years ago

Keep the parsed policies in memory so that they can be retrieved by about:policies

Categories

(Firefox :: Enterprise Policies, enhancement, P2)

Product:
Firefox
Component:
Enterprise Policies
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: Felipe, Assigned: kanika16047)

References

Details

Attachments

(1 file)

Kanika Bug 1465950 - Parsed policies stored by EnterprisePolicies.js to display on about:policies page
59 bytes, text/x-review-board-request
Felipe
: review+
Details 
Currently, the EnterprisePolicies code reads the JSON from policies.json and iterates through all of its keys [1], parsing and validating each one, and running the code for each valid policy.

However, after doing this, it doesn't store that anywhere, because it currently doesn't need to. But we'll need to do that in order to implement the about:policies page.

The catch is that we don't want to store the unparsedPolicies, because part of the purpose of about:policies is to let the admin know which policies were actually correctly specified and therefore accepted. So we should only store the validated ones.

To make them retrievable, there are a couple of alternatives. One would be to add a new method in the nsIEnterprisePolicies API [2]. Another would be to do it through an observer

Related, we might also want to store some extra info: what source was used for the policies: Windows GPO or a policies.json file


[1] https://searchfox.org/mozilla-central/rev/83a923ef7a3b95a516f240a6810c20664b1e0ac9/browser/components/enterprisepolicies/EnterprisePolicies.js#115

[2] https://searchfox.org/mozilla-central/source/toolkit/components/enterprisepolicies/nsIEnterprisePolicies.idl
Assignee: nobody → ksaini
My suggestion is to go with the first approach mentioned above: add a new method in nsIEnterprisePolicies.idl [1] called getActivePolicies() that will return an object with the stored policies.

You define this function in the .idl file, and implement it in EnterprisePolicies.js

To return an object, you need to define this method as returning a `jsval` (this is an example: [2])


In the _activatePolicies function, you should store all the parsedParameters objects which is what should be returned by your function when it gets called.



[1] https://searchfox.org/mozilla-central/source/toolkit/components/enterprisepolicies/nsIEnterprisePolicies.idl

[2] https://searchfox.org/mozilla-central/rev/d0a41d2e7770fc00df7844d5f840067cc35ba26f/mobile/android/components/SessionStore.idl#38
Comment on attachment 8986515 [details]
Kanika Bug 1465950 - Parsed policies stored by EnterprisePolicies.js to display on about:policies page

https://reviewboard.mozilla.org/r/251838/#review258308

Yeah, this is looking good. Besides the comments below, there's just one more thing:  There's also a stub implementation of nsIEnterprisePolicies for the content process. It's in the EnterprisePoliciesContent.js file.

That implementation must also implement everything declared in nsIEnterprisePolicies.idl, so you'll need to implement the new fuction there too. But the content process should not be using this function, so what I think we should do is to make the getActivePolicies function just `throw Cr.NS_ERROR_NOT_AVAILABLE` in the stub EnterprisePoliciesContent.js version.

And, of course, it will need tests!

::: browser/components/enterprisepolicies/EnterprisePolicies.js:93
(Diff revision 1)
>        this.status = Ci.nsIEnterprisePolicies.FAILED;
>        return;
>      }
>  
>      this.status = Ci.nsIEnterprisePolicies.ACTIVE;
> +    this._parsedParameters = [];

this object should mimic more or less how the input JSON looks.

so instead of an array, let's make it an object called `_parsedPolicies`, and store each parsedParameter in the loop below, with the property name being the policyName.

::: toolkit/components/enterprisepolicies/nsIEnterprisePolicies.idl:18
(Diff revision 1)
>    const short FAILED        = 2;
>  
>    readonly attribute short status;
>  
>    bool isAllowed(in ACString feature);
> +  jsval getActivePolicies();

I should have added more comments to the this file since the beginning..

Even though this file is lacking in comments, most .idl files should be well documented..  So can you document this function that you're adding with a comment? It can be brief, just describing what this is for, and what's the return value.

(simple example: https://searchfox.org/mozilla-central/source/browser/components/migration/nsIBrowserProfileMigrator.idl#46)

also, nit: add an empty line before this new block that you're adding
Attachment #8986515 - Flags: review?(felipc)
Comment on attachment 8986515 [details]
Kanika Bug 1465950 - Parsed policies stored by EnterprisePolicies.js to display on about:policies page

https://reviewboard.mozilla.org/r/251838/#review258592

Cool, looks great now! I'll give the final r+ once the tests are finished.

You can just create a new test file in browser/components/enterprisepolicies/tests/browser/  (don't forget to include it in the browser.ini file).  In the test, you should use setupPolicyEngineWithJSON to setup a few simple policies, and then use .getActivePolicies() to get the results and check that they are as expected. Tip: there's an Assert.deepEqual function that can compare two objects, so you could use that.

It'd be nice if the test includes some error cases (e.g., including some invalid policies to make sure they don't appear in the .getActivePolicies), and also use ContentTask.spawn() to verify that the content process implementation throws the expected error.
Attachment #8986515 - Flags: review?(felipc)
Status: NEW → ASSIGNED
let wccr = Cc["@mozilla.org"].getService(Ci.nsIEnterprisePolicies);
is(expected, wccr.getActivePolicies(), "fine"); 

I couldn't call the getActivePolicies() in the test file. I tried to see some examples and it followed the above way but @mozilla.org is not the correct domain. Can you please help? Thank you.
The right way would be:

let policies = Cc[""@mozilla.org/browser/enterprisepolicies;1"].getService(Ci.nsIEnterprisePolicies);

but you don't need to write that! There's a shortcut:  `Services.policies`


(you can see all the shortcuts in Services.jsm: https://searchfox.org/mozilla-central/source/toolkit/modules/Services.jsm)
(In reply to :Felipe Gomes (needinfo me!) from comment #7)
> The right way would be:
> 
> let policies =
> Cc[""@mozilla.org/browser/enterprisepolicies;1"].getService(Ci.
> nsIEnterprisePolicies);
> 
> but you don't need to write that! There's a shortcut:  `Services.policies`
> 
> 
> (you can see all the shortcuts in Services.jsm:
> https://searchfox.org/mozilla-central/source/toolkit/modules/Services.jsm)

Thank you! I actually tried Services.policies before posting the query here because it gave me an error saying there exists no function called getActivePolicies() and it is still the same. It has no problem calling isAllowed() so I wonder if I've missed out somewhere in declaring the new function. 

Also, the content process test may be a lot wrong. I studied about what are content processes and how exactly I could call the stub implementation of getActivePolicies(). I found out how recieveMessage of EnterprisePoliciesContent.js is being called,
https://searchfox.org/mozilla-central/rev/39b790b29543a4718d876d8ca3fd179d82fc24f7/toolkit/mozapps/extensions/amContentHandler.js#66

Could I follow something similar?
(In reply to Kanika Saini from comment #9)
> (In reply to :Felipe Gomes (needinfo me!) from comment #7)
> > The right way would be:
> > 
> > let policies =
> > Cc[""@mozilla.org/browser/enterprisepolicies;1"].getService(Ci.
> > nsIEnterprisePolicies);
> > 
> > but you don't need to write that! There's a shortcut:  `Services.policies`
> > 
> > 
> > (you can see all the shortcuts in Services.jsm:
> > https://searchfox.org/mozilla-central/source/toolkit/modules/Services.jsm)
> 
> Thank you! I actually tried Services.policies before posting the query here
> because it gave me an error saying there exists no function called
> getActivePolicies() and it is still the same. It has no problem calling
> isAllowed() so I wonder if I've missed out somewhere in declaring the new
> function. 

Ah, what you're doing is correct. It's just that I forgot that new functions being added to .idl files don't work in artifact builds. So you'll need a full build to make it work.
Comment on attachment 8986515 [details]
Kanika Bug 1465950 - Parsed policies stored by EnterprisePolicies.js to display on about:policies page

https://reviewboard.mozilla.org/r/251838/#review259598

::: browser/components/enterprisepolicies/tests/browser/browser_policy_active_policies.js:31
(Diff revision 3)
> +
> +add_task(async function test_content_process() {
> +  await ContentTask.spawn(gBrowser.selectedBrowser, null, function() {
> +    Assert.deepEqual( await content.wrappedJSObject.getActivePolicies(), Cr.NS_ERROR_NOT_AVAILABLE, "Error: getActivePolicies() function doesn't exist");
> +  });
> +});

What you're doing with ContentTask.spawn is correct! That's the way (in tests) to make a function run in the content process.

But what you need to check is a bit different. To call the function, you just call Services.policies.getActivePolicies() as you normally would   (this will work with a full build, as I realized in the previous comment).

But there's no need to compare it to an object. Actually you'll need to wrap the call in a try-catch block (because otherwise the exception will make the test fail), and then you should compare the returned exception with Cr.NS_ERROR_NOT_AVAILABLE
Attachment #8986515 - Flags: review?(felipc)
Thank you!
It's working as expected now. :D
One last thing, is the name of the test file appropriate? Since it's not about a policy as such, I considered naming the test file as browser_getActivePolicies.js instead of browser_policy_active_policies.js.
Comment on attachment 8986515 [details]
Kanika Bug 1465950 - Parsed policies stored by EnterprisePolicies.js to display on about:policies page

https://reviewboard.mozilla.org/r/251838/#review259762
Attachment #8986515 - Flags: review?(felipc) → review+
(In reply to Kanika Saini from comment #13)
> Thank you!
> It's working as expected now. :D
> One last thing, is the name of the test file appropriate? Since it's not
> about a policy as such, I considered naming the test file as
> browser_getActivePolicies.js instead of browser_policy_active_policies.js.

Ah, we have been naming these as browser_policies_*. Could you rename that to browser_policies_getActivePolicies.js and update the patch?
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04352a50d097
Keep parsed policies stored by EnterprisePolicies.js to display them on about:policies page. r=felipe
Backed out changeset 04352a50d097 (bug 1465950) for linting failure on /enterprisepolicies/tests/browser/browser_policies_getActivePolicies.js on a CLOSED TREE

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

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=04352a50d097bcf965238b22e067b0ee0c9fa3b4

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=185074262&repo=mozilla-inbound&lineNumber=256

Log snippet:

[vcs 2018-06-27T03:37:54.363Z] 244530 files updated, 0 files merged, 0 files removed, 0 files unresolved
[vcs 2018-06-27T03:37:54.648Z] updated to 04352a50d097bcf965238b22e067b0ee0c9fa3b4
[vcs 2018-06-27T03:37:54.680Z] PERFHERDER_DATA: {"framework": {"name": "vcs"}, "suites": [{"extraOptions": ["c3.xlarge"], "lowerIsBetter": true, "name": "clone", "shouldAlert": false, "subtests": [], "value": 117.50969195365906}, {"extraOptions": ["c3.xlarge"], "lowerIsBetter": true, "name": "update", "shouldAlert": false, "subtests": [], "value": 101.98561310768127}, {"extraOptions": ["c3.xlarge"], "lowerIsBetter": true, "name": "overall", "shouldAlert": false, "subtests": [], "value": 219.90457606315613}]}
[vcs 2018-06-27T03:37:54.992Z] TinderboxPrint:<a href=https://hg.mozilla.org/integration/mozilla-inbound/rev/04352a50d097bcf965238b22e067b0ee0c9fa3b4 title='Built from mozilla-inbound revision 04352a50d097bcf965238b22e067b0ee0c9fa3b4'>04352a50d097bcf965238b22e067b0ee0c9fa3b4</a>
[task 2018-06-27T03:37:54.992Z] executing ['bash', '-cx', 'cd /builds/worker/checkouts/gecko/ && cp -r /build/node_modules_eslint node_modules && ln -s ../tools/lint/eslint/eslint-plugin-mozilla node_modules && ln -s ../tools/lint/eslint/eslint-plugin-spidermonkey-js node_modules && ./mach lint -l eslint -f treeherder --quiet\n']
[task 2018-06-27T03:37:55.000Z] + cd /builds/worker/checkouts/gecko/
[task 2018-06-27T03:37:55.000Z] + cp -r /build/node_modules_eslint node_modules
[task 2018-06-27T03:37:55.714Z] + ln -s ../tools/lint/eslint/eslint-plugin-mozilla node_modules
[task 2018-06-27T03:37:55.716Z] + ln -s ../tools/lint/eslint/eslint-plugin-spidermonkey-js node_modules
[task 2018-06-27T03:37:55.717Z] + ./mach lint -l eslint -f treeherder --quiet
[task 2018-06-27T03:37:56.513Z] New python executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python2.7
[task 2018-06-27T03:37:56.513Z] Also creating executable in /builds/worker/checkouts/gecko/obj-x86_64-pc-linux-gnu/_virtualenvs/init/bin/python
[task 2018-06-27T03:37:58.311Z] Installing setuptools, pip, wheel...done.
[task 2018-06-27T03:37:59.521Z] running build_ext
[task 2018-06-27T03:37:59.521Z] building 'psutil._psutil_linux' extension
[task 2018-06-27T03:37:59.521Z] creating build
[task 2018-06-27T03:37:59.521Z] creating build/temp.linux-x86_64-2.7
[task 2018-06-27T03:37:59.521Z] creating build/temp.linux-x86_64-2.7/psutil
[task 2018-06-27T03:37:59.521Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-06-27T03:37:59.521Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-06-27T03:37:59.521Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_linux.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o
[task 2018-06-27T03:37:59.521Z] creating build/lib.linux-x86_64-2.7
[task 2018-06-27T03:37:59.521Z] creating build/lib.linux-x86_64-2.7/psutil
[task 2018-06-27T03:37:59.521Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o build/temp.linux-x86_64-2.7/psutil/_psutil_linux.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so
[task 2018-06-27T03:37:59.521Z] building 'psutil._psutil_posix' extension
[task 2018-06-27T03:37:59.521Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_common.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_common.o
[task 2018-06-27T03:37:59.521Z] x86_64-linux-gnu-gcc -pthread -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -fno-strict-aliasing -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -fPIC -DPSUTIL_POSIX=1 -DPSUTIL_VERSION=543 -DPSUTIL_LINUX=1 -I/usr/include/python2.7 -c psutil/_psutil_posix.c -o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o
[task 2018-06-27T03:37:59.521Z] x86_64-linux-gnu-gcc -pthread -shared -Wl,-O1 -Wl,-Bsymbolic-functions -Wl,-Bsymbolic-functions -Wl,-z,relro -fno-strict-aliasing -DNDEBUG -g -fwrapv -O2 -Wall -Wstrict-prototypes -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security -Wl,-Bsymbolic-functions -Wl,-z,relro -Wdate-time -D_FORTIFY_SOURCE=2 -g -fstack-protector-strong -Wformat -Werror=format-security build/temp.linux-x86_64-2.7/psutil/_psutil_common.o build/temp.linux-x86_64-2.7/psutil/_psutil_posix.o -o build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so
[task 2018-06-27T03:37:59.521Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_linux.so -> psutil
[task 2018-06-27T03:37:59.521Z] copying build/lib.linux-x86_64-2.7/psutil/_psutil_posix.so -> psutil
[task 2018-06-27T03:37:59.521Z] 
[task 2018-06-27T03:37:59.521Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-06-27T03:43:25.666Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/enterprisepolicies/tests/browser/browser_policies_getActivePolicies.js:15:4 | Missing semicolon. (semi)
[task 2018-06-27T03:43:25.666Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/enterprisepolicies/tests/browser/browser_policies_getActivePolicies.js:36:7 | Expected space(s) after "catch". (keyword-spacing)
Flags: needinfo?(ksaini)
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce5b67578d0c
Keep parsed policies stored by EnterprisePolicies.js to display them on about:policies page. r=felipe
Backed out for failing eslint at /builds/worker/checkouts/gecko/browser/components/enterprisepolicies/tests/browser/browser_policies_getActivePolicies.js:15:4

Push that started the failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ce5b67578d0c1e9f22bbab7822d2717cd80c47fe

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

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/d45a45dd4b5b6dc659000fa0f45df5972bef0f7c
Flags: needinfo?(felipc)
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6039fa792e1b
Keep parsed policies stored by EnterprisePolicies.js to display them on about:policies page. r=felipe
Flags: needinfo?(felipc)
https://hg.mozilla.org/mozilla-central/rev/6039fa792e1b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
Flags: needinfo?(ksaini)
I am marking as wontfix for 62 since this seems dependent on the creation of an about:policies page in bug 1465953 and it hasn't landed on Nightly yet. Feel free to ping release managers if you feel that an uplift to 62beta would make sense.
status-firefox62: affected → wontfix
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: