This differential moves notifications crypto account persistence from SQLite to IndexedDB on web
Details
Details
Execute test plan for D12676. If it works it means we can establish sessions between web and other devices.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- marcin/eng-8237
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| web/shared-worker/worker/worker-crypto.js | ||
|---|---|---|
| 233 | This is migration logic. | |
Comment Actions
Right now two JS threads (shared worker and service worker) can concurrently modify the same key in IndexedDB. I think to make it safe we should use transactions to make sure this is safe.
Unfortunately, looks like locaforage is not supporting this: https://github.com/localForage/localForage/issues/582 and https://github.com/localForage/localForage/issues/17.
Could you do some research about whether it's safe or suggest an alternative solution?
(this could change most of the code so not reviewing details of this diff)
Comment Actions
This differential was updated to IndexedDB transactions that are available through localforage modifications introduced in parent diff. This diff can be reviewed by just looking at types introduced in localforage_v1.5.js in parent diff.
Comment Actions
Overall idea looks okay, I am afraid about performance but I don't think there is a better alternative for now.
- Could you do detailed testing of all edge-cases?
- Could you include Safari in the testing plan?
- Next time, could you avoid creating diffs doing a lot of things? (refactoring, moving code, updating logic, migrating) - this one was really hard to review.
| web/push-notif/notif-crypto-utils.js | ||
|---|---|---|
| 1 ↗ | (On Diff #42716) | Could we somehow divide this file to extract methods run only on service workers to a separate file? This can be done as a separate diff |
| 80 ↗ | (On Diff #42716) | not sure why we need DB_LABEL suffix, isn't this confusing? |
| 135–144 ↗ | (On Diff #42716) | make params read-only |
| 168–170 ↗ | (On Diff #42716) | do we need this? should be included as part of above check (line 160) |
| 226 ↗ | (On Diff #42716) | |
| 492–494 ↗ | (On Diff #42716) | Can this type be more specific - to avoid refinement? |
| 511–518 ↗ | (On Diff #42716) | I would use a validator here - or makes types more specific |
| 522 ↗ | (On Diff #42716) | This error should be more specific |
| 583–585 ↗ | (On Diff #42716) | |
| 594–609 ↗ | (On Diff #42716) | looking at usage, don't think this is needed, see my comment below on callsite |
| 619 ↗ | (On Diff #42716) | why this could not also be ?SubtleCrypto$JsonWebKey? |
| 628–630 ↗ | (On Diff #42716) | according to the type above we know that notifsAccountEncryptionKey is ?CryptoKey - so what is the purpose of calling validateCryptoKey? |
| web/shared-worker/worker/worker-crypto.js | ||
| 205–210 ↗ | (On Diff #42716) | this can be moved to the top at this function, as it's used before destructing props here |
| 253–258 ↗ | (On Diff #42716) | what is the purpose of this syntax and awaiting this? |
| 253–271 ↗ | (On Diff #42716) | this logic cause fetching notif account from IDB twice, I think can be simplified |
| 254–256 ↗ | (On Diff #42716) | could you explain this check? |
| 233 | shouldn't we remove data from SQLite? or at least override with an empty string? | |
| web/push-notif/notif-crypto-utils.js | ||
|---|---|---|
| 492–494 ↗ | (On Diff #42716) | It is not going to work since olmEncryptionKeyDBLabel` is variable defined at runtime. |
| 522 ↗ | (On Diff #42716) | Actually it is specific. The validateCryptoKey will only return undefined if crypto key is undefined. This can only happen if session is not initialized. |
| 628–630 ↗ | (On Diff #42716) | Because as you correctly caught in the comment above the type above is wrong and it can be ?SubtleCrypto$JsonWebKey |
| web/shared-worker/worker/worker-crypto.js | ||
| 253–258 ↗ | (On Diff #42716) | The purpose of awaiting this to know whether we should enter condition at 260 or not provided we are calling getOrCreateOlmAccount for notifs account.
The question about syntax is very vague to me. I don't understand which syntax are you referring to. I think that I am not using any unusual syntax or violating any conventions of our codebase. |
| 254–256 ↗ | (On Diff #42716) | If we are calling getOrCreateOlmAccount for content we don't want to enter condition at line 260. This check ensures this. |
| web/push-notif/notif-crypto-utils.js | ||
|---|---|---|
| 80 ↗ | (On Diff #42716) | When we were implementing keyserver encrypted notifs on November I wanted to use suffix ...keyDBKey but on of the reviewers suggested that it is strange and keyDBLabel sounds better. |
| web/push-notif/notif-crypto-utils.js | ||
|---|---|---|
| 505–508 ↗ | (On Diff #43006) | I think it's better to move this code into validateCryptoKey |
| 607 ↗ | (On Diff #43006) | you should add validation here or better - move invocation of validator as part of validateCryptoKey |
| 1 ↗ | (On Diff #42716) | Can you link diff or at least create a follow-up task? |
| web/shared-worker/worker/worker-crypto.js | ||
| 253–258 ↗ | (On Diff #42716) | I was asking about creating an additional promise wrapper: const x = await (async () => { return await foo(); })();is the same as: const x = await foo(); you can achieve the same logic without this: let maybeNotifsCryptoAccount: ?NotificationAccountWithPicklingKey =
undefined;
if (accountIDInDB == sqliteQueryExecutor.getNotifsAccountID()) {
try {
maybeNotifsCryptoAccount = await getNotifsCryptoAccount();
} catch (e) {}
}Which is shorter and I think more readable but leaving the final decision up to you |
| web/push-notif/notif-crypto-utils.js | ||
|---|---|---|
| 505–508 ↗ | (On Diff #43006) | Discussed it with @kamil that it is not necessary. The purpose of assertWithValidator is to do type refinement before passing to validateCryptoKey. We don't need assertWithValidator if flow lets us pass variable to validateCryptoKey right away. |
| 607 ↗ | (On Diff #43006) | As explained in the comment above it it not necessary since the type is understood to flow and doesn't need refinement. |
| web/shared-worker/worker/worker-crypto.js | ||
| 253–258 ↗ | (On Diff #42716) | The code this comment refers to is not a direct equivalent of the example you provided because of the if statement inside a promise. I think that current approach is better since we can define immutable variable while yours introduces mutability. |