From 01b8b7d57a15de19e4af90d918eaf8453f26a5ac Mon Sep 17 00:00:00 2001 From: Valere Date: Wed, 8 Dec 2021 14:17:08 +0100 Subject: [PATCH] Code review --- .../internal/crypto/DefaultCryptoService.kt | 2 +- .../sdk/internal/crypto/MXOlmDevice.kt | 19 ++++++++-- .../internal/crypto/OneTimeKeysUploader.kt | 36 ++++++++----------- 3 files changed, 31 insertions(+), 26 deletions(-) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/DefaultCryptoService.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/DefaultCryptoService.kt index 19ba5c21dc..7d9c351410 100755 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/DefaultCryptoService.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/DefaultCryptoService.kt @@ -437,7 +437,7 @@ internal class DefaultCryptoService @Inject constructor( if (syncResponse.deviceUnusedFallbackKeyTypes != null && // Generate a fallback key only if the server does not already have an unused fallback key. !syncResponse.deviceUnusedFallbackKeyTypes.contains(KEY_SIGNED_CURVE_25519_TYPE)) { - oneTimeKeysUploader.setNeedsNewFallback() + oneTimeKeysUploader.needsNewFallback() } oneTimeKeysUploader.maybeUploadOneTimeKeys() diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/MXOlmDevice.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/MXOlmDevice.kt index 50f3e6acd0..e1a706df79 100755 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/MXOlmDevice.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/MXOlmDevice.kt @@ -150,13 +150,26 @@ internal class MXOlmDevice @Inject constructor( return null } - fun generateFallbackKey() { + /** + * Generates a new fallback key if there is not already + * an unpublished one. + * @return true if a new key was generated + */ + fun generateFallbackKeyIfNeeded(): Boolean { try { - store.getOlmAccount().generateFallbackKey() - store.saveOlmAccount() + if (!hasUnpublishedFallbackKey()) { + store.getOlmAccount().generateFallbackKey() + store.saveOlmAccount() + return true + } } catch (e: Exception) { Timber.e("## generateFallbackKey() : failed") } + return false + } + + internal fun hasUnpublishedFallbackKey(): Boolean { + return getFallbackKey()?.get(OlmAccount.JSON_KEY_ONE_TIME_KEY).orEmpty().isNotEmpty() } fun forgetFallbackKey() { diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/OneTimeKeysUploader.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/OneTimeKeysUploader.kt index 9366ecbd6d..88e8f62496 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/OneTimeKeysUploader.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/OneTimeKeysUploader.kt @@ -29,7 +29,9 @@ import javax.inject.Inject import kotlin.math.floor import kotlin.math.min -const val FIVE_MINUTES = 5 * 60_000L +// THe spec recommend a 5mn delay, but due to federation +// or server down we give it a bit more time (1 hour) +const val FALLBACK_KEY_FORGET_DELAY = 60 * 60_000L @SessionScope internal class OneTimeKeysUploader @Inject constructor( @@ -59,8 +61,13 @@ internal class OneTimeKeysUploader @Inject constructor( oneTimeKeyCount = currentCount } - fun setNeedsNewFallback() { - needNewFallbackKey = true + fun needsNewFallback() { + if (olmDevice.generateFallbackKeyIfNeeded()) { + // As we generated a new one, it's already forgetting one + // so we can clear the last publish time + // (in case the network calls fails after to avoid calling forgetKey) + saveLastFallbackKeyPublishTime(0L) + } } /** @@ -120,9 +127,9 @@ internal class OneTimeKeysUploader @Inject constructor( // Check if we need to forget a fallback key val latestPublishedTime = getLastFallbackKeyPublishTime() - if (latestPublishedTime != 0L && System.currentTimeMillis() - latestPublishedTime > FIVE_MINUTES) { + if (latestPublishedTime != 0L && System.currentTimeMillis() - latestPublishedTime > FALLBACK_KEY_FORGET_DELAY) { // This should be called once you are reasonably certain that you will not receive any more messages - // that use the old fallback key (e.g. 5 minutes after the new fallback key has been published) + // that use the old fallback key Timber.d("## forgetFallbackKey()") olmDevice.forgetFallbackKey() } @@ -155,20 +162,9 @@ internal class OneTimeKeysUploader @Inject constructor( keysThisLoop = min(keyLimit - keyCount, ONE_TIME_KEY_GENERATION_MAX_NUMBER) olmDevice.generateOneTimeKeys(keysThisLoop) } - if (needNewFallbackKey && !hasUnpublishedFallbackKey()) { - // if there is already fallback key, but that hasn't been published yet, we - // can use that instead of generating a new one - olmDevice.generateFallbackKey() - Timber.d("maybeUploadOneTimeKeys: Fallback key generated") - // As we generated a new one, it's already forgetting one - // so we can clear the last publish time - // (in case the network calls fails after to avoid calling forgetKey) - saveLastFallbackKeyPublishTime(0L) - } - // not copy paste error we check before sending if there is - // an unpublished key in order to saveLastFallbackKeyPublishTime if needed - val hadUnpublishedFallbackKey = hasUnpublishedFallbackKey() + // We check before sending if there is an unpublished key in order to saveLastFallbackKeyPublishTime if needed + val hadUnpublishedFallbackKey = olmDevice.hasUnpublishedFallbackKey() val response = uploadOneTimeKeys(olmDevice.getOneTimeKeys()) olmDevice.markKeysAsPublished() if (hadUnpublishedFallbackKey) { @@ -189,10 +185,6 @@ internal class OneTimeKeysUploader @Inject constructor( } } - private fun hasUnpublishedFallbackKey(): Boolean { - return olmDevice.getFallbackKey()?.get(OlmAccount.JSON_KEY_ONE_TIME_KEY).orEmpty().isNotEmpty() - } - private fun saveLastFallbackKeyPublishTime(timeMillis: Long) { storage.edit().putLong("last_fb_key_publish", timeMillis).apply() }