32018 - wrong UI for SSL SMTP in account manager

Status: VERIFIED FIXED

(SeaMonkey :: MailNews: Account Configuration, enhancement, P3)

Description

[John G. Myers]

In the account manager, there is a single checkbox "Use secure connection 
(SSL)".  Since SMTP uses STARTTLS to implement SSL, this should instead be three 
radio buttons, one each for "Never," "If possible," and "Always."

The current checkbox is ineffective anyway.  It should be removed from the UI 
until it is implemented correctly.

Comment 1

[John Unruh]

Marking fixed. The UI is there now. There is a checkbox for Use secure 
connection (SSL), and two optional boxes, Always and When available. Opening a 
new bug 46393 since the prefs are not actually saved to disk.

Comment 2

[John G. Myers]

The UI is really stupid.  It is representing a three way choice with a checkbox 
and two radio buttons.  A correct UI would use three radio buttons.

Comment 3

[John Unruh]

Good point. Clicking on "Use Secure Connection" leaves "Always" and "When 
Available" both unchecked, even though one of them should now be checked. The 
options should be as jgmeyers originally stated - "this should instead be three 
radio buttons, one each for "Never," "If possible," and "Always."". 
Reassigning component to preferences.

Comment 4

[sairuh (rarely reading bugmail)]

should this belong in mailnews?

Comment 5

[lchiang]

cc: jglick (UI design) and pavlov (who is implementing this feature as per a
separate bug)

Comment 6

[jglick]

It was supposed to be a check box "Use secure connection (SSL)" with two radio 
buttons below it, "Always" and "If possible".  If the check box is unchecked, 
radio buttons should be disabled.  If check box is checked, one of the radio 
buttons should always be selected.  This way, if the feature is enabled, the 
user picks one of two choices. If the feature is not enabled, no decision needs 
to be made.

http://gooey/client/5.0/specs/mail/accountSetup/images/AcctServerSmtp1.gif

If folks have a problem with that, 3 radio buttons is fine also.

SLL currently doesn't appear in SMTP account settings (8/25/00 build)so i'm not 
sure if this is still an issue.

Comment 7

[John G. Myers]

A checkbox and two radio buttons is a complicated, clunky way to represent a 
choice between three options.  A three way radio button is simpler and clearer.

Comment 8

[lchiang]

We had a bug, I think, to remove SMTP SSL from Account Settings (when we thought
that the feature would not be in).  We may need to revive that setting.  Nbaca,
can you check?

Comment 9

[Alec Flett]

massive reassign of account manager bugs -> sspitzer
please feel free to put me back on the CC if you have any questions/comments

Comment 10

[Jarrod Gray]

We need the ability to modify the SMTP SSL preferences. I think this is needed 
for rtm because without the interface the product is unusable for a large group 
of people. This should be a simple fix. Whatever was taken out in bug 46393 just 
needs to be put back in.

Comment 11

[Jarrod Gray]

This bonsai query will show the backout for bug 46393 which should be the bug 
that removed the UI.

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&bra
nchtype=match&dir=&file=&filetype=match&who=alecf%25netscape.com&whotype=match&s
ortby=Who&hours=2&date=explicit&mindate=07%2F31%2F2000+13%3A41&maxdate=07%2F31%2
F2000+13%3A43&cvsroot=%2Fcvsroot

Comment 12

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

accepting.

someone named "Magick" was talking to me about how to do this in #mozilla.

I'll try to find out who that was, and cc him.

Comment 13

[Jarrod Gray]

Magick == [e-mail]

Maybe someone can englighten me on whether this should be a group of 3 radio 
buttons or a checkbox and 2 radios. I personally like the 3 radios - it seems to 
be more straightforward.

In nsSmtpProtocol.h it looks like the try_ssl preference is setup to be used as 
as 3 radios:

97 typedef enum _PrefTrySSL {
98     PREF_NO_SSL = 0,
99     PREF_TRY_SSL = 1,
100     PREF_ALWAYS_SSL = 2
101 } PrefTrySSL;

Any thoughts?

Comment 14

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

I'd say do what 4.x did:

have three radio buttons.

I'll attach a screen shot.

Comment 15

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

Attachment: screen shot of 4.x ui

Comment 16

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

