Closed
Bug 327263
Opened 19 years ago
Closed 3 years ago
ldap search filters are ineffective
Categories
(Thunderbird :: Address Book, defect)
Thunderbird
Address Book
Tracking
(Not tracked)
RESOLVED
WORKSFORME
People
(Reporter: neuroc0der, Assigned: aceman)
References
Details
(Keywords: helpwanted, Whiteboard: [patchlove])
Attachments
(2 obsolete files)
User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8.0.1) Gecko/20060111 Firefox/1.5.0.1
there are several issues here closely related so i list them one-by-one:
- ldap search filters currently used in the address book are substring
search filters. these while relatively harmless against small ldap
directories can be very time consuming against directories with large
number of entries to search. this is mostly an issue for corporate
deployments where it affects the time for the user to get desired
entries and also puts unnecessary additional load on the ldap server.
- displayName vs cn. displayName is not always present while cn is so
it is best to look for cn attribute not displayName attribute. this
makes the search more effective.
- in compose new mail if ldap directory lookup is enabled it consist
of cn, mail and sn attributes but is missing givenName attribute
which can also improve the search.
- autocomplete ignore strings with @ in them which is not right.
they are valid strings like email addresses and should be
allowed in the search request.
there are also issues with UI not giving any visual feedback to
the users that their search is in progress but i'll submit them
as a separate bug. i'm not sure if two should be linked together
Reproducible: Always
Reporter | ||
Comment 1•19 years ago
|
||
Attachment #211964 -
Flags: review+
Reporter | ||
Updated•19 years ago
|
Attachment #211964 -
Flags: review+ → review?
Updated•19 years ago
|
Attachment #211964 -
Flags: review? → review?(bienvenu)
Comment 2•19 years ago
|
||
(In reply to comment #0)
> - displayName vs cn. displayName is not always present while cn is so
> it is best to look for cn attribute not displayName attribute. this
> makes the search more effective.
This is currently being discussed in bug 323608. IMO we need to finish discussing it there before we can accept a patch for it.
> - autocomplete ignore strings with @ in them which is not right.
> they are valid strings like email addresses and should be
> allowed in the search request.
I think this is done on purpose and is the same with non-ldap address books, cc'ing some people who might know.
> there are also issues with UI not giving any visual feedback to
> the users that their search is in progress but i'll submit them
> as a separate bug. i'm not sure if two should be linked together
IMHO Generally its best to handle issues under separate bugs/patches as it is easier to see what is happening and also makes the patches simpler to review.
Depends on: 323608
Comment 3•19 years ago
|
||
Comment on attachment 211964 [details] [diff] [review]
suggested fix
I'm going to punt to Dan who knows much more about these issues.
Attachment #211964 -
Flags: superreview?(bienvenu)
Attachment #211964 -
Flags: review?(dmose)
Attachment #211964 -
Flags: review?(bienvenu)
Reporter | ||
Comment 4•19 years ago
|
||
Attachment #211964 -
Attachment is obsolete: true
Attachment #215876 -
Flags: review?
Attachment #211964 -
Flags: superreview?(bienvenu)
Attachment #211964 -
Flags: review?(dmose)
Comment 5•19 years ago
|
||
Comment on attachment 215876 [details] [diff] [review]
new patch against HEAD
This needs dmose' email address for him to pick it up.
Attachment #215876 -
Flags: review? → review?(dmose)
Updated•18 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: MacOS X → All
Hardware: Macintosh → All
Comment 6•18 years ago
|
||
Comment on attachment 215876 [details] [diff] [review]
new patch against HEAD
Sorry for the long delay in reviewing; I'll try and do better in the future.
>Index: mozilla/mail/locales/en-US/chrome/messenger/messenger.properties
>===================================================================
>RCS file: /cvsroot/mozilla/mail/locales/en-US/chrome/messenger/messenger.properties,v
>retrieving revision 1.17
>diff -u -8 -p -r1.17 messenger.properties
>--- mozilla/mail/locales/en-US/chrome/messenger/messenger.properties 22 Feb 2006 01:21:53 -0000 1.17
>+++ mozilla/mail/locales/en-US/chrome/messenger/messenger.properties 22 Mar 2006 09:34:51 -0000
>@@ -237,17 +237,17 @@ mail.addr_book.displayName.lastnamefirst
> # @V == the escaped value typed in the quick search bar in the addressbook
> #
> # note, changing this might require a change to SearchNameOrEmail.label
> # in messenger.dtd
> #
> # LOCALIZATION NOTES - please add phonetic names as below when "mail.addr_book.show_phonetic_fields" is true
> # "?(or(PrimaryEmail,c,@V)(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V)(PhoneticFirstName,c,@V)(PhoneticLastName,c,@V))"
> #
>-mail.addr_book.quicksearchquery.format=?(or(PrimaryEmail,c,@V)(DisplayName,c,@V)(FirstName,c,@V)(LastName,c,@V))
>+mail.addr_book.quicksearchquery.format=?(or(PrimaryEmail,bw,@V)(DisplayName,bw,@V)(FirstName,bw,@V)(LastName,bw,@V))
This seems like a reasonable change to me, but I'd be interested in Standard8's and bienvenu's thoughts on this, as it's going to effect all addressbooks, not just LDAP ones. If we do decide that this is the right thing to do, I'd suggest tweaking the example in the comment as well.
>-pref("ldap_2.servers.default.attrmap.DisplayName", "cn,commonname");
>+pref("ldap_2.servers.default.attrmap.DisplayName", "cn,displayName,commonname");
We pretty much decided over in <https://bugzilla.mozilla.org/show_bug.cgi?id=323608#c10> that we didn't want to do this.
If you wanted to file a separate bug on giving the addressbook a concept of different field/attribute mappings for search & display, that would be a fine thing, since in the current world, if we don't search on displayName, we'll never actually use it for displaying.
>Index: mozilla/mailnews/addrbook/src/nsLDAPAutoCompleteSession.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/addrbook/src/nsLDAPAutoCompleteSession.cpp,v
>retrieving revision 1.45
>diff -u -8 -p -r1.45 nsLDAPAutoCompleteSession.cpp
>--- mozilla/mailnews/addrbook/src/nsLDAPAutoCompleteSession.cpp 3 Feb 2006 14:18:09 -0000 1.45
>+++ mozilla/mailnews/addrbook/src/nsLDAPAutoCompleteSession.cpp 22 Mar 2006 09:34:54 -0000
>@@ -68,17 +68,17 @@ static PRLogModuleInfo *sLDAPAutoComplet
> // version.
> //
> NS_IMPL_THREADSAFE_ISUPPORTS3(nsLDAPAutoCompleteSession,
> nsIAutoCompleteSession, nsILDAPMessageListener,
> nsILDAPAutoCompleteSession)
>
> nsLDAPAutoCompleteSession::nsLDAPAutoCompleteSession() :
> mState(UNBOUND),
>- mFilterTemplate("(|(cn=%v1*%v2-*)(mail=%v1*%v2-*)(sn=%v1*%v2-*))"),
>+ mFilterTemplate("(|(cn=%v1*%v2-*)(mail=%v1*%v2-*)(sn=%v1*%v2-*)(givenName=%v1*%v2-*))"),
Excellent idea.
>- // ignore the empty string, strings with @ in them, and strings
>- // that are too short
>+ // ignore the empty string and strings that are too short
> //
> if (searchString[0] == 0 ||
>- nsDependentString(searchString).FindChar(PRUnichar('@'), 0) !=
>- kNotFound ||
> nsDependentString(searchString).FindChar(PRUnichar(','), 0) !=
> kNotFound ||
The original idea behind was that if you were typing a full-email address, that meant that you wanted to type it yourself. In organizations where the LDAP server only serves one domain that makes sense. In other cases, not so much. In general, this seems like a reasonable change to me, but I'd be interested in hearing thoughts from Standard8 and bienvenu on this too.
Attachment #215876 -
Flags: review?(dmose) → review-
Reporter | ||
Comment 7•18 years ago
|
||
(In reply to comment #6)
> >-pref("ldap_2.servers.default.attrmap.DisplayName", "cn,commonname");
> >+pref("ldap_2.servers.default.attrmap.DisplayName", "cn,displayName,commonname");
>
> We pretty much decided over in
> <https://bugzilla.mozilla.org/show_bug.cgi?id=323608#c10> that we didn't want
> to do this.
> If you wanted to file a separate bug on giving the addressbook a concept of
> different field/attribute mappings for search & display, that would be a fine
> thing, since in the current world, if we don't search on displayName, we'll
> never actually use it for displaying.
when i submitted the patch originally it was still undecided at the time and
i kept displayName as a safety catch just in case. i'm fine with removing it
now as it is no longer required. once you guys decide on the rest of the
changes i shall submit a new patch so let me know.
Comment 8•18 years ago
|
||
> The original idea behind was that if you were typing a full-email address,
> that meant that you wanted to type it yourself. In organizations where the
> LDAP server only serves one domain that makes sense. In other cases, not so
> much. In general, this seems like a reasonable change to me, but I'd be
> interested in hearing thoughts from Standard8 and bienvenu on this too.
In my (and my wife's) experience, which involves emailing folks from a variety of organizations, autocompleting on a string containing a @ would be useful (as long as it also applied to local addressbooks).
Comment 9•18 years ago
|
||
nsAbAutoCompleteSession::OnStartLookup would need a similar change to allow autocomplete with '@' in the string. And the autocomplete widget itself might need to be changed - I'll go look at that.
Comment 10•18 years ago
|
||
I'd definitely prefer autocomplete to keep working after I typed the '@'. I'm going to submit a patch to do that for local ab's in bug 200804 - the autocomplete part of this patch could end up in that other bug, if people want...
Comment 11•18 years ago
|
||
autocomplete after @ is now fixed for local ab's, so fixing it for ldap would have the added benefit of consistency :-)
Updated•16 years ago
|
Assignee: mscott → nobody
Comment 13•16 years ago
|
||
Comment on attachment 215876 [details] [diff] [review]
new patch against HEAD
Patch has bitrotted.
$ patch --dry-run < ~/Desktop/tbTestPatches/patch-327263.diff
patching file messenger.properties
Hunk #1 FAILED at 237.
1 out of 1 hunk FAILED -- saving rejects to file messenger.properties.rej
can't find file to patch at input line 34
Perhaps you should have used the -p or --strip option?
The text leading up to this was:
--------------------------
|Index: mozilla/mailnews/mailnews.js
|===================================================================
|RCS file: /cvsroot/mozilla/mailnews/mailnews.js,v
|retrieving revision 3.262
|diff -u -8 -p -r3.262 mailnews.js
|--- mozilla/mailnews/mailnews.js 2 Mar 2006 15:51:00 -0000 3.262
|+++ mozilla/mailnews/mailnews.js 22 Mar 2006 09:34:52 -0000
--------------------------
File to patch:
Attachment #215876 -
Attachment is obsolete: true
Comment 14•15 years ago
|
||
Anton, any chance for a new patch?
Comment 17•3 years ago
|
||
Is this still true with the introduction of js-ldap in 90.0b2? https://archive.mozilla.org/pub/thunderbird/releases/90.0b2/
Updated•3 years ago
|
Comment 18•3 years ago
|
||
Christian, can you confirm whether some or all of comment 0 is still a problem?
Flags: needinfo?(christian.fertig)
Comment 19•3 years ago
|
||
Hi,
I can confirm that the search is working fine and fast with our "small" directory (<100 Users), that @ is working in search terms and that substrings seems to function in searches. But I'm allways using ESR versions, so I can't tell anything about 90.0b2
Christian
Flags: needinfo?(christian.fertig)
Comment 20•3 years ago
|
||
Thanks. That is a sufficient test.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → WORKSFORME
You need to log in
before you can comment on or make changes to this bug.
Description
•