Element-R: Don't mark QR code verification as done until it's done (#3791)

* Element-R: Don't mark QR code verification as done too soon

The rust crypto sdk doesn't actually finish QR code verification until the
`m.key.verification.done` is received, so make sure we don't tell the
application it is done before that happens.

Fixes https://github.com/vector-im/element-web/issues/26293

* ignore fallback line

* Revert unnecessary changes

Can't get the coverage high enough on this and it's not needed.

spec/integ/crypto/verification.spec.ts

@@ -419,6 +419,15 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st
         });
 
         it("can verify another via QR code with an untrusted cross-signing key", async () => {
+            // This is a slightly weird thing to test; if we don't trust the cross-signing key, normally we would
+            // spam out a verification request to all devices rather than targeting a single device. Still, it's
+            // a thing both the Matrix protocol and the js-sdk API support, so we may as well test it.
+            //
+            // Since we don't yet trust the master key, this is a type 0x02 QR code:
+            //   "self-verifying in which the current device does not yet trust the master key"
+            //
+            // By the end of it, we should trust the master key.
+
             aliceClient = await startTestClient();
             // QRCode fails if we don't yet have the cross-signing keys, so make sure we have them now.
             e2eKeyResponder.addCrossSigningData(SIGNED_CROSS_SIGNING_KEYS_DATA);
@@ -490,12 +499,28 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st
             reciprocateQRCodeCallbacks.confirm();
             await sendToDevicePromise;
 
+            // at this point, on legacy crypto, the master key is already marked as trusted, and the request is "Done".
+            // Rust crypto, on the other hand, waits for the 'done' to arrive from the other side.
+            if (request.phase === VerificationPhase.Done) {
+                // legacy crypto: we're all done
+                const userVerificationStatus = await aliceClient.getCrypto()!.getUserVerificationStatus(TEST_USER_ID);
+                // eslint-disable-next-line jest/no-conditional-expect
+                expect(userVerificationStatus.isCrossSigningVerified()).toBeTruthy();
+                await verificationPromise;
+            } else {
+                // rust crypto: still in flight
+                // eslint-disable-next-line jest/no-conditional-expect
+                expect(request.phase).toEqual(VerificationPhase.Started);
+            }
+
             // the dummy device replies with its own 'done'
             returnToDeviceMessageFromSync(buildDoneMessage(transactionId));
 
-            // ... and the whole thing should be done!
+            // ... and now we're really done.
             await verificationPromise;
             expect(request.phase).toEqual(VerificationPhase.Done);
+            const userVerificationStatus = await aliceClient.getCrypto()!.getUserVerificationStatus(TEST_USER_ID);
+            expect(userVerificationStatus.isCrossSigningVerified()).toBeTruthy();
         });
 
         it("can try to generate a QR code when QR code is not supported", async () => {

src/crypto-api/verification.ts

@@ -224,13 +224,25 @@ export enum VerificationPhase {
     /** An `m.key.verification.ready` event has been sent or received, indicating the verification request is accepted. */
     Ready,
 
-    /** An `m.key.verification.start` event has been sent or received, choosing a verification method */
+    /**
+     * The verification is in flight.
+     *
+     * This means that an `m.key.verification.start` event has been sent or received, choosing a verification method;
+     * however the verification has not yet completed or been cancelled.
+     */
     Started,
 
-    /** An `m.key.verification.cancel` event has been sent or received at any time before the `done` event, cancelling the verification request */
+    /**
+     * An `m.key.verification.cancel` event has been sent or received at any time before the `done` event, cancelling
+     * the verification request
+     */
     Cancelled,
 
-    /** An `m.key.verification.done` event has been **sent**, completing the verification request. */
+    /**
+     * The verification request is complete.
+     *
+     * Normally this means that `m.key.verification.done` events have been sent and received.
+     */
     Done,
 }
 

src/rust-crypto/verification.ts

@@ -569,7 +569,11 @@ export class RustQrCodeVerifier extends BaseRustVerifer<RustSdkCryptoJs.Qr> impl
                 return VerificationPhase.Started;
             case QrState.Confirmed:
                 // we have confirmed the other side's scan and sent an `m.key.verification.done`.
-                return VerificationPhase.Done;
+                //
+                // However, the verification is not yet "Done", because we have to wait until we have received the
+                // `m.key.verification.done` from the other side (in particular, we don't mark the device/identity as
+                // verified until that happens). If we return "Done" too soon, we risk the user cancelling the flow.
+                return VerificationPhase.Started;
             case QrState.Reciprocated:
                 // although the rust SDK doesn't immediately send the `m.key.verification.start` on transition into this
                 // state, `RustVerificationRequest.scanQrCode` immediately calls `reciprocate()` and does so, so in practice