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)
MailNews Core
LDAP Integration
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.
Updated•23 years ago
|
Component: LDAP Tools → LDAP Mail/News Integration
Product: Directory → MailNews
QA Contact: __UNKNOWN__
Target Milestone: --- → mozilla0.9.3
Assignee | ||
Comment 2•23 years ago
|
||
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
Assignee | ||
Updated•23 years ago
|
Priority: -- → P3
Comment 4•23 years ago
|
||
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}>");
Assignee | ||
Comment 5•23 years ago
|
||
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
Updated•23 years ago
|
Priority: P3 → P2
Updated•23 years ago
|
Whiteboard: [relnote] Document workaround in release notes
Comment 6•23 years ago
|
||
Greetings from Burbank: we need this bad :-)
Assignee | ||
Updated•23 years ago
|
Target Milestone: mozilla0.9.3 → mozilla0.9.4
Comment 8•23 years ago
|
||
Assignee | ||
Comment 9•23 years ago
|
||
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?
Assignee | ||
Comment 10•23 years ago
|
||
Assignee | ||
Comment 11•23 years ago
|
||
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.
Assignee | ||
Comment 12•23 years ago
|
||
Assignee | ||
Updated•23 years ago
|
Comment 13•23 years ago
|
||
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
Assignee | ||
Comment 14•23 years ago
|
||
Assignee | ||
Comment 15•23 years ago
|
||
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=
Comment 16•23 years ago
|
||
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•23 years ago
|
||
> 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•23 years ago
|
||
ok, never mind, my bad - more invisible fun with templates.
Assignee | ||
Comment 19•23 years ago
|
||
> + 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
Assignee | ||
Comment 20•23 years ago
|
||
Comment 21•23 years ago
|
||
there are more instances of dropping error codes in nsAbLDAPAutoCompleteFormatter
fix those, and sr=bienvenu
Assignee | ||
Comment 22•23 years ago
|
||
Assignee | ||
Comment 23•23 years ago
|
||
Assignee | ||
Comment 24•23 years ago
|
||
Comment 25•23 years ago
|
||
r=srilatha for the MsgComposeCommands.js part of the patch
Assignee | ||
Comment 26•23 years ago
|
||
Assignee | ||
Comment 27•23 years ago
|
||
Assignee | ||
Comment 28•23 years ago
|
||
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•23 years ago
|
||
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.
Updated•23 years ago
|
QA Contact: yulian
Assignee | ||
Comment 30•23 years ago
|
||
Fix checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Whiteboard: have patch, r=, sr=; test-building on all platforms
Assignee | ||
Comment 32•23 years ago
|
||
*** Bug 96739 has been marked as a duplicate of this bug. ***
Comment 33•23 years ago
|
||
*** Bug 95317 has been marked as a duplicate of this bug. ***
Comment 34•23 years ago
|
||
*** Bug 90737 has been marked as a duplicate of this bug. ***
Updated•20 years ago
|
Product: MailNews → Core
Updated•17 years ago
|
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.
Updated•16 years ago
|
Product: Core → MailNews Core
You need to log in
before you can comment on or make changes to this bug.
Description
•