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.

Sign up for GitHub

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

e2ee should not hinder verification #1598

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open

e2ee should not hinder verification #1598

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion MatrixSDK/Crypto/Algorithms/MXEncrypting.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@

@return a MXHTTPOperation instance. May be nil if all required materials is already in place.
*/
- (MXHTTPOperation*)ensureSessionForUsers:(NSArray<NSString*>*)users
- (MXHTTPOperation*)ensureSessionForUsers:(NSArray<NSString*>*)users forceDistributeToUnverified: (BOOL) forceDistributeToUnverified
Copy link
Contributor

Choose a reason for hiding this comment

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

Sadly we don't have obj-c style checker / formatter to check this automatically, but to aligh the style:

  • no spaces between type and parameter, i.e. forceDistributeToUnverified: (BOOL) forceDistributeToUnverified => forceDistributeToUnverified:(BOOL)forceDistributeToUnverified (across the whole PR)
  • if some parameters are on multiple lines (e.g. success and failure), then all should be, incl forceDistributeToUnverified, and aligned vertically by the double colon

success:(void (^)(NSObject *sessionInfo))success
failure:(void (^)(NSError *error))failure;

Expand Down
33 changes: 29 additions & 4 deletions MatrixSDK/Crypto/Algorithms/Megolm/MXMegolmEncryption.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ - (MXHTTPOperation*)encryptEventContent:(NSDictionary*)eventContent eventType:(M
queuedEncryption.failure = failure;
[pendingEncryptions addObject:queuedEncryption];

return [self ensureSessionForUsers:users success:^(NSObject *sessionInfo) {
BOOL forceDistributeToUnverified = [self isVerificationEvent:eventType eventContent:eventContent];
return [self ensureSessionForUsers:users forceDistributeToUnverified:forceDistributeToUnverified success:^(NSObject *sessionInfo) {

MXOutboundSessionInfo *session = (MXOutboundSessionInfo*)sessionInfo;
[self processPendingEncryptionsInSession:session withError:nil];
Expand All @@ -103,14 +104,37 @@ - (MXHTTPOperation*)encryptEventContent:(NSDictionary*)eventContent eventType:(M
}];
}

- (MXHTTPOperation*)ensureSessionForUsers:(NSArray<NSString*>*)users
- (BOOL) isVerificationEvent:(MXEventTypeString) eventType eventContent:(NSDictionary*)eventContent
Copy link
Contributor

Choose a reason for hiding this comment

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

Optionally this whole method could be moved into MXTools, as [MXTools isVerificationEvent], which will make it easier to unit test

{
switch ([MXTools eventType:eventType])
{
case MXEventTypeKeyVerificationKey:
case MXEventTypeKeyVerificationMac:
case MXEventTypeKeyVerificationDone:
case MXEventTypeKeyVerificationReady:
case MXEventTypeKeyVerificationStart:
case MXEventTypeKeyVerificationAccept:
case MXEventTypeKeyVerificationCancel: {
return YES;
}
case MXEventTypeRoomMessage: {
NSString *msgType = eventContent[kMXMessageTypeKey];
return [msgType isEqualToString:kMXMessageTypeKeyVerificationRequest];
}
default: {
return NO;
}
}
}

- (MXHTTPOperation*)ensureSessionForUsers:(NSArray<NSString*>*)users forceDistributeToUnverified: (BOOL) forceDistributeToUnverified
success:(void (^)(NSObject *sessionInfo))success
failure:(void (^)(NSError *error))failure
{
NSDate *startDate = [NSDate date];

MXHTTPOperation *operation;
operation = [self getDevicesInRoom:users success:^(MXUsersDevicesMap<MXDeviceInfo *> *devicesInRoom) {
operation = [self getDevicesInRoom:users forceDistributeToUnverified:forceDistributeToUnverified success:^(MXUsersDevicesMap<MXDeviceInfo *> *devicesInRoom) {

MXHTTPOperation *operation2 = [self ensureOutboundSession:devicesInRoom success:^(MXOutboundSessionInfo *session) {

Expand Down Expand Up @@ -166,6 +190,7 @@ - (BOOL)isSessionSharingHistory:(MXOutboundSessionInfo *)session
@param failure A block object called when the operation fails.
*/
- (MXHTTPOperation *)getDevicesInRoom:(NSArray<NSString*>*)users
Copy link
Contributor

Choose a reason for hiding this comment

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

This method includes documentation above so would be good to include the new parameter. Also perhaps just a matter of personal preference, but the method getDevicesInRoom does not suggest anything about distributing keys so perhaps the new parameter would be clearer as includeUnverifiedUsers rather than forceDistributeToUnverified

forceDistributeToUnverified: (BOOL) forceDistributeToUnverified
success:(void (^)(MXUsersDevicesMap<MXDeviceInfo *> *devicesInRoom))success
failure:(void (^)(NSError *))failure
{
Expand Down Expand Up @@ -198,7 +223,7 @@ - (MXHTTPOperation *)getDevicesInRoom:(NSArray<NSString*>*)users
}

if (deviceInfo.trustLevel.localVerificationStatus == MXDeviceBlocked
|| (!deviceInfo.trustLevel.isVerified && encryptToVerifiedDevicesOnly))
|| (!deviceInfo.trustLevel.isVerified && encryptToVerifiedDevicesOnly && !forceDistributeToUnverified))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better / cleaner to include this extra condition in the BOOL encryptToVerifiedDevicesOnly = declaration.

{
// Remove any blocked devices
MXLogDebug(@"[MXMegolmEncryption] getDevicesInRoom: blocked device: %@", deviceInfo);
Expand Down
4 changes: 2 additions & 2 deletions MatrixSDK/Crypto/Algorithms/Olm/MXOlmEncryption.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ - (MXHTTPOperation*)encryptEventContent:(NSDictionary*)eventContent eventType:(M
failure:(void (^)(NSError *error))failure
{
MXWeakify(self);
return [self ensureSessionForUsers:users success:^(NSObject *sessionInfo) {
return [self ensureSessionForUsers:users forceDistributeToUnverified:NO success:^(NSObject *sessionInfo) {
MXStrongifyAndReturnIfNil(self);

NSMutableArray *participantDevices = [NSMutableArray array];
Expand Down Expand Up @@ -99,7 +99,7 @@ - (MXHTTPOperation*)encryptEventContent:(NSDictionary*)eventContent eventType:(M
} failure:failure];
}

- (MXHTTPOperation*)ensureSessionForUsers:(NSArray<NSString*>*)users
- (MXHTTPOperation*)ensureSessionForUsers:(NSArray<NSString*>*)users forceDistributeToUnverified: (BOOL) forceDistributeToUnverified
success:(void (^)(NSObject *sessionInfo))success
failure:(void (^)(NSError *error))failure
{
Expand Down
2 changes: 1 addition & 1 deletion MatrixSDK/Crypto/MXCrypto.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -757,7 +757,7 @@ - (MXHTTPOperation*)ensureEncryptionInRoom:(NSString*)roomId
if (alg)
{
// Check we have everything to encrypt events
MXHTTPOperation *operation2 = [alg ensureSessionForUsers:userIds success:^(NSObject *sessionInfo) {
MXHTTPOperation *operation2 = [alg ensureSessionForUsers:userIds forceDistributeToUnverified:NO success:^(NSObject *sessionInfo) {

if (success)
{
Expand Down
1 change: 1 addition & 0 deletions changelog.d/6519.bugfix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Can't verify user when option to send keys to verified devices only is selected