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

Add event decryption decoration #1743

Merged
merged 1 commit into from
Mar 27, 2023
Merged

Add event decryption decoration #1743

merged 1 commit into from
Mar 27, 2023

Conversation

Anderas
Copy link
Contributor

@Anderas Anderas commented Mar 21, 2023

Previously the legacy SDK would expose untrusted property to mark events that were decrypted using an untrusted key (e.g. one downloaded from asymmetrical backup). On the app side, this untrusted property would be combined with other encryption properties, namely the trust of the sender and their device. Only then the combine result gets turned into a visual decoration (e.g. red or grey shield).

To improve this, and align with rust-sdk, expose the decoration color directly from the crypto SDK, alongside an explanation message if necessary. This moves some of the trust computation down to the crypto module and leaves the app with fewer decisions to make.

@Anderas Anderas requested review from a team and nimau and removed request for a team March 21, 2023 11:23
@codecov
Copy link

codecov bot commented Mar 21, 2023

Codecov Report

Patch coverage: 30.76% and project coverage change: +0.02 🎉

Comparison is base (87920ab) 25.83% compared to head (9e5a4ac) 25.85%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1743      +/-   ##
===========================================
+ Coverage    25.83%   25.85%   +0.02%     
===========================================
  Files          615      616       +1     
  Lines        96992    97076      +84     
  Branches     41796    41832      +36     
===========================================
+ Hits         25060    25102      +42     
- Misses       71114    71155      +41     
- Partials       818      819       +1     
Impacted Files Coverage Δ
...K/Background/Crypto/MXLegacyBackgroundCrypto.swift 0.00% <0.00%> (ø)
...o/Algorithms/RoomEvent/MXRoomEventDecryption.swift 2.34% <0.00%> (+0.02%) ⬆️
...lgorithms/Megolm/MXMegolmDecryptionUnitTests.swift 0.00% <0.00%> (ø)
...sts/Crypto/CryptoMachine/DecryptedEvent+Stub.swift 0.00% <0.00%> (ø)
MatrixSDKTests/JSONModels/MXEventFixtures.swift 0.00% <0.00%> (ø)
MatrixSDK/JSONModels/MXEvent.m 29.57% <40.00%> (ø)
...xSDK/Crypto/Algorithms/Megolm/MXMegolmDecryption.m 85.79% <53.84%> (-1.22%) ⬇️
...sions/MXEventDecryptionResult+DecryptedEvent.swift 86.20% <88.23%> (-2.03%) ⬇️
...rypto/Algorithms/MXEventDecryptionDecoration.swift 100.00% <100.00%> (ø)
...trixSDK/Crypto/CryptoMachine/MXCryptoMachine.swift 17.06% <100.00%> (+0.37%) ⬆️

... and 9 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nimau
Copy link
Contributor

nimau commented Mar 21, 2023

Hi @Anderas,
This sounds good to me but I'm not familiar with this part, do you mind to ask another review from the crypto team ?

@Anderas Anderas requested a review from pixlwave March 21, 2023 16:30
Copy link
Member

@pixlwave pixlwave left a comment

Choose a reason for hiding this comment

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

LGTM 😎

Copy link
Member

Choose a reason for hiding this comment

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

Should we have a .api changelog too that documents the removal of the untrusted property and its replacements for other users of the SDK?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea that makes sense, renamed. thx

@Anderas Anderas force-pushed the andy/trust_shields branch 2 times, most recently from 04894c0 to a7c9b01 Compare March 27, 2023 14:02
@Anderas Anderas merged commit abcef50 into develop Mar 27, 2023
@Anderas Anderas deleted the andy/trust_shields branch March 27, 2023 16:16
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.

3 participants