Mark Reporter
	Description • 7 years ago

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/61.0.3163.100 Safari/537.36

Steps to reproduce:

1.  Have Thunderbird configured with your Yahoo IMAP and SMTP account settings.
2.  Log into Yahoo Email - https://login.yahoo.com/
3.  Update Yahoo Password - Account > Account Info > Account Security > Change Password
4.  Open Thunderbird and click on "Get Messages" > "Get All New Messages"


Actual results:

A Thunderbird error prompt is shown with the following exception (See attached screenshot).

The current operation on 'Inbox' did not succeed.  The mail server for account XXXX@XXXX responded: [CLIENTBUG] SELECT Command is not valid in this state

All folders for the Yahoo Mail account are then deleted including the Inbox, Draft, Sent, Archive, Buld Mail and Trash folders.

My Yahoo Mail account holds around 15 GB of data which get's wiped.

-- The Fix --

https://forums.yahoo.net/t5/Account-security/CLIENTBUG-SELECT-command-is-not-valid-in-this-state/td-p/300162

1. Go to Account Settings --> Server Settings --> Security Settings
2. Change Authentication Method to something different than the one you have
3. Close and re-open Thunderbird; not working
4. Change Authentication Method back to "Normal Password"
5. Close and re-open Thunderbird; working
6. File --> Offline --> Download/Sync Now...


Expected results:

Thunderbird should have prompted me for my updated Thunderbird password.

	Mark Reporter
	Comment 1 • 7 years ago

Sorry typo.

Expected results:

Thunderbird should have prompted me for my updated Yahoo password.

	Mark Reporter
	Comment 2 • 7 years ago

Sorry I made a mistake with my previous comment.

-- The Fix --

1.  Tools > Options > Security > Saved Passwords...
2.  Search - yahoo
3.  Remove All
4.  Close

	gene smith Assignee
	Comment 4 • 7 years ago

Reporter, please look at my last 3 comments in the referenced duplicate bug and report there if it fixes your problem. We haven't heard back from the reporter there. Thanks

	gene smith Assignee
	Comment 5 • 7 years ago

Reporter never mind I see you got it working but no pwd prompt by tb. So maybe not exactly a dupe.

	Mark Reporter
	Comment 6 • 7 years ago

Sorry about the confusion with my post.  I will try to clarify as I don't think this is a duplicate issue.

1.  Basically, I had Thunderbird configured with my Yahoo Account (Standard Web Login Username + Password + Allow apps that use less secure sign-in), working as expected for years.

2.  I updated my Yahoo Password online.

3. The next time I attempted to sync my Yahoo Account using my Thunderbird client I was given the error as described above ([CLIENTBUG] SELECT Command is not valid in this state), and all my Yahoo Account folders were automatically removed.  I was not prompted for an updated password.

4.  I tried many things to re-connect my Yahoo account, though I was never prompted for my updated password until I went to Tools > Options > Security > Saved Passwords... > Search - yahoo > Remove All > Close.

5.  I was then prompted for my Yahoo account password, and my Yahoo folders were re-created and the download process started.  Thunderbird then starting the sync process from scratch (And having to download around 15GB from Yahoo).

	gene smith Assignee
	Comment 7 • 7 years ago

What I saw happening, and I think is happening for you, and as I tried to document in bug 1394642, is that after you changed your password at yahoo, tb can't know that, so it tried to authenticate using keys based on your old password. For some reason yahoo accepts the resulting imap "authenticate plain" with no error. Had there been an error here you would have been prompted to enter a new password. However, tb goes on and does other imap commands like SELECT and yahoo fails all of them so you see no emails listed (and you probably can't send emails either via smtp).

So this just seems like strange behavior by yahoo (*) rather than an actual tb bug. I also determined that you can force tb to prompt for a new password by deleting the stored password. There is no direct way to tell tb to use a new password except when you create or re-create an account.

I am surprised that all your local copies of emails were deleted and had to be re-download once the new password was entered and accepted. Unless you deleted the account in tb and told it to delete the data too, all your old emails should still be stored under ImapMail/ in your profile area and not have to be re-downloaded.

(*) There have been other reports of inconsistent authentication by yahoo depending on the ip address you actually connect to but I don't know the bug number(s).

	gene smith Assignee
	Comment 8 • 7 years ago

> I also determined that you can force tb to prompt for a new password
> by deleting the stored password. 

I may be wrong. You don't have to delete the password. You can click on the visible password in the stored passwords dialog and change it there. Haven't tried it but it looks like it should work.

	Mark Reporter
	Comment 9 • 7 years ago

The biggest problem was that when I attempted to check my mailbox (Get Messages > Get All New Messages) and I received the error "[CLIENTBUG] SELECT Command is not valid in this state", that it was at this time that my all my Yahoo email folders disappeared 
 and were deleted.

Might as well have deleted my whole account, as it would have been simpler to recover from.

	gene smith Assignee
	Comment 10 • 7 years ago

I think you are correct and there is a real tb bug here. I was last setup with yahoo to use 2-step auth, don't allow insecure apps and an app-specific password generated by yahoo (which is just entered in as a normal password in tb). I turned off don't allow insecure apps at yahoo sec. site. Tb then failed to connect with either my normal yahoo password or my app-specific password. I could see in the wireshark trace that the imap "authenticate plain" failed but tb went ahead and tried other IMAP commands that resulted in [CLIENTBUG] responses from yahoo. So I changed my normal yahoo password and was able to do imap with yahoo. So now I am in the same mode as you are, I think.

So even though the yahoo auth response is "NO" tb goes on does imap commands but never prompts for a new password unless you delete the yahoo entry in the password manager AND restart tb.

Tb also deletes the existing stored emails when it gets [CLIENTBUG] responses like you see. I think this is because all the yahoo folders become unsubscribed and unsubscribe deletes the locally stored emails when a folder is unsubscribed.

So tb should prompt for a new password when authentication fails. Currently the only way to get a prompt is to remove the entry from the password manager. Also, you must restart tb for any change in pwd mgr to take effect it seems.

If authentication fails, tb should not continue on with other IMAP commands that are not allowed by imap rfc unless the session is in authenticated state. If auth fails, the account should just transition to tb "offline" mode instead of all the folders vanishing and locally stored emails being deleted.

I must have been wrong in by analysis of Bug 1394642. I thought the auth was accepted by yahoo and yahoo was just sending [CLIENTBUG] responses irrationally. I will check this some more tomorrow. (Unfortunately, if you fool with yahoo security setting too much in too short a time they lock you out for an unspecified time.)

	gene smith Assignee
	Comment 12 • 7 years ago

I removed all login info stored for yahoo from tb's password manager. Now each time tb starts up I type in my password, without storing the password, and yahoo emails work OK. Then as a test I type in the wrong yahoo password at tb startup. The tb takes the password and then generates a hashed/encrypted password string and then does an imap "Authenticate plain" command through the now connected TLS tunnel to the yahoo server. Yahoo responds with "+" and tb then sends the hashed password. Yahoo then responds with "NO [AUTHENTICATIONFAILED] Authenticate invalid credentials". But tb basically ignores this and continues on try to list folders and select inbox via imap commands. These subsequent imap commands results in the [CLIENTBUG] response from yahoo that in turn causes tb to "delete" all the yahoo folders and locally stored emails.

For yahoo "login succeeded" gets printed in the log *before* the NO response arrives. That's why it thinks it's authenticated and does commands only allowed in authenticated imap state, which the server rightly points out is a CLIENTBUG.

I tried another account and put in a bad password for it. It only had "login" capability so it sends the literal username and password through the tunnel. In this case, there is a NO LOGIN FAILED response and the message "login succeeded" is not printed and additional password prompts for a correct password occur. Also, for this account a bad password does not result in folders and local emails being removed.

