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.
> > [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.
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.
> > [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.
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 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.
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.
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.