89198 - LDAP autocomplete item display name is not properly quoted

Status: VERIFIED FIXED

(MailNews Core :: LDAP Integration, defect, P2)

Description

[David Saxby]

Unfortunately, no URL because this is within our company firewall.
Problem: when performing LDAP lookup against our company LDAP directory, it
seems the incorrect mail address is returned.  For example, assume that our
company is called "nowhere.com" (purely as an example) and that full email
addresses are in the form [e-mail].  Additionally, an
alias can be entered and LDAP returns the full address.  In the example above,
this would be flastnam (first letter of first name and up to seven characters of
second name).

Currently, using this example, if I enter "flastnam" into the mail "To" field of
Mozilla, the LDAP directory would return:
LASTNAME, FIRSTNAME <[e-mail]>
When you hit "send", it returns the error:
An error occurred while sending mail.  The mail server responded 5.1.1
<LASTNAME>...User unknown.
Compare this with Netscape 4.x.  If I type the same thing, it would return the
following email address to the "To"field:
"LASTNAME.FIRSTNAME" <[e-mail]>
When hitting "send", everything is fine.  Note the difference appears to be the
double quotes added.
Finally, if you enter something like:
firstname.lastname
into the "To" field, it would return:
LASTNAME <[e-mail]>
In this case, everything is fine and the message is sent correctly.

Comment 1

[Mark Smith [:mcs]]

Changed product and owner.

Comment 2

[Dan Mosedale (:dmosedale, :dmose)]

What's going on here is that the display name strings returned from LDAP are not
being quoted for use as a "phrase" as per RFC 2822.  I assume there is already
code to do this in mailnews somewhere.  Ducarroz, can you point me to any such
code?  

I'm thinking that perhaps the right way to fix this is to add a new pair of
delimiter meta-chars to the outputFormat parser with the semantics of
"everything inside this delimiter-pair needs to be RFC 2822 phrase-quoted."

For now, there's a workaround if the LDAP server you're connecting against has
the sn (surname) and gn (givenname) attributes set on its entries.  Quit the
browser completely, and edit your prefs.js file as follows:

* find the server entry for the LDAP server you are autocompleting against

* in this case, I'm using SomeNameOrOther

* add the following line:

user_pref("ldap_2.servers.SomeNameOrOther.autoComplete.outputFormat", "[gn] [sn]
<{mail}>");

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

*** Bug 91243 has been marked as a duplicate of this bug. ***

Comment 4

[Tony DeJoie]

Ok in Solaris I needed to change your workaround so that the quotes remained 
intact, this worked for me (needed to add \" to make sure the quotes were 
actually imbeded in the full email address):

user_pref("ldap_2.servers.SomeNameOrOther.autoComplete.outputFormat", "\"[gn] 
[sn]\" <{mail}>");

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Tony: right you are; thanks.

Turns out that to fix this without introducing extra dependencies, I need to
factor the output formatter out of the autocomplete session itself.  I started
that yesterday afternoon, and am partway done.  After that happens, it looks
like the thing to do is to use the nsIMsgHeaderParser::MakeFullAddress function
to do the quoting work.

I'll post a patch here in the next small number of work days.

Comment 6

[Jussi-Pekka Mantere]

Greetings from Burbank: we need this bad :-)

Comment 7

[Michael Baffoni]

*** Bug 91698 has been marked as a duplicate of this bug. ***

Comment 8

[Joe Hewitt (gone)]

Attachment: patch to show comment column in autocomplete results

Comment 9

[Dan Mosedale (:dmosedale, :dmose)]

OK, I've got a version of this fix working; it still needs a bunch of cleanup,
after which I'll post it here.

As I was doing the work for this, I realized that it would be easy to, at the
same time, add more identifying information to autocomplete entries (so that you
could be sure you were sending to John Smith in accounting and not John Smith in
marketing.  Hewitt was kind enough to cons up a little patch that makes the
compose window autocomplete widget display the "comment" attribute of each
nsIAutoCompleteItem as well, which is where I'm putting this info (it doesn't
actually get reflected into the text field once selected, similar to the web
page title in the browser URL bar autocomplete dropdown).

I'm going to attach a screenshot of what it looks like.

mpt, jglick, ducarroz: any objection to or comments about this proposed UI change?

Comment 10

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: png of proposed UI tweak

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