At this point I don't know why there is a difference between these two methods. I will look some more at this tomorrow.

	gene smith Assignee
	Comment 13 • 7 years ago

I see the problem with yahoo. S=imap server, C=client/tb.:

When good uid and password have been entered in tb:
C: aaa authenticate plain
S: +
C: <sends literal base64 encoded uid/password>
S: aaa OK Your in!
<tb then logs: "Login succeeded". All additional imap command succeed.

When bad uid and password have been entered in tb, non-yahoo servers do this:
C: aaa authenticate plain
S: +
C: <sends literal base64 encoded uid/password>
S: NO bad credentials
<tb then logs: "Authlogin fails" and tb attempts no imap commands valid only in auth state>

But yahoo/tb does this when a bad password is provided by tb:

C: aaa authenticate plain
S: +
C: <sends literal base64 encoded uid/password>
S: + {"status":"invalid_credentials","scheme":"basic","scope":"https://mail.yahoo.com/"}
<tb then logs "Login succeeded">
C: bbb namespace
S: aaa NO [AUTHENTICATIONFAILED] your credentials are bad
C: ccc SELECT Inbox
S: BAD [CLIENTBUG] send for most responses that require auth state.

(The message after the 2nd '+' is actually base64 encoded text that I have shown decoded back to normal ascii. Just to be clear, the + sent by the server is specified in the imap rfc to mean "I am expecting more literal data, please send it".)

The problem is that tb doesn't expect the 2nd + response and the text that follows and ignores it and decides the login is successful *before* it even sees the "aaa NO" response. So it goes ahead and does a "namespace" command that the server sees as the requested re-transmit of the credentials. The server then responds with NO but tb ignores this and just continues on. It tries to list folders and select Inbox that all fail which causes tb to not show them and to actually deleted the local emails in those folders. Note: deletion of local email storage also occurs when you unsubscribe a folder so this is not unexpected behavior since the folder appear to be gone based on server responses.

Tb also fails to prompt for a new password because thinks the login passed.

A fix for this is to not ignore the 2nd + and re-send the bad credentials. Here's what happens when this occurs:

C: aaa authenticate plain
S: +
C: <sends literal base64 encoded uid/password>
S: + {"status":"invalid_credentials","scheme":"basic","scope":"https://mail.yahoo.com/"}
C: <sends literal base64 encoded uid/password>
S aaa NO [AUTHENTICATIONFAILED] your credentials are bad
<tb then logs "Authlogin failed">

Now tb knows authentication failed. It does not remove folders or locally stored email. Prompt for corrected password occurs when any folder is access. Locally stored email is readable.

A soon to be made patch will show an implementation of this fix.

	gene smith Assignee
	Comment 14 • 7 years ago

This fixes the problem by checking that the server is waiting for an authentication string re-send. If so, tb now just re-sends the same string. If the 2nd send fails (which it probably will unless the first send was actually caused by comm errors and not a bad password) tb then tries the slightly less secure "authenticate login" method. If that fails, tb then prompts for a corrected password and tries "authenticate plain/login" again and continues to prompt until a good password is entered or the user cancels the password entry pop-up dialog.

I am allowing for a maximum of 2 retries (retrys) of the "auth plain" string send in case yahoo (or another imap server) should decide to prompt more than one time after a bad auth string is sent. On the 3rd retry it now just fails the authentication instead of waiting for an imap "NO" response which cause tb to prompt the user for a new password. 

Note: An alternate fix is to just force an immediate fails when the server requests the first auth string re-send. This would cause tb to fail the auth and then, after a timeout waiting for the string, the server will send the "NO" response, which at that point is ignored. This is also what happens when the retries are exhausted.

	Jorg K (CEST = GMT+2)
	Comment 15 • 7 years ago

Comment on attachment 8919542 [details] [diff] [review]
1408610-review-changes0.patch

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

Some comments.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +5909,5 @@
>        PR_Free(base64Str);
>        rv = SendData(m_dataOutputBuf, true /* suppress logging */);
>        if (NS_SUCCEEDED(rv))
>          ParseIMAPandCheckForNewMail(currentCommand);
> +      int retrys = 0;

retries, here and below (https://www.wordhippo.com/what-is/the-plural-of/retry.html)

@@ +5912,5 @@
>          ParseIMAPandCheckForNewMail(currentCommand);
> +      int retrys = 0;
> +      while (GetServerStateParser().WaitingForMoreClientInput())
> +      {
> +        if (retrys < 2)

Consider using retries++ here.

@@ +5917,5 @@
> +        {
> +          // Server is asking for authentication string again. Allow the
> +          // server to request a limited number of retrys before failing
> +          // authentication.
> +          MOZ_LOG(IMAP, LogLevel::Debug, ("gds: re-sending pwd"));  // temp test gds

Logging is OK, but please with a non-personalised string ;-)
Here and below.

@@ +5921,5 @@
> +          MOZ_LOG(IMAP, LogLevel::Debug, ("gds: re-sending pwd"));  // temp test gds
> +          rv = SendData(m_dataOutputBuf, true /* suppress logging */);
> +          if (NS_SUCCEEDED(rv))
> +          {
> +            ParseIMAPandCheckForNewMail(currentCommand);

break missing here?

@@ +5927,5 @@
> +        }
> +        else
> +        {
> +          // Too many retrys. Force error detection upon return below.
> +          GetServerStateParser().SetConnected(false);

Is this necessary? Previously it would just fall through with a failure in rv from SendData().

	gene smith Assignee
	Comment 16 • 7 years ago

> break missing here?

I don't think so. At this point want to go back and check if server is wanting the password sent again; if not, the loop is exited and return value is set at the bottom. The break below occurs when retries are exceeded. 
However, it should break out if the SendData() fails that I've put in the updated patch.

> Is this necessary? Previously it would just fall through with a failure in rv from SendData().

Actually, SendData() always succeeded in my testing so rv returned by SendData() is NS_OK. The problem is that yahoo server is expecting and waiting for a re-send of the password but tb just moves on does not return a failure from AuthLogin() since the SendData() rv is NS_OK. 

Currently yahoo only asks for a password and, if bad, it asks for a re-send. If the re-send fails too, yahoo does not wait any more. The loop is assuming that there may be a server that will ask for more than one re-send, say 10 to be extreme, or maybe there is a server with no limit, who knows. So to ensure that AuthLogin() returns an error on the 3rd re-send request and does not get stuck here, the error is returned. Rather than calling "GetServerStateParser().SetConnected(false);" it would be easier to cause a return of NS_ERROR_FAILURE at the NS_ENSURE_SUCCESS(rv, rv); near the bottom. I've done this in the revised patch.

There was a simpler fix that I tried. Just set the error and return when "waiting for a re-send" is detected and don't re-send the auth string. This eliminates the loop and resend count. This works but the subsequent tries of other auth methods (*) requests and responses get interleaved instead of happening in a straight line manner and looks confusing in the imap log and in wireshark.

> retries, here and below (https://www.wordhippo.com/what-is/the-plural-of/retry.html)

Yes, I know it's spelled wrong. Just looked better to me as "retrys". Calling it "resends" in updated patch. Hope that's spelled right :).

> Logging is OK, but please with a non-personalised string ;-)
> Here and below.

Took one of them out and removed the "gds:" in the revised patch. Also, I only intended them as a temporary debug device, hence the // temp test gds.

> Consider using retries++ here.

Yes, moving that up near the SendData() call looks better since that's what it's counting. Haven't done that in the revised patch but will in the next if we stay with this general method.

The revised patch also now only enters the resend loop if the initial auth send succeeds. (But as I mentioned above, I've never seen SendData() fail.)

Also, the max number of resends maybe should be a #define somewhere instead of just 2 or maybe even a preference value?

(*) When tb has a bad password, with yahoo the following authentication methods occur:

authenticate PLAIN  <--- yahoo/tb issue is only for PLAIN
authenticate LOGIN
login <uid> <pwd>

	Wayne Mery (:wsmwk)
	Comment 17 • 7 years ago

some possible duplicates https://mzl.la/2yH9O94

	gene smith Assignee
	Comment 18 • 7 years ago

(In reply to Wayne Mery (:wsmwk) from comment #17)
> some possible duplicates https://mzl.la/2yH9O94

The first two I entered comments for and neither are dups. The 3-5 are all POP related so not dups. 6th one is this bug. Last one involves oauth2 on gmail that seems to work fine for me and also not a dup.

	Jorg K (CEST = GMT+2)
	Comment 19 • 7 years ago

Hmm, right now, the list has only six bugs:
Bug 1309327, bug 1337445, bug 1338754, bug 516651, bug 1408610, bug 1176399.
I can only see Gene's comment in the first one, but not the second. Did that second one drop off the list?

	gene smith Assignee
	Comment 20 • 7 years ago

Sorry, must have counted wrong. 2-4 seem to be pop related, although info for 2 is a bit sketchy. 5 is this bug and 6 is gmail oauth2.

	Jorg K (CEST = GMT+2)
	Comment 21 • 7 years ago

Before I complain about nits, I took the liberty to reshuffle it myself. Please let me know if you like it.

	gene smith Assignee
	Comment 22 • 7 years ago

Looks fine to me. Don't know if it should be mentioned in the code but will at least add a note here: When password is re-sent it is always the exact same string and not a "corrected" password entered by the user. Yahoo is probably expecting a correction but there is no facility in tb, that I know of, that allows this during an imap authentication transaction.

	Jorg K (CEST = GMT+2)
	Comment 23 • 7 years ago

So why not just add this:

+      // Send authentication string (true: suppress logging the string)
+      rv = SendData(m_dataOutputBuf, true);
       if (NS_SUCCEEDED(rv))
+      {
         ParseIMAPandCheckForNewMail(currentCommand);
+        if (GetServerStateParser().WaitingForMoreClientInput())
+        {
+          // Server is asking for authentication string again, apparently
+          // is wasn't happy. All we could do is to send the same
+          // non-happy-making string again, which is pointless, so let's give up here.
+          rv = NS_ERROR_FAILURE;
+        }

	gene smith Assignee
	Comment 24 • 7 years ago

That's what I suggested in Comment 16:  

quote:
There was a simpler fix that I tried. Just set the error and return when "waiting for a re-send" is detected and don't re-send the auth string. This eliminates the loop and resend count. This works but the subsequent tries of other auth methods (*) requests and responses get interleaved instead of happening in a straight line manner and looks confusing in the imap log and in wireshark.
:end quote

Did you think the patch was sending a possibly different password in the re-sends?

Anyhow, if you prefer the simple method, we can go that way.

	Jorg K (CEST = GMT+2)
	Comment 25 • 7 years ago

(In reply to gene smith from comment #24)
> Did you think the patch was sending a possibly different password in the
> re-sends?
No. I only looked at the patch in more detail yesterday and asked myself why we sent the same stuff multiple times if the first round already didn't work.

> Anyhow, if you prefer the simple method, we can go that way.
Yes, I prefer simple. You wrote:
===
This works but the subsequent tries of other auth methods (*) requests and responses get interleaved instead of happening in a straight line manner and looks confusing in the imap log and in wireshark.
===

It's complicated computer software, so things will look a little confusing. I just don't see how sending the "wrong" thing once and repeat twice (three times in total) will make the logs look clearer. You'll still get some interleaving from the last failed repetition with the "wrong" password with the new different auth method attempt, no?

Or are their cases where you repeat once and then get an error and carry on to try the next thing?

	gene smith Assignee
	Comment 26 • 7 years ago

I looked closer at the "interleaving" with the simple method and here's what happens (yh == yahoo):

tb:  1 authenticate PLAIN
yh:  +
tb:  <encoded auth string>
yh:  + <encode string saying auth string is bad>
tb:  2 authenticate LOGIN     <------ yahoo sees this as a password resend that it rejects with "1 NO ..."
yh:  1 NO [AUTHENTICATIONFAILED] AUTHENTICATE Invalid credentials
tb:  4 login <plain text uid>  <plain text pwd>
yh:  4 NO [AUTHENTICATIONFAILED] LOGIN Invalid credentials
tb prompts for corrected password

Yahoo responds to "2 authenticate LOGIN" with "1 NO ..." since it sees it as the auth string it is expecting. Tb detects the error and moves on to send "4 login ..." that gets rejected with "4 NO ...". So "authenticate LOGIN" is essentially skipped.

Now here's what happens with the less-simple method:

tb:  1 authenticate PLAIN
yh:  +
tb:  <encoded auth string>
yh:  + <encode string saying auth string is bad>
tb:  <encoded auth string>
yh:  1 NO [AUTHENTICATIONFAILED] AUTHENTICATE Invalid credentials
tb:  2 authenticate LOGIN                         
yh:  +
tb:   <encoded uid>
yh:  + <encoded string saying auth string is bad>  <--actually, just means now send password (*see below)
tb:  <encoded password>
yh:  2 BAD [AUTHENTICATIONFAILED] LOGIN Invalid credentials
tb:  4 login <plain text uid>  <plain text pwd>
yh:  4 NO [AUTHENTICATIONFAILED] AUTHENTICATE Invalid credentials
tb prompts for corrected password

With this method all the auth methods are tried if necessary and each runs sequentially to completions. Also, an error does not have to be forced with this method unless the the server reaches the resend limit (that is only there for the as yet never seen server that requests more than one resend for "authenticate PLAIN").

So if you still prefer the simple method I will make a new patch.

(*) The as-is tb code always expects this 2nd + response for "authenticate LOGIN".

	Jorg K (CEST = GMT+2)
	Comment 27 • 7 years ago

Thanks for the clarification. I think the less-simple method is indeed better. However, I see that we only need to retry once to keep things in sync, right? So how about:

+      // Send authentication string (true: suppress logging the string)
+      rv = SendData(m_dataOutputBuf, true);
       if (NS_SUCCEEDED(rv))
+      {
         ParseIMAPandCheckForNewMail(currentCommand);
+        if (GetServerStateParser().WaitingForMoreClientInput())
+        {
+          // Server is asking for authentication string again. So send the
+          // same string again although we already know that it will be rejected again.
           // We do that to get a firm authentication failure instead of
           // a re-send request. That keeps things in order before we try
           // different methods.
+          rv = SendData(m_dataOutputBuf, true );
+          if (NS_FAILED(rv))
+            break;
+          ParseIMAPandCheckForNewMail(currentCommand);
           if ((GetServerStateParser().WaitingForMoreClientInput())
             rv = NS_ERROR_FAILURE;
+        }
+      } // if SendData() succeeded

	Jorg K (CEST = GMT+2)
	Comment 28 • 7 years ago

Sorry, of course make that:
           // different methods.
+          rv = SendData(m_dataOutputBuf, true );
+          if (NS_NS_SUCCEEDED(rv))
           {
+            ParseIMAPandCheckForNewMail(currentCommand);
             if ((GetServerStateParser().WaitingForMoreClientInput())
               rv = NS_ERROR_FAILURE;

	Jorg K (CEST = GMT+2)
	Comment 29 • 7 years ago

What do you think of this? No code duplication, one retry, simple loop that always terminates, I only have it to be able to 'continue' for a second try.

Not compiled and untested! ;-(

	gene smith Assignee
	Comment 30 • 7 years ago

It looked like it would work to me and it compiles but it jumps out of the loop after continue. I though continue jumped to the top of the loop but it actually jumps to where the loop condition is checked. Since the condition is "while (false)" it falls right out.

I changed it like this and it works:

      bool firstTry = true;
      while (true)
      {
        // Send authentication string (true: suppress logging the string)
        rv = SendData(m_dataOutputBuf, true);
        if (NS_FAILED(rv))
          break;
        ParseIMAPandCheckForNewMail(currentCommand);
        if (GetServerStateParser().WaitingForMoreClientInput())
        {
          // Server is asking for authentication string again. So send the
          // same string again although we already know that it will be
          // rejected again. We do that to get a firm authentication failure
          // instead of a re-send request. That keeps things in order before
          // failing "authenticate PLAIN" and trying another method if capable.
          if (firstTry)
          {
            firstTry = false;
            continue;
          }
          // Return error if server asks for auth string more than 2 times.
          rv = NS_ERROR_FAILURE;
          break;
        }
        else
          // Normal exit point. Server asked for auth string 1 or 2 times.
          break;
      }

Has lot of breaks and the continue. Maybe you can make it "prettier". :)  But I personally prefer an "unwound" implementation like this, that also works and, I think, does the same thing:

      // Send authentication string (true: suppress logging the string)
      rv = SendData(m_dataOutputBuf, true);
      if (NS_SUCCEEDED(rv))
      {
        ParseIMAPandCheckForNewMail(currentCommand);
        if (GetServerStateParser().WaitingForMoreClientInput())
        {
          // Send authentication string again (true: suppress logging the string)
          rv = SendData(m_dataOutputBuf, true);
          if (NS_SUCCEEDED(rv))
          {
            ParseIMAPandCheckForNewMail(currentCommand);
            if (GetServerStateParser().WaitingForMoreClientInput())
            {
              // Server requesting 3rd authentication string resend. Too many.
              rv = NS_ERROR_FAILURE;
            }
          }
        }
      }

	Jorg K (CEST = GMT+2)
	Comment 31 • 7 years ago

(In reply to gene smith from comment #30)
> Since the condition is "while (false)" it falls right out.
Oops :-((

> Has lot of breaks and the continue. Maybe you can make it "prettier". :)
Will try. Give me 5 minutes.

> But I personally prefer an "unwound" implementation like this, that also
> works and, I think, does the same thing:
Yes, I started with that, but let's not code the same calls twice.

	gene smith Assignee
	Comment 32 • 7 years ago

I just now noticed that my "unwound" method is the same as your two comments above!

	Jorg K (CEST = GMT+2)
	Comment 33 • 7 years ago

I hope this is pretty enough ;-)

One loop, and a few clear breaks out of the loop:
1) on error, 2) if no further reply is required, 3) if it's already a resend.

	gene smith Assignee
	Comment 34 • 7 years ago

Comment on attachment 8922107 [details] [diff] [review]
1408610-review-changes1.patch (v5)

It works fine and looks good too. I say go with this patch if you don't need me to do anything else on this.

	gene smith Assignee
	Comment 35 • 7 years ago

I was curious as to what would happen if the first encoded auth string for "authenticate plain" was bad and the correct one was encoded in the re-send. This might simulate network corruption of the first auth string. Interesting thing is that they *both* fail. Then it moves on to "authenticate login" and it also fails! Finally it tries the simple imap login and only it works.

Here's what happens when tb talks to yahoo (yh):

tb:  1 authenticate PLAIN
yh:  +
tb:  <BAD encoded auth string> <--- I changed the 5th char to 'z', was 'd'
yh:  + <encode string saying auth string is bad>
tb:  <GOOD encoded auth string> <--- I returned the 5th char back to 'd'
yh:  1 NO [AUTHENTICATIONFAILED] AUTHENTICATE Invalid credentials
tb:  2 authenticate LOGIN                         
yh:  +
tb:   <GOOD encoded uid>
yh:  + <encoded string saying auth string is bad>  <--actually, just means now send password
tb:  <GOOD encoded password>
yh:  2 BAD [AUTHENTICATIONFAILED] LOGIN Invalid credentials
tb:  4 login <GOOD plain text uid>  <GOOD plain text pwd>
yh:  4 OK AUTHENTICATE complete
All is well...

I can also duplicate this with manually entered openssl commands. Don't know what's going on here. I think what it means is that including the same (bad or good) auth string in the resend really does no good. The 2nd + followed by the encoded error message is really telling you that your login is no good and you can acknowledge it just by sending anything back to the server, e.g., even just CRLF. 
Still need to determine why "authenticate LOGIN" doesn't work even with what seems like good uid and pwd.

	Jorg K (CEST = GMT+2)
	Comment 36 • 7 years ago

Comment on attachment 8922107 [details] [diff] [review]
1408610-review-changes1.patch (v5)

(In reply to gene smith from comment #34)
> It works fine and looks good too. I say go with this patch if you don't need
> me to do anything else on this.
Thanks for your patience while looking for the best solution here. I sent this on a try run
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=e0e10be130649451458173cf39ba0cbfc6697e1e
which was successful, so I'm going to land this later today.

	Pulsebot
	Comment 37 • 7 years ago

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/83f249160f6c
Resend auth string when imap server requests it. r=jorgk

	gene smith Assignee
	Comment 38 • 7 years ago

(In reply to gene smith from comment #35)

> Still need to determine why "authenticate LOGIN" doesn't work even with what
> seems like good uid and pwd.

Just to follow-up on this, I have 6-7 test imap accounts with different servers. Besides yahoo, only 1 of them even supports "authenticate LOGIN". When I use it with that server, it accepts the uid and password but then it prints "renegotiating" and seems to cause a fatal error in the server or openssl, not sure which. Also, found a post on imap mailing list saying that this auth method is obsolete and deprecated so not surprising that it often doesn't work. It is described in a draft rfc, but the example is for smtp, not imap: google "draft-murchinson-sasl-login".

Also, the patch as committed is still valid. No need to change it because of these findings.

	Jorg K (CEST = GMT+2)
	Comment 40 • 6 years ago

TB 52.7 ESR:
https://hg.mozilla.org/releases/comm-esr52/rev/1dd72405c2e271ae37a16950cc676b1aff5926df

	Bill Mattison
	Comment 42 • 6 years ago

See my comments added to bug 1447360 on Friday, March 23, 2017.

Thanks,
Bill.

	Wayne Mery (:wsmwk)
	Comment 43 • 6 years ago

Mark do you find that your problem is gone when using 52.7.0?

	Matt
	Comment 44 • 6 years ago

Do we have another bug for the loss of all mail/folders on authentication failure?  THat is not specific to Yahoo,  or at least I do not think so.  But it is undesirable.

	gene smith Assignee
	Comment 45 • 6 years ago

Bug 1453643 precludes folder loss on auth failure for PLAIN and OAUTH2. See discussion starting at Bug 1453643 Comment 6 for more info.

	Mark Reporter
	Comment 46 • 6 years ago

(In reply to Wayne Mery (:wsmwk) from comment #43)
> Mark do you find that your problem is gone when using 52.7.0?

I have been very busy so I have not had the time to specifically retest this issue and post my results.  From what I recall I had this issue reoccur once on a proposed "fixed" version, which was frustrating, though it looked like this thread was till being updated with comments from other people, so I just left it to others to carry forward.  Since then I have been very careful in handling password changes to avoid the reproducible steps I gave above.  Fortunately, I have not had this issue since.