Closed Bug 242014 Opened 16 years ago Closed 16 years ago
Creating a new LDAP directory always sets the protocol
Version to 2
If you create a new LDAP directory from the address book, we always set .currentVersion to "2". We are supposed to be using LDAPv3 by default. This means all new LDAP directories are running the wrong protocol version. But previous LDAP directories are running v3. This happens because when we create a new directory server we call DIR_AddNewAddressBook which calls DIR_SavePrefsForOneServer. This method checks to see if the v3 flag has been set on the directory (which it has not for a new directory) so we write out the version string as "2"): http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/src/nsDirPrefs.cpp#3851 This is probably a 0.6 stopper I need to investigate.
Just to summarize my findings: 1) Existing LDAP directories are treated as v3 2) New LDAP directories are treated as v2 I assume we really intend all directories to be treated as v3 and the user must change the current protocol version string to "2" if they want v2 for that directory.
Target Milestone: --- → mozilla1.7final
When we read an old directory in we call DIR_GetPrefsForOneServer which does: char *versionString = DIR_GetStringPref(prefstring, "protocolVersion", tempstring, "3"); DIR_ForceFlag(server, DIR_LDAP_VERSION3, !strcmp(versionString, "3")); if no default protocolVersion string, we default to version3 and set the flag on the directory. When creating a new directory no one ever sets this flag on the directory server. We call DIR_AddNewAddressBook which calls DIR_SavePrefsForOneServer. This routine checks the v3 flag sees that it is not set so it writes out "2" as the version string. Next time we start up we see the version 2. The fix might be as simple as force setting the v3 pref from DIR_AddNewAddressBook.
Status: NEW → ASSIGNED
Yes, that was the intent. Your proposed fix sounds good to me.
Comment on attachment 147283 [details] [diff] [review] proposed fix What isn't shown from the diff is that this modification is inside of DIR_AddNewAddressBook under an if ldap directory condition. I'm assuming we will want to force migrated directories to be v3 which come through this routine as well.
I checked this patch into the tbird 0.6 branch. Will make adjustments if any come from the review comments.
Attachment #147283 - Flags: superreview?(bienvenu) → superreview+
Comment on attachment 147283 [details] [diff] [review] proposed fix >Index: nsDirPrefs.cpp >=================================================================== >RCS file: /cvsroot/mozilla/mailnews/addrbook/src/nsDirPrefs.cpp,v >retrieving revision 1.88 >diff -u -w -r1.88 nsDirPrefs.cpp >--- nsDirPrefs.cpp 14 Feb 2004 02:09:27 -0000 1.88 >+++ nsDirPrefs.cpp 29 Apr 2004 01:37:43 -0000 >@@ -457,6 +457,9 @@ > server->uri = nsCRT::strdup(uri); > if (authDn) > server->authDn = nsCRT::strdup(authDn); >+ // force new LDAP directories to be treated as v3...Question, how does a customer override this default? Currently, the customer can't override the default itself, but can force v2 on a specific server by editing prefs.js by hand. Can you change the comment to reflect this? >+ // I think we want to be doing this for the migration case as well... Migrating from what? 4.x? I think we do want to be doing this for the migration case as well: LDAPv2 has been declared obsolete by the IETF. LDAPv3 support has been good for years and is extremely widely deployed. Does your comment mean that the migration code path runs through these very lines? Can you change the comment to clarify? >+ DIR_ForceFlag(server, DIR_LDAP_VERSION3, PR_TRUE); > } > if (maxHits) > server->maxHits = maxHits; Anyway, looks good; thanks for cleaning up after me here. As an aside, I think DIR_Server is an abstraction that has outlived its usefulness and should be gotten rid of, since IMO all it does is complicate the code. Accessing prefs directly should be plenty fast these days. However, I know Seth doesn't agree with me on this one...
Attachment #147283 - Flags: review?(dmose) → review+
Comment on attachment 147283 [details] [diff] [review] proposed fix we'll need this for 1.7 already fixed on the tbird 0.6 branch and the trunk
Attachment #147283 - Flags: approval1.7?
fixed with Dan's comment suggestions.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment on attachment 147283 [details] [diff] [review] proposed fix a=chofmann for 1.7
Attachment #147283 - Flags: approval1.7? → approval1.7+
fixed on the trunk, tbird 0.6 branch and the 1.7 branch
You need to log in before you can comment on or make changes to this bug.