Bug 41025

(In reply to comment #0)
> Created an attachment (id=51382) [details]
> Proof of concept patch implementing an owner identity.
> 
> The colord daemon wants to use PolicyKit to perform access control on its D-Bus
> API.  While it maintains system-wide state it does not need root privileges to
> operate.
> 
> Currently PolicyKit allows only root to query whether a specific user is
> authorised to perform an action, which means that colord either must run as
> root or can't use PolicyKit for access control.
> 
> The attached patch provides a proof-of-concept implementation of adding an
> “owner” identifier to a PolkitAction.  The idea is that the owner of an action
> (or root) may always query whether a PolkitIdentity is authorised to perform
> the action.
> 
> Is this sort of approach likely to be acceptable?

Not, really, from the patch:
> -                 "(ssssssuuu@a{ss})",
> +                 "(sssssssuuu@a{ss})",

We can't really break D-Bus API like that...

I think we should just add a new annotation, say, org.freedesktop.policykit.allow_check, that is a space-separated string list of identities that can be parsed using polkit_identity_from_string(). See

 http://cgit.freedesktop.org/PolicyKit/commit/?id=6bbd5189e967e8ddc36100bf22cd12bcb152ab5f

for how I just added another annotation.

(In reply to comment #1)
> (In reply to comment #0)
> > Created an attachment (id=51382) [details] [details]
> > Proof of concept patch implementing an owner identity.
> > 
> > The colord daemon wants to use PolicyKit to perform access control on its D-Bus
> > API.  While it maintains system-wide state it does not need root privileges to
> > operate.
> > 
> > Currently PolicyKit allows only root to query whether a specific user is
> > authorised to perform an action, which means that colord either must run as
> > root or can't use PolicyKit for access control.
> > 
> > The attached patch provides a proof-of-concept implementation of adding an
> > “owner” identifier to a PolkitAction.  The idea is that the owner of an action
> > (or root) may always query whether a PolkitIdentity is authorised to perform
> > the action.
> > 
> > Is this sort of approach likely to be acceptable?
> 
> Not, really, from the patch:
> > -                 "(ssssssuuu@a{ss})",
> > +                 "(sssssssuuu@a{ss})",
> 
> We can't really break D-Bus API like that...
> 
> I think we should just add a new annotation, say,
> org.freedesktop.policykit.allow_check, that is a space-separated string list of
> identities that can be parsed using polkit_identity_from_string(). See
> 
> 
> http://cgit.freedesktop.org/PolicyKit/commit/?id=6bbd5189e967e8ddc36100bf22cd12bcb152ab5f
> 
> for how I just added another annotation.

Oh, great.  I initially thought of doing this through annotations, but it looked like they were reserved for client use rather than for changing the behaviour of polkit.  That'll be much easier.  I should be able to whip up a patch for this in the next couple of days.

(In reply to comment #2)
> Oh, great.  I initially thought of doing this through annotations, but it
> looked like they were reserved for client use rather than for changing the
> behaviour of polkit.  That'll be much easier.  I should be able to whip up a
> patch for this in the next couple of days.

I'm planning to do a release this week - so if you want this to be in the release please submit a patch before Friday. Thanks!

(In reply to comment #3)
> (In reply to comment #2)
> > Oh, great.  I initially thought of doing this through annotations, but it
> > looked like they were reserved for client use rather than for changing the
> > behaviour of polkit.  That'll be much easier.  I should be able to whip up a
> > patch for this in the next couple of days.
> 
> I'm planning to do a release this week - so if you want this to be in the
> release please submit a patch before Friday. Thanks!

I'm now testing a patch locally.  Fingers crossed, should be good to go tomorrow.

Created attachment 51737 [details] [review]
Add org.freedesktop.policykit.owner annotation

Here's a respun patch implementing this behaviour via the org.freedesktop.policykit.owner annotation.  Sorry for the delay.

Thanks!

Sorry for being slow with this, will merge it later today

OK, sorry for the lag, I finally got around to looking at this.

The patch looks correct but it seemed a little wasteful insofar that we look up the action description and build the owners_of_action list. I've committed a slightly simpler patch that also does the checking in a separate subroutine:

 http://cgit.freedesktop.org/PolicyKit/commit/?id=5cd68a3aa8d5d0fdbbd3baef0601350bd43a0e4d

I've tested that it works properly (using the pkcheck(1) command and hand-editing .policy files) but please check if it works for you as well. Thanks!