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

Share Megolm session keys when inviting a new user #1447

Merged
merged 2 commits into from
Apr 27, 2022

Conversation

Anderas
Copy link
Contributor

@Anderas Anderas commented Apr 26, 2022

Relates to element-hq/element-ios#4947 which is an implementation of matrix-org/matrix-spec-proposals#3061

After previously storing sharedHistory flag for inbound Megolm sessions, add new mechanism that will share these inbound keys with a user invited to the current room.

Because the current Rest API allows sharing of only one key at a time and because sharing keys always increases the amount of potential issues (e.g. how far in the past?, shared with which devices?, do we retry if sharing fails?), the scope of this feature is currently limited to only share the "immediate context", which is arbitrarily interpreted as keys for the last 20 messages in this room. This should achieve the outcome that when you are invited to a room, you can see any immediate messages before that may be related to you invitation.

We will be re-implementing key-sharing in RustSDK where the feature will be more robust and accounting for more edge cases.

In this PR:

  • add new MXSharedHistoryKeyManager (equivalent to existing MXOutgoingRoomKeyRequestManager and MXIncomingRoomKeyRequestManager), which handles the complexity of fetching the last 20 messages, extracting their session details, and creating key sharing requests accordingly
  • minor refactor of MXMegolmDecryption to allow sharing of keys via MXSharedHistoryKeyRequest. Previously one could only share keys with one device per user at a time, with the new request keys may be shared with multiple devices at once. Existing tests (in particular MXCryptoShareTests.testNominalCase()) should ensure the refactor did not break existing behaviour
  • implement key sharing in MXRoom when a user is invited. The behaviour is controlled by a build / config flag, so that forms of the project can disable this behavior.

@Anderas Anderas requested review from a team and gileluard and removed request for a team April 26, 2022 15:18
@Anderas Anderas force-pushed the andy/4947_sharing_keys branch from 5493d8b to e941973 Compare April 26, 2022 15:25
@Anderas Anderas force-pushed the andy/4947_sharing_keys branch from e941973 to 07526b3 Compare April 26, 2022 15:26
Copy link
Contributor

@gileluard gileluard left a comment

Choose a reason for hiding this comment

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

LGTM. Please just check braces for objective-c files.

chainIndex:nil];

MXUsersDevicesMap<NSDictionary*> *contentMap = [[MXUsersDevicesMap alloc] init];
for (MXDeviceInfo *deviceInfo in devices) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
for (MXDeviceInfo *deviceInfo in devices) {
for (MXDeviceInfo *deviceInfo in devices)
{

MXUsersDevicesMap<NSDictionary*> *contentMap = [[MXUsersDevicesMap alloc] init];
for (MXDeviceInfo *deviceInfo in devices) {
MXOlmSessionResult *olmSessionResult = [results objectForDevice:deviceInfo.deviceId forUser:userId];
if (olmSessionResult.sessionId) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (olmSessionResult.sessionId) {
if (olmSessionResult.sessionId)
{

if (olmSessionResult.sessionId) {
NSDictionary *message = [self->crypto encryptMessage:payload forDevices:@[deviceInfo]];
[contentMap setObject:message forUser:userId andDevice:deviceInfo.deviceId];
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
} else {
}
else
{

}
}

if (contentMap.count == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (contentMap.count == 0) {
if (contentMap.count == 0)
{

@@ -116,6 +121,11 @@ - (id)initWithRoomId:(NSString *)roomId matrixSession:(MXSession *)mxSession2 an
{
_roomId = roomId;
mxSession = mxSession2;

if (mxSession.crypto) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (mxSession.crypto) {
if (mxSession.crypto)
{

@@ -1932,9 +1942,23 @@ - (MXHTTPOperation*)inviteUser:(NSString*)userId
success:(void (^)(void))success
failure:(void (^)(NSError *error))failure
{
if (MXSDKOptions.sharedInstance.enableRoomSharedHistoryOnInvite) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (MXSDKOptions.sharedInstance.enableRoomSharedHistoryOnInvite) {
if (MXSDKOptions.sharedInstance.enableRoomSharedHistoryOnInvite)
{

@Anderas
Copy link
Contributor Author

Anderas commented Apr 27, 2022

LGTM. Please just check braces for objective-c files.

Thx, would be great to have a linter / formatter for this, as one has to fight the Xcode tooling / autocompletion with this style

@Anderas Anderas merged commit 7864fdf into develop Apr 27, 2022
@Anderas Anderas deleted the andy/4947_sharing_keys branch April 27, 2022 14:40
@Anderas Anderas requested a review from BillCarsonFr April 28, 2022 13:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants