588761 - [autoconfig] POP3 tryToLogon() sometimes returns success incorrectly. Account wizard can create accounts with wrong username/password.

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, defect)

Description

[Ben Bucksch (:BenB)]

Reproduction:
1. File | New | Mail Account...
2. email [e-mail] , password asdfkloi
3. Select "POP"
4. Click "Create Account..."
5. After the dialog closed, select the new account, and
   click "Get Mail" (you need to click it manually to see the error).

Actual result:
In step 4, the dialog shows "checking username/password", apparently indeed contacts the server, and after a while the dialog closes (which should happen only when the username and password were accepted by the server).

In step 5, you get a message dialog with the error:
"Sending of password did not succeed. Mail server pop.googlemail.com responded: Username and password not accepted."

So, Step 4 claimed the password was correct, but it wasn't. Step 4 failed to catch the problem, although it's designed to catch it.

Expected result:
Step 4: The account wizard dialog shows
"Login failed.
Mail server pop.googlemail.com responded:
Username and password not accepted."
The dialog does not close.

Parity:
Works for IMAP (more or less).

Implementation:
IDL function tryToLogon()

Comment 1

[David :Bienvenu]

protocol log?

Comment 2

[Ben Bucksch (:BenB)]

Can you first try whether the reproduction works for you?

Comment 3

[David :Bienvenu]

I've tried it many times in the past, and I tried it just now with other pop3 servers and the mechanism seems to work. Incorrect username and password resulted in the dialog telling me that the username & password combination was incorrect, for a pop3 server. You're saying it's always broken for you? When I press stop, I'm not given the opportunity to edit the account settings to use pop at all (this is with a trunk build from yesterday).

Comment 4

[Ben Bucksch (:BenB)]

> When I press stop,

No need, just follow the steps above, with @gmail.com . It offers you POP3.
If you still can't reproduce, I'll attach a log tomorrow.

Comment 5

[Ben Bucksch (:BenB)]

(and, FWIW, it has always been broken for me, as far as I can see. I saw it with other servers, too, and noticed it months ago already.)

Comment 6

[David :Bienvenu]

