Skip to content

Conversation

@BillCarsonFr
Copy link
Member

@BillCarsonFr BillCarsonFr commented Oct 5, 2022

Pull Request Checklist

Fixes #6519
Verification events sent in e2ee rooms should bypass the encrypt to verified devices only option. If not it renders verification impossible.

Motivation and context

As per spec:

When using in-room messages and the room has encryption enabled, clients should ensure that encryption does not hinder the verification. For example, if the verification messages are encrypted, clients must ensure that all the recipient’s unverified devices receive the keys necessary to decrypt the messages, even if they would normally not be given the keys to decrypt messages in the room. Alternatively, verification messages may be sent unencrypted, though this is not encouraged.

@BillCarsonFr BillCarsonFr requested review from a team and manuroe October 5, 2022 09:40
@codecov
Copy link

codecov bot commented Oct 5, 2022

Codecov Report

❌ Patch coverage is 0% with 27 lines in your changes missing coverage. Please review.
✅ Project coverage is 13.48%. Comparing base (6178f5f) to head (248ab03).
⚠️ Report is 836 commits behind head on develop.

Files with missing lines Patch % Lines
...xSDK/Crypto/Algorithms/Megolm/MXMegolmEncryption.m 0.00% 25 Missing ⚠️
MatrixSDK/Crypto/Algorithms/Olm/MXOlmEncryption.m 0.00% 1 Missing ⚠️
MatrixSDK/Crypto/MXCrypto.m 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1598      +/-   ##
===========================================
- Coverage    13.48%   13.48%   -0.01%     
===========================================
  Files          541      541              
  Lines        86690    86712      +22     
  Branches     36828    36842      +14     
===========================================
  Hits         11693    11693              
- Misses       74555    74577      +22     
  Partials       442      442              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@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

@param success A block object called when the operation succeeds.
@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


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.

}

- (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

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.

Can't verify user when option to send keys to verified devices only is selected

2 participants