Closed Bug 1461106 Opened 2 years ago Closed 2 years ago

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

Categories

(Thunderbird :: Security, defect)

defect
Not set
minor

Tracking

(thunderbird_esr60 fixed, thunderbird60 fixed, thunderbird61 wontfix, thunderbird62 fixed)

RESOLVED FIXED
Thunderbird 62.0
Tracking Status
thunderbird_esr60 --- fixed
thunderbird60 --- fixed
thunderbird61 --- wontfix
thunderbird62 --- fixed

People

(Reporter: jsbruner, Assigned: jsbruner)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch WIP Patch (obsolete) β€” β€” Splinter Review
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.
Any update here? The patch looks good, would you like a try run?
Flags: needinfo?(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)
Flags: needinfo?(jsbruner)
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://mozilla@jorgk.com@hg.mozilla.org/try-comm-central| in hgrc.
(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
(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 :-(
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!
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?
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.
Attached patch bug8975283.patch β€” β€” Splinter Review
This fixes the "missing implementation" assertions. I think it adequately resolves this bug, but I will start another try run to verify.
Attachment #8975283 - Attachment is obsolete: true
(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 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.
Attachment #8980782 - Flags: review?(jorgk)
Comment on attachment 8980782 [details] [diff] [review]
bug8975283.patch

Thanks for the persistence!
Attachment #8980782 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
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
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 62.0
From a supporters point of view, it would be nice to see this (and similar incoming server password patches) in Thunderbird 60.
Attachment #8980782 - Flags: approval-comm-esr60+
Attachment #8980782 - Flags: approval-comm-beta+
(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.
(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
You need to log in before you can comment on or make changes to this bug.