Fix decrypting redacted event #6148
Conversation
BillCarsonFr
requested review from
a team,
onurays and
bmarty
and removed request for
a team
| @@ -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) | |||
BillCarsonFr
force-pushed
the
feature/bca/fix_decrypt_redacted_event
branch
from
52fc6d8 to
7a739dc
Compare
| @@ -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()) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| val eventBobPov = bobSession.getRoom(e2eRoomID)?.getTimelineEvent(timelineEvent.eventId)!! | ||
|
|
||
| testHelper.runBlockingTest { | ||
| try { |
There was a problem hiding this comment.
do we need the try/catch with custom error message?
There was a problem hiding this comment.
I updated the test.
|
|
||
| testHelper.runBlockingTest { | ||
| try { | ||
| bobSession.cryptoService().decryptEvent(eventBobPov.root, "") |
There was a problem hiding this comment.
this test currently doesn't assert any results, only that an exception is not thrown (which all junit tests handle by default), this means the payload of https://github.com/vector-im/element-android/pull/6148/files#diff-090252be59df798b53cf50ae12a6826f79c92565b02d8a557bf589ba52f3d40fR116 could change and the test would potentially still pass
val result = bobSession.cryptoService().decryptEvent(eventBobPov.root, "")
result shouldBeEqualTo MXEventDecryptionResult(clearEvent = mapOf(
"room_id" to "roomId",
...
)
)There was a problem hiding this comment.
It asserts that it doesn't throw with an understandable message. But yes it could check more
matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/EventDecryptor.kt
Show resolved
Hide resolved
matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/crypto/EventDecryptor.kt
Outdated
Show resolved
Hide resolved
|
|
||
| // get the event from bob | ||
| testHelper.waitWithLatch { | ||
| testHelper.retryPeriodicallyWithLatch(it) { |
There was a problem hiding this comment.
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.
BillCarsonFr
force-pushed
the
feature/bca/fix_decrypt_redacted_event
branch
from
29e463f to
d7c8abb
Compare
Type of change
Content
The SDK was throwing decryption errors (null algorithm) when trying to decrypt a redacted event.
This was not really visible to the user but it could trigger auto rageshakes (if on in settings), and report decryption failure to posthig
Change is as per js-sdk https://github.com/matrix-org/matrix-js-sdk/blob/ad030bfc1f919fecd80e249fc69769ce58bf9201/src/crypto/index.ts#L2873
Motivation and context
Screenshots / GIFs
Tested devices
Checklist