On Thu, Apr 24, 2014 at 11:48:47AM +0200, Pavel Březina wrote:
On 04/23/2014 10:43 PM, Jakub Hrozek wrote:
>Hi,
>
>the attached patches upstream functionality that mod_lookup_identity has
>been using for a while, but which we couldn't push to master due to the
>pending sbus changes. Now that these are accepted, it's time to merge
>the DBus methods themselves. Previously, mod_lookup_identity relied on a
>private branch with quite a bit of ad-hoc code.
>
>Please note that when the work in progress to expose users, groups and
>domains is merged as well, this interface will become obsolete in favor
>of the objects. But we shouldn't let a released version of
>mod_lookup_identity rely on unreleased sssd code any longer. Moreover,
>all patches except the last one will be used even later on and there's
>no fixed timeline on converting mod_lookup_identity.
>
>[PATCH 1/6] SBUS: two trivial style fixes
>SSIA
>
>[PATCH 2/6] SBUS: Add a convenience function
>Adds a convenience function that constructs a DBusError internally and
>as such can be used to mark an sbus request as failed without having
>to create a DBusError instance by the caller. The error message must be
>either allocated on top of dbus_req or a constant, DBus itself doesn't
>copy the error message! This is reflected in the doxygen docstring.
>
>[PATCH 3/6] IFP: Add utility functions
>Adds a number of utility functions, most importanly ifp_req_create().
>The ifp_req is a structure that will be passed along with the ifp
>request and contains the sbus request and some ifp-specific data, like
>caller ID or the responder context.
>
>[PATCH 4/6] IFP: use a list of allowed_uids for authentication
>Similar to the PAC responder, the InfoPipe uses a list of UIDs that are
>allowed to communicate with the IFP responder.
>
>[PATCH 5/6] IFP: Initialize negative cache timeout
>In order to avoid hitting the back end with repetitive requests, the
>InfoPipe responder needs a negative cache, too. This patch follows the
>convention set by other responders, where the negative cache timeouts are
>read from the [nss] section. This is not ideal, however, and ticket #2318
>tracks moving the configuration to the [ifp] section primarily.
>
>The timeout is also a separate parameter in the NSS context. We should
>consider moving it to the negcache context instead (#2317).
>
>[PATCH 6/6] IFP: Add GetUserAttrs call
>Adds a DBus method that allows the caller to retrieve attributes of a
>user. The synopsis of the call is as follows:
> <method name="GetUserAttr">
> <arg type="s" name="user"
direction="in"/>
> <arg type="as" name="attr"
direction="in"/>
> <arg type="a{sv}" name="values"
direction="out"/>
> </method>
>
>The return value is an array (one attribute per array member) of
>dictionaries. The key of the dictionary is the attribute name, the value
>is a variant containing the attribute values as strings.
>
>If an attribute does not exist or is not permitted to be read, no error
>is returned. If the users does not exist, the method returns an error.
>
>In future patches this function will be marked as obsolete in favor of
>object-oriented approach.
>
>ifp_user_get_attr_unpack_msg is a separate function to allow extending
>it in a later patch.
>
>The function to check the cache validity duplicates quite a bit of code
>with the NSS responder. The refactoring would be nice to get done along
>with #843. I have a patch in my private branch, but it's not complete,
>sorry.
Patch 1:
Ack.
Patch 2:
Ack.
Patch 3:
Ack.
Patch 4:
Ack.
Patch 5:
Ack.
Patch 6:
Ack.
Only the commit subject needs to be fixed. You use Attrs there,
however, the method uses singular.
User attributes are correctly returned. Allowed_uids option works.
I would just like to propose two more patches:
* IFP: indentation fixes
Just spaces.
* IFP: report error from ifp_req_create()
This patch makes d-bus error reporting more meaningful.
Instead of generic:
Error org.freedesktop.DBus.Error.Failed: Could not create ifp_req
You get:
Error org.freedesktop.DBus.Error.Failed: Permission denied
when user is not found in ACL.
Thanks, I merged your patches in, the first one pretty much verbatim,
the second one with modifications -- I named the variable 'ireq' not
'req', sine 'req' should be mostly used for tevent requests and fixed
some bugs in your version of ifp_req_create, in paricular the ireq was
freed unconditionally in the done handler :-)
I also fixed some bugs Jan P. found when testing the request with his
modules. So far I've pushed the whole patchset into my private branch,
feel free to check them out. I'm still working on Stef's suggestions.