Hi,

I'm not yet convinced that GPermission is a good idea. Let's rewind a bit and look at things from a 50,000 feet point of view.

The process should be like this

 1. Let's try to clearly agree on the detailed user experience we
    are trying to achieve with GSettings that is related to permissions
    and/or polkit. 

and only then

 2. Sketch out an implementation that uses e.g. polkit

 3. Sketch out the GSettings API

 (with 2. and 3. kinda happening in parallel)

 4. Identify common things and then maybe factor it into a
    GPermission type.

I guess I'm trying to say that designing a GPermission type is like putting the cart before the horse.

I'll take a stab at 1. in the next comment. Stay tuned.

1) The only case I am interested in addressing at this point is the "unlock to edit this dialog" case.  I will do something quite different for the "push these settings to defaults" and I don't think we need a complicated API for this -- certainly it won't involve anything along the lines of a GPermission.

GPermission is strictly for dealing with the non-oneshot case.  Consider it as a model for the polkit 'unlock' button (which we could then have in Gtk).  This allows it to be quite simple.  I don't think we should complicate the design by considering oneshot actions and so on...

Suggested user-experience when it comes to GSettings and permissions / polkit. Take 1.

It is clear that people want to achieve two goals

 1. Make it easy to push settings into system-default and
    system-mandatory context - just like GConf's make-default
    polkit helper currently allows

 2. Make it easy to implement preference dialogs with an
    "Unlock" button that looks like

    http://hal.freedesktop.org/docs/polkit-gtk/polkit-gtk-1-polkitlockbutton.html

Detailed requirements on goal 1:

 1a. People want to enable system admins to easily customize who is
     authorized for the particular action. E.g. the privilege required
     for the desktop background capplet's "Make default" button may
     not be the same as the network manager "Make default" button.

     For example, some admins should be able to specify that the
     desktop background capplet's "make default" button is free for
     all while the NM requires the root password.

     In polkit parlance all this means is that it should be possible
     configure the polkit action id. Then admins can use all the
     existing polkit infrastructure for configuring this.

 1b. People want to customize the message shown in the authentication
     dialog. For example for the desktop background capplet's "Make
     default" button you want to show e.g.

      To change the desktop background, you need to authenticate

     while the NM might be

      To change the Networking Parameters, enter your password

     or similar. A generic stock message like "To change the setting
     please authenticate" is not good enough.

 1c. In some cases you want each app to provide code to override
     the authentication dialog message for the same reasons mentioned
     here

      http://hal.freedesktop.org/docs/polkit/PolkitBackendActionLookup.html

     To allow for this, the Mechanism (e.g. dconf) will have to pass
     details like @path, @context and possibly @key as a PolkitDetails
     object when checking for the authorization / triggering the
     authentication dialog cf.

      http://hal.freedesktop.org/docs/polkit/polkit-1-polkitauthority.html#polkit-authority-check-authorization

I'm waiting a bit to write details about use case 2. Thoughts so far?

Case #2 is not at all what I am trying to solve here.  There will have to be something else for this.

I will try to explain what I see for #2 (which is the use case I am trying to cover with GPermission):

2a: the admin or mechanism authors will still want the same finegrained level of control here.  Unlocking the gdm settings dialog should not provide carte blanche to every setting in the system.

The important thing is that GPermission is obtained from the GSettings object.  The GSettings object is for a specific group of settings, so this allows us to search through PolicyKit (by annotations) for an appropriate matching action ID.  The GPermission that is returned by GSettings is then matching this specific action.  "allowed" is mapped to if this action is currently authorised.  acquire() and release() are mapped to requesting (with user interaction) the action ID and dropping it from the cache of permitted actions.

It is not the case that there is one generic GPermission for "write to any setting in the system".  A GPermission is always associated with a particular GSettings.

It may happen, however, that unlocking a GPermission for one GSettings will have the result of unlocking another.  This is the case when the path annotation in PolicyKit covers both GSettings objects and this may well be desired by the mechanism author or admin.

2b: The customisation of the message that appears in the dialog is left entirely to the admin or the author of the mechanism.  This is given on a per-policy-file basis.  The application should have no control over this.

The application is free to request the ability to modify the "wrong" set of settings (ie: so that a different authentication dialog box is shown) but in this end this is pointless -- the mechanism will be checking that the application has the ability to perform the modifications that it is trying.

2c: I'm not too concerned about details in dialog boxes for blanket authentications (ie: not one-shot) and I think it doesn't really make sense.  The reason that you're requesting the permission in the first place may not be the same as how you intend to use the permission in the future (while you still have it).  It certainly makes sense in the one-shot case that you list the actual keys that will be changed, but for the blanket case it is sufficient to tell the user that they will be modifying "Login Screen Settings".

One thing we might do here in a very limited way is to provide a backend to display the path given in the annotation in the policy file in the details area of the dialog.  This would let techy users see the exact path that is meant by "Login Screen Settings".  I don't consider this to be particularly important, but it may be a nice feature.

