525238 - Authentication method (plaintext/encrypted password, Kerberos/GSSAPI etc.) should be explicit, not just "Secure authentication"

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Ben Bucksch (:BenB)]

Go to Account Settings | Server Settings for IMAP or POP. There's a
[x] Secure authentication
checkbox.

The wording is extremely vague, so vague that not even techies know what it means and does.

What it currently does is (please correct me, if it's wrong):
If unchecked, we do plaintext password authentication, i.e. PLAIN or LOGIN .
If checked, we require hashed password authentication, i.e. CRAM-MD5 (or DIGEST-MD5, but not sure if supported),
   *OR* GSSAPI (Kerberos) or NTML (Windows single signon)



That's a problem, because
1) encrypted password and Kerberos are very different authorization mechanisms. (In Kerberos, the user never enters a password in applications, but on the OS level, and the apps contact an authorization server.) It, inherently, makes no sense from a user or application point of view to lump both together.
2) we don't know whether we need to ask for a password
3) we need to know whether the user wants to use Kerberos in order to give proper error messages when the user didn't authenticate to the Kerberos server yet. In other words, this bug blocks bug 524698.
4) we may, in some cases, need to uselessly try Kerberos, in case the server claims to support GSSAPI, but doesn't, and Kerberos is not properly set up on the client - just enough to attempt a connection to a Kerberos server, but not properly to actually authenticate. This largely depends on the OS and its default configuration. In these cases, the login time for "encrypted password" users would be considerably and needlessly longer.

Expected result:
- In Account Settings | Server Settings for IMAP or POP. There's a dropdown:
Authentication: [Password, transmitted insecurely]
                [Encrypted password              ]
                [Single signon (Kerberos, NTLM)  ]

- The Account Creation wizard already tries Kerberos/NTML first, and if that fails tries encrypted passwords, and if that fails tries cleartext passwords, and configures (forces) in prefs the best method that worked.

Comment 1

[Ben Bucksch (:BenB)]

Bryan, yay/nay?
I need this primarily for 3).

Comment 2

[Ben Bucksch (:BenB)]

(But that "secure auth" this never made sense to me anyways.)

Comment 3

[Nikolay Shopik]

Looks like dupe of bug 495412

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

This seems fine, however I just want to make sure that the default mode doesn't look scary to people when they are using a secure connection. i.e. if SSL is enabled then the Authentication method should "normal/secure" to the user.

If there is no SSL connection then we can be honest about the password being very insecure.  This is an effort to make sure that we don't get some of the user effect that is currently happening where people with SSL connection are checking the [x] Secure authentication box because it seems like a good idea.  In fact this often breaks the users ability to connect to the server.

For example:

= If SSL connection =

Authentication: [Password (default)              ]
                [Encrypted password              ]
                [Single signon (Kerberos, NTLM)  ]

= If No Connection Security =

Authentication: [Password, transmitted insecurely]
                [Encrypted password              ]
                [Single signon (Kerberos, NTLM)  ]

I'm not entirely sure about the "Password (default)" text but I want to give that warm fuzzy feeling to people who don't need to shoot themselves in the secure connection foot.  Any other suggestions for the 'secure plaintext password' string?

Comment 6

[Ben Bucksch (:BenB)]

Yes, I'll change the label for cleartext password based on whether SSL is checked, that makes sense.
How about "Normal Password" as label for cleartext password with SSL?

Comment 7

[Bryan Clark (DevTools PM) [:clarkbw]]

(In reply to comment #6)
> How about "Normal Password" as label for cleartext password with SSL?

Ok, do you think I could still get a (default) for the description value? :)

Comment 8

[Ben Bucksch (:BenB)]

No, that costs extra. ;-P
And, FWIW, it's actually not true. We default to secure auth. See bug 525083 comment 6 ff for why.

Comment 9

[Ben Bucksch (:BenB)]

I now have:
  Password, transmitted insecurely (in case of SSL: Normal password)
  Encrypted password
  Kerberos / GSSAPI (Single sign-on)
  NTLM (Windows single sign-on)
  Any secure method (deprecated)

Comment 10

[Ben Bucksch (:BenB)]

Attachment: WIP v3

Current state of work.

Contains
* IMAP (tested and working well so far)
* POP (not yet tested)
* Account Settings UI
* Account Wizard - both guess config and XML

It's far more work than I expected. The IMAP TryToLogon() logic was extremely hard to decipher, I had to straighten it considerably and documented it where it's still non-obvious. It's working much better than before, though, all the strange behaviour where it suddenly stopped trying or asked for a password when non is needed, are gone. It's behaves much more logical and predictable now, even from a user perspective.

It will still ask for a password when the server offers CRAM and GSSAPI and the old "secure auth" is in prefs and GSSAPI fails, but that's why there is the new option "Kerberos" in the prefs. Otherwise, it just either uses GSSAPI or password, not both.

It will also give a clearer error msg when the selected auth is not supported, solving bug 339050.

TODO:
- Test POP
- SMTP: Implement protocol and UI, and test

Comment 11

[Ben Bucksch (:BenB)]

I now have:
  Password, transmitted insecurely [in case of SSL: "Normal password"]
  Encrypted password
  Kerberos / GSSAPI
  NTLM
  Any secure method (deprecated) [appears only after migration]

Comment 12

[Ben Bucksch (:BenB)]

Attachment: Screenshot: Prefs, non-SSL

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 414156 [details]
Screenshot: Prefs, non-SSL

looks good.  Thanks for the screenshot and answering all my questions!

Comment 14

[Ben Bucksch (:BenB)]

Attachment: WIP v7

Current state.
In addition to above:
* Making logic auth method fallback more explicit and logical - done
* SMTP implemented and tested
* SMTP UI implemented and tested - done
* POP mostly working

TODO:
- POP still erratic, partly due pre-existing bugs
- Adapt OExpress / Outlook migrators
- "No auth" pref case
- Bugfixes

Comment 15

[Ben Bucksch (:BenB)]

Attachment: WIP v9 - working fairly well

It's now mostly finished.

Fixed POP.
Also implemented bug 339050 (proper error for Kerberos).

TODO:
- ReloadSmtpPanel - bug 58506
- OExpress / Outlook migrators
- migrate auth_login = 0
- Bugfixes
- Tests

Comment 16

[Ben Bucksch (:BenB)]

Attachment: WIP v10

TODO:
- ReloadSmtpPanel - bug 58506
- Tests

Comment 17

[Ben Bucksch (:BenB)]

Now also on http://hg.mozilla.org/users/mozilla.BenB_bucksch.org/mail-auth/

Comment 18

[Ben Bucksch (:BenB)]

WIPv10 =
http://hg.mozilla.org/users/mozilla.BenB_bucksch.org/mail-auth/rev/4de6ebed6fa4

Comment 19

[Nikolay Shopik]

When Kerberos/GSSAPI selected username option must be depricated or disabled otherwise it confuses, we already know username and realm. Problem may arise, by default when user create new mailbox only leftmost part of email used to put into username field. So if user have [e-mail] username will look like "user". Thunderbird will send only username principal, but username should be always contain realm otherwise same username from different real could access same mailbox.
Also if you by mistake manually enter into username field - [e-mail] instead [e-mail] it just won't work because realm must be capital all time.

Comment 20

[Ben Bucksch (:BenB)]

hm, you're right. The mail GSSAPI code
<http://mxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgProtocol.cpp#835>
uses the username, but the gssapi-lib calling code explicitly doesn't want it:
<http://mxr.mozilla.org/comm-central/source/mozilla/extensions/auth/nsAuthGSSAPI.cpp#375>

I'll see to remove it. Thanks for the hint.

However, that would mean that the user can only ever access one Kerberos mailbox at the same server at a time. It seems to be a limitation of the Unix gssapi lib implementation to hold only one identity/TGT at a time, but an unnecessary one, and a rather silly one. Any opinions?

Comment 21

[Ludovic Hirlimann [:Usul]]

(In reply to comment #20)

> However, that would mean that the user can only ever access one Kerberos
> mailbox at the same server at a time. It seems to be a limitation of the Unix
> gssapi lib implementation to hold only one identity/TGT at a time, but an
> unnecessary one, and a rather silly one. Any opinions?

I think the way kerberos is used and designed, having only one account per user is very acceptable.

Comment 22

[Nikolay Shopik]

If we still would like to add selection between TGT, maybe we could simplify this by drop down selection of identities? And this apply only when user using Windows/SSPI, but not MIT libs(Windows,Unix,etc).

BTW if this is Unix gssapi lib limitation Bug 523498 is invalid then. Still need to be asked on Kerberos mailling list, maybe implementation updated since Kerberos added to Thunderbird.
Anyway I'm agree with Ludo, this why it's called single sign-on :).

Comment 23

[Ben Bucksch (:BenB)]

Thanks for the pointer to bug 523498. I personally don't think it's acceptable, and apparently that other user thinks so too. But having a separate bug on it means we can keep this out of this bug. So, for this bug, I'll remove the username (should not be hard), and we can re-add it here, with textfield or dropdown or whatever, in the other bug, if possible and somebody cares. Thanks.

Comment 24

[Ben Bucksch (:BenB)]

Attachment: WIP v12 - with tests

Spent the last 1.5 weeks on unit tests.

- Implemented CRAM-MD5 (for real) in fakeservers
- AUTH PLAIN and LOGIN consistently in all fakeservers
- fixed bugs in testsuite
- Implemented tests to test login schemes

Still some small things to do, but this is already in good shape I think.

Test builds from TryServer:
<http://s3.mozillamessaging.com/build/try-server/2009-12-17_04:[e-mail]-mailauth2/[e-mail]>
<http://s3.mozillamessaging.com/build/try-server/2009-12-17_04:[e-mail]-mailauth2/[e-mail]>
<http://s3.mozillamessaging.com/build/try-server/2009-12-17_04:[e-mail]-mailauth2/[e-mail]>

Everybody, please give it a try and take a careful look at all the login situations you can think of (various schemes, servers, biff etc.).

Comment 25

[Ben Bucksch (:BenB)]

Linux <http://s3.mozillamessaging.com/build/try-server/2009-12-17_04:[e-mail]-mailauth2/[e-mail]2>

Comment 26

[Nikolay Shopik]

I'm sorry Ben, but this build (Win32) doesn't work for me. When creating new profile it just won't create new email account. And doesn't work for existing profile too, just show up empty windows.
I've tried latest nightly and it works for me.

Comment 27

[Ben Bucksch (:BenB)]

OK, I haven't tried the TryServer builds myself (my own Linux build works), I'll take a look later.

Comment 28

[Nikolay Shopik]

Tried to build on Win32, same problem.

Comment 29

[Ben Bucksch (:BenB)]

Fixed the problem (in hg repo), was a silly JS syntax error in migration.

Comment 30

[Nikolay Shopik]

Ok, I've tried
python client.py checkout
make -f client.mk
But still see empty windows, should delete and clone hg repo again?

Comment 31

[Nikolay Shopik]

