From 7e5022ef36f6854c5db2f4e58f586bf17d7f694b Mon Sep 17 00:00:00 2001 From: cketti Date: Wed, 15 Mar 2023 19:04:50 +0100 Subject: [PATCH 1/3] IMAP: Put server response text in AuthenticationFailedException --- .../k9/mail/store/imap/RealImapConnection.kt | 12 ++++- .../mail/store/imap/ResponseTextExtractor.kt | 27 +++++++++++ .../mail/store/imap/RealImapConnectionTest.kt | 15 +++---- .../store/imap/ResponseTextExtractorTest.kt | 45 +++++++++++++++++++ 4 files changed, 87 insertions(+), 12 deletions(-) create mode 100644 mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/ResponseTextExtractor.kt create mode 100644 mail/protocols/imap/src/test/java/com/fsck/k9/mail/store/imap/ResponseTextExtractorTest.kt diff --git a/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/RealImapConnection.kt b/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/RealImapConnection.kt index 67adcece22..6575c834e4 100644 --- a/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/RealImapConnection.kt +++ b/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 { @@ -524,7 +528,11 @@ internal class RealImapConnection( close() } - AuthenticationFailedException(negativeResponseException.message!!) + AuthenticationFailedException( + message = "Authentication failed", + throwable = negativeResponseException, + messageFromServer = ResponseTextExtractor.getResponseText(lastResponse), + ) } else { close() diff --git a/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/ResponseTextExtractor.kt b/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/ResponseTextExtractor.kt new file mode 100644 index 0000000000..9c4911818f --- /dev/null +++ b/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 + } + } +} diff --git a/mail/protocols/imap/src/test/java/com/fsck/k9/mail/store/imap/RealImapConnectionTest.kt b/mail/protocols/imap/src/test/java/com/fsck/k9/mail/store/imap/RealImapConnectionTest.kt index 2cebadbaf5..eeb8a19580 100644 --- a/mail/protocols/imap/src/test/java/com/fsck/k9/mail/store/imap/RealImapConnectionTest.kt +++ b/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).hasMessageThat().contains("Go away") + assertThat(e.messageFromServer).isEqualTo("Go away") } server.verifyConnectionClosed() @@ -231,8 +230,7 @@ class RealImapConnectionTest { imapConnection.open() fail("Expected exception") } catch (e: AuthenticationFailedException) { - // FIXME: improve exception message - assertThat(e).hasMessageThat().contains("Login Failure") + assertThat(e.messageFromServer).isEqualTo("Login Failure") } server.verifyConnectionClosed() @@ -288,8 +286,7 @@ class RealImapConnectionTest { imapConnection.open() fail("Expected exception") } catch (e: AuthenticationFailedException) { - // FIXME: improve exception message - assertThat(e).hasMessageThat().contains("Who are you?") + assertThat(e.messageFromServer).isEqualTo("Who are you?") } server.verifyConnectionClosed() @@ -395,8 +392,7 @@ class RealImapConnectionTest { imapConnection.open() fail() } catch (e: AuthenticationFailedException) { - assertThat(e).hasMessageThat() - .isEqualTo("Command: AUTHENTICATE XOAUTH2; response: #2# [NO, SASL authentication failed]") + assertThat(e.messageFromServer).isEqualTo("SASL authentication failed") } } @@ -482,8 +478,7 @@ class RealImapConnectionTest { imapConnection.open() fail() } catch (e: AuthenticationFailedException) { - assertThat(e).hasMessageThat() - .isEqualTo("Command: AUTHENTICATE XOAUTH2; response: #3# [NO, SASL authentication failed]") + assertThat(e.messageFromServer).isEqualTo("SASL authentication failed") } } diff --git a/mail/protocols/imap/src/test/java/com/fsck/k9/mail/store/imap/ResponseTextExtractorTest.kt b/mail/protocols/imap/src/test/java/com/fsck/k9/mail/store/imap/ResponseTextExtractorTest.kt new file mode 100644 index 0000000000..f40e77f425 --- /dev/null +++ b/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() + } +} From faf3a0b64e505da2cb018f3ecc3cf8bf7750b259 Mon Sep 17 00:00:00 2001 From: cketti Date: Wed, 15 Mar 2023 19:39:58 +0100 Subject: [PATCH 2/3] On authentication failures display message from server to the user --- .../com/fsck/k9/activity/setup/AccountSetupCheckSettings.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/ui/legacy/src/main/java/com/fsck/k9/activity/setup/AccountSetupCheckSettings.kt b/app/ui/legacy/src/main/java/com/fsck/k9/activity/setup/AccountSetupCheckSettings.kt index 1ac11bffd1..96dc2f9cb0 100644 --- a/app/ui/legacy/src/main/java/com/fsck/k9/activity/setup/AccountSetupCheckSettings.kt +++ b/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) { From 76b52d17acd74a75b18fd8c2467542363d775c20 Mon Sep 17 00:00:00 2001 From: cketti Date: Wed, 15 Mar 2023 19:48:57 +0100 Subject: [PATCH 3/3] IMAP: Ignore errors during LOGIN fallback and throw original exception --- .../k9/mail/store/imap/RealImapConnection.kt | 15 +++++++++++ .../mail/store/imap/RealImapConnectionTest.kt | 27 +++++++++++++++++++ 2 files changed, 42 insertions(+) diff --git a/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/RealImapConnection.kt b/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/RealImapConnection.kt index 6575c834e4..22dc8eb62f 100644 --- a/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/RealImapConnection.kt +++ b/mail/protocols/imap/src/main/java/com/fsck/k9/mail/store/imap/RealImapConnection.kt @@ -449,7 +449,22 @@ internal class RealImapConnection( throw e } + loginOrThrow(e) + } + } + + @Suppress("ThrowsCount") + private fun loginOrThrow(originalException: AuthenticationFailedException): List { + 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 } } diff --git a/mail/protocols/imap/src/test/java/com/fsck/k9/mail/store/imap/RealImapConnectionTest.kt b/mail/protocols/imap/src/test/java/com/fsck/k9/mail/store/imap/RealImapConnectionTest.kt index eeb8a19580..d5b019df72 100644 --- a/mail/protocols/imap/src/test/java/com/fsck/k9/mail/store/imap/RealImapConnectionTest.kt +++ b/mail/protocols/imap/src/test/java/com/fsck/k9/mail/store/imap/RealImapConnectionTest.kt @@ -990,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,