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

Respect isShareKeysOnInviteEnabled() flag when creating new outbound megolm sessions #8540

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

bradtgmurray
Copy link

@bradtgmurray bradtgmurray commented Jun 14, 2023

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

@bmarty bmarty added the Z-Community-PR Issue is solved by a community member's PR label Jun 16, 2023
@bmarty bmarty requested a review from BillCarsonFr June 16, 2023 09:45
@bmarty
Copy link
Member

bmarty commented Jun 16, 2023

Thanks for the PR @bradtgmurray .

@BillCarsonFr can you look at it please?

@BillCarsonFr
Copy link
Member

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
Copy link

CLA assistant check
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants