From 927dbc7c20072939fd0ebdf4cc89301a41d075c2 Mon Sep 17 00:00:00 2001 From: Andy Stadler Date: Mon, 31 Jan 2011 15:10:49 -0800 Subject: [PATCH] Don't send IMAP ID unless server supports it Only send IMAP ID to servers that include ID in the CAPABILITY response. Always sending IMAP ID was found to cause problems with some servers. Better compliance with RFC 2971. Thanks to Samsung for debugging & reporting this. Change-Id: I495f80949f9f811470853a1f2f8e506d8236d8cf --- .../android/email/mail/store/ImapStore.java | 44 +++++++------- .../email/mail/store/ImapStoreUnitTests.java | 58 +++++++++++++------ 2 files changed, 62 insertions(+), 40 deletions(-) diff --git a/src/com/android/email/mail/store/ImapStore.java b/src/com/android/email/mail/store/ImapStore.java index fcaa4a069..6a08bb923 100644 --- a/src/com/android/email/mail/store/ImapStore.java +++ b/src/com/android/email/mail/store/ImapStore.java @@ -1429,29 +1429,31 @@ public class ImapStore extends Store { } // ID - // Assign user-agent string (for RFC2971 ID command) - String mUserAgent = getImapId(mContext, mUsername, mRootTransport.getHost(), - capabilityResponse); - if (mUserAgent != null) { - mIdPhrase = ImapConstants.ID + " (" + mUserAgent + ")"; - } else if (DEBUG_FORCE_SEND_ID) { - mIdPhrase = ImapConstants.ID + " " + ImapConstants.NIL; - } - // else: mIdPhrase = null, no ID will be emitted + if (capabilityResponse.contains(ImapConstants.ID)) { + // Assign user-agent string (for RFC2971 ID command) + String mUserAgent = getImapId(mContext, mUsername, mRootTransport.getHost(), + capabilityResponse); + if (mUserAgent != null) { + mIdPhrase = ImapConstants.ID + " (" + mUserAgent + ")"; + } else if (DEBUG_FORCE_SEND_ID) { + mIdPhrase = ImapConstants.ID + " " + ImapConstants.NIL; + } + // else: mIdPhrase = null, no ID will be emitted - // Send user-agent in an RFC2971 ID command - if (mIdPhrase != null) { - try { - executeSimpleCommand(mIdPhrase); - } catch (ImapException ie) { - // Log for debugging, but this is not a fatal problem. - if (Config.LOGD && Email.DEBUG) { - Log.d(Email.LOG_TAG, ie.toString()); + // Send user-agent in an RFC2971 ID command + if (mIdPhrase != null) { + try { + executeSimpleCommand(mIdPhrase); + } catch (ImapException ie) { + // Log for debugging, but this is not a fatal problem. + if (Config.LOGD && Email.DEBUG) { + Log.d(Email.LOG_TAG, ie.toString()); + } + } catch (IOException ioe) { + // Special case to handle malformed OK responses and ignore them. + // A true IOException will recur on the following login steps + // This can go away after the parser is fixed - see bug 2138981 } - } catch (IOException ioe) { - // Special case to handle malformed OK responses and ignore them. - // A true IOException will recur on the following login steps - // This can go away after the parser is fixed - see bug 2138981 for details } } diff --git a/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java b/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java index 1bbc82fde..a4a01ceb5 100644 --- a/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java +++ b/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java @@ -133,7 +133,7 @@ public class ImapStoreUnitTests extends AndroidTestCase { */ public void testLoginFailure() throws Exception { MockTransport mockTransport = openAndInjectMockTransport(); - expectLogin(mockTransport, false, new String[] {"* iD nIL", "oK"}, + expectLogin(mockTransport, false, false, new String[] {"* iD nIL", "oK"}, "nO authentication failed"); try { @@ -152,7 +152,7 @@ public class ImapStoreUnitTests extends AndroidTestCase { false); // try to open it, with STARTTLS - expectLogin(mockTransport, true, + expectLogin(mockTransport, true, false, new String[] {"* iD nIL", "oK"}, "oK user authenticated (Success)"); mockTransport.expect( getNextTag(false) + " SELECT \"" + FOLDER_ENCODED + "\"", new String[] { @@ -366,6 +366,19 @@ public class ImapStoreUnitTests extends AndroidTestCase { mFolder.open(OpenMode.READ_WRITE, null); } + /** + * Confirm that when IMAP ID is not in capability, it is not sent/received. + * This supports RFC 2971 section 3, and is important because certain servers + * (e.g. imap.vodafone.net.nz) do not process the unexpected ID command properly. + */ + public void testImapIdNotSupported() throws MessagingException { + MockTransport mockTransport = openAndInjectMockTransport(); + + // try to open it + setupOpenFolder(mockTransport, null, "rEAD-wRITE"); + mFolder.open(OpenMode.READ_WRITE, null); + } + /** * Test small Folder functions that don't really do anything in Imap */ @@ -462,7 +475,7 @@ public class ImapStoreUnitTests extends AndroidTestCase { * @param mockTransport the mock transport we're using * @param imapIdResponse the expected series of responses to the IMAP ID command. Non-final * lines should be tagged with *. The final response should be untagged (the correct - * tag will be added at runtime). + * tag will be added at runtime). Pass "null" to test w/o IMAP ID. * @param readWriteMode "READ-WRITE" or "READ-ONLY" */ private void setupOpenFolder(MockTransport mockTransport, String[] imapIdResponse, @@ -485,15 +498,16 @@ public class ImapStoreUnitTests extends AndroidTestCase { } private void expectLogin(MockTransport mockTransport, String[] imapIdResponse) { - expectLogin(mockTransport, false, imapIdResponse, "oK user authenticated (Success)"); + expectLogin(mockTransport, false, (imapIdResponse != null), imapIdResponse, + "oK user authenticated (Success)"); } - private void expectLogin(MockTransport mockTransport, boolean startTls, String[] imapIdResponse, - String loginResponse) { + private void expectLogin(MockTransport mockTransport, boolean startTls, boolean withId, + String[] imapIdResponse, String loginResponse) { // inject boilerplate commands that match our typical login mockTransport.expect(null, "* oK Imap 2000 Ready To Assist You"); - expectCapability(mockTransport); + expectCapability(mockTransport, withId); // TLS (if expected) if (startTls) { @@ -501,27 +515,32 @@ public class ImapStoreUnitTests extends AndroidTestCase { getNextTag(true) + " Ok starting TLS"); mockTransport.expectStartTls(); // After switching to TLS the client must re-query for capability - expectCapability(mockTransport); + expectCapability(mockTransport, withId); } // ID - String expectedNextTag = getNextTag(false); - // Fix the tag # of the ID response - String last = imapIdResponse[imapIdResponse.length-1]; - last = expectedNextTag + " " + last; - imapIdResponse[imapIdResponse.length-1] = last; - mockTransport.expect(getNextTag(false) + " ID \\(.*\\)", imapIdResponse); - - getNextTag(true); // Advance the tag for ID response. + if (withId) { + String expectedNextTag = getNextTag(false); + // Fix the tag # of the ID response + String last = imapIdResponse[imapIdResponse.length-1]; + last = expectedNextTag + " " + last; + imapIdResponse[imapIdResponse.length-1] = last; + mockTransport.expect(getNextTag(false) + " ID \\(.*\\)", imapIdResponse); + getNextTag(true); // Advance the tag for ID response. + } // LOGIN mockTransport.expect(getNextTag(false) + " LOGIN user \"password\"", getNextTag(true) + " " + loginResponse); } - private void expectCapability(MockTransport mockTransport) { + private void expectCapability(MockTransport mockTransport, boolean withId) { + String capabilityList = withId + ? "* cAPABILITY iMAP4rev1 sTARTTLS aUTH=gSSAPI lOGINDISABLED iD" + : "* cAPABILITY iMAP4rev1 sTARTTLS aUTH=gSSAPI lOGINDISABLED"; + mockTransport.expect(getNextTag(false) + " CAPABILITY", new String[] { - "* cAPABILITY iMAP4rev1 sTARTTLS aUTH=gSSAPI lOGINDISABLED", + capabilityList, getNextTag(true) + " oK CAPABILITY completed"}); } @@ -1538,7 +1557,8 @@ public class ImapStoreUnitTests extends AndroidTestCase { expectLogin(mock); mStore.checkSettings(); - expectLogin(mock, false, new String[] {"* iD nIL", "oK"}, "nO authentication failed"); + expectLogin(mock, false, false, + new String[] {"* iD nIL", "oK"}, "nO authentication failed"); try { mStore.checkSettings(); fail();