1135610 - Changing identity does not change From: name and address in actual e-mail

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Jan Moesen]

I have mail for several domains coming together in one mailbox. E.g.
[e-mail] and [e-mail] all end up in the same (IMAP) mailbox.

Because Thunderbird chooses the first identity when composing a new e-mail, I
sometimes sent mail from [e-mail] while I meant to send it from
[e-mail].

To work around this, I changed the first identity to be bogus. I gave it a name
that catches the eye ("– DO NOT USE – DO NOT USE – DO NOT USE – DO NOT USE – DO
NOT USE – DO NOT USE –") and an invalid e-mail address ("@@") so my SMTP server
complains when I tried to send it. That reminds me to choose the right identity
for the e-mail. This all works fine.

What no longer works in the Daily builds, but still works in Earlybird, is that
*after* my SMTP complains and I select the right identity, it still uses the
dummy identity ("- DO NOT USE - DO NOT USE - …") for the "From:" header, even
though the Return-Path is now "[e-mail]".

This is undesirable. Has this changed recently? It works fine in Earlybird, but not in Daily. I think I first noticed it about a week ago. There was no reply in #thunderbird on IRC.

STR:
1) Create a new profile
2) Create an e-mail account with two identities
2a) Name: "WRONG"; E-mail address: "@"
2b) Name: "RIGHT"; E-mail address: "[e-mail]"
3) Create a new e-mail messages with a recipient and subject of your choice
4) Try to send it from the first identity, "WRONG". The SMTP server should reply with an error message
5) Try to send it from the second identity, "RIGHT". The message should go through.
6) Open the message in the recipient's inbox. It will say: "From: WRONG <"">"

I would expect it to say: "From: RIGHT <[e-mail]>".

Comment 1

[WADA:World Anti-bad-Duping Agency]

(In reply to Jan Moesen from comment #0)
> STR:
> 1) Create a new profile
> 2) Create an e-mail account with two identities
> 2a) Name: "WRONG"; E-mail address: "@"
> 2b) Name: "RIGHT"; E-mail address: "[e-mail]"
> 3) Create a new e-mail messages with a recipient and subject of your choice
> 4) Try to send it from the first identity, "WRONG". The SMTP server should reply with an error message

Did Tb actulaly send the mail with "From: WRONG <"">" to SMTP server?
Did SMTP server accept the mail with "From: WRONG <"">"? Or rejected?

> 5) Try to send it from the second identity, "RIGHT". The message should go
> through.
> 6) Open the message in the recipient's inbox. It will say: "From: WRONG <"">"
> I would expect it to say: "From: RIGHT <[e-mail]>".

What do you call by "recipient's inbox"?

If message header is From: WRONG <"">, TB tries to pass "" as FROM mail address to SMTP server upon mail send.
SMTP server rejects FROM mail address == "". If Tb's request to SMTP is rejected, the mail won't be sent to recipient by SMTP server.
How can un-sent mail be seen at Inbox of recipient?

Do you call "Sent folder in Tb" by the "recipient's inbox"?
Problem of Bug 780124, isn't it?

Comment 2

[Jan Moesen]

> Did Tb actulaly send the mail with "From: WRONG <"">" to SMTP server?
Yes.

> Did SMTP server accept the mail with "From: WRONG <"">"? Or rejected?
Yes. The Return-Path is set to [e-mail], which would be the address in "MAIL FROM". The "From:" header is inside the "DATA" block, and can be pretty much anything you want.

> What do you call by "recipient's inbox"?
The mail as received by the recipient. I.e., when sending to [e-mail], John Doe's inbox at example.com, however it might be accessed.

> If message header is From: WRONG <"">, TB tries to pass "" as FROM mail
> address to SMTP server upon mail send.
> SMTP server rejects FROM mail address == "". If Tb's request to SMTP is
> rejected, the mail won't be sent to recipient by SMTP server.
> How can un-sent mail be seen at Inbox of recipient?
The *first* time it is rejected because the envelope address/return path/MAIL FROM/whatchamacallit is not valid. The *second* time it /does/ get sent because the address /is/ properly formatted. However, the address in the "From:" message header is not updated.

> Do you call "Sent folder in Tb" by the "recipient's inbox"?
No.

> Problem of Bug 780124, isn't it?
Not quite the same.

Comment 3

[WADA:World Anti-bad-Duping Agency]

(In reply to Jan Moesen from comment #2)
> The *first* time it is rejected because the envelope address/return path/MAIL FROM/whatchamacallit is not valid. 
> > Problem of Bug 780124, isn't it?
> Not quite the same.

It looks Bug 780124 won't fortunately happen in your environment. (bug occurs if SMTP server kills connection after error response).

Following looks for "second send with identity change after first send failure with wrong identity/wrong address"
> 5) Try to send it from the second identity, "RIGHT". The message should go through.
> 6) Open the message in the recipient's inbox. It will say: "From: WRONG <"">"

> Yes. The Return-Path is set to [e-mail], which would be the address in "MAIL FROM".
> The "From:" header is inside the "DATA" block, and can be pretty much anything you want.

What do you mean by "inside the DATA block"?
From: header of the received mail was wrong "From: WRONG <"">", wasn't it?
Or WRONG is shown at From column of Thread pane of Tb?

Comment 4

[Jan Moesen]

As a brief introduction to SMTP, this is a condensed version of what happens. "C" is the client (Thunderbird), S is the SMTP server.

C connects to the host name and port of S
S: 220 smtp.example.com
C: MAIL FROM:<@>
S: 501 5.1.7 Bad sender address syntax
C: QUIT
S: 221 2.0.0 Bye

On the second try, after I have changed the identity:

S: 220 smtp.example.com
C: MAIL FROM:<[e-mail]>
S: 250 2.1.0 Ok
C: RCPT TO:<[e-mail]>
S: 250 2.1.5 Ok
C: DATA
S: 354 End data with <CR><LF>.<CR><LF>
C: From: WRONG <"">
…
C: .
S: 250 2.0.0 Ok: queued
C: QUIT
S: 221 2.0.0 Bye

The "DATA" block is the actual e-mail message, including the headers. What it says in "From:" is wrong, and does not match what Thunderbird tells the SMTP server in the "MAIL FROM" command.

It *looks* like the composed message is "cached" and not updated after changing the identity.

Comment 5

[WADA:World Anti-bad-Duping Agency]

It sounds that "event of iidentity change" doesn't invoke message source change.
If you touch or alter To:/CC:, Subject textt, Body text after identity change, whaat message source is creted by "Send Later"?

Comment 6

[WADA:World Anti-bad-Duping Agency]

Unable to reproduce problem in Tb 31.3.0 by manual "Save As draft" and "Send Later", without auto-save..
Send only issue?
Or auto-save is relevant? auto-save starts with identity-1, while auto-saving, identity-2 is chosen, auto-saave completes, then send.

Is problem consistently reproduced?

Comment 7

[WADA:World Anti-bad-Duping Agency]

(In reply to Jan Moesen from comment #4)
> C: MAIL FROM:<@>
> S: 501 5.1.7 Bad sender address syntax
> 
> C: DATA
> S: 354 End data with <CR><LF>.<CR><LF>
> C: From: WRONG <"">

Is message source in recipient's Inbox "From: WRONG <>" instead of "From: WRONG <@>"
Is this flow actual log data? Or for explantion of phenomenon?

Comment 8

[WADA:World Anti-bad-Duping Agency]

(In reply to Jan Moesen from comment #0)
> I have mail for several domains coming together in one mailbox. E.g.
> [e-mail] and [e-mail] all end up in the same (IMAP) mailbox.
> 
> STR:
> 1) Create a new profile
> 2) Create an e-mail account with two identities
> 2a) Name: "WRONG"; E-mail address: "@"
> 2b) Name: "RIGHT"; E-mail address: "[e-mail]"
> 3) Create a new e-mail messages with a recipient and subject of your choice
> 4) Try to send it from the first identity, "WRONG". The SMTP server should reply with an error message
> 5) Try to send it from the second identity, "RIGHT". The message should go through.

How, where, when did you change From: address?
Because "From: address list in composer window" is one which was set upon start of mail composition, following can occur.
   id1=A A <a@a.a.a>, id2=B B <@>
   Start composing. if id1 is chosen,  A A <a@a.a.a> is shown, if id2 is chosen,  B B <@> is shown.
   change identity setting at Account Settings, save change :   id1=A A ＜@>, id2=B B <ｂ@b.b.b>
   At identity list of composer window, A A <a@a.a.a> is still shown when id1 is shosen, and B B <@>  is still shown when id2 is chosen.
   Choose id1(A A <a@a.a.a> is shown at composer window) , Save As draft
      => From: A A <@> is written in message source

Comment 9

[[e-mail]]

Attachment: Possible patch

This bug might be fallout from the back end changes in bug 87987, in which case this part of that patch should suffice to fix it.

Comment 10

[Jan Moesen]

FWIW, the whole SMTP transaction is not even necessary: it also happens when saving a message as a draft.

In the setup above with two identities: save using one identity, edit, change to the other identity, save again. "X-Identity-Key" gets updated, "From" does not.

The patch mentions "GenericSendMessage", and I know nothing of the internals, so I don't know if the "Save as draft" path goes through that function, too.

Comment 11

[Magnus Melin [:mkmelin]]

Interesting way to circumvent ISP From checking... I was able to reproduce.

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 8574131 [details] [diff] [review]
Possible patch

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

This isn't quite correct, but it seems it does select the correct identity now.

With the patch I (literally) get "From: id15"
id number 15 is indeed the correct identity it should have been from.

Comment 13

[[e-mail]]

Attachment: Possible patch

I've added more of the patch from bug 87987, this should help.

Comment 14

[Magnus Melin [:mkmelin]]

Comment on attachment 8574598 [details] [diff] [review]
Possible patch

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

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3237,5 @@
> +function getCurrentIdentityKey()
> +{
> +    // get the identity key
> +    var identityList = GetMsgIdentityElement();
> +    return identityList.selectedItem.getAttribute("identitykey");

nit: 2 space indention for all new code

@@ +3917,5 @@
>      var identityElement = document.getElementById("msgIdentity");
>      var prevIdentity = gCurrentIdentity;
>  
>      if (identityElement) {
> +        var idKey = identityElement.selectedItem.getAttribute("identitykey");

This is causing an exception on startup

Error: identityElement.selectedItem is null
Source File: chrome://messenger/content/messengercompose/MsgComposeCommands.js
Line: 3921

Comment 15

[[e-mail]]

Attachment: Third time lucky?

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 8577015 [details] [diff] [review]
Third time lucky?

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

Yep. r=mkmelin

We should really have a mozmill test for this where we change identity and check the selected one corresponds to From in the message put in Outbox.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +3238,5 @@
> +function getCurrentIdentityKey()
> +{
> +    // get the identity key
> +    var identityList = GetMsgIdentityElement();
> +    return identityList.selectedItem.getAttribute("identitykey");

2 space indent for new code

Comment 17

[[e-mail]]

Pushed comm-central changeset f1483d6e7b56.

Comment 18

[[e-mail]]

Comment on attachment 8577015 [details] [diff] [review]
Third time lucky?

[Approval Request Comment]
Regression caused by (bug #): 87987
User impact if declined: Can't change identity when composing mail

Comment 19

[Kent James (:rkent)]

Comment on attachment 8577015 [details] [diff] [review]
Third time lucky?

https://hg.mozilla.org/releases/comm-aurora/rev/8be34b101c6c

Comment 20

[Kent James (:rkent)]

There are new mozmill test failures occurring on comm-aurora that are likely due to this patch. Could somebody please check and comment? I am likely going to have to back it out.

See https://treeherder.mozilla.org/#/jobs?repo=comm-aurora&revision=8be34b101c6c

Comment 21

[Kent James (:rkent)]

OK it is not just comm-aurora, the comm-central patch shows the same errors.

Comment 22

[Kent James (:rkent)]

Backed out from comm-aurora https://hg.mozilla.org/releases/comm-aurora/rev/f70c5e679fab

Comment 23

[Magnus Melin [:mkmelin]]

https://treeherder.mozilla.org/logviewer.html#?job_id=13677&repo=comm-central

TEST-UNEXPECTED-FAIL | /builds/slave/test/build/mozmill/composition/test-address-widgets.js | test-address-widgets.js::test_address_types
12147 TEST-UNEXPECTED-FAIL | /builds/slave/test/build/mozmill/composition/test-send-button.js | test-send-button.js::test_send_enabled_prefilled_address_from_identity
12172 TEST-UNEXPECTED-FAIL | /builds/slave/test/build/mozmill/composition/test-send-button.js | test-send-button.js::test_send_enabled_address_contacts_sidebar
17542 TEST-UNEXPECTED-FAIL | /builds/slave/test/build/mozmill/composition/test-signature-updating.js | test-signature-updating.js::testPlaintextComposeWindowSwitchSignatures
17579 TEST-UNEXPECTED-FAIL | /builds/slave/test/build/mozmill/composition/test-signature-updating.js | test-signature-updating.js::testHTMLComposeWindowSwitchSignatures
17605 TEST-UNEXPECTED-FAIL | /builds/slave/test/build/mozmill/composition/test-signature-updating.js | test-signature-updating.js::testHTMLComposeWindowSwitchSignaturesWithSuppressedSeparator
18511 TEST-UNEXPECTED-FAIL | (runtestlist.py) | Exited with code 1 during directory run

Comment 24

[Kent James (:rkent)]

Backed out from c-c https://hg.mozilla.org/comm-central/rev/3d901426e62c

Comment 26

[Kent James (:rkent)]

Could we get the test failures fixed here so that we can reland this?

Comment 27

[Magnus Melin [:mkmelin]]

If not we should back out https://hg.mozilla.org/comm-central/rev/6fe2526c838f and 
https://hg.mozilla.org/comm-central/rev/c3bf5c52ce41 until it's working.

Comment 28

[[e-mail]]

Relanded with test fixes in comm-central changeset 35d9101cb3b1.

Comment 29

[Kent James (:rkent)]

Comment on attachment 8577015 [details] [diff] [review]
Third time lucky?

I'll try again to push to comm-aurora:

https://hg.mozilla.org/releases/comm-aurora/rev/091960adda43

Unfortunately mozmill is down on comm-central so I have no confirmation that tests work now. But tests still work on comm-aurora so we can watch it there.

Comment 30

[Kent James (:rkent)]

Comment on attachment 8577015 [details] [diff] [review]
Third time lucky?

Backed out again due to test failures from comm-aurora
https://hg.mozilla.org/releases/comm-aurora/rev/83e056f26c1a

Comment 31

[[e-mail]]

With typo fixes:

https://hg.mozilla.org/releases/comm-aurora/rev/d3e7a38fe3c5

If this works I'll apply the typo fixes to trunk too.

Comment 32

[[e-mail]]

Typo fixes pushed: https://hg.mozilla.org/comm-central/rev/66d2ea9ace92

Comment 33

[Magnus Melin [:mkmelin]]

So is dev-doc-needed what we use to notify add-on authors that they may need to change their code? 
In this case, use the identitykey attribute instead of value, for the identityList items

Comment 34

[Wayne Mery (:wsmwk)]

addon-compat :)