This differential initialises olm session for notifications on web via server request mechanism. The keyserver checks whether the web client has the olm session. If it doesn't it requests session intialisation. The web client uses the shared logic to get necessary keys from the keyserver and web specific logic to create the session and store it in IndexedDB.
Details
Details
- Checkout to master.
- Log-out.
- Log-in.
- Checkout to the branch with this diff stack.
Then:
- Using mariadb console ensure that new olm session was created on the keyserver.
- Using Developer Tools ensure that new entry was created in IndexedDB and that it contains pickled olm session for notifs.
- Log-out.
- Using Developer Tools ensure that entry with olm session for notifs was removed (it is not implemented in this differential but this line(https://github.com/CommE2E/comm/blob/master/web/database/worker/db-worker.js#L154) should clear entire IndexedDB on log-out).
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-5191-v2
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| web/account/account-hooks.js | ||
|---|---|---|
| 168 ↗ | (On Diff #32188) | If we try to initialize olm session for notifications but there are no olm accounts it means that we either:
or
Both are programmer errors. Theoretically we could use gerOrCreateCryptoStore here but I am afraid it would aallow programmers to introduce errors described above. |
| keyserver/src/session/cookies.js | ||
|---|---|---|
| 838 ↗ | (On Diff #32188) | Note that this will need to be updated to NEXT_CODE_VERSION in order for me to update it before a deploy (I think you're aware of this though) |
| web/account/account-hooks.js | ||
| 154 ↗ | (On Diff #32188) | Same concern as I outlined here: https://phab.comm.dev/D9532#inline-60149 |
Comment Actions
Ensure that there are no two notifications session creations promises running at the same time.
Comment Actions
Looks good, might be good to have someone with more knowledge about crypto system take another look
Comment Actions
Bundle picling key and pickled notifications session together and persist encrypted in IndexedDB.
This is necessary since we can't access redux (needed for pickling key of notifications account) and persisting pickling key in IndexedDB without additional
encryption is not acceptable either (due to securoty issues).
| web/account/account-hooks.js | ||
|---|---|---|
| 230 ↗ | (On Diff #32765) | Might be good to explain why Safari is special-cased here with a comment |
Comment Actions
This seems like a significant change. @marcin, should you consider re-requesting review?
Comment Actions
Substantial changes are actually done to D9661. Logics of this differential remains unchanged - we just change the structure of data kept in IndexedDB. But for better safety I can re-request review.
| keyserver/src/session/cookies.js | ||
|---|---|---|
| 838 ↗ | (On Diff #32188) | Yes, I understand this. I always start with FURUTRE_CODE_VERSION to handle the possibility that we will not be able to release the feature immediately after landing. If I now that the feature can be released immediately after landing I replace it with NEXT_CODE_VERSION. |
Comment Actions
Make pendingSessionUpdate and updateCreationTimestamp fialds mandatory and initialize them with a copy of mainSession and Date.now() respectively.
| keyserver/src/session/cookies.js | ||
|---|---|---|
| 836 ↗ | (On Diff #33377) | (reminder about NEXT_CODE_VERSION) |