Details

Reviewers

michal
tomek
marcin

Commits

rCOMM458a0db0527a: [web-db] implement clearing sensitive data

Summary

Initial version of hook responsible for handling sensitive data.

Test Plan

Tested on both web and desktop

Make sure the user is logged out and there is no opened app
- IndexedDB storage should be empty

And view shared workers (in Chrome it's URL chrome://inspect/#workers) and there shouldn't be anything connected to localhost.

see if worker was spawned

- see Worker logs (it's separate console)
- it should say Creating fresh database - Db version: 0
Close the tab
- worker should die (not listed)
Open the tab
- console should log Database initialization success
- see if worker was spawned
- see Worker logs - it should say Database exists and is properly encrypted, using persisted data
- Log out
- IndexedDB content should be deleted
- worker is alive until tab is opened

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kamil created this revision.Mar 27 2023, 6:28 AM

kamil held this revision as a draft.

Herald added subscribers: atul, ashoat. · View Herald TranscriptMar 27 2023, 6:28 AM

kamil added a parent revision: D7190: [web-db] implement custom `redux-persist` storage.Mar 27 2023, 6:29 AM

Harbormaster failed remote builds in B17665: Diff 24185!Mar 27 2023, 6:44 AM

rebase

Harbormaster failed remote builds in B17688: Diff 24211!Mar 27 2023, 8:25 AM

amend missing changes

Harbormaster completed remote builds in B17694: Diff 24217.Mar 27 2023, 8:47 AM

kamil published this revision for review.Mar 27 2023, 8:47 AM

ashoat added inline comments.Mar 27 2023, 4:15 PM

web/database/database-module-provider.js
81 ↗	(On Diff #24217)	Doesn't need to be a template literal
web/database/sqlite-data-handler-web.js
11 ↗	(On Diff #24217)	Don't think we need the "Web" part here or in the filename

tomek requested changes to this revision.Mar 30 2023, 4:58 AM

tomek added inline comments.

web/database/database-module-provider.js
67–69 ↗	(On Diff #24217)	Should we ignore the exception here?
91–93 ↗	(On Diff #24217)	We can also consider rejecting a promise
web/database/sqlite-data-handler-web.js
50–52 ↗	(On Diff #24217)	This is equivalent to just `handleSensitiveData()`

This revision now requires changes to proceed.Mar 30 2023, 4:58 AM

refactor

remove unused effect dependency

kamil added inline comments.Apr 3 2023, 3:02 AM

web/database/database-module-provider.js
85 ↗	(On Diff #24556)	clearing database means that there is no user - and we do not support database for anonymous users
67–69 ↗	(On Diff #24217)	it's caught in call site
web/database/sqlite-data-handler.js
36 ↗	(On Diff #24556)	I think we could try to manually clear indexedDB with persistent content here if `clearSensitiveData` fails, I can introduce this in next differential

Harbormaster completed remote builds in B17925: Diff 24555.Apr 3 2023, 3:12 AM

Harbormaster completed remote builds in B17926: Diff 24556.

marcin accepted this revision.Apr 3 2023, 4:30 AM

marcin added inline comments.

web/database/worker/db-worker.js
179 ↗	(On Diff #24556)	A typical question: can this throw or fail and if so - should we handle such situation?

tomek requested changes to this revision.Apr 4 2023, 2:05 AM

tomek added inline comments.

web/database/database-module-provider.js
85 ↗	(On Diff #24556)	This is a little confusing as `notSupported` should mean that e.g. a db can't be created on this device due to e.g. technical limitations. But it may make sense to keep this name if it would be handled in the same way as our current not supported status.
web/database/sqlite-data-handler.js
29–34 ↗	(On Diff #24556)	Do we have to always call this code? Maybe we should call it only when `currentUserData.userID !== currentLoggedInUserID`?
36 ↗	(On Diff #24556)	Better error handling is a good idea - manual clearing sounds reasonable. At least, we shouldn't swallow the exception.
42–52 ↗	(On Diff #24556)	Is this logic correct? Is it possible to call `userLoggedIn`, then return because `!rehydrateConcluded`, then in the next render, call `userLoggedIn` again?
43 ↗	(On Diff #24556)	Can we find a better name than `userLoggedIn`? A function that performs an action should contain a verb in its name.

This revision now requires changes to proceed.Apr 4 2023, 2:05 AM

refactor code

kamil edited the summary of this revision. (Show Details)Apr 27 2023, 6:41 AM

kamil edited the test plan for this revision. (Show Details)

kamil added inline comments.

web/database/sqlite-data-handler.js
29–34 ↗	(On Diff #24556)	right, makes sense
36 ↗	(On Diff #24556)	rethrowing error for now - creating a task for better error handling ENG-3610.
42–52 ↗	(On Diff #24556)	Replying but after renaming `userLoggedIn` => `initDBForLoggedInUser` I know it's confusing (probably because of the naming), but we will move the entire redux to the database, which means we might want to call `initDBForLoggedInUser` before rehydration. In the next render calling `initDBForLoggedInUser` will have no effect.
43 ↗	(On Diff #24556)	renaming to `initDBForLoggedInUser`
web/database/worker/db-worker.js
179 ↗	(On Diff #24556)	it will propagate from worker to worker proxy, then to the database module provider, and then the caller should handle this - in that case, it's in `sqlite-data-handler`