558793 - bwinton's review of auth pref change bug 525238

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Ben Bucksch (:BenB)]

This is a followup to bug 525238 comment 103, which I forgot to address.

Comment 1

[Ben Bucksch (:BenB)]

Bug 525238 comment 103 From Blake Winton (:bwinton) 2010-03-16 18:53:16 PDT

> (From update of attachment 432283 [details] [diff] [review])
> >+++ 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);

Answered in bug 525238 comment 126. Copy:

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.

> >+++ 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?

Solved in my parallel guessConfig bug 549040.

> >@@ -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.  :)

Solved in my parallel guessConfig bug 549040.

> >+   */
> >+  _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?

Solved in my parallel guessConfig bug 549040.

> >+/**
> >+ * @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?

Solved in attached patch.

> >+++ 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.

Solved in attached patch.

> >+      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?

Better comment in attached patch.

> 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.  ;)

Comment 2

[Ben Bucksch (:BenB)]

Attachment: Fix, v1

This fixes the above things. Most changes were comments only.

I found, while testing, 2 other bugs:
- The SMTP vs. "smtp" means that the [Re-test] button is broken
  (will always report fail).
- We should prefer CRAM-MD54 over NTLM, because CRAM is more secure

Esp. because of the "smtp"-issue, let's please get this in quickly (before beta2 in any case).

Comment 3

[Ben Bucksch (:BenB)]

Attachment: Fix, v2

Better SMTP auth fallback

Comment 4

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

Comment on attachment 438506 [details] [diff] [review]
Fix, v2

>+++ b/mailnews/base/prefs/content/accountcreation/accountConfig.js
>@@ -89,30 +89,42 @@ AccountConfig.prototype =
>     return {
>       type : null, // string-enum: "pop3", "imap", "nntp"
>       hostname : null,
>       port : null, // Integer
>       username : null, // String. May be a placeholder (starts and ends with %).
>       password : null,
>       // enum: 1 = plain, 2 = SSL/TLS, 3 = STARTTLS always
>       // ('TLS when available' is insecure and not supported here)
>-      socketType : null,
>+      socketType : 0,

We should probably document the 0 value for the enum.

>+      /**
>+       * {Array of Ci.nsMsgAuthMethod} (same as .auth)
>+       * Other auth methods that we think the server supports.
>+       * They are ordered by descreasing preference.
>+       *
>+       * (Note: unlike |.incomingAlternatives|, this array also includes
>+       * the current auth method |.auth| as first element, while
>+       * |.incomingAlternatives| does not include |.incoming|. Adapt
>+       * |.authAlternatives| to have the first element removed.)

What do you think about making authAlternatives the same as incomingAlternatives, or making incomingAlternatives the same as authAlternatives?

>+++ b/mailnews/base/prefs/content/accountcreation/verifyConfig.js
>@@ -209,46 +209,55 @@ urlListener.prototype =
>+      // Assume that SMTP server has same methods working as incoming.
>+      // Broken assumption, but we currently have no SMTP verification.
>+      // TODO implement real SMTP verification
>+      if (this.mConfig.outgoing.auth == brokenAuth &&
>+          this.mConfig.outgoing.authAlternatives.indexOf(
>+              this.mConfig.incoming.auth) != -1)

I think we can indent this by two spaces less.

>+        this.mConfig.outgoing.auth = this.mConfig.incoming.auth;
>+      this._log.info("  outgoing auth: " + this.mConfig.outgoing.auth);

r=me with nits fixed this time.  ;)

Oh, and be sure to set the in-testsuite flag appropriately.

Thanks,
Blake.

Comment 5

[Ben Bucksch (:BenB)]

Attachment: Fix, v3

- Nits fixed
- |authAlternatives| no longer contains selected |auth|

Comment 6

[Ben Bucksch (:BenB)]

Commited as <http://hg.mozilla.org/comm-central/rev/e2ddbff77613>
Thanks for the fast review.

FIXED