Error console shows this
Error: missing ; before statement
Source File: chrome://messenger/content/msgMail3PaneWindow.js
Line: 1184, Column: 75
Source Code:
                : (useSecAuth ? at.kAuthSecure : at.kAuthPasswordCleartext)));

Comment 32

[Ben Bucksch (:BenB)]

Nikolay, yes, that's exactly the bug that I fixed.
See <https://hg.mozilla.org/users/mozilla.BenB_bucksch.org/mail-auth/rev/f32566515b28>. Try "hg pull; hg update" in the TB source dir. If that doesn't work, try
hg pull https://hg.mozilla.org/users/mozilla.BenB_bucksch.org/mail-auth/
and then hg update

Comment 33

[Nikolay Shopik]

Got it working, only after I pull from your repo.
So far this looks good for me. Things I've noticed there no differentiation between no tickets or principal doesn't exist, will show you same error. 

Also when you are create for example new smtp server with name mail.example.org and have principal for this name it will work even if you change hostname for something like mailserver.example.org. So if you create new account with wrong hostname and it won't work because no principal exist and then rename to correct it still won't work untill you recreate it with correct hostname.
If this out of scope of this bug I may fill new one

And I suggest reword error message 
Please check that you are logged in to the Kerberos/GSSAPI realm.
To
Please check that you are acquired tickets to the Kerberos/GSSAPI realm.

Comment 34

[Ben Bucksch (:BenB)]

> So far this looks good for me.

Great!

> no differentiation between no tickets or principal doesn't exist

I can't differentiate, this is all hidden in the OS gssapi lib.

See also bug 524698.

> So if you create new account with wrong
> hostname and it won't work because no principal exist and then rename to
> correct it still won't work untill you recreate it with correct hostname.
> If this out of scope of this bug I may fill new one

I don't know what you mean... That the user account (principal) doesn't exist, or the realm (account domain) doesn't exist?
Anyways, yes, this is out of the scope of this bug. Please file a new bug.

Comment 35

[Nikolay Shopik]

I've filled Bug 536028.

Last not for patch is username still available for change, but should be disabled and full username@REALM should be passed to remote server.

Comment 36

[Ben Bucksch (:BenB)]

Attachment: Fix v1

I'll be gone for vacation for a good 3 weeks, and it's almost finished anyways (see below), so good chance to ask for a review.

bienvenu, could you please review this at a convenient time? (Of course you may be on holidays, too.) Please review carefully, I need your eyes specifically to check the C++ code, because only you know all of the corner cases. I tried to consider everything I know...

Implement
- ReloadSmtpPanel - bug 58506
- remove username for kerberos
- support Kerberos in XML in account creation (createinbackend, readfromxml)
- Unit test for IMAP auth
Bugs
- IMAP old-style login broken?
- POP3 servers advertize auth schemes, but drop connection (instead of just -ERR) when we try them. cache failures across connections. Store m_failedAuthSchemes in pop3conndata or pop3server, if necessary.
- account wizard, manual config: outgoing auth mech
Test
 with real-world ISP servers
 outlook express migration on win
 seamonkey
 migration

Comment 37

[Ben Bucksch (:BenB)]

(The stuff starting with "Implement" was my remaining personal TODO list.)

Comment 38

[Ben Bucksch (:BenB)]

If anybody wonders why you keep scrolling the patch, it's because it's >9700 lines long, of which ~3000 lines are test code.
Probably makes sense to do the review in a TXT file. Feel free to check a REVIEW.txt into the repo root.
https://hg.mozilla.org/users/mozilla.BenB_bucksch.org/mail-auth/

Comment 39

[Ben Bucksch (:BenB)]

I really would like bienvenu to review this, though. Sorry :-/

Comment 40

[David :Bienvenu]

see bug 536218 for a specific issue.

Comment 41

[Ben Bucksch (:BenB)]

Test builds, new try:
<http://s3.mozillamessaging.com/build/try-server/2010-01-25_04:[e-mail]-try3/[e-mail]>
<http://s3.mozillamessaging.com/build/try-server/2010-01-25_04:[e-mail]-try3/[e-mail]2>
<http://s3.mozillamessaging.com/build/try-server/2010-01-25_04:[e-mail]-try3/[e-mail]>

Comment 42

[Nikolay Shopik]

These new builds still give me choice of username when Kerberos selected.

Comment 43

[Ben Bucksch (:BenB)]

Nikolay, I implemented it (hide username field when GSSAPI is selected), but ran into 2 problems:
* Thunderbird needs a username for incoming servers for internal purposes.
  It uses the username + hostname as ID in URLs, the directory name on disk
  and similar. (There's a difference between the "username", which is used
  internally, and the "realUsername", which is used against the server,
  as you noticed in bug 530319, but the fact remains that we need
  some username.) I tried using "nousername" as dummy username, but that's
  of course ugly. Changing TB's reliance on a username is not feasible.
* Most importantly, the username apparently *is* used. If I use "nousername"
  (or something else) as username in the username UI field, I can no longer
  log in to the Kerberos server. If I change it back to "ben", I can log in
  again.

So, I am not making that change. I have a patch, in case anybody is interested. Bug 523498 asks to differentiate between several GSSAPI identities anyways.

Comment 44

[Nikolay Shopik]

Can we just get username from current ticket, does krb5-gssapi provide such function and put it into username field?

Comment 45

[Ben Bucksch (:BenB)]

Maybe, but not in this bug.

Comment 46

[Nomis101]

I'm not sure if this is relevant, but if I open Thunderbird with this patch without any profile I get an NS_ERROR_UNEXPECTED. You can reproduce this with the following steps (on OS X). If the Application is in your Application folder, open Terminal and type:
$ mkdir /profile
$ /Applications/Shredder.app/Contents/MacOS/thunderbird-bin -profile /profile
Than the Terminal tells me:

-- Exception object --
+ QueryInterface (function) 3 lines
+ message (string) 'Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsIPrefBranch.getCharPref]'
+ result (number) 2147549183
+ name (string) 'NS_ERROR_UNEXPECTED'
+ filename (string) 'chrome://messenger/content/msgMail3PaneWindow.js'
+ lineNumber (number) 1489
+ columnNumber (number) 0
+ location (object) JS frame :: chrome://messenger/content/msgMail3PaneWindow.js :: MigrateServerAuthPref :: line 1489
+ inner (object) null
+ data (object) null
+ initialize (function) 3 lines
*

The mentioned line 1489 is
> var accounts = gPrefBranch.getCharPref("mail.accountmanager.accounts").split(",");

Tested with the trybuild from Comment 41 and with an own build (with patch) on Mac OS X 10.6.
I don't get this if I start with my normal profile, only if it is a new empty profile.

Comment 47

[Ben Bucksch (:BenB)]

> I open Thunderbird with this patch without any profile

I don't think it's possible to open TB without any profile?
Do you mean a fresh, new profile?

> var accounts = gPrefBranch.getCharPref("mail.accountmanager.accounts").split(",");

(Thanks for quoting the line, because my line numbers are different, for some reason.)

I think that is an unexpected error. The pref should exist, but doesn't in your case. Maybe because you don't have any accounts (not even Local Folders) yet.

> I get an NS_ERROR_UNEXPECTED

This is indeed unexpected. Given that this is the migration, and it has a try/catch and is simply logging the error, I don't think it's a problem.

Comment 48

[Nomis101]

(In reply to comment #47)
> I don't think it's possible to open TB without any profile?
> Do you mean a fresh, new profile?
Yes, a new profile without an account.

> This is indeed unexpected. Given that this is the migration, and it has a
> try/catch and is simply logging the error, I don't think it's a problem.
OK

Comment 49

[Ben Bucksch (:BenB)]

Attachment: Patch, v12

Current patch. Fully implemented, ready for review.


Here are my notes:

Questions:
- IMAP and POP: why tries counter and loop over logins? Apparently infinite loops, see bug 262584. We now just prompt directly, so no problem anymore I think. Removed in patch. (what about biff, though? Just exit? See TODO comment in patch.)
- Why overlay smtpEditOverlay.xul in am-smtp.xul? Removed in patch.
- POP3_READ_PASSWORD state: why directly to SEND_USERNAME (skip password) here?
- What is "logon fallback" (nsMsgIncomingServer::GetLogonFallback()) pref for? Added in bug 249240 #c20. Seems rarely used (barely any references in code and on web). Removed in patch. If needed, can better do in InitPrefAuthMethods().
- What was "authLogin" pref for? Why did it GetPassword() and old-style login in IMAP, and no password (just USER, no PASS command) in POP? what should auth_login = false (no login) do for IMAP? AUTH EXTERNAL? <http://www.apps.ietf.org/rfc/rfc4422.html#page-29>

wontfix:
- POP3 servers advertize auth schemes, but drop connection (instead of just -ERR) when we try them. cache failures across connections. Store m_failedAuthSchemes in pop3conndata or pop3server, if necessary.
  E.g. pop.gmx.net without SSL when trying USER with a non-existing account
  Can't fix due to bug 533006. That bug also means we didn't properly cover this case before either. Given that we didn't get many bug reports, doesn't seem to be a real-world problem.

Bugs found and filed:
- POP biff: why are we saying hello to the server and then quit when we don't have a password? Periodical biff is not done. Filed bug 533083.
- Server closes connection after failed username/password, and we don't notice that and dangle. Filed bug 533006.

Comment 50

[Ben Bucksch (:BenB)]

New preferences documentation:

-pref("mail.server.default.logon_fallback", true);
-pref("mail.server.default.auth_login", true);
-pref("mail.server.default.useSecAuth", false);
+pref("mail.server.default.authMethod", 3); // cleartext password. @see nsIMsgIncomingServer.authMethod.
Old to new:
+      /* auth_login = false => no auth
+         else: useSecAuch = true => "secure auth"
+         else: cleartext pw */

-pref("mail.smtpserver.default.auth_method", 1); // auth any
-pref("mail.smtpserver.default.useSecAuth", false);
-pref("mail.smtpserver.default.trySecAuth", true);
+pref("mail.smtpserver.default.authMethod", 3); // cleartext password. @see nsIMsgIncomingServer.authMethod.
Old to new:
+      /* auth_method = 0 => no auth
+         else: useSecAuch = true => "secure auth"
+         else: cleartext pw */

Valid values for *.authMethod:
+  /// No login needed. E.g. IP-address-based.
+  const long kAuthNo = 1;
+  /// Let us pick any of the auth types supported by the server.
+  /// Discouraged, because vulnerable to MITM attacks, even if server offers secure auth.
+  const long kAuthAny = 2;
+  /// password in the clear. PLAIN / LOGIN
+  const long kAuthPasswordCleartext = 3;
+  /// hashed password. CRAM-MD5, DIGEST-MD5
+  const long kAuthPasswordEncrypted = 4;
+  /// Kerberos / GSSAPI (Unix single-signon)
+  const long kAuthGSSAPI = 5;
+  /// NTLM is a Windows single-singon scheme.
+  /// Includes MSN / Passport.net, which is the same with a different name.
+  const long kAuthNTLM = 6;
+  /// Encrypted password or Kerberos / GSSAPI or NTLM.
+  /// Deprecated, for migration only.
+  const long kAuthSecure = 8;

Comment 51

[Ben Bucksch (:BenB)]

(Explanation: A "-" (minus) in front of the lines above means that this pref is now deprecated and no longer used in new TB versions. New TB versions will only use it for automatically migrating it to the new pref authMethod, if the latter doesn't exist yet. During migration, the old user prefs are left intact, in case the user goes back to an older version.)

I plan to add the pref definitions to <http://kb.mozillazine.org/Mail_and_news_settings> later once this is bug is checked in, if I am allowed to and don't forget it.

Comment 52

[Ben Bucksch (:BenB)]

Attachment: Patch, v13

ops, attached wrong file.

Comment 53

[David :Bienvenu]

Comment on attachment 424983 [details] [diff] [review]
Patch, v13

I tried to apply this - it has bit-rotted some. We also talked over IRC about replacing the printfs with PR_Logs

Comment 54

[Simon Wilkinson]

(In reply to comment #44)
> Can we just get username from current ticket, does krb5-gssapi provide such
> function and put it into username field?

No, you can't. The mapping between Kerberos principal and username is a decision of the remote server, and the client doesn't have the necessary information to make that choice. A client may, however send an empty username to the server (this is the authorization identity, in SASL speak), which tells the server to use the default for that principal.

Ben: I'm happy to take a look at this patch when you get a chance to upload a new revision.

Comment 55

[Ben Bucksch (:BenB)]

Attachment: Patch, v14 - merged, no PRLogs yet

bienvenu, please imagine that the printfs are PRLogs until I change them :).

Comment 56

[Ben Bucksch (:BenB)]

Attachment: Fix, v15 - printf()s removed (and merged with trunk)

printf()s replaced with PR_LOG(), as requested.

nsMsgProtocol.cpp doesn't have a PR_LOG module (and I don't know which one to use), but I still think they're useful, e.g. for bug 530319, so I just surrounded them with #ifdef DEBUG_BenB.

Comment 57

[Simon Wilkinson]

Some quick comments from a cursory read of the patch:

> 5104=The Kerberos/GSSAPI ticket was not accepted by the IMAP server %S. Please check that you are logged in to the Kerberos/GSSAPI realm

"Please check you have valid Kerberos tickets" would probably be a better way of putting this.
(and similarly for the POP and SMTP errors). I'm also not convinced that you have to use Kerberos/GSSAPI. In this use case GSSAPI implies Kerberos (nothing else is supported by the GSSAPI SASL mechanism, due to a mistake when it was specified).

There still seems to be a lot of debugging code scattered throughout the patch. For example, in mailnews/base/prefs/content/accountUtils.js :

>if (!getCurrentAccount())var servers = smtpService.smtpServers;
>    alert("select server");
>    selectServer(null, null);
>    alert("selected server");
>}

