119291 - LDAP attributes should be customizable

Status: RESOLVED FIXED

(MailNews Core :: LDAP Integration, defect, P1)

Description

[Roland Felnhofer]

LDAP attribute definition should be more flexible. 
Default LDAP attributes being used in Mozilla AB, should be defined in
'mailnews.js'.
Users should have the possibility to overwrite these default settings by making
entries in 'prefs.js'

Example:
pref("ldap_2.servers.attributes.countryname", "countryname");

user_pref("ldap_2.servers.roka.attributes.countryname", "co");

Comment 1

[Chan Min Wai]

Anyone try to make this possiblem and what would be the problem for LDAP write
function?

Comment 2

[Mats Larsson]

Anyone working on this?

Comment 3

[Mats Larsson]

Still nobody working on this?

Comment 4

[Scott MacGregor]

a combination of dmose, David and myself may try to take a look at this in the
coming months.

Comment 5

[Mats Larsson]

Much appreciated.

Comment 6

[Scott MacGregor]

I think in 4.x this worked a little differently...

the pref contained both a display name and the ldap attribute to use....i.e.:

user_pref("ldap_2.servers.roka.attributes.street", "State:st");

That way the UI shows the string "State" instead of street and we use the ldap
attribute, "st". Do I have that right?

Comment 7

[Mats Larsson]

According to http://developer.netscape.com/docs/manuals/communicator/ldap45.htm both

pref("ldap_2.servers.megacorp.attributes.<standard_attr>", "<your_attr>");

and

pref("ldap_2.servers.megacorp.attributes.<standard_attr>",
"<displayed_name>:<your_attr>");

are valid. The second form is used when you want to change the display name of
the attribute.

Comment 8

[David :Bienvenu]

taking

Comment 9

[jon]

has there been any progres with this bug? Will it be possible to show images,
like jpegPhoto in inetOrgPerson?

Comment 10

[Chris Ebenezer]

4.7X also has the support for custom filters, which specify the order in which 
various subfields of the entry will be used to compose queries to the server.  
This also doesn't appear to be supported.

Comment 11

[Mats Larsson]

Any progress David?

Comment 12

[David :Bienvenu]

no, sorry, I've been swamped with non-ldap stuff

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Chris: the autocomplete code allows one to set custom filters, though the
addressbook code does not yet.  

http://lxr.mozilla.org/seamonkey/source/mailnews/addrbook/public/nsIAbLDAPAutoCompFormatter.idl

http://lxr.mozilla.org/seamonkey/source/mailnews/compose/resources/content/MsgComposeCommands.js#864

have more information on how this works.

Comment 14

[John Woodell]

Seems to me that this should work just like the LDAP browser
that was integrated with Netscape 4.x, and live on the server.

This data exists with Netscape Directory servers, but with
OpenLDAP I have a file called 'ldaptemplates.conf' that ends...

item cis        "Street Address"        streetAddress
item cis        "Locality"              l
item cis        "State or Province"     st
item cis        "Postal Code"           postalCode
. . .
Item cis        "Other Mailbox"         otherMailbox
item time,ro    "Last Modified"         lastModifiedTime
item dn,ro      "Modified By"           lastModifiedBy

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #14)
> Seems to me that this should work just like the LDAP browser
> that was integrated with Netscape 4.x, and live on the server.

As far as I can tell, the two parts of this sentence contradict each other,
unless you're counting "autoconfig" into the equation.  In 4.x, these were
specified with preferences, not on the server.

Comment 16

[Mark Smith [:mcs]]

*** Bug 281389 has been marked as a duplicate of this bug. ***

Comment 17

[Mats Larsson]

Any progress David?

Comment 18

[Craig Harding]

The full thunderbird attribute list is something that I've been looking for,
especially the extra ones that aren't listed on this page:
http://www.mozilla.org/projects/thunderbird/specs/ldap.html

The Home Address fields aren't listed and I don't know what their attribute
fields would be called.. anyone know?

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: proposed interface for an attribute mapping object, v1

Attached is an interface for an object intended to replace
nsAbLDAPProperties.cpp.  It allows for a precedence-ordered list of attributes
to be specified per addressbook property (though someday it may be worth
simplifying to only allow one attribute per-property).	I'm going to start
writing a JS component implementing this interface (shaver and vlad tell me it
should be fast enough for this).  The list will be read from prefs, and thus
will be overridable globally as well on a per-server basis.  The intent is that
this interface be used by the LDIF import and export code too.

Comments welcome.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: proposed interface & code, v2

Now with a service interface, some JS scaffolding, and first work at replacing
the calls to the old code.  More to come.

Comment 21

[Mats Larsson]

Will this make it into Firefox/Thunderbird as well?

Comment 22

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #21)
> Will this make it into Firefox/Thunderbird as well?

Thunderbird, yes.  Firefox isn't built with LDAP support at all.

Comment 23

[Mats Larsson]

Tbird of course. My stupidity. Are we talking Tbird 1.1 or?

Comment 24

[John Woodell]

This feature could make it difficult to document how LDAP mapping works...
becasue it can be changed. Also, I would not want a user to be able to 
set ( 'co' 'friendlyCountryName' ) instead of ( 'c' 'countryName' )
and then bust the 'Get Map' button, without realizing it.

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #23)
> Tbird of course. My stupidity. Are we talking Tbird 1.1 or?

It is my hope that this will make it into 1.1, yes.

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #24)
> This feature could make it difficult to document how LDAP mapping works...
> becasue it can be changed. 

Slightly more complex documentation will have to be written, that's true. 
However, this is a degree of flexibility that sysadmins truly need, I think. 
Lots of times as an admin, you may not have the option of picking the mozilla
compliant schema for your data, and this is the best you'll be able to do.

> Also, I would not want a user to be able to 
> set ( 'co' 'friendlyCountryName' ) instead of ( 'c' 'countryName' )
> and then bust the 'Get Map' button, without realizing it.

The only way that a user is going to be able to mess with this is by mucking
directly in the preferences, at which point they're already playing with fire.

Comment 27

[John Woodell]

(In reply to comment #26)
> However, this is a degree of flexibility that sysadmins truly need...

I agree, this is all good stuff, but I hope we don't end up with something 
that is overly complex (and possibly flawed) as a reaction to the feature  
being half-baked for so many years. My hope is that when the standard mapping
for all AB attributes is finally released, with the right mix of open-standards
meets Outlook/ActiveDirectory compatibility... admins will say, "gosh, this is
just what we wanted for so many years, and I have more important things to do
than to learn how this works, and then muddle with it."

Unfortunately, I would much rather have a dropdown for country, that stores
and exports both the country code and friendly name to LDIF, than have a 
flexible mapping that makes life difficult for users and admins. Further,
I would hope that address fields 1 and 2 are merged into a multi-line field
like outlook and palm-desktop use. This kind of power in the GUI makes a
one-to-one mapping hard to imagine.

Finally, the ability to relabel Custom 1-4 are on the top of my wish list,
but changing the name of the attribute in LDAP is at the bottom.

All that said, I am confident that we are in good shape 
with whatever dmose comes up with.   :-)

Comment 28

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: proposed interface & code, v3

OK, in this patch, the interface is more fleshed out, and I've changed almost
all of the users of nsAbLDAPProperties to use it.  Still need to implement the
map and service components in JS, and do various cleanups.

Comment 29

[Mats Larsson]

We've been waiting for this feature for years. It was there in Netscape 4 and on
my site we  (I as an andmin) used it to customize the Netscape AB to our needs.
This is a much needed Enterprise feature (almost as much as autoconfig is) and I
for sure hope dmose will have the time to finish it for both Moz and Tbird this
time. And I really want it in both Moz and Tbird asap since both is used at my
site. Keep up the good work.

Comment 30

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #27)
>
> I agree, this is all good stuff, but I hope we don't end up with something 
> that is overly complex (and possibly flawed) as a reaction to the feature  
> being half-baked for so many years. My hope is that when the standard mapping
> for all AB attributes is finally released, with the right mix of 
> open-standards meets Outlook/ActiveDirectory compatibility... admins will
> say, "gosh, this is just what we wanted for so many years, and I have more
> important things to do than to learn how this works, and then muddle with it."

Many admins will, I hope, say that too.  However, many won't have that luxury. 
Often, LDAP schemata are determined by interactions with other products, 
and it's typically hard to make changes to groups of applications all at once.

In fact, part of the reason I'm focussing on this bug now is that I no longer
believe we're going to be able to hit exactly the right balance with the default
schema on the first time out.  In the spirit of "worse-is-better", I think it
makes more sense to try and land the default schema, warts and all, sooner
rather than later.  But in order to make it reasonable to land an imperfect
schema, I've come to believe that it needs to be customizable so that admins can
tweak and change it to match their local setup.

> Unfortunately, I would much rather have a dropdown for country, that stores
> and exports both the country code and friendly name to LDIF, than have a 
> flexible mapping that makes life difficult for users and admins. Further,
> I would hope that address fields 1 and 2 are merged into a multi-line field
> like outlook and palm-desktop use. This kind of power in the GUI makes a
> one-to-one mapping hard to imagine.
> 
> Finally, the ability to relabel Custom 1-4 are on the top of my wish list,
> but changing the name of the attribute in LDAP is at the bottom.

Indeed, there are lots of things to address in Mozilla's LDAP support, and
everyone would prioritize that list differently, I'm sure.  :-/

> All that said, I am confident that we are in good shape 
> with whatever dmose comes up with.   :-)

Thanks.  :-)

Comment 31

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v4

Component is largely implemented now.  Included directory/xpcom/public/nsLDAP.h
diffs that I forgot last time.	More to come.

Comment 32

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch v5

Commentary on the general strategy here: the plan is to have a default global
mapping of properties to lists of ldap attributes, similar to the way things
work in the existing code.  However, this mapping will be defined entirely in
prefs.	Additionally, by setting a pref, a user will be able to override this
mapping on a per-server basis.	Any given LDAP attribute will only be allowed
to map to a single addressbook field.

In the interest of landing this sooner rather than later, this patch does not
attempt to solve the labelling of addressbook fields in the GUI as 4.x did.

Comment 33

[David :Bienvenu]

the mail stuff shouldn't be part of this patch :-)
Index: mail/base/content/mailOverlay.xul, etc.


