Closed Bug 135778 Opened 22 years ago Closed 22 years ago

support for "auth / Log in with user name and password" in LDAP addressbook / LDAP autocomplete

Categories

(SeaMonkey :: MailNews: Address Book & Contacts, defect, P1)

defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla1.0.1

People

(Reporter: sspitzer, Assigned: dmosedale)

References

()

Details

(Whiteboard: [adt1 rtm],custrtm-)

Attachments

(3 files, 10 obsolete files)

2.94 KB, image/png
Details
6.35 KB, image/png
Details
63.90 KB, patch
dmosedale
: review+
Bienvenu
: superreview+
Details | Diff | Splinter Review
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
QA Contact: nbaca → yulian
> 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.
Summary: proper support for "Log in with user name and password" → support for "Log in with user name and password" in LDAP addressbook / LDAP autocomplete
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.
Assignee: sspitzer → dmose
Summary: support for "Log in with user name and password" in LDAP addressbook / LDAP autocomplete → support for "auth / Log in with user name and password" in LDAP addressbook / LDAP autocomplete
Status: NEW → ASSIGNED
Keywords: nsbeta1
OS: Windows 2000 → All
Hardware: PC → All
Keywords: nsbeta1nsbeta1+
Priority: -- → P2
Whiteboard: [adt2]
> 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.
Whiteboard: [adt2] → [adt1 rtm]
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. ***
Attached patch proof-of-concept patch, v1 (obsolete) — Splinter Review
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.
Blocks: 143047
Attached patch cleaned-up patch, v2 (obsolete) — Splinter Review
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.
Attachment #82793 - Attachment is obsolete: true
Attached patch cleaned up patch, v3 (obsolete) — Splinter Review
Last patch had the wrong options to diff.
Attachment #83433 - Attachment is obsolete: true
Attached patch LDAP auth patch, v4 (obsolete) — Splinter Review
Now includes LDAP auth for the addressbook too.  This is a still a
work-in-progress, but we're not too far off now.
Attachment #83437 - Attachment is obsolete: true
Attached patch patch, v5 (obsolete) — Splinter Review
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.
Attachment #83664 - Attachment is obsolete: true
Attached patch patch v6 (obsolete) — Splinter Review
* 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.
Attachment #83971 - Attachment is obsolete: true
Attached patch patch, v7 (obsolete) — Splinter Review
* 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.
Attachment #84483 - Attachment is obsolete: true
Attached image password dialog image (obsolete) —
Attachment #84671 - Attachment is obsolete: true
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?
Blocks: 146564
Blocks: 146566
Blocks: 146568
Blocks: 146569
Blocks: 146575
Whiteboard: [adt1 rtm] → [adt1 rtm],custrtm-
Target Milestone: --- → mozilla1.0.1
Priority: P2 → P1
Attached patch patch, v8 (obsolete) — Splinter Review
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
Attachment #84565 - Attachment is obsolete: true
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.
Attachment #84997 - Flags: review+
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;
Attached patch patch, v9Splinter Review
Attachment #84566 - Attachment is obsolete: true
Attachment #84997 - Attachment is obsolete: true
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.
Attachment #85887 - Flags: review+
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
Attachment #85887 - Flags: superreview+
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?
Keywords: adt1.0.0
changing to current nomination keyword.

Dan,if this has been checked into the trunk, can you mark this as fixed?
Keywords: adt1.0.0adt1.0.1
Marking fixed, as this has been checked in on the trunk.  Branch checkin still
required.
Status: ASSIGNED → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Keywords: mozilla1.0.1
Blocks: 148891
No longer blocks: 146564, 146566, 146568, 146569, 146575
Blocks: 146575
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.
Keywords: adt1.0.1adt1.0.1+
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"
Attachment #85887 - Flags: approval+
Fix checked in on the 1.0 branch.
Verified with 20020611 Branch builds on various platforms.
Status: RESOLVED → VERIFIED
Product: Browser → Seamonkey
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: