Page MenuHomePhabricator

[web-db] implement clearing sensitive data
ClosedPublic

Authored by kamil on Mar 27 2023, 6:28 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Mar 11, 5:30 PM
Unknown Object (File)
Mon, Mar 11, 5:13 PM
Unknown Object (File)
Sat, Mar 9, 8:36 AM
Unknown Object (File)
Fri, Mar 8, 6:57 PM
Unknown Object (File)
Fri, Mar 8, 6:57 PM
Unknown Object (File)
Fri, Mar 8, 6:57 PM
Unknown Object (File)
Fri, Mar 8, 6:57 PM
Unknown Object (File)
Fri, Mar 8, 6:56 PM
Subscribers

Details

Summary

Initial version of hook responsible for handling sensitive data.

Test Plan

Tested on both web and desktop

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

image.png (642×656 px, 68 KB)

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

image.png (283×1 px, 37 KB)

  1. Log in
    • console should log Database initialization success
    • check indexedDB content

image.png (694×693 px, 95 KB)

  • see if worker was spawned

image.png (297×1 px, 33 KB)

    • see Worker logs (it's separate console)
    • it should say Creating fresh database - Db version: 0
  1. Close the tab
    • worker should die (not listed)
  2. 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
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Mar 27 2023, 8:47 AM
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

remove unused effect dependency

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

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
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

This revision is now accepted and ready to land.Apr 28 2023, 3:35 AM
kamil edited the summary of this revision. (Show Details)

rebase before landing

This revision was automatically updated to reflect the committed changes.