+NS_IMETHODIMP nsAbDirectoryQueryArguments::GetTypeSpecificArg(nsISupports** aArg)
+{
+    if (!aArg)
+        return NS_ERROR_NULL_POINTER;
+

can you use NS_ENSURE_ARG_POINTER(aArg); or will that generate spurious warnings?

Comment 34

[Mats Larsson]

(In reply to comment #32)

> In the interest of landing this sooner rather than later, this patch does not
> attempt to solve the labelling of addressbook fields in the GUI as 4.x did.

That's fine with me but do you plan to implement this later on?

Comment 35

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #33)
> the mail stuff shouldn't be part of this patch :-)
> Index: mail/base/content/mailOverlay.xul, etc.
>
> [...]
> 
> can you use NS_ENSURE_ARG_POINTER(aArg); or will that generate spurious warnings?

Good catches; next version will have both of those fixed.

Comment 36

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #34)
> (In reply to comment #32)
> 
> > In the interest of landing this sooner rather than later, this patch does not
> > attempt to solve the labelling of addressbook fields in the GUI as 4.x did.
> 
> That's fine with me but do you plan to implement this later on?

In some ideal world, yes.  Realistically, it's not super high on my priority
list.  But I'd certainly help anyone who was interested in working on patches to
this end.

Comment 37

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch v6

It works.  Lots more implementation and tons of cleanup.

Comment 38

[David :Bienvenu]

looks good, though this code is not my particular area of expertise...can you
get a good sr from someone like shaver or neil?

             if (!mFinished && !mWaitingForPrevQueryToFinish)
             {
             rv = OnLDAPMessageSearchEntry (aMessage, getter_AddRefs (queryResult));
-            NS_ENSURE_SUCCESS(rv, rv);
             }
             break;

indentation here looks funny (probably my fault :-( )


@@ -387,6 +388,23 @@ NS_IMETHODIMP nsAbLDAPDirectory::StartSe

in this method, there are a lot of NS_ENSURE_SUCCESS(rv, rv); 

Are all those errors fatal, or is there a reasonable fallback?

I'll put my r=

Comment 39

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #38)
> looks good, though this code is not my particular area of expertise...can you
> get a good sr from someone like shaver or neil?

OK, thanks.

> 
>              if (!mFinished && !mWaitingForPrevQueryToFinish)
>              {
>              rv = OnLDAPMessageSearchEntry (aMessage, getter_AddRefs
(queryResult));
> -            NS_ENSURE_SUCCESS(rv, rv);
>              }
>              break;
> 
> indentation here looks funny (probably my fault :-( )

Fixed in my tree.
 
> @@ -387,6 +388,23 @@ NS_IMETHODIMP nsAbLDAPDirectory::StartSe
> 
> in this method, there are a lot of NS_ENSURE_SUCCESS(rv, rv); 
> 
> Are all those errors fatal, or is there a reasonable fallback?

They're pretty much all fatal: if you can't get a map object, there's no point
in starting the search, because even if results came back, you'd have no way to
map them back to addressbook fields.

Comment 40

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 183204 [details] [diff] [review]
patch v6

>+++ mailnews/addrbook/public/Makefile.in	10 May 2005 21:52:37 -0000
>@@ -76,8 +76,8 @@ XPIDLSRCS       += \
>                 nsIAbLDAPReplicationService.idl \
>                 nsIAbLDAPReplicationQuery.idl \
>                 nsIAbLDAPReplicationData.idl \
>+		nsIAbLDAPAttributeMap.idl \
> 		$(NULL)
> endif

Indentation.

>+/**
>+ * A mapping between addressbook properties and ldap attributes.
>+ *
>+ * Each addressbook property can map to one or more attributes.  If
>+ * there is no entry in preferences for a field, the getters generally
>+ * return NS_ERROR_FAILURE.  If there is a null entry, NS_ERROR_ABORT
>+ * is returned.  The idea is that a property which has a default set
>+ * of attributes can be overridden for a specific server with a
>+ * zero-length string.  So in general, the semantics should be the
>+ * same in both cases, and callers will implement that by just
>+ * checking for success or failure.  If, however, it's really
>+ * important for some caller to see which case is hit, the specific
>+ * error code can be inspected.

That seems pretty script-hostile, and I wouldn't expect to get _ABORT for a
null entry.  (Is a null entry what you get if there's an empty string
specified?)

Could we use an empty string, and make it be Void for no-entry case, with a
successful return for both cases?  That would give the same empty-string dual
behaviour we have now, without making exception-aware callers wrap in try 
blocks.  (In JS, they would get a null string for one case, and an empty string
for the other, but both test as false, making for a natural pattern there as
well.)

(We talked about this on IRC, and I think settled on something like the above,
with a %{ C++ %} ABOOK_FOUND_ENTRY(rv, str) helper.)

>+    // if the first item is a null-string, abort
>+    aAttrs = this.mPropertyMap[aProperty];
>+    if (!aAttrs[0].length) {
>+      throw Components.results.NS_ERROR_ABORT;
>+    }

"null string" != "empty string".  In this case I think you will end up
returning null from JS, which will give you an empty + IsVoid string in C++. 
Should the aAttrs array follow the same convention?

Also, is it guaranteed that aAttrs will never come back with an empty array? 
If it does, you'll throw something entirely different here
(XPC_JS_SCRIPT_ERROR) or some such.

>+    var attrs = aAttributeList.split(",");
>+
>+    // check to make sure this call won't allow multiple mappings to be
>+    // created, if requested
>+    if (!aAllowInconsistencies) {
>+      for (var attr in attrs) {

In this case, attr will be integers from 0 to attrs.length,
not necessarily in order.  Do you mean |for each|?

>+    // delete any attr mappings created by the existing property map entry
>+    for (attr in this.mPropertyMap[aProperty]) {
>+      delete this.mAttrMap[attr];
>+    }

|for each| here?

>+    // add these attrs to the attrmap
>+    for (attr in attrs) {
>+      this.mAttrMap[attr] = aProperty;
>+    }

And again.

>+  setCardPropertiesFromLDAPMessage: function
>+    setCardPropertiesFromLDAPMessage(aMessage, aCard) {
>+
>+    var cardValueWasSet = false;
>+
>+    var msgAttrCount = {};
>+    var msgAttrs = aMessage.getAttributes(msgAttrCount);
>+
>+    // downcase the array for comparison
>+    function toLower(a) { return a.toLowerCase(); }
>+    msgAttrs = msgAttrs.map(toLower);

Righteous!  I almost want a variant that uses the element
as |this|, but I can't bring myself to write it to save the
two-liners that replace it.

>+          try {
>+            var valueCount = {};
>+            var values = aMessage.getValues(attr, valueCount);
>+            aCard.setCardValue(prop, values[0]);
>+
>+            cardValueWasSet = true;
>+          } catch (ex) {

When discarding an out param's value like that, it's
customary to just inline the object literal:

   var values = aMessage.getValues(attr, {});

Pretty common pattern in our JS for "pointless out".

>+
>+  QueryInterface: function QueryInterface(iid) {
>+    if (iid.equals(Components.interfaces.nsIAbLDAPAttributeMap) ||
>+        iid.equals(Components.interfaces.nsISupports))
>+      return this;
>+
>+    Components.returnCode = Components.results.NS_ERROR_NO_INTERFACE;
>+    return null;
>+  }
>+}

Is that better than throwing?  I usually structure that as

if (!iid.equals(CI.nsIAbLDAPAttributeMap) &&
    !iid.equals(CI.nsISupports) {
    throw Components.results.NS_ERROR_NO_INTERFACE;
}

return this;

because I prefer early-out for error, but tastes can vary.

>+  registerSelf: function (compMgr, fileSpec, location, type) {
>+    if (this.firstTime) {
>+      debug("*** Deferring registration of Addressbook LDAP Attribute" 
>+            + " Map components\n");
>+      this.firstTime = false;
>+      throw Components.results.NS_ERROR_FACTORY_REGISTER_AGAIN;
>+    }

Too much copying from the sample code!	You don't need the
didactic first-time failure, or, IMO, the big block comment
preceding it.  (I'm glad people find the sample component
to be useful, though!)

>+    if (cid.equals(NS_ABLDAPATTRIBUTEMAP_CID)) {
>+      return this.nsAbLDAPAttributeMapFactory;
>+    } else if (cid.equals(NS_ABLDAPATTRIBUTEMAPSERVICE_CID)) {
>+      return this.nsAbLDAPAttributeMapServiceFactory;
>+    }

else-after-return is a non sequitur (which is why you don't
have an "else" before the throw, I bet =) ).

>+        // since we've added attrs, we definitely need a comma next time
>+        // we're here
>+        needComma = PR_TRUE;

Ah, for a join. =)

sr=shaver with the string/throw fixes, sample code copying, careful checking of
for/for each.  The rest is to your taste.

Comment 41

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 183204 [details] [diff] [review]
patch v6

Approving much-needed LDAP love for 1.8b2.

Comment 42

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #40)
> Could we use an empty string, and make it be Void for no-entry case, with a
> successful return for both cases?  That would give the same empty-string dual
> behaviour we have now, without making exception-aware callers wrap in try 
> blocks.  (In JS, they would get a null string for one case, and an empty string
> for the other, but both test as false, making for a natural pattern there as
> well.)
> 
> (We talked about this on IRC, and I think settled on something like the above,
> with a %{ C++ %} ABOOK_FOUND_ENTRY(rv, str) helper.)

Done.
 
> >+    // if the first item is a null-string, abort
> >+    aAttrs = this.mPropertyMap[aProperty];
> >+    if (!aAttrs[0].length) {
> >+      throw Components.results.NS_ERROR_ABORT;
> >+    }
> 
> "null string" != "empty string".  In this case I think you will end up
> returning null from JS, which will give you an empty + IsVoid string in C++. 
> Should the aAttrs array follow the same convention?
> 
> Also, is it guaranteed that aAttrs will never come back with an empty array? 
> If it does, you'll throw something entirely different here
> (XPC_JS_SCRIPT_ERROR) or some such.

There could well be no entry for a property when getAttributes is called.  So
in that case, I've chosen to explicitly throw NS_ERROR_FAILURE.

> >+    var attrs = aAttributeList.split(",");
> >+
> >+    // check to make sure this call won't allow multiple mappings to be
> >+    // created, if requested
> >+    if (!aAllowInconsistencies) {
> >+      for (var attr in attrs) {
> 
> In this case, attr will be integers from 0 to attrs.length,
> not necessarily in order.  Do you mean |for each|?

I do indeed; good catch!

> >+    // delete any attr mappings created by the existing property map entry
> >+    for (attr in this.mPropertyMap[aProperty]) {
> >+      delete this.mAttrMap[attr];
> >+    }
> 
> |for each| here?

Fixed.

> >+    // add these attrs to the attrmap
> >+    for (attr in attrs) {
> >+      this.mAttrMap[attr] = aProperty;
> >+    }
> 
> And again.

Fixed.

> >+  setCardPropertiesFromLDAPMessage: function
> >+    setCardPropertiesFromLDAPMessage(aMessage, aCard) {
> >+
> >+    var cardValueWasSet = false;
> >+
> >+    var msgAttrCount = {};
> >+    var msgAttrs = aMessage.getAttributes(msgAttrCount);
> >+
> >+    // downcase the array for comparison
> >+    function toLower(a) { return a.toLowerCase(); }
> >+    msgAttrs = msgAttrs.map(toLower);
> 
> Righteous!  I almost want a variant that uses the element
> as |this|, but I can't bring myself to write it to save the
> two-liners that replace it.

Heh, yeah, I thought about that too, but it didn't seem worth the bother. 
Besides which, this pattern was copied from devmo-test.  :-)

> 
> >+          try {
> >+            var valueCount = {};
> >+            var values = aMessage.getValues(attr, valueCount);
> >+            aCard.setCardValue(prop, values[0]);
> >+
> >+            cardValueWasSet = true;
> >+          } catch (ex) {
> 
> When discarding an out param's value like that, it's
> customary to just inline the object literal:
> 
>    var values = aMessage.getValues(attr, {});
> 
> Pretty common pattern in our JS for "pointless out".

Fixed.

> >+
> >+  QueryInterface: function QueryInterface(iid) {
> >+    if (iid.equals(Components.interfaces.nsIAbLDAPAttributeMap) ||
> >+        iid.equals(Components.interfaces.nsISupports))
> >+      return this;
> >+
> >+    Components.returnCode = Components.results.NS_ERROR_NO_INTERFACE;
> >+    return null;
> >+  }
> >+}
> 
> Is that better than throwing?  I usually structure that as
> 
> if (!iid.equals(CI.nsIAbLDAPAttributeMap) &&
>     !iid.equals(CI.nsISupports) {
>     throw Components.results.NS_ERROR_NO_INTERFACE;
> }
> 
> return this;
> 
> because I prefer early-out for error, but tastes can vary.

Good points; changed.

> >+  registerSelf: function (compMgr, fileSpec, location, type) {
> >+    if (this.firstTime) {
> >+      debug("*** Deferring registration of Addressbook LDAP Attribute" 
> >+            + " Map components\n");
> >+      this.firstTime = false;
> >+      throw Components.results.NS_ERROR_FACTORY_REGISTER_AGAIN;
> >+    }
> 
> Too much copying from the sample code!	You don't need the
> didactic first-time failure, or, IMO, the big block comment
> preceding it.  (I'm glad people find the sample component
> to be useful, though!)

You're right; that stuff was unnecessary.  Elided.

> >+    if (cid.equals(NS_ABLDAPATTRIBUTEMAP_CID)) {
> >+      return this.nsAbLDAPAttributeMapFactory;
> >+    } else if (cid.equals(NS_ABLDAPATTRIBUTEMAPSERVICE_CID)) {
> >+      return this.nsAbLDAPAttributeMapServiceFactory;
> >+    }
> 
> else-after-return is a non sequitur (which is why you don't
> have an "else" before the throw, I bet =) ).

Good point; cleaned up.

New patch forthcoming.

Comment 43

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: patch, v7

Patch with comments addressed and license boilerplate updated.	Carrying
forward r/sr/a.

Comment 44

[Dan Mosedale (:dmosedale, :dmose)]

Fix checked in.  Mats, could you file a new bug for GUI tweaks of the
addressbook field names?  I think that's probably a general addressbook bug, and
not LDAP specific.

Comment 45

[Roland Felnhofer]

(In reply to comment #44)
Thank you very much !!!!

Comment 46

[Darin Fisher]

Drive-by review:

It looks like nsIAbDirectoryQueryArguments needs to have its UUID changed since
an attribute was added.

Comment 47

[Dan Mosedale (:dmosedale, :dmose)]

Attachment: bump IID as suggested by darin

Comment 48

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 186338 [details] [diff] [review]
bump IID as suggested by darin

r+sr+a=shaver

Comment 49

[Pascal Welter]

This work looks great but what about the private
information (home adress and so on) ?
Looks like the fiels are shown but not queried from
ldap. Is that right ?

Comment 50

[Roland Felnhofer]

(In reply to comment #49)
Hi Pascal,
just add that to your 'prefs.js':

user_pref("ldap_2.servers.default.attrmap.Custom1", "custom1,mozillacustom1");
user_pref("ldap_2.servers.default.attrmap.Custom2", "custom2,mozillacustom2");
user_pref("ldap_2.servers.default.attrmap.Custom3", "custom3,mozillacustom3");
user_pref("ldap_2.servers.default.attrmap.Custom4", "custom4,mozillacustom4");
user_pref("ldap_2.servers.default.attrmap.HomeAddress",
"homepostaladdress,mozillahomestreet");
user_pref("ldap_2.servers.default.attrmap.HomeAddress2",
"mozillahomepostaladdress2,mozillahomestreet2");
user_pref("ldap_2.servers.default.attrmap.HomeCity", "mozillahomelocalityname");
user_pref("ldap_2.servers.default.attrmap.HomeCountry",
"mozillahomecountryname,mozillahomefriendlycountryname");
user_pref("ldap_2.servers.default.attrmap.HomeState", "mozillahomestate");
user_pref("ldap_2.servers.default.attrmap.HomeZipCode", "mozillahomepostalcode");
user_pref("ldap_2.servers.default.attrmap.NickName", "mozillanickname");
user_pref("ldap_2.servers.default.attrmap.PreferMailFormat", "mozillausehtmlmail");
user_pref("ldap_2.servers.default.attrmap.SecondEmail", "mailalternateaddress");
user_pref("ldap_2.servers.default.attrmap.WebPage1", "url,mozillaworkurl");
user_pref("ldap_2.servers.default.attrmap.WebPage2", "mozillahomeurl");
user_pref("ldap_2.servers.default.attrmap.WorkAddress",
"postofficebox,postaladdress,streetaddress,street");
user_pref("ldap_2.servers.default.attrmap.WorkAddress2",
"mozillapostaladdress2,mozillaworkstreet2");
user_pref("ldap_2.servers.default.attrmap.WorkCountry", "c,co");
user_pref("ldap_2.servers.default.attrmap._AimScreenName", "nsaimid");

Comment 51

[Wes Perdue]

(In reply to comment #50)

Roland,

Is this fix included in 1.5b1?  If so, I can't determine exactly how to use this 
new feature.  I've added one field to our LDAP database: gfnSkypeId.  I've added 
the line:

user_pref("ldap_2.servers.default.attrmap.gfnSkypeId", "gfnSkypeId");

to user.js and the attribute doesn't show up on a query for a user with that 
attribute.  I've also tried:

user_pref("ldap_2.servers.default.attrmap.gfnSkypeId", "mozillanickname");

hoping to map it to an existing Thunderbird field, but that didn't work either.

What am I missing?

Thanks!

Comment 52

[Roland Felnhofer]

(In reply to comment #51)

> Is this fix included in 1.5b1?
I don't know, but goto: Tools - Options... - Advanced - Config Editor
and filter for: attrmap
If you see 30-40 line it will work.

> user_pref("ldap_2.servers.default.attrmap.gfnSkypeId", "mozillanickname");
It works the other way round ;-)

user_pref("ldap_2.servers.default.attrmap.NickName", "gfnSkypeId");

user_pref("ldap_2.servers.default.attrmap.Mozilla Address Book Field Name",
"Your LDAP attribute name");

Cheers
Roland

Comment 53

[Jantje Smit]

(In reply to comment #52)

following your instructions, i see the list of configurable attributes. when i
edit an attribute (and restart TB) i don't see any changes in the addressbook.

i'm using 'version 1.5 Beta 1 (20050908)'. would this mean that it is not
supported in the abook of this version?

Comment 54

[Roland Felnhofer]

(In reply to comment #53)

> i'm using 'version 1.5 Beta 1 (20050908)'. would this mean that it is not
> supported in the abook of this version?
I don't thinks so - I'm using Version 1.0+ (20050715) and it works!
Are you sure about the version number 1.5 looks a little bit high.

Just to be sure - you've edited the correct "prefs.js" from Thunderbird (NOT
Firefox) and your LDAP server works.

Comment 55

[Mark Banner (:standard8)]

*** Bug 215326 has been marked as a duplicate of this bug. ***

Comment 56

[u115577]

How do I set the commentFormat for custom attributes?
The following format is valid?

user_pref(
 "ldap_2.servers.DIRECTORYNAME.autoComplete.commentFormat", 
 "Department: [nsCalOrgUnit2]");

It would be helpful if someone could update this page:
http://www.mozilla.org/projects/thunderbird/specs/ldap.html

Comment 57

[Dan Mosedale (:dmosedale, :dmose)]

(In reply to comment #56)
> How do I set the commentFormat for custom attributes?
> The following format is valid?
> 
> user_pref(
>  "ldap_2.servers.DIRECTORYNAME.autoComplete.commentFormat", 
>  "Department: [nsCalOrgUnit2]");

That should work, yeah.

> It would be helpful if someone could update this page:
> http://www.mozilla.org/projects/thunderbird/specs/ldap.html

Agreed, this would be a fine thing.  If anyone on this bug is interested in
transferring that information to wiki.mozilla.org and updating it there, please
let me know, and I can help you get started.

Comment 58

[Thomas Bauer]

I use thunderbird release 1.5 and successfully managed to use the ldap displayname as displayname instead of the default cn with the following preference:

user_pref("ldap_2.servers.default.attrmap.DisplayName","DisplayName");

However, I can not achieve the same for the autoCompletion, although comment #13 states that something similar should work.

I tried:

user_pref("ldap_2.servers.myServer.autoComplete.nameFormat", "[displayName]");
and
user_pref("ldap_2.servers.myServer_1.autoComplete.nameFormat", "[displayName]");

where myServer is the name of my server, but none seems to work, autocomplete still uses the cn (in my case a number for each user) as displayname.

Any help is appreciated.

Comment 59

[Thomas Bauer]

sorry, as always, after trying lots of adjustments for over an hour, and filing a bug report, I found the problem myself:

user_pref("ldap_2.servers.myServer_1.autoComplete.nameFormat", "[displayName]");

works, I just took the wrong version of the server name. In pref.js I found two versions, one with an underscore and one without, anhd of course I first took the wrong one.

Comment 60

[Mats Larsson]

(In reply to comment #44)
> Fix checked in.  Mats, could you file a new bug for GUI tweaks of the
> addressbook field names?  I think that's probably a general addressbook bug, and
> not LDAP specific.

Finally filed as https://bugzilla.mozilla.org/show_bug.cgi?id=324389