242014 - Creating a new LDAP directory always sets the protocolVersion to 2

Status: RESOLVED FIXED

(MailNews Core :: LDAP Integration, defect)

Description

[Scott MacGregor]

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.

Comment 1

[Scott MacGregor]

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.

Comment 2

[Scott MacGregor]

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.

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

Yes, that was the intent.  Your proposed fix sounds good to me.

Comment 4

[Scott MacGregor]

Attachment: proposed fix

Comment 5

[Scott MacGregor]

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.

Comment 6

[Scott MacGregor]

I checked this patch into the tbird 0.6 branch. Will make adjustments if any
come from the review comments.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

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...

Comment 8

[Scott MacGregor]

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

Comment 9

[Scott MacGregor]

fixed with Dan's comment suggestions.

Comment 10

[chris hofmann]

Comment on attachment 147283 [details] [diff] [review]
proposed fix

a=chofmann for 1.7

Comment 11

[Scott MacGregor]

fixed on the trunk, tbird 0.6 branch and the 1.7 branch