proper support for "Log in with user name and password" some of the code for has already been written by rdayal for changelog, see bug #128087

How hard is it to adapt Rajiv's code?

Is this bug specific to LDAP directories, how should it be tested?

Reassign QA Contact to myself

> How hard is it to adapt Rajiv's code? I don't know yet. > Is this bug specific to LDAP directories Yes, LDAP directories. The UI for this is in the general tab. see http://www.mozilla.org/mailnews/specs/addressbook/#Directory The code has been checked in by dmose, but the UI element is hidden until we fix this bug. > how should it be tested? you can't test anything yet. Not until we add support for this, and then enable the UI.

I've been doing some thinking about this, and I think our design for this needs to change slightly. Specifically, right now the replication code uses an email address for binding. And this is what Seth has been talking about generalizing. This is the way 4.x worked, and is fairly user friendly. However, what's really being specified to the LDAP server under the hood is the bind DN. This is useful for folks who want to bind as the administrator, or sites that want to set up a special user for binding (neither of these users are likely to have an email address). Additionally, forcing the user or admin to specifiy the bind DN is very easy to code, and would be less work than generalizing Rajiv's code. I propose that in the prefs UI, instead of having a checkbox for "bind with username and password", we have a textbox for "user to bind as (email addr or DN)". If nothing's there, we bind anonymously. If an email address, we do (at least for replication until the code gets generalized and moved somewhere, probably to nsLDAPConnection) a search on the email addr to figure out the bind DN. Otherwise, we just bind with the DN specified. And we would only bring up a dialog in the case where anonymous is specified, but the server refuses an anonymous bind. Thoughts?

sounds good, just some general questions: 1) Do we know why 4.x just used email? Is making the round trip to determine a dn from the email address part of some rfc for auth? What do other clients do? 2) how would we tell a bind dn from a email address? (does a bind dn start with "dn=..." 3) will we be using prefs to store the email (or bind dn) and wallet to store the password? Where did 4.x store the email we used for auth? What I'm getting at: will we be able to make this work automatically for migrated 4.x users? 4) will the specified bind dn (or email) also be hooked into the auth code that rdayal wrote? let me re-assign to dmose, since I think he'll be the one to work on this.

> 1) Do we know why 4.x just used email? Is making the round trip > to determine s dn from the email address part of some rfc for auth? > What do other clients do? There is no RFC. Most clients support binding as a DN, and many support binding using some other identifier (in their UI). It would be really good to be more flexible than 4.x; perhaps: a) If the string looks like a DN, just use it without searching. b) Otherwise, insert the value into a configurable filter string and do a search. For example, the configuration could look like: (&(objectClass=person)(mail=%s)) // default? or (&(objectClass=person)(uid=%s)) // user id based search I think that is basically what dmose suggested, with the addition that the search filter should be configurable (ideally). > 2) how would we tell a bind dn from a email address? > (does a bind dn start with "dn=..." (no) But you can look for tag=value, at the start of a string to make a pretty good guess that it is a DN and not an email address or some other value.

this needs to be fixed for rtm so raising impact.

I agree with Mark, I think eventhough underneath we use the dn to bind (including when we specify email id / user id) most end users of Netscape / AddressBook may not be well versed with even LDAP basic terminologies and thus we should not change the UI to allow user to specify 'just' a DN, we should have the flexibility of using either user id, email id or DN. Even for the administrators or users who maynot have an email id, "uid" will suffice, but we can have the option of specifying DN so that a search operation to get the DN can be avoided if the user is aware of this. Also I think the auth code in replication can be reused and easily moved to a centralized place for everyone to use. >> 3) will we be using prefs to store the email (or bind dn) and wallet to store >> the password? Where did 4.x store the email we used for auth? What I'm >> getting at: will we be able to make this work automatically for migrated 4.x >> users? For 4.x the email id was stored in the prefs. We can use the email id for migrated profile however we will need to display the password dialog. Seems like 4.x stored munged/encrypted password too if the auth.savePassword pref was set, however we cannot use it since using it would then require the routines 4x used to decrypt the password. Further once we have this implemented we donot really need to display the user-password dialog everytime for replication. For replication we can directly use the saved userid/emailid/dn and password and bind using it. The user can change the saved data by going to the prefs UI.

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