In nsSmtpProtocol::SendEhloResponse you seem to fall straight through to sending a normal HELO, regardless of whether authentication has been requested - the old code used to exit with an error here. I can't see anywhere else that this case is handled, though.

In the case where the user is explicitly using GSSAPI, we should never prompt them for a password. If GSSAPI fails, then they should be told so, and asked to check their current tickets.

That's all for now. More later, I hope.

Comment 58

[Nikolay Shopik]

(In reply to comment #54)
> A client may, however send an empty username
> to the server (this is the authorization identity, in SASL speak), which tells
> the server to use the default for that principal.
That's probably good choice by default, instead of current one. Thanks for explanation.

Comment 59

[Ben Bucksch (:BenB)]

> There still seems to be a lot of debugging code scattered throughout the patch.

Yes, I intentionally left all the debugging code in so far, to help the reviewer and any possible changes I still have to do. For me, removing the debugging stuff is the *last* thing to do.

> "have valid Kerberos tickets" [vs. "logged in to Kerberos realm"]
> GSSAPI implies Kerberos

Error messages are made for end-users, not techies. A secretary starting in a large organization may not know what a "ticket" is, but she understands "logged in".
Similarly, most people use Kerberos, so it's important to have that in the message, but GSSAPI is the technically correct term, so I use both. But if you say that GSSAPI can never support anything other than Kerberos, I can just as well say only "Kerberos", yes.

> In the case where the user is explicitly using GSSAPI, we should never
> prompt them for a password. If GSSAPI fails, then they should be told so,
> and asked to check their current tickets.

That's what this patch does, or at least was intended. In fact, that was kind of the point of it all. Where do I show the password prompt when "GSSAPI" (not "Any Secure") was selected as auth mech?

Comment 60

[Ben Bucksch (:BenB)]

The alert()s really were pointless and bad. Fixed some other useless debug statements, too. Should be releasable now, with 2 exceptions (kDebug = true and the dump() in migration) that I'll change later.

Comment 61

[Ben Bucksch (:BenB)]

Attachment: Patch. v16

New version....

Maybe we can just use the hg repo [1] as base for review? That would save attaching 400K-patches all the time...

[1] http://hg.mozilla.org/users/mozilla.BenB_bucksch.org/mail-auth/

Comment 62

[David :Bienvenu]

In the interest of making the patch smaller, I would think that some of the test infrastructure changes (i.e., the fake server enhancements) and even some of the tests that don't depend on core changes could land w/o the core changes, right?

Comment 63

[Ben Bucksch (:BenB)]

If you tell me which files I should land, I will. :)
(Separating it out into 2 patches would be harder, though)

Comment 64

[David :Bienvenu]

I'm kinda hoping you'll know which tests don't depend on core changes.

Comment 65

[Ben Bucksch (:BenB)]

Attachment: Fix, v17

Fix POP3 PR_LOG and tests after bitrot and change mistakes

Comment 66

[David :Bienvenu]

Comment on attachment 426933 [details] [diff] [review]
Fix, v17

+    const at = Components.interfaces.nsIMsgIncomingServer;

I take it at means authType - if you could spell that out, it would make the code easier to read.

+    for (var i = 0; i < accounts.length; i++)

should use let i = 0; here and below.

+      try { auth_login = gPrefBranch.getBoolPref(server + "auth_login"); } catch (e) {}
+      try { useSecAuth = gPrefBranch.getBoolPref(server + "useSecAuth"); } catch (e) {}

Mozilla style is to make this multi-line.

+      gPrefBranch.setIntPref(server + "authMethod",
+              auth_login == false
+                ? at.kAuthNo
+                : (useSecAuth ? at.kAuthSecure : at.kAuthPasswordCleartext));

the parameters should line up, so auth_login should line up with server above it.

+         else: useSecAuch = true => "secure auth"

typo - useSecAuth

and lose the dump statements...

+    rv = m_hostSessionList->SetPasswordForHost(GetImapServerKey(), password.get());
+    rv = m_hostSessionList->GetPasswordVerifiedOnline(GetImapServerKey(), passwordAlreadyVerified);
+    if (NS_SUCCEEDED(rv) && !passwordAlreadyVerified)

The first rv is dropped, so probably better to say (void) 

+  if (!getCurrentAccount())
+  {
+    selectServer(null, null);
+  }

no need for braces here.

indentation wrong here:

+        else if (m_currentAuthMethod == POP3_HAS_AUTH_USER &&
+            !m_password_already_sent)

and here:

+        if (m_password_already_sent || // (also true for GSSAPI)
+          m_currentAuthMethod == POP3_HAS_AUTH_NONE)

breaks go on their own line:

+      PR_LOG(POP3LOGMODULE, PR_LOG_DEBUG, ("POP username"));
+      m_pop3ConData->next_state = POP3_SEND_USERNAME; break;

This looks like it's returning an nsresult:

+PRInt32 nsSmtpProtocol::NextAuthMethod()

or mixing them. But if it's an nsresult, it should return NS_OK, not 0. And I think the name should be AdvanceToNextAuthMethod() or something like that to be clear that it's not actually returning an auth method.

What's the verdict here? If we're doing what the comment says, we should remove the commented out code, leave the comment, and remove the TODO part of the comment.

+            /*
+            // EHLO is always needed if authentication is requested.
+            // TODO just fall into ProcessAuth and fail there?
+            else if (m_prefAuthMethods > 0)
+            {
+                m_nextState = SMTP_ERROR_DONE;
+                m_urlErrorState = NS_ERROR_COULD_NOT_LOGIN_TO_SMTP_SERVER_AUTH_NONE;
+                return(NS_ERROR_COULD_NOT_LOGIN_TO_SMTP_SERVER);
+            }*/

I don't remember why we added the hidden logon_fallback pref, but I think if we're not failing from secure to insecure methods, it shouldn't be important.

more breaks that should go on their own line:

+      case is.kAuthNo: authStr = "authNo"; break;
+      case is.kAuthPasswordEncrypted: authStr = "authPasswordEncrypted"; break;

don't want to let this in :

-let kDebug = false;
+let kDebug = true;

I think we're going to want Blake to look at the autoconfig changes. And I can't really review the seamonkey changes, officially.

Should these commented out lines just get removed?

+            //if (!GetServerStateParser().LastCommandSuccessful())
+            //  m_failedAuthMethods |= kHasCRAMCapability;
+          }

I'm going to look at the test changes in more detail, since I'd like to land the ones of those we can first...

Comment 67

[David :Bienvenu]

Does this patch affect/break existing profiles? I.e., if I run with this patch, and then go back to 3.0, or 2.0x, could it potentially break authentication for those users by changing some prefs?

         default:
           if (aExitCode != NS_ERROR_ABORT && !NS_IS_MSG_ERROR(aExitCode))
+          {
             aExitCode = NS_ERROR_SMTP_SEND_FAILED_UNKNOWN_REASON;
+          }

no need to add these braces.


