This differential implements establishment of olm sessions for e2e notifs on both sides during log in.
Details
Details
Log in to the keyserver and examine content on olm_sessions table. New entry should appear.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| keyserver/src/responders/user-responders.js | ||
|---|---|---|
| 293 ↗ | (On Diff #25824) |
const olmSessionPromise = (async () => { if ( userViewerData.cookieID && initialNotificationsEncryptedMessage && signedIdentityKeysBlob ) { await createOlmSession( initialNotificationsEncryptedMessage, 'notifications', userViewerData.cookieID, ); } })(); const setNewSessionPromise = (async () => { if (calendarQuery) { await setNewSession(viewer, calendarQuery, newServerTime); } })(); await Promise.all([olmSessionPromise, setNewSessionPromise]); |
| native/account/log-in-panel.react.js | ||
| 235 ↗ | (On Diff #25824) | Why are you doing this await sequentially? I've shared feedback on this to you repeatedly. Please integrate it into your workflow going forward. Whenever you are adding a new await keyword, you should expect your reviewer will request changes unless it is necessary. |
| 236 ↗ | (On Diff #25824) | You appear to have left a console.log here |
| keyserver/src/responders/user-responders.js | ||
|---|---|---|
| 293 ↗ | (On Diff #25824) | I am afraid it is dangerous since setNewSession creates new cookieID in cookies table that is a foreign key in a row that createOlmSession inserts into olm_sessions table. Letting those two promises run simultaneously creates possibility that we will create olm session with cookieID that does not exist in cookies table in the case of setNewSession failure. Since we have a convention of not using FOREIGN KEY, blocking on setNewSession is our only way to ensure database is not left in inconsistent state. Definitely it is my mistake to call createOlmSession before setNewSession - it must be called afterwards. However I would block on setNewSession before createOlmSession. |
| native/account/log-in-panel.react.js | ||
| 235 ↗ | (On Diff #25824) | In this particular case it is a must, since await this.props.logInExtraInfo(); calls commCoreModule.initializeCryptoAccount which must complete before any key crypto operation takes place. Otherwise we will run into an error. |
| keyserver/src/responders/user-responders.js | ||
|---|---|---|
| 293 ↗ | (On Diff #25824) | Okay, that sounds reasonable. Nevertheless, please make sure that you do not introduce a new await here at the function level. You can probably combine this with the await on line 336. |
| native/account/log-in-panel.react.js | ||
| 235 ↗ | (On Diff #25824) | Thanks for explaining! |