Last Comment Bug 316170 - Address Book LDAP Replication doesn't work for authenticated connections.
: Address Book LDAP Replication doesn't work for authenticated connections.
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Address Book (show other bugs)
: Trunk
: All All
: -- normal with 3 votes (vote)
: ---
Assigned To: Mark Banner (:standard8)
:
Mentors:
http://bonsai.mozilla.org/cvsblame.cg...
: 375268 393892 426337 436095 480931 (view as bug list)
Depends on:
Blocks: 311632
  Show dependency treegraph
 
Reported: 2005-11-12 04:26 PST by Mark Banner (:standard8)
Modified: 2012-03-16 01:43 PDT (History)
19 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
First version (10.72 KB, patch)
2005-11-12 05:25 PST, Mark Banner (:standard8)
no flags Details | Diff | Review
Second version WIP (don't use) (34.66 KB, patch)
2007-03-11 15:41 PDT, Mark Banner (:standard8)
no flags Details | Diff | Review
Remove change log code from build (checked in). (7.48 KB, patch)
2007-03-12 14:48 PDT, Mark Banner (:standard8)
mozilla: review+
mozilla: superreview+
Details | Diff | Review
Third version (diff -w) (32.88 KB, patch)
2007-03-13 13:40 PDT, Mark Banner (:standard8)
neil: review+
mozilla: superreview+
Details | Diff | Review
Third version (normal patch) (38.63 KB, patch)
2007-03-13 13:41 PDT, Mark Banner (:standard8)
no flags Details | Diff | Review

Description Mark Banner (:standard8) 2005-11-12 04:26:43 PST
Bug 231965 changed LDAP Replication to use the default download all option, rather than change log. This was a temporary fix for 1.5 to get LDAP Replication working again, with bug 311632 to revert back to change log replication later on.

However bug 231965 didn't go far enough - whilst it now works fine for non-authenticated connections, it doesn't for ones that require authentication. I missed this as I was using a server that supported either :-(

The problem line is shown in the URL above - we're passing null as our bind dn. Its a bit more work than just specifying the dn, but I think we might be able to sort it for the next version if we want it.
Comment 1 Mark Banner (:standard8) 2005-11-12 05:25:56 PST
Created attachment 202796 [details] [diff] [review]
First version

I think this patch fixes the problem. It copies a lot of code from the ldap directory query section but I've done this with the 1.5 branch in mind, trying to keep the minimum of changes. We really could do with a follow up to synchronise the authentication of the 3 bits of code (ldap directory, replication & change log).

If anyone can test this and verify my results, that would be useful.
Comment 2 Fletcher 2005-11-12 10:31:41 PST
(In reply to comment #1)
> If anyone can test this and verify my results, that would be useful.

I'd be happy to.  I just checked build "version 1.6a1 (20051112)" on Mac OS X, and it still attempts only an unauthenticated bind.  I jumped the gun, I guess.  I'll give it another try after the next nightly build.

I assume that when you run it, it actually populates the address book with downloaded data?  Because when I removed the authentication requirement on my server the transaction looked good, but no contacts appeared in Thunderbird's address book.  I'm talking about my previous test, I don't have time to repeat that test until tomorrow.
Comment 3 Mark Banner (:standard8) 2005-11-12 11:15:06 PST
(In reply to comment #2)
> (In reply to comment #1)
> I'd be happy to.  I just checked build "version 1.6a1 (20051112)" on Mac OS X,
> and it still attempts only an unauthenticated bind.  I jumped the gun, I guess.
>  I'll give it another try after the next nightly build.

The patch hasn't been checked in yet, so it won't be in the nightlies. You can however build Thunderbird/SeaMonkey yourself and apply the patch to it.

> 
> I assume that when you run it, it actually populates the address book with
> downloaded data?  Because when I removed the authentication requirement on my
> server the transaction looked good, but no contacts appeared in Thunderbird's
> address book.  I'm talking about my previous test, I don't have time to repeat
> that test until tomorrow.
> 

When you download a database, the address book doesn't use it until you put the Thunderbird/SeaMonkey into offline mode. Then when you do quick searches on the ldap database you'll be able to access the data.
Comment 4 Karl Lattimer 2006-10-19 06:02:18 PDT
I've just tested this with nightly 3 alpha 1 (20061019)

Getting the following errors on ldap's side. 

connection_get(10)
connection_get(10): got connid=0
connection_read(10): checking for input on id=0
ber_get_next
ber_get_next: tag 0x30 len 12 contents:
ber_get_next
ber_get_next on fd 10 failed errno=11 (Resource temporarily unavailable)
do_bind
ber_scanf fmt ({imt) ber:
ber_scanf fmt (m}) ber:
>>> dnPrettyNormal: <>
<<< dnPrettyNormal: <>, <>
do_bind: version=3 dn="" method=128
send_ldap_result: conn=0 op=0 p=3
send_ldap_result: err=48 matched="" text="anonymous bind disallowed"
send_ldap_response: msgid=1 tag=97 err=48
ber_flush: 39 bytes to sd 10
do_bind: v3 anonymous bind
connection_get(10)
connection_get(10): got connid=0
connection_read(10): checking for input on id=0
ber_get_next
ber_get_next: tag 0x30 len 5 contents:
ber_get_next
ber_get_next on fd 10 failed errno=0 (Success)
connection_read(10): input error=-2 id=0, closing.
connection_closing: readying conn=0 sd=10 for close
connection_close: deferring conn=0 sd=10
do_unbind
connection_resched: attempting closing conn=0 sd=10
connection_close: conn=0 sd=10

Looks to me like its still trying anonymous bind almost a year on. Is this being actively worked on, has anyone tested the patch? Is there any chance of it going into the next release?

Comment 5 Mark Banner (:standard8) 2007-03-11 15:41:33 PDT
Created attachment 258237 [details] [diff] [review]
Second version WIP (don't use)

This patch is a WIP version. It reuses the nsAbLDAPListenerBase class I created a while ago to get the authentication correct.

Currently this patch won't work - it crashes in Abort() on the OnLDAPMessageBind as mQuery doesn't exist. Interestingly, when nsLDAPOperation::SearchExt is called there is no listener existing, presumably why its aborted - so its probably trying to abort twice.

I need to think about this a bit more so I'm posting it so I can view it when I'm not at my dev computer.
Comment 6 Mark Banner (:standard8) 2007-03-12 14:48:15 PDT
Created attachment 258347 [details] [diff] [review]
Remove change log code from build (checked in).

Currently the Change Log implementation of replication doesn't work (bug 311632), and we should have proper detection for it anyway. To fix this bug I'm restructuring the full replication classes so that I can reuse existing code. Rather than fix change log implementation as well, or do the minimum to make it compile, I'd prefer just to comment it out from the build for future implementation by bug 311632. So this patch does just that... (I'm not going to cvs remove anything).
Comment 7 Mark Banner (:standard8) 2007-03-13 13:40:56 PDT
Created attachment 258454 [details] [diff] [review]
Third version (diff -w)

This patch restructures the nsAbLDAPReplicationData to re-use the nsAbLDAPListenerBase class that I created before so we now use the same auth code for both replication and queries, hence resolving the bug.

I've also done some restructuring to make things easier so that the ReplicationData is passed everything it needs to know via the Init function.

The constructor for nsAbLDAPListenerBase can also be called with no parameters - hence the data can be set up later, and hence the need for some extra checks.

Note that the change log replication is disabled in cvs now - I'm not worrying about that here. Lets just get some authenticated replication working first.

Normal patch coming in a mo.
Comment 8 Mark Banner (:standard8) 2007-03-13 13:41:54 PDT
Created attachment 258455 [details] [diff] [review]
Third version (normal patch)
Comment 9 neil@parkwaycc.co.uk 2007-03-20 10:34:05 PDT
Comment on attachment 258454 [details] [diff] [review]
Third version (diff -w)

>-[scriptable, uuid(126202D1-4460-11d6-B7C2-00B0D06E5F27)]
>-interface nsIAbLDAPChangeLogQuery : nsISupports {
>+// XXX This interface currently isn't implemented as it didn't work.
>+// Bug 311632 should fix it
>+//[scriptable, uuid(126202D1-4460-11d6-B7C2-00B0D06E5F27)]
>+//interface nsIAbLDAPChangeLogQuery : nsISupports
I don't think //ing it out is going to help...

What could I do to demonstrate the effect of this patch, given that I don't even know how to set up an LDAP address book for my Microsoft Active Directory domain (which I think requires an authenticated connection)?
Comment 10 Mark Banner (:standard8) 2007-03-23 04:58:22 PDT
(In reply to comment #9)
> I don't think //ing it out is going to help...
Ok, I was probably a bit over the top there.

> What could I do to demonstrate the effect of this patch, given that I don't
> even know how to set up an LDAP address book for my Microsoft Active Directory
> domain (which I think requires an authenticated connection)?

Well we could either get you to set it one up, or it may be easier to pass review to someone else (David?)
Comment 11 Mark Banner (:standard8) 2007-03-26 03:03:24 PDT
*** Bug 375268 has been marked as a duplicate of this bug. ***
Comment 12 neil@parkwaycc.co.uk 2007-03-27 04:00:02 PDT
OK, so I've got an LDAP server that requires auth (local Microsoft AD server),
and when I try to synchronise with trunk it immediately fails.

With this patch it asks for my password, and then looks as if it's trying to synchronise, and counts 400 cards, but then it fails anyway...

The following exception in the error console may be relevant:
Error: [Exception... "'Failure' when calling method: [nsIAbLDAPAttributeMap::setCardPropertiesFromLDAPMessage]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "<unknown>"  data: no]
Comment 13 Mark Banner (:standard8) 2007-03-27 04:17:06 PDT
(In reply to comment #12)
> OK, so I've got an LDAP server that requires auth (local Microsoft AD server),
> and when I try to synchronise with trunk it immediately fails.
> With this patch it asks for my password, and then looks as if it's trying to
> synchronise, and counts 400 cards, but then it fails anyway...
> The following exception in the error console may be relevant:

Ah, that's an expection that happens every time you do an ldap query, with or without the patch (which I really ought to file a bug on and fix sometime).

Do you get the replication success message or an replication failed message?

If you get success, then the offline mode should still work even with that exception.

If you get replication failed, then its annoying as I decided not to hook up the error reporting to the UI in this version of the patch as it'd be too much to do in the one patch. However, you could try putting a printf here:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/mailnews/addrbook/src/nsAbLDAPReplicationData.cpp&rev=1.22&mark=428-430#418

to print the errorCode which should then tie up with http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/directory/xpcom/base/public/nsILDAPErrors.idl#60
Comment 14 neil@parkwaycc.co.uk 2007-03-27 14:18:03 PDT
Comment on attachment 258454 [details] [diff] [review]
Third version (diff -w)

r=me with comment #9 fixed.
Comment 15 Mark Banner (:standard8) 2007-03-27 14:45:32 PDT
Comment on attachment 258454 [details] [diff] [review]
Third version (diff -w)

I'll fix Neil's comment 9 when I checkin the patch (unless it needs an update before then).
Comment 16 David :Bienvenu 2007-03-27 15:06:49 PDT
Comment on attachment 258454 [details] [diff] [review]
Third version (diff -w)

Thx, Mark! Have you verified that we're not leaking ldap connections after this change? E.g., we haven't introduced any cycles into the ldap object ownership graph?
Comment 17 Mark Banner (:standard8) 2007-03-28 00:18:12 PDT
(In reply to comment #16)
> (From update of attachment 258454 [details] [diff] [review])
> Thx, Mark! Have you verified that we're not leaking ldap connections after this
> change? E.g., we haven't introduced any cycles into the ldap object ownership
> graph?

I'm fairly sure I haven't, as when I was it wasn't shutting down properly, but I'll check that we're not leaking objects anyway before checking this in.
Comment 18 David :Bienvenu 2007-03-28 07:36:57 PDT
I'd use netstat to make sure we're not leaking connections after using compose window autocomplete and address book search...

I think it's only SSL ldap connections that have trouble shutting down. Not sure if you were using SSL or not.
Comment 19 Mark Banner (:standard8) 2007-04-03 10:09:34 PDT
I'm convinced we're freeing connections the same as if not better than before, so we shounld't have a problem here.

I've checked this into trunk -> fixed.
Comment 20 Christian Emig 2007-06-15 00:26:33 PDT
Just having updated from TB 2.0.0.0 to 2.0.0.4 (build date 2007-06-04) I still cannot see the "LDAP download feature" work for me.

Clicking on "Offline/Download now" it says "Replication succeeded" but I cannot see any address. Not even when switching TB to offline-mode. A Wireshark dump on the authenticated LDAP-SSL-connection displays just about 10 exchanged packets which should be ways too little for the roughly 200 entries on the LDAP. 

Can anyone confirm?
Comment 21 Mark Banner (:standard8) 2007-06-15 00:34:25 PDT
(In reply to comment #20)
> Just having updated from TB 2.0.0.0 to 2.0.0.4 (build date 2007-06-04) I still
> cannot see the "LDAP download feature" work for me.
...
> Can anyone confirm?

This bug won't be fixed on any of the Thunderbird 2.x releases. The changes it requires (including to interfaces) are too significant for the TB 2.x security and stability releases. This will be included in Thunderbird 3.0.
Comment 22 Christian Emig 2007-06-15 00:36:25 PDT
I see. Thanks Mark for the quick reply.
Comment 23 Mark Banner (:standard8) 2007-08-27 13:54:59 PDT
*** Bug 393892 has been marked as a duplicate of this bug. ***
Comment 24 Mark Banner (:standard8) 2008-04-23 11:37:13 PDT
*** Bug 426337 has been marked as a duplicate of this bug. ***
Comment 25 Mark Banner (:standard8) 2008-05-28 06:47:34 PDT
*** Bug 436095 has been marked as a duplicate of this bug. ***
Comment 26 Dave Howe 2008-05-28 07:04:35 PDT
ok, thanks. when are we likely to see a version this is fixed in? I notice it is marked "will be in next release" over a year ago. 

Are there any workarounds I/we can use to get a usable address book in the meantime?
Comment 27 Mark Banner (:standard8) 2008-05-28 07:20:53 PDT
(In reply to comment #26)
> ok, thanks. when are we likely to see a version this is fixed in? I notice it
> is marked "will be in next release" over a year ago. 

We are currently looking at a release sometime this year/very early next year.

> Are there any workarounds I/we can use to get a usable address book in the
> meantime?

The only way currently, would be to pick up a nightly build of the latest release, or the first alpha version (http://www.mozillamessaging.com/en-US/news/stories/2008-05-12-01), note that with both of these options you should use them with care (and backing up files etc) as they are likely to be unstable than a final release version.
Comment 28 hansen 2009-03-03 16:13:23 PST
*** Bug 480931 has been marked as a duplicate of this bug. ***
Comment 29 rblon 2010-02-19 03:47:43 PST
did this fix make TB 3, as I still have the issue that I only see the addresses when I search (in the write screen or in the address book itself), but not in the normal address book view?
Comment 30 Mark Banner (:standard8) 2010-02-19 04:49:19 PST
(In reply to comment #29)
> did this fix make TB 3, as I still have the issue that I only see the addresses
> when I search (in the write screen or in the address book itself), but not in
> the normal address book view?

Yes, this was fixed long before TB 3, although I don't think this is your issue. If you mean you must enter a search before you see any cards, that's a known issue filed elsewhere.
Comment 31 Dave Howe 2010-02-19 06:53:32 PST
No, the issue I have is related to the fact that hitting the "download now" button on the offline tab gives "replication failed" when the target server requires authentication (in this case, windows server 2008 - we updated again *sigh*. I literally just (in the last minute or so) downloaded a fresh copy of the installer from the website and installed it, to verify the issue is still occurring, and now get a different error. going to do some ldap debugging and see if I can figure out what is up...
Comment 32 Jadranko Dragoje 2011-11-17 00:52:13 PST
(In reply to Dave Howe from comment #31)
> No, the issue I have is related to the fact that hitting the "download now"
> button on the offline tab gives "replication failed" when the target server
> requires authentication (in this case, windows server 2008 - we updated
> again *sigh*. I literally just (in the last minute or so) downloaded a fresh
> copy of the installer from the website and installed it, to verify the issue
> is still occurring, and now get a different error. going to do some ldap
> debugging and see if I can figure out what is up...

Did you find the solution for this problem?
Comment 33 Sorin Sbarnea 2012-03-14 10:22:26 PDT
Does the Download option ever work? It never worked for me in year, If this is true , and nobody can work on it, it should be removed from the UI.
Comment 34 Mark Banner (:standard8) 2012-03-16 01:43:26 PDT
(In reply to Sorin Sbarnea from comment #33)
> Does the Download option ever work? It never worked for me in year, If this
> is true , and nobody can work on it, it should be removed from the UI.

Yes it does work.

However, please do not comment on closed bugs - as with the previous comments here they've effectively been lost due to the bug no longer being active.

If you have an issue, please file a new bug. If you do so, I would suggest you get an LDAP log first from Thunderbird and check that over so you can try and see what's happening:

https://wiki.mozilla.org/MailNews:Logging

Note You need to log in before you can comment on or make changes to this bug.