Fix NPE when switching IMAP into TLS
* Update MockTransport to allow TLS connections
* Test TLS connection in ImapStore unit tests
* The bugfix: Re-query capabilities after closing/reopening parser for TLS
(Note: Actually, this is required by the IMAP RFC 3501, 6.2.1)
Bug: 3315939
Change-Id: I51f838043e87750b5712a1bd2e4f9c821b58c808
diff --git a/src/com/android/email/mail/store/ImapStore.java b/src/com/android/email/mail/store/ImapStore.java
index c96cd53..fcaa4a0 100644
--- a/src/com/android/email/mail/store/ImapStore.java
+++ b/src/com/android/email/mail/store/ImapStore.java
@@ -232,7 +232,7 @@
*
* @param userName the username of the account
* @param host the host (server) of the account
- * @param capability the capabilities string from the server
+ * @param capabilityResponse the capabilities list from the server
* @return a String for use in an IMAP ID message.
*/
/* package */ static String getImapId(Context context, String userName, String host,
@@ -1407,17 +1407,9 @@
mParser.readResponse();
// CAPABILITY
- ImapResponse capabilityResponse = null;
- for (ImapResponse r : executeSimpleCommand(ImapConstants.CAPABILITY)) {
- if (r.is(0, ImapConstants.CAPABILITY)) {
- capabilityResponse = r;
- break;
- }
- }
- if (capabilityResponse == null) {
- throw new MessagingException("Invalid CAPABILITY response received");
- }
+ ImapResponse capabilityResponse = queryCapabilities();
+ // TLS
if (mTransport.canTryTlsSecurity()) {
if (capabilityResponse.contains(ImapConstants.STARTTLS)) {
// STARTTLS
@@ -1426,6 +1418,8 @@
mTransport.reopenTls();
mTransport.setSoTimeout(MailTransport.SOCKET_READ_TIMEOUT);
createParser();
+ // Per RFC requirement (3501-6.2.1) gather new capabilities
+ capabilityResponse = queryCapabilities();
} else {
if (Config.LOGD && Email.DEBUG) {
Log.d(Email.LOG_TAG, "TLS not supported but required");
@@ -1434,6 +1428,7 @@
}
}
+ // ID
// Assign user-agent string (for RFC2971 ID command)
String mUserAgent = getImapId(mContext, mUsername, mRootTransport.getHost(),
capabilityResponse);
@@ -1460,6 +1455,7 @@
}
}
+ // LOGIN
try {
// TODO eventually we need to add additional authentication
// options such as SASL
@@ -1565,6 +1561,23 @@
return responses;
}
+ /**
+ * Query server for capabilities.
+ */
+ private ImapResponse queryCapabilities() throws IOException, MessagingException {
+ ImapResponse capabilityResponse = null;
+ for (ImapResponse r : executeSimpleCommand(ImapConstants.CAPABILITY)) {
+ if (r.is(0, ImapConstants.CAPABILITY)) {
+ capabilityResponse = r;
+ break;
+ }
+ }
+ if (capabilityResponse == null) {
+ throw new MessagingException("Invalid CAPABILITY response received");
+ }
+ return capabilityResponse;
+ }
+
/** @see ImapResponseParser#logLastDiscourse() */
public void logLastDiscourse() {
mDiscourse.logLastDiscourse();
diff --git a/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java b/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java
index 867e86a..1bbc82f 100644
--- a/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java
+++ b/tests/src/com/android/email/mail/store/ImapStoreUnitTests.java
@@ -26,13 +26,13 @@
import com.android.email.mail.FetchProfile;
import com.android.email.mail.Flag;
import com.android.email.mail.Folder;
+import com.android.email.mail.Folder.FolderType;
+import com.android.email.mail.Folder.OpenMode;
import com.android.email.mail.Message;
+import com.android.email.mail.Message.RecipientType;
import com.android.email.mail.MessagingException;
import com.android.email.mail.Part;
import com.android.email.mail.Transport;
-import com.android.email.mail.Folder.FolderType;
-import com.android.email.mail.Folder.OpenMode;
-import com.android.email.mail.Message.RecipientType;
import com.android.email.mail.internet.MimeBodyPart;
import com.android.email.mail.internet.MimeMultipart;
import com.android.email.mail.internet.MimeUtility;
@@ -50,7 +50,6 @@
import android.test.MoreAsserts;
import android.test.suitebuilder.annotation.SmallTest;
-import java.io.IOException;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.regex.Pattern;
@@ -80,7 +79,7 @@
*/
private final static String FOLDER_ENCODED = "&ZeU-";
- private static ImapResponse CAPABILITY_RESPONSE = ImapTestUtils.parseResponse(
+ private final static ImapResponse CAPABILITY_RESPONSE = ImapTestUtils.parseResponse(
"* CAPABILITY IMAP4rev1 STARTTLS");
/* These values are provided by setUp() */
@@ -129,9 +128,13 @@
// TODO: inject specific facts in the initial folder SELECT and check them here
}
+ /**
+ * Test simple login with failed authentication
+ */
public void testLoginFailure() throws Exception {
MockTransport mockTransport = openAndInjectMockTransport();
- expectLogin(mockTransport, new String[] {"* iD nIL", "oK"}, "nO authentication failed");
+ expectLogin(mockTransport, false, new String[] {"* iD nIL", "oK"},
+ "nO authentication failed");
try {
mStore.getConnection().open();
@@ -141,9 +144,34 @@
}
/**
+ * Test simple TLS open
+ */
+ public void testTlsOpen() throws MessagingException {
+
+ MockTransport mockTransport = openAndInjectMockTransport(Transport.CONNECTION_SECURITY_TLS,
+ false);
+
+ // try to open it, with STARTTLS
+ expectLogin(mockTransport, true,
+ new String[] {"* iD nIL", "oK"}, "oK user authenticated (Success)");
+ mockTransport.expect(
+ getNextTag(false) + " SELECT \"" + FOLDER_ENCODED + "\"", new String[] {
+ "* fLAGS (\\Answered \\Flagged \\Draft \\Deleted \\Seen)",
+ "* oK [pERMANENTFLAGS (\\Answered \\Flagged \\Draft \\Deleted \\Seen \\*)]",
+ "* 0 eXISTS",
+ "* 0 rECENT",
+ "* OK [uNSEEN 0]",
+ "* OK [uIDNEXT 1]",
+ getNextTag(true) + " oK [" + "rEAD-wRITE" + "] " +
+ FOLDER_ENCODED + " selected. (Success)"});
+
+ mFolder.open(OpenMode.READ_WRITE, null);
+ assertTrue(mockTransport.isTlsStarted());
+ }
+
+ /**
* TODO: Test with SSL negotiation (faked)
* TODO: Test with SSL required but not supported
- * TODO: Test with TLS negotiation (faked)
* TODO: Test with TLS required but not supported
*/
@@ -390,9 +418,17 @@
* Set up a basic MockTransport. open it, and inject it into mStore
*/
private MockTransport openAndInjectMockTransport() {
+ return openAndInjectMockTransport(Transport.CONNECTION_SECURITY_NONE, false);
+ }
+
+ /**
+ * Set up a MockTransport with security settings
+ */
+ private MockTransport openAndInjectMockTransport(int connectionSecurity,
+ boolean trustAllCertificates) {
// Create mock transport and inject it into the ImapStore that's already set up
MockTransport mockTransport = new MockTransport();
- mockTransport.setSecurity(Transport.CONNECTION_SECURITY_NONE, false);
+ mockTransport.setSecurity(connectionSecurity, trustAllCertificates);
mockTransport.setMockHost("mock.server.com");
mStore.setTransport(mockTransport);
return mockTransport;
@@ -427,8 +463,7 @@
* @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).
- * @param "READ-WRITE" or "READ-ONLY"
- * @return the next tag# to use
+ * @param readWriteMode "READ-WRITE" or "READ-ONLY"
*/
private void setupOpenFolder(MockTransport mockTransport, String[] imapIdResponse,
String readWriteMode) {
@@ -450,18 +485,26 @@
}
private void expectLogin(MockTransport mockTransport, String[] imapIdResponse) {
- expectLogin(mockTransport, imapIdResponse, "oK user authenticated (Success)");
+ expectLogin(mockTransport, false, imapIdResponse, "oK user authenticated (Success)");
}
- private void expectLogin(MockTransport mockTransport, String[] imapIdResponse,
+ private void expectLogin(MockTransport mockTransport, boolean startTls, String[] imapIdResponse,
String loginResponse) {
// inject boilerplate commands that match our typical login
mockTransport.expect(null, "* oK Imap 2000 Ready To Assist You");
- mockTransport.expect(getNextTag(false) + " CAPABILITY", new String[] {
- "* cAPABILITY iMAP4rev1 sTARTTLS aUTH=gSSAPI lOGINDISABLED",
- getNextTag(true) + " oK CAPABILITY completed"});
+ expectCapability(mockTransport);
+ // TLS (if expected)
+ if (startTls) {
+ mockTransport.expect(getNextTag(false) + " STARTTLS",
+ getNextTag(true) + " Ok starting TLS");
+ mockTransport.expectStartTls();
+ // After switching to TLS the client must re-query for capability
+ expectCapability(mockTransport);
+ }
+
+ // ID
String expectedNextTag = getNextTag(false);
// Fix the tag # of the ID response
String last = imapIdResponse[imapIdResponse.length-1];
@@ -471,10 +514,17 @@
getNextTag(true); // Advance the tag for ID response.
+ // LOGIN
mockTransport.expect(getNextTag(false) + " LOGIN user \"password\"",
getNextTag(true) + " " + loginResponse);
}
+ private void expectCapability(MockTransport mockTransport) {
+ mockTransport.expect(getNextTag(false) + " CAPABILITY", new String[] {
+ "* cAPABILITY iMAP4rev1 sTARTTLS aUTH=gSSAPI lOGINDISABLED",
+ getNextTag(true) + " oK CAPABILITY completed"});
+ }
+
private void expectNoop(MockTransport mockTransport, boolean ok) {
String response = ok ? " oK success" : " nO timeout";
mockTransport.expect(getNextTag(false) + " NOOP",
@@ -1420,7 +1470,7 @@
}
/**
- * Test for {@link ImapStore#getConnection} and {@link ImapStore#keepConnectionForReuse}
+ * Test for {@link ImapStore#getConnection}
*/
public void testGetConnection() throws Exception {
MockTransport mock = openAndInjectMockTransport();
@@ -1488,7 +1538,7 @@
expectLogin(mock);
mStore.checkSettings();
- expectLogin(mock, new String[] {"* iD nIL", "oK"}, "nO authentication failed");
+ expectLogin(mock, false, new String[] {"* iD nIL", "oK"}, "nO authentication failed");
try {
mStore.checkSettings();
fail();
diff --git a/tests/src/com/android/email/mail/transport/MockTransport.java b/tests/src/com/android/email/mail/transport/MockTransport.java
index 81bfb02..9b9a2b0 100644
--- a/tests/src/com/android/email/mail/transport/MockTransport.java
+++ b/tests/src/com/android/email/mail/transport/MockTransport.java
@@ -44,8 +44,7 @@
private static final String SPECIAL_RESPONSE_IOEXCEPTION = "!!!IOEXCEPTION!!!";
- private boolean mSslAllowed = false;
- private boolean mTlsAllowed = false;
+ private boolean mTlsStarted = false;
private boolean mOpen;
private boolean mInputOpen;
@@ -60,6 +59,7 @@
public static final int ACTION_INJECT_TEXT = 0;
public static final int ACTION_CLIENT_CLOSE = 1;
public static final int ACTION_IO_EXCEPTION = 2;
+ public static final int ACTION_START_TLS = 3;
int mAction;
String mPattern;
@@ -86,6 +86,8 @@
return "Expect the client to close";
case ACTION_IO_EXCEPTION:
return "Expect IOException";
+ case ACTION_START_TLS:
+ return "Expect StartTls";
default:
return "(Hmm. Unknown action.)";
}
@@ -142,6 +144,10 @@
mPairs.add(new Transaction(Transaction.ACTION_IO_EXCEPTION));
}
+ public void expectStartTls() {
+ mPairs.add(new Transaction(Transaction.ACTION_START_TLS));
+ }
+
private void sendResponse(Transaction pair) {
switch (pair.mAction) {
case Transaction.ACTION_INJECT_TEXT:
@@ -170,6 +176,13 @@
}
/**
+ * Check that TLS was started
+ */
+ public boolean isTlsStarted() {
+ return mTlsStarted;
+ }
+
+ /**
* This simulates a condition where the server has closed its side, causing
* reads to fail.
*/
@@ -289,8 +302,9 @@
public void reopenTls() /* throws MessagingException */ {
SmtpSenderUnitTests.assertTrue(mOpen);
- SmtpSenderUnitTests.assertTrue(mTlsAllowed);
- SmtpSenderUnitTests.fail("reopenTls() not implemented");
+ Transaction expect = mPairs.remove(0);
+ SmtpSenderUnitTests.assertTrue(expect.mAction == Transaction.ACTION_START_TLS);
+ mTlsStarted = true;
}
public void setSecurity(int connectionSecurity, boolean trustAllCertificates) {
