546278 - [autoconfig] variables in hostnames are not replaced

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Kohei Yoshino]

+++ This bug was initially created as a clone of Bug #534604 +++

I can't believe I didn't notice until now, but variables in the <hostname>s are not replaced with appropriate values.

Think about <hostname>%EMAILDOMAIN%</hostname>

http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/accountConfig.js#194

>   account.incoming.hostname =
>     sanitize.hostname(_replaceVariable(account.incoming.hostname,

If the incoming/outgoing hostnames are taken from XML, they are already sanitized/decapitalized as %emaildomain%:

http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/readFromXML.js#74

As the variables should be uppercase, _replaceVariable() doesn't work for the hostnames, and the final hostnames are set as %emaildomain%.

This affects the following configurations:

https://live.mozillamessaging.com/autoconfig/janis.or.jp
https://live.mozillamessaging.com/autoconfig/mx1.tiki.ne.jp
https://live.mozillamessaging.com/autoconfig/pop.shibata.ne.jp

Patch will follow...

Comment 1

[Kohei Yoshino]

Attachment: patch v1

Comment 2

[Ben Bucksch (:BenB)]

r=BenB

Comment 3

[Ben Bucksch (:BenB)]

(nice idea, the way you fixed it)

Comment 4

[Kohei Yoshino]

Attachment: screenshot

Comment 5

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

Comment on attachment 426972 [details] [diff] [review]
patch v1

So, I like the fix, but it's missing tests, and we are requiring them for new patches.

(For a while, I thought that it could also lead to uppercase characters in the hostname, if they weren't replaced, or if they were entered as uppercase.  A test would have convinced me that that couldn't happen much faster than me having to dig through the source code. ;)

On the plus side, there are already some xpcshell tests for the autoconfig in mailnews/base/test/unit/test_autoconfigUtils.js and it already loads up the accountConfig.js file, and a test for this fix doesn't need to do any UI manipulation, so all you need to do is add some more tests in the:
  if (xmlReader)
block in the run_test function at the bottom, and it should be good to go.

And I guess, while I'm asking for stuff, could we change the comment to:
With the variables in mind, capitalize the hostnames which will then be sanitized and decapitalized. (Bug 546278)
so that I will realize that the sanitization will decapitalize them the next time I'm reading the code?

Thanks,
Blake.

Comment 6

[Kohei Yoshino]

Attachment: patch v2 (test added)

I have not tested it yet, but added a xpcshell-based testcase and changed the comment in replaceVariables().

Comment 7

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

Comment on attachment 428722 [details] [diff] [review]
patch v2 (test added)

I mostly love this patch, but have a couple of small nits.

>+++ b/mailnews/base/test/unit/test_autoconfigUtils.js
>@@ -346,21 +346,72 @@ function test_copying_readFromXML()
>+function test_replaceVariables()

We could use a docstring for this function.

>+{
>+  let clientConfigXML = new XML(
>+    '<clientConfig>' +
>+    '  <emailProvider id="inbox.lv">' +

We should probably use example.com or mozilla.invalid here, instead of
using a real domain.

>+  let config = {
>+    "incoming server username": { actual: account.incoming.username, expected: "yamato.nadeshiko" },
>+    "outgoing server username": { actual: account.outgoing.username, expected: "[e-mail]" },
>+    "incoming server hostname": { actual: account.incoming.hostname, expected: "pop.inbox.lv" },
>+    "outgoing server hostname": { actual: account.outgoing.hostname, expected: "smtp.inbox.lv" },
>+    "user real name": { actual: account.identity.realname, expected: "Yamato Nadeshiko" },
>+    "user email address": { actual: account.identity.emailAddress, expected: "[e-mail]" }
>+  }
>+
>+  for (let i in config) {
>+    assert_equal(config[i].actual, config[i].expected,
>+      "Configured " + i + " is " + config[i].actual + ". It should be " + config[i].expected + ".");
>+  }

In the other tests, I've added some helper functions, like assert_equal_try_orders and assert_equal_try_orders, which would print the correct message, then I could just call them serially, instead of looping through an object.  I feel that it's a little clearer and more consistent with the rest of the file, and so I would like you to change that code to something more like:
/**
 * Test that two config entries are the same.
 */
function assert_equal_config(aA, aB, field)
{
  assert_equal(aA, aB, "Configured " + i + " is incorrect.");
};

# …
  assert_equal_config(account.incoming.username, "yamato.nadeshiko",
                      "incoming server username");
  assert_equal_config(account.outgoing.username, "[e-mail]",
                      "outgoing server username");
  assert_equal_config(account.incoming.hostname, "pop.inbox.lv",
                      "incoming server hostname");
  assert_equal_config(account.outgoing.hostname, "smtp.inbox.lv",
                      "outgoing server hostname");
  assert_equal_config(account.identity.realname, "Yamato Nadeshiko",
                      "user real name");
  assert_equal_config(account.identity.emailAddress, "[e-mail]",
                      "user email address");
Possibly with extra line breaks to keep it under 80 characters.

So, r=me with those nits fixed, and since it's mailnews code, you'll need a superreview from someone.

Thanks,
Blake.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 428722 [details] [diff] [review]
patch v2 (test added)

sr=dmose for the mailnews bits (note that I haven't explicitly tested, however)

Comment 9

[Ludovic Hirlimann [:Usul]]

Kohei Yoshino : ping !

Comment 10

[Kohei Yoshino]

Sorry for the super delay... I'll look into the patch this weekend.

Comment 11

[Ben Bucksch (:BenB)]

Kohei, wait a bit, this is conflicting with bug 549045 and may no longer be needed with that patch.

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

Unfortunately, as much as I'd like to, I believe the risk profile around bug 549045 is such that we can't take it for 3.1.  So, Kohei, please go ahead with this patch.  Thanks!

Comment 13

[Kohei Yoshino]

Attachment: patch v3

Comment 14

[Ben Bucksch (:BenB)]

Please add the test to test_autoconfigXML.js instead.

Comment 15

[Ben Bucksch (:BenB)]

(and see there how to use E4X XML literals)

Comment 16

[Kohei Yoshino]

Attachment: patch v4

Added the test to test_autoconfigXML.js.

Comment 17

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 438485 [details] [diff] [review]
patch v4

Carrying forward r+ from previous patch, sr=dmose.

Comment 18

[Dan Mosedale (:dmosedale, :dmose)]

Pushed: <http://hg.mozilla.org/comm-central/rev/f5908b6ebd28>.

Thanks for the fix, Kohei!

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

I have no idea whether this is likely to be effecting our conversion funnel in an interesting way (maybe there's some way to figure that out by grepping the ISP database & logs), but after this bakes on 3.1, we could consider taking this for 3.0.next.  Nominating.

Comment 20

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

I would suggest we don't take this patch for two reasons.

1) This patch mainly enables us to roll a bunch of similar configs into one.  (i.e. if foo.com, bar.com, and baz.com are all run by the same people, but the mail servers are mail.foo.com, mail.bar.com, and mail.baz.com, we used to need three entries in the autoconfig database.  With this patch, we only need one entry, but it doesn't add any extra benefits for the client.)

2) We'll still need to support 3.0.0-3.0.4 clients in the autoconfig (using the script from bug 557401), so we can't actually take advantage of this in the configs we server to TB 3.0.

The main effect of this patch will be to make our ISP config reviewing/maintaining process much easier.

Later,
Blake.

Comment 21

[Dan Mosedale (:dmosedale, :dmose)]

Sold; thanks, Blake!