Closed Bug 242014 Opened 16 years ago Closed 16 years ago

Creating a new LDAP directory always sets the protocolVersion to 2

Categories

(MailNews Core :: LDAP Integration, defect)

x86
Windows 2000
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.7final

People

(Reporter: mscott, Assigned: mscott)

References

Details

(Keywords: fixed1.7)

Attachments

(1 file)

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.
Attached patch proposed fixSplinter Review
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.
Attachment #147283 - Flags: superreview?(bienvenu)
Attachment #147283 - Flags: review?(dmose)
Flags: blocking1.7?
Flags: blocking1.7?
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
Keywords: fixed1.7
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.