Make lots of OlmDevice asynchronous #524

richvdh · 2017-08-10T14:03:53Z

A whole series of commits, each making a different method of OlmDevice asynchronous.

The tests in this code suffer from the races fixed in #523, so the tests are failing until that lands.

Prepare for some refactoring which will add an extra tick to decryption by adding some `awaitDecryption` calls in the integration tests.

Add an asynchronous `init` method to OlmDevice which initialises the OlmAccount.

* OlmDevice.getSessionIdsForDevice * OlmDevice.getSessionIdForDevice * OlmDevice.getSessionInfoForDevice

* OlmDevice.encryptMessage * OlmDevice.decryptMessage * OlmDevice.matchesSession

* OlmDevice.getOneTimeKeys * OlmDevice.markKeysAsPublished

* OlmDevice.generateOneTimeKeys becomes async * Stash maxOneTimeKeys at init so that maxNumberOfOneTimeKeys can remain sync

* OlmDevice.hasInboundSessionKeys * OlmDevice.getInboundGroupSessionKey The latter means that MegolmDecryption.shareKeysWithDevice takes longer before it sends out the keyshare, so means the unit test needed an update

kegsay · 2017-08-10T16:14:19Z

spec/integ/matrix-client-crypto.spec.js

+            console.log(client.credentials.userId + " received event",
+                        event);
+
+            client.removeListener("event", onEvent);


You could've also used client.once. https://nodejs.org/api/events.html#events_emitter_once_eventname_listener

No, because then it would be harder to ignore the m.room.member events

kegsay · 2017-08-10T16:17:32Z

spec/unit/crypto/algorithms/megolm.spec.js

