Jakub Hrozek <jhro...@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.
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://fedorahosted.org/mailman/listinfo/sssd-devel
