Skip to content

Use the last olm session that got a message #776

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.

Sign up for GitHub

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

Merged
merged 9 commits into from
Nov 15, 2018
Merged

Use the last olm session that got a message #776

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
28540ad
Use the last olm session that got a message
dbkr Nov 1, 2018
37f0a9a
Try tests on node 10
dbkr Nov 1, 2018
2a6a67c
Inbound session creation counts as a received message
dbkr Nov 1, 2018
fcadf6e
Store last received message ts on olm session
dbkr Nov 12, 2018
e17a39d
PR feedback
dbkr Nov 13, 2018
a30845f
lint
dbkr Nov 14, 2018
5bc68c0
Handle last received message ts being undefined
dbkr Nov 14, 2018
408407b
Fix typo
dbkr Nov 14, 2018
3c85bd5
Time goes forwards
dbkr Nov 14, 2018
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
2 changes: 1 addition & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
language: node_js
node_js:
- node # Latest stable version of nodejs.
- "10.11.0"
script:
- ./travis.sh
82 changes: 55 additions & 27 deletions src/crypto/OlmDevice.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -295,8 +295,8 @@ OlmDevice.prototype._storeAccount = function(txn, account) {
*/
OlmDevice.prototype._getSession = function(deviceKey, sessionId, txn, func) {
this._cryptoStore.getEndToEndSession(
deviceKey, sessionId, txn, (pickledSession) => {
this._unpickleSession(pickledSession, func);
deviceKey, sessionId, txn, (sessionInfo) => {
this._unpickleSession(sessionInfo, func);
},
);
};
Expand All @@ -306,15 +306,17 @@ OlmDevice.prototype._getSession = function(deviceKey, sessionId, txn, func) {
* function with it. The session object is destroyed once the function
* returns.
*
* @param {string} pickledSession
* @param {object} sessionInfo
* @param {function} func
* @private
*/
OlmDevice.prototype._unpickleSession = function(pickledSession, func) {
OlmDevice.prototype._unpickleSession = function(sessionInfo, func) {
const session = new global.Olm.Session();
try {
session.unpickle(this._pickleKey, pickledSession);
func(session);
session.unpickle(this._pickleKey, sessionInfo.session);
const unpickledSessInfo = Object.assign({}, sessionInfo, {session});

func(unpickledSessInfo);
} finally {
session.free();
}
Expand All @@ -324,14 +326,17 @@ OlmDevice.prototype._unpickleSession = function(pickledSession, func) {
* store our OlmSession in the session store
*
* @param {string} deviceKey
* @param {OlmSession} session
* @param {object} sessionInfo {session: OlmSession, lastReceivedMessageTs: int}
* @param {*} txn Opaque transaction object from cryptoStore.doTxn()
* @private
*/
OlmDevice.prototype._saveSession = function(deviceKey, session, txn) {
const pickledSession = session.pickle(this._pickleKey);
OlmDevice.prototype._saveSession = function(deviceKey, sessionInfo, txn) {
const sessionId = sessionInfo.session.session_id();
const pickledSessionInfo = Object.assign(sessionInfo, {
session: sessionInfo.session.pickle(this._pickleKey),
});
this._cryptoStore.storeEndToEndSession(
deviceKey, session.session_id(), pickledSession, txn,
deviceKey, sessionId, pickledSessionInfo, txn,
);
};

Expand Down Expand Up @@ -461,7 +466,15 @@ OlmDevice.prototype.createOutboundSession = async function(
session.create_outbound(account, theirIdentityKey, theirOneTimeKey);
newSessionId = session.session_id();
this._storeAccount(txn, account);
this._saveSession(theirIdentityKey, session, txn);
const sessionInfo = {
session,
// 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).
Copy link
Member

Choose a reason for hiding this comment

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

not stored in the pickle any more

Copy link
Member Author

Choose a reason for hiding this comment

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

ooh, yep

lastReceivedMessageTs: Date.now(),
};
this._saveSession(theirIdentityKey, sessionInfo, txn);
} finally {
session.free();
}
Expand Down Expand Up @@ -510,7 +523,13 @@ OlmDevice.prototype.createInboundSession = async function(

const payloadString = session.decrypt(messageType, ciphertext);

this._saveSession(theirDeviceIdentityKey, session, txn);
const sessionInfo = {
session,
// this counts as an received message: set last received message time
Copy link
Member

Choose a reason for hiding this comment

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

s/an/a/

// to now
lastReceivedMessageTs: Date.now(),
};
this._saveSession(theirDeviceIdentityKey, sessionInfo, txn);

result = {
payload: payloadString,
Expand Down Expand Up @@ -558,13 +577,20 @@ OlmDevice.prototype.getSessionIdsForDevice = async function(theirDeviceIdentityK
* @return {Promise<?string>} session id, or null if no established session
*/
OlmDevice.prototype.getSessionIdForDevice = async function(theirDeviceIdentityKey) {
const sessionIds = await this.getSessionIdsForDevice(theirDeviceIdentityKey);
if (sessionIds.length === 0) {
const sessionInfos = await this.getSessionInfoForDevice(theirDeviceIdentityKey);
if (sessionInfos.length === 0) {
return null;
}
// Use the session with the lowest ID.
sessionIds.sort();
return sessionIds[0];
// Use the session that has most recently received a message
sessionInfos.sort((a, b) => {
if (a.lastReceivedMessageTs !== b.lastReceivedMessageTs) {
return a.lastReceivedMessageTs - b.lastReceivedMessageTs;
} else {
if (a.sessionId === b.sessionId) return 0;
return a.sessionId < b.sessionId ? -1 : 1;
}
});
return sessionInfos[sessionInfos.length - 1].sessionId;
Copy link
Member

Choose a reason for hiding this comment

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

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;

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, I was reluctant to bother changing it since it was already a sort, but fair enough

};

/**
Expand All @@ -587,9 +613,10 @@ OlmDevice.prototype.getSessionInfoForDevice = async function(deviceIdentityKey)
this._cryptoStore.getEndToEndSessions(deviceIdentityKey, txn, (sessions) => {
const sessionIds = Object.keys(sessions).sort();
for (const sessionId of sessionIds) {
this._unpickleSession(sessions[sessionId], (session) => {
this._unpickleSession(sessions[sessionId], (sessInfo) => {
info.push({
hasReceivedMessage: session.has_received_message(),
lastReceivedMessageTs: sessInfo.lastReceivedMessageTs,
hasReceivedMessage: sessInfo.session.has_received_message(),
sessionId: sessionId,
});
});
Expand Down Expand Up @@ -620,9 +647,9 @@ OlmDevice.prototype.encryptMessage = async function(
await this._cryptoStore.doTxn(
'readwrite', [IndexedDBCryptoStore.STORE_SESSIONS],
(txn) => {
this._getSession(theirDeviceIdentityKey, sessionId, txn, (session) => {
res = session.encrypt(payloadString);
this._saveSession(theirDeviceIdentityKey, session, txn);
this._getSession(theirDeviceIdentityKey, sessionId, txn, (sessionInfo) => {
res = sessionInfo.session.encrypt(payloadString);
this._saveSession(theirDeviceIdentityKey, sessionInfo, txn);
});
},
);
Expand All @@ -647,9 +674,10 @@ OlmDevice.prototype.decryptMessage = async function(
await this._cryptoStore.doTxn(
'readwrite', [IndexedDBCryptoStore.STORE_SESSIONS],
(txn) => {
this._getSession(theirDeviceIdentityKey, sessionId, txn, (session) => {
payloadString = session.decrypt(messageType, ciphertext);
this._saveSession(theirDeviceIdentityKey, session, txn);
this._getSession(theirDeviceIdentityKey, sessionId, txn, (sessionInfo) => {
payloadString = sessionInfo.session.decrypt(messageType, ciphertext);
sessionInfo.lastReceivedMessageTs = Date.now();
this._saveSession(theirDeviceIdentityKey, sessionInfo, txn);
});
},
);
Expand Down Expand Up @@ -679,8 +707,8 @@ OlmDevice.prototype.matchesSession = async function(
await this._cryptoStore.doTxn(
'readonly', [IndexedDBCryptoStore.STORE_SESSIONS],
(txn) => {
this._getSession(theirDeviceIdentityKey, sessionId, txn, (session) => {
matches = session.matches_inbound(ciphertext);
this._getSession(theirDeviceIdentityKey, sessionId, txn, (sessionInfo) => {
matches = sessionInfo.session.matches_inbound(ciphertext);
});
},
);
Expand Down
19 changes: 15 additions & 4 deletions src/crypto/store/indexeddb-crypto-store-backend.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,10 @@ export class Backend {
getReq.onsuccess = function() {
const cursor = getReq.result;
if (cursor) {
results[cursor.value.sessionId] = cursor.value.session;
results[cursor.value.sessionId] = {
session: cursor.value.session,
lastReceivedMessagets: cursor.value.lastReceivedMessageTs,
Copy link
Member

Choose a reason for hiding this comment

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

Why is the case changed here (lastReceivedMessagets vs lastReceivedmessageTs)?

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, good spot

};
cursor.continue();
} else {
try {
Expand All @@ -332,7 +335,10 @@ export class Backend {
getReq.onsuccess = function() {
try {
if (getReq.result) {
func(getReq.result.session);
func({
session: getReq.result.session,
lastReceivedMessagets: getReq.result.lastReceivedMessageTs,
Copy link
Member

Choose a reason for hiding this comment

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

same here

});
} else {
func(null);
}
Expand All @@ -342,9 +348,14 @@ export class Backend {
};
}

storeEndToEndSession(deviceKey, sessionId, session, txn) {
storeEndToEndSession(deviceKey, sessionId, sessionInfo, txn) {
const objectStore = txn.objectStore("sessions");
objectStore.put({deviceKey, sessionId, session});
objectStore.put({
deviceKey,
sessionId,
session: sessionInfo.session,
lastReceivedMessageTs: sessionInfo.lastReceivedMessageTs,
});
}

// Inbound group sessions
Expand Down
16 changes: 11 additions & 5 deletions src/crypto/store/indexeddb-crypto-store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,10 @@ export default class IndexedDBCryptoStore {
* @param {string} sessionId The ID of the session to retrieve
* @param {*} txn An active transaction. See doTxn().
* @param {function(object)} func Called with A map from sessionId
* to Base64 end-to-end session.
* to session information object with 'session' key being the
* Base64 end-to-end session and lastReceivedMessageTs being the
* timestamp in milliseconds at which the session last received
* a message.
*/
getEndToEndSession(deviceKey, sessionId, txn, func) {
this._backendPromise.value().getEndToEndSession(deviceKey, sessionId, txn, func);
Expand All @@ -296,7 +299,10 @@ export default class IndexedDBCryptoStore {
* @param {string} deviceKey The public key of the other device.
* @param {*} txn An active transaction. See doTxn().
* @param {function(object)} func Called with A map from sessionId
* to Base64 end-to-end session.
* to session information object with 'session' key being the
* Base64 end-to-end session and lastReceivedMessageTs being the
* timestamp in milliseconds at which the session last received
* a message.
*/
getEndToEndSessions(deviceKey, txn, func) {
this._backendPromise.value().getEndToEndSessions(deviceKey, txn, func);
Expand All @@ -306,12 +312,12 @@ export default class IndexedDBCryptoStore {
* Store a session between the logged-in user and another device
* @param {string} deviceKey The public key of the other device.
* @param {string} sessionId The ID for this end-to-end session.
* @param {string} session Base64 encoded end-to-end session.
* @param {string} sessionInfo Session information object
* @param {*} txn An active transaction. See doTxn().
*/
storeEndToEndSession(deviceKey, sessionId, session, txn) {
storeEndToEndSession(deviceKey, sessionId, sessionInfo, txn) {
this._backendPromise.value().storeEndToEndSession(
deviceKey, sessionId, session, txn,
deviceKey, sessionId, sessionInfo, txn,
);
}

Expand Down
20 changes: 17 additions & 3 deletions src/crypto/store/localStorage-crypto-store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,21 @@ export default class LocalStorageCryptoStore extends MemoryCryptoStore {
}

_getEndToEndSessions(deviceKey, txn, func) {
return getJsonItem(this.store, keyEndToEndSessions(deviceKey));
const sessions = getJsonItem(this.store, keyEndToEndSessions(deviceKey));
const fixedSessions = {};

// fix up any old sessions to be objects rather than just the base64 pickle
for (const [sid, val] of Object.entries(sessions || {})) {
if (typeof val === 'string') {
fixedSessions[sid] = {
session: val,
};
} else {
fixedSessions[sid] = val;
}
}

return fixedSessions;
}

getEndToEndSession(deviceKey, sessionId, txn, func) {
Expand All @@ -79,9 +93,9 @@ export default class LocalStorageCryptoStore extends MemoryCryptoStore {
func(this._getEndToEndSessions(deviceKey) || {});
}

storeEndToEndSession(deviceKey, sessionId, session, txn) {
storeEndToEndSession(deviceKey, sessionId, sessionInfo, txn) {
const sessions = this._getEndToEndSessions(deviceKey) || {};
sessions[sessionId] = session;
sessions[sessionId] = sessionInfo;
setJsonItem(
this.store, keyEndToEndSessions(deviceKey), sessions,
);
Expand Down
4 changes: 2 additions & 2 deletions src/crypto/store/memory-crypto-store.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,13 +234,13 @@ export default class MemoryCryptoStore {
func(this._sessions[deviceKey] || {});
}

storeEndToEndSession(deviceKey, sessionId, session, txn) {
storeEndToEndSession(deviceKey, sessionId, sessionInfo, txn) {
let deviceSessions = this._sessions[deviceKey];
if (deviceSessions === undefined) {
deviceSessions = {};
this._sessions[deviceKey] = deviceSessions;
}
deviceSessions[sessionId] = session;
deviceSessions[sessionId] = sessionInfo;
}

// Inbound Group Sessions
Expand Down