1461106 - Deleting SMTP password doesn't affect sending mail until application restarts.

Status: RESOLVED FIXED

(Thunderbird :: Security, defect)

Description

[Josiah Bruner [:jsbruner]]

This is bug 516464, but for SMTP.

Pre-Conditions:

1. Thunderbird contains an email account `e` with a saved SMTP account in the password manager

STR:

1. Open compose
2. Send an email using account `e` -> Should send successfully
3. Open password manager
4. Delete saved SMTP account for `e`. -> At this point TB shouldn't know how to authenticate with the SMTP server.
5. Send an email using account `e` again

Expected Results:

Sending an email (step 5) should prompt the user for their password

Actual Results:

The email from step 5 sends without prompting.

Analysis:

The root cause for this issue is (likely) the same as bug 516464. That is, the password is stored in cache, and when the password is remove from the password manager, we don't clear the cached password.

Comment 1

[Josiah Bruner [:jsbruner]]

Attachment: WIP Patch

I'm still in the process of testing this a bit more thoroughly, but at first glance this appears to fix the issue.

Interestingly, even though nsSmtpServer inherits nsSupportsWeakReference, when I tried to pass true to the last param of AddObserver(), nsSmtpServer never received notifications. Using false, however, did work.

This seems contradictory to what I did in bug 516464. I will need to research this in more detail.

Comment 2

[Jorg K (CEST = GMT+2)]

Any update here? The patch looks good, would you like a try run?

Comment 3

[Josiah Bruner [:jsbruner]]

(In reply to Jorg K (GMT+2) (bustage-fix only, NI for urgent reviews) from comment #2)
> Any update here? The patch looks good, would you like a try run?

It seems to work locally, but the patch likely needs to implemt NSIObserver similar to what was needed in the other bug. But, a try run would verify that! 

So if it's convenient, a try run would be great. (I think I got access to push to try now, but can't attempt until tomorrow)

Comment 4

[Jorg K (CEST = GMT+2)]

I'll leave it to you then. For the record, I use:
hg qnew -m "try: -b do -p macosx64,linux64 -u all" try && hg push -f -r tip cc-try && hg qpop && hg qdelete try
where cc-try is set to |cc-try = ssh://[e-mail]@hg.mozilla.org/try-comm-central| in hgrc.

Comment 5

[Josiah Bruner [:jsbruner]]

Took me several tries, but I got it: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0ca2944b3d031aee4e2fb348c23480041aa6df13

Comment 6

[Josiah Bruner [:jsbruner]]

(In reply to Josiah Bruner [:jsbruner] (needinfo for responses) from comment #5)
> Took me several tries, but I got it:
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=0ca2944b3d031aee4e2fb348c23480041aa6df13

Whoops. "Warnings treated as errors". New attempt: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=185d4cac7a045318c03935c67d5ba4f8ed790d0c

Comment 7

[Jorg K (CEST = GMT+2)]

(In reply to Josiah Bruner [:jsbruner] (needinfo for responses) from comment #5)
> Took me several tries, but I got it:
You always need to do your try push from C-C tip, due to a lot of bustage we repair regularly, nothing else will likely work.

Lots of debug crashes in the latest push :-(

Comment 8

[Jorg K (CEST = GMT+2)]

Looks like you know what to do to fix the crashes. Please do, run anther try and then NI me for review. Thanks for the persistence!

Comment 9

[:aceman]

Is there a need for the password caching? Can't the password just be fetched from the Password manager when a message is sent? Is that very expensive?

Comment 10

[Josiah Bruner [:jsbruner]]

I believe the caching is used to reduce the number of times we prompt the user for their password when the password manager is *not* used. It's a UX feature, not a perf one likely.

Comment 11

[Josiah Bruner [:jsbruner]]

Attachment: bug8975283.patch

This fixes the "missing implementation" assertions. I think it adequately resolves this bug, but I will start another try run to verify.

Comment 12

[Josiah Bruner [:jsbruner]]

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6cb9ff50aa3ab66b875b8ab9648688584f333416

Comment 13

[Josiah Bruner [:jsbruner]]

(In reply to Josiah Bruner [:jsbruner] (needinfo for responses) from comment #12)
> Try run:
> https://treeherder.mozilla.org/#/jobs?repo=try-comm-
> central&revision=6cb9ff50aa3ab66b875b8ab9648688584f333416

Didn't rebase properly. Again: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=50988bab8d541cafffe4f49f642fdd003b7ed4db

Comment 14

[Josiah Bruner [:jsbruner]]

Comment on attachment 8980782 [details] [diff] [review]
bug8975283.patch

Try run looks good, so requesting review. 

Jorg, I know you aren't taking more minor reviews but since you reviewed the other bug I was hoping you'd make an exception. :) If not, please redirect to a better reviewer.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8980782 [details] [diff] [review]
bug8975283.patch

Thanks for the persistence!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/c6dbf51f6017
Remove SMTP password from cache when deleted from password manager to prevent stale connection attempts. r=jorgk

Comment 17

[Alex Ihrig [:Thunderbird_Mail_DE]]

From a supporters point of view, it would be nice to see this (and similar incoming server password patches) in Thunderbird 60.

Comment 18

[Jorg K (CEST = GMT+2)]

(In reply to AlexIhrig from comment #17)
> From a supporters point of view, it would be nice to see this (and similar
> incoming server password patches) in Thunderbird 60.
I'll uplift/backport this bug and its sister, bug 516464, to TB 60 now. If you're a supporter and well-versed in "all things Mozilla", you should know that anyone can request uplift by setting the flag "approval-comm-beta?" or "approval-comm-esr60?". I will then decide on the request.

Comment 19

[Jorg K (CEST = GMT+2)]

TB 60 beta 7 (BETA_60_CONTINUATION branch):
https://hg.mozilla.org/releases/comm-beta/rev/074ed27e1c152136815eb6fa119bd07494e1500c

Comment 20

[Alex Ihrig [:Thunderbird_Mail_DE]]

(In reply to Jorg K (GMT+2) (bustage-fix and urgent reviews only) from comment #18)
> If
> you're a supporter and well-versed in "all things Mozilla", you should know
> that anyone can request uplift by setting the flag "approval-comm-beta?" or
> "approval-comm-esr60?". I will then decide on the request.

Thanks for the explanation and backporting the patches

Comment 21

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/releases/comm-esr60/rev/daa94d432c60