105103 - LDAP attributs are case sensitive

Status: VERIFIED FIXED

(MailNews Core :: LDAP Integration, defect)

Description

[Roland Felnhofer]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:0.9.5+)
Gecko/20011012
BuildID:    2001101203

The LDAP attributs are case sensitive - That is not RFC 2252 compliant!

...
An AttributeDescription can be used as the value in a NAME part of an
AttributeTypeDescription.  Note that these are case insensitive.
...

Here an example: I'm using OpenLdap 2.0.11 (it was the same with OpenLdap
2.0.7). The attribute 'givenname' is definde in the core.schema of OpenLdap as
'givenName' (caps 'N' of Name). in that case the address book apps is not able
to display the first name neither if you select 'Show Name As' -> 'Last, First'
nor in the address card. After I canged the core.schema form 'givenName' to
'givenname' it was displayed properly. Same affect can be seen with other
attributs having upercase characters in the attribut name.

I don't think it's a good idea to have to change the schema.

If you need further information feel free to ask.

Reproducible: Always

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

This appears to be addressbook-specific.

Comment 2

[John Marmion]

I only noticed this at the end of last week. Reading the RFC appears to confirm 
Roland's description. I need to submit a patch to fix this.

Comment 3

[John Marmion]

Attachment: Small Patch to fix ldap attribute names support for case insensitive strings

Comment 4

[John Marmion]

Roland are you able to test this patch before I submit for a review?

Comment 5

[John Marmion]

I had a brief conversation with [e-mail]. about this patch. Dan
wondered would it not be simpler to just lower case the string before before the
findMozillaPropertyFromLDAP() and save on the footprint of not only an extra
class but also the consumption of more memory by using the OWN parameter rather
than the NEVER_OWN. My approach was to ensure that we had a fault tolerant
solution on both ends. If additional Mozilla LDAP attributes are addded then
this solution will cater for that. I believe this is a price worth paying. But
we won't fall out over either solution.

Comment 6

[Roland Felnhofer]

Hi John,

Unfortunately I have no C-compiler installed on my PC. I would be very willing
to test it. So if you could just compile the appropriate module (DLL) and let me
know what nightly build I shell install and where I can get the DLL. Maybe you
can send it to me by mail or place it somewhere on a FTP server. I would install
what ever nightly build and replace than the appropriate DLL (Is it
nsldap32v40.dll ?)

Comment 7

[Roland Felnhofer]

I agree with you that your fault tolerant solution is a better approach to solve
the bug.

Comment 8

[Roland Felnhofer]

John, as already said at bug 108366 I've got a dev. env.
Also this patch seems to work. At least givenName, telephoneNumber and
facsimileTelephoneNumber works. That are currently the only 3 attributes in my
test LDAP directory using uppercase characters in the name. If you need more
attributes to test against please let me know.

Thank you so far.

Comment 9

[John Marmion]

Thanks for the feedback Roland.

requested review today for this patch .

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 56889 [details] [diff] [review]
Small Patch to fix ldap attribute names support for case insensitive strings

r=[e-mail]

Comment 11

[(not reading, please use [e-mail] instead)]

Comment on attachment 56889 [details] [diff] [review]
Small Patch to fix ldap attribute names support for case insensitive strings

let me take a close look tomorrow.  subclassing seems like overengineering in
this case.  there's a whole bunch of string code already in mozilla (including
string arrays).

I want to review this code and the other string classes in the ab / ldap code,
I think we can use existing string classes.

dmose, can you look as well?

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 56889 [details] [diff] [review]
Small Patch to fix ldap attribute names support for case insensitive strings

OK, since the strings in the tables are globals in the code (they could
actually probably even be statics, couldn't they?), they're never going to go
away as long as this shared library as loaded, so let's definitely stick with
NEVER_OWN.  Additionally, getting rid of the nsCaseCStringKey and just
downcasing the string before you create the nsCStringKey shouldn't cause any
fault tolerance problems that I can see.

Comment 13

[John Marmion]

Attachment: Update patch following review

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 61566 [details] [diff] [review]
Update patch following review

>--- mailnews/addrbook/src/nsAbLDAPProperties.cpp	Thu Dec 13 12:15:31 2001
>+++ mailnews.patch/addrbook/src/nsAbLDAPProperties.cpp	Thu Dec 13 12:10:12 2001
>@@ -200,7 +200,9 @@
> const MozillaLdapPropertyRelation* MozillaLdapPropertyRelator::findMozillaPropertyFromLdap (const char* ldapProperty)
> {
>     Initialize();
>-    nsCStringKey key (ldapProperty) ;
>+    nsCAutoString valueStr(ldapProperty);

Above is one traversal of the string (the copy-constructor).

>+    valueStr.ToLowerCase()

The downcasing requires a second string traversal.  

It shouldn't be difficult to do this as a single copy-while-downcasing.

Also, due to the missing semicolon, this doesn't even look like it would
compile.  Please don't submit code for review until you've compiled
and tested it.	If this is just a cut-n-paste error, and you have done
that stuff, my please accept my apologies.

>+    nsCStringKey key (valuStr.get()) ;
>     return NS_REINTERPRET_CAST(const MozillaLdapPropertyRelation *, mLdapToMozilla.Get(&key)) ;
> 

How about something like this, which should only do a single string
traversal and updates the code to use the very newest in mozilla string
technologies:  :-)

nsCAutoString downcasedProp;
ToLowerCase(nsDependentCString(ldapProperty), downcasedProp);

return NS_REINTERPRET_CAST(const MozillaLdapPropertyRelation *,
			   mLdapToMozilla.Get(nsCStringKey(downcasedProp));

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Also, while you're here, can you make all the unnecessary globals be statics?

Comment 16

[John Marmion]

Attachment: Update Patch folowing Review

Thanks for the input Dan. Sorry about the glitch in the previous patch. I left
the nsCStringKey key () as is, as this is consistent with how this is used
throughout this module. On the 'static' issue , I declared the table to be
static to limit its scope but all the member variables are already declared
static in the header file.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 61753 [details] [diff] [review]
Update Patch folowing Review

Ah, I missed the member variable decls in the header; I get it now.

>+    nsCStringKey key (lowercasedProp.get()) ;
> 
>     return NS_REINTERPRET_CAST(const MozillaLdapPropertyRelation *, mLdapToMozilla.Get(&key)) ;

You shouldn't need a .get() and the & in front of key, because
nsCStringKey has a constructor that takes an nsACString as an argument.
Fix that, and you've got r=[e-mail].

Comment 18

[John Marmion]

Dan,I attempted to fix that as you suggested but I am getting the following
error on Linux:

nsAbLDAPProperties.cpp: In function `static const struct
MozillaLdapPropertyRelation *
MozillaLdapPropertyRelator::findMozillaPropertyFromLdap(const char *)':
nsAbLDAPProperties.cpp:211: no matching function for call to `nsHashtable::Get
(nsCStringKey)'
../../../dist/include/xpcom/nsHashtable.h:118: candidates are: void *
nsHashtable::Get(nsHashKey *)

using: 

return NS_REINTERPRET_CAST(const MozillaLdapPropertyRelation *,
			   mLdapToMozilla.Get(nsCStringKey(lowercasedProp)));

Forgive me if I am missing something - I presume this is what you want me to do
instead of:

nsCStringKey key (lowercasedProp.get()) ;

return NS_REINTERPRET_CAST(const MozillaLdapPropertyRelation *,
mLdapToMozilla.Get(&key)) ;

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

I misread the signature; sorry about that.  But this should work, I think, and
avoids the unnecessary .get().

nsCStringKey key (lowercasedProp);

return NS_REINTERPRET_CAST(const MozillaLdapPropertyRelation *,
mLdapToMozilla.Get(&key)) ;

Comment 20

[John Marmion]

Attachment: Update Patch for Review

Remove the get() method as suggested.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 62189 [details] [diff] [review]
Update Patch for Review

r=[e-mail]

Comment 22

[John Marmion]

This patch is still correct aaginst today's head.

Comment 23

[(not reading, please use [e-mail] instead)]

Comment on attachment 62189 [details] [diff] [review]
Update Patch for Review

sr=sspitzer.

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Fix checked in; thanks for the patch, John.

Comment 25

[grylchan]

2003-01-02-05-trunk Xp
2003-01-02-08-trunk Mac OS 10.1.2, linux 2.2

verified fix checked into trunk via lxr and
that you can view add book cards via last, first or first,last
with different Cases and no problem displaying the addresses
in address book.

marking verified