All users were logged out of Bugzilla on October 13th, 2018

Update username on Login Manager when it is changed in AccountManager

RESOLVED FIXED in Thunderbird 53.0

Status

--
minor
RESOLVED FIXED
9 years ago
2 years ago

People

(Reporter: tanstaafl, Assigned: aceman)

Tracking

Trunk
Thunderbird 53.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

4.10 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3
Build Identifier: 3.0.4

If I have an account that initially required only the local part of the email address for the Login username, and the provider changes to requiring the full email address, when I change the Login username to add the @example.com part, TB doesn't even *attempt* to use the already saved password, it simply deletes it and requires you to re-enter it.

I have only confirmed this for IMAP accounts...

Reproducible: Always

Steps to Reproduce:
1. Add an imap account that accepts either/both just the localpart of your email address, or the full email address as the username

2. Create the account and save the password with just the local part of the email address for the username field, and confirm it is working.

3. Restart TB, then change the username to include the full email address.
Actual Results:  
Check your email - TB will prompt you for the password again

Expected Results:  
TB should at least *try* to use the new username with the already stored password.
(Reporter)

Comment 1

9 years ago
Maybe this should be reclassified as a bug, since I just confirmed that when changing the username for the *SMTP* (Outgoing) server, TB *does* retry with the new username and existing password, so it 'just works' without prompting to re-enter the same password again.
Severity: enhancement → minor
(Reporter)

Updated

9 years ago
Version: unspecified → 3.0
So you are saying that it's true for imap and not for smtp ? Was autosync enabled in your configuration (and thus the password was not cached or something) ?
(Reporter)

Comment 3

