Respect isShareKeysOnInviteEnabled() flag when creating new outbound megolm sessions

[bradtgmurray]

Signed-off-by: Brad Murray [email protected]

Type of change

• Feature
• Bugfix
• Technical
• Other :

Content

This code...

https://github.com/vector-im/element-android/blob/cf366f7a9c1e83284e91702400fde2371af2b02a/matrix-sdk-android/src/kotlinCrypto/java/org/matrix/android/sdk/internal/crypto/algorithms/megolm/MXMegolmEncryption.kt#L211

...is comparing what's currently in the CryptoStore for the room to the current session were using and rotating the session if they disagreed about sharing history. When checking the CryptoStore for the current room state, the feature flag is respected...

https://github.com/vector-im/element-android/blob/77a6c67ea6adb8a599f9f521e23c0b374fdca341/matrix-sdk-android/src/kotlinCrypto/java/org/matrix/android/sdk/internal/crypto/store/db/RealmCryptoStore.kt#L760

...but when we were creating a new session as a result we aren't checking the state of the flag, meaning we'll create new sessions with shouldShareHistory = true. This will lead to the session still mismatching and continuing to get unecessarily rotated. This PR respects the feature flag so that new sessions will be created with sharedHistory = false if the flag is disabled.

Motivation and context

We shouldn't unecessarily rotate the megolm keys more than necessary when this feature flag is off, which is the default setting.

Checklist

• Changes has been tested on an Android device or Android emulator with API 21
• UI change has been tested on both light and dark themes
• Accessibility has been taken into account. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#accessibility
• Pull request is based on the develop branch
• Pull request includes a new file under ./changelog.d. See https://github.com/vector-im/element-android/blob/develop/CONTRIBUTING.md#changelog
• Pull request includes screenshots or videos if containing UI changes
• Pull request includes a sign off
• You've made a self review of your PR
• If you have modified the screen flow, or added new screens to the application, you have updated the test UiAllScreensSanityTest.allScreensTest()

[bmarty]

Thanks for the PR @bradtgmurray .

@BillCarsonFr can you look at it please?

[BillCarsonFr]

Good catch, but not sure we want to fix it exactly like that.
The CryptoRoomEntity#shouldShareHistory should match what the state of the room is. So we probably want to remove the if (!isShareKeysOnInviteEnabled()) return false check in CryptoStore#shouldShareHistory()

And then we should in prepareNewSessionInRoom() override the flag on the outbound session using the flag. And in ensureOutboundSession we move up the check on the flag instead of just checking the room flag (that just reflects the server side room state)

YDT?

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}