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

RESOLVED FIXED

Status

--
major
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: kohei, Assigned: bwinton)

Tracking

unspecified

Firefox Tracking Flags

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

Details

(Whiteboard: [UXprio])

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

9 years ago
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 test@mx1.tiki.ne.jp as an username

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

Expected results:
The incoming username: test
The outgoing username: test@mx1.tiki.ne.jp
(Reporter)

Comment 1

9 years ago
Created attachment 417457 [details] [diff] [review]
patch v1
blocking-thunderbird3.0: --- → ?

Comment 2

9 years ago
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

9 years ago
(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.
(Assignee)

Comment 4

9 years ago
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.
blocking-thunderbird3.0: ? → -
status-thunderbird3.0: --- → wanted
Flags: blocking-thunderbird3.1?

Comment 5

9 years ago
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

9 years ago
(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).
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...
Assignee: nobody → kohei.yoshino.bugs
Group: mozilla-confidential
blocking-thunderbird3.1: --- → beta1+
status-thunderbird3.0: wanted → ---
Flags: blocking-thunderbird3.1?
Group: mozilla-confidential
(Assignee)

Comment 8

9 years ago
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.
Attachment #417457 - Flags: review-

Comment 9

9 years ago
bwinton, that "hostname with placeholder" is a different bug and not caused by this patch nor related to this bug.
(Assignee)

Comment 10

9 years ago
Yeah, but it's a very small fix, and the config in question needs it fixed to work anyways.
(Reporter)

Comment 11

9 years ago
Bug 546278 will resolve bwinton's comment 8.
(Assignee)

Comment 12

9 years ago
(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.
Status: NEW → ASSIGNED
Whiteboard: [UXprio]
Whiteboard: [UXprio] → [UXprio][needs unit test]
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...
blocking-thunderbird3.1: beta1+ → beta2+
(Reporter)

Comment 14

9 years ago
Created attachment 428703 [details] [diff] [review]
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 test@mx1.tiki.ne.jp.

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.)
Attachment #417457 - Attachment is obsolete: true
(Reporter)

Comment 15

9 years ago
(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

9 years ago
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.
(Reporter)

Comment 17

9 years ago
Created attachment 428767 [details] [diff] [review]
patch v3

This loads a local autoconfig file (my patch in Bug 548324 applied) and contains some improvements.
Attachment #428703 - Attachment is obsolete: true
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.
(Reporter)

Comment 19

9 years ago
Created attachment 428778 [details] [diff] [review]
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!
Attachment #428767 - Attachment is obsolete: true
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.
Attachment #428778 - Flags: superreview+
Attachment #428778 - Flags: review?(bwinton)
(Reporter)

Comment 21

9 years ago
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.
Attachment #428778 - Flags: superreview+
Attachment #428778 - Flags: review?(bwinton)

Comment 22

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

example.com or mozilla.invalid, per RFC
(Reporter)

Comment 23

9 years ago
Created attachment 429412 [details] [diff] [review]
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).
Attachment #428778 - Attachment is obsolete: true
Attachment #429412 - Flags: superreview?(dmose)
Attachment #429412 - Flags: review?(bwinton)
(Reporter)

Comment 24

9 years ago
And moved the MozMill test under 'account' directory where people can add another account-related tests.
Comment on attachment 429412 [details] [diff] [review]
patch v4

sr=dmose for the mailnews/ pieces; note that I haven't tested these changes myself.
Attachment #429412 - Flags: superreview?(dmose) → superreview+
(Assignee)

Comment 26

9 years ago
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: "yamato.nadeshiko@example.com", password: "abc12345" };

This could be re-formatted as:
var user = {
  name: "Yamato Nadeshiko",
  email: "yamato.nadeshiko@example.com",
  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.
Attachment #429412 - Flags: review?(bwinton) → review-
Whiteboard: [UXprio][needs unit test] → [UXprio][needs updated patch, review, landing]
Assignee: kohei.yoshino.bugs → bwinton
(Assignee)

Comment 27

9 years ago
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
<kohei.yoshino.bugs@gmail.com>".  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.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
status-thunderbird3.1: --- → beta2-fixed
Resolution: --- → FIXED
Whiteboard: [UXprio][needs updated patch, review, landing] → [UXprio]

Updated

9 years ago
Blocks: 570138
You need to log in before you can comment on or make changes to this bug.