534604 - [autoconfig] username for outgoing server (SMTP) is not set properly if it is different from the one for incoming server (POP/IMAP)

Status: RESOLVED FIXED

(Thunderbird :: Account Manager, defect)

Description

[Kohei Yoshino [:kohei]]

If the username format for outgoing server (SMTP) is different from the one for incoming server (POP/IMAP), the outgoing username is not set properly -- the incoming username will be used instead.

This bug affects the tiki.ne.jp servers:
https://live.mozillamessaging.com/autoconfig/mx1.tiki.ne.jp

Steps to reproduce:
Input [e-mail] as an username

Actual results:
The incoming username: test
The outgoing username: test

Expected results:
The incoming username: test
The outgoing username: [e-mail]

Comment 1

[Kohei Yoshino [:kohei]]

Attachment: patch v1

Comment 2

[Ben Bucksch (:BenB)]

We're not really set up to support that, esp. in the guess code and the login
verify code.

Did you try whether EMAILLOCALPART works for outgoing, and whether full
EMAILADDRESS works for incoming? If so, please configure that. Otherwise,
please contact the ISP to fix their setup to allow the same username for both.

Comment 3

[u217243]

(In reply to comment #2)
> Did you try whether EMAILLOCALPART works for outgoing, and whether full
> EMAILADDRESS works for incoming? If so, please configure that. Otherwise,
> please contact the ISP to fix their setup to allow the same username for both.

The ISP, tiki.ne.jp, said they only accepted EMAILLOCALPART for incoming and EMAILADDRESS for outgoing. hal.ne.jp also has the same problem -- EMAILADDRESS for incoming and EMAILLOCALPART for outgoing.

I think this is obviously Thunderbird's bug. The patch works for me, but as you pointed out, further investigation is required.

Comment 4

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

I think this is sort of important, since as Ben mentioned, the guess code won't currently handle this case, and so handling appropriately-formatted server configs is the only way we'll be able to give users the correct values in the autoconfig.

While it's true that we will only verify the incoming login and not the outgoing login, I don't think that that's as serious a problem, and I think that that could be handled in a future patch or new bug.

Thanks,
Blake.

Comment 5

[mailtrader]

Another case:
In my organisation localpart is not used as username (we use domain logon instead), so using autoconfig is quite difficult without "magic 'get from ldap samaccountname attribute autoconfig' script". If I could use %username% windows env variable, that would be great.

Comment 6

[rsx11m]

(In reply to comment #4)
> 
> While it's true that we will only verify the incoming login and not the
> outgoing login, I don't think that that's as serious a problem, and I think
> that that could be handled in a future patch or new bug.

It would be a good idea to extend the authentication probing to the outgoing server as well. This would catch the case here, but also cases where no user name is required at all (currently a subpart of bug 522633 migration).

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

It would be really great if we could get this for beta 1 (it's one of the UX things that Bryan called out in our UX priorities doc <https://wiki.mozilla.org/Thunderbird:UX/Priorities/3.1>).  Blake, could you have a look at the code patch here?

Kohei, unless it's specifically impractical, we're now requiring automated tests for each changes to the core code that lands.  This seems like it's likely to be testable with MozMill.  Would you be willing to take a run a writing a test for this ?  if so, https://developer.mozilla.org/en/Thunderbird/Thunderbird_MozMill_Testing offers a starting point for this.  We're more than happy to help out by answering questions here or on IRC, improving documentation, etc...

Comment 8

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

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

So, I like it, but when I tested it, it revealed a bug that I think we should try to fix.

Specifically, when we sanitize the incoming hostname (in http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/sanitizeDatatypes.js#118), it gets lower-cased which means that when we try to replace the placeholders (in http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/accountConfig.js#194 and …#198), they don't get the new value, because it's defined in uppercase.

So, I guess the easiest thing to do would be to switch the values in otherVariables to lowercase before making those two calls to _replaceVariable, but I leave it up to you as to how you want to handle that.  (But I think you should just roll the fix for that into this patch.)

Thanks,
Blake.

Comment 9

[Ben Bucksch (:BenB)]

bwinton, that "hostname with placeholder" is a different bug and not caused by this patch nor related to this bug.

Comment 10

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

Yeah, but it's a very small fix, and the config in question needs it fixed to work anyways.

Comment 11

[Kohei Yoshino [:kohei]]

Bug 546278 will resolve bwinton's comment 8.

Comment 12

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

(In reply to comment #11)
> Bug 546278 will resolve bwinton's comment 8.

Okay, I'm happy to have that fixed in the other bug, which removes that part of my objection.

The only thing left blocking my approval of this patch is that it needs a test (or two).  If you can add that, you'll get my r+.

Thanks,
Blake.

Comment 13

[Dan Mosedale (:dmosedale, :dmose)]

Although we very much want this as soon as we can get it, if this were the last bug standing tomorrow night at 23:59, I don't think we'd hold the release for it, so moving out to block beta2. 

Kohei, would you be willing to take a run at putting together an automated test for this using MozMill?  <https://developer.mozilla.org/en/Thunderbird/Thunderbird_MozMill_Testing> has some information to help get started, and there are a number folks around who are willing and interested in helping answer questions that aren't already covered by the existing documentation...

Comment 14

[Kohei Yoshino [:kohei]]

Attachment: patch v2 (test added)

Sorry for the delay... This is my first time to write a MozMill test!

It works for me and an error returns as expected:
Configured outgoing server username is test. It should be [e-mail].

It also covers Bug 546278; if you comment out the line "outgoing server username", you may see an another error:
Configured incoming server hostname is %emaildomain%. It should be mx1.tiki.ne.jp.

As noted in the source, I have tried to load a local configuration file with a fake account, but MozMill loads http://localhost:43340/account-setup/%EMAILDOMAIN% as binary. So the test loads a live config file from remote. There might be a better way. (A possible workaround: adding request.overrideMimeType("text/xml") to FetchHTTP.)

Comment 15

[Kohei Yoshino [:kohei]]

(In reply to comment #14)
> As noted in the source, I have tried to load a local configuration file with a
> fake account, but MozMill loads
> http://localhost:43340/account-setup/%EMAILDOMAIN% as binary. So the test loads
> a live config file from remote. There might be a better way. (A possible
> workaround: adding request.overrideMimeType("text/xml") to FetchHTTP.)

Filed as Bug 548324.

Comment 16

[Ben Bucksch (:BenB)]

If you only want to test the config file and not HTTP specifically, you can drop the config file into the folder isp/ in the install dir (that should work for MozMill, too). The name should be the email domain + ".xml", e.g. <thunderbird>/isp/mx1.tiki.ne.jp.xml for the domain in the initial description.

Comment 17

[Kohei Yoshino [:kohei]]

Attachment: patch v3

This loads a local autoconfig file (my patch in Bug 548324 applied) and contains some improvements.

Comment 18

[Mark Banner (:standard8)]

The httpd fakeserver should be able to do the content type you require. I haven't yet figured out how to get to the required function.

Comment 19

[Kohei Yoshino [:kohei]]

Attachment: patch v3.1

> The httpd fakeserver should be able to do the content type you require. I
> haven't yet figured out how to get to the required function.

Just found collector.httpd.registerContentType!

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 428778 [details] [diff] [review]
patch v3.1

Thanks, Kohei!  sr=dmose for the few lines that require it because they are in mailnews/, conditional on an r+ from bwinton.

Comment 21

[Kohei Yoshino [:kohei]]

Comment on attachment 428778 [details] [diff] [review]
patch v3.1

Found a mistake:

>+  if (config == null)
>+    throw new Error("Timeout. Couldn't get the configuration file for " + user_email + ".");

The user_email variable doesn't exist.

And, I've used the autoconfig XML file from 
http://mxr.mozilla.org/comm-central/source/mailnews/base/test/unit/test_autoconfigUtils.js
but noticed that inbox.lv is an actual domain. This should be changed to a dummy domain like mozilla.test, I think.

I'll update the patch.

Comment 22

[Ben Bucksch (:BenB)]

> should be changed to a dummy domain like mozilla.test

example.com or mozilla.invalid, per RFC

Comment 23

[Kohei Yoshino [:kohei]]

Attachment: patch v4

Fixed an error, changed the server to example.com, and modified the emulating manual input code (to make sure the Continue button works well).

Comment 24

[Kohei Yoshino [:kohei]]

And moved the MozMill test under 'account' directory where people can add another account-related tests.

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

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

sr=dmose for the mailnews/ pieces; note that I haven't tested these changes myself.

Comment 26

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

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

So, the first time I ran the test, it passed.

But, the second and fourth times I ran it, it said:
TEST-UNEXPECTED-FAIL |  test_mail_account_setup
  EXCEPTION: account is undefined
    at: test-mail-account-setup-wizard.js line 149
       subtest_verify_account([object Object]) test-mail-account-setup-wizard.js 149
            frame.js 463
       WindowWatcher_notify([object XPCWrappedNative_NoHelper]) test-window-helpers.js 261
make: *** [mozmill-one] Error 1


And the third and fifth times I ran it, it said:
TEST-UNEXPECTED-FAIL |  test_mail_account_setup
  EXCEPTION: Configured incoming server hostname is pop.%emaildomain%. It should be pop.example.com.
    at: test-mail-account-setup-wizard.js line 174
       Error("Configured incoming server hostname is pop.%emaildomain%. It should be pop.example.com.")  0
       subtest_verify_account([object Object]) test-mail-account-setup-wizard.js 174
            frame.js 463
       WindowWatcher_notify([object XPCWrappedNative_NoHelper]) test-window-helpers.js 261

So I'm thinking that perhaps there's some cleanup that you're not doing or something, and it should be looked into before I give it the r+.

I've been using "make SOLO_TEST=account/test-mail-account-setup-wizard.js mozmill-one" to run the test.

>+++ b/mail/test/mozmill/account/test-mail-account-setup-wizard.js
>@@ -0,0 +1,175 @@
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Messaging.

I've been told that this should really be "The Mozilla Foundation", because they own the copyright on Mozilla Corp and Mozilla Messaging code.

>+ * Portions created by the Initial Developer are Copyright (C) 2009

And I think this should be "(C) 2010".  :)

>+var MODULE_NAME = "test-mail-account-setup-wizard";
>+
>+var RELATIVE_ROOT = "../shared-modules";
>+var MODULE_REQUIRES = ["window-helpers"];
>+
>+var mozmill = {};
>+Components.utils.import("resource://mozmill/modules/mozmill.js", mozmill);
>+var controller = {};
>+Components.utils.import("resource://mozmill/modules/controller.js", controller);
>+var elib = {};
>+Components.utils.import("resource://mozmill/modules/elementslib.js", elib);
>+
>+var wh, mc, awc, account, incoming, outgoing;
>+var user = { name: "Yamato Nadeshiko", email: "[e-mail]", password: "abc12345" };

This could be re-formatted as:
var user = {
  name: "Yamato Nadeshiko",
  email: "[e-mail]",
  password: "abc12345"
};
and then it wouldn't break the 80-character width limit.

>+// Remove an account on the Account Manager
>+function remove_account(amc) {
>+  let win = amc.window;
>+  

You've got some trailing spaces here, and in a couple of other places.

>+  for (let i = 0; i < 10; i++) {
>+    controller.sleep(600);
>+    if (config = awc.window.gEmailConfigWizard._currentConfigFilledIn != null)
>+      break;
>+  }

There has to be a better way for us to do this.  Suggestions, anyone?
Could you register your code as a listener on the autoconfig stuff, and get a notification when it's ready?

>+  // Open the advanced settings (Account Manager) to create the account immediately.
>+  // We use an invalid email/password so the setup will fail anyway.

  // Open the advanced settings (Account Manager) to create the account
  // immediately.  We use an invalid email/password so the setup will fail
  // anyway.

would be less than 80 characters wide.

And you also need to add "account" to the mail/test/mozmilll/mozmilltests.list to have "make mozmill" pick it up.

>+++ b/mail/test/mozmill/account/xml/example.com
>@@ -0,0 +1,24 @@
>+    <incomingServer type="pop3">
>+      <hostname>pop.%EMAILDOMAIN%</hostname>
[snip…]
>+    </incomingServer>
>+    <outgoingServer type="smtp">
>+      <hostname>smtp.example.com</hostname>

Do we want to bother testing the % substitution on the outgoing hostname, or do we think that's going to work just fine?

All in all, it looks very close to being accepted, and I'm fairly impressed with it as your first mozmill test.

Thanks,
Blake.

Comment 27

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

Patch fixed up and pushed as
http://hg.mozilla.org/comm-central/rev/19d5618679fe

Arrrgh!  I just realized that I forgot the -u "Kohei Yoshino
<[e-mail]>".  I'm really sorry about that, Kohei!  Let me
know how I can make it up to you, and thank you again for all your hard work on
this bug.