Jakub Hrozek <jhrozek(a)redhat.com> wrote:
On Fri, Dec 09, 2011 at 03:18:38PM +0100, Jan Zelený wrote:
> > On Thu, Dec 08, 2011 at 11:02:05AM +0100, Jakub Hrozek wrote:
> > > [PATCH 1/6] sss_utf8_tolower utility function+unit tests
> > > This will be used later on to lowercase the usernames. Also includes
> > > unit tests for the whole sss_utf8 module.
>
> Nack, it seems that this patch breaks the test suite (or perhaps it's the
> last one, I'm not 100% sure.
I ran "make check" in-tree and didn't see any error, but I think I see
the bug now.
The root cause was that I included sss_tc_utf8_tolower() into util.c which
made it depend on sss_utf8.c and by extension on the UTF8 library of
choice. At the same time the memberof plugin depends on util.c so we would
had linked the plugin with the UTF8 library.
I decided to include the talloc wrapper in a new module "sss_tc_utf8.c"
-- I think a module with a single function is a lesser evil than linking
the whole memberof module with an UTF8 library (which might be even GLib..)
A revised patch is attached.
Everything works fine now, thanks for fixing that. Also, as you said, the
approach is much better I think. Ack
> > > [PATCH 2/6] Responders: Split getting domain by name
into separate
> > > function A utility function originally written for the SUDO
> > > responder, but it turns out it's useful earlier
>
> Ack
>
> > > [PATCH 3/6] Use the case sensitivity flag in responders
> > > Reads the configuration option and includes its handling in the
> > > responders.
> > > Unfortunately some parts like negative cache are more complex because
> > > one domain can be case sensitive and another insensitive during
> > > multidomain searches.
>
> Nack,
> why are you using talloc strdup in functions like
> nss_cmd_getgrnam_search? I mean talloc_strdup(dctx, cmdctx->name). I
> suspect it's for better consistency with behavior of the
> sss_tc_utf8_tolower() but there is no real need for it IMO.
To be able to free the domain-specific name inside the domain loop which
I forgot to do in the first iteration of patches.
I changed the patch to free "name" on every iteration to avoid clinging
to allocated memory.
Now it makes sense, thanks. Please check those free() calls on lines 637, 675
and 793 of the patch. Do I miss something or should there be name as their
argument instead of NULL?
> The pd_set_primary_name() doesn't seem like a part of the
patch, or is
> it?
>
>I'd
>
> prefer separate patch for it and places where it is used. I'm also
> thinking whether or not should it also detect the case_sensitive flag.
The reasoning begind pd_set_primary_name() is username canonicalization.
sysdb_getpwnam() can return data based on both the real name or the
lowercased alias, because the filter it uses matches both "name" and
"nameAlias". Only "name" corresponds to RDN, though. On many
places,
including auth and access, SSSD depends on searching "user by name" by
constructing a user DN based on the username and domain name,.
There is no need to pass the case_sensitive flag, strcmp is already case
sensitive and the name we search with is lowercased in the previous
hunk if case_sensitive is true.
I'm not saying that the function is useless or anything, I understand its
purpose very well. I'm just saying that it should probably have its own patch.
Of course, I don't insist on it.
> > > [PATCH 4/6] Refactor saving sdap entities
> > > There was too much code duplication between
> > > sdap_save_{user,group,netgroup} and my later patch would add even
> > > more. This patch removes the most egregious cases.
>
> Nack,
> I think that the "multivalued" argument is really not necessary, is it?
> Without it, there is also no need for those two wrapper macros.
It's actually required to maintain the same behaviour as the original code.
Ah, ok. I didn't see that use case, sorry for the fuss.
In some cases such as when storing modifyTimestamp we would only
store
the first value unconditionally even if the attribute was multivalued on
the server. This is the case for both original and refactored code.
I haven't used the macro for storing all string attributes. Some attributes
like ldap_user_principal are munged before saving to sysdb and that's
difficult to generalize withut some kind of callback, but still, only the
first value is saved even if the original attribute was multivalued.
Is there any value in throwing an error like we do with the POSIX set
of attributes?
I think a DEBUG, let's say on SSSDBG_MINOR_FAILURE should be enough. In case
the attribute is trimmed, it is important to know that this happened.
> I noticed that at one place, there was strdup() called before
adding the
> string to sysdb_attrs. I didn't inspect this much further, so my question
> is whether it would make any sense to do that in
> sdap_attrs_add_ldap_attr() and sdap_save_all_names()?
Which strdup was that? At one place isn't really descriptive.
The place isn't really important, the idea counts ;-) Anyway, there is only
one such place in the code in question: sdap_async_users.c:202
I think it's safe to discard the strup there but as I said, I haven't
inspected this extensively.
I haven't touched the cases where we need to pass the attribute
values out
(like USN), the other cases were duping the value on user_attrs which is
allocated on tmpctx and never stolen or moved elsewhere.
> > > [PATCH 5/6] Use the case sensitivity flag in the LDAP provider
> > > When saving users,groups and netgroups, saves the lowercase version
> > > of the name as an alias in addition to the original name that is
> > > still stored as name as well as part of RDN.
>
> Nack,
> I don't think there is a need for aliases to be both in the list both
> untransformed and in lowercase. Please check that and if it isn't
> necessary, please remove it.
If the duplicate lowercase attribute is removed, aliases wouldn't work
if the user switches the case sensitivity flag of a domain from false to
true (because only secondaryname would be stored and client might ask for
secondaryName as stored on server).
An extra sysdb attribute is not much harm, really.
Ok, no problem. If it has its purpose, I have no problem with it.
> Variable lowername in sdap_save_all_names() is not used. Also instead of
> argument "dom", I would prefer "case_sensitive" being directly
given to
> the sdap_save_all_names().
Thanks, fixed.
> > > [PATCH 6/6] Use the case sensitivity flag in the simple access
> > > provider Performs string comparisons in case sensitive or
> > > insensitive manner depending on the configuration.
>
> Ack
>
> Jan
It seems that there are no issues with the general approach so I will
file a ticket to handle LOCAL provider (which will probably get deferred)
and prepare another patch to handle the proxy provider.
Agreed.