Closed Bug 316170 Opened 19 years ago Closed 17 years ago

Address Book LDAP Replication doesn't work for authenticated connections.

Categories

(MailNews Core :: Address Book, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: standard8, Assigned: standard8)

References

(Blocks 1 open bug, )

Details

Attachments

(3 files, 2 obsolete files)

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.
Attached patch First version (obsolete) — Splinter Review
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.
(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.
(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.
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?

Attached patch Second version WIP (don't use) (obsolete) — Splinter Review
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.
Attachment #202796 - Attachment is obsolete: true
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).
Attachment #258347 - Flags: superreview?(bienvenu)
Attachment #258347 - Flags: review?(bienvenu)
Attachment #258347 - Flags: superreview?(bienvenu)
Attachment #258347 - Flags: superreview+
Attachment #258347 - Flags: review?(bienvenu)
Attachment #258347 - Flags: review+
Attachment #258347 - Attachment description: Remove change log code from build. → Remove change log code from build (checked in).
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.
Attachment #258237 - Attachment is obsolete: true
Attachment #258454 - Flags: review?(neil)
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)?
(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?)
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]
(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 on attachment 258454 [details] [diff] [review]
Third version (diff -w)

r=me with comment #9 fixed.
Attachment #258454 - Flags: review?(neil) → review+
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).
Attachment #258454 - Flags: superreview?(bienvenu)
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?
Attachment #258454 - Flags: superreview?(bienvenu) → superreview+
(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.
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.
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.
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
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?
(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.
I see. Thanks Mark for the quick reply.
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?
(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.
Product: Core → MailNews Core
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?
(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.
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...
(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?
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.
(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
You need to log in before you can comment on or make changes to this bug.