After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 512494 - Configuration Editor should use PolicyKit to set mandatory and default keys without restarting
Configuration Editor should use PolicyKit to set mandatory and default keys w...
Status: RESOLVED WONTFIX
Product: gconf-editor
Classification: Applications
Component: general
unspecified
Other All
: Normal enhancement
: ---
Assigned To: Gconf Editor Maintainers
Gconf Editor Maintainers
gnome[unmaintained]
Depends on: 558619
Blocks:
 
 
Reported: 2008-01-28 05:39 UTC by Dylan McCall
Modified: 2018-08-17 18:41 UTC
See Also:
GNOME target: 2.32.x
GNOME version: Unversioned Enhancement


Attachments
Beginning of new world order (22.16 KB, patch)
2008-10-30 05:17 UTC, Vincent Untz
reviewed Details | Review
Updated patch (23.41 KB, patch)
2008-10-30 16:45 UTC, Vincent Untz
committed Details | Review
Part 2 (13.32 KB, patch)
2008-10-31 00:16 UTC, Vincent Untz
needs-work Details | Review
Updated part 2 patch (20.93 KB, patch)
2008-10-31 17:31 UTC, Vincent Untz
needs-work Details | Review
depend on polkit-dbus only if present (4.62 KB, patch)
2009-04-20 02:48 UTC, Yaakov Selkowitz
none Details | Review

Description Dylan McCall 2008-01-28 05:39:54 UTC
A neat feature in gconf is the ability to set default and mandatory keys. Particularly handy when I want to set a specific list of random screensavers, but gnome-screensaver keeps putting it back to its own list of every screensaver. I can change the list manually, set it as a mandatory key and never worry again!

Sadly, that is not so, because setting a key as mandatory or default requires root privileges. The problem with running "sudo gconf-edit" is that now all those keys I had set for my local user are gone; instead, all I can see is the keys set by the root user.
Thus, that function is almost useless.

At the moment, gconf-edit demands that I be running as root in order to set a key as mandatory or default. It would be very good if we were instead greeted with an authorization dialog, ultimately leading to that key being set as mandatory very quickly and easily thanks to the magic of PolicyKit.
Comment 1 Vincent Untz 2008-10-30 05:17:23 UTC
Created attachment 121621 [details] [review]
Beginning of new world order

Here's a patch that makes the items in the popup menu work with the gconf policykit helper.

Note that it doesn't help for showing a window with only default/mandatory settings, as available in the file menu. But I think we could still commit this for now. Opinions?

(the patch also contains the patch from bug 558483)
Comment 2 Cosimo Cecchi 2008-10-30 08:58:17 UTC
Comment on attachment 121621 [details] [review]
Beginning of new world order

The patch looks mostly fine to me, I think we should get this in anyway as a first step for PolicyKit integration.
Some comments on the patch:

>Index: src/gconf-policykit.c

Please don't mix tabs and spaces inside this file.

>+ * Copyright (C) 2007 David Zeuthen <david@fubar.dk>

Is DavidZ really the author of the code? :D

>+
>+typedef struct {
>+	gint ref_count;
>+        gchar *call;

I think this should be const char *call.

>+	g_debug ("helper refused; returned polkit_result='%s' and polkit_action='%s'",
>+		 result, action);

Please remove the debug output.

>+
>+	data = g_new0 (GConfPKCallbackData, 1);
>+	data->ref_count = 1;
>+	data->call = "SetSystem";

If you do ref_count++ in set_key_async (), shouldn't you set ref_count to 0 here? Otherwise the data won't get free'd after the call?

>+	data = g_new0 (GConfPKCallbackData, 1);
>+	data->ref_count = 1;
>+	data->call = "SetMandatory";

Same as before.

>Index: src/gconf-policykit.h

You seem to miss
#ifndef __GCONF_POLKIT_H__
#define __GCONF_POLKIT_H__
...
#endif
Comment 3 Vincent Untz 2008-10-30 15:39:56 UTC
(wasn't cc'ed on the bug)
Comment 4 Vincent Untz 2008-10-30 15:43:47 UTC
(In reply to comment #2)
> >+ * Copyright (C) 2007 David Zeuthen <david@fubar.dk>
> 
> Is DavidZ really the author of the code? :D

Mostly, yes, since it's based on code from the clock applet he wrote. So I'm even able to say that all the problems you raised are his fault :-) I'll fix stuff now.
Comment 5 Vincent Untz 2008-10-30 16:45:33 UTC
Created attachment 121668 [details] [review]
Updated patch

This should fix all the raised issues. Note that the ref stuff is right. I just changed a bit the code to make it clearer (with ref/unref functions).
Comment 6 Vincent Untz 2008-10-30 16:49:52 UTC
Forgot the ChangeLog:

2008-10-30  Vincent Untz  <vuntz@gnome.org>

	Add some preliminary PolicyKit support for setting default/mandatory
	values. This only works with the popup context menu because of
	limitations of the API provided by the gconf PolicyKit helper.
	Part of bug #512494.

	* configure.in: depend on polkit-dbus and dbus-glib-1
	* src/Makefile.am:
	* src/gconf-policykit.[ch]: add new files
	* src/gconf-editor-window.c:
	(gconf_editor_popup_policykit_callback): new, callback used when
	calling the PolicyKit asynchronous API.
	(gconf_editor_popup_window_set_as_default),
	(gconf_editor_popup_window_set_as_mandatory): rework to handle both
	direct write and write through PolicyKit
	(list_view_button_press_event),
	(gconf_editor_window_list_view_popup_menu): update to have the right
	sensitivity of popup menu items with the new PolicyKit stuff.

Okay to commit?
Comment 7 Cosimo Cecchi 2008-10-30 16:54:54 UTC
Thanks for the update, looks good :)
Comment 8 Vincent Untz 2008-10-30 17:02:55 UTC
Cool, committed.