OK, here's an extremely rough work-in-progress patch. This implements bind dn / password authentication for LDAP autocomplete. Next step: sort out the interfaces for generalizing parts of Rajiv's replication code into a userid/email fallback search.

Still a work in progress, but in much better shape now. One of the main things I need to sort out is that this seems to expose a serious focus problem.

Last patch had the wrong options to diff.

Now includes LDAP auth for the addressbook too. This is a still a work-in-progress, but we're not too far off now.

In this version of the patch, I made these changes: * Removed unused function & member var * Cleaned up error messages * Call nsIPrompt with the server URL, not with the search URL Remaining open issues here: * is calling the windowwatcher with nsnull as the parent window the right thing here? * put up UI screenshots, and ask to jglick review * isolate the two or three new text strings that have been added here and ask robinfc to review * see if there's an old 4.x pref we can re-use here to avoid having to deal with pref migration issues. otherwise file pref migration bug. open issues that will have new bugs filed on them: * Fix weird UI behavior (auto-select of an item immediately after auth in autocomplete). Discussed with hewitt; plan is to add an attribute to the autocomplete widget with the semantics "ignore loss of focus before all sessions have completed". * Figure out why wallet service isn't remembering passwords across connections. * Having multiple servers configured on the same machine will confuse wallet. Need to incorporate all auth information into the LDAP URL; requires C SDK and XPCOM SDK changes. * decide RTM strategy on integration with the userid / email address filter approach used by replication code.

* Fixed the code that calls windowwatcher so that the dialog in the addressbook is properly parented. * Switched the pref from ".auth.login" to ".auth.dn" for 4.x compatibility. * Switched the UI to say "Bind DN" rather than "Login", since that's currently all that is supported. We can (potentially) switch back in the future.

* Added back a couple of try / catch clauses to replace the many that were hoisted out of functions in pref-directory-add.js. * Fixed goofy logic in nsDirPrefs which was deleting the .auth.dn pref when .savePasswords wasn't set to true. * Fixed JS strict warning caused by earlier versions of this patch. * Changed accesskey to not be uppercase of other accesskey in the same dialog.

Wording looks OK for password entry dialog and LDAP server properties dialog.

sr=sspitzer looks good, let's also get srilatha to do a review. some comments / questions: 1) when I delete a LDAP addressbook / autocomplete server, will we clear the auth dn pref from prefs.js? 2) Is the way storing the auth dn something that will be friendly to non US- ASCII emails (thinking about IDN here)? 3) There's some duplicated code in nsAbLDAPDirectoryQuery.cpp and nsLDAPAutoCompleteSession.cpp. It looks like more of the "we need this duplicated code until we combine all the ldap connection code". if so, can you log a bug about it, or add note to the code and the existing bug to clean it up later? 4) I'd add a comment in MsgComposeCommands.js, making it clear that you're using the compose window as the parent for the auth prompter similar to your other comment: + // get the addressbook window, as it will be used to parent the auth + // prompter dialog + // 5) does the way we hold on to the auth prompter cause any problems? Does holding on to it mean we hold onto the dom window? Does the auth prompter and the window get properly released? (Any concerns with the cached compose window?) Should we set the authprompter to null after using it to avoid these problems? 6) do we need the getter for authprompter? (seems like the setter is only used) 7) I like the try / catch cleanup. having one try / catch instead wrapping a lot of little things with try / catch (given that they shouldn't fail, and we didn't do much when they did.)

> 2) Is the way storing the auth dn something that will be friendly to non US- > ASCII emails (thinking about IDN here)? are bind dn's always US-ASCII? It looks like we might allow the user to put anything in there (like an email address, like in 4.x). that's why I asked. let me know if I'm off base.

Within the LDAP protocol and within the libldap calls, DNs are UTF-8 encoded.

> 1) when I delete a LDAP addressbook / autocomplete server, will we clear the > auth dn pref from prefs.js? I _think_ all the prefs having to do with a single server get deleted en masse in DIR_ClearPrefBranch(). Srilatha, can you confirm this? > 2) Is the way storing the auth dn something that will be friendly to non US- > ASCII emails (thinking about IDN here)? Are bind dn's always US-ASCII? It > looks like we might allow the user to put anything in there (like an email > address, like in 4.x). that's why I asked. let me know if I'm off base. I'm storing both the URI and the auth.dn pref as WString ComplexValue prefs, which ultimately is encoded in UTF8 on disk, I believe. So we should be fine here. > 3) There's some duplicated code in nsAbLDAPDirectoryQuery.cpp and > nsLDAPAutoCompleteSession.cpp. It looks like more of the "we need this > duplicated code until we combine all the ldap connection code". if so, can > you log a bug about it, or add note to the code and the existing bug to clean > it up later? The cleanup is all about the nsLDAPService. Every consumer of nsILDAPConnection will be evaluated for that cleanup. There are already multiple bugs logged. > 4) I'd add a comment in MsgComposeCommands.js, making it clear that you're > using the compose window as the parent for the auth prompter > similar to your other comment: > > + // get the addressbook window, as it will be used to parent the auth > + // prompter dialog Done. > 5) does the way we hold on to the auth prompter cause any problems? Does > holding on to it mean we hold onto the dom window? Does the auth prompter and > the window get properly released? (Any concerns with the cached compose > window?) Should we set the authprompter to null after using it to avoid these > problems? Looks like there is gonna be a circular reference problem here. Just setting the authprompter to null after use is likely to cause other issues. I'll try and figure out a solution. > 6) do we need the getter for authprompter? (seems like the setter is only > used) I suppose not, but there's no way to specify a "write-only" attribute in IDL, so I'm inclined to leave it as is.

>> 1) when I delete a LDAP addressbook / autocomplete server, will we clear the >> auth dn pref from prefs.js? >I _think_ all the prefs having to do with a single server get deleted en masse >in DIR_ClearPrefBranch(). Srilatha, can you confirm this? Yes, that's right r=srilatha for the code in mailnews/addrbook/prefs thanks for cleaning up the try/catch

1) > I suppose not, but there's no way to specify a "write-only" > attribute in IDL, so I'm inclined to leave it as is. when I've hit this, I usually do: void setAuthPrompt(in nsIAuthPrompt authPrompter); 2) about the circular ownership: compose window -> ac session -> auth prompt -> compose window I agree, let's wait until we figure out if this is a problem, and if so, how to break it before checking in. any idea on when the UI freeze is? Could we land some of this patch (the UI and the LDAP AB stuff, which doesn't have the this problem) and land the rest later?

Changes: * eliminate unnecessary <hbox> that I added to pref-directory-add.xul * fix bug where creating new servers didn't always work by moving getService for nsLDAPService to the top-level of pref-directory-add.js

sspitzer wrote: >> I suppose not, but there's no way to specify a "write-only" >> attribute in IDL, so I'm inclined to leave it as is. > >when I've hit this, I usually do: > >void setAuthPrompt(in nsIAuthPrompt authPrompter); I bet you really meant to use "getAuthPrompt" in your example. But doing that doesn't even compile on Linux (which is correct behavior). If there platforms where that does compile, I would half-expect the dynamic-linker to barf on it at run-time. If I change the type to nsresult, it will compile, but still generates a compiler warning. Given that in addition to adding a compiler warning, this seems to be pretty clearly a violation of the XPCOM-type system, I'd prefer to leave it as is. I could change the body of the function to "return NS_ERROR_NOT_IMPLEMENTED" or equivalent, if you'd rather, though the existing code is so tiny I doubt this buys us much. On another note, it seems as though there may not be an circular reference problem here, I'm still working on verifying this in various difference situations using a debugger and the refcount-balancer.

Is the bind dn field a requirement? Can we have username instead where it will just fill in the uid for you?

1) >>void setAuthPrompt(in nsIAuthPrompt authPrompter); > >I bet you really meant to use "getAuthPrompt" in your example. But doing that >doesn't even compile on Linux (which is correct behavior). I really did mean setAuthPrompt(), as your code only ever uses the setter. that idl should turn into NS_IMETHOD nsLDAPAutoCompleteSession::SetAuthPrompter(nsIAuthPrompt *aAuthPrompter); but I'm fine leaving it the way you have it, even though the getter is unused. (But I would like to see why this doesn't work for you, but it works here: nsIMsgNewsFolder.idl: void setReadSetFromStr(in string setStr); nsIMsgPrintEngine.idl: void setWindow(in nsIDOMWindowInternal ptr); nsIMsgIncomingServer.idl: void setFilterList(in nsIMsgFilterList aFilterList); etc. 2) >On another note, it seems as though there may not be an circular reference >problem here, I'm still working on verifying this in various difference >situations using a debugger and the refcount-balancer. That seems like the biggest road block to landing this. once your sure the ownership model doesn't have any cycles (or if it does, they are broken properly), I'll do a final review of the last patch.

putterman: for the moment, bind dn is a requirement; see bug 146564 for a partial explanation of this, and ping me personally if you need more details. sspitzer: 1) I totally misinterpreted your suggestion; no wonder it didn't compile. I'm just gonna leave this as is for now, since you've said you're ok with that. 2) After stepping through both cached and uncached compose window scenarios in the debugger, I see no leakage, and I even see why. It turns out that much of the cleanup for the compose window happens as a result of a call to nsDocShell::Destroy, and this happens before the window's destructor is called. Among other things, this causes the compose window JSContext to be released, which causes the LDAP autocomplete session to be released, which calls its destructor, which releases the nsIAuthPrompt nsCOMPtr, which calls that destructor, and so at that point, there's no longer an owning ref to the window.

Comment on attachment 84997 [details] [diff] [review] patch, v8 r=sspitzer, looks good. and thanks for the figuring out the ownership model.

I have a couple nits below, but one big question re the use of wallet: I don't see any code to remove the password from wallet if the logon fails, and I know we've had problems with that in the past because the password manager category services weren't getting created. + rv = mServerURL->GetAsciiHost(host); + if (NS_FAILED(rv)) { + FinishAutoCompleteLookup(nsIAutoCompleteStatus::failureItems, rv, + UNBOUND); + return NS_ERROR_FAILURE; + } why not return rv here? and here: + nsCOMPtr<nsIAuthPrompt> authPrompter; + rv = windowWatcherSvc->GetNewAuthPrompter( + abDOMWindow, getter_AddRefs(authPrompter)); + if (NS_FAILED(rv)) { + NS_ERROR("nsAbQueryLDAPMessageListener::OnLDAPInit():" + " error getting auth prompter"); + return NS_ERROR_FAILURE;

Comment on attachment 85887 [details] [diff] [review] patch, v9 I've addressed both return value comments, and made it forget the password when appropriate. Changes are minor; the only files that have changed since the previous rev are nsLDAPAutoCompleteSession.cpp and nsAbLDAPDirectoryQuery.cpp. Carrying forward the has-review from the previous rev.

why do you suggest popping up a dialog in that comment? Doesn't the call you made remove the password from wallet by itself? Or does it just set it as empty? Anyway, I'll sr this so you can get it in, but I'm curious.

Comment on attachment 85887 [details] [diff] [review] patch, v9 sr=bienvenu

The comment suggests popping up the dialog only in the case where the attempt to remove the password has failed.

Patch v9 checked into the trunk.

yulian, can you verify this on the trunk?

changing to current nomination keyword. Dan,if this has been checked into the trunk, can you mark this as fixed?

Marking fixed, as this has been checked in on the trunk. Branch checkin still required.

Working very well on Linux bild 2002060308. But why there is not input box for "Bind DN password" on Directory Server Properties window?

yulian, can you verify this on the trunk with today's builds?

verified with 20020604 Trunk build on Win2K.

Petr: this works the same way the mail protocol configuration works: the password saving stuff is all handled by the wallet; it's not settable directly in preferences.

Mail sent to drivers requesting branch checkin approval.

adt1.0.1+ (on ADT's behalf) approval for checkin into the 1.0 branch. pls check this in asap, then add the "fixed1.0.1" keyword.

More verification, this time from a Win2K user showed up in my email this morning: > I've been running the "daily build" that includes the fixed LDAP authentication > (bug 135778) for a couple of days now. It not only works, but looks great! > > Thanks! I can finally actually use Mozilla for my mail on a full time basis now.

"please land this on the 1.0.1 branch. once there remove the "mozilla1.0.1+" keyword, and add the "fixed1.0.1"

Fix checked in on the 1.0 branch.

Verified with 20020611 Branch builds on various platforms.