Closed Bug 89198 Opened 23 years ago Closed 23 years ago

LDAP autocomplete item display name is not properly quoted

Categories

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

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.4

People

(Reporter: sax, Assigned: dmosedale)

References

Details

Attachments

(10 files)

(deleted), patch
Details | Diff | Splinter Review
(deleted), image/png
Details
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
(deleted), patch
Details | Diff | Splinter Review
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 firstname.lastname@nowhere.com. 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 <FIRSTNAME.LASTNAME@NOWHERE.COM> 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" <FIRSTNAME.LASTNAME@NOWHERE.COM> 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 <FIRSTNAME.LASTNAME@NOWHERE.COM> In this case, everything is fine and the message is sent correctly.
Component: LDAP Tools → LDAP Mail/News Integration
Product: Directory → MailNews
QA Contact: __UNKNOWN__
Target Milestone: --- → mozilla0.9.3
Changed product and owner.
Assignee: mcs → 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}>");
Blocks: 17880
OS: Windows NT → All
Hardware: PC → All
Summary: LDAP directory returns incorrect address when using alias lookup → LDAP autocomplete item display name is not properly quoted
Priority: -- → P3
*** Bug 91243 has been marked as a duplicate of this bug. ***
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}>");
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.
Status: NEW → ASSIGNED
Priority: P3 → P2
Whiteboard: [relnote] Document workaround in release notes
Greetings from Burbank: we need this bad :-)
Target Milestone: mozilla0.9.3 → mozilla0.9.4
*** Bug 91698 has been marked as a duplicate of this bug. ***
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?
Attached image png of proposed UI tweak (deleted) —
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.
Keywords: patch, review
Whiteboard: [relnote] Document workaround in release notes → have patch; need r=,sr=
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
Attached patch patch, v2 (deleted) — Splinter Review
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.
Keywords: review
Whiteboard: have patch; need r=,sr= → have patch, r=; need sr=
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
> 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).
ok, never mind, my bad - more invisible fun with templates.
> + 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.
Whiteboard: have patch, r=; need sr= → have patch, r=, sr=; test-building on all platforms
there are more instances of dropping error codes in nsAbLDAPAutoCompleteFormatter fix those, and sr=bienvenu
r=srilatha for the MsgComposeCommands.js part of the patch
bienvenu: the changes to MsgComposeCommands.js were not completely trivial; if you could look them over once more, I'd really appreciate it. Thanks.
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.
QA Contact: yulian
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Whiteboard: have patch, r=, sr=; test-building on all platforms
Verified.
Status: RESOLVED → VERIFIED
*** Bug 96739 has been marked as a duplicate of this bug. ***
*** Bug 95317 has been marked as a duplicate of this bug. ***
*** Bug 90737 has been marked as a duplicate of this bug. ***
Product: MailNews → Core
Attachment #46138 - Attachment description: patch v6. There were exception handling problems in setupLdapAutoComplete(), both pre-existing and from this patch. Pushed try/catch statements up one-level so that unexpected failures wouldn't try and continue executing code inside the setupLdapAutoCom → patch v6. Pushed try/catch statements up one-level so that unexpected failures wouldn't try and continue executing code inside the setupLdapAutoComplete function.
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: