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

crypto: Expose a method to pin a user's identity #4068

Merged
merged 2 commits into from
Oct 7, 2024

Conversation

andybalaam
Copy link
Contributor

@andybalaam andybalaam requested review from a team as code owners October 3, 2024 14:58
@andybalaam andybalaam requested review from stefanceriu and BillCarsonFr and removed request for a team October 3, 2024 14:58
crates/matrix-sdk/src/room/mod.rs Outdated Show resolved Hide resolved
crates/matrix-sdk/src/room/mod.rs Outdated Show resolved Hide resolved
bindings/matrix-sdk-ffi/src/room.rs Outdated Show resolved Hide resolved
Copy link

codecov bot commented Oct 3, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 84.66%. Comparing base (351fbf6) to head (ee67ca8).
Report is 4 commits behind head on main.

Files with missing lines Patch % Lines
crates/matrix-sdk-crypto/src/identities/user.rs 75.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4068   +/-   ##
=======================================
  Coverage   84.66%   84.66%           
=======================================
  Files         269      269           
  Lines       28694    28700    +6     
=======================================
+ Hits        24294    24300    +6     
  Misses       4400     4400           

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

Copy link
Member

@BillCarsonFr BillCarsonFr left a comment

Choose a reason for hiding this comment

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

A bit original to have the API on room, but looks like it's convenient for apps

crates/matrix-sdk/src/room/mod.rs Outdated Show resolved Hide resolved
bindings/matrix-sdk-ffi/CHANGELOG.md Outdated Show resolved Hide resolved
crates/matrix-sdk/src/room/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@poljar poljar left a comment

Choose a reason for hiding this comment

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

Left some nits, looks good otherwise.

bindings/matrix-sdk-ffi/src/encryption.rs Show resolved Hide resolved
/// the previously pinned one and bringing it out of pin violation.
///
/// An identity which is not verified and is in pin violation will require
/// user approval before it can be used. See
Copy link
Contributor

Choose a reason for hiding this comment

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

Used how? Maybe we should expand on that and perhaps link to the different settings (TrustRequirement?) which are affected by this.

Copy link
Member

Choose a reason for hiding this comment

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

Yep, we should, though I'm out of my depth here since I've not been keeping this close of an eye on the new trust requirement code.

Copy link
Member

@BillCarsonFr BillCarsonFr Oct 7, 2024

Choose a reason for hiding this comment

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

We don't require the pin-violation to be resolved before using the identity anymore (using here is for sharing room key or trust requirement for decryption). It was considered but it is per design a non blocking warning, so the sdk remembers if the warning was displayed to the user, until resolved the warning should be shown by UI.
So just outdated doc?

Copy link
Member

@dkasak dkasak Oct 7, 2024

Choose a reason for hiding this comment

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

Hmm, I have a number of questions now.

  • What exactly is the effect of identity_needs_user_approval() == true? It simply displays a warning with no other effect?
  • Do we differentiate between an identity_needs_user_approval() == true state created due to a verified identity mismatch vs a pin violation? Or is the effect exactly the same in both cases?
    • My question doesn't make sense since identity_needs_user_approval() == true is never created due to a verified identity mismatch. So what is the behaviour when a verified identity is mismatched? And what is the name of the method which enforces this case?
  • Is any of this going to change after the "transition period"?

Also, can you suggest wording that would make it accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What exactly is the effect of identity_needs_user_approval() == true? It simply displays a warning with no other effect?

Yes. (This is for unverified identities.)

So what is the behaviour when a verified identity is mismatched?

In legacy/transition mode (they are the same) we will block sending and decrypting messages to/from an identity with a verification violation. I think it will be the same in future too.

what is the name of the method which enforces this case?

withdraw_verification changes a (potentially-violating) identity back into a pinned identity. has_verification_violation asks whether this identity has a verification violation.

I find these methods very confusing and am trying to think about how to structure them better.

Is any of this going to change after the "transition period"?

I don't think so.

Also, can you suggest wording that would make it accurate?

I went for "UIs should display a warning to the user when encountering an identity which is not verified and is in pin violation."

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.

6 participants