(In reply to comment #4)
> > When I press stop,
> 
> No need, just follow the steps above, with @gmail.com . It offers you POP3.
> If you still can't reproduce, I'll attach a log tomorrow.

Are we running the same code? It picks IMAP for me and always has (could it be a Europe vs US issue?). If I wait for it to finish, and click edit, and change it to pop3 and change the port to 995, and cross my fingers and pray, and pick retest configuration, it either does nothing, or it verifies the server. When I click create account, it tells me the user name and password combination is wrong.

Comment 7

[Ben Bucksch (:BenB)]

Attachment: POP3 protocol log, trunk

This is a log from a comm-central trunk nightly Linux from today.

I did the reproduction steps above, several times.
In the first or second attempt, I tried IMAP first, and it correctly said "invalid username/password", then I selected POP3, and it said the same. I aborted, tried again, this time straight POP3, and it created the account (incorrectly). Repeated, same. Eventually tried the IMAP first (got "invalid username"), POP afterwards, and there it created the POP account.

Comment 8

[Ben Bucksch (:BenB)]

In my 3.1 tree (trunk didn't build for me, for other reasons), based on a suggestion from bienvenu, I inserted, at the beginning of function OnStopRunningUrl() in verifyConfig.js, line 166 <http://mxr.mozilla.org/comm-central/source/mailnews/base/prefs/content/accountcreation/verifyConfig.js#166>
the following code:
    if (Components.isSuccessCode(aExitCode))
      alert("got success");
    else
      alert("got fail");
Then I tried again, and indeed I got a dialog saying "got success".
So, seems the problem is in the POP3 backend code, not in the account creation wizard.

Comment 9

[David :Bienvenu]

Correcting summary, since this is a somewhat random bug. If this is indeed a regression, my guess would be the async password changes...

Comment 10

[David :Bienvenu]

nominating for blocking 3.3 - it's bad, if you enter a bad password, or we guess the wrong username, thinking the password worked.

Comment 11

[Ben Bucksch (:BenB)]

Agreed. This easily leads to wrong configs being generated, because in our guess config code, the username format detection relies on the login properly failing or working.

Comment 12

[David :Bienvenu]

Attachment: proposed fix

This fixes it for me. I'll try to figure out what we can do for a unit test, since mozmill can't talk to fakeservers...

Comment 13

[David :Bienvenu]

we might want to consider this for 3.1.10 since it's a simple fix, and without it, username guessing is broken for pop3 since we think the first username is always right. This in turn is pretty bad for user retention.

Comment 14

[Ben Bucksch (:BenB)]

bienvenu, the patch makes sense to me, but I haven't tested it yet. (r=BenB, if that helps)
But wouldn't it be better - to avoid similar bugs in the future - to let the other code that *should* get a password, but can't due to a missing window, just fail with an error? Currently, we have lots of "if (msgWindow) { ...do all the stuff... }" and no proper |else|, e.g. in nsPop3Protocol::OnPromptStart() and nsMsgIncomingServer::GetPasswordWithUI(). I realize there's the biff case, but IMHO that should be more explicit.

Currently, we just give up silently when there's no window and a password failure, and that causes such bugs.

Comment 15

[David :Bienvenu]

I agree with you in principal about making the missing window case make those password functions fail, but I don't think it wouldn't fix this bug, because this code:
      if (TestFlag(POP3_PASSWORD_FAILED) && msgWindow)
      {
      /* We got here because the password was wrong, so go
        read a new one and re-open the connection. */

        m_pop3ConData->next_state = POP3_READ_PASSWORD;
        m_pop3ConData->command_succeeded = PR_TRUE;
        status = 0;
        break;
      }
prevents us from ever seeing failed logons.

I'm also seeing that we're not reprompting for the password with my isp's pop3 server in the get new mail case, when we have a msgWindow.

Comment 16

[David :Bienvenu]

(In reply to comment #15)

> I'm also seeing that we're not reprompting for the password with my isp's pop3
> server in the get new mail case, when we have a msgWindow.

And that's because my pop3 server drops the connection on a bad password, and we don't handle that.

Comment 17

[Ben Bucksch (:BenB)]

> because this code:
>      if (TestFlag(POP3_PASSWORD_FAILED) && msgWindow)
>      /* We got here because the password was wrong, so go
>        read a new one and re-open the connection. */
> prevents us from ever seeing failed logons.

Yeah, that's what I meant. We should fix those places.

(BTW: I don't see that "&& msgWindow)" in the line you cite:
<http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3Protocol.cpp#4080>.)

Comment 18

[David :Bienvenu]

(In reply to comment #17)

> Yeah, that's what I meant. We should fix those places.
> 
> (BTW: I don't see that "&& msgWindow)" in the line you cite:
> <http://mxr.mozilla.org/comm-central/source/mailnews/local/src/nsPop3Protocol.cpp#4080>.)

That's because that's in the fixed code, not the existing code :-)

Comment 19

[Ben Bucksch (:BenB)]

Oh, sorry, I was confused :)

Comment 20

[David :Bienvenu]

Attachment: fix with unit test

this adds a unit test, and adds dropOnAuth to the pop3 fake server. I still have to look into some remaining issues, in particular, verifyLogon still falsely returns true if there are multiple auth mechanisms supported by the server, but we drop the connection on failure, and the aforementioned bug in re-prompting for password in the bad password case when the server drops the connection.

Comment 21

[David :Bienvenu]

Comment on attachment 522778 [details] [diff] [review]
fix with unit test

Handling the reprompting for password when auth fails is going to be orthogonal to this fix, except that I'll need the dropOnAuth changes to the pop3 fakeserver for the testing.

Comment 22

[Ben Bucksch (:BenB)]

Comment on attachment 522778 [details] [diff] [review]
fix with unit test

As said above, looks good to me, with the following reservations.
- We should fix the other code in a follup bug.
- I didn't test it yet
- Shouldn't the |nsCOMPtr<nsIMsgWindow> msgWindow;| be right before its use,
  to avoid unnecessary creation of the variable?
- It seems to me, from a superficial look, that the new test is
  duplicating existing tests, e.g.
  - test_pop3AuthMethods.js (no UI)
  - test_pop3PasswordFailure.js (with UI)
  - test_pop3ServerBrokenCRAMDisconnect.js [1] (drop connection)
  Wouldn't it be better to adapt or copy one of these instead of starting
  from scratch?
- test_pop3ServerBrokenCRAMDisconnect.js [1] already implements the
  "drop on auth", in another way. I would prefer that approach,
  although I'd also accept yours (you did the same in SMTP fake server).

[1] http://mxr.mozilla.org/comm-central/source/mailnews/local/test/unit/test_pop3ServerBrokenCRAMDisconnect.js

Comment 23

[David :Bienvenu]

(In reply to comment #22)
> Comment on attachment 522778 [details] [diff] [review]
> fix with unit test
> 
> As said above, looks good to me, with the following reservations.
> - We should fix the other code in a follup bug.
Yes, I'm working on that.
> - I didn't test it yet
> - Shouldn't the |nsCOMPtr<nsIMsgWindow> msgWindow;| be right before its use,
>   to avoid unnecessary creation of the variable?
Yeah, I did that first, but it generates a compile error because it would be inside a case statement. Ah, I guess I can add stupid braces to quiet the error.
> - It seems to me, from a superficial look, that the new test is
>   duplicating existing tests, e.g.
>   - test_pop3AuthMethods.js (no UI)
>   - test_pop3PasswordFailure.js (with UI)
>   - test_pop3ServerBrokenCRAMDisconnect.js [1] (drop connection)
>   Wouldn't it be better to adapt or copy one of these instead of starting
>   from scratch?
verifyLogon is a bit different because there's an initial msg window and no reprompting because we clear the msgWindow right after starting the URL. It's trying to simulate the autoconfig wizard. Don't worry, I'm duplicating the passwordFailure.js test for the other bug about never prompting for a password when it's a bad password and the server drops the connection :-)

> - test_pop3ServerBrokenCRAMDisconnect.js [1] already implements the
>   "drop on auth", in another way. I would prefer that approach,
>   although I'd also accept yours (you did the same in SMTP fake server).

I think this is testing a slightly different server behavior, such that the server only drops the connection on bad auth, not every auth, like the cram-md5 case.

Comment 24

[David :Bienvenu]

Attachment: variable declaration inside switch statement

I moved the decl of msgWindow closer to where it's used...thx for reviewing this!

Comment 25

[Ben Bucksch (:BenB)]

> it generates a compile error because it would be inside a case statement.

Oh, I didn't know that, sorry. The old code is fine, then, or whatever you prefer.

Comment 26

[Ben Bucksch (:BenB)]

> verifyLogon is a bit different because there's an initial msg window

Why's that, BTW?

Comment 27

[David :Bienvenu]

(In reply to comment #26)
> > verifyLogon is a bit different because there's an initial msg window
> 
> Why's that, BTW?

iirc, it has to do with setting up the cert exception handling mechanism, which is derived from the initial msgWindow's docShell.

I wanted to make my test as much like the account wizard because we can't currently test that part of the account wizard with mozmill.

Comment 28

[David :Bienvenu]

fixed on trunk - http://hg.mozilla.org/comm-central/rev/dbb11495c869

I'm not 100% sure this is what I was seeing earlier, but the issue fixed by this patch is 100% reproducible and fixed by the patch.

Comment 29

[Ben Bucksch (:BenB)]

Thanks, bienvenu.

Comment 30

[Ben Bucksch (:BenB)]

continuation see bug 671965