+/**
+ * @returns aResult 0 = retry, 1 = cancel, 2 = new password (you need to ask for it)
+ */
 NS_MSG_BASE nsresult MsgPromptLoginFailed(nsIMsgWindow *aMsgWindow,

This kind of comment is more appropriate for nsMsgUtils.h. And in fact, there's already a more complete comment there, so no need to duplicate here.

+  // pre-flight that we have nss - on the ui thread - for MD5 etc.
+  switch (authMethod)
   {
+  case nsIMsgIncomingServer::kAuthPasswordEncrypted:
+  case nsIMsgIncomingServer::kAuthSecure:
+  case nsIMsgIncomingServer::kAuthAny:
     nsCOMPtr<nsISignatureVerifier> verifier = do_GetService(SIGNATURE_VERIFIER_CONTRACTID, &rv);
     NS_ENSURE_SUCCESS(rv, rv);
+    break;
   }

the case lines need to be indented.

+
+            for (PRUint32 j=0; j<16; j++)

spaces around operators.

+            m_currentAuthMethod = POP3_HAS_AUTH_NONE;
+            m_pop3ConData->next_state = POP3_SEND_USERNAME; // what sense does that make? we just asked the user for a password (and checked that it's not empty)!

that comment line is way too long - maybe move the comment to its own line (or fix the code so the comment isn't needed)

If you're going to remove the logonfailure count, you should just remove it, e.g.,
-    PRInt32 logonFailureCount;
+    //PRInt32 logonFailureCount;

+  PRInt32 m_failedAuthMethods; // dito

typo - ditto.

looking at the fake server and related changes in more detail:

+  var challenge = AuthCRAM.createChallenge("fake.invalid");
+  challenge = atob(challenge); // decode. function currently returns it already encoded
+  var chap = challenge.split("@");

what does chap mean? A more meaningful var name would be good.

+  var real = new Array();
+  for (var i = 0; i < fromServer.them.length; i++)

real what? more meaningful var would make the code more readable.

in test_nsIMsgFolderListenerIMAP.js, is removing resetTest() needed, or just cleanup, because the call isn't needed anymore?

 function addFolder(parent, folderName, storeIn)
 {
-  gServer.resetTest();

no need for braces here:

+    if (firstTest) {
+      firstTest = false;
+    }
+    else
+      server.resetTest();

and at least one other place where the same code pattern occurs.

+ * Portions created by the Initial Developer are Copyright (C) 2008
+ * the Initial Developer. All Rights Reserved.

2010

+    var digest = this.md5(
+      this.xor(key, kOuterPad).concat(
+      this.md5(
+        this.xor(key, kInnerPad).concat(
+        text))));

oddly wrapped - this.md5 should be indented, last line could go with the prev line, etc).

-        return this._tag + " BAD okay, I won't authenticate you.";
+        return this._tag + " BAD okay, chicken, as you wish";

uh... :-)

+      return "OK Hello friend! Friends give friends good advise: Next time, use CRAM-MD5";
+    }
+    else {
+      return "BAD Crook";
+    }

"advice". and maybe more descriptive text than "Crook" :-)

just remove the commented out line?

+      //return "BAD I don't know anybody of that name"; // reveals valid usernames

It'll be really cool to have these auth tests!

Comment 68

[Ben Bucksch (:BenB)]

Re style: in many cases, esp. with the break;s and the "at", I was trying to keep the code more concise to make the meaning clearer, but I'll change them.

> oddly wrapped

I was trying to make clearer what this statement does, because it's hard to read, but I guess I can't really. I'll fix the wrapping.

> the case lines need to be indented.

I thought the most common style was to put them on the same column as the switch and {}?

+            /*
+            // EHLO is always needed if authentication is requested.
+            // TODO just fall into ProcessAuth and fail there?

> What's the verdict here? If we're doing what the comment says, we
> should remove the commented out code, leave the comment, and remove
> the TODO part of the comment.

That TODO, like most other TODOs which I left in at this stage, are a question to you. It's something I wasn't sure of and was hoping you'd have advise.

+            m_pop3ConData->next_state = POP3_SEND_USERNAME; // what sense does
that make? we just asked the user for a password (and checked that it's not
empty)!

> that comment line is way too long

Do you know what's up with that (what the comment asks)?

> the logonfailure count, you should just remove it

OK.

> in test_nsIMsgFolderListenerIMAP.js, is removing resetTest() needed, or just
> cleanup, because the call isn't needed anymore?

IIRC both. Not needed, and now breaks the test, because resetTest() is now resetting more thoroughly and that test neither sets up the server again after the reset nor needs the reset.

> Does this patch affect/break existing profiles? I.e., if I run with
> this patch, and then go back to 3.0, or 2.0x, could it potentially
> break authentication for those users by changing some prefs?

It's designed to not break them, by simply leaving the old user prefs in there.

(I have one case where it doesn't work anymore, but that could be any reason including bad build. Also, if the user changes the pref in 3.1, the change takes no effect in 3.0, of course.)

> It'll be really cool to have these auth tests!

Thanks :). Took me over a week to write them.
I'm a bit proud of the CRAM-MD5-implementation, which is for real, no fake.

Comment 69

[Nikolay Shopik]

(In reply to comment #67)
> Does this patch affect/break existing profiles? I.e., if I run with this patch,
> and then go back to 3.0, or 2.0x, could it potentially break authentication for
> those users by changing some prefs?

David, see  Bug 536862, this is one time crash I get when change prefs and return to 3.0 version

Comment 70

[Ben Bucksch (:BenB)]

As you can read there, it's most likely unrelated. It's crashing in GSSAPI lib, and you can't reproduce it. Please don't blame me for foreign crashes.

Comment 71

[Nikolay Shopik]

Ben, I don't blame you. I just answer to David question. And if it unrelated to migration code, I'm ok with that.

Comment 72

[David :Bienvenu]

thx, Nikolay

Comment 73

[Ben Bucksch (:BenB)]

Attachment: Fix, v18

Fixed review nits. Changes: <http://hg.mozilla.org/users/mozilla.BenB_bucksch.org/mail-auth/rev/4866e4b9aa64>

Comment 74

[Ben Bucksch (:BenB)]

Attachment: Fakeserver only, v1 (commited)

Per comment 62, here are the changes to the fakeserver, which don't depend on the backend changes.

Comment 75

[David :Bienvenu]

Comment on attachment 427654 [details] [diff] [review]
Fakeserver only, v1 (commited)

No need for temp var hardCodedLine here:

+  var hardcodedLine = "AGZyZWQxAHdpbG1hMg==";
+  do_check_eq(line, hardcodedLine);

don't need temp var req here:

+    var req = AuthLOGIN.decodeLine(line);
+    if (req == this.kUsername) {
+      this._nextAuthFunction = this.authLOGINPassword;
+      this._multiline = true;
+      return "+ " + btoa("Password:");
+    }
+    else {
+      // Don't return "BAD" error yet, because that would reveal valid usernames
+      this._nextAuthFunction = this.authLOGINBadUsername;
+      this._multiline = true;
+      return "+ " + btoa("Password:"); // Just pretend
+    }

and it looks like you could just say:

this._nextAuthFunction = (this.kUsername == AuthLOGIN.decodeLine(line); ? this.authLOGINPassword : this.authLOGINBadUsername;

and get rid of the if entirely since the other lines are the same. So the function is 3 lines instead of 12 :-) I think this function occurs a couple times in the patch, so it could be cleaned up a couple times.

still prefer let instead of var for these kinds of loops:

+    for (var i = 0; i < this._readers.length; i++)
+      this._readers[i].setDebugLevel(debug);

You can just stick the "." on the end of the previous literal:

+    capa += "IMPLEMENTATION fakeserver\r\n";
+    capa += ".";

No need for braces here:

+      if (line == "*") {
+        return "-ERR Okay, as you wish. Chicken";
+      }
+      if (!func || typeof(func) != "function") {
+        return "-ERR I'm lost. Internal server error during auth";
+      }

No need for braces here:
+    if (lineRest) { // all in one command, called initial client response, see RFC 4954
+      return this.authPLAINCred(lineRest);
     }

r=me, with those things addressed.

Comment 76

[Ben Bucksch (:BenB)]

You think this is OK?
+ if (AuthLOGIN.decodeLine(line) == this.kUsername)
+   this._nextAuthFunction = this.authLOGINPassword;
+ else
+   // Don't return "BAD" error yet, because that would reveal valid usernames
+   this._nextAuthFunction = this.authLOGINBadUsername;
+ this._multiline = true;
+ return "+ " + btoa("Password:");

With ?:, I'd have to wrap anyways :)

+    capa += "IMPLEMENTATION fakeserver\r\n";
+    capa += ".";

I just fear the dot would get lost.

I'll fix the rest. Thanks!

Comment 77

[David :Bienvenu]

(In reply to comment #76)
> You think this is OK?
yes, that's OK.

> 
> +    capa += "IMPLEMENTATION fakeserver\r\n";
> +    capa += ".";
> 
> I just fear the dot would get lost.
> 
> I'll fix the rest. Thanks!

you could say "...\r\n" + ".", I suppose.

Comment 78

[Ben Bucksch (:BenB)]

Comment on attachment 427654 [details] [diff] [review]
Fakeserver only, v1 (commited)

Thanks, bienvenu!

All style changes to attachment 427654 [details] [diff] [review] (fakeserver part) done and commited to comm-central as <http://hg.mozilla.org/comm-central/rev/0c3188870f0d>.

Comment 79

[Ben Bucksch (:BenB)]

Attachment: Fix, v20 - without fakeserver changes

Comment 80

[David :Bienvenu]

Comment on attachment 428272 [details] [diff] [review]
Fix, v20 - without fakeserver changes

This comment style doesn't match our existing styles of either // for each line, or 
/** 
 *
 */
occurs in a bunch of places

We're going to need to break the autoconfig stuff into a separate patch, as much as possible, so that Blake can review it

These breaks still need to go on their own line:

+      case is.kAuthNo: authStr = "authNo"; break;
+      case is.kAuthPasswordEncrypted: authStr = "authPasswordEncrypted"; break;
+      case is.kAuthGSSAPI: authStr = "authKerberos"; break;
+      case is.kAuthNTLM: authStr = "authNTLM"; break;
+      case is.kAuthSecure: authStr = "authAnySecure"; break;
+      case is.kAuthPasswordCleartext:
+ 

I also remembered why we had the limit to logon attempts, which was to make it harder for the user to get locked out for entering too many bad passwords. The user can get around it, even with the loop limits, but that was the thinking.

that's it for now...

Comment 81

[Ben Bucksch (:BenB)]

> This comment style

What do you mean with "this"?

I use /** ... */ for JavaDoc/doxygen comments which document function signatures or classes/files, and /* ... */ for multi-line comments inside a function. I can change the latter to //, if you prefer that.

> need to break the autoconfig stuff into a separate patch, as much as possible

It's not really possible, because the autoconfig changes are a direct result of the authMethod pref changes. (In other words, they are one atomic changeset.)
Of course I can make a separate diff file for them, solely for the purpose of review, but it wouldn't work when applied. Wouldn't it be simpler if bwinton just looked at accountcreation/ parts of the current patch?

> These breaks still need to go on their own line:

OK, will do.

> I also remembered why we had the limit to logon attempts, which was to make it
> harder for the user to get locked out for entering too many bad passwords. The
> user can get around it, even with the loop limits, but that was the thinking.

Ah, that's good to know. I don't think that's relevant anymore. I changed the logic considerably. I don't think we are now trying more often than we did before (if anything, we may try less, because the prefs are more specific). We also give the user clearer control.

Comment 82

[David :Bienvenu]

sorry, I meant to paste in an example of a comment with non-standard style:

+      /* auth_login = false => no auth
+         else: useSecAuth = true => "secure auth"
+         else: cleartext pw */

Comment 83

[Ben Bucksch (:BenB)]

Done, see <https://hg.mozilla.org/users/mozilla.BenB_bucksch.org/mail-auth/>

Comment 84

[Ben Bucksch (:BenB)]

Comment on attachment 428272 [details] [diff] [review]
Fix, v20 - without fakeserver changes

bwinton, please review the mailnews/base/prefs/content/accountcreation/ changes *only*.

