Closed Bug 1429186 Opened 3 years ago Closed 3 years ago

Policy: Set and lock homepage

Categories

(Firefox :: Enterprise Policies, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 60
Tracking Status
firefox60 --- verified

People

(Reporter: Felipe, Assigned: bytesized)

References

Details

Attachments

(1 file)

Set the homepage and lock it.

We need to figure out how this works UX-wise with this setting in about:preferences
(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?
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.
Assignee: nobody → ksteuber
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)
Flags: needinfo?(mozilla)
(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.
Flags: needinfo?(mozilla)
(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
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"]
}
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?
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?
Flags: needinfo?(andrei.br92)
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.
Flags: needinfo?(brjones)
Flags: needinfo?(aalhazwani)
(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.
Flags: needinfo?(aalhazwani)
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.
Flags: needinfo?(brjones)
Attachment #8954990 - Flags: review?(florian)
Attachment #8954990 - Flags: review?(felipc)
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
Attachment #8954990 - Flags: review?(felipc)
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 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
Attachment #8954990 - Flags: review?(felipc) → review+
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.
Attachment #8954990 - Flags: review?(florian)
Pushed by ksteuber@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/406c8d7d134f
Create an enterprise policy to set the homepage and, optionally, lock it r=Felipe
Attachment #8954990 - Flags: review?(florian)
Flags: needinfo?(andrei.br92)
https://hg.mozilla.org/mozilla-central/rev/406c8d7d134f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Depends on: 1442739
Depends on: 1442743
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.
Blocks: 1447345
We retested this on beta builds with ADM and JSON policy formats and it is verified as fixed.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.