Closed Bug 1428922 Opened 6 years ago Closed 6 years ago

Implement permissions policies (cookies, popups, etc.)

Categories

(Firefox :: Enterprise Policies, enhancement, P1)

Product:
Firefox
Component:
Enterprise Policies
Type:
enhancement

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
firefox60 --- verified

People

(Reporter: Felipe, Assigned: Felipe)

References

Details

Attachments

(6 files)

Bug 1428922 - Add tests for the permissions policies.
59 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1428922 - Implement helper function to support various permissions-type policies, and use it to implement the Flash Plugin policy.
59 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1428922 - Implement popups policy.
59 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1428922 - Implement allow-addons policy.
59 bytes, text/x-review-board-request
nika
: review+
Details
Bug 1428922 - Implement cookies policy.
59 bytes, text/x-review-board-request
nika
: review+
Details
Part 0: Add an EXPIRE_POLICY expire type to nsPermissionManager
9.52 KB, patch
baku
: review+
Details | Diff | Splinter Review 
There are 4 policies that can be implemented at once that involve granting per-host permissions to the following features:

- popups
- cookies
- flash
- installing webextensions

These policies should use the Permissions service to add a temporary (i.e., session-expiring) permission so that it has no permanent effect in the profile, allowing for it to be dynamically changed by a sysadmin.
Assignee: nobody → felipc
Status: NEW → ASSIGNED
Comment on attachment 8941652 [details]
Bug 1428922 - Add tests for the permissions policies.

hmm for some reason, ReviewBoard only published the last cset in the series.. Let me fix it
Attachment #8941652 - Flags: review?(nika)
Attachment #8941652 - Flags: review?(nika)
Comment on attachment 8941652 [details]
Bug 1428922 - Add tests for the permissions policies.

https://reviewboard.mozilla.org/r/211892/#review217860

r- because I think there are some more edge cases which deserve testing here.