Comment 85

[David :Bienvenu]

Comment on attachment 428272 [details] [diff] [review]
Fix, v20 - without fakeserver changes

I still see this several places:

+      /* auth_login = false => no auth
+         else: useSecAuth = true => "secure auth"
+         else: cleartext pw */

second line here isn't indented enough:

+      if (!gPrefBranch.prefHasUserValue(server + "useSecAuth") &&
+        !gPrefBranch.prefHasUserValue(server + "auth_method"))
+        continue;

capitalizing Authentication here (and elsewhere) is odd:

+5101=You cannot log in to %S, because the server does not support the selected authentication method. "Please change the Authentication method in the Account Settings."

+## @name POP3_INTERNAL_ERROR
+## @loc None

this only seems to be used during authentication, so I think it's better to include that in the error message, and the #define name.

some breaks that still need to be on their own line:

+    switch (aServer.authMethod)
+    {
+      case is.kAuthNo: authStr = "authNo"; break;
+      case is.kAuthPasswordEncrypted: authStr = "authPasswordEncrypted"; break;
+      case is.kAuthGSSAPI: authStr = "authKerberos"; break;
+      case is.kAuthNTLM: authStr = "authNTLM"; break;
+      case is.kAuthSecure: authStr = "authAnySecure"; break;

The smtp server edit dialog looks weird - the username field is right aligned, and there's no longer room for my whole user name.

+   * Compatibility note: This attribute had a different meaning in TB < 3.1
    */
   attribute long authMethod;

Why does this mean that a profile touched with 3.1 will be fine with 2.x or 3.0? Isn't this var persisted as a pref? If the meaning is different, that would seem to imply an incompatibility of some sort.

lose the parens here:

+                return(NS_ERROR_COULD_NOT_LOGIN_TO_SMTP_SERVER);

typo - anything:

+          // we didn't even try anyting, so we had a non-working config (pref doesn't match server)

Generally, for if clauses, we align them after the ( on the preceding line (this is in a bunch of places):

+    else if (m_currentAuthMethod == SMTP_AUTH_NTLM_ENABLED ||
+        m_currentAuthMethod == SMTP_AUTH_MSN_ENABLED)
 
I'm skeptical that you needed to add the call to m_hostSessionList->SetCapabilityForHost because parsing the capability response calls the same method. So I'd prefer that you not forget the capabilities here.

+                  // force re-issue of Capability(), because servers may enable
+                  // other auth features (e.g. PLAIN auth) after we upgraded to a secure connection
+                  m_hostSessionList->SetCapabilityForHost(GetImapServerKey(), kCapabilityUndefined);

The logonCookie stuff should all get ripped out at some point; it had to do with AOL imap's proprietary auth mechanism, which we stopped supporting a long time ago. Just an FYI - you can leave it in. 

lose the commented out line:

+  //do_timeout(1000, nextTest);
+  nextTest();

We create an inbox when we create the server so the user will have something to click on to initiate a connection to the server and run folder discovery. So lose the dumps and commented out code:

+    dump("performExpand()\n");
+    incomingServer.performExpand(null);
+    dump("server.performTest()\n");
+    server.performTest("LSUB");
+
+    //dump("checking root folder, should " + (thisTest.expectSuccess ? "work" : "fail") + "\n");
+    //var rootFolder = incomingServer.rootFolder;
+    //do_check_eq(thisTest.expectSuccess, rootFolder.containsChildNamed("Inbox")); -- see comment at top

need to rev the uuid here:

 [scriptable, uuid(FC67F5C9-C8BB-46c1-90CB-1E752ECCEB19)]
 interface nsIPop3IncomingServer : nsISupports {
   attribute boolean leaveMessagesOnServer;
   attribute boolean headersOnly;
   attribute boolean deleteMailLeftOnServer;
-  attribute boolean authLogin;

Is this really an interesting thing to log? Less busy logs are easier to read :

+  if (!POP3LOGMODULE)
+      POP3LOGMODULE = PR_NewLogModule("POP3");
+  PR_LOG(POP3LOGMODULE, PR_LOG_DEBUG, ("new protocol object"));
+

cases need to be indented here:

+void nsPop3Protocol::InitPrefAuthMethods(PRInt32 authMethodPrefValue)
+{
+  // for m_prefAuthMethods, using the same flags as server capablities.
+  switch (authMethodPrefValue)
+  {
+  case nsIMsgIncomingServer::kAuthNo:
+    m_prefAuthMethods = POP3_HAS_AUTH_NONE;
+    break;
+  case nsIMsgIncomingServer::kAuthPasswordCleartext:
+    m_prefAuthMethods = POP3_HAS_AUTH_USER |
+        POP3_HAS_AUTH_LOGIN | POP3_HAS_AUTH_PLAIN;
+    break;

comment style:

+      /* AuthGSSAPI* falls in here in case of an auth failure.
+         If Kerberos was the only method, assume that
+         the user is just not logged in yet, and show an appropriate error. */

and just below that, need indented breaks.

remove commented out code here:

+    /* remove
+    if (m_currentAuthMethod == POP3_HAS_AUTH_GSSAPI)
+      m_pop3ConData->next_state = POP3_AUTH_GSSAPI;
+    else if (m_currentAuthMethod == POP3_HAS_AUTH_APOP)
+      m_pop3ConData->next_state = POP3_SEND_PASSWORD;
+    else if (m_currentAuthMethod == POP3_HAS_AUTH_CRAM_MD5)
+      m_pop3ConData->next_state = POP3_SEND_USERNAME;
+    else if (m_currentAuthMethod == POP3_HAS_AUTH_NTLM)
+      m_pop3ConData->next_state = POP3_AUTH_NTLM;
+    else if (m_currentAuthMethod == POP3_HAS_AUTH_LOGIN)
+      m_pop3ConData->next_state = POP3_AUTH_LOGIN;
+    else if (m_currentAuthMethod == POP3_HAS_AUTH_PLAIN)
+      m_pop3ConData->next_state = POP3_SEND_USERNAME;
+    else if (m_currentAuthMethod == POP3_HAS_AUTH_USER)
+      m_pop3ConData->next_state = POP3_SEND_USERNAME;
+    else
+      return Error(POP3_AUTH_MECH_NOTSUPPORTED);
+    */
+

feel free to rename AuthFallback method here:

+/**
+ * Attention: Misnomer!
+ * This is not really a fallback which is called in case of failure,
+ * but it's called when we finished one auth step (e.g. sending username
+ * or password) and want to proceed to the next one.
+ * If there's an auth failure, ProcessAuth() is called, ironically.
+ */
 PRInt32 nsPop3Protocol::AuthFallback()

remove commented out line:

-            m_pop3ConData->logonFailureCount++;
+            //m_pop3ConData->logonFailureCount++;

and here:

+            /*if (m_prefAuthMethods == POP3_HAS_AUTH_GSSAPI)
+               //not actually triggered, but in ProcessAuth()
+              Error(POP3_GSSAPI_FAILURE);
+            else*/
+              Error(POP3_PASSWORD_FAILURE);

rv looks to be ignored and dropped here:

+    if (m_currentAuthMethod == POP3_HAS_AUTH_NTLM)
+        rv = DoNtlmStep1(m_username.get(), password.get(), cmd);
+    else if (m_currentAuthMethod == POP3_HAS_AUTH_CRAM_MD5)

the comment line is way too long here. I'm also unclear on the comment itself. We're about to send a username, not a password, right?
-          PRBool prefBool = PR_FALSE;
-          m_pop3ConData->pause_for_read = PR_FALSE;
-          m_pop3Server->GetAuthLogin(&prefBool);
-
-          if (prefBool)
+          if (m_prefAuthMethods == POP3_HAS_AUTH_NONE)
+          {
+            m_currentAuthMethod = POP3_HAS_AUTH_NONE;
+            m_pop3ConData->next_state = POP3_SEND_USERNAME; // what sense does that make? we just asked the user for a password (and checked that it's not empty)!
+          }
+          else

nsIPop3IncomingServer has a handy method to reset capabilities, nsIPop3IncomingServer.pop3CapabilityFlags - did that not work, instead of deleting and recreating the server object?

+    // Mailnews caches server capabilities, so try to reset it
+    deletePop3Server();
+    incomingServer = createPop3Server();

please remove these lines:

+
+pref("browser.dom.window.dump.enabled", false);
+pref("nglayout.debug.disable_xul_fastload", true);

that's about it - I'm going to go run the tests now. I have not tested autoconfig, but I did test various authentication errors by using the account settings UI (speaking of which, it would look nicer if the connection type dropdown lined up with the auth method dropdown, but clarkbw has the real say)

Comment 86

[Ben Bucksch (:BenB)]

> I still see this several places:
> +      /* auth_login = false => no auth

Yes, as said in comment 83, I fixed this in the hg branch.
There, you can see clearly the intermediate diffs, what I changed as result of your review.

Same for the switch/case linebreaks and intentions - I already did those :-)

> capitalizing Authentication here (and elsewhere) is odd:

> +5101=You cannot log in to %S, because the server does not support the selected authentication method. "Please change the Authentication method in the Account Settings."

This is attempting to reflect the exact UI wording. Shall I put it in quotes or change the capitalization? i.e.
Please change the 'Authentication method' in the 'Account Settings'.   or
Please change the authentication method in the account settings.   ?

> +## @name POP3_INTERNAL_ERROR
> +## @loc None
> +4047=Internal state error. This is an internal, unexpected error in
> the application, please report it as bug.

> this only seems to be used during authentication, so I think it's better to
> include that in the error message, and the #define name.

I meant this as generic error, but I can of course make it specific to auth, if you wish.

> +   * Compatibility note: This attribute had a different meaning in TB < 3.1
>     */
>    attribute long authMethod;

> Why does this mean that a profile touched with 3.1 will be fine with 2.x or
> 3.0? Isn't this var persisted as a pref? If the meaning is different, that
> would seem to imply an incompatibility of some sort.

This note applies only to the IDL attribute, not the pref. Yes, they are reflected to a pref, but the old pref is called auth_method, the new one is called authMethod. So, no conflict.
The old pref auth_method was actually a misnomer: It wasn't about the auth *method*, but would only switch auth on and off, IIRC. Now, the new pref authMethod really is about the method, and is the same as for the incoming server.

> The logonCookie stuff should all get ripped out at some point; it had to do
> with AOL imap's proprietary auth mechanism, which we stopped supporting a long
> time ago. Just an FYI - you can leave it in. 

Good. AOL now supports AUTH=PLAIN anyways, luckily.

+    //do_check_eq(thisTest.expectSuccess,
rootFolder.containsChildNamed("Inbox")); -- see comment at top
> We create an inbox when we create the server

Even if we could never log in??

I was looking for a reliable way of checking that the server allows us access (or not).

> Is this really an interesting thing to log?
+  PR_LOG(POP3LOGMODULE, PR_LOG_DEBUG, ("new protocol object"));

No, it's not. :)

+          if (m_prefAuthMethods == POP3_HAS_AUTH_NONE)
+            m_pop3ConData->next_state = POP3_SEND_USERNAME; // what sense does
that make? we just asked the user for a password (and checked that it's not
empty)!

> I'm also unclear on the comment itself.
> We're about to send a username, not a password, right?

We do that when the server does *not* need auth. Why ask for a password and send a username when we specifically said it's a non-auth server?
I'd have to look closer at the code again.

+    // Mailnews caches server capabilities, so try to reset it
+    deletePop3Server();
+    incomingServer = createPop3Server();

> nsIPop3IncomingServer has a handy method to reset capabilities,
> nsIPop3IncomingServer.pop3CapabilityFlags - did that not work, instead of
> deleting and recreating the server object?

I wasn't aware of that.
Now that I have the delete/recreate code, I wonder whether I should keep it, because it's probably safer as test. Not sure yet.


I'll fix the many other nits.

> I did test various authentication errors by using the account
> settings UI

Good!

Thanks,

Ben

Comment 87

[David :Bienvenu]

(In reply to comment #86)
> > I still see this several places:
> > +      /* auth_login = false => no auth
> 
> Yes, as said in comment 83, I fixed this in the hg branch.
> There, you can see clearly the intermediate diffs, what I changed as result of
> your review.

Sorry, I find those interdiffs really painful to look at and since you attached a new patch, I thought it would have those changes.

> > +5101=You cannot log in to %S, because the server does not support the selected authentication method. "Please change the Authentication method in the Account Settings."
> 
> This is attempting to reflect the exact UI wording. Shall I put it in quotes or
> change the capitalization? i.e.
> Please change the 'Authentication method' in the 'Account Settings'.   or
> Please change the authentication method in the account settings.   ?

I'll leave this up to Bryan.

> 
> > +## @name POP3_INTERNAL_ERROR
> > +## @loc None
> > +4047=Internal state error. This is an internal, unexpected error in
> > the application, please report it as bug.
> 
> > this only seems to be used during authentication, so I think it's better to
> > include that in the error message, and the #define name.
> 
> I meant this as generic error, but I can of course make it specific to auth, if
> you wish.

Specific is better. Generic errors are a royal pain to diagnose.

> 
> > +   * Compatibility note: This attribute had a different meaning in TB < 3.1
> >     */
> >    attribute long authMethod;
> 
> > Why does this mean that a profile touched with 3.1 will be fine with 2.x or
> > 3.0? Isn't this var persisted as a pref? If the meaning is different, that
> > would seem to imply an incompatibility of some sort.
> 
> This note applies only to the IDL attribute, not the pref. Yes, they are
> reflected to a pref, but the old pref is called auth_method, the new one is
> called authMethod. So, no conflict.
Ah, thx!

> 
> +    //do_check_eq(thisTest.expectSuccess,
> rootFolder.containsChildNamed("Inbox")); -- see comment at top
> > We create an inbox when we create the server
> 
> Even if we could never log in??

Yes. As I said, we need something obvious for the user to click on in order to talk to the server, and the Inbox is the natural choice. So we create an Inbox, the user clicks on it, and we go through folder discovery.

> 
> I was looking for a reliable way of checking that the server allows us access
> (or not).
You mean, whether we've logged on or not? nsIImapServerSink (an interface on the imap server object) has an attribute userAuthenticated which you could use.
> 
> > Is this really an interesting thing to log?
> +  PR_LOG(POP3LOGMODULE, PR_LOG_DEBUG, ("new protocol object"));
> 
> No, it's not. :)
Great, thx.
> 
> +          if (m_prefAuthMethods == POP3_HAS_AUTH_NONE)
> +            m_pop3ConData->next_state = POP3_SEND_USERNAME; // what sense does
> that make? we just asked the user for a password (and checked that it's not
> empty)!
> 
> > I'm also unclear on the comment itself.
> > We're about to send a username, not a password, right?
> 
> We do that when the server does *not* need auth. Why ask for a password and
> send a username when we specifically said it's a non-auth server?
> I'd have to look closer at the code again.

I guess the comment is a bit out of context - we asked for the password earlier, right?

> 
> +    // Mailnews caches server capabilities, so try to reset it
> +    deletePop3Server();
> +    incomingServer = createPop3Server();
> 
> > nsIPop3IncomingServer has a handy method to reset capabilities,
> > nsIPop3IncomingServer.pop3CapabilityFlags - did that not work, instead of
> > deleting and recreating the server object?
> 
> I wasn't aware of that.
> Now that I have the delete/recreate code, I wonder whether I should keep it,
> because it's probably safer as test. Not sure yet.
> 

Maybe safer in the long run, unless it turns out that deleting a server and recreating the same server in the same session causes issues :-) But it seems like a more real world test to use the same server, since that's what happens with user profiles, right?

Comment 88

[Ben Bucksch (:BenB)]

> since you attached a new patch, I thought it would have those changes.

I didn't, but it's admittedly easy to get confused, with 20 patches :)

> Sorry, I find those interdiffs really painful to look at

interdiffs are, yes, but hg branches make real, normal diffs, which is a big reason why I like them:
https://hg.mozilla.org/users/mozilla.BenB_bucksch.org/mail-auth/rev/a64f232eeacb
https://hg.mozilla.org/users/mozilla.BenB_bucksch.org/mail-auth/rev/224d06d836ff

In fact, one reason why I created the hg branch was to make it easier for you with the many review rounds, to see what I changed.

But I'll create a new patch, after I'm done with your latest comments, no problem at all ;-).

> Generic errors are a royal pain to diagnose.

Having been victim to NS_ERROR_FAILURE many times, I agree totally ;-).

