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

Element-R: await /keys/query during Verification requests #3932

Merged
merged 6 commits into from
Dec 5, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 19 additions & 2 deletions spec/unit/rust-crypto/verification.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,9 +62,13 @@ describe("VerificationRequest", () => {

describe("startVerification", () => {
let request: RustVerificationRequest;
let machine: Mocked<RustSdkCryptoJs.OlmMachine>;
let inner: Mocked<RustSdkCryptoJs.VerificationRequest>;

beforeEach(() => {
request = makeTestRequest();
inner = makeMockedInner();
machine = { getDevice: jest.fn() } as unknown as Mocked<RustSdkCryptoJs.OlmMachine>;
request = makeTestRequest(inner, machine);
});

it("does not permit methods other than SAS", async () => {
Expand All @@ -73,7 +77,15 @@ describe("VerificationRequest", () => {
);
});

it("raises an error if the other device is unknown", async () => {
await expect(request.startVerification("m.sas.v1")).rejects.toThrow(
"startVerification(): other device is unknown",
);
});

it("raises an error if starting verification does not produce a verifier", async () => {
jest.spyOn(inner, "otherDeviceId", "get").mockReturnValue(new RustSdkCryptoJs.DeviceId("other_device"));
machine.getDevice.mockResolvedValue({} as RustSdkCryptoJs.Device);
await expect(request.startVerification("m.sas.v1")).rejects.toThrow(
"Still no verifier after startSas() call",
);
Expand Down Expand Up @@ -118,11 +130,13 @@ describe("isVerificationEvent", () => {
/** build a RustVerificationRequest with default parameters */
function makeTestRequest(
inner?: RustSdkCryptoJs.VerificationRequest,
olmMachine?: RustSdkCryptoJs.OlmMachine,
outgoingRequestProcessor?: OutgoingRequestProcessor,
): RustVerificationRequest {
inner ??= makeMockedInner();
olmMachine ??= {} as RustSdkCryptoJs.OlmMachine;
outgoingRequestProcessor ??= {} as OutgoingRequestProcessor;
return new RustVerificationRequest(inner, outgoingRequestProcessor, []);
return new RustVerificationRequest(olmMachine, inner, outgoingRequestProcessor, []);
}

/** Mock up a rust-side VerificationRequest */
Expand All @@ -133,5 +147,8 @@ function makeMockedInner(): Mocked<RustSdkCryptoJs.VerificationRequest> {
phase: jest.fn().mockReturnValue(RustSdkCryptoJs.VerificationRequestPhase.Created),
isPassive: jest.fn().mockReturnValue(false),
timeRemainingMillis: jest.fn(),
get otherDeviceId() {
return undefined;
},
} as unknown as Mocked<RustSdkCryptoJs.VerificationRequest>;
}
13 changes: 12 additions & 1 deletion src/rust-crypto/rust-crypto.ts
Original file line number Diff line number Diff line change
Expand Up @@ -940,6 +940,7 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
.map(
(request) =>
new RustVerificationRequest(
this.olmMachine,
request,
this.outgoingRequestProcessor,
this._supportedVerificationMethods,
Expand Down Expand Up @@ -970,6 +971,7 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv

if (request) {
return new RustVerificationRequest(
this.olmMachine,
request,
this.outgoingRequestProcessor,
this._supportedVerificationMethods,
Expand Down Expand Up @@ -1005,6 +1007,7 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
methods,
);
return new RustVerificationRequest(
this.olmMachine,
request,
this.outgoingRequestProcessor,
this._supportedVerificationMethods,
Expand Down Expand Up @@ -1080,6 +1083,7 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
);
await this.outgoingRequestProcessor.makeOutgoingRequest(outgoingRequest);
return new RustVerificationRequest(
this.olmMachine,
request,
this.outgoingRequestProcessor,
this._supportedVerificationMethods,
Expand Down Expand Up @@ -1118,6 +1122,7 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
);
await this.outgoingRequestProcessor.makeOutgoingRequest(outgoingRequest);
return new RustVerificationRequest(
this.olmMachine,
request,
this.outgoingRequestProcessor,
this._supportedVerificationMethods,
Expand Down Expand Up @@ -1405,7 +1410,12 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
if (request) {
this.emit(
CryptoEvent.VerificationRequestReceived,
new RustVerificationRequest(request, this.outgoingRequestProcessor, this._supportedVerificationMethods),
new RustVerificationRequest(
this.olmMachine,
request,
this.outgoingRequestProcessor,
this._supportedVerificationMethods,
),
);
}
}
Expand Down Expand Up @@ -1622,6 +1632,7 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
this.emit(
CryptoEvent.VerificationRequestReceived,
new RustVerificationRequest(
this.olmMachine,
request,
this.outgoingRequestProcessor,
this._supportedVerificationMethods,
Expand Down
26 changes: 23 additions & 3 deletions src/rust-crypto/verification.ts
Original file line number Diff line number Diff line change
Expand Up @@ -57,11 +57,13 @@ export class RustVerificationRequest
/**
* Construct a new RustVerificationRequest to wrap the rust-level `VerificationRequest`.
*
* @param inner - VerificationRequest from the Rust SDK
* @param outgoingRequestProcessor - `OutgoingRequestProcessor` to use for making outgoing HTTP requests
* @param supportedVerificationMethods - Verification methods to use when `accept()` is called
* @param olmMachine - The `OlmMachine` from the underlying rust crypto sdk.
* @param inner - VerificationRequest from the Rust SDK.
* @param outgoingRequestProcessor - `OutgoingRequestProcessor` to use for making outgoing HTTP requests.
* @param supportedVerificationMethods - Verification methods to use when `accept()` is called.
*/
public constructor(
private readonly olmMachine: RustSdkCryptoJs.OlmMachine,
private readonly inner: RustSdkCryptoJs.VerificationRequest,
private readonly outgoingRequestProcessor: OutgoingRequestProcessor,
private readonly supportedVerificationMethods: string[],
Expand Down Expand Up @@ -135,6 +137,15 @@ export class RustVerificationRequest
return this.inner.otherDeviceId?.toString();
}

/** Get the other device involved in the verification, if it is known */
private async getOtherDevice(): Promise<undefined | RustSdkCryptoJs.Device> {
const otherDeviceId = this.inner.otherDeviceId;
if (!otherDeviceId) {
return undefined;
}
return await this.olmMachine.getDevice(this.inner.otherUserId, otherDeviceId, 5);
}

/** True if the other party in this request is one of this user's own devices. */
public get isSelfVerification(): boolean {
return this.inner.isSelfVerification();
Expand Down Expand Up @@ -322,6 +333,10 @@ export class RustVerificationRequest
throw new Error(`Unsupported verification method ${method}`);
}

Copy link
Member

Choose a reason for hiding this comment

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

Is this the critical part that causes us to avoid the race? If so, can we have a comment here explaining that?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, sorry. Have added a comment.

if (!(await this.getOtherDevice())) {
richvdh marked this conversation as resolved.
Show resolved Hide resolved
throw new Error("startVerification(): other device is unknown");
}

const res:
| [RustSdkCryptoJs.Sas, RustSdkCryptoJs.RoomMessageRequest | RustSdkCryptoJs.ToDeviceRequest]
| undefined = await this.inner.startSas();
Expand Down Expand Up @@ -392,6 +407,11 @@ export class RustVerificationRequest
* Implementation of {@link Crypto.VerificationRequest#generateQRCode}.
*/
public async generateQRCode(): Promise<Buffer | undefined> {
// make sure that we have a list of the other user's devices (workaround https://github.com/matrix-org/matrix-rust-sdk/issues/2896)
if (!(await this.getOtherDevice())) {
throw new Error("generateQRCode(): other device is unknown");
}

const innerVerifier: RustSdkCryptoJs.Qr | undefined = await this.inner.generateQrCode();
// If we are unable to generate a QRCode, we return undefined
if (!innerVerifier) return;
Expand Down
Loading