I'm moving hewitt's patch over to bug 92135, since that's really part of the UI
stuff, and can land decoupled from my LDAP patch.  I'm hoping to see some
version of both of these land for 0.9.4.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch to do address quoting correctly, v1

Comment 13

[Leif Hedstrom]

Looks good to me. Since it was pointed out to me in one of my SR, I have to
mention it here to. The code

+    *aFormatter = mFormatter;
+    NS_IF_ADDREF(mFormatter);


Can be "optimized" to 

     NS_IF_ADDRED(*aFormatter = mFormatter);


Doesn't matter, but I just wanted to comment on something. ;)

R=leif

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v2

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

OK, the new patch has the change leif suggested in several places.  Also, I
noticed that I got the default template strings mixed up: I was using the
"required delimiters" {} around the strings that should have had the "optional
delimiters" [] and vice versa.  So that's fixed now too.

Comment 16

[David :Bienvenu]

 NS_IF_ADDREF(*aFormatter = mFormatter);

I think this will generate two assignments (unless the compiler is exceedingly
clever)

+    if (! aFormatter ) {
+        return NS_ERROR_NULL_POINTER;

if you don't mind the assertion (i.e., this is an unexpected error), you can use
NS_ENSURE_ARG_POINTER here

nsAbLDAPAutoCompleteFormatter::Format

here you keep dropping error rv's in favor of NS_ERROR_FAILURE. Unless you've
got a good reason for doing that, it would be better to return the actual error.
NS_ERROR_FAILURE's are hard to track down.

+    NS_IF_ADDREF(*aItem = item);

again, two assignments

other than that, sr=bienvenu

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

> NS_IF_ADDREF(*aFormatter = mFormatter);
>
> I think this will generate two assignments (unless the compiler is exceedingly
> clever)

No, NS_IF_ADDREF is actually implemented using a template exactly so you can do
this.

http://lxr.mozilla.org/seamonkey/source/xpcom/base/nsISupportsUtils.h#1187

Some people consider this the preferred syntax (since I think it also generates
only one dereference and calls AddRef on the correct interface).

Comment 18

[David :Bienvenu]

ok, never mind, my bad - more invisible fun with templates.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

> +    if (! aFormatter ) {
> +        return NS_ERROR_NULL_POINTER;
>
> if you don't mind the assertion (i.e., this is an unexpected error), you 
> can use NS_ENSURE_ARG_POINTER here

I generally avoid the NS_ENSURE_* stuff since it hides a return, making the code
 less obvious to read.

> nsAbLDAPAutoCompleteFormatter::Format
>
> here you keep dropping error rv's in favor of NS_ERROR_FAILURE. Unless you've
> got a good reason for doing that, it would be better to return the actual
> error.
> NS_ERROR_FAILURE's are hard to track down.

This was indeed superfluous here.  Fixed

I've left the NS_IF_ADDREF stuff the same, as per dbaron's comment.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch v3, with rv propagation cleanup in ::Format

Comment 21

[David :Bienvenu]

there are more instances of dropping error codes in nsAbLDAPAutoCompleteFormatter

fix those, and sr=bienvenu

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch v4, cleaned up remaining rv propagation

Comment 23

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch v5. Fixes issues in MsgComposeCommands.js noticed by srilatha.

Comment 24

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch v6. Pushed try/catch statements up one-level so that unexpected failures wouldn't try and continue executing code inside the setupLdapAutoComplete function.

Comment 25

[Srilatha Moturi]

r=srilatha for the MsgComposeCommands.js part of the patch

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch v7; fixes brain farts in windows makefile.win

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch v8: files, interfaces, and classes renamed to cope with the Mac's filename limit

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

bienvenu: the changes to MsgComposeCommands.js were not completely trivial; if
you could look them over once more, I'd really appreciate it.  Thanks.

Comment 29

[David :Bienvenu]

sr=bienvenu - looks like you are checking for offline state, but I just want to
make sure you've tried a few tests while offline, and while not offline but not
connected to the network.

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

Fix checked in.

Comment 31

[yulian chang]

Verified.

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

*** Bug 96739 has been marked as a duplicate of this bug. ***

Comment 33

[Srilatha Moturi]

*** Bug 95317 has been marked as a duplicate of this bug. ***

Comment 34

[Peter Janes]

*** Bug 90737 has been marked as a duplicate of this bug. ***