note, the old ui was in the prefs dialog.

the new ui would not live there.  it was just to give you and idea of the
wording and the radio buttons.

my rule is:  when in doubt, follow existing UI designs.

Comment 17

[jglick]

Radio buttons are fine.

"Use Secure Socket Layer (SSL) for outgoing messages:
 O-Never   O-If available   O-Always"

Do we need the part about TLS? If so, we should explain what TLS is.

Comment 18

[Jarrod Gray]

Attachment: initial patch

Comment 19

[Jarrod Gray]

The attatched patch still has some glitch that does not allow the preferences to
be displayed or saved in the first smtp window - it works fine in the advanced
settings when a server is actually selected. There must be something simple I'm
overlooking. Any comments? Thanks.

Comment 20

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

I've fixed the problem that magick mentioned.

I've also made some adjustments.  magick, can you review it and try it out?

I'll attach a patch.

if it looks good and works good for you, we'll get alecf to review it.

Comment 21

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

Attachment: new version of the fix.

Comment 22

[Jarrod Gray]

Works for me and looks good. Only comment I would have is to remove the dumps.

Comment 23

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

Attachment: new version of the fix, please review again.

Comment 24

[Jarrod Gray]

newest patch works for me.
thanks sspitzer

Comment 25

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

before we check this in, I need to talk to jefft.

we have some ui that changes auth method (none, any, login, tls try, tls only)
but it is only a checkbox that switches between none and any.

I need to understand more about trySSL vs auth method before we clean up the ui.

I want to get it all right in one swoop.

Comment 26

[John G. Myers]

Authentication should be a separate config option than SSL.  For authentication 
policy, one wants the user to specify separately for each possible class of 
authentication mechanism what set of circumstances permit its use.  For example, 
one may want to only send plaintext passwords if the encryption strength is at 
least X bits, yet permit challenge-response mechanisms such as CRAM-MD5 over 
unencrypted or weakly encrypted connections.

There is really no reason not to authenticate with EXTERNAL if the server offers 
it.  The current "auth/noauth" option probably intends to just disable 
mechanisms which require user input.  I believe this is bad client 
design--in situations where a server doesn't require authentication, it is the 
server's responsibility to not offer any authentication mechanisms.

Comment 27

[jefft]

Well said, John. This is the reason I try to group all authentication method
within a single enum and created a new enum of "any" compares to the 4.7 prefs.
We need to have a little bit thinking to do here. As I remember when I
implemented SSL SMTP for 4.7, once we establish an SSL SMTP we still need to go
throung another round of EHLO to figure out what authentication mechanism
spported under SSL SMTP and then do an EXTERNAL authentication again for
Netscape Messenger 4.0 server.

Comment 28

[Ninoschka Baca]

Adding keyword Mail3. Shouldn't this be enabled and working?

Comment 29

[cls]

I'm a bit confused as to what the patch (attch 19004) was supposed to
accomplish.  Or maybe my expectations are off.  It provides the user a chance to
change smtp auth preferences comparable to 4.x but it doesn't appear to work.  I
created a new outgoing mail server, mail.seawood.org, and clicked on 'Use secure
connection (SSL) - Always'.  But when I sent an email using that server, the
mail appeared to be delivered fine without any additional dialogs.   Since
mail.seawood.org is an alias and the server has a different name on the cert, I
would have expected to have been prompted about the name discrepancy like I am
for imap connections.  I checked the preferences and to my dismay, I discovered
that auth_method was not set how I would have expected it to be.  The
preferences look like:

user_pref("mail.smtpserver.smtp3.auth_method", 0);
user_pref("mail.smtpserver.smtp3.hostname", "mail.seawood.org");
user_pref("mail.smtpserver.smtp3.try_ssl", 2);
user_pref("mail.smtpserver.smtp3.username", "");

I would have expected auth_method to be set to 4, PREF_AUTH_TLS_ONLY. 

After some more digging, lxr shows that PREF_ALWAYS_SSL is only in
nsSmtpProtocol.h .   Does this mean that there's no actual SSL SMTP support in
mailnews?

Comment 30

[scottputterman]

marking nsbeta1+ and moving to mozilla0.8

Comment 31

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

that patch is bogus.

