Closed Bug 546278 Opened 10 years ago Closed 10 years ago

[autoconfig] variables in hostnames are not replaced

Categories

(Thunderbird :: Account Manager, defect)

defect
Not set

Tracking

(blocking-thunderbird3.1 beta2+, thunderbird3.1 beta2-fixed, blocking-thunderbird3.0 -, thunderbird3.0 wontfix)

RESOLVED FIXED
Tracking Status
blocking-thunderbird3.1 --- beta2+
thunderbird3.1 --- beta2-fixed
blocking-thunderbird3.0 --- -
thunderbird3.0 --- wontfix

People

(Reporter: kohei, Assigned: kohei)

References

Details

Attachments

(2 files, 3 obsolete files)

+++ 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...
Attached patch patch v1 (obsolete) — Splinter Review
r=BenB
(nice idea, the way you fixed it)
Attached image screenshot
Attachment #426972 - Flags: review+
Attachment #426972 - Flags: review?(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.
Attachment #426972 - Flags: review?(bwinton) → review-
Whiteboard: [needs unit test]
blocking-thunderbird3.1: beta1+ → beta2+
Attached patch patch v2 (test added) (obsolete) — Splinter Review
I have not tested it yet, but added a xpcshell-based testcase and changed the comment in replaceVariables().
Attachment #426972 - Attachment is obsolete: true
Attachment #428722 - Flags: review?(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: "yamato.nadeshiko@inbox.lv" },
>+    "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: "yamato.nadeshiko@inbox.lv" }
>+  }
>+
>+  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, "yamato.nadeshiko@inbox.lv",
                      "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, "yamato.nadeshiko@inbox.lv",
                      "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.
Attachment #428722 - Flags: review?(bwinton) → review+
Whiteboard: [needs unit test] → [patch up, needs sr]
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)
Attachment #428722 - Flags: superreview+
Assignee: nobody → kohei.yoshino.bugs
Whiteboard: [patch up, needs sr] → [needs updated patch with review nits fixed]
Kohei Yoshino : ping !
Sorry for the super delay... I'll look into the patch this weekend.
Kohei, wait a bit, this is conflicting with bug 549045 and may no longer be needed with that patch.
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!
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #438472 - Flags: superreview?(dmose)
Attachment #438472 - Flags: review?(bwinton)
Attachment #428722 - Attachment is obsolete: true
Please add the test to test_autoconfigXML.js instead.
(and see there how to use E4X XML literals)
Attached patch patch v4Splinter Review
Added the test to test_autoconfigXML.js.
Attachment #438472 - Attachment is obsolete: true
Attachment #438472 - Flags: superreview?(dmose)
Attachment #438472 - Flags: review?(bwinton)
Comment on attachment 438485 [details] [diff] [review]
patch v4

Carrying forward r+ from previous patch, sr=dmose.
Attachment #438485 - Flags: superreview+
Attachment #438485 - Flags: review+
Pushed: <http://hg.mozilla.org/comm-central/rev/f5908b6ebd28>.

Thanks for the fix, Kohei!
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [needs updated patch with review nits fixed]
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.
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.
Depends on: 568153
You need to log in before you can comment on or make changes to this bug.