Use the last olm session that got a message #776
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Tests now failing because of lack of new olm - looks like there are still issues on node 11 |
dbkr
added a commit
that referenced
this pull request
Nov 8, 2018
* Start a new Olm sessions with a device when we get an undecryptable message on it. * Send a dummy message on that sessions such that the other end knows about it. * Re-send any outstanding keyshare requests for that device. Also includes a unit test for megolm that isn't very related but came out as a result anyway. Includes #776 Fixes element-hq/element-web#3822
uhoreg
approved these changes
Nov 9, 2018
src/crypto/OlmDevice.js
Outdated
| return a.sessionId < b.sessionId ? -1 : 1; | ||
| } | ||
| }); | ||
| return sessionInfos[sessionInfos.length - 1].sessionId; |
There was a problem hiding this comment.
looping through the sessionInfos array, and keeping track of the sessionId with the smallest timestamp/sessionId is approximately the same number of lines of code, and faster (O(n) rather than O(n log n)). i.e.
let idxOfMin = 0;
for (i = 1; i < sessionInfos.length; i++) {
if (sessionInfos[i].lastReceivedMessageTs < sessionInfos[idxOfMin].lastReceiveMessageTs
|| (sessionInfos[i].lastReceivedMessageTs === sessionInfos[idxOfMin].lastReceiveMessageTs
&& sessionInfos[i].sessionId < sessinInfos[idxOfMin].sessionId)) {
idxOfMin = i;
}
return sessionInfos[idxOfMin].sessionId;There was a problem hiding this comment.
yeah, I was reluctant to bother changing it since it was already a sort, but fair enough
src/crypto/OlmDevice.js
Outdated
| @@ -509,6 +509,8 @@ OlmDevice.prototype.createInboundSession = async function( | |||
| this._storeAccount(txn, account); | |||
|
|
|||
| const payloadString = session.decrypt(messageType, ciphertext); | |||
| // this counts as an received message | |||
uhoreg
approved these changes
Nov 12, 2018
| results[cursor.value.sessionId] = cursor.value.session; | ||
| results[cursor.value.sessionId] = { | ||
| session: cursor.value.session, | ||
| lastReceivedMessagets: cursor.value.lastReceivedMessageTs, |
There was a problem hiding this comment.
Why is the case changed here (lastReceivedMessagets vs lastReceivedmessageTs)?
| func(getReq.result.session); | ||
| func({ | ||
| session: getReq.result.session, | ||
| lastReceivedMessagets: getReq.result.lastReceivedMessageTs, |
uhoreg
reviewed
Nov 12, 2018
src/crypto/OlmDevice.js
Outdated
| // Pretend we've received a message at this point, otherwise | ||
| // if we try to send a message to the device, it won't use | ||
| // this session (storing the creation time separately would | ||
| // make the pickle longer and would not be useful otherwise). |
There was a problem hiding this comment.
not stored in the pickle any more
src/crypto/OlmDevice.js
Outdated
| this._saveSession(theirDeviceIdentityKey, session, txn); | ||
| const sessionInfo = { | ||
| session, | ||
| // this counts as an received message: set last received message time |
dbkr
commented
Nov 13, 2018
src/crypto/OlmDevice.js
Outdated
| // Pretend we've received a message at this point, otherwise | ||
| // if we try to send a message to the device, it won't use | ||
| // this session (storing the creation time separately would | ||
| // make the pickle longer and would not be useful otherwise). |
src/crypto/OlmDevice.js
Outdated
| return a.sessionId < b.sessionId ? -1 : 1; | ||
| } | ||
| }); | ||
| return sessionInfos[sessionInfos.length - 1].sessionId; |
There was a problem hiding this comment.
yeah, I was reluctant to bother changing it since it was already a sort, but fair enough
| results[cursor.value.sessionId] = cursor.value.session; | ||
| results[cursor.value.sessionId] = { | ||
| session: cursor.value.session, | ||
| lastReceivedMessagets: cursor.value.lastReceivedMessageTs, |
uhoreg
approved these changes
Nov 14, 2018
This was referenced Dec 13, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Implements matrix-org/matrix-spec-proposals#1596
For element-hq/element-web#3822
Requires https://github.com/matrix-org/olm-backup/pull/77 (+release)