I need to understand what the nsSMTPProtocol is doing before I can fix the ui.

at some point, SMTP over SSL worked.

Comment 32

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

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

Comment 33

[scottputterman]

reassigning to naving

Comment 34

[scottputterman]

moving to mozilla0.9

Comment 35

[scottputterman]

marking nsbeta1-

Comment 36

[John G. Myers]

Attachment: Proposed fix

Comment 37

[John G. Myers]

I coded a fix.  Reassigning to self.

Comment 38

[Scott MacGregor]

John, can you change the following in smtpEditOverlay.xul to:
+       <box orient="horizontal" style="margin-left: 2em">
to
<hbox style="....">
...
</hbox> 

I really like the smtp protocol changes. It simplifies a lot of crap that was in
there. However I did not test your patch to make sure it works. Assuming it runs
properly on your end, you are good to go.

sr=mscott

Comment 39

[John G. Myers]

That's r=mscott as well?

Comment 40

[John G. Myers]

Fix checked in.  This fix exposes bug 67507 in pipnss.  Fortunately, pipnss is 
not part of the build.

Comment 41

[John G. Myers]

If you set the preference to "Never", the next time you visit the preference 
panel it resets to "If possible".

Comment 42

[John G. Myers]

Attachment: proposed fix

Comment 43

[Mike Shaver (:shaver emeritus)]

Ah, the subtleties of type conversion.

sr=shaver

Comment 44

[Brendan Eich [:brendan]]

Not so fast, shaver!  trySSL is declared in nsISmtpServer.idl
(mailnews/compose/public) as an attribute of type long. Why should any JS ever
compare its value to ""?

/be

Comment 45

[John G. Myers]

If I remove my mail.smtpserver.smtp1.try_ssl preference from prefs.js, something 
else appears to (correctly) set the value of server.trySSL to 1.

So if there's no possibility it could be "" (and thus have the 
getElementsByAttribute() call pick a label instead of a radio button) the test 
could be removed.

Comment 46

[Brendan Eich [:brendan]]

If server is always bound to an object implementing nsISmtpServer, I don't see
how the type can be non-numeric in JS.  What is that condition trying to test,
really?  If it's looking for a non-zero value, it can just test (server.trySSL)
and not even worry about operators and conversion rules.

/be

Comment 47

[John G. Myers]

The condition is trying to test for a missing/unspecified pref.  The default 
value of the pref is supposed to be "1", not "0".

Comment 48

[John G. Myers]

Attachment: Cleaner prefs fix

Comment 49

[John G. Myers]

Argh, creating a new server from the advanced panel brings it up without any of 
the radio buttons selected.

Comment 50

[John G. Myers]

Attachment: Updated prefs fix

Comment 51

[Brendan Eich [:brendan]]

+        var elements = gSmtpTrySSL.getElementsByAttribute("data", server.trySSL);
         if (elements.length == 0)
             elements = gSmtpTrySSL.getElementsByAttribute("data", "1");
         gSmtpTrySSL.selectedItem = elements[0];
+    } else {
+        gSmtpTrySSL.selectedItem =
+            gSmtpTrySSL.getElementsByAttribute("data", "1")[0];
     }

could be simplified to

+        var elements = gSmtpTrySSL.getElementsByAttribute("data", server.trySSL);
         if (elements.length == 0)
             elements = gSmtpTrySSL.getElementsByAttribute("data", "1");
+    } else {
+        elements = gSmtpTrySSL.getElementsByAttribute("data", "1");
     }
     gSmtpTrySSL.selectedItem = elements[0];
 
or (to move var elements above the if/else):

+    var elements;
+    if (server) {
  . . .
+        elements = gSmtpTrySSL.getElementsByAttribute("data", server.trySSL);
         if (elements.length == 0)
             elements = null;
+    }
+    if (!elements)
+        elements = gSmtpTrySSL.getElementsByAttribute("data", "1");
     gSmtpTrySSL.selectedItem = elements[0];

This last consolidates the default getElementsByAttribute call.

The 0.8 branch has been cut.  Drivers needs r= and sr=, and some more words
about how likely the bug this patch fixes is to bite, and how hard it bites (no
workarounds)?

/be

Comment 52

[John G. Myers]

