108366 - xmozillausehtmlmail does not work with LDAP

Status: VERIFIED FIXED

(MailNews Core :: LDAP Integration, defect)

Description

[Roland Felnhofer]

I created my own LDAP schema extension xmozilla.schema. It holds the following
object class and attributes:

# req. core

# attribute defs

attributetype ( 1.3.6.1.4.1.9131.2.1.1
    NAME ( 'xmozillanickname' 'nickname' ) SUP name )

attributetype ( 1.3.6.1.4.1.9131.2.1.2
        NAME ( 'xmozillausehtmlmail' )
        SYNTAX 1.3.6.1.4.1.1466.115.121.1.7
        SINGLE-VALUE )

attributetype ( 1.3.6.1.4.1.9131.2.1.3
        NAME 'xmozillauseconferenceserver'
        SYNTAX 1.3.6.1.4.1.1466.115.121.1.26
        SINGLE-VALUE )

attributetype ( 1.3.6.1.4.1.9131.2.1.4
    NAME 'xmozillaanyphone'
        SYNTAX 1.3.6.1.4.1.1466.115.121.1.26 )

# objectClass defs

objectclass ( 1.3.6.1.4.1.9131.2.2.1
    NAME 'xmozilla'
    SUP organizationalPerson
    STRUCTURAL
    MAY (
        xmozillanickname $
        xmozillausehtmlmail $
        xmozillauseconferenceserver $
        xmozillaanyphone ) ) 

As it can be seen I initially defined 'xmozillausehtmlmail' as Boolean.
John Marmion gave me the hint that the mork database stores:

0: unknown
1: plain
2: html

So it was obvious for me that it's not a boolean attribute any more.

I defined 'xmozillausehtmlmail' as INTEGER

attributetype ( 1.3.6.1.4.1.9131.2.1.2
         NAME ( 'xmozillausehtmlmail' )
         SYNTAX 1.3.6.1.4.1.1466.115.121.1.27
         SINGLE-VALUE )

and I tried defining it as STRING

attributetype ( 1.3.6.1.4.1.9131.2.1.2
         NAME ( 'xmozillausehtmlmail' )
         SYNTAX 1.3.6.1.4.1.1466.115.121.1.26
         SINGLE-VALUE )

I set xmozillausehtmlmail = 2 but "Prefers to receive messages formatted as"
remained "Unknown"

Cheers
Roland

Comment 1

[Roland Felnhofer]

Sorry forgot to tell the build. It's : 2001110103
LDAP server OpenLDAP Ver: 2.0.11

Comment 2

[Roland Felnhofer]

By searching through the code I found the following:

nsAbLDAPProperties.cpp, line 174

173 // mozilla specfic
174 {MozillaProperty_Boolean, "SendPlainText",    "xmozillausehtmlmail"},
175 // ?
176 {MozillaProperty_Int, "LastModifiedDate",    "modifytimestamp"}

So LDAP the attribut for xmozillausehtmlmail has to be BOOLEAN??

Comment 3

[John Marmion]

here is how it works. If we examine the foolowing code fragment:

else if (!PL_strcmp(attrname, kPreferMailFormatColumn))
243     {
244         PRUint32 format = nsIAbPreferMailFormat::unknown;
245         GetPreferMailFormat(&format);
246         nsString formatStr;
247         switch (format) {
248         case nsIAbPreferMailFormat::unknown :
249             formatStr.AssignWithConversion("unknown");
250             break;
251         case nsIAbPreferMailFormat::plaintext :
252             formatStr.AssignWithConversion("plaintext");
253             break;
254         case nsIAbPreferMailFormat::html :
255             formatStr.AssignWithConversion("html");
256             break;
257         default :
258             formatStr.AssignWithConversion("unknown");
259             break;
260         }
261         *value = ToNewUnicode(formatStr);


So the code assumes that the LDAP values are :

"unknown"
"plaintext"
"html"

and these get converted to integer values 

0
1
2

in Mozilla.

I need to change that type from Boolean to Int and also probaly rename the ldap
attribute to "xmozillapreferedmailformat" to be more informative. I will keep
the existing name for backward compatibility.

But if you use the above string values , it should work as is.

Comment 4

[John Marmion]

Attachment: patch to remove ambiguity in LDAP attribute name

Comment 5

[Roland Felnhofer]

You are also working on the "LDAP attributes are case sensitive" (
http://bugzilla.mozilla.org/show_bug.cgi?id=105103 ) as I mentioned there I have
no C compiler installed on my PC. If you sent me or let me know where I can get
the compiled version of  the module, could you include this patch too? So I
could test them both. 

As you explained and recommended I changed the attribute definition for
'xmozillausehtmlmail' to sting and set the value to "html" but it still shows
unknown. I as tried it with "plaintext" – same result.

Comment 6

[John Marmion]

Roland, It looks like I will have to set up an LDAP Server and add this
attribute to it. Our LDAP Testing to date has been against our internal
corporate servers which don't have this attribute. Looking at what the code
should be doing is obviously no substitute for finding out what it is doing. I
will let you know how I get on. Thanks for your patience.

Comment 7

[Roland Felnhofer]

Attachment: Mozilla Schema extension

Comment 8

[Roland Felnhofer]

John, to support you with your testing I'll post here my LDAP mozilla schema.
You can use it as it is. It should not create any conflicts with other schemas
because it uses my own Private Enterprise Number.

Comment 9

[John Marmion]

Attachment: Latest patch to fix this problem

I set up an OpenLDAP server and I can see the problem. This patch should fix
this.
Roland, this patch creates a new version of addrbook.dll. I can make this dll
available for you to test. What mozilla milestone do you wish me to provide
this against? I am presuming you are testing this on Windows. I can also
include the fix for bug 105103 as that fix also only involves addrbook.dll.

Comment 10

[Roland Felnhofer]

John, I'm currently using build 2001111303 Win32. It would be great if you could
combine this two patches and let me have access to the patched 'addrbook.dll'

Comment 11

[Roland Felnhofer]

John, I managed to get an development environment. So I patch the FTP source
(2001-11-14) with your patch. It seems to work fine with both attributtes
'xmozillaprefermailformat' as well as with 'xmozillausehtmlmail'. I'll tried it
with both attributes and all 4 possible settings. 

1) xmozillaprefermailformat = html         Card displays: HTML
2) xmozillaprefermailformat = plaintext    Card displays: Plain Text
3) xmozillaprefermailformat = unknown      Card displays: Unknown
4) NO xmozillaprefermailformat             Card displays: Unknown

1) xmozillausehtmlmail = html              Card displays: HTML
2) xmozillausehtmlmail = plaintext         Card displays: Plain Text
3) xmozillausehtmlmail = unknown           Card displays: Unknown
4) NO xmozillausehtmlmail                  Card displays: Unknown

BOTH attributtes: html winnes over plaintext, unknown winnes over plaintext,
html winnes over unknown

Thank you!

Comment 12

[Roland Felnhofer]

Attachment: Mozilla Schema extension (attribute definition fixed)

Comment 13

[Roland Felnhofer]

John can you please set my attachment "Mozilla Schema extension" to 'obsolete',
I'm not authorized.

Comment 14

[John Marmion]

Thanks for the feedback Roland.

I requested a review for this patch today.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 57755 [details] [diff] [review]
Latest patch to fix this problem

r=[e-mail]

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Reassigning to John.

Comment 17

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

patch seems fine, but I'm sure there is better string for this, that doesn't
involve copying value to the heap.

why that casecompare?  is the caller going to pass in UNKNOWN and unknown and
Unknown, etc?  (forgive my ignorance)

I think case compare like that will invoke the i18 code.

why not something like this:

switch (value[0]) {
  case 'h':
  case 'H':
    format = nsIAbPreferMailFormat::html;
    break;
  case 'p':
  case 'P':
    format = nsIAbPreferMailFormat::plaintext;
    break;
  default:
    format = nsIAbPreferMailFormat::unknown;
    break;
}

Comment 18

[Alec Flett]

Comment on attachment 57755 [details] [diff] [review]
Latest patch to fix this problem

>--- mailnews/addrbook/src/nsAbCardProperty.cpp	Wed Nov 14 15:22:44 2001
>+++ mailnews.patch/addrbook/src/nsAbCardProperty.cpp	Wed Nov 14 15:22:04 2001
>@@ -347,11 +347,11 @@
>    	{
>         PRUint32 format = nsIAbPreferMailFormat::unknown;
>         nsString formatStr(value);
>-        if (formatStr.CompareWithConversion("unknown", PR_TRUE))
>+        if (!formatStr.CompareWithConversion("unknown", PR_TRUE))
>             format = nsIAbPreferMailFormat::unknown;
>-        if (formatStr.CompareWithConversion("plaintext", PR_TRUE))
>+        if (!formatStr.CompareWithConversion("plaintext", PR_TRUE))
>             format = nsIAbPreferMailFormat::plaintext;
>-        if (formatStr.CompareWithConversion("html", PR_TRUE))
>+        if (!formatStr.CompareWithConversion("html", PR_TRUE))
>             format = nsIAbPreferMailFormat::html;
>         SetPreferMailFormat(format);
>     }


ack.
First of all, don't use CompareWithConversion, use NS_LITERAL_STRING instead:
-	 if (formatStr.CompareWithConversion("unknown", PR_TRUE))
+	 if (formatStr.Equals(NS_LITERAL_STRING("unknown"), PR_TRUE))