9 years ago
(In reply to comment #2)
> So you are saying that it's true for imap and not for smtp?

Yes...

> Was autosync enabled in your configuration (and thus the password was
> not cached or something) ?

No. I do set certain folders to offline use, but have GLODA permanently disabled via user.js.

Sadly I'll never be able to use GLODA, since it requires all IMAP stores to by fully sync'd locally, and I have 16+ IMAP accounts, most of which contain many GB's of emails making local syncing impractical.

Comment 4

8 years ago
(In reply to comment #3)
> (In reply to comment #2)
> > So you are saying that it's true for imap and not for smtp?
> 
> Yes...

bienvenu ^^


Charles \/

> > Was autosync enabled in your configuration (and thus the password was
> > not cached or something) ?
> 
> No. I do set certain folders to offline use, but have GLODA permanently
> disabled via user.js.
> 
> Sadly I'll never be able to use GLODA, since it requires all IMAP stores to by
> fully sync'd locally, and I have 16+ IMAP accounts, most of which contain many
> GB's of emails making local syncing impractical.

glodaquilla extension can help fine tune what you sync.

also, lots of very large messages can bloat the gloda index, and cause poor performance. fixed in v3.1 Bug 543737 - gloda needs to avoid indexing ridiculously large message bodies

And there is of course your Bug 508276 - IMAP - UI for Skipping Large Attachments (only downloaded on demand)
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > So you are saying that it's true for imap and not for smtp?
> > 
> > Yes...

Contrary to this bug, I do think that it is reasonable that we don't remember the password when you change the username - for the vast majority of cases (and I expect just changing username is rare anyway) I would expect that the password would be different, because the user is effectively switching to a different account.

Therefore I would probably say that SMTP not loosing passwords is probably the real bug, as we should be consistent across all of them.

I'd welcome bienvenu's thoughts here though.

Comment 6

8 years ago
I agree that using an old password for a new username is not a great thing to do. If we were super smart, we could detect that the username has made this very specific change from user to user@host and migrate the password.  That would be the kind of thing we'd accept a patch for, I suppose.

Or, in this scenario, it might be good to leave the old password in the password manager so the user can go retrieve it by hand.
(Reporter)

Comment 7

8 years ago
Why complicate things?

TB already knows how to handle failed auth attempts (and thankfully can now tell the difference between an auth failure and a failure to connect to the server), so just keep the password and let TB do its thing.

Comment 8

8 years ago
(In reply to comment #7)
> Why complicate things?
Passwords are looked up by the corresponding username and server, so we *are* doing the least complicated thing now.
(Reporter)

Comment 9

8 years ago
(In reply to comment #8)
> (In reply to comment #7)
> > Why complicate things?
> Passwords are looked up by the corresponding username and server, so we *are*
> doing the least complicated thing now.

Oh, ok... I was thinking the 'Site' is basically the 'account and so it looked it up by 'account'...

Ok, well, I'd be ok with

(In reply to comment #6)
> If we were super smart, we could detect that the username has made this
> very specific change from user to user@host and migrate the password.

Or just ignore the @tld part? The point of this bug was a provider changing a username from just the localpart to the full email address.

I'd think this should be trivial with a very simple regex added to the lookup?

> Or, in this scenario, it might be good to leave the old password in the
> password manager so the user can go retrieve it by hand.

I don't have the problem of forgetting passwords (since I use the Passwordmaker extension - awesome extension, by the way, if you have a lot of passwords), but yeah, this might be useful to some people...
(Reporter)

Comment 10

8 years ago
(In reply to comment #9)
> Ok, well, I'd be ok with

Sorry, meant to delete that...

Updated

8 years ago
Component: Preferences → Security
QA Contact: preferences → thunderbird

Comment 11

7 years ago
confirming based on comment 6
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: Changing username unconditionally loses the associated password → Update username on Login Manager when it is changed in AccountManager
Component: Security → Account Manager
(Assignee)

Comment 12

4 years ago
TB must at least ask before changing the username. What if the user has several accounts on the server and just switching them around in one account in the account manager (think TB developer:)).
It seems the UI (password manager) can retrieve which usernames are stored for which servers. So if there is no special "secured" access needed to retrieve this info also from the account manager, this feature could be done.
Component: Account Manager → Account Manager
OS: Windows XP → All
Product: Thunderbird → MailNews Core
Hardware: x86 → All
Version: 3.0 → Trunk
Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.
(Assignee)

Comment 14

2 years ago
But it is true that currently there is code to intentionally remove the stored username/password on username or hostname change.

In the same function we already detect if only username changed and if the previous one was the same as the part before @ of the new one.
https://dxr.mozilla.org/comm-central/rev/6e7067af19ca683b85b5860776f803704baa0fdb/mailnews/base/util/nsMsgIncomingServer.cpp#1198

So I think what this bug requests is doable.
Assignee: nobody → acelists
(Assignee)

Comment 15

2 years ago
Created attachment 8813832 [details] [diff] [review]
patch

This should do it.
Attachment #8813832 - Flags: review?(mkmelin+mozilla)
(Assignee)

Comment 16

2 years ago
Notice we do not need to update the username in the password manager, just keep the existing entry. We lookup the entry via the old hostname and old username (but send the real one when logging in).

That is until we make something in bug 302388.
Status: NEW → ASSIGNED
Comment on attachment 8813832 [details] [diff] [review]
patch

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

::: mailnews/base/util/nsMsgIncomingServer.cpp
@@ +1217,5 @@
> +  // If username changed and the new username just added a domain we can keep password.
> +  if (!hostnameChanged && (atPos != kNotFound)) {
> +    if (StringHead(NS_ConvertASCIItoUTF16(newName), atPos)
> +                  .Equals(NS_ConvertASCIItoUTF16(oldName))) {
> +      keepPassword = true;

Seems you don't need the keepPassword variable, just do ForgetPassword(); here

And no need for the if-if either, just if &&
Attachment #8813832 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 18

2 years ago
(In reply to Magnus Melin from comment #17)
> Comment on attachment 8813832 [details] [diff] [review]
> patch
> 
> Review of attachment 8813832 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: mailnews/base/util/nsMsgIncomingServer.cpp
> @@ +1217,5 @@
> > +  // If username changed and the new username just added a domain we can keep password.
> > +  if (!hostnameChanged && (atPos != kNotFound)) {
> > +    if (StringHead(NS_ConvertASCIItoUTF16(newName), atPos)
> > +                  .Equals(NS_ConvertASCIItoUTF16(oldName))) {
> > +      keepPassword = true;
> 
> Seems you don't need the keepPassword variable, just do ForgetPassword();
> here

What if hostnameChanged == true?

> And no need for the if-if either, just if &&

OK.
(In reply to :aceman from comment #18)
> What if hostnameChanged == true?

Right, you need to negate things. But the point was you set keepPassword but only use it once, so the variable is unneded.
(Assignee)

Comment 20

2 years ago
Created attachment 8814700 [details] [diff] [review]
patch v2

You mean like this?

The patch fails to apply in the test file because it depends on bug 1308767 first.
Attachment #8813832 - Attachment is obsolete: true
Attachment #8814700 - Flags: review?(mkmelin+mozilla)
Attachment #8814700 - Flags: review?(mkmelin+mozilla) → review+
(Assignee)

Comment 21

2 years ago
Thanks.
Keywords: checkin-needed
(Assignee)

Comment 22

2 years ago
https://hg.mozilla.org/comm-central/rev/77fd6aa4aa5da29f62dbce828cc62bd1ac2d2fab
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 53.0
You need to log in before you can comment on or make changes to this bug.