1429186 - Policy: Set and lock homepage

Status: VERIFIED FIXED

(Firefox :: Enterprise Policies, enhancement, P1)

Description

[:Felipe Gomes (needinfo for replies!)]

Set the homepage and lock it.

We need to figure out how this works UX-wise with this setting in about:preferences

Comment 1

[Florian Quèze [:florian]]

(In reply to :Felipe Gomes (needinfo me!) from comment #0)
> Set the homepage and lock it.
> 
> We need to figure out how this works UX-wise with this setting in
> about:preferences

Show the URL there in the text field, and disable the text field and buttons? Maybe with a message explaining this setting has been locked by the system administrator?

Comment 2

[Robin Steuber (they/them) [:bytesized]]

A note: The prefs currently controlling this are |browser.startup.homepage| and possibly |browser.startup.page| (depending on how we want this to work).

However, Bug 1051082 intends to change this. I don't believe that anyone is working on it right now, but probably good to keep in mind.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Mike, two questions:

1- is there really any interest in the use case of setting the homepage but *not* locking it? Or should we only care about the case of setting & locking the homepage?

2- Firefox supports having multiple homepages. Do we want to support this too?  (by making the policy an array type instead of a simple URL type)

Comment 4

[Mike Kaply [:mkaply]]

(In reply to :Felipe Gomes (needinfo me!) from comment #3)
> Mike, two questions:
> 
> 1- is there really any interest in the use case of setting the homepage but
> *not* locking it? Or should we only care about the case of setting & locking
> the homepage?

Yes. Homepage is one of those things where a company/organization would set the default value and allow a user to change.

Chrome has the concept of default values in GPO for things that you set only on the initial creation of the profile. Our world is a little different. We would call those default values.

In the GPO UI, I'll show this as  URL to enter the homepage and a checkbox for locked.

> 
> 2- Firefox supports having multiple homepages. Do we want to support this
> too?  (by making the policy an array type instead of a simple URL type)

I think this is rarely used and make things more complicated. I think we can just allow the same format as Firefox - http://www.foo.com|http://bar.com

That means we'll have to make homepage not a URL in the schema.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

(In reply to Mike Kaply [:mkaply] from comment #4)
> Yes. Homepage is one of those things where a company/organization would set
> the default value and allow a user to change.
> 
> In the GPO UI, I'll show this as  URL to enter the homepage and a checkbox
> for locked.

Ok, so what I suggest for the JSON structure for this policy is:

"Homepage": {
  "URL": "https://www.example.com",
  Locked: true
}

(In reply to Mike Kaply [:mkaply] from comment #4)
> I think this is rarely used and make things more complicated. I think we can
> just allow the same format as Firefox - http://www.foo.com|http://bar.com
> 
> That means we'll have to make homepage not a URL in the schema.

Ah I'd really to avoid doing that if possible. Of course the policy could itself could validate it, but I'd prefer the validator to be doing all the work on the input.

Some options:

A) Only support one URL as more than one is rarely used

B) Expose it on the GPO UI as an array + the checkbox..

C) We could easily make the validator code to accept a single item for array items, for example:

if (!Array.isArray(param)) {
  param = [param];
}

That means we could accept both:

URL: "https://www.example.com"

and

URL: ["https://www.example1.com", "https://www.example2.com"]


But I don't know if that helps at all with the GPO UI.


If none of these options are reasonable then we can make the policy do the URL validation

Comment 6

[:Felipe Gomes (needinfo for replies!)]

Mkaply had a great idea on IRC: use one field to be a single URL, and another optional one as an array to provide additional homepages. Like so:

"Homepage": {
  "URL": "https://www.example1.com",
  "Locked": true,
  "Additional": ["https://www.example2.com", "https://www.example3.com"]
}

Comment 7

[Andrei Oprea [:andreio]]

In the Activity Stream team we are working on migrating our preferences in about:preferences.
One of the options is control over about:home and about:newtab
Best overview would be in this design https://mozilla.invisionapp.com/share/EGFI16P8Y36#/screens/276669529_Preferences_-_Home_Selected
This is tracked in bug 1404890. I think there should be some coordination?

Comment 8

[:Felipe Gomes (needinfo for replies!)]

There's also recent UI that landed to indicate when an add-on is controlling the homepage, bug 1436165. Andrei, is that also being coordinated with the changes you're working on?

Comment 9

[:shell escalante]

Hi Amin,
Hi Brian,

You worked out the general message for about:preferences to say that some of it was controlled by Policy in https://bugzilla.mozilla.org/show_bug.cgi?id=1432906#c4

Did you mock the "more contextual message bar that is next to the blocked/disabled preferences sections or options?"

This bug is one where we'd want the general and contextual message.  The area that we will be adding it is described in Comment 7.

Comment 10

[Amin Al Hazwani [:amin] (Firefox UX)]

(In reply to :shell escalante from comment #9)
> Did you mock the "more contextual message bar that is next to the
> blocked/disabled preferences sections or options?"

Hi Shell,

Yes, the more contextual bar you can see it here https://mozilla.invisionapp.com/share/QWFUIA593BH#/screens/279275422_Enterprise_Edition (scroll down, it's the under "Extra Credit" title).

The challenge here for the default messages is to know what admins are blocking/disabling/force-enabling so that Brian can craft more effective strings. I already asked Felipe on this bug https://bugzilla.mozilla.org/show_bug.cgi?id=1432906#c12 if this is possible.

Comment 11

[Robin Steuber (they/them) [:bytesized]]

It sounds like Bug 1432906 covers my concerns here. I just need to finish writing the test for this patch and I will put it up for review.

Comment 12

[Robin Steuber (they/them) [:bytesized]]

Attachment: Bug 1429186 - Create an enterprise policy to set the homepage and, optionally, lock it

Review commit: https://reviewboard.mozilla.org/r/224172/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/224172/

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8954990 [details]
Bug 1429186 - Create an enterprise policy to set the homepage and, optionally, lock it

https://reviewboard.mozilla.org/r/224172/#review230132

::: browser/components/enterprisepolicies/Policies.jsm:228
(Diff revision 1)
> +        setAndLockPref("browser.startup.homepage", homepages);
> +        setAndLockPref("browser.startup.page", 1);
> +        setAndLockPref("pref.browser.homepage.disable_button.current_page", true);
> +        setAndLockPref("pref.browser.homepage.disable_button.bookmark_page", true);
> +        setAndLockPref("pref.browser.homepage.disable_button.restore_default", true);
> +        manager.disallowFeature("changeHomepage");

disallowFeature ended up unused for this policy, so remove it here and from the test

::: browser/components/enterprisepolicies/Policies.jsm:378
(Diff revision 1)
> + *        string.
> + * @param {Function} callback
> + *        The callback to be run when the pref value changes
> + */
> +function runOncePerModification(actionName, policyValue, callback) {
> +  let prefName = `browser.policies.runOncePerModification.${actionName}`;

you could use the same pref naming as runOnce (i.e., `browser.policies.runonce.${actionName}`), because the same actionName shouldn't be used more than once.. so some prefs will be booleans, some other ones will be strings

::: browser/components/enterprisepolicies/Policies.jsm:384
(Diff revision 1)
> +  let oldPolicyValue = Services.prefs.getStringPref(prefName, undefined);
> +  if (policyValue === oldPolicyValue) {
> +    log.debug(`Not running action ${actionName} again because the policy's value is unchanged`);
> +    return;
> +  }
> +  callback();

I know this is how I originally wrote runOnce, but I just realized a problem here. Can you move this callback for after the pref is set? (and fix it in runOnce too?)

if the callback throws, the same error will throw every time.

It's not necessary to wrap in a try-catch because this is already wrapped by the manager code calling onBeforeUIStartup.

::: browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js:39
(Diff revision 1)
> +  }
> +});
> +
> +add_task(async function homepage_test_simple() {
> +  await setupPolicyEngineWithJson({
> +                                    "policies": {

uber nit, but I'd prefer if you used this style of indentation: https://searchfox.org/mozilla-central/rev/769222fadff46164f8cc0dc7a0bae5a60dc2f335/browser/components/enterprisepolicies/tests/browser/browser_policy_default_browser_check.js#15 :P

Comment 14

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 8954990 [details]
Bug 1429186 - Create an enterprise policy to set the homepage and, optionally, lock it

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224172/diff/1-2/

Comment 15

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 8954990 [details]
Bug 1429186 - Create an enterprise policy to set the homepage and, optionally, lock it

https://reviewboard.mozilla.org/r/224172/#review230132

> you could use the same pref naming as runOnce (i.e., `browser.policies.runonce.${actionName}`), because the same actionName shouldn't be used more than once.. so some prefs will be booleans, some other ones will be strings

It could use the same pref naming scheme, but I don't really see any reason to do this. It seems like it would provide no benefits and would prevent us from ever writing code like:
```javascript
runOnce('policyName', fn1);
runOncePerModification('policyName', fn2);
```
requiring instead:
```javascript
runOnce('policyName1', fn1);
runOncePerModification('policyName2', fn2);
```
We do not currently use this pattern, but we might want to in the future. And perhaps more importantly, I just don't see any compelling reason to make this change.

Comment 16

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8954990 [details]
Bug 1429186 - Create an enterprise policy to set the homepage and, optionally, lock it

https://reviewboard.mozilla.org/r/224172/#review230366

Comment 17

[Florian Quèze [:florian]]

Comment on attachment 8954990 [details]
Bug 1429186 - Create an enterprise policy to set the homepage and, optionally, lock it

https://reviewboard.mozilla.org/r/224172/#review230376

Unless you have something specific in mind that I should review, I don't think you need my review, felipe's is enough. I did a quick look and didn't see anything scary but I haven't done a full review.

::: browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js:46
(Diff revision 2)
> +        "URL": "http://example1.com/"
> +      }
> +    }
> +  });
> +is(Services.prefs.getStringPref("browser.startup.homepage", ""),
> +"http://example1.com/", "Homepage URL should have been set");

the indent is broken here.

::: browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js:48
(Diff revision 2)
> +    }
> +  });
> +is(Services.prefs.getStringPref("browser.startup.homepage", ""),
> +"http://example1.com/", "Homepage URL should have been set");
> +is(Services.prefs.getIntPref("browser.startup.page", -1), 1,
> +     "Homepage should be used instead of blank page or previous tabs");

