Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature/bma/avoid error log #5703

Merged
merged 10 commits into from
Apr 6, 2022
Merged

Feature/bma/avoid error log #5703

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2491559
Avoid error log when the Event is not a LocationEvent
bmarty Apr 6, 2022
45bd106
Small optimization
bmarty Apr 6, 2022
e27dcbc
Create Event.getMsgType() extension
bmarty Apr 6, 2022
e8f421f
Create Event.isLocationMessage()
bmarty Apr 6, 2022
785ccc8
Format file
bmarty Apr 6, 2022
26e80ba
KeysBackup: Better handling of 404 to avoid error log
bmarty Apr 6, 2022
85a0010
Add changelog about the logs
bmarty Apr 6, 2022
7e8c7f3
Fix long line. It's OK to log the whole data class (same log than pre…
bmarty Apr 6, 2022
4d19d03
`matrix-sdk-android-rx` module has been replaced by `matrix-sdk-andro…
bmarty Apr 6, 2022
7cbe77e
Use correct copyright for SDK module
bmarty Apr 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/5703.misc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Reduce error logs
1 change: 1 addition & 0 deletions changelog.d/5703.sdk
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
KeysBackupService.getCurrentVersion takes a new type `KeysBackupLastVersionResult` in the callback.
19 changes: 10 additions & 9 deletions .../src/androidTest/java/org/matrix/android/sdk/internal/crypto/keysbackup/KeysBackupTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,10 +39,11 @@ import org.matrix.android.sdk.common.TestConstants
import org.matrix.android.sdk.common.TestMatrixCallback
import org.matrix.android.sdk.internal.crypto.MXCRYPTO_ALGORITHM_MEGOLM_BACKUP
import org.matrix.android.sdk.internal.crypto.crosssigning.DeviceTrustLevel
import org.matrix.android.sdk.internal.crypto.keysbackup.model.KeysBackupLastVersionResult
import org.matrix.android.sdk.internal.crypto.keysbackup.model.KeysBackupVersionTrust
import org.matrix.android.sdk.internal.crypto.keysbackup.model.MegolmBackupCreationInfo
import org.matrix.android.sdk.internal.crypto.keysbackup.model.rest.KeysVersion
import org.matrix.android.sdk.internal.crypto.keysbackup.model.rest.KeysVersionResult
import org.matrix.android.sdk.internal.crypto.keysbackup.model.toKeysVersionResult
import org.matrix.android.sdk.internal.crypto.model.ImportRoomKeysResult
import java.util.Collections
import java.util.concurrent.CountDownLatch
Expand Down Expand Up @@ -403,9 +404,9 @@ class KeysBackupTest : InstrumentedTest {
assertTrue(testData.aliceSession2.cryptoService().keysBackupService().isEnabled)

// - Retrieve the last version from the server
val keysVersionResult = testHelper.doSync<KeysVersionResult?> {
val keysVersionResult = testHelper.doSync<KeysBackupLastVersionResult> {
testData.aliceSession2.cryptoService().keysBackupService().getCurrentVersion(it)
}
}.toKeysVersionResult()

// - It must be the same
assertEquals(testData.prepareKeysBackupDataResult.version, keysVersionResult!!.version)
Expand Down Expand Up @@ -463,9 +464,9 @@ class KeysBackupTest : InstrumentedTest {
assertTrue(testData.aliceSession2.cryptoService().keysBackupService().isEnabled)

// - Retrieve the last version from the server
val keysVersionResult = testHelper.doSync<KeysVersionResult?> {
val keysVersionResult = testHelper.doSync<KeysBackupLastVersionResult> {
testData.aliceSession2.cryptoService().keysBackupService().getCurrentVersion(it)
}
}.toKeysVersionResult()

// - It must be the same
assertEquals(testData.prepareKeysBackupDataResult.version, keysVersionResult!!.version)
Expand Down Expand Up @@ -565,9 +566,9 @@ class KeysBackupTest : InstrumentedTest {
assertTrue(testData.aliceSession2.cryptoService().keysBackupService().isEnabled)

// - Retrieve the last version from the server
val keysVersionResult = testHelper.doSync<KeysVersionResult?> {
val keysVersionResult = testHelper.doSync<KeysBackupLastVersionResult> {
testData.aliceSession2.cryptoService().keysBackupService().getCurrentVersion(it)
}
}.toKeysVersionResult()

// - It must be the same
assertEquals(testData.prepareKeysBackupDataResult.version, keysVersionResult!!.version)
Expand Down Expand Up @@ -835,9 +836,9 @@ class KeysBackupTest : InstrumentedTest {
keysBackupTestHelper.prepareAndCreateKeysBackupData(keysBackup)

// Get key backup version from the homeserver
val keysVersionResult = testHelper.doSync<KeysVersionResult?> {
val keysVersionResult = testHelper.doSync<KeysBackupLastVersionResult> {
keysBackup.getCurrentVersion(it)
}
}.toKeysVersionResult()

// - Check the returned KeyBackupVersion is trusted
val keysBackupVersionTrust = testHelper.doSync<KeysBackupVersionTrust> {
Expand Down
5 changes: 5 additions & 0 deletions matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/failure/Extensions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,11 @@ fun Throwable.is401() =
httpCode == HttpsURLConnection.HTTP_UNAUTHORIZED && /* 401 */
error.code == MatrixError.M_UNAUTHORIZED

fun Throwable.is404() =
this is Failure.ServerError &&
httpCode == HttpsURLConnection.HTTP_NOT_FOUND && /* 404 */
error.code == MatrixError.M_NOT_FOUND

fun Throwable.isTokenError() =
this is Failure.ServerError &&
(error.code == MatrixError.M_UNKNOWN_TOKEN ||
Expand Down
5 changes: 3 additions & 2 deletions ...d/src/main/java/org/matrix/android/sdk/api/session/crypto/keysbackup/KeysBackupService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package org.matrix.android.sdk.api.session.crypto.keysbackup
import org.matrix.android.sdk.api.MatrixCallback
import org.matrix.android.sdk.api.listeners.ProgressListener
import org.matrix.android.sdk.api.listeners.StepProgressListener
import org.matrix.android.sdk.internal.crypto.keysbackup.model.KeysBackupLastVersionResult
import org.matrix.android.sdk.internal.crypto.keysbackup.model.KeysBackupVersionTrust
import org.matrix.android.sdk.internal.crypto.keysbackup.model.MegolmBackupCreationInfo
import org.matrix.android.sdk.internal.crypto.keysbackup.model.rest.KeysVersion
Expand All @@ -31,9 +32,9 @@ interface KeysBackupService {
* Retrieve the current version of the backup from the homeserver
*
* It can be different than keysBackupVersion.
* @param callback onSuccess(null) will be called if there is no backup on the server
* @param callback Asynchronous callback
*/
fun getCurrentVersion(callback: MatrixCallback<KeysVersionResult?>)
fun getCurrentVersion(callback: MatrixCallback<KeysBackupLastVersionResult>)

/**
* Create a new keys backup version and enable it, using the information return from [prepareKeysBackupVersion].
Expand Down
84 changes: 48 additions & 36 deletions matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/events/model/Event.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,9 @@ data class Event(
fun getDecryptedTextSummary(): String? {
if (isRedacted()) return "Message Deleted"
val text = getDecryptedValue() ?: run {
if (isPoll()) { return getPollQuestion() ?: "created a poll." }
if (isPoll()) {
return getPollQuestion() ?: "created a poll."
}
return null
}

Expand Down Expand Up @@ -300,57 +302,67 @@ data class Event(
}
}

/**
* Return the value of "content.msgtype", if the Event type is "m.room.message"
* and if the content has it, and if it is a String
*/
fun Event.getMsgType(): String? {
if (getClearType() != EventType.MESSAGE) return null
return getClearContent()?.get(MessageContent.MSG_TYPE_JSON_KEY) as? String
}

fun Event.isTextMessage(): Boolean {
return getClearType() == EventType.MESSAGE &&
when (getClearContent()?.get(MessageContent.MSG_TYPE_JSON_KEY)) {
MessageType.MSGTYPE_TEXT,
MessageType.MSGTYPE_EMOTE,
MessageType.MSGTYPE_NOTICE -> true
else -> false
}
return when (getMsgType()) {
MessageType.MSGTYPE_TEXT,
MessageType.MSGTYPE_EMOTE,
MessageType.MSGTYPE_NOTICE -> true
else -> false
}
}

fun Event.isImageMessage(): Boolean {
return getClearType() == EventType.MESSAGE &&
when (getClearContent()?.get(MessageContent.MSG_TYPE_JSON_KEY)) {
MessageType.MSGTYPE_IMAGE -> true
else -> false
}
return when (getMsgType()) {
MessageType.MSGTYPE_IMAGE -> true
else -> false
}
}

fun Event.isVideoMessage(): Boolean {
return getClearType() == EventType.MESSAGE &&
when (getClearContent()?.get(MessageContent.MSG_TYPE_JSON_KEY)) {
MessageType.MSGTYPE_VIDEO -> true
else -> false
}
return when (getMsgType()) {
MessageType.MSGTYPE_VIDEO -> true
else -> false
}
}

fun Event.isAudioMessage(): Boolean {
return getClearType() == EventType.MESSAGE &&
when (getClearContent()?.get(MessageContent.MSG_TYPE_JSON_KEY)) {
MessageType.MSGTYPE_AUDIO -> true
else -> false
}
return when (getMsgType()) {
MessageType.MSGTYPE_AUDIO -> true
else -> false
}
}

fun Event.isFileMessage(): Boolean {
return getClearType() == EventType.MESSAGE &&
when (getClearContent()?.get(MessageContent.MSG_TYPE_JSON_KEY)) {
MessageType.MSGTYPE_FILE -> true
else -> false
}
return when (getMsgType()) {
MessageType.MSGTYPE_FILE -> true
else -> false
}
}

fun Event.isAttachmentMessage(): Boolean {
return getClearType() == EventType.MESSAGE &&
when (getClearContent()?.get(MessageContent.MSG_TYPE_JSON_KEY)) {
MessageType.MSGTYPE_IMAGE,
MessageType.MSGTYPE_AUDIO,
MessageType.MSGTYPE_VIDEO,
MessageType.MSGTYPE_FILE -> true
else -> false
}
return when (getMsgType()) {
MessageType.MSGTYPE_IMAGE,
MessageType.MSGTYPE_AUDIO,
MessageType.MSGTYPE_VIDEO,
MessageType.MSGTYPE_FILE -> true
else -> false
}
}

fun Event.isLocationMessage(): Boolean {
return when (getMsgType()) {
MessageType.MSGTYPE_LOCATION -> true
else -> false
}
}

fun Event.isPoll(): Boolean = getClearType() in EventType.POLL_START || getClearType() in EventType.POLL_END
Expand Down
117 changes: 56 additions & 61 deletions ...c/main/java/org/matrix/android/sdk/internal/crypto/keysbackup/DefaultKeysBackupService.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import org.matrix.android.sdk.internal.crypto.MegolmSessionData
import org.matrix.android.sdk.internal.crypto.ObjectSigner
import org.matrix.android.sdk.internal.crypto.actions.MegolmSessionDataImporter
import org.matrix.android.sdk.internal.crypto.crosssigning.fromBase64
import org.matrix.android.sdk.internal.crypto.keysbackup.model.KeysBackupLastVersionResult
import org.matrix.android.sdk.internal.crypto.keysbackup.model.KeysBackupVersionTrust
import org.matrix.android.sdk.internal.crypto.keysbackup.model.KeysBackupVersionTrustSignature
import org.matrix.android.sdk.internal.crypto.keysbackup.model.MegolmBackupAuthData
Expand All @@ -54,6 +55,7 @@ import org.matrix.android.sdk.internal.crypto.keysbackup.model.rest.KeysVersion
import org.matrix.android.sdk.internal.crypto.keysbackup.model.rest.KeysVersionResult
import org.matrix.android.sdk.internal.crypto.keysbackup.model.rest.RoomKeysBackupData
import org.matrix.android.sdk.internal.crypto.keysbackup.model.rest.UpdateKeysBackupVersionBody
import org.matrix.android.sdk.internal.crypto.keysbackup.model.toKeysVersionResult
import org.matrix.android.sdk.internal.crypto.keysbackup.tasks.CreateKeysBackupVersionTask
import org.matrix.android.sdk.internal.crypto.keysbackup.tasks.DeleteBackupTask
import org.matrix.android.sdk.internal.crypto.keysbackup.tasks.DeleteRoomSessionDataTask
Expand Down Expand Up @@ -586,21 +588,28 @@ internal class DefaultKeysBackupService @Inject constructor(

cryptoCoroutineScope.launch(coroutineDispatchers.main) {
try {
val keysBackupVersion = getKeysBackupLastVersionTask.execute(Unit)
val recoveryKey = computeRecoveryKey(secret.fromBase64())
if (isValidRecoveryKeyForKeysBackupVersion(recoveryKey, keysBackupVersion)) {
awaitCallback<Unit> {
trustKeysBackupVersion(keysBackupVersion, true, it)
when (val keysBackupLastVersionResult = getKeysBackupLastVersionTask.execute(Unit)) {
KeysBackupLastVersionResult.NoKeysBackup -> {
Timber.d("No keys backup found")
}
val importResult = awaitCallback<ImportRoomKeysResult> {
restoreKeysWithRecoveryKey(keysBackupVersion, recoveryKey, null, null, null, it)
}
withContext(coroutineDispatchers.crypto) {
cryptoStore.saveBackupRecoveryKey(recoveryKey, keysBackupVersion.version)
is KeysBackupLastVersionResult.KeysBackup -> {
val keysBackupVersion = keysBackupLastVersionResult.keysVersionResult
val recoveryKey = computeRecoveryKey(secret.fromBase64())
if (isValidRecoveryKeyForKeysBackupVersion(recoveryKey, keysBackupVersion)) {
awaitCallback<Unit> {
trustKeysBackupVersion(keysBackupVersion, true, it)
}
val importResult = awaitCallback<ImportRoomKeysResult> {
restoreKeysWithRecoveryKey(keysBackupVersion, recoveryKey, null, null, null, it)
}
withContext(coroutineDispatchers.crypto) {
cryptoStore.saveBackupRecoveryKey(recoveryKey, keysBackupVersion.version)
}
Timber.i("onSecretKeyGossip: Recovered keys ${importResult.successfullyNumberOfImportedKeys} out of ${importResult.totalNumberOfKeys}")
} else {
Timber.e("onSecretKeyGossip: Recovery key is not valid ${keysBackupVersion.version}")
}
}
Timber.i("onSecretKeyGossip: Recovered keys ${importResult.successfullyNumberOfImportedKeys} out of ${importResult.totalNumberOfKeys}")
} else {
Timber.e("onSecretKeyGossip: Recovery key is not valid ${keysBackupVersion.version}")
}
} catch (failure: Throwable) {
Timber.e("onSecretKeyGossip: failed to trust key backup version ${keysBackupVersion?.version}")
Expand Down Expand Up @@ -875,63 +884,49 @@ internal class DefaultKeysBackupService @Inject constructor(
.executeBy(taskExecutor)
}

override fun getCurrentVersion(callback: MatrixCallback<KeysVersionResult?>) {
override fun getCurrentVersion(callback: MatrixCallback<KeysBackupLastVersionResult>) {
getKeysBackupLastVersionTask
.configureWith {
this.callback = object : MatrixCallback<KeysVersionResult> {
override fun onSuccess(data: KeysVersionResult) {
callback.onSuccess(data)
}

override fun onFailure(failure: Throwable) {
if (failure is Failure.ServerError &&
failure.error.code == MatrixError.M_NOT_FOUND) {
// Workaround because the homeserver currently returns M_NOT_FOUND when there is no key backup
callback.onSuccess(null)
} else {
// Transmit the error
callback.onFailure(failure)
}
}
}
this.callback = callback
}
.executeBy(taskExecutor)
}

override fun forceUsingLastVersion(callback: MatrixCallback<Boolean>) {
getCurrentVersion(object : MatrixCallback<KeysVersionResult?> {
override fun onSuccess(data: KeysVersionResult?) {
getCurrentVersion(object : MatrixCallback<KeysBackupLastVersionResult> {
override fun onSuccess(data: KeysBackupLastVersionResult) {
val localBackupVersion = keysBackupVersion?.version
val serverBackupVersion = data?.version

if (serverBackupVersion == null) {
if (localBackupVersion == null) {
// No backup on the server, and backup is not active
callback.onSuccess(true)
} else {
// No backup on the server, and we are currently backing up, so stop backing up
callback.onSuccess(false)
resetKeysBackupData()
keysBackupVersion = null
keysBackupStateManager.state = KeysBackupState.Disabled
}
} else {
if (localBackupVersion == null) {
// backup on the server, and backup is not active
callback.onSuccess(false)
// Do a check
checkAndStartWithKeysBackupVersion(data)
} else {
// Backup on the server, and we are currently backing up, compare version
if (localBackupVersion == serverBackupVersion) {
// We are already using the last version of the backup
when (data) {
KeysBackupLastVersionResult.NoKeysBackup -> {
if (localBackupVersion == null) {
// No backup on the server, and backup is not active
callback.onSuccess(true)
} else {
// We are not using the last version, so delete the current version we are using on the server
// No backup on the server, and we are currently backing up, so stop backing up
callback.onSuccess(false)
resetKeysBackupData()
keysBackupVersion = null
keysBackupStateManager.state = KeysBackupState.Disabled
}
}
is KeysBackupLastVersionResult.KeysBackup -> {
if (localBackupVersion == null) {
// backup on the server, and backup is not active
callback.onSuccess(false)
// Do a check
checkAndStartWithKeysBackupVersion(data.keysVersionResult)
} else {
// Backup on the server, and we are currently backing up, compare version
if (localBackupVersion == data.keysVersionResult.version) {
// We are already using the last version of the backup
callback.onSuccess(true)
} else {
// We are not using the last version, so delete the current version we are using on the server
callback.onSuccess(false)

// This will automatically check for the last version then
deleteBackup(localBackupVersion, null)
// This will automatically check for the last version then
deleteBackup(localBackupVersion, null)
}
}
}
}
Expand All @@ -954,9 +949,9 @@ internal class DefaultKeysBackupService @Inject constructor(
keysBackupVersion = null
keysBackupStateManager.state = KeysBackupState.CheckingBackUpOnHomeserver

getCurrentVersion(object : MatrixCallback<KeysVersionResult?> {
override fun onSuccess(data: KeysVersionResult?) {
checkAndStartWithKeysBackupVersion(data)
getCurrentVersion(object : MatrixCallback<KeysBackupLastVersionResult> {
override fun onSuccess(data: KeysBackupLastVersionResult) {
checkAndStartWithKeysBackupVersion(data.toKeysVersionResult())
}

override fun onFailure(failure: Throwable) {
Expand Down
Loading