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.

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

Fix decrypting redacted event #6148

Merged
merged 4 commits into from
May 25, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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/6148.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix decrypting redacted event from sending errors
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
/*
* Copyright 2022 The Matrix.org Foundation C.I.C.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package org.matrix.android.sdk.internal.crypto

import androidx.test.ext.junit.runners.AndroidJUnit4
import org.junit.Assert
import org.junit.FixMethodOrder
import org.junit.Test
import org.junit.runner.RunWith
import org.junit.runners.MethodSorters
import org.matrix.android.sdk.InstrumentedTest
import org.matrix.android.sdk.api.session.events.model.Event
import org.matrix.android.sdk.api.session.events.model.toModel
import org.matrix.android.sdk.api.session.getRoom
import org.matrix.android.sdk.api.session.room.getTimelineEvent
import org.matrix.android.sdk.common.CommonTestHelper
import org.matrix.android.sdk.common.CryptoTestHelper

@RunWith(AndroidJUnit4::class)
@FixMethodOrder(MethodSorters.JVM)
class DecryptRedactedEventTest : InstrumentedTest {

@Test
fun doNotFailToDecryptRedactedEvent() {
val testHelper = CommonTestHelper(context())
val cryptoTestHelper = CryptoTestHelper(testHelper)

val testData = cryptoTestHelper.doE2ETestWithAliceAndBobInARoom(true)
val e2eRoomID = testData.roomId
val aliceSession = testData.firstSession
val bobSession = testData.secondSession!!

val roomALicePOV = aliceSession.getRoom(e2eRoomID)!!
val timelineEvent = testHelper.sendTextMessage(roomALicePOV, "Hello", 1).first()
val redactionReason = "Wrong Room"
roomALicePOV.sendService().redactEvent(timelineEvent.root, redactionReason)

// get the event from bob
testHelper.waitWithLatch {
testHelper.retryPeriodicallyWithLatch(it) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't know this tricky helper function :) I was always trying to listen sync response. Test would loose time ~1 seconds but not a big problem for now.

bobSession.getRoom(e2eRoomID)?.getTimelineEvent(timelineEvent.eventId)?.root?.isRedacted() == true
}
}

val eventBobPov = bobSession.getRoom(e2eRoomID)?.getTimelineEvent(timelineEvent.eventId)!!

testHelper.runBlockingTest {
try {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need the try/catch with custom error message?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I updated the test.

val result = bobSession.cryptoService().decryptEvent(eventBobPov.root, "")
Assert.assertEquals(
"Unexpected redacted reason",
redactionReason,
result.clearEvent.toModel<Event>()?.unsignedData?.redactedEvent?.content?.get("reason")
)
Assert.assertEquals(
"Unexpected Redacted event id",
timelineEvent.eventId,
result.clearEvent.toModel<Event>()?.unsignedData?.redactedEvent?.redacts
)
} catch (failure: Throwable) {
Assert.fail("Should not throw when decrypting a redacted event")
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import org.matrix.android.sdk.api.session.crypto.model.MXUsersDevicesMap
import org.matrix.android.sdk.api.session.events.model.Event
import org.matrix.android.sdk.api.session.events.model.EventType
import org.matrix.android.sdk.api.session.events.model.content.OlmEventContent
import org.matrix.android.sdk.api.session.events.model.toContent
import org.matrix.android.sdk.api.session.events.model.toModel
import org.matrix.android.sdk.internal.crypto.actions.EnsureOlmSessionsForDevicesAction
import org.matrix.android.sdk.internal.crypto.actions.MessageEncrypter
Expand All @@ -42,7 +43,7 @@ import javax.inject.Inject

private const val SEND_TO_DEVICE_RETRY_COUNT = 3

private val loggerTag = LoggerTag("CryptoSyncHandler", LoggerTag.CRYPTO)
private val loggerTag = LoggerTag("EventDecryptor", LoggerTag.CRYPTO)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


@SessionScope
internal class EventDecryptor @Inject constructor(
Expand Down Expand Up @@ -110,6 +111,16 @@ internal class EventDecryptor @Inject constructor(
if (eventContent == null) {
Timber.tag(loggerTag.value).e("decryptEvent : empty event content")
throw MXCryptoError.Base(MXCryptoError.ErrorType.BAD_ENCRYPTED_MESSAGE, MXCryptoError.BAD_ENCRYPTED_MESSAGE_REASON)
} else if (event.isRedacted()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if returning an empty result is confusing and difficult to back trace, what do you think about having the callers of internalDecryptEvent avoid passing known non-processable events?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think to have it in a single place as there could be several callers (and future callers). Also if at some point the redaction reason is encrypted it would have to be processed here I think

// we shouldn't attempt to decrypt a redacted event because the content is cleared and decryption will fail because of null algorithm
return MXEventDecryptionResult(
clearEvent = mapOf(
"room_id" to event.roomId.orEmpty(),
"type" to EventType.MESSAGE,
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
"content" to emptyMap<String, Any>(),
"unsigned" to event.unsignedData.toContent()
)
)
} else {
val algorithm = eventContent["algorithm"]?.toString()
val alg = roomDecryptorProvider.getOrCreateRoomDecryptor(event.roomId, algorithm)
Expand Down