Invalidate device lists when encryption is enabled in a room #359
Conversation
we only really hit this in the tests, but it's a bit silly to download the complete device list on every message.
89ced19 added some code which flagged our own device list as in need of an update. However, 8d50274 then added code such that we invalidate *all* members of e2e rooms on the first initialsync - which should include ourselves. We can therefore remove the redundant special-case, which mostly serves to simplify the tests.
| @@ -64,7 +64,7 @@ OlmEncryption.prototype._ensureSession = function(roomMembers) { | |||
| } | |||
|
|
|||
| const self = this; | |||
| this._prepPromise = self._crypto.downloadKeys(roomMembers, true).then(function(res) { | |||
| this._prepPromise = self._crypto.downloadKeys(roomMembers).then(function(res) { | |||
There was a problem hiding this comment.
is this for the old m.room.encrypted m.olm.v1.curve25519-aes-sha2 messages?
There was a problem hiding this comment.
yup. that's why it hasn't been a problem in practice.
| @@ -351,7 +351,6 @@ describe("MatrixClient", function() { | |||
|
|
|||
| httpBackend.when("POST", "/keys/query").check(function(req) { | |||
| expect(req.data).toEqual({device_keys: { | |||
| '@alice:localhost': {}, | |||
There was a problem hiding this comment.
i'm a little surprised that we no longer see our own key here...
There was a problem hiding this comment.
or rather, our own userId.
so... the reason for this is, as per the comment on bd07310, we no longer automatically queue up our own userId as needing a refresh - instead it happens once the initialsync happens. In this test, there is no initialsync, so our own userid isn't invalidated.
| type: 'm.room.encryption', | ||
| skey: '', | ||
| content: { | ||
| algorithm: 'm.olm.v1.curve25519-aes-sha2', |
There was a problem hiding this comment.
should I be surprised that the alg here is olm and not megolm, given it's for a room?
There was a problem hiding this comment.
Per
, this test file doesn't really do megolm - most of the tests here pre-existed megolm. The actual algorithm doesn't make any difference for this test.
|
sounds plausible to me, modulo a few questions of confusion |
|
lgtm |
... and other stories.
Most of this was cleanup work to get to a state where this change didn't break all the tests inexplicably. The commits might make some sense on their own - or I can pull it into separate PRs if you like.