[SSSD] [PATCH] Ability to set a domain as case sensitive or insensitive

Jan Zelený jzeleny at redhat.com
Fri Dec 9 14:18:38 UTC 2011


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

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

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.

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

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()?

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

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().

> > [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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20111209/8c8d5867/attachment.sig>


More information about the sssd-devel mailing list