::: browser/components/enterprisepolicies/tests/browser/browser_policies_popups_cookies_addons_flash.js:17
(Diff revision 1)
> +  await startWithCleanSlate();
> +  await setupPolicyEngineWithJson("config_popups_cookies_addons_flash.json");
> +  is(Services.policies.status, Ci.nsIEnterprisePolicies.ACTIVE, "Engine is active");
> +});
> +
> +add_task(async function test_popups_allow_deny() {

Some things which would probably be good to test, to make sure it acts the way that you are wanting it to:

1. What happens if the user has a saved permission in their permissions database which is also listed in the policy engine during startup?

2. What happens if the user clears their user data for a site which has a permission set by the policy engine? Should it be cleared or restored?

3. What does a user see when they open the site info page for a website with policy-specified permissions? Are they able to modify those permissions? Will those modified permissions persist across restarts?

::: browser/components/enterprisepolicies/tests/browser/browser_policies_popups_cookies_addons_flash.js:26
(Diff revision 1)
> +     "Allow origin was correctly allowed");
> +
> +  is(Services.perms.testPermission(URI("https://www.deny.com"),
> +                                   "popup"),
> +     Ci.nsIPermissionManager.DENY_ACTION,
> +     "Deny origin was correctly allowed");

nit: allowed here and below doesn't make any sense

::: browser/components/enterprisepolicies/tests/browser/config_popups_cookies_addons_flash.json:5
(Diff revision 1)
> +{
> +  "policies": {
> +    "popups": {
> +      "allow": [
> +        "https://www.allow.com"

Are you planning to also support overriding the default behaviour for these policies?
Attachment #8941652 - Flags: review?(nika) → review-
Comment on attachment 8941894 [details]
Bug 1428922 - Implement helper function to support various permissions-type policies, and use it to implement the Flash Plugin policy.

https://reviewboard.mozilla.org/r/212102/#review217856

::: browser/components/enterprisepolicies/schemas/policies.json:41
(Diff revision 1)
> +      "type": "object",
> +      "properties": {
> +        "allow": {
> +          "type": "array",
> +          "items": {
> +            "type": "origin"

I can't find this file in the tree (I suppose it was added recently?)

Can you link me the code which performs the origin validation and transformation? I'd like to take a look at that and make sure it's appropriate here.
Attachment #8941894 - Flags: review?(nika) → review-
Comment on attachment 8941895 [details]
Bug 1428922 - Implement popups policy.

https://reviewboard.mozilla.org/r/212104/#review217862
Attachment #8941895 - Flags: review?(nika) → review+
Comment on attachment 8941896 [details]
Bug 1428922 - Implement allow-addons policy.

https://reviewboard.mozilla.org/r/212106/#review217864
Attachment #8941896 - Flags: review?(nika) → review+
Comment on attachment 8941897 [details]
Bug 1428922 - Implement cookies policy.

https://reviewboard.mozilla.org/r/212108/#review217866
Attachment #8941897 - Flags: review?(nika) → review+
Oh thanks for the quick review! I was writing this comment about how it works (since the code hasn't landed yet). You probably already figured it out, but here it goes:


Bug 1419102 implements a component that on startup will look for a .json file in the installation folder of Firefox (not the profile folder). This json is meant to be added by a sysadmin in that folder, and it will define which policies to activate.

To implement a new policy, one needs to define its scheme in the in-tree schemas/policies.json file, and implement it (with a matching name) in Policies.jsm

The parameter validation and parsing is done by the code in Policies.jsm  (the PoliciesValidator object).

Take a look at the patches in bug 1419102 for more details..  (in the near future the policies will also be ready from Windows GPO, in addition to the user-provided json file).


(I'll reply to the review comments next)
(In reply to Nika Layzell [:mystor] from comment #7)
> Comment on attachment 8941652 [details]
> Bug 1428922 - Add tests for the permissions policies.
> 
> https://reviewboard.mozilla.org/r/211892/#review217860
> 
> r- because I think there are some more edge cases which deserve testing here.
> 
> :::
> browser/components/enterprisepolicies/tests/browser/
> browser_policies_popups_cookies_addons_flash.js:17
> (Diff revision 1)
> > +  await startWithCleanSlate();
> > +  await setupPolicyEngineWithJson("config_popups_cookies_addons_flash.json");
> > +  is(Services.policies.status, Ci.nsIEnterprisePolicies.ACTIVE, "Engine is active");
> > +});
> > +
> > +add_task(async function test_popups_allow_deny() {
> 
> Some things which would probably be good to test, to make sure it acts the
> way that you are wanting it to:
> 
> 1. What happens if the user has a saved permission in their permissions
> database which is also listed in the policy engine during startup?
> 
> 2. What happens if the user clears their user data for a site which has a
> permission set by the policy engine? Should it be cleared or restored?
> 
> 3. What does a user see when they open the site info page for a website with
> policy-specified permissions? Are they able to modify those permissions?
> Will those modified permissions persist across restarts?

Those are some really good considerations. I used the EXPIRE_SESSION policy so that this doesn't have any permanent effect on the user's profile, so whenever the sysadmin changes the policies, there's no clean-up to do.

But it looks like we might want to add some new support in the permission manager for this, with a new permission type that is not editable/eraseable/overrideable..

For the ALLOW permission, that's not a big deal.. It's just the user creating some trouble for himself, but that's ok. For the DENY one it's more complicated I guess..  Some sysadmins might be looking for enforcement, while others don't care much and are just creating sensible defaults.

I'll ask Mike Kaply to see what the existing 3rd-party solutions do here, to see how much we need to worry about it.

> 
> :::
> browser/components/enterprisepolicies/tests/browser/
> browser_policies_popups_cookies_addons_flash.js:26
> (Diff revision 1)
> > +     "Allow origin was correctly allowed");
> > +
> > +  is(Services.perms.testPermission(URI("https://www.deny.com"),
> > +                                   "popup"),
> > +     Ci.nsIPermissionManager.DENY_ACTION,
> > +     "Deny origin was correctly allowed");
> 
> nit: allowed here and below doesn't make any sense

good catch, copy&paste mistake

> 
> :::
> browser/components/enterprisepolicies/tests/browser/
> config_popups_cookies_addons_flash.json:5
> (Diff revision 1)
> > +{
> > +  "policies": {
> > +    "popups": {
> > +      "allow": [
> > +        "https://www.allow.com"
> 
> Are you planning to also support overriding the default behaviour for these
> policies?

Indeed I am, that's why I structured it this way. But it's tracked in a separate bug: bug 1429169
In this new version of the patch I added some code to enforce the permissions set by the policies. It observes the perm-changed notification, and if the change is related to a permission that is tracked by the police engine, we re-set its value to the desired value.

I believe this is good enough for the MVP version, and we can follow-up on separate bugs to see if this can be better supported by the UI and/or directly built-in in the permission manager itself.

I also added testcases to cover that pre-existing permissions set on the profile get overridden by what's set through the police engine.
Comment on attachment 8941652 [details]
Bug 1428922 - Add tests for the permissions policies.

https://reviewboard.mozilla.org/r/211892/#review218290

Thanks, looks good.

::: browser/components/enterprisepolicies/tests/browser/browser_policies_popups_cookies_addons_flash.js:109
(Diff revision 3)
> +                     Ci.nsIPermissionManager.EXPIRE_SESSION);
> +
> +  checkPermission("allow.com", "ALLOW", "popup");
> +
> +  // Also change one un-managed permission to make sure it doesn't
> +  // cause any problems to the police engine or the permission manager.

nit policy
Attachment #8941652 - Flags: review?(nika) → review+
Comment on attachment 8941894 [details]
Bug 1428922 - Implement helper function to support various permissions-type policies, and use it to implement the Flash Plugin policy.

https://reviewboard.mozilla.org/r/212102/#review218292

::: browser/components/enterprisepolicies/helpers/PermissionPolicies.jsm:54
(Diff revision 3)
> +  observe(subject, topic, data) {
> +    if (topic != "perm-changed") {
> +      return;
> +    }
> +
> +    // Enforce the values set through the police engine any time someone attempts

I don't like this solution, I'd rather implement something for this directly inside nsPermissionManager, as it'll be less code and easier to maintain.

I'll attach a patch to this bug which adds an EXPIRE_POLICY expire type to nsIPermissionManager with the following effects:

1. Changes with EXPIRE_POLICY are not persisted to the DB.
2. It is not possible to change a permission once it is set with EXPIRE_POLICY.
Attachment #8941894 - Flags: review?(nika) → review-
I think this should be a cleaner way to implement policy permissions than using EXPIRE_SESSION, and resetting them whenever a change event is fired.

This change adds EXPIRE_POLICY which is not persisted to the database, and rejects attempts to change existing permissions which are marked as EXPIRE_POLICY.

MozReview-Commit-ID: JJm7g2GSIjT
Attachment #8942325 - Flags: review?(amarchesini)
Yay, thank you! I'll update my patch and tests to use this new expire type :)

Will this need any UI changes in about:preferences or the Site Identity panel to handle this new type correctly?
Comment on attachment 8941894 [details]
Bug 1428922 - Implement helper function to support various permissions-type policies, and use it to implement the Flash Plugin policy.

https://reviewboard.mozilla.org/r/212102/#review218332

r=me, but I think it'd be nicer if you could avoid the PermissionPolicies.jsm file and just put the addAllowDenyPermissions function directly in Policies.jsm like you did in the first version of this patch.

> Will this need any UI changes in about:preferences or the Site Identity panel to handle this new type correctly?

I took a look at the list of consumers. Right now I think to the UI it'll probably look like a normal persistent permission (https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/browser/modules/SitePermissions.jsm#357-362). 

That being said, we will probably want to do some changes to the UI to make sure that the UI elements don't let you try to change the permissions. I figure that that is something you folks can do in a follow-up.
Attachment #8941894 - Flags: review?(nika) → review+
Attachment #8942325 - Flags: review?(amarchesini) → review+
I'll leave you to land these patches Felipe
Flags: needinfo?(felipc)
Sure thing! Thanks :)
Flags: needinfo?(felipc)
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a37f11b2eb25
Part 0: Add an EXPIRE_POLICY expire type to nsPermissionManager, r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/e01fa0b2b8b8
Implement helper function to support various permissions-type policies, and use it to implement the Flash, Cookies, Install-Addons and Popups policy. r=mystor
https://hg.mozilla.org/mozilla-central/rev/a37f11b2eb25
https://hg.mozilla.org/mozilla-central/rev/e01fa0b2b8b8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1433290
Depends on: 1433513
Depends on: 1433524
Depends on: 1433528
Depends on: 1433585
Depends on: 1434048
We tested this on Nightly[61] and beta[60] builds 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: 1455888
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: