[PATCHES] ldap_*_search_base doesn't fully limit the group / netgroup

[Pavel Březina]

https://fedorahosted.org/sssd/ticket/960

[Jan Zelený]

> https://fedorahosted.org/sssd/ticket/960

patch #0001:
Nack,
first of all, I still don't like setting *_filter to NULL on the beginning of 
the helper function. In case the function fails, the content of this variable 
should not be tampered with.

In your comment in the helper function, you assume that comma can be in the 
attribute name. I'm don't think this is allowed by RFC. Is there any other 
scenario this "if" is valid for? If not I suggest removing it entirely. If 
there is, please modify it to something like this:

if (len_diff >= 2 && dn[len_diff-1] == ',' && dn[len_diff-2] == '\')

patch #0002:
Nack,
regarding our previous conversation in the original thread, I'm pretty sure, 
returning ret is better than returning EOK every time. Also I'm pretty sure 
that if you return other return code than EAGAIN, the tevent_req_error() will 
be called later.

patch #0003:
Ack,
for any other who would like to review this patch, please note that the change 
only limits number of queries going to LDAP server. It will not eliminate all 
queries due to the filtering being done by an (|....) filter.

Thanks
Jan

[Pavel Březina]

Dne 29.11.2011 14:40, Jan Zelený napsal(a):
> > https://fedorahosted.org/sssd/ticket/960
>
> patch #0001:
> Nack,
> first of all, I still don't like setting *_filter to NULL on the beginning of
> the helper function. In case the function fails, the content of this variable
> should not be tampered with.
>
> In your comment in the helper function, you assume that comma can be in the
> attribute name. I'm don't think this is allowed by RFC. Is there any other
> scenario this "if" is valid for? If not I suggest removing it entirely. If
> there is, please modify it to something like this:
>
> if (len_diff>= 2&&  dn[len_diff-1] == ','&&  dn[len_diff-2] == '\')
>
> patch #0002:
> Nack,
> regarding our previous conversation in the original thread, I'm pretty sure,
> returning ret is better than returning EOK every time. Also I'm pretty sure
> that if you return other return code than EAGAIN, the tevent_req_error() will
> be called later.
>
> patch #0003:
> Ack,
> for any other who would like to review this patch, please note that the change
> only limits number of queries going to LDAP server. It will not eliminate all
> queries due to the filtering being done by an (|....) filter.
>
> Thanks
> Jan

New patches attached.

[Jan Zelený]

> Dne 29.11.2011 14:40, Jan Zelený napsal(a):
> > > https://fedorahosted.org/sssd/ticket/960
> >
> > patch #0001:
> > Nack,
> > first of all, I still don't like setting *_filter to NULL on the
> > beginning of the helper function. In case the function fails, the
> > content of this variable should not be tampered with.
> >
> > In your comment in the helper function, you assume that comma can be in
> > the attribute name. I'm don't think this is allowed by RFC. Is there any
> > other scenario this "if" is valid for? If not I suggest removing it
> > entirely. If there is, please modify it to something like this:
> >
> > if (len_diff>= 2&&  dn[len_diff-1] == ','&&  dn[len_diff-2] == '\')
> >
> > patch #0002:
> > Nack,
> > regarding our previous conversation in the original thread, I'm pretty
> > sure, returning ret is better than returning EOK every time. Also I'm
> > pretty sure that if you return other return code than EAGAIN, the
> > tevent_req_error() will be called later.
> >
> > patch #0003:
> > Ack,
> > for any other who would like to review this patch, please note that the
> > change only limits number of queries going to LDAP server. It will not
> > eliminate all queries due to the filtering being done by an (|....)
> > filter.
> >
> > Thanks
> > Jan
>
> New patches attached.

Ack to all three. I did only differential review between this and previous set 
of patches but I tested those patches before.

Jan

[Pavel Březina]

Dne 14.12.2011 16:03, Jan Zelený napsal(a):
> > Dne 29.11.2011 14:40, Jan Zelený napsal(a):
> > > > https://fedorahosted.org/sssd/ticket/960
> >
> > New patches attached.
>
> Ack to all three. I did only differential review between this and previous set
> of patches but I tested those patches before.
>
> Jan

I forgot to document the fact, that dereference will be disabled if any 
search base contains filter. New patch attached.

[Pavel Březina]

Dne 14.12.2011 19:16, Pavel Březina napsal(a):
> Dne 14.12.2011 16:03, Jan Zelený napsal(a):
> > > Dne 29.11.2011 14:40, Jan Zelený napsal(a):
> > > > > https://fedorahosted.org/sssd/ticket/960
> > >
> > > New patches attached.
> >
> > Ack to all three. I did only differential review between this and previous set
> > of patches but I tested those patches before.
> >
> > Jan
>
> I forgot to document the fact, that dereference will be disabled if any
> search base contains filter. New patch attached.
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://fedorahosted.org/mailman/listinfo/sssd-devel

Ooops, once again with the patches attached :)

[Stephen Gallagher]

On Wed, 2011-12-14 at 19:23 +0100, Pavel Březina wrote:
> Dne 14.12.2011 19:16, Pavel Březina napsal(a):
> > Dne 14.12.2011 16:03, Jan Zelený napsal(a):
> > > > Dne 29.11.2011 14:40, Jan Zelený napsal(a):
> > > > > > https://fedorahosted.org/sssd/ticket/960
> > > >
> > > > New patches attached.
> > >
> > > Ack to all three. I did only differential review between this and previous set
> > > of patches but I tested those patches before.
> > >
> > > Jan
> >
> > I forgot to document the fact, that dereference will be disabled if any
> > search base contains filter. New patch attached.
> > _______________________________________________
> > sssd-devel mailing list
> > [e-mail]
> > https://fedorahosted.org/mailman/listinfo/sssd-devel
>
> Ooops, once again with the patches attached :)

I modified the grammar of the manpage entry slightly and pushed it to
master. Good work!