Actually, this code is really obnoxious with its use of strings, not to mention
using Compare()
when it just wants to know about equality. While you're there, we should fix
this the right way. 
nsString formatStr(value) is just wrong - that's making an allocation and a
copy just to do a 
string comparison. Instead:

+ if (NS_LITERAL_STRING("unknown").Equals(value))

or, if you MUST do case-insensitive compares, do
+ if (NS_LITERAL_STRING("unknown").Equals(value,
nsCaseInsensitiveStringComparator())

Secondly, yes, i18n libraries get invoked when you do case-insensitive compares
on unicode 
strings. It's not terribly slow (maybe 2x the speed of a case-sensitive
compare) but you 
should avoid it if you can, especially if you're in a loop. 
That's why you have to #include "nsUnicharUtils.h" just to get 
nsCaseInsensitiveStringComparator

Comment 19

[John Marmion]

In my defence to both Seth and Alec, when I am fxing a bug I normally
concentrate solely on fixing the bug and not how the code might have been
designed in the first place. 

I believe Seth's approach is not very robust and is inherently dangerous. So we
are using 'html', 'plaintext' today, what if we use some play on 'h' or 'p' in
the future then his code will break. We must remember that this code is about
mapping an LDAP User attribute 'xmozillausehtmlmail' to an existing card
property in Mozilla. Yes I believe we should do a caseCompare as no where does
it say that these values in LDAP must be exactly these values.

As for Alex, if you are going to bandy about  words like 'obnoxious' then make
sure that your own solution will at least compile:

NS_LITERAL_STRING("unknown").Equals(value,nsCaseInsensitiveStringCompare())

Also I am not convinced that creating 3 instances of an nsAFlatString() is
anyway less 'obnoxious'.

What I would change is:  I would add an 'else if' on the two last cases. 

Comment 20

[Alec Flett]

hey, I said the code was obnoxious, not you.. and my suggestion might not have
compiled, but hey I'm typing this in a bug, not compiling it, give me a break..  :)

Also, you're only creating the nsDependentStrings when the compare is actually
done.. the cost of creating one is absolutely minimal and is far less than the
cost of doing a CompareWithConversion (Which involves converting from ISO Latin1
to unicode)

All I'm saying is
1) if you're going to change those lines, change them the right way :)
2) if you change them the right way, you don't need nsString valueStr(value);

this is just part of the standard review process...

Comment 21

[Roland Felnhofer]

Hmm... it seems it will take a while until the code gets checked in, anyhow.
Could somebody also review my Mozilla schema extension and replace my own
'privat' OID (9131)with a more 'official' one?

Comment 22

[John Marmion]

Glad to see its not personal Alec, only business right!

As usual I am open to persuasion on this. We learned most of our code practises
in Mozilla by investigating the Address Book stuff. My point is not that your
suggestion does not compile because of a typo but because the Equals method of
nsAFlatString() does not support the CaseInsensitiveStringComparator()
parameter. So if we agree that we need to support CaseInsensitive, how best can
we compare a PRUnichar string against a const char *. 

Comment 23

[Alec Flett]

the "approved" way to have a literal string that you want to use for unicode
purposes is to use NS_LITERAL_STRING("foo") - under most compilers, that
actually makes "foo" a unicode string at build time (using L"foo" under windows,
for example) which looks, literally, like:
 PRUnichar buf[] = {PRUnichar('f'), PRUnichar('o'), PRUnichar('o'), PRUnichar(0) };
and then wraps a nsDependentString around it. on compilers that don't support
unicode literal strings, it does the conversion at runtime automatically.

I'm not sure why my code didn't compile, but it should work fine..

nsAString.Equals looks like:
      inline PRBool Equals( const abstract_string_type&, const
nsStringComparator& = nsDefaultStringComparator() ) const;
      PRBool Equals( const char_type*, const nsStringComparator& =
nsDefaultStringComparator() ) const;

so you can use nsCaseInsensitiveStringComparator() (it derives from
nsStringComparator), but you have to #include "nsUnicharUtils.h" to get it, if
it's not already included. note the "ns" in front of
nsCaseInsensitiveStringComparator() (you didn't include that in you comment, at
least)

Comment 24

[John Marmion]

Attachment: Update Patch following Review

Using the nsUnicharUtils.h as suggested by Alec works fine.

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 61736 [details] [diff] [review]
Update Patch following Review

If we're gonna change the attribute name, wouldn't
"xmozillapreferredmailformat" be grammatically more correct?  Fix that, and add
a comment next to the old attribute name explaining that it's deprecated, and
you've got r=[e-mail]

Comment 26

[John Marmion]

Attachment: Update Patch with latest review request

