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

Session verification incoming request support #4153

Merged
merged 11 commits into from
Oct 28, 2024

Conversation

stefanceriu
Copy link
Member

@stefanceriu stefanceriu commented Oct 18, 2024

This PR introduced FFI level API support for interacting with incoming session verification requests. Best reviewed commit by commit

  • it exposes a new did_receive_verification_request SessionVerificationController delegate method that includes information about the incoming request/device/sender
  • the client can acknowledge_verification_request to get informed about it getting accepted somewhere else or cancelled
  • the client can also accept_verification_request to start interacting with it

This PR also internally switches to the VerificationRequest::changes publisher, fixes a couple of invalid VerificationMachine tests and starts storing requesting DeviceData directly in VerificationRequestState. Requested

Relates to element-hq/element-meta/issues/2464, fixes #3595.

pub async fn is_verified(&self) -> Result<bool, ClientError> {
let device =
self.encryption.get_own_device().await?.context("Our own device is missing")?;
pub fn set_delegate(&self, delegate: Option<Box<dyn SessionVerificationControllerDelegate>>) {
Copy link
Member Author

@stefanceriu stefanceriu Oct 18, 2024

Choose a reason for hiding this comment

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

I considered switching away from a delegate and towards a listener but the public api changes are so small that I don't really think it's worth it at this point. Let's reassess when we start thinking about user verification.

@stefanceriu
Copy link
Member Author

Please double check the commit messages, I tried making them fit the new contributing guidelines but not sure I nailed it.

Copy link

codecov bot commented Oct 21, 2024

Codecov Report

Attention: Patch coverage is 60.00000% with 20 lines in your changes missing coverage. Please review.

Project coverage is 84.89%. Comparing base (ca1d829) to head (61b6518).
Report is 23 commits behind head on main.

Files with missing lines Patch % Lines
...tes/matrix-sdk-crypto/src/verification/requests.rs 64.86% 13 Missing ⚠️
...ates/matrix-sdk-crypto/src/verification/machine.rs 36.36% 7 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4153      +/-   ##
==========================================
+ Coverage   84.85%   84.89%   +0.03%     
==========================================
  Files         269      269              
  Lines       28916    28902      -14     
==========================================
- Hits        24537    24536       -1     
+ Misses       4379     4366      -13     

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

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.

Most of this look great, thanks for handling this. It is a bit unfinished though.

@stefanceriu stefanceriu changed the title Session verification request support Session verification incoming request support Oct 24, 2024
@stefanceriu stefanceriu force-pushed the stefan/sessionVerificationRequestSupport branch from 24e2bb2 to 142d420 Compare October 24, 2024 10:07
@richvdh
Copy link
Member

richvdh commented Oct 25, 2024

Taking myself off the reviewer list here because it looks like @poljar already has context on it and will do a better job than me anyway

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.

Very nice. Thanks for taking care of this.

@poljar
Copy link
Contributor

poljar commented Oct 28, 2024

Oh about the commits, some of them you got right some of them not.

fix(crypto): ... is correct, crypto(verification) is not. In the second case you should have used fix(verification) or something along those lines.

The format is documented here: https://www.conventionalcommits.org/en/v1.0.0/.

…ation requests

fixup! feat(ffi): add support for receiving and working with session verification requests
…lly accepting one

- allows handling remote cancellations on verification requests that have not yet been accepted
@stefanceriu stefanceriu force-pushed the stefan/sessionVerificationRequestSupport branch from 40d2c20 to 61b6518 Compare October 28, 2024 13:54
@stefanceriu stefanceriu merged commit df4a5c3 into main Oct 28, 2024
40 checks passed
@stefanceriu stefanceriu deleted the stefan/sessionVerificationRequestSupport branch October 28, 2024 15:04
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.

Receiving a verification request does not work
4 participants