> it seems like a more real world test to use the same server, since
> that's what happens with user profiles, right?

Yes, that's the idea.

Comment 89

[David :Bienvenu]

+++ b/mailnews/base/public/nsIMsgIncomingServer.idl

this also needs a uuid rev

Comment 90

[Ben Bucksch (:BenB)]

Attachment: Fix, v21

Fixed all review comments apart from the following, which are still open:

1.
feel free to rename AuthFallback method

2.
The smtp server edit dialog looks weird - the username field is right aligned,
and there's no longer room for my whole user name.

3.
> +          if (m_prefAuthMethods == POP3_HAS_AUTH_NONE)
> +            m_pop3ConData->next_state = POP3_SEND_USERNAME; // what sense does
> that make? we just asked the user for a password (and checked that it's not
> empty)!
> 
 > > I'm unclear on the comment
 > > We're about to send a username, not a password, right?
> 
> We do that when the server does *not* need auth. Why ask for a password and
> send a username when we specifically said it's a non-auth server?
> I'd have to look closer at the code again.

4.
> You mean, whether we've logged on or not? nsIImapServerSink (an interface on
> the imap server object) has an attribute userAuthenticated which you could use.

Tried it, doesn't work, returns false in the second round (instead of true as it should). Would need to debug it.

Comment 91

[Ben Bucksch (:BenB)]

Attachment: Fix, v22

Changes:
- Renamed POP3 AuthFallback() to NextAuthStep()
- Aligned pref UI layout
- Talked with bienvenu about POP3_HAS_AUTH_NONE -> POP3_SEND_USERNAME
  (see above), and found out that I misunderstood auth_login = 0
  (for incoming) to mean "no auth", although it means "disable AUTH=*, use
  old-style protocol-native login instead".
  Fixed migration, implemented the option in IMAP and POP.
- Added new (hidden) auth methods options to pref UI, hidden unless selected.
- Removed UI string duplication, using stringbundle instead of DTD.

Comment 92

[Ben Bucksch (:BenB)]

Comment on attachment 430784 [details] [diff] [review]
Fix, v22

On IRC: <bienvenu> I think it's definitely at the point where you should request sr from someone (maybe Neil?) and start banging on the other folks that need to do their reviews

Comment 93

[David :Bienvenu]

Comment on attachment 430784 [details] [diff] [review]
Fix, v22

account settings seem to be broken when I ran with this patch. Not sure why; I'll try popping and pushing and rebuilding.

Comment 94

[David :Bienvenu]

Comment on attachment 430784 [details] [diff] [review]
Fix, v22

account settings definitely broken..

Comment 95

[Ben Bucksch (:BenB)]

Can you be more specific?

Comment 96

[David :Bienvenu]

account settings definitely completely broken. 

They come up blank for me. Backing out the patch fixes the problem; applying it again breaks account settings. Exception must be getting swallowed because I don't see one.

Comment 97

[Ben Bucksch (:BenB)]

Attachment: Fix, v23

Bienvenu, I tested the patch v22 with the current c-c, and the Account Wizard comes up fine for me, small size with 3 fields as expected. Same with this patch. Are you sure there's no problenm in your source tree, e.g. a conflict with some other patch?

Comment 98

[David :Bienvenu]

Comment on attachment 431903 [details] [diff] [review]
Fix, v23

account settings are working with this second patch.

Comment 99

[David :Bienvenu]

Comment on attachment 431903 [details] [diff] [review]
Fix, v23

There's still commented out code in the patch:

-            else
-                // EHLO is always needed if authentication is requested.
-                if (m_prefAuthMethod == PREF_AUTH_ANY)
-                {
-                    m_nextState = SMTP_ERROR_DONE;
-                    m_urlErrorState = NS_ERROR_COULD_NOT_LOGIN_TO_SMTP_SERVER_AUTH_NONE;
-                    return(NS_ERROR_COULD_NOT_LOGIN_TO_SMTP_SERVER);
-                }
+            /*
+            // EHLO is always needed if authentication is requested.
+            // TODO just fall into ProcessAuth and fail there?
+            else if (m_prefAuthMethods > 0)
+            {
+                m_nextState = SMTP_ERROR_DONE;
+                m_urlErrorState = NS_ERROR_SMTP_AUTH_NOT_SUPPORTED;
+                return NS_ERROR_SMTP_AUTH_FAILURE;
+            }*/

Some compilers complain when there's no default case:

+  switch (authMethod)
   {
-    nsCOMPtr<nsISignatureVerifier> verifier = do_GetService(SIGNATURE_VERIFIER_CONTRACTID, &rv);
-    NS_ENSURE_SUCCESS(rv, rv);
+    case nsIMsgIncomingServer::kAuthPasswordEncrypted:
+    case nsIMsgIncomingServer::kAuthSecure:
+    case nsIMsgIncomingServer::kAuthAny:
+      nsCOMPtr<nsISignatureVerifier> verifier =
+          do_GetService(SIGNATURE_VERIFIER_CONTRACTID, &rv);
+      NS_ENSURE_SUCCESS(rv, rv);
+      break;
   }

I think this logging is probably just extra noise:

+  PR_LOG(IMAP, PR_LOG_DEBUG, ("IMAP auth: server caps 0x%X, pref 0x%X, failed 0x%X, avail caps 0x%X",
+        serverCaps, m_prefAuthMethods, m_failedAuthMethods, availCaps));
+  PR_LOG(IMAP, PR_LOG_DEBUG, ("(GSSAPI = 0x%X, CRAM = 0x%X, NTLM = 0x%X, "
+        "MSN =  0x%X, PLAIN = 0x%X, LOGIN = 0x%X, old-style IMAP login = 0x%X)",
+        kHasAuthGssApiCapability, kHasCRAMCapability, kHasAuthNTLMCapability,
+        kHasAuthMSNCapability, kHasAuthPlainCapability, kHasAuthLoginCapability,
+        kHasAuthOldLoginCapability));

This essentially swallows the rv by converting any failure to create digest into NS_ERROR_FAILURE - can you rework that?

There's a more specific error code you can use for this, NS_ERROR_NULL_POINTER at least...

do_queryReferent returns a lovely error code - better to use that than NS_ERROR_FAILURE.
+  nsCOMPtr<nsIMsgIncomingServer> server = do_QueryReferent(m_server);
+  NS_ENSURE_TRUE(server, NS_ERROR_FAILURE);
+  nsresult rv;

+  NS_ENSURE_TRUE(m_imapServerSink, NS_ERROR_FAILURE);
+  NS_ENSURE_TRUE(m_server, NS_ERROR_FAILURE);

           rv = m_imapServerSink->CramMD5Hash(decodedChallenge, password.get(), &digest);
-
         PR_Free(decodedChallenge);
-        if (NS_SUCCEEDED(rv) && digest)
+        NS_ENSURE_TRUE(digest, NS_ERROR_FAILURE);
+        if (NS_SUCCEEDED(rv))

Why swallow rv here:

+      if (NS_FAILED(rv)) // all methods failed
+      {
+        PR_LOG(IMAP, PR_LOG_ERROR, ("huch? there are auth methods, and we resetted failed ones, but ChooseAuthMethod still fails."));
+        return NS_ERROR_FAILURE;
+      }

We use NS_ERROR_ABORT if the user cancels:

+          // We also have to clear the password failed flag, otherwise we'll
+          // automatically try again.
+          ClearFlag(POP3_PASSWORD_FAILED);
+          return NS_ERROR_FAILURE;

It would be nice to fix the indentation of  PRInt32 nsPop3Protocol::SendPassword()
 to use 2 spaces, since you've already changed most of the method. (And Neil will probably make you do it anyway :-) )

r=me, with these addressed.

Comment 100

[Ben Bucksch (:BenB)]

Attachment: Fix, v24

> I think this logging is probably just extra noise

Actually, this line was the most important of all :).

Fixed the other comments.

Here's the change for the error return values:
<https://hg.mozilla.org/users/mozilla.BenB_bucksch.org/mail-auth/rev/9eae64929ab6>

Comment 101

[Ben Bucksch (:BenB)]

Comment on attachment 432283 [details] [diff] [review]
Fix, v24

Also fixed:
- password cancel and empty
- Don't prompt for password for AUTH EXTERNAL in SMTP

Comment 102

[David :Bienvenu]

Comment on attachment 432283 [details] [diff] [review]
Fix, v24

interdiff looks reasonable.

Comment 103

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 432283 [details] [diff] [review]
Fix, v24

>+++ b/mailnews/base/prefs/content/accountcreation/createInBackend.js
>@@ -54,29 +54,28 @@ function createAccountInBackend(config)
>                     .getService(Ci.nsISmtpService);
> 
>   // incoming server
>   var inServer = accountManager.createIncomingServer(
>       config.incoming.username,
>       config.incoming.hostname,
>       sanitize.enum(config.incoming.type, ["pop3", "imap", "nntp"]));
>   inServer.port = config.incoming.port;
>+  inServer.authMethod = config.incoming.auth;
>+  inServer.password = config.incoming.password;

I think this line should already be handled by the line below this comment.
Also, if the user has asked us to not remember the password, we probably shouldn't.  ;)

>   if (config.rememberPassword && config.incoming.password.length)
>     rememberPassword(inServer, config.incoming.password);
[snip…]

>+++ b/mailnews/base/prefs/content/accountcreation/guessConfig.js
>@@ -538,26 +542,26 @@ HostDetector.prototype =
>-        let secureAuth = this._advertisesSecureAuth(type, wiredata);
>+        let authMethods = this._advertisesAuthMethods(type, wiredata); // TODO

What should we be todo-ing here?

>@@ -565,42 +569,70 @@ HostDetector.prototype =
>+  /**
>+   * Which auth mechanism the server claims to support.
>+   * (That doesn't necessarily reflect reality, it is more an upper bound.)
>+   * @returns Array with values for AccountConfig.incoming/outgoing.auth ,
>+   * in decreasing order of preference.
>+   * E.g. [ 5, 4 ] for a server that supports only Kerberos and encrypted passwords.

This line and a couple of others down below are a little longer than 80 characters.

And we could document the protocol and capaResponse parameters, since we have the nice docstring here anyways.  :)

>+   */
>+  _advertisesAuthMethods : function(protocol, capaResponse)
>   {
>+    // for imap, capabilities contain one or more of the following for secure auth:
>     // "AUTH=CRAM-MD5", "AUTH=NTLM", "AUTH=GSSAPI", "AUTH=MSN"
>     // for pop3, the auth mechanisms are returned in capa as the following:

Should we change this phrasing to match the imap?

>+/**
>+ * @param authMethods @see return value of _advertisesAuthMethods()
>+ * @return one of them, the preferred one
>+ * Note: this might be Kerberos, which might not actually work,
>+ * so you might need to try the others, too.
>+ */
>+function chooseBestAuthMethod(authMethods)
>+{
>+  return authMethods[0]; // take first (= most preferred)
>+}

What will you return if there are no supported auth methods?
of if authMethods is null?

>+++ b/mailnews/base/prefs/content/accountcreation/verifyConfig.js
>@@ -219,34 +215,45 @@ urlListener.prototype =
[snip…]
>+    if (this.mConfig.incoming.authAlternatives &&
>+        this.mConfig.incoming.authAlternatives.length > 1)/* &&
>+        (this.mConfig.incoming.authAlternatives[1] > Ci.nsIMsgIncomingServer.kAuthPasswordCleartext || // next best auth is secure
>+         this.mServer.socketType == Ci.nsIMsgIncomingServer.useSSL ||
>+         this.mServer.socketType == Ci.nsIMsgIncomingServer.alwaysUseTLS))*/

This commented out block was really confusing me.

>+      this.mConfig.outgoing.auth = this.mConfig.incoming.auth; // TODO good idea???

I would think not really.  Don't we have a list of authAlternatives for the outgoing config?

Other than that, I can't see any problems with it.

r=me for the stuff under /accountcreation/, with the nits fixed or explained away.  ;)

Later,
Blake.

Comment 104

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 432283 [details] [diff] [review]
Fix, v24

mail/locales/en-US/chrome/messenger/imapMsgs.properties

The new error strings seem more vague than the old ones.  Can we still know if secure auth is the problem and recommend changing that?

I know this new layout will reduce the number of people just checking off secure auth because it looks like a good idea but if we could be specific before it would be nice to continue doing that.  Otherwise things look good.

Comment 105

[Ben Bucksch (:BenB)]

> Can we still know if
> secure auth is the problem and recommend changing that?

There is no such thing as "secure auth" anymore.
I think the new messages are very specific, in that they direct them to an exact UI widget to change.

I could hardcode the case where we have only encrypted passwords enabled and recommend to choose plaintext passwords instead, if that works for you?

Comment 106

[Ben Bucksch (:BenB)]

Some try server builds with patch 24:
<http://s3.mozillamessaging.com/build/try-server/2010-03-16_05:[e-mail]-mailauth5/[e-mail]>
<http://s3.mozillamessaging.com/build/try-server/2010-03-16_05:[e-mail]-mailauth5/[e-mail]>
<http://s3.mozillamessaging.com/build/try-server/2010-03-16_05:[e-mail]-mailauth5/[e-mail]2>
I have not tried these builds myself. (I did try patch v24, though.)

Comment 107

[Ben Bucksch (:BenB)]

Attachment: Fix, v25

Bryan, bienvenu, made the error messages more specific in the encrypted vs. plain password cases.
<http://hg.mozilla.org/users/mozilla.BenB_bucksch.org/mail-auth/rev/0c553b30215e>
(fixed 2 whitespaces issues in a followup commit)

Only other change is to fix smtpEditOverlay.js onLockPreference() to not throw and break the dialog when the mail.smtp.defaultserver pref is not set, as in one of my profiles.

Comment 108

[[e-mail]]

Comment on attachment 433229 [details] [diff] [review]
Fix, v25

>+        socketType == is.useSSL || socketType == is.alwaysUseTLS
>+        ? "authPasswordCleartextViaSSL" : "authPasswordCleartextInsecurely");
My preference is to have the ? at the end of the previous line.

>+    const is = Components.interfaces.nsIMsgIncomingServer;
I don't like that variable name. In fact, I don't think the flags should be defined on nsIMsgIncomingServer, since you use them for outgoing too. Instead, create interfaces (e.g. in MailNewsTypes2.idl) especially to name flags. In your case, I would create nsMsgAuthMethod and nsMsgSocketType interfaces.

>+# LOCALIZATION NOTE (5102): %S is the server address
>+5104=The Kerberos/GSSAPI ticket was not accepted by the IMAP server %S. Please check that you are logged in to the Kerberos/GSSAPI realm.
Oops ;-)

>+function MigrateServerAuthPref()
>+{
I have to say I'm not keen on this approach. Would it be possible to write nsSmtpServer/nsMsgIncomingServer::GetAuthMethod to do this instead?

Comment 109

[Ben Bucksch (:BenB)]

>+        socketType == is.useSSL || socketType == is.alwaysUseTLS
>+        ? "authPasswordCleartextViaSSL" : "authPasswordCleartextInsecurely");
> My preference is to have the ? at the end of the previous line.

I think it's a lot more readable as-is, but OK.

> I don't think the flags should be
> defined on nsIMsgIncomingServer, since you use them for outgoing too. Instead,
> create interfaces (e.g. in MailNewsTypes2.idl) especially to name flags. In
> your case, I would create nsMsgAuthMethod and nsMsgSocketType interfaces.

That's a rather big change (need to change all the existing places which use the socket constants), but I can see the point. Given that you didn't ask for much else, I'll try to do that.

> Would it be possible to write
> nsSmtpServer/nsMsgIncomingServer::GetAuthMethod to do this instead?

Hard.
A) it's a different language, so I have to rewrite the whole function.
B) There are other migration functions right next to mine, I just added this one. I also think that migration should be done in JS, not C++. I think mid-term we should have a migration.js or .jsm, which is shared by TB and SM and contains all migration. I do *not* want that migration scattered all over the code.
C) it would probably run only when the user uses the server. I want to migrate the pref on first start immediately, because the old one is really deprecated. I don't want to have a mix and match of half the accounts being migrated and the other not. Likely, the UI would fail in that case as well.
So, I think it should stay as-is.

Comment 110

[[e-mail]]