@@ -144,7 +148,7 @@ describe("MegolmDecryption", function() {
                megolmDecryption.shareKeysWithDevice(keyRequest);

                // it's asynchronous, so we have to wait a bit
-                return awaitEnsureSessions;
+                return awaitEncryptForDevice;


Why are we waiting for something different now?

There are two steps here:
1: ensure we have olm sessions for each of the devices (this has always been asynchronous as it involves API calls)
2: encrypt the content for each device (which used to be synchronous, but is now async)

In short, we now need to wait for step 2 to happen, whereas previously waiting for step 1 was sufficient

kegsay · 2017-08-10T16:20:02Z

src/crypto/OlmDevice.js

+        account.free();
+    }
+
+    this.deviceCurve25519Key = e2eKeys.curve25519;


This will throw if _initialise_account throws or if there are no identity keys as e2eKeys will be undefined.

if _initialise_account throws, we'll never get here.

OlmAccount.identity_keys() is specced to return a JSON-encoded object, so e2eKeys can't be undefined here.

kegsay · 2017-08-10T16:20:36Z

src/crypto/OlmDevice.js

@@ -367,10 +384,10 @@ OlmDevice.prototype.getSessionIdsForDevice = function(theirDeviceIdentityKey) {
 *
 * @param {string} theirDeviceIdentityKey Curve25519 identity key for the
 *     remote device
- * @return {string?}  session id, or null if no established session
+ * @return {Promise<string?>}  session id, or null if no established session


done e5565c6

kegsay · 2017-08-10T16:23:20Z

src/crypto/algorithms/megolm.js

 };

 /**
 * @inheritdoc
 */
-MegolmDecryption.prototype.hasKeysForKeyRequest = async function(keyRequest) {
+MegolmDecryption.prototype.hasKeysForKeyRequest = function(keyRequest) {


I'm struggling to see any rhyme or reason for which functions need to be async and which don't? How are you determining this?

They all need to be asynchronous. (The actual logic was that I wanted anything that touched sessionStore to be async, because I wanted to back the sessionStore with indexeddb, so sessionStore had to get an async interface).

Of course they can be asynchronous without being async. In this case: hasInboundSessionKeys is being changed to return a Promise, so the async here is redundant. I agree it could have stayed.

kegsay · 2017-08-10T16:25:32Z

src/crypto/index.js

+utils.inherits(Crypto, EventEmitter);
+
+/**
+ * Initialise the crypto module so that it is ready for use


You should mention that it returns a promise and that it is only initialised once said promise resolves.

done e5565c6

I couldn't repro the failure locally, but this looks like it should fix the test failures.

Ok, this *really* ought to fix the racy test.

This failed a test, so let's just bump up the timeout a bit more.

richvdh · 2017-08-17T12:16:54Z

I've had enough of this stuff. I'm taking the executive decision to merge it, in kegan's absence.

richvdh added 13 commits August 10, 2017 15:01

Add delays to tests to wait for things to decrypt

c870930

Prepare for some refactoring which will add an extra tick to decryption by adding some `awaitDecryption` calls in the integration tests.

Make OlmDevice initialisation asynchronous

f2f8ad6

Add an asynchronous `init` method to OlmDevice which initialises the OlmAccount.

make session tracking methods in OlmDevice async

8f527a6

* OlmDevice.getSessionIdsForDevice * OlmDevice.getSessionIdForDevice * OlmDevice.getSessionInfoForDevice

Make OlmDevice olmSession methods asynchronous

e943a6e

* OlmDevice.encryptMessage * OlmDevice.decryptMessage * OlmDevice.matchesSession

Make OlmDevice.sign async

5b93d52

OlmDevice.oneTimeKeys async

a5f397b

* OlmDevice.getOneTimeKeys * OlmDevice.markKeysAsPublished

Make OlmDevice key generation async

7d2bc12

* OlmDevice.generateOneTimeKeys becomes async * Stash maxOneTimeKeys at init so that maxNumberOfOneTimeKeys can remain sync

Olm session creation async

e52985e

Make OlmDevice.addInboundGroupSession async

2894e25

Make OlmDevice.importInboundGroupSession async

cfd6109

Make OlmDevice.decryptGroupMessage async

337c9cb

Make some OlmDevice megolm methods async

8a0f73b

* OlmDevice.hasInboundSessionKeys * OlmDevice.getInboundGroupSessionKey The latter means that MegolmDecryption.shareKeysWithDevice takes longer before it sends out the keyshare, so means the unit test needed an update

Make OlmDevice.exportInboundGroupSession async

110bd33

richvdh assigned kegsay Aug 10, 2017

kegsay reviewed Aug 10, 2017

View reviewed changes

richvdh added 5 commits August 15, 2017 18:31

Merge remote-tracking branch 'origin/develop' into rav/async_crypto/1

f00558d

review comments

e5565c6

Hopefully, fix racy megolm test

1221240

I couldn't repro the failure locally, but this looks like it should fix the test failures.

Try harder to wait for megolm decryption

9f2fce4

Ok, this *really* ought to fix the racy test.

Crypto test: Bump the timeout when waiting for Ali to claim keys

ca91fba

This failed a test, so let's just bump up the timeout a bit more.

richvdh merged commit 2d82a7b into develop Aug 17, 2017

richvdh deleted the rav/async_crypto/1 branch August 17, 2017 12:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make lots of OlmDevice asynchronous #524

Make lots of OlmDevice asynchronous #524

richvdh commented Aug 10, 2017 •

edited

Loading

kegsay Aug 10, 2017

richvdh Aug 15, 2017

kegsay Aug 10, 2017

richvdh Aug 15, 2017

kegsay Aug 10, 2017

richvdh Aug 15, 2017

kegsay Aug 10, 2017

richvdh Aug 15, 2017

kegsay Aug 10, 2017

richvdh Aug 15, 2017

kegsay Aug 10, 2017

richvdh Aug 15, 2017

richvdh commented Aug 17, 2017

Make lots of OlmDevice asynchronous #524

Make lots of OlmDevice asynchronous #524

Conversation

richvdh commented Aug 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

richvdh commented Aug 17, 2017

richvdh commented Aug 10, 2017 •

edited

Loading