(In reply to comment #4)
> Case #2 is not at all what I am trying to solve here.

Of course I meant to say "#1".

One thing to mention that would be very nice: PolicyKit (in its gobject-based library) could provide an implementation of GPermission that you get like this:

PolKitPermission *     polkit_permission_new       (const gchar *action_id);

GSettings would probably use this and pass the created object out via g_settings_get_permission() or whatever I end up calling it.  That would then be passed along to the button in Gtk.

(In reply to comment #4)
> Case #2 is not at all what I am trying to solve here.  There will have to be
> something else for this.
> 
> I will try to explain what I see for #2 (which is the use case I am trying to
> cover with GPermission):

Many thanks, this helps a lot - basically I wanted to make sure that we can show useful dialogs (well, less useless - auth dialogs are mostly always useless..) with precise information.

> It is not the case that there is one generic GPermission for "write to any
> setting in the system".  A GPermission is always associated with a particular
> GSettings.

OK, this makes sense and it's a lot easier to see what you are trying to achieve. It might help to point in SECTION:gpermission that GPermission is an abstract type and that apps can listen to notify::allowed.

Before committing to the name GPermission, it would be useful to think about other places it can be used. Things like ordinary polkit stuff and the 'gvfs-copy some.conf /etc' (cf. bug 617979) comes to mind. I'll try to do that.

The GSettings docs will have to mention that if using polkit-1, they can drop a .policy file in /usr/share/polkit-1/actions with annotations cf. the polkit docs. And ditto for OS X and Win32 implementations if (and when) we add that...

How would gsettings use polkit_permission_new ? do you propose to link gio against polkit ? I was under the impression that the polkit dependency would be confined to dconf ? how will this work eg on win32 ?

i plan to add an API to GSettingsBackend:

  GPermission *     (* get_permission)   (GSettingsBackend   *backend,
                                          const gchar        *path);

then dconf implements this by scanning through polkit annotations for the right path and calling polkit_permission_new() and returning that.

for windows, the registry backend would instead do whatever it needs to do to get clearance to modify registry keys -- probably it would end up having to implement its own GPermission

> i plan to add an API to GSettingsBackend:
> 
>  GPermission *     (* get_permission)   (GSettingsBackend   *backend,
>                                           const gchar        *path);

Ok, that makes some sense


g_permission_impl_update()

Looks very ugly to me, btw, and in no way necessary.

Ugly is really what I was trying for.  Having the word "impl" here is the closest I can get to what would normally be a protected method in other object oriented systems.  I want anybody looking at this function to think "I don't want to touch this".

I don't want to have normal _set_() calls or writable properties because I don't want anybody to think that it's appropriate to set them.

> Having the word "impl" here is the
> closest I can get to what would normally be a protected method

There's multiple other ways to let implementations set the properties, though.

You can make GPermission an interface and only implement the properties in the implementations.

You can keep the abstract base-class, but override the properties in the subclasses.

(In reply to comment #11)
> Ugly is really what I was trying for.  Having the word "impl" here is the
> closest I can get to what would normally be a protected method in other object
> oriented systems.  I want anybody looking at this function to think "I don't
> want to touch this".
> 
> I don't want to have normal _set_() calls or writable properties because I
> don't want anybody to think that it's appropriate to set them.

Isn't it sufficient to say that GPermission subclasses just need to override the properties in this abstract base class and also emit notify::<propname> as appropriate?

GPermission could also be a GInterface and then you'd use g_object_interface_install_property(). Maybe that's nicer. I don't know.

I don't want it to be an interface.  I think that would encourage the wrong sort of behaviour (like maybe I'd be inclined to have GSettings implement it, which I think is wrong).

I considered the idea of having the properties be implemented by the subclass but I wanted to avoid that for several reasons:

  1) We have to decide about how the property gets implemented.
    a) via a getter vfunc
    b) via a normal property override
    c) both

  This gets a little bit ugly.  Particularly, I don't like the case where calling a very fast-looking get_foo() function turns into a GObject property lookup.


  2) Doing it this way ensures that the proper notifications will be sent and also prevents the implementor from trying to cheat (ie: doing the work lazy as the requests come, which would almost certainly involve blocking IO, or also not sending proper notifications).


I should mention that for my PolkitPermissions example above the _new() function would be blocking/cancellable/failable and there would also be an async version.

So, I don't think the ugly bit is a blocker, in general this looks nice and useful.

Two additional points that came up in the hallway discussion between David and me are:

- There should be a default implementation that e.g. the memory backend can return, maybe ? I think it would be nice if the g_settings_get_permission() method always returns an object

- David has this lockdown feature in his polkitlockbutton that would be nice to keep

About lock-down. Typically, the lock button serves two purposes

The first is to allow authorized users to change settings. It goes like this

 - open pref dialog, widgets are insensitve (greyed out)
 - press unlock
 - prove your authorization by authenticating as e.g. the admin or yourself
 - widgets are now sensitive
 - change settings
 - press lock
 - widgets are now insensitive

In this setup the text on the button typically reads

 - Click the lock to change settings
 - Click the lock to prevent changing settings

However this setup assumes that the user doesn't have the permission to start with. Which is not true if the user is e.g. an administrator. In this case you typically want the button to read 

 "Click the lock to prevent other users from changing settings"

Basically what this does is to use polkit's AddLockdownForAction() method to lock down the action, see

  http://hal.freedesktop.org/docs/polkit/eggdbus-interface-org.freedesktop.PolicyKit1.Authority.html#eggdbus-method-org.freedesktop.PolicyKit1.Authority.AddLockdownForAction

If you examine PolkitLockButton, see

 http://hal.freedesktop.org/docs/polkit-gtk/polkit-gtk-1-polkitlockbutton.html

then you will see that this widget attempts to provide this functionality. I'm not 100% happy with the way this currently works but I think a future GtkPermissionButton could make this a lot nicer.

So I think GPermission should add the following

 :locked-down property
 :can-lock-down property
 add_lock_down()    (with _sync, _async, _finish() variants)
 remove_lock_down() (with _sync, _async, _finish() variants)

It would probably be a good idea to think about how GtkPermissionButton should work....

On IRC we decided it would probably be wrong to add the API mentioned in comment 16 and that it would probably be better with a dedicated page in the Account Editor to lock down (and remove lock down from) certain Polkit actions instead. We should however leave ample room in the GPermissionClass struct for expansion in case we want to something like this in the future

Comment on attachment 162703 [details] [review]
GPermission patch

committed with padding for GPermissionClass struct