Neither proposed replacement appears simpler, the logic in both proposals is 
convoluted.  But I'll check in whatever version gets an r=.

The bug will affect anyone who sets the SMTP SSL option to "Never".  The 
workaround is for the user to set the radio button back to "Never" each and 
every time they bring up the SMTP Server pane in the account manager.

Also, users configuring a SMTP server for the first time may be confused by the 
lack of any selected radio buttons.

Comment 53

[Brendan Eich [:brendan]]

Please forgive my nit-picking, it's not an obstacle to taking the patch.  It's
ideally a joint questing after the best quality code.

The *logic* in my first proposal is identical to the patch's logic; the only
change is to data flow, to unify assignment to gSmtpTrySSL.selectedItem.

I agree my second proposal has obviously (and slightly) more involved logic.  I
withdraw it!

Appearances are not what I'm concerned about, so much as code space and runtime
efficiency, but if you really think the patch is clearer, it's ok with me -- it
is a small difference.  What got me started, and what still seems convoluted as
well as inefficient to me, is the gSmtpTrySSL.getElementsByAttribute("data",
"1") expression's asymmetric repetition across the then and else in the patch.

/be

Comment 54

[Brendan Eich [:brendan]]

Just conferred on IRC, drivers say "get into mah belly", er, "branch".
(MOZILLA_0_8_BRANCH)

/be

Comment 55

[John G. Myers]

The brendan first proposal is odd in using "var" only in the then clause, 
omitting it in the else clause.  In any case, I expect the else clause to have 
more code in a subsequent patch, so it's fine as it is.

Comment 56

[Brendan Eich [:brendan]]

Local variables in JS have function activation scope, so one var will do (while
two will get you a strict warning, to dispell block-scope confusion).  I chose
first opportunity to declare in source order, but perhaps hoisting the
uninitialized var declaration above the if/else is better.

Gilding the lily here; FYI only.  We like either version for 0.8.

/be

Comment 57

[Henrik Gemal]

There's also missing a ":" after "Use secure connection (SSL)"

It should read:
"Use secure connection (SSL):"

and in all other prefs we have the radiobutton under each other never besides.
So I think the dialog should be:
Use secure connection (SSL):
 ( ) Never
 ( ) When available
 ( ) Always

Comment 58

[Asa Dotzler [:asa]]

John, can we get this checked into the Mozilla 0.8 branch please.

Comment 59

[John G. Myers]

I do not have an r=, I do not know who to get an r= from, and mscott hasn't 
replied to my mail asking who should review my patch. 

Comment 60

[Brendan Eich [:brendan]]

Cc'ing sspitzer for an sr= on top of my r=.

/be

Comment 61

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

jgmyers, thanks for tackling this.

can you attach a full patch?  Too many seperate patches for me to review and test.

as soon as you got a full patch, I can test and review.

Comment 62

[John G. Myers]

sspitzer: the full patch is the 02/08/01 17:11 one.

Comment 63

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

sr=sspitzer

Comment 64

[John G. Myers]

Fix checked in to both 0.8 branch and tip.

Comment 65

[Ninoschka Baca]

Build 2001-02-19-08: NT4, Linux RH 6.2, Mac 9.04

1. A ":" is still needed after (SSL) so it states "Use Secure Connection (SSL):"
2. The spec states that the second radio button should state "If possible" but 
it now states "When available". Is this ok?
3. Henrik suggested having vertical radio buttons, the spec shows horizontal 
radio buttons. Currently they are horizontal, is this ok?

Comment 66

[John G. Myers]

1) I'll add the colon as part of another bug.

3) Horizontal radio buttons also exist on the prefs panel under Appearance/Fonts 
and "Mail and Newsgroups".  Their use here is not inconsistent.

As for (2) I'm not sure which wording is better.

Comment 67

[Ninoschka Baca]

Since noone appears to have strong feelings on the wording for the second radio 
button, keeping "When available" is fine with me. 

John, what bug number will be used to track adding the ":"?

Comment 68

[John G. Myers]

I'll probably include the : fix in a followon patch to bug 64777

Comment 69

[Ninoschka Baca]

Build 2001-04-02-04: NT4
Build 2001-04-02-08: Linux RH 6.2, Mac 9.04
Verified Fixed.