Page MenuHomePhabricator

[web] Enable db for all users

Authored by michal on Fri, Sep 8, 6:40 AM.



Part of ENG-4844
Depends on D9108

Enabling web database for all users, including non-staff and anonymous users. Merged clearSensitiveData with init because they need to be done atomically: clearSensitiveData leaves db in uninitialized state, which caused problems in some edge cases. Also renamed notSupported to notRunning as it better describes the db state. Also simplified web/sqlite-data-handler so it has structure more similar to the native/sqlite-data-handler.

Draft migration handled in the next diff.

Test Plan

Tested on chromium and safari:

  • [on master] Replaced code in isSQLiteSupported:
return false; // <- added
if (!isDev && (!currentLoggedInUserID || !isStaff(currentLoggedInUserID))) {
  return false;
  • Logged in, made sure that there is persist:root in local storage, closed the web page
  • [Applied this diff] Opened the web page
  • Checked logs
    • "Database initialization success"
    • "migrating state to sqlite storage"
    • "version match, noop migration"
  • Checked if selected apps were persisted after reload
  • Reloaded web app, logged out, reload again -> checked if after each operation I got "Database initialization success" log

Tested on chromium:

  • [On master] Replaced the code like in the previous test case, but _didn't_ log in
  • Made sure that there is persist:root in local storage, closed the web page
  • [Applied diff + added console log when user id is set] Opened the web page, checked that the logs are the same as in the previous test case (db init and sqlite migration)
  • Logged in -> user id was set
  • Reloaded -> got "Database intialization success"
  • Logged out -> got "Clearing sensitive data" and "Database intialization success"

Diff Detail

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

158 ↗(On Diff #30862)

While keyserverStoreTransform isn't directly connected to state having version 4, they were landed at the same time, so it should work in practice (cc @inka)

158–177 ↗(On Diff #30862)

We need to simulate the keyserverStoreTransform for data stored in the old local storage (because redux persist will only run it for the sqlite storage which is empty).

We don't just use keyserverStoreTransform.out(oldStorage) because the transform might change in the future, but we need to treat this code like migration code (it shouldn't change).

michal requested review of this revision.Fri, Sep 8, 6:58 AM
kamil requested changes to this revision.Mon, Sep 11, 12:30 AM

Looks good! Only some questions inline

38 ↗(On Diff #30862)

isn't clearDatabase better than shouldRestart?
shouldRestart makes me feel like this should only restart database engine, but this also deletes all the data

45 ↗(On Diff #30862)

just wondering, if we want to try to delete data, we should do it, even if we do it twice in a row, the only exceptions:

  • init is in progress, we shouldn't break this process
  • database is not supported (we don't have a worker or indexedDB) - this is checked in line 39.
144 ↗(On Diff #30862)

it was introduced in D9105 but I'll ask here as we don't need the initial state (or in the future additional data from the endpoint)

Is there a way to somehow call getDatabaseModule() function here to start init process right ahead to make this available do redux to rehydrate earlier and improve initial loading time? Previously constructor was starting the init promise and other usage could only await this

33–36 ↗(On Diff #30862)

this condition was supposed to be optimization (but it was wrong 🤦‍♂️), we should SET_CURRENT_USER_ID only when there is no ID in db or IDs are different

158–177 ↗(On Diff #30862)

sounds like descriptive comment in code should be helpful

159–160 ↗(On Diff #30862)

are you sure we can use preloadedState state here and thil will work? Shouldn't we at least check if it's defined? (I am thinking about future changes in website responder)

This revision now requires changes to proceed.Mon, Sep 11, 12:30 AM

Rename shouldRestart, start db intialization on load, add comment for transform migration

45 ↗(On Diff #30862)

But in case of initError or notRunning there's nothing to clear anyway right?

33–36 ↗(On Diff #30862)

The code after this diff looks like the native/sqlite-data-handler.js equivalent. Not sure what do you think, should I still change the condition to !currentDBUserID || currentDBUserID !== currentLoggedInUserID?

159–160 ↗(On Diff #30862)

Even if we add a check here, this will need to be changed anyway later. I prefer to handle all the cases of using the preloadedState in the future separating-intial-redux-state stack

Could you add to the test plan case, when you first apply this diff and then log in? (I remember this was a tricky case when implementing the first version)

45 ↗(On Diff #30862)

In theory yes, but if something went wrong during deletion there might be a problem. This is just my thought about increasing security a bit, this is not a huge thing so leaving this up to you 😅

33–36 ↗(On Diff #30862)

I don't have a strong opinion, but I'll lean toward optimization and decreasing database calls when they're not needed. It was suggested here:

159–160 ↗(On Diff #30862)

makes sense

This revision is now accepted and ready to land.Tue, Sep 12, 12:53 AM
45 ↗(On Diff #30862)

just discovered that my comment doesn't make any sense, If the status is different than databaseStatuses.initSuccess we couldn't schedule this on worker anyway (e.g. the browser does not support workers) - so it's okay the way it is

Amend test plan, don't set userID if not needed. Also added a log for clearing sensitive data.

Generally looks good, would be good for @kamil to take a final pass. Only thing that I'd change before landing is early returning in handleSensitiveData.

38 ↗(On Diff #30951)

I'd maybe define something like the following above

type InitOptions = {
  +clearDatabase: boolean,

personal preference but totally up to you

40 ↗(On Diff #30951)

Again, doesn't really matter but I think it's usually stylized as SQLite instead of Sqlite

30–40 ↗(On Diff #30951)

Looks like if we early return on (currentDBUserID === currentLoggedInUserID) we can reduce one level of indentation?

This revision is now accepted and ready to land.Thu, Sep 14, 9:14 AM
kamil added 1 blocking reviewer(s): inka.

Database stuff LGTM, but after ENG-4930 I think @inka should also take a look at migrating code.

30–40 ↗(On Diff #30951)


This revision now requires review to proceed.Fri, Sep 15, 2:07 AM

Small fixes and early return

inka added inline comments.
168 ↗(On Diff #30951)

Since the variable is named oldStorage it seems weird that we are changing it. Can we just return here?

This revision is now accepted and ready to land.Fri, Sep 15, 6:30 AM

Return instead of modifying