Closed
Bug 1429186
Opened 7 years ago
Closed 7 years ago
Policy: Set and lock homepage
Categories
(Firefox :: Enterprise Policies, enhancement, P1)
Firefox
Enterprise Policies
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
Comment 1•7 years ago
|
||
(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?
Assignee | ||
Comment 2•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → ksteuber
Reporter | ||
Comment 3•7 years ago
|
||
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)
Comment 4•7 years ago
|
||
(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)
Reporter | ||
Comment 5•7 years ago
|
||
(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
Reporter | ||
Comment 6•7 years ago
|
||
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•7 years ago
|
||
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?
Reporter | ||
Comment 8•7 years ago
|
||
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)
Comment 9•7 years ago
|
||
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)
Comment 10•7 years ago
|
||
(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)
Assignee | ||
Comment 11•7 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Attachment #8954990 -
Flags: review?(florian)
Attachment #8954990 -
Flags: review?(felipc)
Reporter | ||
Comment 13•7 years ago
|
||
mozreview-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/#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 hidden (mozreview-request) |
Assignee | ||
Comment 15•7 years ago
|
||
mozreview-review-reply |
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.
Reporter | ||
Comment 16•7 years ago
|
||
mozreview-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/#review230366
Attachment #8954990 -
Flags: review?(felipc) → review+
Comment 17•7 years ago
|
||
mozreview-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)
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
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
Updated•7 years ago
|
Attachment #8954990 -
Flags: review?(florian)
Updated•7 years ago
|
Flags: needinfo?(andrei.br92)
Comment 20•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox60:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Updated•7 years ago
|
status-firefox59:
affected → ---
Comment 21•7 years ago
|
||
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•6 years ago
|
||
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.
Description
•