Merge pull request #413 from matrix-org/feature/issue2

MXCrypto: Use the last olm session that got a message
matrix-org · Dec 28, 2018 · 12c04c4 · 12c04c4
2 parents c0f9a44 + e5b2aa7
commit 12c04c4
Show file tree

Hide file tree

Showing 12 changed files with 255 additions and 99 deletions.
diff --git a/CHANGES.rst b/CHANGES.rst
@@ -8,6 +8,7 @@ Improvements:
  - isValidRecoveryKey() ignores now all whitespace characters, not only spaces
 
 Bugfix:
+ - MXCrypto: Use the last olm session that got a message (vector-im/riot-android#2772).
  - Ensure there is no ghost device in the Realm crypto store (vector-im/riot-android#2784)
 
 API Change:

diff --git a/...oidsdk/crypto/CryptoStoreMigrationTest.kt → ...dsdk/crypto/CryptoStoreImportationTest.kt b/...oidsdk/crypto/CryptoStoreMigrationTest.kt → ...dsdk/crypto/CryptoStoreImportationTest.kt
@@ -19,11 +19,13 @@ package org.matrix.androidsdk.crypto
 import android.support.test.InstrumentationRegistry
 import android.text.TextUtils
 import org.junit.Assert.*
+import org.junit.Before
 import org.junit.FixMethodOrder
 import org.junit.Test
 import org.junit.runners.MethodSorters
 import org.matrix.androidsdk.common.*
 import org.matrix.androidsdk.crypto.data.MXDeviceInfo
+import org.matrix.androidsdk.crypto.data.MXOlmSession
 import org.matrix.androidsdk.data.RoomState
 import org.matrix.androidsdk.data.cryptostore.IMXCryptoStore
 import org.matrix.androidsdk.data.cryptostore.MXFileCryptoStore
@@ -34,11 +36,12 @@ import org.matrix.androidsdk.rest.model.Event
 import org.matrix.androidsdk.rest.model.crypto.RoomKeyRequestBody
 import org.matrix.androidsdk.util.Log
 import org.matrix.olm.OlmAccount
+import org.matrix.olm.OlmManager
 import org.matrix.olm.OlmSession
 import java.util.concurrent.CountDownLatch
 
 @FixMethodOrder(MethodSorters.JVM)
-class CryptoStoreMigrationTest {
+class CryptoStoreImportationTest {
 
     private val mTestHelper = CommonTestHelper()
     private val mCryptoTestHelper = CryptoTestHelper(mTestHelper)
@@ -47,9 +50,14 @@ class CryptoStoreMigrationTest {
     private val sessionTestParamLegacy = SessionTestParams(withInitialSync = true, withCryptoEnabled = true, withLegacyCryptoStore = true)
     private val sessionTestParamRealm = SessionTestParams(withInitialSync = true, withCryptoEnabled = true, withLegacyCryptoStore = false)
 
+    @Before
+    fun ensureLibLoaded() {
+        OlmManager()
+    }
+
     @Test
-    fun test_migrationEmptyStore() {
-        testMigration(
+    fun test_importationEmptyStore() {
+        testImportation(
                 doOnFileStore = {
                     // Nothing to do for this test
                 },
@@ -61,10 +69,10 @@ class CryptoStoreMigrationTest {
     }
 
     @Test
-    fun test_migrationOlmAccount() {
+    fun test_importationOlmAccount() {
         val olmAccount = OlmAccount()
 
-        testMigration(
+        testImportation(
                 doOnFileStore = {
                     it.storeAccount(olmAccount)
                 },
@@ -77,8 +85,8 @@ class CryptoStoreMigrationTest {
     }
 
     @Test
-    fun test_migrationRooms() {
-        testMigration(
+    fun test_importationRooms() {
+        testImportation(
                 doOnFileStore = {
                     it.storeRoomAlgorithm("roomId1", "algo1")
                     it.storeRoomAlgorithm("roomId2", "algo2")
@@ -94,12 +102,12 @@ class CryptoStoreMigrationTest {
     }
 
     @Test
-    fun test_migrationUsers() {
+    fun test_importationUsers() {
         val deviceTrackingStatus = HashMap<String, Int>().apply {
             put("userId1", MXDeviceList.TRACKING_STATUS_DOWNLOAD_IN_PROGRESS)
         }
 
-        testMigration(
+        testImportation(
                 doOnFileStore = {
                     it.storeUserDevice("userId1", MXDeviceInfo().apply {
                         deviceId = "deviceId1"
@@ -118,7 +126,7 @@ class CryptoStoreMigrationTest {
     }
 
     @Test
-    fun test_migrationOutgoingRoomKeyRequest() {
+    fun test_importationOutgoingRoomKeyRequest() {
         val request = OutgoingRoomKeyRequest(
                 // Request body
                 HashMap<String, String>().apply {
@@ -136,7 +144,7 @@ class CryptoStoreMigrationTest {
                     mCancellationTxnId = "mCancellationTxnId"
                 }
 
-        testMigration(
+        testImportation(
                 doOnFileStore = {
                     it.getOrAddOutgoingRoomKeyRequest(request)
                 },
@@ -154,7 +162,7 @@ class CryptoStoreMigrationTest {
     }
 
     @Test
-    fun test_migrationIncomingRoomKeyRequest() {
+    fun test_importationIncomingRoomKeyRequest() {
         val request = IncomingRoomKeyRequest().apply {
             mUserId = "userId"
             mDeviceId = "DeviceId"
@@ -167,7 +175,7 @@ class CryptoStoreMigrationTest {
             }
         }
 
-        testMigration(
+        testImportation(
                 doOnFileStore = {
                     it.storeIncomingRoomKeyRequest(request)
                 },
@@ -187,14 +195,14 @@ class CryptoStoreMigrationTest {
     }
 
     @Test
-    fun test_migrationOlmSessions() {
+    fun test_importationOlmSessions() {
         val session = OlmSession()
 
         val sessionId = session.sessionIdentifier()
 
-        testMigration(
+        testImportation(
                 doOnFileStore = {
-                    it.storeSession(session, "deviceID")
+                    it.storeSession(MXOlmSession(session), "deviceID")
                 },
                 checkOnRealmStore = {
                     val sessionsFromRealm = it.getDeviceSessionIds("deviceID")
@@ -205,21 +213,21 @@ class CryptoStoreMigrationTest {
                     val sessionFromRealm = it.getDeviceSession(sessionId, "deviceID")
 
                     assertNotNull(sessionFromRealm)
-                    assertEquals(sessionId, sessionFromRealm?.sessionIdentifier())
+                    assertEquals(sessionId, sessionFromRealm?.olmSession?.sessionIdentifier())
                 })
     }
 
     @Test
-    fun test_migrationInboundGroupSessions() {
-        // This is tested in test_integration_migrationInboundGroupSession
+    fun test_importationInboundGroupSessions() {
+        // This is tested in test_integration_importationInboundGroupSession
     }
 
     /* ==========================================================================================
      * Integration tests
      * ========================================================================================== */
 
     @Test
-    fun test_integration_migrationEmptyStore() {
+    fun test_integration_importationEmptyStore() {
         Log.e(LOG_TAG, "test01_testCryptoNoDeviceId")
 
         // Create an account using the file store
@@ -232,10 +240,10 @@ class CryptoStoreMigrationTest {
 
         assertNotNull(bobSession.credentials.deviceId)
 
-        // Open again the session, with the Realm store. It will trigger the migration
+        // Open again the session, with the Realm store. It will trigger the importation
         val bobSession2 = mTestHelper.createNewSession(bobSession, sessionTestParamRealm)
 
-        // Migration should be ok
+        // Importation should be ok
         assertNotNull(bobSession2.crypto)
         assertNotNull(bobSession2.crypto?.cryptoStore)
         assertTrue(bobSession2.crypto?.cryptoStore is RealmCryptoStore)
@@ -248,8 +256,8 @@ class CryptoStoreMigrationTest {
     }
 
     @Test
-    fun test_integration_migrationInboundGroupSession() {
-        Log.e(LOG_TAG, "test_integration_migrationInboundGroupSession")
+    fun test_integration_importationInboundGroupSession() {
+        Log.e(LOG_TAG, "test_integration_importationInboundGroupSession")
 
         val context = InstrumentationRegistry.getContext()
         val results = java.util.HashMap<String, Any>()
@@ -345,8 +353,8 @@ class CryptoStoreMigrationTest {
      * Private
      * ========================================================================================== */
 
-    private fun testMigration(doOnFileStore: (IMXCryptoStore) -> Unit,
-                              checkOnRealmStore: (IMXCryptoStore) -> Unit) {
+    private fun testImportation(doOnFileStore: (IMXCryptoStore) -> Unit,
+                                checkOnRealmStore: (IMXCryptoStore) -> Unit) {
         val context = InstrumentationRegistry.getContext()
 
         val credentials = cryptoStoreHelper.createCredential()
@@ -359,7 +367,7 @@ class CryptoStoreMigrationTest {
         // Let each test do what they want to configure the file store
         doOnFileStore.invoke(fileCryptoStore)
 
-        // It will trigger the migration
+        // It will trigger the importation
         val realmCryptoStore = RealmCryptoStore()
         realmCryptoStore.initWithCredentials(context, credentials)
 
@@ -374,6 +382,6 @@ class CryptoStoreMigrationTest {
     }
 
     companion object {
-        private const val LOG_TAG = "CryptoStoreMigrationTest"
+        private const val LOG_TAG = "CryptoStoreImportationTest"
     }
 }
diff --git a/matrix-sdk/src/androidTest/java/org/matrix/androidsdk/crypto/CryptoStoreTest.kt b/matrix-sdk/src/androidTest/java/org/matrix/androidsdk/crypto/CryptoStoreTest.kt
@@ -18,7 +18,13 @@ package org.matrix.androidsdk.crypto
 
 import org.junit.Assert.*
 import org.junit.Test
+import org.matrix.androidsdk.crypto.data.MXOlmSession
 import org.matrix.androidsdk.data.cryptostore.IMXCryptoStore
+import org.matrix.olm.OlmAccount
+import org.matrix.olm.OlmManager
+import org.matrix.olm.OlmSession
+
+private const val DUMMY_DEVICE_KEY = "DeviceKey"
 
 class CryptoStoreTest {
 
@@ -50,6 +56,73 @@ class CryptoStoreTest {
         cryptoStore.deleteStore()
     }
 
+    @Test
+    fun test_lastSessionUsed() {
+        // Ensure Olm is initialized
+        OlmManager()
+
+        val cryptoStore: IMXCryptoStore = cryptoStoreHelper.createStore(true)
+
+        assertNull(cryptoStore.getLastUsedSessionId(DUMMY_DEVICE_KEY))
+
+        val olmAccount1 = OlmAccount().apply {
+            generateOneTimeKeys(1)
+        }
+
+        val olmSession1 = OlmSession().apply {
+            initOutboundSession(olmAccount1,
+                    olmAccount1.identityKeys()[OlmAccount.JSON_KEY_IDENTITY_KEY],
+                    olmAccount1.oneTimeKeys()[OlmAccount.JSON_KEY_ONE_TIME_KEY]?.values?.first())
+        }
+
+        val sessionId1 = olmSession1.sessionIdentifier()
+        val mxOlmSession1 = MXOlmSession(olmSession1)
+
+        cryptoStore.storeSession(mxOlmSession1, DUMMY_DEVICE_KEY)
+
+        assertEquals(sessionId1, cryptoStore.getLastUsedSessionId(DUMMY_DEVICE_KEY))
+
+        val olmAccount2 = OlmAccount().apply {
+            generateOneTimeKeys(1)
+        }
+
+        val olmSession2 = OlmSession().apply {
+            initOutboundSession(olmAccount2,
+                    olmAccount2.identityKeys()[OlmAccount.JSON_KEY_IDENTITY_KEY],
+                    olmAccount2.oneTimeKeys()[OlmAccount.JSON_KEY_ONE_TIME_KEY]?.values?.first())
+        }
+
+        val sessionId2 = olmSession2.sessionIdentifier()
+        val mxOlmSession2 = MXOlmSession(olmSession2)
+
+        cryptoStore.storeSession(mxOlmSession2, DUMMY_DEVICE_KEY)
+
+        // Ensure sessionIds are distinct
+        assertNotEquals(sessionId1, sessionId2)
+
+        // Note: we cannot be sure what will be the result of getLastUsedSessionId() here
+
+        mxOlmSession2.onMessageReceived()
+        cryptoStore.storeSession(mxOlmSession2, DUMMY_DEVICE_KEY)
+
+        // sessionId2 is returned now
+        assertEquals(sessionId2, cryptoStore.getLastUsedSessionId(DUMMY_DEVICE_KEY))
+
+        Thread.sleep(2)
+
+        mxOlmSession1.onMessageReceived()
+        cryptoStore.storeSession(mxOlmSession1, DUMMY_DEVICE_KEY)
+
+        // sessionId1 is returned now
+        assertEquals(sessionId1, cryptoStore.getLastUsedSessionId(DUMMY_DEVICE_KEY))
+
+        // Cleanup
+        olmSession1.releaseSession()
+        olmSession2.releaseSession()
+
+        olmAccount1.releaseAccount()
+        olmAccount2.releaseAccount()
+    }
 
     companion object {
         private const val LOG_TAG = "CryptoStoreTest"

diff --git a/matrix-sdk/src/main/java/org/matrix/androidsdk/crypto/MXCrypto.java b/matrix-sdk/src/main/java/org/matrix/androidsdk/crypto/MXCrypto.java
@@ -59,6 +59,7 @@
 import org.matrix.androidsdk.rest.model.sync.SyncResponse;
 import org.matrix.androidsdk.util.JsonUtils;
 import org.matrix.androidsdk.util.Log;
+import org.matrix.olm.OlmAccount;
 
 import java.lang.reflect.Constructor;
 import java.util.ArrayList;
@@ -2148,7 +2149,7 @@ private void uploadOneTimeKeys(final ApiCallback<KeysUploadResponse> callback) {
         final Map<String, Map<String, String>> oneTimeKeys = mOlmDevice.getOneTimeKeys();
         Map<String, Object> oneTimeJson = new HashMap<>();
 
-        Map<String, String> curve25519Map = oneTimeKeys.get("curve25519");
+        Map<String, String> curve25519Map = oneTimeKeys.get(OlmAccount.JSON_KEY_ONE_TIME_KEY);
 
         if (null != curve25519Map) {
             for (String key_id : curve25519Map.keySet()) {