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

Make sure that MegolmEncryption.setupPromise always resolves #2960

Merged
merged 3 commits into from
Dec 9, 2022
Merged
Show file tree
Hide file tree
Changes from all 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
20 changes: 20 additions & 0 deletions spec/unit/crypto/algorithms/megolm.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -490,6 +490,26 @@ describe("MegolmDecryption", function () {

expect(mockBaseApis.queueToDevice).not.toHaveBeenCalled();
});

it("shouldn't wedge the setup promise if sharing a room key fails", async () => {
// @ts-ignore - private field access
const initialSetupPromise = await megolmEncryption.setupPromise;
expect(initialSetupPromise).toBe(null);

// @ts-ignore - private field access
megolmEncryption.prepareSession = () => {
throw new Error("Can't prepare session");
};

await expect(() =>
// @ts-ignore - private field access
megolmEncryption.ensureOutboundSession(mockRoom, {}, {}, true),
).rejects.toThrow();

// @ts-ignore - private field access
const finalSetupPromise = await megolmEncryption.setupPromise;
expect(finalSetupPromise).toBe(null);
});
});
});

Expand Down
33 changes: 26 additions & 7 deletions src/crypto/algorithms/megolm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,23 @@ export class MegolmEncryption extends EncryptionAlgorithm {
* @param singleOlmCreationPhase - Only perform one round of olm
* session creation
*
* This method updates the setupPromise field of the class by chaining a new
* call on top of the existing promise, and then catching and discarding any
* errors that might happen while setting up the outbound group session. This
* is done to ensure that `setupPromise` always resolves to `null` or the
* `OutboundSessionInfo`.
*
* Using `>>=` to represent the promise chaining operation, it does the
* following:
*
* ```
* setupPromise = previousSetupPromise >>= setup >>= discardErrors
* ```
*
* The initial value for the `setupPromise` is a promise that resolves to
* `null`. The forceDiscardSession() resets setupPromise to this initial
* promise.
*
* @returns Promise which resolves to the
* OutboundSessionInfo when setup is complete.
*/
Expand Down Expand Up @@ -278,18 +295,20 @@ export class MegolmEncryption extends EncryptionAlgorithm {
};

// first wait for the previous share to complete
const prom = this.setupPromise.then(setup);
const fallible = this.setupPromise.then(setup);

// Ensure any failures are logged for debugging
prom.catch((e) => {
// Ensure any failures are logged for debugging and make sure that the
// promise chain remains unbroken
//
// setupPromise resolves to `null` or the `OutboundSessionInfo` whether
// or not the share succeeds
this.setupPromise = fallible.catch((e) => {
logger.error(`Failed to setup outbound session in ${this.roomId}`, e);
return null;
});

// setupPromise resolves to `session` whether or not the share succeeds
this.setupPromise = prom;

// but we return a promise which only resolves if the share was successful.
return prom;
return fallible;
}

private async prepareSession(
Expand Down