1460757 - display of mail server authentication methods in about:troubleshooting seems off and missing some values

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, enhancement)

Description

[:aceman]

In about:troubleshooting page we display the authentication method used for each account configured in TB.

The code in mail/components/about-support/content/accounts.js displays these strings according to the authMethod pref for the account:
// l10n properties in messenger.properties corresponding to each auth method
var gAuthMethodProperties = {
  "1": "authOld",
  "2": "authPasswordCleartextInsecurely",
  "3": "authPasswordCleartextViaSSL",
  "4": "authPasswordEncrypted",
  "5": "authKerberos",
  "6": "authNTLM",
  "8": "authAnySecure"
};

The strings for these IDs are in messenger.properties:
# Account Settings - Both Incoming and SMTP server
authNo=No authentication
authOld=Password, original method (insecure)
authPasswordCleartextInsecurely=Password, transmitted insecurely
authPasswordCleartextViaSSL=Normal password
authPasswordEncrypted=Encrypted password
authKerberos=Kerberos / GSSAPI
authExternal=TLS Certificate
authNTLM=NTLM
authOAuth2=OAuth2
authAnySecure=Any secure method (deprecated)
authAny=Any method (insecure)

While the allowed constants for the pref ar defined in mailnews/base/public/MailNewsTypes2.idl and have a bit more values:
interface nsMsgAuthMethod {
    // 0 is intentionally undefined and invalid
    /// No login needed. E.g. IP-address-based.
    const nsMsgAuthMethodValue none = 1;
    /// Do not use AUTH commands (e.g. AUTH=PLAIN),
    /// but the original login commands that the protocol specified
    /// (POP: "USER"/"PASS", IMAP: "login", not valid for SMTP)
    const nsMsgAuthMethodValue old = 2;
    /// password in the clear. AUTH=PLAIN/LOGIN or old-style login.
    const nsMsgAuthMethodValue passwordCleartext = 3;
    /// hashed password. CRAM-MD5, DIGEST-MD5
    const nsMsgAuthMethodValue passwordEncrypted = 4;
    /// Kerberos / GSSAPI (Unix single-signon)
    const nsMsgAuthMethodValue GSSAPI = 5;
    /// NTLM is a Windows single-singon scheme.
    /// Includes MSN / Passport.net, which is the same with a different name.
    const nsMsgAuthMethodValue NTLM = 6;
    /// Auth External is cert-based authentication
    const nsMsgAuthMethodValue External = 7;
    /// Encrypted password or Kerberos / GSSAPI or NTLM.
    /// @deprecated - for migration only.
    const nsMsgAuthMethodValue secure = 8;
    /// 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 nsMsgAuthMethodValue anything = 9;

    /// Use OAuth2 to authenticate.
    const nsMsgAuthMethodValue OAuth2 = 10;
}

So e.g. OAuth2 or External aren't in the list of strings, so they would be translated in about:troubleshooting. They would show in English (e.g. External instead of the "TLS certificate" translation).
It is possible to set (or at least preserve) these values in the Account manager -> Server settings.

Also the mapping of the authMethod constants in accounts.js to strings seems to be off, compared to e.g. 1 maps to authOld, while that should be 2 per nsMsgAuthMethod .

Also compare to mailnews/base/prefs/content/server.xul:
 <menuitem id="authMethod-no" value="1"/>
 <menuitem id="authMethod-old" value="2"/>
 <menuitem id="authMethod-password-cleartext" value="3"/>
 <menuitem id="authMethod-password-encrypted" value="4"/>
 <menuitem id="authMethod-kerberos" value="5"/>
 <menuitem id="authMethod-ntlm" value="6"/>
 <menuitem id="authMethod-external" value="7"/>
 <menuitem id="authMethod-oauth2" value="10"/>
 <menuitem id="authMethod-anysecure" value="8"/>
 <menuitem id="authMethod-any" value="9"/>

And see in mailnews/base/prefs/content/server.xul how those items get labeled:
  setLabelFromStringBundle("authMethod-no", "authNo");
  setLabelFromStringBundle("authMethod-old", "authOld");
  setLabelFromStringBundle("authMethod-kerberos", "authKerberos");
  setLabelFromStringBundle("authMethod-external", "authExternal");
  setLabelFromStringBundle("authMethod-ntlm", "authNTLM");
  setLabelFromStringBundle("authMethod-oauth2", "authOAuth2");
  setLabelFromStringBundle("authMethod-anysecure", "authAnySecure");
  setLabelFromStringBundle("authMethod-any", "authAny");
  setLabelFromStringBundle("authMethod-password-encrypted",
      "authPasswordEncrypted");
  //authMethod-password-cleartext already set in selectSelect()
    
  // Hide deprecated/hidden auth options, unless selected
  hideUnlessSelected(document.getElementById("authMethod-no"));
  hideUnlessSelected(document.getElementById("authMethod-old"));
  hideUnlessSelected(document.getElementById("authMethod-anysecure"));
  hideUnlessSelected(document.getElementById("authMethod-any"));

  // switch "insecure password" label
  setLabelFromStringBundle("authMethod-password-cleartext",
    socketType == Ci.nsMsgSocketType.SSL ||
    socketType == Ci.nsMsgSocketType.alwaysSTARTTLS ?
    "authPasswordCleartextViaSSL" : "authPasswordCleartextInsecurely");

Comment 1

[:aceman]

Does anyone know if the mixups or non-translations may be intentional?

Comment 2

[:aceman]

Attachment: 1460757.patch

In case this is not intentional, this would be the fix.
You can test this by setting the pref manually in prefs.js: user_pref("mail.server.server5.authMethod", x); for x=1..10 .
Then compare the display of the auth method name in Account settings -> Server settings and the Help-> Troubleshooting info page

Comment 3

[:aceman]

Attachment: 1460757.patch v1.1

Some accounts (e.g. Chat) report a value of authMethod=0 so I added it in the patch. The 0 (zero) was shown verbatim, so I guess we could display the "No authentication" string so that it does not look as an error.

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 8975264 [details] [diff] [review]
1460757.patch v1.1

Review of attachment 8975264 [details] [diff] [review]:
-----------------------------------------------------------------

This looks ok to me, r=mkmelin

::: mail/components/about-support/content/accounts.js
@@ +148,4 @@
>      }
>      else {
>        prettyAuthMethod = plainAuthMethod;
> +      Cu.reportError("Missing translation for authMethod: " + plainAuthMethod);

please remove this, it's also not correct (it's about missing mapping, you never end up trying to get the translation)

Comment 5

[Magnus Melin [:mkmelin]]

(In reply to :aceman from comment #1)
> Does anyone know if the mixups or non-translations may be intentional?

I doubt it.

Comment 6

[:aceman]

Attachment: 1460757.patch v1.2

Thanks.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/39187814fb14
use constants from interfaces instead of hardcoding them where possible. r=mkmelin, ui-r=Paenglab DONTBUILD

Comment 8

[:aceman]

The commit message wasn't as great here:( I think the patch was spun off from other bug and I didn't update the message.

Comment 9

[Jorg K (CEST = GMT+2)]

Well, the bug had checkin-needed. I always check commit messages, but I couldn't think of anything better. We could have added "... in about:support/accounts" or so.