Dan, made those two changes as you requested. While I agree that the
'xmozillapreferredmailformat' sounds more correct, I was somewhat reluctant to
change this because we are using the corresponding Mozilla Attribute
'PreferMailFormat' which is used extensively throughout the code and
corresponding methods i.e. GetPreferMailFormat(), SetPreferMailFormat(). I
don't believe the patch warrants changing all those occurrences. Also, in the
same way that 'DisplayedName' sounds more correct than 'DisplayName', people
still understand its meaning.

So, I leave the final decision to you. I am keen to get the patch accepted.

Comment 27

[John Marmion]

Attachment: Update Patch against the head

The recently landed fix for bug 104158 has broken this patch. I have updated
the patch against the head.

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 61953 [details] [diff] [review]
Update Patch against the head

I don't feel too strongly about it; but my inclination is to go with preferred
and not propagate  mozilla's internal historical cruft out to the end-user /
sysadmin level.  r=[e-mail]

Comment 29

[Dan Mosedale (:dmosedale, :dmose)]

Roland, can you open a new bug for the schema-change stuff?  You're right that
we ought to have an official mozilla schema, but that's a bunch more work than
just the small fix here.

Comment 30

[Roland Felnhofer]

Dan, I opend a new bug ( http://bugzilla.mozilla.org/show_bug.cgi?id=116692 ) to
define a offical Mozilla schema extension.

Comment 31

[John Marmion]

Attachment: Update Patch following bug 73868

The changes made by the fix for bug 73868 to nsAbCardProperty.cpp make some of
this patch redundant. The patch changes to nsAbLDAPProperties.cpp only are
sufficient to ensure that this bug is fixed. I tested this against today's
head. The changes introduced in 73868 assume that the Mozilla Attribute is
called 'PreferMailFormat' while it actually is 'SendPlainText'. This could map
to 'SecondEmail'. This patch fixes this and sets the Mozilla attribute to
'PreferMailFormat' and introduces a new LDAP attribute called
'xmozillapreferredmailformat' as discussed during the review.

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 63252 [details] [diff] [review]
Update Patch following bug 73868 

r=[e-mail]

Comment 33

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

Comment on attachment 63252 [details] [diff] [review]
Update Patch following bug 73868 

sr=sspitzer

Comment 34

[Dan Mosedale (:dmosedale, :dmose)]

Fix checked in.  Thanks for the patch, John.

Comment 35

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

there's a problem.

mork stores it as 0,1,2 {unknown, plain, html}

but on the wire, 'xmozillausehtmlmail' is a boolean.

it's either true, false or not given.

for ldif export (and import) we do this:

0 -> export nothing
1 -> export "xmozillausehtmlmail: false"
2 -> export "xmozillausehtmlmail: true"

for import, we do

"xmozillausehtmlmail: false" -> 1
"xmozillausehtmlmail: true"  -> 2

if nothing is specified, we do nothing and the default is "unknown".

thanks to dmose for pointing this out.  I'll log a new bug on john.

Comment 36

[yulian chang]

John,
How should I test it?
Thanks!

Yulian

Comment 37

[John Marmion]

Yulian, to test this you need to be able to set up an LDAP Address Book. There
is no UI to do this yet (bug #83091) but it can be achieved by simply editing
the prefs.js. file. This is described on the front page of
http://abzilla.mozdev.org but here is a quick summary. 

First exit Mozilla and add the following 4 lines to your prefs.js file:


For example if your ldap server is called myLDAP and the base dn = "dc=sun,
dc=com"

user_pref("ldap_2.servers.myLDAP.description", "myLDAP");
user_pref("ldap_2.servers.myLDAP.dirType", 777);
user_pref("ldap_2.servers.myLDAP.position", 99);
user_pref("ldap_2.servers.myLDAP.uri",
	"moz-abldapdirectory://myLDAP:389/dc=sun,dc=com");

the "dirType" must be written like that and gets set to 777
the position is the next available position in the prefs.js file. I have set it
to 99 for simplicity but typically it would be set to the next available
position in the prefs.js file + 1. This determines where the Address Book is
positioned in the left-hand pane of the AB.

Now run Mozilla AB and the LDAP Address Book should appear in the list of
Address Books. With the landing of the quick search feature of bug #83023, then
you should be able to search and return data.

To test this fix, you need to be able to add attributes to your LDAP Server. You
need to add the attribute "xmozillapreferredmailformat" to 3 different entries.
Assign the value "unknown", "plaintext" and "html" to the first, second and
third entry respectively. Next search for these entries in the Mozilla LDAP
Address Book and display their properties. 

Check that the field "Prefers to receive messages formatted as" is set
accordingly for each of the three entries.

Repeat the process for the LDAP attribute "xmozillausehtmlmail".

Comment 38

[Dan Mosedale (:dmosedale, :dmose)]

Yulian: I'm not sure this is worth testing until we get bug 119331 sorted out.

Comment 39

[yulian chang]

Verified with bug 119331.