Support for .ready verification event (MSC2366) & other things #1140
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
as MSC2366 adds an extra interactive step to the verification process, we can't wait for the verifier after sending the request. This is a breaking change in the js-sdk as it changes the return type of an existing method.
so we don't need to import the PHASE_ constants where we need to check
as it's harder to determine the other side of a request, given the in-room code also processes remote echos for own events.
also put logic to block non-participating senders in VerificationRequest so it is shared between both channels. Remote echo's should not be passed to the verifier though.
as once in done, the request is removed from the request map and the second .done event that comes in will not find the request anymore, so the request wouldn't be attached to the event anymore, breaking rendering it in the timeline.
next up is inspecting the .request event to determine it reliably in InRoomChannel
also fail validation with any event not sent by or directed to us
as it doesn't need to happen for ToDeviceChannel
but doesn't work yet? data where liveEvent is fished out is undefined
this makes the verifier want to interact with the other party when just reloading the session.
rather than the sender and from_device (which is not always set) as this was one of the things breaking to_device verification of ones own devices.
this was one of the things breaking to_device verification
3 tasks
|
Meant this to be a draft PR, still need to improve comments. |
turt2live
approved these changes
Jan 16, 2020
| @@ -894,8 +894,8 @@ MatrixClient.prototype.acceptVerificationDM = function(event, method) { | |||
| * @param {Array} devices array of device IDs to send requests to. Defaults to | |||
| * all devices owned by the user | |||
| * | |||
| * @returns {Promise<module:crypto/verification/Base>} resolves to a verifier | |||
| * when the request is accepted by the other user | |||
| * @returns {Promise<module:crypto/verification/request/VerificationRequest>} resolves to a VerificationRequest | |||
There was a problem hiding this comment.
Usually we don't change the contract of our public functions, though in this case we're about to do a major version bump for the new build system anyways so this seems fine and convenient.
There was a problem hiding this comment.
Indeed, see the breaking change comment in the description.
|
I've merged in develop after I completely refactored the entire build system. |
Merged
but instead use Promise.resolved() as then always runs in the next tick.
although it doesn't make or break the test
as InRoomChannel looks at this to decide whether an event is a remote echo (and to pass it to the verifier or not)
which now returns a Promise of VerificationRequest instead of verifier
a request should be accepted by calling accept() on the request.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
MSC: https://github.com/uhoreg/matrix-doc/blob/key_verification_accept/proposals/2366-key-verification-accept.md
Part of element-hq/element-web#11224
Required by matrix-org/matrix-react-sdk#3796
So #1109 formalized the request object that was already used in
Crypto/index. This PR further puts all of the responsibilities ofKeyVerificationStateObserver(react-sdk) intoVerificationRequest(js-sdk) so thereact-sdkcan use the latter directly to drive the UI in matrix-org/matrix-react-sdk#3796. This makes it easier to keep all the parts that need to response to the state of a request in sync (toast, event tiles and right panel).Most of the work in this PR went into displaying historical request (e.g. loaded from cache, or when back-paginating) correctly.
In order to make this work, I had to rewrite the state machine logic in
VerificationRequest, and ended up storing all events by sender and type to not rely on order, because when loading events from cache the events are be emitted in reverse chronological order (e.g. receiving the .done event first and .request event last). To prevent the verifier or the verification request sending out any events (like .cancel) while replaying the events of a historical request, anobserveOnlyflag was added. This flag is turned off when receiving a non-live event (changes had to be made to determine this more reliably, e.g. when loading from cache), or when an event is too old, or when we detect the request is between two other parties (e.g. the user in the DM and another one of my own devices).In-room requests are now also stored per roomId, not userId, as determining the other party of the request can be hard when the .request is not the first event received (when loading from cache). We need to take determine the other part for that event and ignore any other senders after that. For this reason the request lookup was abstracted into
InRoomRequestsandToDeviceRequests.The state machine now also relies on remote echos being passed to
handleEvent(which are faked for to_device messages) as this handles replaying historical events, sending, and handling live events in one code path. Not relying on local echo for verification actions anymore also mean I could deleteRequestCallbackChannel, which was used to inform theVerificationRequestthat the verifier was sending something and to do local echo of that event.As said, this PR also adds support for the .ready event as specified in MSC2366. This turns accepting a request into a separate step than choosing a verification method. To support this,
VerificationRequestgained anacceptmethod andwaitForVerifierhad to be changed as getting a verifier (when receiving a.start) now requires callingaccept()first and not just waiting.VerificationRequestnow also handles cancelling with a timeout before the request is accepted (verifier has its own timeout after accepting a request).Breaking change
2da7253 changes the return type of
client.acceptVerificationandclient.acceptVerificationDMto return a promise ofVerificationRequestobject instead of a promise of the verifier. Alsoclient.acceptVerificationDMwas deleted, requests should now be accepted throughVerificationRequest.accept. See commit message why this was needed. This being a breaking change, it might warrant bumping the major version number.Cleanups still to do:
The commit log is a bit of a mess because of having to rewrite the state machine and at the same time adding .ready support. I had planned to restructure the commits but ran out of time, apologies to the reviewer for this.