ElementR: fix emoji verification stalling when both ends hit start at the same time #4004
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
BillCarsonFr
approved these changes
Jan 24, 2024
richvdh
requested changes
Jan 25, 2024
richvdh
reviewed
Jan 26, 2024
richvdh
reviewed
Jan 26, 2024
richvdh
reviewed
Jan 26, 2024
richvdh
approved these changes
Jan 26, 2024
Co-authored-by: Richard van der Hoff <[email protected]>
|
This is ready to merge, but I'm taking this afternoon off, and I don't want to merge this and then run off right away, so I'll merge on Monday |
richvdh
approved these changes
Jan 27, 2024
| } | ||
| const crossSigningKeys = JSON.parse(uploadSigningKeysRequest.body); | ||
| // note: the upload signatures request and upload signing keys requests | ||
| // don't need to me marked as sent in the Olm machine |
There was a problem hiding this comment.
Suggested change
| // don't need to me marked as sent in the Olm machine | |
| // don't need to be marked as sent in the Olm machine |
| '{"one_time_key_counts":{"signed_curve25519":100}}', | ||
| ); | ||
| const crossSigningSignatures = JSON.parse(uploadSignaturesRequest.body); | ||
| if (crossSigningSignatures[userId] && crossSigningSignatures[userId][deviceId]) { |
There was a problem hiding this comment.
it looks like this should always be true? I'd be tempted to get rid of this condition.
Alternatively, a ? operator would simplify:
Suggested change
| if (crossSigningSignatures[userId] && crossSigningSignatures[userId][deviceId]) { | |
| if (crossSigningSignatures[userId]?.[deviceId]) { |
Doesn't matter though.
Comment on lines
465
to
467
| customHandler: | ||
| | ((request: OutgoingRequest | RustSdkCryptoJs.UploadSigningKeysRequest) => Promise<any>) | ||
| | undefined = undefined, |
There was a problem hiding this comment.
Consider defining something like:
type CustomRequestHandler = ((request: OutgoingRequest | RustSdkCryptoJs.UploadSigningKeysRequest) => Promise<any>);Also, ? after a param is effectively the same as | undefined, and = undefined is the default.
Suggested change
| customHandler: | |
| | ((request: OutgoingRequest | RustSdkCryptoJs.UploadSigningKeysRequest) => Promise<any>) | |
| | undefined = undefined, | |
| customHandler?: CustomRequestHandler, |
| async function makeOutgoingRequest( | ||
| request: OutgoingRequest | RustSdkCryptoJs.UploadSigningKeysRequest, | ||
| ): Promise<any> { | ||
| let resp: any = customHandler && (await customHandler(request)); |
There was a problem hiding this comment.
Suggested change
| let resp: any = customHandler && (await customHandler(request)); | |
| let resp: any = await customHandler?.(request); |
I think?
Comment on lines
536
to
543
| const promise = (async () => { | ||
| while (!stopRequestLoop) { | ||
| const requests = await ourOlmMachine.outgoingRequests(); | ||
| for (const request of requests) { | ||
| await makeOutgoingRequest(request); | ||
| } | ||
| } | ||
| })(); |
There was a problem hiding this comment.
this is a bit hard to read.
Suggested change
| const promise = (async () => { | |
| while (!stopRequestLoop) { | |
| const requests = await ourOlmMachine.outgoingRequests(); | |
| for (const request of requests) { | |
| await makeOutgoingRequest(request); | |
| } | |
| } | |
| })(); | |
| async function runLoop() { | |
| while (!stopRequestLoop) { | |
| const requests = await ourOlmMachine.outgoingRequests(); | |
| for (const request of requests) { | |
| await makeOutgoingRequest(request); | |
| } | |
| } | |
| } | |
| const loopCompletedPromise = runLoop(); |
| request: OutgoingRequest | RustSdkCryptoJs.UploadSigningKeysRequest, | ||
| ): Promise<any> { | ||
| let resp: any = customHandler && (await customHandler(request)); | ||
| if (resp == undefined) { |
There was a problem hiding this comment.
Suggested change
| if (resp == undefined) { | |
| if (resp === undefined) { |
There was a problem hiding this comment.
It would be quite nice to stick the whole body of this if condition into a separate handleRequest(ourOlmMachine, theirOlmMachine) function.
| resp = { failures: {} }; | ||
| } | ||
| } | ||
| if (!(request instanceof RustSdkCryptoJs.UploadSigningKeysRequest) && request.id) { |
There was a problem hiding this comment.
I think you can spell this more succinctly as
Suggested change
| if (!(request instanceof RustSdkCryptoJs.UploadSigningKeysRequest) && request.id) { | |
| if (request.id) { |
but maybe that upsets typescript?
There was a problem hiding this comment.
Indeed, TypeScript gets upset about that
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
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.
fixes element-hq/element-web#26723
When both ends hit "start", one of the sides will "win" and the other will "lose". The Rust crypto crate on the losing side replaces the SAS verifier with a new one. The current code does not detect this, and continues trying to use the old one. This PR switches the verifier to the new one when it changes.
Checklist
Here's what your changelog entry will look like:
🐛 Bug Fixes