120432 - Need "use secure connection (SSL) checkbox in General Panel for LDAP addressbook / directory

Status: VERIFIED FIXED

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

Description

[scottputterman]

We need to add the Secure checkbox and the Logon checkbox to the Directory
Properties Dialog's General Panel.  The LDAP spec will eventually be updated to
cover this.

Comment 1

[Srilatha Moturi]

Attachment: screen shot with the secure and login checkboxes

Comment 2

[robinf]

I'm thinking that "Secure" might be misunderstood by some users to mean that
their data is secure. How about using "Secure Connection" instead ?

Wording suggestion for second check box: "Log in with user name and password".

Comment 3

[Dan Mosedale (:dmosedale, :dmose)]

In the IMAP and SMTP account panes, the preferred wording appears to be "Use
secure connection (SSL)" which I like.

Comment 4

[robinf]

Agree.

Comment 5

[jglick]

Is "Require user name and password" more accurate?

Comment 6

[timeless]

no, because you can have a password protected session that isn't SSL.

Comment 7

[jglick]

Attachment: General Tab

Comment 8

[Srilatha Moturi]

Attachment: patch v1

patch for secure and Logon Checkboxes

Comment 9

[(not reading, please use [e-mail] instead)]

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

Comment 10

[Srilatha Moturi]

Moving to 1.0

Comment 11

[Michael Buckland]

Discussed in Mail News bug mtg with Engineering QA Mktng PjM.  Decided to ADT2
this bug. 

Comment 12

[Srilatha Moturi]

Attachment: updated against the trunk

Comment 13

[(not reading, please use [e-mail] instead)]

looks good.  some nits and concerns:
1)

 var gPortNumber = 389;
+var gSecPortNumber = 636;

these should be constants.

const kDefaultLDAPPort = 389;
const kDefaultSecureLDAPPort = 636;

2)
 
+function onSecure()
+{
+  var port = document.getElementById("port");
+  if (document.getElementById("secure").checked) {
+    if(port.value == gPortNumber)
+      port.value = gSecPortNumber;
+  }
+  else if (port.value == gSecPortNumber)
+    port.value = gPortNumber;
+}

you've made it so we only switch to the default port if the user was using the 
default port.
Note, that's now how we do it for servers in the account manager.  I think we 
should
be consistent.

3)

+  pref_string_content = login.checked;
+  gPrefInt.setBoolPref(pref_string_title, pref_string_content);

As we've discussed in other bugs, reading these LDAP prefs directly is ok.  But 
what
are the side effects of setting them directly, instead of going through the 
DIR_Server code.
I'm concerned that any place in this code where you call a pref setter, it 
would take affect immediately.
can you investigate and see if these setters only "take affect" after restart?

If this is an issue, I think we should spin it off to another bug, as there is 
other setters in this same file
that we need to address at the same time.

4)

+             <checkbox id="secure" label="&directorySecure.label;"
+                       accesskey="&directorySecure.accesskey;"
+                       oncommand="onSecure();"/>

until the LDAP over SSL patch lands, we should add disable="true"
I'm confident LDAP over SSL will land, and when it does, we'll remove that.
But at least this gets the UI in.

5)


+      if (secure) {
+        ldapUrl.options |= ldapurl.OPT_SECURE;
+        port = 636;
+      }
+      else
         port = 389;

please define constants, see #1.


6)

+      try {
+        port = gPrefInt.getIntPref(pref_string + ".port");
+      }

why is this named gPrefInt?  That's a wacky name for the pref branch service.  
I'm not saying you should
fix it, I'm just wondering why it is named that.

Comment 14

[(not reading, please use [e-mail] instead)]

this blocks the LDAP over SSL feature.

I think adding the auth part of the UI will affect rdayal's LDAP replication 
work as well.

Comment 15

[(not reading, please use [e-mail] instead)]

note to jglick:  there might be a potential UI issue in here.

when we toggle secure / non-secure, we toggle the port.

unlike the account manager UI, the port UI is on a different tab, so the user
might not see it change.

perhaps it should be on the same tab as the secure / non-secure checkbox?

Comment 16

[Srilatha Moturi]

> you've made it so we only switch to the default port if the user was using the 
> default port.
4.x behaves that way and I was trying to make it consistent with 4.x. 

> can you investigate and see if these setters only "take affect" after restart?
> If this is an issue, I think we should spin it off to another bug, as there is 
> other setters in this same file that we need to address at the same time.

The behaviour is similar to changing the hostname (as described in bug 124553).

I will disable both the checkboxes until we have the backend implementation.
And the name gPrefInt... Initially when this code was written, we were using
nsIPref interface and later it was changed to nsIPrefBranch /Service

> perhaps it should be on the same tab as the secure / non-secure checkbox?
I agree with Seth on this.

Comment 17

[jglick]

>perhaps it should be on the same tab as the secure / non-secure checkbox?

Is it important that the user actually see the port number change?
If so, the dialog needs to be made larger so Port can fit below Base DN. 

Comment 18

[Srilatha Moturi]

Attachment: screen shot with port number in the general tab

> Is it important that the user actually see the port number change?
It is important that the user should know that the port # changed in the
following scenario:
If the user sets the port# to some value other than the default
Then changes the selection on the Secure checkbox which will change the port#.

Comment 19

[jglick]

>screen shot with port number in the general tab
Srilatha, is it possible to make the dialog wider horizontally? The text fields 
should be long enough so that typical data entered in the text fields can be 
viewed with having to scroll. It also looks like the dialog should be longer 
vertically as well. The "Base DN" field looks like it supposed to be 2 lines but 
only 1.5 lines would be visible. It should either be sized to display 1 full 
line or 2 full lines. Thanks.

Comment 20

[jglick]

correction: ...withOUT having to scroll.

Comment 21

[Srilatha Moturi]

Attachment: screen shot with increased width

k, how does this look. Base DN field is only one line.

Comment 22

[(not reading, please use [e-mail] instead)]

a side note, the secure checkbox is well understood, and once we've got a fix 
for 107411 we'll need this UI.

the checkbox for using username / password is less understood, and can't be 
shown until we figure these out:

When do we popup the prompt?  do we do the same things as is 4.x?  ask for 
email and password to bind?  do we need to take that email and anonymously 
determine the bind dn from it?  all unknowns right now, we need to look into 
the 4.x code.

rajiv has added some code for auth for replication, see #128087.

we might need to combine / move that code around to be shared.

Comment 23

[jglick]

>k, how does this look. Base DN field is only one line.

Thanks for widening, looks good. Base DN still looks taller vertically than the 
other text fields though.

Comment 24

[jglick]

Attachment: password dialog

>When do we popup the prompt?  

When user attempts a search on an ldap directory.

Comment 25

[Srilatha Moturi]

Attachment: updated patch, using constants for port#s

Updated the patch based on Seth's comments. Also move port to the general tab.
and increased the width of the dialog.

Comment 26

[Dan Mosedale (:dmosedale, :dmose)]

Patch checked in on both branch and trunk with the fix for 107411.  Note that
the "login" check box is currently hidden, since there's not yet code in the
backend to support it.

Comment 27

[(not reading, please use [e-mail] instead)]

the login UI will be enabled as part of #135778.

Updating summary to reflect that this was only for 

Comment 28

[yulian chang]

Verified with 20020410 trunk builds on various platforms.