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

src/com/android/email/mail/store/ImapStore.java

@@ -232,7 +232,7 @@ public class ImapStore extends Store {
      *
      * @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 @@ public class ImapStore extends Store {
                 mParser.readResponse();
 
                 // CAPABILITY
-                ImapResponse capabilityResponse = null;
+                ImapResponse capabilityResponse = queryCapabilities();
-                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");
-                }
 
+                // TLS
                 if (mTransport.canTryTlsSecurity()) {
                     if (capabilityResponse.contains(ImapConstants.STARTTLS)) {
                         // STARTTLS
@@ -1426,6 +1418,8 @@ public class ImapStore extends Store {
                         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 @@ public class ImapStore extends Store {
                     }
                 }
 
+                // ID
                 // Assign user-agent string (for RFC2971 ID command)
                 String mUserAgent = getImapId(mContext, mUsername, mRootTransport.getHost(),
                         capabilityResponse);
@@ -1460,6 +1455,7 @@ public class ImapStore extends Store {
                     }
                 }
 
+                // LOGIN
                 try {
                     // TODO eventually we need to add additional authentication
                     // options such as SASL
@@ -1565,6 +1561,23 @@ public class ImapStore extends Store {
             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();

tests/src/com/android/email/mail/store/ImapStoreUnitTests.java

@@ -26,13 +26,13 @@ import com.android.email.mail.Body;
 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.AndroidTestCase;
 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 @@ public class ImapStoreUnitTests extends AndroidTestCase {
      */
     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 @@ public class ImapStoreUnitTests extends AndroidTestCase {
         // 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();
@@ -140,10 +143,35 @@ public class ImapStoreUnitTests extends AndroidTestCase {
         }
     }
 
+    /**
+     * 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 @@ public class ImapStoreUnitTests extends AndroidTestCase {
      * 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 @@ public class ImapStoreUnitTests extends AndroidTestCase {
      * @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"
+     * @param readWriteMode "READ-WRITE" or "READ-ONLY"
-     * @return the next tag# to use
      */
     private void setupOpenFolder(MockTransport mockTransport, String[] imapIdResponse,
             String readWriteMode) {
@@ -450,18 +485,26 @@ public class ImapStoreUnitTests extends AndroidTestCase {
     }
 
     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[] {
+        expectCapability(mockTransport);
-                "* cAPABILITY iMAP4rev1 sTARTTLS aUTH=gSSAPI lOGINDISABLED",
-                getNextTag(true) + " oK CAPABILITY completed"});
 
+        // 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 @@ public class ImapStoreUnitTests extends AndroidTestCase {
 
         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 @@ public class ImapStoreUnitTests extends AndroidTestCase {
     }
 
     /**
-     * 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 @@ public class ImapStoreUnitTests extends AndroidTestCase {
         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();

tests/src/com/android/email/mail/transport/MockTransport.java

@@ -44,8 +44,7 @@ public class MockTransport implements Transport {
 
     private static final String SPECIAL_RESPONSE_IOEXCEPTION = "!!!IOEXCEPTION!!!";
 
-    private boolean mSslAllowed = false;
+    private boolean mTlsStarted = false;
-    private boolean mTlsAllowed = false;
 
     private boolean mOpen;
     private boolean mInputOpen;
@@ -60,6 +59,7 @@ public class MockTransport implements Transport {
         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 @@ public class MockTransport implements Transport {
                     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 @@ public class MockTransport implements Transport {
         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:
@@ -169,6 +175,13 @@ public class MockTransport implements Transport {
         return mTrustCertificates;
     }
 
+    /**
+     * 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 class MockTransport implements Transport {
 
     public void reopenTls() /* throws MessagingException */ {
         SmtpSenderUnitTests.assertTrue(mOpen);
-        SmtpSenderUnitTests.assertTrue(mTlsAllowed);
+        Transaction expect = mPairs.remove(0);
-        SmtpSenderUnitTests.fail("reopenTls() not implemented");
+        SmtpSenderUnitTests.assertTrue(expect.mAction == Transaction.ACTION_START_TLS);
+        mTlsStarted = true;
     }
 
     public void setSecurity(int connectionSecurity, boolean trustAllCertificates) {