Now, I'm not sure what we can do for the windows with mandatory/default settings. The issue is that the API from the PolicyKit helper does not accept values: we can "only" copy a key from the user gconf db to the system gconf db. It kind of makes sense, since providing an API over dbus to handle all the gconf types might not be fun (although it's probably possible).

So I can imagine a hack around this: we read the mandatory/default settings with GConfEngine as we do right now, but then, when making a change, we change it locally first, do the PK magic, and reset the local value to what we had before.

That's horribly hacky, but I don't know if something else is possible.
Comment 9 Cosimo Cecchi 2008-10-30 17:24:07 UTC
(In reply to comment #8)

> Now, I'm not sure what we can do for the windows with mandatory/default
> settings. The issue is that the API from the PolicyKit helper does not accept
> values: we can "only" copy a key from the user gconf db to the system gconf db.
> It kind of makes sense, since providing an API over dbus to handle all the
> gconf types might not be fun (although it's probably possible).
> 
> So I can imagine a hack around this: we read the mandatory/default settings
> with GConfEngine as we do right now, but then, when making a change, we change
> it locally first, do the PK magic, and reset the local value to what we had
> before.
> 
> That's horribly hacky, but I don't know if something else is possible.

I understand the problem, but I don't like the solution, because it will cause unnecessary pain for all the applications that are notified by the GConf key value change. This way, they would get notified twice for an (useless) value.
Another (quite hackish too) way to solve the problem could be having a PK wrapper over gconftool-2 and transform ourselves the value into a sensible string.
Comment 10 Vincent Untz 2008-10-30 17:29:17 UTC
(In reply to comment #9)
> I understand the problem, but I don't like the solution, because it will cause
> unnecessary pain for all the applications that are notified by the GConf key
> value change. This way, they would get notified twice for an (useless) value.

Nod.

> Another (quite hackish too) way to solve the problem could be having a PK
> wrapper over gconftool-2 and transform ourselves the value into a sensible
> string.

Well, if we serialize the value in a string, we don't need to use a wrapper around gconftool-2: we can simply add a new dbus method to the PK helper. Sounds like a plan :-)
Comment 11 Vincent Untz 2008-10-30 17:31:06 UTC
Bah, this is too easy: gconf_value_decode/gconf_value_encode.
Comment 12 Cosimo Cecchi 2008-10-30 18:33:18 UTC
(In reply to comment #10)

> Well, if we serialize the value in a string, we don't need to use a wrapper
> around gconftool-2: we can simply add a new dbus method to the PK helper.
> Sounds like a plan :-)

Are you taking care of the GConf patch as well?
Please post the bug number here when you come up with a patch :)
Comment 13 Vincent Untz 2008-10-31 00:16:45 UTC
Created attachment 121704 [details] [review]
Part 2

The gconf part is bug 558619. I've tried this patch in gconf-editor, but unfortunately, I can't really test it: I get empty windows for default or mandatory settings (this is unrelated to the patch, since I have the same issue without it, it seems).

Hrm weird.
Comment 14 Cosimo Cecchi 2008-10-31 14:27:17 UTC
I committed some fixes to gconf-editor trunk which might me related to this. Unfortunately, I can't manage to get PolicyKit and DBus running in jhbuild so I can't try the GConf patch, but I don't have empty windows issues, so this might be related to your GConf installation.
Comment 15 Vincent Untz 2008-10-31 14:47:59 UTC
I plugged my brain again. The issue is that most distros have more than one source for default configuration. You typically have (with different names):

 /etc/gconf/gconf.xml.upstream: upstream defaults
 /etc/gconf/gconf.xml.vendor: distro defaults
 /etc/gconf/gconf.xml.local: local defaults set by the sysadmin

I was seeing nothing since I was working on /etc/gconf/gconf.xml.local which is empty.

So we need a way to have gconf-editor "merge" those default sources. Hrm. gconf_engine_get_for_addresses() is our friend :-) Just need a way to know how to have those addresses. Any idea?

(maybe we could parse /etc/gconf/2/path and keep the last readonly addresses that are system-wide?)
Comment 16 Vincent Untz 2008-10-31 17:31:49 UTC
Created attachment 121738 [details] [review]
Updated part 2 patch

Okay, this kind of works now (if you ignore the cool hardcoding for sources).

The main issue, though, is that we don't get notified about the changes, so what's displayed in gconf-editor doesn't change at all. I'm not sure it's easy to fix :/
Comment 17 Cosimo Cecchi 2008-10-31 19:20:18 UTC
(In reply to comment #15)
 
> So we need a way to have gconf-editor "merge" those default sources. Hrm.
> gconf_engine_get_for_addresses() is our friend :-) Just need a way to know how
> to have those addresses. Any idea?
> 
> (maybe we could parse /etc/gconf/2/path and keep the last readonly addresses
> that are system-wide?)

GConf internally already provides a parser, which we could just make public: gconf_load_source_path (), which returns a list of all the addresses in the configuration file.
What I can't find is a way to determine if the source is for mandatory or default values.
Comment 18 Vincent Untz 2008-11-01 00:43:17 UTC
(In reply to comment #17)
> What I can't find is a way to determine if the source is for mandatory or
> default values.

Mandatory = xml:readonly before the first xml:readwrite
defaults = xml:readonly after the last xml:readwrite
Comment 19 Cosimo Cecchi 2008-11-01 01:08:49 UTC
(In reply to comment #18)

> Mandatory = xml:readonly before the first xml:readwrite
> defaults = xml:readonly after the last xml:readwrite

Hmm...then how are we supposed to parse that something like "xml:readonly:/etc/gconf/gconf.xml.mandatory" is mandatory vs. e.g. "xml:readonly:/etc/gconf/gconf.xml.defaults"? They both have only a xml:readonly and no xml:readwrite.
Anyway, if that would be the rule, it should be as simple as c/p-ing gconf_load_source_path () (or making it public, though I don't know if it really makes sense outside of this use-case) and parse each address as mandatory or default to get the GConfEngine.
Comment 20 Vincent Untz 2008-11-01 13:23:17 UTC
(In reply to comment #19)
> (In reply to comment #18)
> 
> > Mandatory = xml:readonly before the first xml:readwrite
> > defaults = xml:readonly after the last xml:readwrite
> 
> Hmm...then how are we supposed to parse that something like
> "xml:readonly:/etc/gconf/gconf.xml.mandatory" is mandatory vs. e.g.
> "xml:readonly:/etc/gconf/gconf.xml.defaults"? They both have only a
> xml:readonly and no xml:readwrite.

You can't know if you don't have the whole list of addresses. That's why you need to parse /etc/gconf/2/path and to know the relative positions vs the xml:readwrite item. If there's no xml:readwrite item in there, then mandatory = defaults.
Comment 21 paul 2008-12-14 23:17:45 UTC
could this be made to have a --disable-polkit option in configure?
Comment 22 Yaakov Selkowitz 2009-04-20 02:43:42 UTC
(In reply to comment #21)
> could this be made to have a --disable-polkit option in configure?

Alternatively, PolicyKit usage could be made dependent on its presence.  I'll attach a patch which I used to build 2.26.0 without it. 

Comment 23 Yaakov Selkowitz 2009-04-20 02:48:42 UTC
Created attachment 132934 [details] [review]
depend on polkit-dbus only if present

* configure.in:
* src/Makefile.am:
* src/gconf-editor-window.c:
* src/gconf-policykit.c: Use PolicyKit only if available, bug 512494.
Comment 24 Cosimo Cecchi 2009-05-06 14:09:19 UTC
I committed an equivalent patch for optional PolicyKit support, from bug 579096.

Vincent, now that the GConf bit it in, could you please update your patch to remove the hardcoding of sources (maybe by either making gconf_load_source_path() public or c/p some bits of it) so that we can complete the support for 2.28?
Comment 25 Cosimo Cecchi 2009-08-24 09:09:00 UTC
Comment on attachment 121738 [details] [review]
Updated part 2 patch

This needs some more love before being committed now that we use PolicyKit 1.0.
Comment 26 André Klapper 2018-08-17 18:41:10 UTC
GConf has been deprecated since 2011.
Hence GConf-Editor is not under active development anymore. Its codebase has been archived: https://gitlab.gnome.org/Archive/gconf-editor/commits/master

GConf's successors are dconf and gsettings. dconf-editor exists.

Closing this report as WONTFIX as part of Bugzilla Housekeeping to reflect
reality. Feel free to open a task in GNOME Gitlab if the issue described in
this task still applies to dconf-editor (not: gconf-editor). Thanks!