561056 - Update username on Login Manager when it is changed in AccountManager

Status: RESOLVED FIXED

(MailNews Core :: Account Manager, defect)

Description

[Charles]

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.

Comment 1

[Charles]

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.

Comment 2

[Ludovic Hirlimann [:Usul]]

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) ?

Comment 3

[Charles]

(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

[Wayne Mery (:wsmwk)]

(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)

Comment 5

[Mark Banner (:standard8)]

(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

[David :Bienvenu]

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.

Comment 7

[Charles]

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

[David :Bienvenu]

(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.

Comment 9

[Charles]

(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...

Comment 10

[Charles]

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

Sorry, meant to delete that...

Comment 11

[Wayne Mery (:wsmwk)]

confirming based on comment 6

Comment 12

[:aceman]

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.

Comment 13

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

Comment 14

[:aceman]

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.

Comment 15

[:aceman]

Attachment: patch

This should do it.

Comment 16

[:aceman]

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.

Comment 17

[Magnus Melin [:mkmelin]]

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 &&

Comment 18

[:aceman]

(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.

Comment 19

[Magnus Melin [:mkmelin]]

(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.

Comment 20

[:aceman]

Attachment: patch v2

You mean like this?

The patch fails to apply in the test file because it depends on bug 1308767 first.

Comment 21

[:aceman]

Thanks.

Comment 22

[:aceman]

https://hg.mozilla.org/comm-central/rev/77fd6aa4aa5da29f62dbce828cc62bd1ac2d2fab