(In reply to comment #109)
> > I don't think the flags should be
> > defined on nsIMsgIncomingServer, since you use them for outgoing too. Instead,
> > create interfaces (e.g. in MailNewsTypes2.idl) especially to name flags. In
> > your case, I would create nsMsgAuthMethod and nsMsgSocketType interfaces.
> That's a rather big change (need to change all the existing places which use
> the socket constants), but I can see the point. Given that you didn't ask for
> much else, I'll try to do that.
Maybe we can hold off on changing the existing places for now (or make nsIMsgIncomingServer inherit from nsMsgSocketType).

I since noticed you used a number of different names for your shorthand for Components.interfaces.nsIMsgIncomingServer, which was also confusing.

> > Would it be possible to write
> > nsSmtpServer/nsMsgIncomingServer::GetAuthMethod to do this instead?
> C) it would probably run only when the user uses the server. I want to migrate
> the pref on first start immediately, because the old one is really deprecated.
Unfortunately msgMail3PaneWindow.js does not count as first start for SeaMonkey - we can compose messages and check for new mail without opening the 3pane.

Comment 111

[Ben Bucksch (:BenB)]

Neil said:
> create nsMsgAuthMethod and nsMsgSocketType interfaces.

Done.
<https://hg.mozilla.org/users/mozilla.BenB_bucksch.org/mail-auth/rev/21609382d9c9>
Another big, huge change (2600 lines this alone).

Also fixed the number in the locale, and the style nits.

> we should have a migration.js or .jsm, which is shared by TB and SM
> and contains all migration.

Per discussion with Neil, will create migration.jsm.
He said I can hook up during SM start, in nsSuiteGlue.js' _init().

Comment 112

[Ben Bucksch (:BenB)]

Attachment: Fix, v26

- Added migration.jsm
- Ported locale changes (based on clarkbw's request) to Seamonkey
- Other small fixes

Tested both Seamonkey and migration, and both work fine.

I had one problem in Seamonkey in one profile where the smtp edit dialog came up with 0 size, but it was fine after I resized it, and it didn't happen in the other profile, so I think it was a localstore.rdf problem.

Comment 113

[[e-mail]]

(In reply to comment #109)
>>+        socketType == is.useSSL || socketType == is.alwaysUseTLS
>>+        ? "authPasswordCleartextViaSSL" : "authPasswordCleartextInsecurely");
>>My preference is to have the ? at the end of the previous line.
>I think it's a lot more readable as-is, but OK.
This wasn't a dig at this one particular line. My understanding is that the current coding style guide suggests putting operators at the end of the line in all cases. So you should really do this whenever you split a ?: line.

I tried the patch out and I did find a niggle when dealing with those menulists whose options vary dynamically - this makes their width change. In particular if you open SMTP settings and subsequently change the connection security from STARTTLS or SSL to none then the menulist is now the wrong width. I couldn't find a good way to fix this that didn't assume that the insecure label is wider.

Comment 114

[Ben Bucksch (:BenB)]

> operators at the end of the line
> you should really do this whenever you split a ?: line.

I see the idea in general, and have done that in most cases, esp. with +, but I take the liberty to derive from it when I think it increases readability in a specific case. For example, I think that

+          auth_login
+            ? (useSecAuth
+               ? Ci.nsMsgAuthMethod.secure
+               : Ci.nsMsgAuthMethod.passwordCleartext)
+            : Ci.nsMsgAuthMethod.old);

is a lot more readable than

+          auth_login ? (useSecAuth ? Ci.nsMsgAuthMethod.secure
+          : Ci.nsMsgAuthMethod.passwordCleartext) : Ci.nsMsgAuthMethod.old);

> I did find a niggle when dealing with those menulists
> whose options vary dynamically - this makes their width change.
> In particular if you open SMTP settings and subsequently change the
> connection security from STARTTLS or SSL to none then the menulist is
> now the wrong width. I couldn't find a good way to fix this that
> didn't assume that the insecure label is wider.

I see what you mean... I didn't think the variable size is a problem, but yes, the label being cut off (then with "Password, transmit..." or somesuch) is not terribly nice. I'll see whether I have an idea.

The patch is otherwise OK?

Comment 115

[[e-mail]]

(In reply to comment #114)
> +          auth_login ? (useSecAuth ? Ci.nsMsgAuthMethod.secure
> +          : Ci.nsMsgAuthMethod.passwordCleartext) : Ci.nsMsgAuthMethod.old);
That's still wrong, since you have a : at the start of the line ;-) Try
auth_login ? (useSecAuth ? Ci.nsMsgAuthMethod.secure :
                           Ci.nsMsgAuthMethod.passwordCleartext) :
             Ci.nsMsgAuthMethod.old);

> The patch is otherwise OK?
Sure.

Comment 116

[Bryan Clark (DevTools PM) [:clarkbw]]

Thanks for the more specific messages Ben!  My only concern is this string: IMAP_AUTH_CHANGE_ENCRYPT_TO_PLAIN_NO_SSL

The IMAP server %S apparently does not support encrypted passwords. If you have just set up this account, please try changing to 'Password, transmitted insecurely' as the 'Authentication method' in the 'Account Settings | Server settings'. If it used to work and now suddenly fails, please contact your email administrator or provider. You might also be under attack. Note that disabling password encryption would mean that your password is transfered entirely unprotected over the Internet or network.


Here's how I'd present the goals of this message in terms of informing the user (in order of importance):
 * Encrypted passwords are not supported
 * You should contact your email administrator for help with these settings
 * Here is a setting to try

The first point seems fairly easy to communicate, I might change your first sentence to this.

"The IMAP server %S does not seem to support encrypted passwords."

The second point also seems like a fairly straightforward point.  I've made this the second point because it is much clearer than the last one which is vague and scary.

"You should contact your email administrator for help with changing these settings in the 'Account Settings | Server settings' to the 'Authentication method', 'Password'."

The "if this is a new account" would really be better if we actually knew that it was a new account.  Otherwise it seems like we should just be pointing people at a KB article which can help them figure out the right settings.

I don't think we can use the sentence "You might also be under attack" as most people would only understand "attack" in the physical sense and get very upset.

The last note seems like something we took care of in the settings chooser by using the "transmitted insecurely" text if they aren't already on SSL.

So my full text would be this:

"The IMAP server %S does not seem to support encrypted passwords.  You should contact your email administrator for help with changing these settings in the 'Account Settings | Server settings' to the 'Authentication method', 'Password'."

Comment 117

[Ben Bucksch (:BenB)]

Per discussion on IRC with Bryan, we'll use the following:
The IMAP server %S does not seem to support encrypted passwords. If you just set up the account, please try changing to 'Password, transmitted insecurely' as the 'Authentication method' in the 'Account Settings | Server settings'. If it used to work and now suddenly fails, this is a common scenario how someone could steal your password.

Comment 118

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 433503 [details] [diff] [review]
Fix, v26

Looks good, with the discussed change string.

"The IMAP server %S does not seem to support encrypted passwords. If you just
set up the account, please try changing to 'Password, transmitted insecurely'
as the 'Authentication method' in the 'Account Settings | Server settings'. If
it used to work and now suddenly fails, this is a common scenario how someone
could steal your password."

Comment 119

[[e-mail]]

Comment on attachment 433503 [details] [diff] [review]
Fix, v26

Well if you can't immediately come up with a fix, at least file a bug on the size truncation.

sr=me with some nicer ?:s

Comment 120

[Ben Bucksch (:BenB)]

Thanks a lot Neil for the quick sr.

So, after a very painful 3 months of review, finally, this is in. Can't believe it. Phew!

<http://hg.mozilla.org/comm-central/rev/895928822eb3>

Comment 121

[Ben Bucksch (:BenB)]

Neil wrote in comment 113:
> I did find a niggle when dealing with those menulists
> whose options vary dynamically - this makes their width change.
> In particular if you open SMTP settings and subsequently change the
> connection security from STARTTLS or SSL to none then the menulist is
> now the wrong width. I couldn't find a good way to fix this that
> didn't assume that the insecure label is wider.
and in comment 119:
> Well if you can't immediately come up with a fix, at least file a bug on the
> size truncation.

Filed bug 553757 as requested.

----

Any problems you find with this, please file new bugs and post the bug number here. This bug is long enough as-is. Thanks!

Everybody: Please test authentication carefully. Behavior (esp. in error case) changed a bit, but it should of course work.

Comment 122

[Ben Bucksch (:BenB)]

Bug 553764: test_imapAuthMethods.js fails on Mac (works on Win and Linux)

Comment 123

[Nikolay Shopik]

Thanks Ben, much appreciated! Since this fix I have no more UI lockup during sending email. This is due using "secure authentication" it was always try to initialize Kerberos even if it not need.

Comment 124

[Simon Paquet [:sipaq]]

You're changing the semantic meaning for several strings in the following files:

- mail/locales/en-US/chrome/messenger/imapMsgs.properties
  - 5102
  - 5109
- mail/locales/en-US/chrome/messenger/localMsgs.properties
  - 4030
- mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
  - 12579
  - 12580
  - 12581

You're thereby ensuring that many/most of our localizers will not notice the changes and that therefore your suggested improvements will not make it to approx. 50% of our userbase.

I therefore suggest that you either provide a followup patch ASAP so that this can get in before the string freeze or back this out immediately. This also applies to SeaMonkey by the way, as you're making the same changes there.

I'd also suggest that reviewers and super-reviewers add this item to their 
internal review checklist. I'm also CC'ing the relevant TB release drivers to make them aware of this.

Comment 125

[Ben Bucksch (:BenB)]

I had explicitly asked to file followup bugs in comment 121. Re-closing.

Comment 126

[Ben Bucksch (:BenB)]

I forgot to respond to bwinton in comment 103.

>+  inServer.password = config.incoming.password;
>   if (config.rememberPassword && config.incoming.password.length)
>     rememberPassword(inServer, config.incoming.password);

> I think this [inServer.password = ]line should already be handled by the
> line below this comment. Also, if the user has asked us to not remember
> the password, we probably shouldn't.  ;)

We need |inServer.password =| line, because we need the password for the login verification we do in the account creation wizard at the end.
However, that does *not* store the password.

You can try that yourself:
- clear password manager (in prefs)
- set up test account, with [ ] remember password unchecked
- wizard will verify the password (against the server) OK
- after the wizard, you can open the account, without password dialog
- after a restart of TB, you get a password dialog for that account.
That's exactly as expected.

In terms of code, the server object only stores the password in object variables (RAM), .password property = SetPassword() setter doesn't store it in the password manager. Only the password *dialog* (which is called by the server object when it needs the password and it's not there) will store in the password manager.
nsImapIncomingServer.cpp just uses generic functions from nsMsgIncomingServer and:
nsMsgIncomingServer::SetPassword() { m_password = aPassword; return NS_OK; } 
If you search for m_password in nsMsgIncomingServer.cpp, you see that the only code which uses m_password is code that *sets* it, e.g. in the GetPasswordWith[out]UI() and Forget...() functions. There's no code that saves the password, only the generic promptPassword dialog does that.