-
Notifications
You must be signed in to change notification settings - Fork 275
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
feat(ffi): add support for starting and responding to user verification requests #4618
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4618 +/- ##
=======================================
Coverage 85.73% 85.73%
=======================================
Files 291 291
Lines 33323 33325 +2
=======================================
+ Hits 28570 28572 +2
Misses 4753 4753 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I left a couple of nits and it's quite sad that we can't, instead of adding event handlers here, have a higher level stream in the SDK which gives you the Verification
object once one gets created.
Maybe one of these days when the OlmMachine
doesn't get permanently killed and resurrected. 🥲
@@ -252,13 +252,13 @@ impl Room { | |||
} | |||
|
|||
pub async fn member(&self, user_id: String) -> Result<RoomMember, ClientError> { | |||
let user_id = UserId::parse(&*user_id).context("Invalid user id.")?; | |||
let user_id = UserId::parse(&*user_id)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a point in removing these? Perhaps it would make sense to have a helper that always applies this context to the UserId
parsing step instead of just removing this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just looked at where UserId::parse
was used and noticed there's significantly more without a context than with. Figured I might as well make it consistent.
I'm not sure it's worth wrapping it, I don't think I've ever seen one of these in practice and even if it were to happen the underlying ruma error is clear enough.
fd39193
to
6ce266a
Compare
This patch expands on the existing device verification flows to add support for verifying users as well.
It does so by:
process_incoming_verification_request
methodrequest_user_verification
method next to the (renamed)request_device_verification
one on theSessionVerificationController
set_ongoing_verification_request