Details

Reviewers

kamil
inka
atul
tomek

Commits

rCOMM2ce478126824: [web] Enable db for all users

Summary

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

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

michal created this revision.Sep 8 2023, 6:40 AM

Herald added a subscriber: ashoat. · View Herald TranscriptSep 8 2023, 6:40 AM

michal added inline comments.Sep 8 2023, 6:44 AM

web/redux/persist.js
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 added a child revision: D9110: [web] Migrate drafts to db for everyone.Sep 8 2023, 6:54 AM

Harbormaster completed remote builds in B22445: Diff 30862.Sep 8 2023, 6:58 AM

michal requested review of this revision.Sep 8 2023, 6:58 AM

Looks good! Only some questions inline

web/database/database-module-provider.js
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
web/database/sqlite-data-handler.js
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
web/redux/persist.js
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.Sep 11 2023, 12:30 AM

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

web/database/database-module-provider.js
45 ↗	(On Diff #30862)	But in case of `initError` or `notRunning` there's nothing to clear anyway right?
web/database/sqlite-data-handler.js
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`?
web/redux/persist.js
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

Harbormaster completed remote builds in B22481: Diff 30912.Sep 11 2023, 1:43 AM

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)

web/database/database-module-provider.js
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 😅
web/database/sqlite-data-handler.js
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: https://phab.comm.dev/D7191?id=24556#inline-48006
web/redux/persist.js
159–160 ↗	(On Diff #30862)	makes sense

This revision is now accepted and ready to land.Sep 12 2023, 12:53 AM

kamil added inline comments.Sep 12 2023, 1:32 AM

web/database/database-module-provider.js
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

michal edited the test plan for this revision. (Show Details)Sep 12 2023, 1:39 AM

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

michal requested review of this revision.Sep 12 2023, 1:42 AM

Harbormaster completed remote builds in B22514: Diff 30951.Sep 12 2023, 1:59 AM

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.

web/database/database-module-provider.js
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
web/database/sqlite-data-handler.js
30–40 ↗	(On Diff #30951)	Looks like if we early return on (currentDBUserID === currentLoggedInUserID) we can reduce one level of indentation?