Skip to content

Conversation

@uhoreg
Copy link
Member

@uhoreg uhoreg commented Aug 27, 2024

Replaces #3701. Part of invisible crypto.

The room event decryption function now takes a DecryptionSetting which allows you to require that event senders have a certain trust level based on their cross-signing status. Events that do not meet the required trust level will fail to decrypt with an error.

  • Public API changes documented in changelogs (optional)

Signed-off-by:

@codecov
Copy link

codecov bot commented Aug 27, 2024

Codecov Report

Attention: Patch coverage is 86.95652% with 6 lines in your changes missing coverage. Please review.

Project coverage is 84.08%. Comparing base (468ee53) to head (00e079b).
Report is 81 commits behind head on main.

Files with missing lines Patch % Lines
crates/matrix-sdk-crypto/src/machine/mod.rs 86.66% 4 Missing ⚠️
...es/matrix-sdk-common/src/deserialized_responses.rs 71.42% 2 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3899   +/-   ##
=======================================
  Coverage   84.07%   84.08%           
=======================================
  Files         266      266           
  Lines       27861    27894   +33     
=======================================
+ Hits        23425    23455   +30     
- Misses       4436     4439    +3     

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

@uhoreg uhoreg requested a review from richvdh August 28, 2024 02:02
@uhoreg uhoreg marked this pull request as ready for review August 28, 2024 02:02
@uhoreg uhoreg requested review from a team as code owners August 28, 2024 02:02
@richvdh
Copy link
Member

richvdh commented Aug 28, 2024

@uhoreg does this relate to a specific issue? Please link it if so. If not, please give more context about what we're trying to achieve in this PR.

@bnjbvr
Copy link
Member

bnjbvr commented Aug 28, 2024

(Also please follow the contribution guidelines for naming PRs and commits! It helps figuring out which PR relates to what :))

CrossSignedOrLegacy,
/// Only decrypt events from cross-signed devices.
CrossSigned,
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you give some more explanation about what happens to events that do not met these requirements?

@uhoreg uhoreg force-pushed the invisible_crypto/decrypt2 branch from c37d9e7 to 0447cd1 Compare August 29, 2024 03:11
@uhoreg
Copy link
Member Author

uhoreg commented Aug 29, 2024

I think that I've addressed all the review comments

@uhoreg uhoreg requested a review from richvdh August 29, 2024 03:27
@bnjbvr
Copy link
Member

bnjbvr commented Aug 29, 2024

Can you follow the contribution guidelines for the naming of this PR and the commits, please?

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

Looks good to me now except:

  • Per #3899 (comment), #[derive(Error)] seems like an unclear way to implement Display.
  • Per #3899 (comment), please update the description of the PR to describe what it is we're fixing.
  • Per benji's requests, please give the PR a name that reflects the area of the code that is affected (crypto:, I guess)
  • Also please rebase your commits to remove the "minor fixes" and "address review", and rename them so that they match the contributing guide. If you pick the right point on main to rebase on top of (468ee53), then github will show the force-push with no changes, which means we keep an audit trail that the code has been reviewed.

@uhoreg uhoreg changed the title Configurable sender trust checking on decrypting crypto: Configurable sender trust checking on decrypting Aug 31, 2024
@uhoreg uhoreg force-pushed the invisible_crypto/decrypt2 branch from 0447cd1 to 00e079b Compare September 4, 2024 03:18
@uhoreg uhoreg requested a review from richvdh September 4, 2024 03:53
Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

🎉 🚢

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