On Tue, Nov 04, 2014 at 07:07:11PM +0100, Jakub Hrozek wrote:
On Tue, Nov 04, 2014 at 07:02:34PM +0100, Lukas Slebodnik wrote:
> On (04/11/14 18:41), Lukas Slebodnik wrote:
> >On (04/11/14 18:09), Sumit Bose wrote:
> >>On Tue, Nov 04, 2014 at 12:17:43PM +0100, Jakub Hrozek wrote:
> >>> On Wed, Oct 29, 2014 at 01:01:43PM +0100, Sumit Bose wrote:
> >>> > I agree, I allowed user_attributes for the nss responder as well
with a
> >>> > fallback to the InfoPipe value so the admin gets it for free if
InfoPipe
> >>> > is already configured but does not have to configure InfoPipe if
not
> >>> > needed. See man page change for details.
> >>> >
> >>> > Thank you for the review, new version attached.
> >>> >
> >>> > bye,
> >>> > Sumit
> >>>
> >>> I'm sorry about the delay in review.
> >>>
> >>> This version looks almost good, but I have one more request -- can we
> >>> move ifp_parse_attr_list_ex into responder_common.c and drop the ifp
> >>> prefix?
> >>
> >>I kind of expected this :-) I was a bit reluctant because I was afraid
> >>to break something in InfoPipe. I created a new file responder_utils.c
> >>for this because responder_common.c looks already quite big.
> >>
> >>New version attached.
> >>
> >>bye,
> >>Sumit
> >>
> >>>
> >>> What that would prevent in particular is:
> >>> > diff --git a/src/responder/nss/nsssrv.c
b/src/responder/nss/nsssrv.c
> >>> > index 1bbeaa1..601ffc2 100644
> >>> > --- a/src/responder/nss/nsssrv.c
> >>> > +++ b/src/responder/nss/nsssrv.c
> >>> > @@ -48,6 +48,7 @@
> >>> > #include "monitor/monitor_interfaces.h"
> >>> > #include "sbus/sbus_client.h"
> >>> > #include "util/util_sss_idmap.h"
> >>> > +#include "responder/ifp/ifp_private.h"
> >>> >
> >>>
> >>> I know we're currently only using a single function, but it's
not very
> >>> nice to use a private interface of another responder.
> >>>
> >>> I'm fine with the rest of the code.
> >
> >
> >>From 52842484474caa2e91ab39c2ef7edc538175aced Mon Sep 17 00:00:00 2001
> >>From: Sumit Bose <sbose(a)redhat.com>
> >>Date: Tue, 28 Oct 2014 19:42:47 +0100
> >>Subject: [PATCH 3/4] nss: parse user_attributes option
> >>
> >>---
> >> src/man/sssd.conf.5.xml | 26 ++++++++++++++++++++++++++
> >> src/responder/nss/nsssrv.c | 17 +++++++++++++++++
> >> src/responder/nss/nsssrv.h | 2 ++
> >> 3 files changed, 45 insertions(+)
> >>
> >//snip
> >
> >>+++ b/src/responder/nss/nsssrv.c
> >>@@ -214,6 +214,7 @@ static int nss_get_config(struct nss_ctx *nctx,
> >> struct confdb_ctx *cdb)
> >> {
> >> int ret;
> >>+ char *tmp_str;
> >>
> >> ret = confdb_get_int(cdb, CONFDB_NSS_CONF_ENTRY,
> >> CONFDB_NSS_ENUM_CACHE_TIMEOUT, 120,
> >>@@ -298,6 +299,22 @@ static int nss_get_config(struct nss_ctx *nctx,
> >> &nctx->homedir_substr);
> >> if (ret != EOK) goto done;
> >>
> >>+
> >>+ ret = confdb_get_string(cdb, nctx, CONFDB_NSS_CONF_ENTRY,
> >>+ CONFDB_IFP_USER_ATTR_LIST, NULL,
&tmp_str);
> >>+ if (ret != EOK) goto done;
> >>+
> >>+ if (tmp_str == NULL) {
> >>+ ret = confdb_get_string(cdb, nctx, CONFDB_IFP_CONF_ENTRY,
> >>+ CONFDB_IFP_USER_ATTR_LIST, NULL,
&tmp_str);
> >>+ if (ret != EOK) goto done;
> >>+ }
> >>+
> >>+ if (tmp_str != NULL) {
> >>+ nctx->extra_attributes = parse_attr_list_ex(nctx, tmp_str,
NULL);
> >>+ if (ret != EOK) goto done;
> > ^^^^^^^^^^
> > It would be better to test "nctx->extra_attributes"
rather than ret
> >
>
> I sent mail very early :-)
>
> These patches depends on another patch set.
>
> CC src/responder/nss/nsssrv_cmd.o
> src/responder/nss/nsssrv_cmd.c: In function ‘nss_cmd_getsidby_search’:
> src/responder/nss/nsssrv_cmd.c:4259:13: error: implicit declaration of function
‘add_strings_lists’ [-Werror=implicit-function-declaration]
> ret = add_strings_lists(cmdctx, default_attrs,
> ^
> cc1: all warnings being treated as errors
>
> LS
Yes, we talked about that with Sumit on IRC. I'll reorder the patches
before pushing.
btw I don't have any more comments, I can even fix the check before
pushing..
The code works fine:
$ dbus-send --print-reply --system --dest=org.freedesktop.sssd.infopipe
/org/freedesktop/sssd/infopipe org.freedesktop.sssd.infopipe.GetUserAttr
string:psuser@ad.example.com array:string:name,gecos,phone
method return sender=:1.27 -> dest=:1.29 reply_serial=2
array [
dict entry(
string "name"
variant array [
string "psuser(a)AD.EXAMPLE.COM"
]
)
dict entry(
string "gecos"
variant array [
string "ps user"
]
)
dict entry(
string "phone"
variant array [
string "444-333"
]
)
]
I had to add phone to the allowed attrs to the nss section on the IPA
server itself (now I see the convenience of using ldap_extra_attrs right
away..) and to the ifp section on the client.