and here

::: browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js:67
(Diff revision 2)
> +        "URL": "http://example1.com/"
> +      }
> +    }
> +  });
> +is(Services.prefs.getStringPref("browser.startup.homepage", ""),
> +"http://example2.com/",

and here

::: browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js:83
(Diff revision 2)
> +      "Homepage": {
> +        "URL": "http://example3.com/"
> +      }
> +    }
> +  });
> +is(Services.prefs.getStringPref("browser.startup.homepage", ""),

and here

::: browser/components/enterprisepolicies/tests/browser/browser_policy_set_homepage.js:99
(Diff revision 2)
> +        "URL": "http://example1.com/",
> +        "Additional": []
> +      }
> +    }
> +  });
> +is(Services.prefs.getStringPref("browser.startup.homepage", ""),

and here and on the next 2 'is' calls.

Comment 18

[Robin Steuber (they/them) [:bytesized]]

Comment on attachment 8954990 [details]
Bug 1429186 - Create an enterprise policy to set the homepage and, optionally, lock it

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/224172/diff/2-3/

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/406c8d7d134f
Create an enterprise policy to set the homepage and, optionally, lock it r=Felipe

Comment 20

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/406c8d7d134f

Comment 21

[Abe - QA (:Abe_LV)]

We tested “set and lock homepage” policy using JSON file.  The homepage can be set and locked by this policy and we verified this manually as fixed.

Test steps and runs are available here: https://testrail.stage.mozaws.net/index.php?/plans/view/7734

The bug will also be retested with ADMX files when it is ready for testing.

Comment 22

[Abe - QA (:Abe_LV)]

We retested this on beta builds with ADM and JSON policy formats and it is verified as fixed.