Created attachment 305997 [details] [review]
checks for u:$USERNAME named profile in /etc/dconf/profile if no $DCONF_PROFILE is present and before checking for 'user'

I just noticed that my previous patch would create a problem if a system has a user named 'user'.

I'm using the prefix "u:" on this one.

Review of attachment 305997 [details] [review]:

This is a fine idea, I think, but I'm concerned about the amount of stat() that this could add...

One note: the "u:" thing is seriously uncool.  "user:" would be better.

Review of attachment 305997 [details] [review]:

also: we could try to make something in the very early session (or gdm?) set DCONF_PROFILE to the appropriate value instead.  I'd vastly prefer that.

Ray suggested dropping a file in the XDG_RUNTIME_DIR and using that instead of an environment variable.  I share his desire to minimise the environment.

A tweak on that idea, though: we could just make XDG_RUNTIME_DIR/dconf-profile a symlink to whatever profile file we find.

This could easily be done from a PAM module, somewhere near the systemd/logind xdg runtime dir setup.  It could also possibly be added to logind as part of some sort of framework to care for the initial population of the runtime dir when it is created.

Created attachment 306727 [details]
pam_dconf.c

a demo of what a pam_dconf module could look like

(In reply to Ryan Lortie (desrt) from comment #5)
> Created attachment 306727 [details]
> pam_dconf.c
> 
> a demo of what a pam_dconf module could look like

nice stuff! a few questions and a remark:

besides populating  get_dconf_profile_path (void), what else is missing here? (as I said, I never quite understood pam, but I'm willing to work on this full time next week, I'm picturing what's needed)

I'm guessing it'd be a matter of stuffing this in the dconf project and reusing dconf_engine_open_profile_file()?

Created attachment 307433 [details] [review]
engine: if a dconf.profile file or link is placed in XDG_RUNTIME_DIR that takes precedence over other profile files

Created attachment 307434 [details] [review]
pam: implementation of a pam module to find locate and link a profile for a specific user from XDG_DATA_DIRS onto XDG_RUNTIME_DIR

These two patches are pending test cases that I'm planning to work on, but it doesn't hurt if you take a look at the implementation in the meantime.

I've leaned towards using USERNAME.profile instead of user:USERNAME given that colons have to be escaped on shell comletion. Same thing for the file placed in XDG_RUNTIME_DIR, dconf.profile now instead of dconf-profile for the sake of symmetry.

Ryan, I've noticed that you are avoiding libtool usage, any feedback on the Makefile.am stuff I'm using to produce the .so would be specially welcome.

Any feedback? Is there anything in this patch that I should be changing? Should I just go ahead and write tests cases and docs?

Review of attachment 307433 [details] [review]:

Other than the complaints below, looks fine to me.

::: engine/dconf-engine-profile.c
@@ +226,3 @@
 
+static FILE *
+dconf_engine_get_runtime_profile () {

This should be (void). And please put the opening { on the next line.

@@ +228,3 @@
+dconf_engine_get_runtime_profile () {
+  gchar *filename = g_build_filename (g_get_user_runtime_dir (), "dconf.profile", NULL);
+  FILE  *fp       = fopen (filename, "r");

I don't know how Ryan feels about it, but I personally prefer to split declarations and functions calls, so I would prefer to see filename and fp declared separately.

@@ +231,3 @@
+
+  if (fp == NULL)
+    g_info ("Unable to open %s: %s", filename, g_strerror (errno));

Not sure if you really want to log this if the file doesn't exist. Thats an entirely normal case, right ?

Review of attachment 307434 [details] [review]:

::: pam/pam_dconf.c
@@ +1,3 @@
+/*
+ * Copyright © 2012 Canonical Limited
+ * Copyright © 2015 Red Hat Limited

Again: s/Limited/Inc/

@@ +26,3 @@
+              char         *base,
+              char         *suffix,
+              char         *file)

This should probably take const char * arguments, looking at the casting thats going on below...

@@ +35,3 @@
+  }
+
+  if (sprintf(result, "%s%s%s", base, suffix, file) < 0)

Space before (. It is a bit of a gratitious use of printf - you could just use strcpy

@@ +38,3 @@
+    {
+      pam_syslog (pamh, LOG_ERR, "There was an error calling sprintf");
+      free(result);

Space before (

@@ +48,3 @@
+{
+  const char *user;
+  int         ret = pam_get_user (pamh, &user, "");

I would prefer to split declaration and function call here.

@@ +93,3 @@
+
+  if (pam_getenv (pamh, "XDG_DATA_DIRS") != NULL)
+    dirs = strdup (pam_getenv (pamh, "XDG_DATA_DIRS"));

There's an ordering constraint here that needs to be documented: if XDG_DATA_DIRS gets set by pam-env, this module needs to be put below pam-env in the stack.

@@ +95,3 @@
+    dirs = strdup (pam_getenv (pamh, "XDG_DATA_DIRS"));
+  else
+    dirs = strdup ("/usr/local/share:/usr/share");

Given that you are using strtok below, I don't think you need to dup this string at all.

@@ +116,3 @@
+
+  if (result == NULL)
+    pam_syslog (pamh, LOG_NOTICE, "Could not find a dconf profile candidate for this user");

This is entirely normal - do we really want to log it ?

@@ +135,3 @@
+  bool        success = 0;
+
+  runtime_dir_path = pam_getenv (pamh, "XDG_RUNTIME_DIR");

This makes me wonder: Can we rely on XDG_RUNTIME_DIR being set here ? It does get set by pam-systemd, which is run pretty late in the pam stack.

At the very least, this ordering constraint will have to be clearly documented.

@@ +139,3 @@
+  if (runtime_dir_path == NULL)
+    {
+      pam_syslog(pamh, LOG_NOTICE, "XDG_RUNTIME_DIR has not been set yet.  Cannot set up dconf profile.");

Space before (

@@ +146,3 @@
+  if (dconf_profile_path == NULL)
+    {
+       pam_syslog(pamh, LOG_NOTICE, "Could not find a dconf profile");

Space before (

@@ +156,3 @@
+  if (symlink_path == NULL)
+    {
+      pam_syslog(pamh, LOG_NOTICE, "Could not allocate memory for dconf profile symlink path");

join_strings already logs allocation failure.

::: pam/pam_dconf.h
@@ +1,2 @@
+/*
+ * Copyright © 2015 Red Hat Limited

This should be either "Alberto Ruiz" or "Red Hat, Inc". There's no such thing as Red Hat Limited.

Apart from tests, whats missing here is a documentation patch. There's a section on profiles in docs/dconf-overview.xml that needs updating to explain how this works.

(In reply to Matthias Clasen from comment #12) 
> @@ +35,3 @@
> +  }
> +
> +  if (sprintf(result, "%s%s%s", base, suffix, file) < 0)
> 
> Space before (. It is a bit of a gratitious use of printf - you could just
> use strcpy


Looking into this... I don't quite get how this is gratuitous, with strcpy I would have to do some extra steps to take into account the offsets of each one of the three strings, each null byte... this is a lot simpler. Let me know if there's anything I'm missing with regards to this suggestion that would make the code simpler/better with three calls to strcpy + the pointer arithmetics.

(In reply to Matthias Clasen from comment #12)
> @@ +95,3 @@
> +    dirs = strdup (pam_getenv (pamh, "XDG_DATA_DIRS"));
> +  else
> +    dirs = strdup ("/usr/local/share:/usr/share");
> 
> Given that you are using strtok below, I don't think you need to dup this
> string at all.

strtok modifies the string, if I use it with the static string it will segfault

(In reply to Matthias Clasen from comment #12)
> @@ +116,3 @@
> +
> +  if (result == NULL)
> +    pam_syslog (pamh, LOG_NOTICE, "Could not find a dconf profile candidate
> for this user");
> 
> This is entirely normal - do we really want to log it ?

I think this could be useful to give certainty to the sysadmin that the module is not finding anything while figuring out what is wrong if he's expecting the profile to be selected. I'll switch to LOG_DEBUG for now and if you think I should still remove it I'll get rid of it.

(In reply to Matthias Clasen from comment #12)
> Review of attachment 307434 [details] [review] [review]: 
> @@ +93,3 @@
> +
> +  if (pam_getenv (pamh, "XDG_DATA_DIRS") != NULL)
> +    dirs = strdup (pam_getenv (pamh, "XDG_DATA_DIRS"));
> 
> There's an ordering constraint here that needs to be documented: if
> XDG_DATA_DIRS gets set by pam-env, this module needs to be put below pam-env
> in the stack.
> 
> @@ +135,3 @@
> +  bool        success = 0;
> +
> +  runtime_dir_path = pam_getenv (pamh, "XDG_RUNTIME_DIR");
> 
> This makes me wonder: Can we rely on XDG_RUNTIME_DIR being set here ? It
> does get set by pam-systemd, which is run pretty late in the pam stack.
> 
> At the very least, this ordering constraint will have to be clearly
> documented.

I've added a README.pam file documenting this, I'm not sure if there's a better place for system integrators/packagers to notice. I'm not sure this belongs to the dconf manual, I'm open to suggestions.

(In reply to Matthias Clasen from comment #11)
> @@ +231,3 @@
> +
> +  if (fp == NULL)
> +    g_info ("Unable to open %s: %s", filename, g_strerror (errno));
> 
> Not sure if you really want to log this if the file doesn't exist. Thats an
> entirely normal case, right ?

Yup, you're right on this one too, I'll switch to using g_debug here unless you think removing this altogether makes more sense.

Created attachment 312852 [details] [review]
pam: implementation of a pam module to find locate and link a profile for a specific user from XDG_DATA_DIRS onto XDG_RUNTIME_DIR

includes fixes for the review comments above

Added README.pam to document how to deploy the pam module.

Created attachment 312853 [details] [review]
engine: if a dconf.profile file or link is placed in XDG_RUNTIME_DIR that takes precedence over other profile files

fixes issues stated in the patch review

I've updated the patches with the comments from Matthias, I will start working on tests and documentation tomorrow.

(In reply to Alberto Ruiz from comment #14)
> (In reply to Matthias Clasen from comment #12) 
> > @@ +35,3 @@
> > +  }
> > +
> > +  if (sprintf(result, "%s%s%s", base, suffix, file) < 0)
> > 
> > Space before (. It is a bit of a gratitious use of printf - you could just
> > use strcpy
> 
> 
> Looking into this... I don't quite get how this is gratuitous, with strcpy I
> would have to do some extra steps to take into account the offsets of each
> one of the three strings, each null byte... this is a lot simpler. Let me
> know if there's anything I'm missing with regards to this suggestion that
> would make the code simpler/better with three calls to strcpy + the pointer
> arithmetics.

Nevermind. If you were using glib, the obvious alternative would be g_strconcat. But you don't, so ignore this.

(In reply to Alberto Ruiz from comment #15)
> (In reply to Matthias Clasen from comment #12)
> > @@ +95,3 @@
> > +    dirs = strdup (pam_getenv (pamh, "XDG_DATA_DIRS"));
> > +  else
> > +    dirs = strdup ("/usr/local/share:/usr/share");
> > 
> > Given that you are using strtok below, I don't think you need to dup this
> > string at all.
> 
> strtok modifies the string, if I use it with the static string it will
> segfault

Hmm, you are right. That seems like an argument for doing it manually, but nevermind.

Created attachment 312947 [details] [review]
pam: implementation of a pam module to find locate and link a profile for a specific user from XDG_DATA_DIRS onto XDG_RUNTIME_DIR

Fix indentation of certain if blocks

Created attachment 313184 [details] [review]
pam: Add test suite for the pam module

This patch adds a test suite for the pam module

Created attachment 313185 [details] [review]
engine: add test case to check if engine picks up $XDG_RUNTIME_DIR/dconf.profile as default profile in the abscence of DCONF_PROFILE

this patch adds a test case for the engine code, makes sure the engine picks up the dconf.profile file from XDG_RUNTIME_DIR

Created attachment 313306 [details] [review]
docs: add docs about new PAM module for per-user profiles

This patch adds docs about the new PAM module for per-user profiles

Review of attachment 312853 [details] [review]:

Looks fine except the filename.

::: engine/dconf-engine-profile.c
@@ +230,3 @@
+  FILE  *fp;
+
+  filename = g_build_filename (g_get_user_runtime_dir (), "dconf.profile", NULL);

Let's assume that one day we want to do dynamic profile switching on a logged-in session.  There's no reason that couldn't work.

If we do it like this, however, we will need to watch $XDG_RUNTIME_DIR for creation of the file.  This could be expensive because we will receive notification every time anything is created here -- in every app.

We could avoid that with some indirection: make a directory here and watch that directory.

Just like for reading the database, the application-side could create the directory if it does not exist (avoiding the problem of needing to watch for creation of the directory itself).

So let's say RUNTIME_DIR/dconf/profile.

While we're at it, let's move the shm files to a different location: RUNTIME_DIR/dconf/shm-flag or so.  This will be annoying for jhbuild but we could maybe mitigate the problem by some symlinks or something...

Review of attachment 312853 [details] [review]:

On second thought, we might want to entertain the possibility of hosting this file outside of a place that the user has write access to...  Maybe /run/dconf/$(uid)/profile or something similar.  Maybe the uid could be the symlink itself.

That means that we'd need to 'up' our technology level a bit because we'd now be responsible for the cleanup (since it wouldn't get taken away with the runtime dir).

Created attachment 313724 [details] [review]
engine: add support for runtime profile selection

Add support to dconf-engine for opening "runtime" profiles.

These profiles are intended to be symbolic links or plain files that
will live either in XDG_RUNTIME_DIR/dconf/profile or
/run/dconf/user/$(uid).

This is intended to allow for a PAM module that makes complex decisions
about application of a specific policy to a user and sets up the profile
at login time, thus preventing the need for this complex decision to be
a part of every program that uses dconf.  This PAM module would not be
part of dconf, but would rather be a part of a dconf-aware system
administrator framework.

In the case that the profile file is found in /run/dconf, then it will
not be possible for the user to override the profile selection,
including via the DCONF_PROFILE environment variable.  This provides a
mechanism for lockdown that is slightly more difficult for a user to
circumvent.  In theory, this is pointless since it can still be defeated
with LD_PRELOAD, but in practice this raises the bar quite a bit.

(In reply to Ryan Lortie (desrt) from comment #30)
> Created attachment 313724 [details] [review] [review]
> engine: add support for runtime profile selection
> 
> Add support to dconf-engine for opening "runtime" profiles.
> 
> These profiles are intended to be symbolic links or plain files that
> will live either in XDG_RUNTIME_DIR/dconf/profile or
> /run/dconf/user/$(uid).
> 
> This is intended to allow for a PAM module that makes complex decisions
> about application of a specific policy to a user and sets up the profile
> at login time, thus preventing the need for this complex decision to be
> a part of every program that uses dconf.  This PAM module would not be
> part of dconf, but would rather be a part of a dconf-aware system
> administrator framework.
> 
> In the case that the profile file is found in /run/dconf, then it will
> not be possible for the user to override the profile selection,
> including via the DCONF_PROFILE environment variable.  This provides a
> mechanism for lockdown that is slightly more difficult for a user to
> circumvent.  In theory, this is pointless since it can still be defeated
> with LD_PRELOAD, but in practice this raises the bar quite a bit.

This behaviour cuts it perfectly for the Fleet Commander use case, so I'll be more than happy to see this in master.

I wonder what are the implications of this patch for xdg-apps...

Attachment 313724 [details] pushed as 4ef5a2a - engine: add support for runtime profile selection

OK.  That's it, then.