Merge pull request #6762 from thundernest/imap_authentication_failure

IMAP: Ignore errors during LOGIN fallback

app/ui/legacy/src/main/java/com/fsck/k9/activity/setup/AccountSetupCheckSettings.kt

@@ -415,7 +415,7 @@ class AccountSetupCheckSettings : K9Activity(), ConfirmationDialogFragmentListen
                 finish()
             } catch (e: AuthenticationFailedException) {
                 Timber.e(e, "Error while testing settings")
-                showErrorDialog(R.string.account_setup_failed_dlg_auth_message_fmt, e.message.orEmpty())
+                showErrorDialog(R.string.account_setup_failed_dlg_auth_message_fmt, e.messageFromServer.orEmpty())
             } catch (e: CertificateValidationException) {
                 handleCertificateValidationException(e)
             } catch (e: Exception) {

mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/RealImapConnection.kt

@@ -359,7 +359,11 @@ internal class RealImapConnection(
     private fun handlePermanentOAuthFailure(e: NegativeImapResponseException): AuthenticationFailedException {
         Timber.v(e, "Permanent failure during authentication using OAuth token")
 
-        return AuthenticationFailedException(message = e.message!!, throwable = e, messageFromServer = e.alertText)
+        return AuthenticationFailedException(
+            message = "Authentication failed",
+            throwable = e,
+            messageFromServer = ResponseTextExtractor.getResponseText(e.lastResponse),
+        )
     }
 
     private fun handleTemporaryOAuthFailure(method: OAuthMethod, e: NegativeImapResponseException): List<ImapResponse> {
@@ -445,7 +449,22 @@ internal class RealImapConnection(
                 throw e
             }
 
+            loginOrThrow(e)
+        }
+    }
+
+    @Suppress("ThrowsCount")
+    private fun loginOrThrow(originalException: AuthenticationFailedException): List<ImapResponse> {
+        return try {
             login()
+        } catch (e: AuthenticationFailedException) {
+            throw e
+        } catch (e: IOException) {
+            Timber.d(e, "LOGIN fallback failed")
+            throw originalException
+        } catch (e: MessagingException) {
+            Timber.d(e, "LOGIN fallback failed")
+            throw originalException
         }
     }
 
@@ -524,7 +543,11 @@ internal class RealImapConnection(
                 close()
             }
 
-            AuthenticationFailedException(negativeResponseException.message!!)
+            AuthenticationFailedException(
+                message = "Authentication failed",
+                throwable = negativeResponseException,
+                messageFromServer = ResponseTextExtractor.getResponseText(lastResponse),
+            )
         } else {
             close()
 

mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/ResponseTextExtractor.kt

@@ -0,0 +1,27 @@
+package com.fsck.k9.mail.store.imap
+
+/**
+ * Extracts the response text from a (negative) status response.
+ */
+internal object ResponseTextExtractor {
+    private const val MINIMUM_RESPONSE_SIZE = 2
+    private const val RESPONSE_CODE_INDEX = 1
+    private const val SIMPLE_RESPONSE_TEXT_INDEX = 1
+    private const val EXTENDED_RESPONSE_TEXT_INDEX = 2
+
+    fun getResponseText(response: ImapResponse): String? {
+        if (response.size < MINIMUM_RESPONSE_SIZE) return null
+
+        val responseTextIndex = if (response.isList(RESPONSE_CODE_INDEX)) {
+            EXTENDED_RESPONSE_TEXT_INDEX
+        } else {
+            SIMPLE_RESPONSE_TEXT_INDEX
+        }
+
+        return if (response.isString(responseTextIndex)) {
+            response.getString(responseTextIndex)
+        } else {
+            null
+        }
+    }
+}

mail/protocols/imap/src/test/java/com/fsck/k9/mail/store/imap/RealImapConnectionTest.kt

@@ -182,8 +182,7 @@ class RealImapConnectionTest {
             imapConnection.open()
             fail("Expected exception")
         } catch (e: AuthenticationFailedException) {
-            // FIXME: improve exception message
+            assertThat(e.messageFromServer).isEqualTo("Go away")
-            assertThat(e).hasMessageThat().contains("Go away")
         }
 
         server.verifyConnectionClosed()
@@ -231,8 +230,7 @@ class RealImapConnectionTest {
             imapConnection.open()
             fail("Expected exception")
         } catch (e: AuthenticationFailedException) {
-            // FIXME: improve exception message
+            assertThat(e.messageFromServer).isEqualTo("Login Failure")
-            assertThat(e).hasMessageThat().contains("Login Failure")
         }
 
         server.verifyConnectionClosed()
@@ -288,8 +286,7 @@ class RealImapConnectionTest {
             imapConnection.open()
             fail("Expected exception")
         } catch (e: AuthenticationFailedException) {
-            // FIXME: improve exception message
+            assertThat(e.messageFromServer).isEqualTo("Who are you?")
-            assertThat(e).hasMessageThat().contains("Who are you?")
         }
 
         server.verifyConnectionClosed()
@@ -395,8 +392,7 @@ class RealImapConnectionTest {
             imapConnection.open()
             fail()
         } catch (e: AuthenticationFailedException) {
-            assertThat(e).hasMessageThat()
+            assertThat(e.messageFromServer).isEqualTo("SASL authentication failed")
-                .isEqualTo("Command: AUTHENTICATE XOAUTH2; response: #2# [NO, SASL authentication failed]")
         }
     }
 
@@ -482,8 +478,7 @@ class RealImapConnectionTest {
             imapConnection.open()
             fail()
         } catch (e: AuthenticationFailedException) {
-            assertThat(e).hasMessageThat()
+            assertThat(e.messageFromServer).isEqualTo("SASL authentication failed")
-                .isEqualTo("Command: AUTHENTICATE XOAUTH2; response: #3# [NO, SASL authentication failed]")
         }
     }
 
@@ -995,6 +990,33 @@ class RealImapConnectionTest {
         server.verifyInteractionCompleted()
     }
 
+    @Test
+    fun `disconnect during LOGIN fallback should throw AuthenticationFailedException`() {
+        val server = MockImapServer().apply {
+            output("* OK example.org server")
+            expect("1 CAPABILITY")
+            output("* CAPABILITY IMAP4 IMAP4REV1 AUTH=PLAIN")
+            output("1 OK CAPABILITY Completed")
+            expect("2 AUTHENTICATE PLAIN")
+            output("+")
+            expect("\u0000$USERNAME\u0000$PASSWORD".base64())
+            output("2 NO AUTHENTICATE failed")
+            expect("3 LOGIN \"$USERNAME\" \"$PASSWORD\"")
+            output("* BYE IMAP server terminating connection")
+            closeConnection()
+        }
+        val imapConnection = startServerAndCreateImapConnection(server)
+
+        try {
+            imapConnection.open()
+            fail("Expected exception")
+        } catch (e: AuthenticationFailedException) {
+            assertThat(e.messageFromServer).isEqualTo("AUTHENTICATE failed")
+        }
+
+        server.verifyInteractionCompleted()
+    }
+
     private fun createImapConnection(
         settings: ImapSettings,
         socketFactory: TrustedSocketFactory,

mail/protocols/imap/src/test/java/com/fsck/k9/mail/store/imap/ResponseTextExtractorTest.kt

@@ -0,0 +1,45 @@
+package com.fsck.k9.mail.store.imap
+
+import assertk.assertThat
+import assertk.assertions.isEqualTo
+import assertk.assertions.isNull
+import com.fsck.k9.mail.store.imap.ImapResponseHelper.createImapResponse
+import org.junit.Test
+
+class ResponseTextExtractorTest {
+    @Test
+    fun `response with response code and response text`() {
+        val imapResponse: ImapResponse = createImapResponse("x NO [AUTHENTICATIONFAILED] Authentication error #23")
+
+        val result = ResponseTextExtractor.getResponseText(imapResponse)
+
+        assertThat(result).isEqualTo("Authentication error #23")
+    }
+
+    @Test
+    fun `response with only response text`() {
+        val imapResponse: ImapResponse = createImapResponse("x NO AUTHENTICATE failed")
+
+        val result = ResponseTextExtractor.getResponseText(imapResponse)
+
+        assertThat(result).isEqualTo("AUTHENTICATE failed")
+    }
+
+    @Test
+    fun `response without response code or text`() {
+        val imapResponse: ImapResponse = createImapResponse("x NO")
+
+        val result = ResponseTextExtractor.getResponseText(imapResponse)
+
+        assertThat(result).isNull()
+    }
+
+    @Test
+    fun `response with only a response code`() {
+        val imapResponse: ImapResponse = createImapResponse("x NO [AUTHENTICATIONFAILED]")
+
+        val result = ResponseTextExtractor.getResponseText(imapResponse)
+
+        assertThat(result).isNull()
+    }
+}