Closed Bug 1583466 Opened 6 years ago Closed 6 years ago

[Linux] Load policies per user from system

Categories

(Firefox :: Enterprise Policies, enhancement, P3)

Product:
Firefox
Component:
Enterprise Policies
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 72
Tracking Flags:
Tracking Status
firefox72 --- fixed

People

(Reporter: stransky, Assigned: stransky)

References

Details

Attachments

(1 file)

Bug 1583466 - [Linux] Load policies per user from system, r=mkaply
47 bytes, text/x-phabricator-request
Details | Review

Downstream bug: https://bugzilla.redhat.com/show_bug.cgi?id=1754460

Firefox only reads policies from a system directory. In Fleet Commander we need a way to deploy policies for the users in a user basis.

The way to do this could be to make firefox to read the policies file from

/run/user/$PID/firefox/policies.json

So Fleet Commander could deploy that file and firefox apply the policies there.

Assignee: nobody → stransky

FYI, we already had a similar bug open:

https://bugzilla.mozilla.org/show_bug.cgi?id=1469629

And we were debating where to load policies in the user space.

Can I ask why you picked/run/user? (I honestly don't know Linux enough to know what the right thing to do).

Type: defect → enhancement
Priority: -- → P3

(In reply to Mike Kaply [:mkaply] from comment #2)

Can I ask why you picked/run/user? (I honestly don't know Linux enough to know what the right thing to do).

This request comes from https://wiki.gnome.org/Projects/FleetCommander project
which enables to deliver per-user customized configuration system wide.

Sample testcase seems to be non-functional. One bug here is that the name if Firefox so we need to remove uppercase from it.

[root@t480s firefox]# pwd
/run/user/1000/firefox
[root@t480s firefox]# ls -l
total 4
-rw-r--r--. 1 oliver oliver 263 sep 25 13:09 policies.json
[root@t480s firefox]# cat policies.json
{
"policies": {
"Bookmarks": [
{
"Title": "Test bookmark",
"URL": "https://example.com",
"Favicon": "https://example.com/favicon.ico",
"Placement": "toolbar" | "menu",
"Folder": "FolderName"
}
]
}
}

(In reply to Mike Kaply [:mkaply] from comment #2)

FYI, we already had a similar bug open:

https://bugzilla.mozilla.org/show_bug.cgi?id=1469629

And we were debating where to load policies in the user space.

Can I ask why you picked/run/user? (I honestly don't know Linux enough to know what the right thing to do).

Hey, Alberto here I am speaking on behalf of the Fleet Commander project:

When you want to deploy a policy you don't want to mess with the user filesystem namespace, mostly because you may choose to deploy a policy when the user is not logged in, in some cases their $HOME might be encrypted or be an NFS/SMB share that is only mounted at login time.

/run is a volatile runtime tree of directories that get deleted at reboot and we use it as a volatile cache for the "compiled down" profile data for several applications.

Hope that clarifies.

New patch updated, I added "browser.policies.perUserDir" pref and when it's set the per-user dir is used so this patch does not interfere with the default config unless user/admin decides so.

Mike, is there any strong objection against this patch/approach?

Frankly if we want Firefox enterprise support we need system-wide Firefox configuration options. I have more requests for global config options like file handle handler associations and so on. And those request come from enterprise customers and real life scenarios.

Also see Bug 1170092 for another real-life example which is blocked for very ridiculous reason:
https://bugzilla.mozilla.org/show_bug.cgi?id=1170092#c7.

Flags: needinfo?(mozilla)

The patch feels very Fleet Commander specific and that's what I'm having trouble with.

Is this solution something that would work for any Linux?

Is:

"/run/user/%d/%s/"

a common location?

I made a comment in the patch that we at least need a better name for XRE_APP_DISTRIBUTION_USER_DIR "XREAppDistUser"

That's not really what it is.

As far as bug 1170092 goes, that's not going to happen. We don't want to read default prefs from a non Firefox location.

You can use autoconfig (autoadmin.global_config_url) to accomplish what that bug is trying to do, and when bug 1469629 is fixed, you'll be able to put policies.json in a central location.

Flags: needinfo?(mozilla)

(In reply to Mike Kaply [:mkaply] from comment #8)

The patch feels very Fleet Commander specific and that's what I'm having trouble with.

There's nothing related to FleetComander. The directory is here by default on most of the systems and anyone can
write to it.

Is this solution something that would work for any Linux?

Is:

"/run/user/%d/%s/"

a common location?

Yes, /run/user/$UID is created by pam_systemd and you can find it on any system which uses systemd. See
https://www.freedesktop.org/software/systemd/man/pam_systemd.html

I made a comment in the patch that we at least need a better name for XRE_APP_DISTRIBUTION_USER_DIR "XREAppDistUser"

That's not really what it is.

As far as bug 1170092 goes, that's not going to happen. We don't want to read default prefs from a non Firefox location.

You can use autoconfig (autoadmin.global_config_url) to accomplish what that bug is trying to do, and when bug 1469629 is fixed, you'll be able to put policies.json in a central location.

That good but does not solve the issue with global per-user policy deployment.

Once we get a better rename on the directory name, I'm ok with this.

That good but does not solve the issue with global per-user policy deployment.

I was specifically referring to bug 1170092 which is about global deployment, not per user deployment.

(In reply to Mike Kaply [:mkaply] from comment #10)

Once we get a better rename on the directory name, I'm ok with this.

Great, Thanks. I'll update the patch.

I've been doing some tests because I've noticed now the last firefox Martin patched with this feature downstream is adding the bookmarks to the toolbar as expected.

Seems there was some zombie firefox process in my machine that prevented the new version to be loaded.

Anyway, even now it works, I tried to execute firefox using strace for checking access to the file and it does not show any line about the file. It is very extrange.

Another thing I noticed is that changing the file contents and for example, rename the folder or the title of a bookmark, as long as the user keeps the old bookmark, does not update it. Just does not create the new ones.

If I add new bookmarks then the new bookmarks are added, but, if I remove all that bookmarks, close firefox and open it again, firefox only creates the first bookmark. If I close and open it again then it creates the rest. I don't know if it is a caching problem, but found this behavior is consistent (maybe open another bug for this?).

Another thing I noticed is that changing the file contents and for example, rename the folder or the title of a bookmark, as long as the user keeps the old bookmark, does not update it. Just does not create the new ones.

Right now we're using the URL as the key, so yes, this is expected behavior.

If I add new bookmarks then the new bookmarks are added, but, if I remove all that bookmarks, close firefox and open it again, firefox only creates the first bookmark. If I close and open it again then it creates the rest. I don't know if it is a caching problem, but found this behavior is consistent (maybe open another bug for this?).

That definitely sounds like a bug.

My suggestion would be that you test with some simple boolean prefs for the feature.

That definitely sounds like a bug.

Ok. I will open a bug for that.

Meanwhile, I noticed the path for "per user" policies is capitalized:

/run/user/$UID/Firefox/

I think that for consistency with all the other app directories at that location we should use it without caps

/run/user/$UID/firefox/

(In reply to Oliver Gutierrez from comment #14)

I think that for consistency with all the other app directories at that location we should use it without caps

/run/user/$UID/firefox/

That's the path used in the latest patch here and also with the experimental patch in Fedora. See the "ToLowerCase()" there.

Perfect.

Thanks Martin :)

So the last thing we need is better name for XRE_APP_DISTRIBUTION_USER_DIR

Does that directory have some other name? distribution dir has a specific meaning in firefox.

(In reply to Mike Kaply [:mkaply] from comment #17)

So the last thing we need is better name for XRE_APP_DISTRIBUTION_USER_DIR

Does that directory have some other name? distribution dir has a specific meaning in firefox.

Mike,

according to [1] it's "user runtime directory" and it's stored in $XDG_RUNTIME_DIR env variable. So maybe XRE_USER_RUNTIME_DIR ?

[1] https://www.freedesktop.org/software/systemd/man/pam_systemd.html

Flags: needinfo?(mozilla)

according to [1] it's "user runtime directory" and it's stored in $XDG_RUNTIME_DIR env variable. So maybe XRE_USER_RUNTIME_DIR ?

That sounds much better.

Flags: needinfo?(mozilla)

(In reply to Mike Kaply [:mkaply] from comment #19)

according to [1] it's "user runtime directory" and it's stored in $XDG_RUNTIME_DIR env variable. So maybe XRE_USER_RUNTIME_DIR ?

That sounds much better.

Updated, Thanks.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a6378a6d5dacc2792abe9fef735149fd9e58c10

I'm also not sure that this directory makes sense. From the XDG information it suggests that this directory is meant to be removed after a user logs out and created fresh when they log back in. I'm not sure why that makes it a good place to put user policies that I would assume you would want to be long-lived.

(In reply to Dave Townsend [:mossop] (he/him) from comment #21)

I'm also not sure that this directory makes sense. From the XDG information it suggests that this directory is meant to be removed after a user logs out and created fresh when they log back in. I'm not sure why that makes it a good place to put user policies that I would assume you would want to be long-lived.

In the specific case of fleet commander, there is a cache of the fleet commander data in /var/lib and the profile output is "compiled" down to downstream specific formats (dconf, some .conf files etc) that then drop in some namespaced /run dir. It makes sense to have /run in your search path for profiles generated at session runtime.

For per-user static profiles my personal opinion is that there should also be a directory in /etc/firefox where you can drop multiple profiles, then you can filter them by using UNIX permissions (i.e. the user can only read the profiles that apply to them), but in any case /run should take precedence over the profile keys in such /etc location.

Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6dde6dcea46a
[Linux] Load policies per user from system, r=mkaply,mossop

Keywords: checkin-needed

The backout appears to be because you added XP_UNIX for GetAppName, but then only call it in XP_WIDGET_GTK

Those need to match.

Thanks, should be OK now.

Flags: needinfo?(stransky)
Keywords: checkin-needed

Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6ac8599ebee3
[Linux] Load policies per user from system, r=mkaply,mossop

Keywords: checkin-needed

Looking at the patch, it still has the MOZ_WIDGET_GTK and XP_UNIX mismatch.

Since Android uses XP_UNIX but doesn't have MOZ_WIDGET_GTK, we're getting this error.

The error is:

[task 2019-10-25T22:44:23.821Z] 22:44:23 ERROR - /builds/worker/workspace/build/src/toolkit/xre/nsXREDirProvider.cpp:108:20: error: unused function 'GetAppName' [-Werror,-Wunused-function]

Looks like I forgot to upload a fixed revision, uploaded now.

Flags: needinfo?(stransky)
Pushed by nbeleuzu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0c34b1340f43 [Linux] Load policies per user from system, r=mkaply,mossop

https://hg.mozilla.org/mozilla-central/rev/0c34b1340f43

Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox72: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 72

Should we uplift this to the ESR?

(In reply to Mike Kaply [:mkaply] from comment #34)

Should we uplift this to the ESR?

Rad Hat ships that as a local patch for ESR so we don't need that, next ESR line is ok for us.

You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: