1294027 - per message From-Header not used for SMTP MAIL FROM

Status: RESOLVED FIXED

(MailNews Core :: Networking: SMTP, defect)

Description

[rene]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Firefox/38.0
Build ID: 20160513170057

Steps to reproduce:

sent mail with per message From header (customized From address)


Actual results:

SMTP HELO response is using underlaying identity email address


Expected results:

SMTP HELO response should use customized From address

Comment 1

[rene]

bug 87987 provides a really incomplete implementation. "MAIL FROM" is used by the SMTP server as a bounce address - it makes no sense to unintentionally set the bounce address differently than the senders identity. As a result, hiding your permanently stored identity is also not working, because the SMTP server will convert the "MAIL FROM" address into the "return path" header and provide this address to any possible spammer as well.

I checked the code but from my point of view the implementation is incomplete. There must be some code added into "nsSmtpProtocol::SendHeloResponse", currently the new identity is only stored in the messageComposeFields, which are not accessible (?) by the asynchronous sending process (my knowledge about the scope of the current changes is not enough to provide a patch),

Comment 2

[SkyLined]

IMHO this is by design: the email address you use to authenticate on the SMTP server is something separate and potentially different from the sender address you provide with an email.

For example: say you have an account on a server ([e-mail]), which allows you to send and receive from any email address on that domain: email sent to [e-mail] or [e-mail] ends up in your inbox, and you can send email from both addresses. In this set-up, if you want to send an email, you have to authenticate to the server as [e-mail] before you can send email from [e-mail] or [e-mail]. This is what Thunderbird now implements if I understand correctly: it is working perfectly like this for me. :)

If you want Thunderbird to use [e-mail] or [e-mail] to authenticate, you will need to add them as separate accounts on both the server and in Thunderbird. If you want to prevent [e-mail] from sending email as [e-mail] or [e-mail], you will need to set that up on the server to make sure it does not allow this.

Comment 3

[SkyLined]

Maybe I misunderstood: are you reporting that the "Return-Path:" is not set to the same value as the "From:" address when you use a customized From address, but that you do expect it to be?

Comment 4

[Adam Miauczyński]

Please read my comment from https://bugzilla.mozilla.org/show_bug.cgi?id=87987#c241 to get additional insight.

@rene, I think the title should be "per message From-Header not used for SMTP MAIL FROM"

Comment 5

[rene]

(In reply to SkyLined from comment #3)
> Maybe I misunderstood: are you reporting that the "Return-Path:" is not set
> to the same value as the "From:" address when you use a customized From
> address, but that you do expect it to be?

As far as I understand the issue, the SMTP server will add a return path header to some email if the HELO response and the From address are different. In this case the bounce address == return-path will set to the HELO address.

So, I don't expect to have a return path added to the email at all, because the SMTP HELO response and the From address should be the same. This is not by design, if you add some additional identity to any account it will not use the account main identity for the HELO response.

Comment 6

[rene]

(In reply to Adam Miauczyński from comment #4)
> @rene, I think the title should be "per message From-Header not used for
> SMTP MAIL FROM"

right, so also in all my comments, if I refer to HELO response I talk about the "MAIL FROM:" in the HELO response (which is different to the senders address). Thanks for pointing to it.

Comment 8

[rene]

Attachment: Bug_1294027-use_mail_senders_address_for_SMTP_MAIL_FROM.patch

this patch stores and reuses the senders address for SMTP MAIL FROM. I'm still new to thunderbird coding, so please have some extra attention to the string handling in nsSmtpProtocol.cpp.

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8939354 [details] [diff] [review]
Bug_1294027-use_mail_senders_address_for_SMTP_MAIL_FROM.patch

Oh, interesting, a first patch (right?) and so complex! Thanks. I assume you're requesting review, yes? I hope to get to it during the week.

Comment 10

[rene]

Yes, it's my first try and I like to request a review, right. The patch is complex and in some edges somehow to complex for me, but learning by doing. It really is is important for me to get this bug fixed - and if nobody else touches this...

Most of the code is adapted from surrounding similar code (like recipients handling), but I'm unsure if some extra conversion is required between the "mCompFields->GetFrom()" in nsMsgSend.cpp and the final use in nsSmtpProtocol.cpp...

Thanks for your time!

Comment 11

[Jorg K (CEST = GMT+2)]

Attachment: Bug_1294027-use_mail_senders_address_for_SMTP_MAIL_FROM.patch (v1) - rebased

Oops, I forgot to set myself as reviewer, a sure way to have this slip though the cracks.

Briefly looking over the patch I can see some style nits, but let's not focus on those right now. The most important thing to know is whether the patch will break any tests, so we need a try run. I rebased the patch (it didn't apply cleanly to latest trunk) and here's a try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=18e5c6315906267c70241cdaf4e39063cf24ee36

Please bear with me for a review, I'm busy with multiple issues at the same time (like everyone else) and will have to get up-to-speed with the problem as well first.

Comment 12

[Jorg K (CEST = GMT+2)]

Comment on attachment 8939499 [details] [diff] [review]
Bug_1294027-use_mail_senders_address_for_SMTP_MAIL_FROM.patch (v1) - rebased

Sadly this caused a whole heap of test failures:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=18e5c6315906267c70241cdaf4e39063cf24ee36&selectedJob=153807457

TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_bug155172.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_bug474774.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_bug474774.js | do_check_transaction - [do_check_transaction : 71] "EHLO test,MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE=23679,RCPT TO:<[e-mail]>,DATA" == "EHLO test,MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE=23679,RCPT TO:<[e-mail]>,DATA" [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_sendMailMessage.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtp8bitMime.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_sendMessageLater2.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_sendMessageLater2.js | do_check_transaction - [do_check_transaction : 71] "EHLO test,MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE=23679,RCPT TO:<[e-mail]>,DATA" == "EHLO test,MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE=23679,RCPT TO:<[e-mail]>,DATA" [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpPassword.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_sendMessageLater.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_sendMessageLater.js | do_check_transaction - [do_check_transaction : 71] "EHLO test,MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE=23679,RCPT TO:<[e-mail]>,DATA" == "EHLO test,MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE=23679,RCPT TO:<[e-mail]>,DATA" [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_sendBackground.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_sendBackground.js | do_check_transaction - [do_check_transaction : 71] "EHLO test,MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE=23679,RCPT TO:<[e-mail]>,DATA" == "EHLO test,MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE=23679,RCPT TO:<[e-mail]>,DATA" [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpPasswordFailure1.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpProxy.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpPasswordFailure2.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpPasswordFailure3.js | xpcshell return code: 0 [log…]
TEST-UNEXPECTED-FAIL | comm/mailnews/compose/test/unit/test_smtpAuthMethods.js | xpcshell return code: 0 

Before I can review the patch, you need to address those failures. Hopefully they all have a common cause.

Since you must be have a local copy of the code and must be able to build, you can also run tests locally, for example:

mozilla/mach xpcshell-test mailnews/compose/test/unit/test_bug155172.js

If you want to run a test in the debugger, do:
mozilla/mach xpcshell-test --interactive mailnews/compose/test/unit/test_bug155172.js

Let me know if you have questions. You picked a hard problem as your first bug :-(

Comment 13

[Jorg K (CEST = GMT+2)]

Oh, looking at this five seconds more, it looks like the tests need to be updated, since the data now contains a different FROM, like
"EHLO test,MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE=23679,RCPT TO:<[e-mail]>,DATA" ==
"EHLO test,MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE=23679,RCPT TO:<[e-mail]>,DATA"
which is expected but needs to be reflected in the tests.

Comment 14

[rene]

ohoh, the tests! Just started to read more about the testing, and after you tip was even able to run the tests locally.

So, what is the task now, should I provide patches for the tests if they have to be changed because of the modified behaviour? I couldn't find any information about that, if you can tell me or point me to some documentation?

Thanks for taking the time, I continue to try fixing the tests locally.

Comment 15

[Jorg K (CEST = GMT+2)]

Yes, the task is to fix the tests and send an additional patch (or integrate the changes into the first patch) to adapt the test to the modified behaviour.

Looking for example at test test_bug474774.js I can see
      do_check_transaction(server.playTransaction(),
                           ["EHLO test",
                            "MAIL FROM:<" + kSender + "> BODY=8BITMIME SIZE=" + originalData.length,
                            "RCPT TO:<" + kTo + ">",
                            "DATA"]);
and kSender is set to "[e-mail]".

So it appears that server.playTransaction() now returns MAIL FROM:<[e-mail]> BODY=8BITMIME SIZE=23679 and the match fails. You need to explain to me and yourself why that behaviour has changed now, so why FROM:<[e-mail]> is used now. If this is now the corrected behaviour, you need to change the test somehow, either change the value of kSender (but that's used elsewhere) or hard-code the from address. It could also be that this is not the expected behaviour, so then there would be something wrong in your code.

The main idea is that a test is documenting and guaranteeing correct behaviour, so whilst the test need to pass we also need to understand why they pass and that the behaviour we see is correct.

I'm not familiar with the setup of those fake accounts, so I don't know why [e-mail] has suddenly popped up. Looking back at comment #12, I can see that at least four tests fail due to this problem.

BTW, you're aware of https://dxr.mozilla.org/comm-central/source/ and https://searchfox.org/comm-central/source without which our lives would be real miserable ;-)

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 8939499 [details] [diff] [review]
Bug_1294027-use_mail_senders_address_for_SMTP_MAIL_FROM.patch (v1) - rebased

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

You'll need to make this optional. Otherwise it will likely fail for many users, whose smtp is expecting the normal address.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +37,5 @@
>  dontShowAlert=THIS IS JUST A PLACEHOLDER.  YOU SHOULD NEVER SEE THIS STRING.
>  
>  ## LOCALIZATION NOTE (tcpReadError): argument %s is the network error
>  tcpReadError=A network error occurred while receiving data.  (Network Error: %s)  Try connecting again.
> +couldNotGetUsersMailAddress=An error occurred while sending mail:  the senders address (From:) was invalid.  Please verify that this email address is correct and try again.

Please no double spaces (even though some strings around there use them)
It's "sender's", here and elsewhere

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 8939499 [details] [diff] [review]
Bug_1294027-use_mail_senders_address_for_SMTP_MAIL_FROM.patch (v1) - rebased

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

Since Magnus already commented, here are the issues I saw.

Looks like Magnus wants to make this optional, so please add a preference to mailnews/mailnews.js.

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +37,5 @@
>  dontShowAlert=THIS IS JUST A PLACEHOLDER.  YOU SHOULD NEVER SEE THIS STRING.
>  
>  ## LOCALIZATION NOTE (tcpReadError): argument %s is the network error
>  tcpReadError=A network error occurred while receiving data.  (Network Error: %s)  Try connecting again.
> +couldNotGetUsersMailAddress=An error occurred while sending mail:  the senders address (From:) was invalid.  Please verify that this email address is correct and try again.

If you change a string, you need to change the string ID as well since otherwise the localisation system doesn't present the string for translation. That system can only handle additions of strings. So make this couldNotGetUsersMailAddress2 and change all the references.
Also, you need to make the same string changes in suite/ code since SeaMonkey is also based on mailnews/ and we try to avoid breaking them.

::: mailnews/compose/public/nsISmtpUrl.idl
@@ +41,5 @@
>     */
>    attribute ACString dsnEnvid;
>  
>    /**
> +   * The sender of this mail (must not be equal to identity).

"must not be equal" means: It's never allowed to be the same. (Seen on a bus: You must not smoke.) So I guess you wanted to say: Can be different from identity.

::: mailnews/compose/src/nsMsgSend.cpp
@@ +3409,5 @@
>      if (!msgStatus)
>        msgStatus = do_QueryInterface(mStatusFeedback);
>  
>      nsCOMPtr<nsIURI> runningUrl;
> +    rv = smtpService->SendMailMessage(mTempFile, buf, mUserIdentity, mCompFields->GetFrom(),

Nit: Add another line instead of glueing to the end.

::: mailnews/compose/src/nsSmtpService.cpp
@@ +101,5 @@
>      if (!aPassword.IsEmpty())
>        smtpServer->SetPassword(aPassword);
>  
>      // this ref counts urlToRun
> +    rv = NS_MsgBuildSmtpUrl(aFilePath, smtpServer, aRecipients, aSenderIdentity, aSender,

Nit: Line too long. We try to keep it at ~80 chars.

::: mailnews/extensions/mdn/src/nsMsgMdnGenerator.cpp
@@ +858,1 @@
>                                       EmptyString(), this, nullptr, nullptr, false, nullptr,

Nit: Please fix the faulty indentation while you're here.

Comment 18

[rene]

Hi, thanks for all the feedback, it will take a while to go through all the test and understand what they are trying to achieve to change the patch or the tests in a useful way. (and this will take time, wow)

I also see the requirement for some option to enable / disable this feature, but how to bring this into the tests? Is there some standard way or should I do all the related tests (which compare the SMTP communication with a expected result) twice? Or should I decide out of my mind if this is relevant in the context of these tests and just set one option?

Comment 19

[Jorg K (CEST = GMT+2)]

Good question. The lazy approach would be to have that preference off by default, then all the tests will still pass.

Then pick a relevant test, set the option and see that it works. Or if possible, in some of the tests run some of the subtests twice with both preference values. There is no easy answer, as I said, you picked a hard one.

Comment 20

[rene]

Attachment: Bug_1294027-use_mail_senders_address_for_SMTP_MAIL_FROM.patch (v2)

Comment 21

[rene]

Attachment: updated_tests_for_Bug_1294027-use_mail_senders_address_for_SMTP_MAIL_FROM_v2.0.patch

Comment 22

[rene]

Hi,

just fixed the mentioned issues in the patch, added some option ("mail.smtp.useSenderForSmtpMailFrom") and additionally changed the implementation of nsMsgMdnGenerator.cpp to use the identity email address in any case.

I didn't found a way to test the mdn extension, hard to search the internet with the given keywords (mdn extension) for how to build and what to achieve with that extension. So any suggestion how to test this extension will be great.

Than I went through all the broken tests, and yes, I decided to do it the hard way. The feature should imho. be enabled by default, because that's what I think people will expect when they change the senders address at all.

The patches had to be changed to clarify 3 different occurrences of sender's address:
1. in the mail itself, like in data/message1.eml or data/429891_testcase.eml
2. in the to-be-created identity
3. in the msgCompose-Fields

If the option "mail.smtp.useSenderForSmtpMailFrom" is set to false, the identity email address is used for smtp MAIL FROM. If the option is set to "true", the senders adress from the msgCompose-Fields is used for smtp MAIL FROM - so far so good.
But I had to learn, that if some mail is send later (msgSend.sendMessageFile) the msgCompFields have to be set before calling this method, but contents are ignored and refilled with the values parsed from the message body just before sending the mail (wow, what a magical piece of code is this msgSendLater.cpp, displaying a message into itself to parse the mail, that's something to get mentally stuck for a while!)

To separate all those cases I modified all used addresses in some distinct ways and changed the tests accordingly - sometimes because of same values in msgCompFields and the mail files it was hard to decide which value was finally used. I'm in no way sure if it is clever to keep all these email addresses differentiated, because in the end the ones in the mail file and in the msgCompose-Fields will be the same all the time, but to trace what's happening it is really helpful. I can change that back if required, no problem.

The major test of the new implementation with the test of both cases is implemented in test_sendMailMessage.js.

What do you think? Best regards, Rene

Comment 23

[rene]

oh, and maybe just to understand the modifications in the tests, following variables are used trough all the modified tests for the sender's address:

1. for the adress in the mail file itself: kTestFileSender
2. for the adress in the to-be-created identity: kIdentityMail
3. for the adress in msgCompFields (sendLater or to call smtpService->SendMailMessage): kSender

regards, Rene

Comment 24

[Jorg K (CEST = GMT+2)]

Nice job, here's a try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=3db623c3a0956d079565970d50f04e82e0c9764d

Comment 25

[Jorg K (CEST = GMT+2)]

Build system problems right now, so again:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=26d57d1b343053f3a0ca2d89a0ce41fbf82f35dd

Comment 26

[Jorg K (CEST = GMT+2)]

All tests passed, as you had said, but better safe than sorry. I'll get to the review in the next few days, it's not trivial so it's not a five minute review.

Comment 27

[Jorg K (CEST = GMT+2)]

Comment on attachment 8941391 [details] [diff] [review]
Bug_1294027-use_mail_senders_address_for_SMTP_MAIL_FROM.patch (v2)

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

I'm terribly sorry about the delay in reviewing this. I had some bustage/breakage to deal with for a few days, so reviews for non-essential features drop in priority. I know that this can be discouraging to new contributors, I know the feeling since I've been in the same situation many times. Sadly, reviewing the test changes will take a little longer still.

The code changes are fine in general which a few nits. One issue: Previously there was one error code NS_ERROR_COULD_NOT_GET_USERS_MAIL_ADDRESS which was used for
a) can't get identity
b) can get identity but e-mail is empty.

Now you're distinguishing the two cases by introducing NS_ERROR_COULD_NOT_GET_SENDERS_IDENTITY. That's fine. But you didn't consider that NS_ERROR_COULD_NOT_GET_USERS_MAIL_ADDRESS is used in other spots:
https://dxr.mozilla.org/comm-central/search?q=NS_ERROR_COULD_NOT_GET_USERS_MAIL_ADDRESS&redirect=false
In msgMapiMain.cpp the new code should most likely be added to the case statement.

r+ with two things addressed:
- add NS_ERROR_COULD_NOT_GET_SENDERS_IDENTITY to case statement in msgMapiMain.cpp.
- fix comment nits.
No need to address the prefBranch stuff, that's for information only.

I'll do some testing now and the try to find time to look at the tests.

Thanks again for the contribution.

::: mailnews/compose/src/nsSmtpProtocol.cpp
@@ +728,5 @@
> +  rv = prefs->GetBranch(nullptr, getter_AddRefs(prefBranch));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  bool useSenderForSmtpMailFrom = false;
> +  prefBranch->GetBoolPref("mail.smtp.useSenderForSmtpMailFrom", &useSenderForSmtpMailFrom);

You copied the hard old code. It can be done much easier in a one-liner:
bool useSenderForSmtpMailFrom =
    mozilla::Preferences::GetBool("mail.smtp.useSenderForSmtpMailFrom", true);
Anyway, since there is code further down in the function using prefBranch it's OK to leave the change as you have it.

@@ +736,2 @@
>    {
> +    // extract the email address from the mail headers

Nit: I know that this comment was pre-existing, but our style guidelines state that you need to start upper-case and terminate in a full stop.

@@ +746,4 @@
>    }
> +  else
> +  {
> +    // extract the email address from the identity

Same here.

Comment 28

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+1) from comment #27)
> I'll do some testing now and the try to find time to look at the tests.
I guess the tests will be the real proof that this is working.

In manual testing I sent myself some messages with modified From: address with and without the patch and inspected the received message headers. With the patch the Return-path: now reflects the edited From address. Nice.

Comment 29

[rene]

Hi Jorg,

thanks for your fast feedback! And at all I'm - on the contrary! - surprised how fast all feedback came from your side since my first patch. That's really nice, and its in no way too much to wait a few days if somebody gets such helpful feedback. I honestly was afraid it might take months until I will get some process - I feel really comfortable as a newcomer here.

To provide an adapted patch I had a look into the MAPI-code to understand it a little bit. The error message conversion into "MAPI_E_INVALID_RECIPS" sounds like talking about some recipient issue. Even the description at msdn says: "One or more recipients were invalid or did not resolve to any address." (msdn [1] is the reference, isn't it?)

This does not fit to both error issues at all and was probably just used because the MAPI did not supported any closer error description. But I think it's misleading because getting told the problem is at the recipients when its on senders side is not helpful. Therefore I suggest to remove the special case for NS_ERROR_COULD_NOT_GET_USERS_MAIL_ADDRESS and let both cases map to default "MAPI_E_FAILURE" == "One or more unspecified errors occurred. No message was sent."

Or am I getting something wrong? What do you think?

Best regards, Rene

[1] https://msdn.microsoft.com/en-us/library/hh802867(v=vs.85).aspx

Comment 30

[Jorg K (CEST = GMT+2)]

Yes, I agree. How about:
    case NS_ERROR_COULD_NOT_GET_USERS_MAIL_ADDRESS :
    case NS_ERROR_COULD_NOT_GET_SENDERS_IDENTITY :
      // Something went wrong with the sender. There's no error we can map to
      // so we use a general error, see:
      // https://msdn.microsoft.com/en-us/library/hh802867(v=vs.85).aspx
      hr = MAPI_E_FAILURE;
      break;

Comment 31

[rene]

Attachment: Bug_1294027-use_mail_senders_address_for_SMTP_MAIL_FROM.patch(v3)

Comment 32

[rene]

Attachment: updated_tests_for_Bug_1294027-use_mail_senders_address_for_SMTP_MAIL_FROM_v3.0.patch

Comment 33

[rene]

Hi Jorg,

just updated the bugfix-patch including your suggestions. Additionally changed comments from the tests-patch to fit the style guideline requirements (only changed comments in the tests, nothing else).

Best regards, Rene

Comment 34

[Jorg K (CEST = GMT+2)]

Comment on attachment 8943589 [details] [diff] [review]
Bug_1294027-use_mail_senders_address_for_SMTP_MAIL_FROM.patch(v3)

Thanks for fixing these small issues. Also thanks for updating the tests to match the style guide. That's much appreciated. I had a quick glance at the differences.

Please have some more patience, reviewing your test changes is high on my list but in the next few days my availability is reduced. I'll get it done soon.

Comment 35

[Jorg K (CEST = GMT+2)]

Comment on attachment 8943590 [details] [diff] [review]
updated_tests_for_Bug_1294027-use_mail_senders_address_for_SMTP_MAIL_FROM_v3.0.patch

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

This is very nice work, indeed. Once I got the hang of it, it was easy to follow.
Looks like the test data in message1.eml grew by 4 bytes, hence the size change from 155 to 159.
No need to send another patch, I'll make SMTP uppercase when landing this.

Thank you for your contribution!

::: mailnews/compose/test/unit/test_bug474774.js
@@ +204,5 @@
>                       .createInstance(Ci.nsIMsgCompFields);
>  
> +  // Setting the compFields sender and recipient to any value is required to
> +  // survive mime_sanity_check_fields in nsMsgCompUtils.cpp.
> +  // Sender and recipient are required for sendMessageFile but smtp

Nit: SMTP and further down.

::: mailnews/compose/test/unit/test_sendMailMessage.js
@@ +38,5 @@
>      // Just a basic test to check we're sending mail correctly.
>      test = "Basic sendMailMessage";
>  
> +    // First do test with identity email address used for smtp MAIL FROM.
> +    Services.prefs.setBoolPref("mail.smtp.useSenderForSmtpMailFrom", false);

Excellent that you have two test cases with both pref values.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/202106d62ad7
use mail sender's address for SMTP MAIL FROM. r=jorgk
https://hg.mozilla.org/comm-central/rev/f1f49dd06edb
use mail sender's address for SMTP MAIL FROM (updated tests). r=jorgk

Comment 37

[rene]

Hi Jorg, thanks for your work and for guiding me trough the process of getting this fix committed. It was motivating and I surely will be back on bugzilla to help improve thunderbird. Regards, Rene