Page MenuHomePhabricator

[web-db] support SQLite for staff and non-mobile browsers
ClosedPublic

Authored by kamil on Mar 27 2023, 6:21 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Jul 22, 8:15 AM
Unknown Object (File)
Tue, Jul 2, 1:51 PM
Unknown Object (File)
Sun, Jun 30, 11:38 PM
Unknown Object (File)
Fri, Jun 28, 10:04 PM
Unknown Object (File)
Fri, Jun 28, 10:31 AM
Unknown Object (File)
Thu, Jun 27, 5:44 AM
Unknown Object (File)
Wed, Jun 26, 2:52 PM
Unknown Object (File)
Wed, Jun 26, 2:52 PM
Subscribers

Details

Summary

Logic based on detect-browser

Test Plan
  1. Test for web app and electron
  2. Test in browser on mobile

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.

annotate function return type

kamil published this revision for review.Mar 27 2023, 8:15 AM
ashoat requested changes to this revision.Mar 27 2023, 4:09 PM
ashoat added inline comments.
keyserver/src/responders/website-responders.js
433 ↗(On Diff #24189)

I guess perhaps headers are case-insensitive, but elsewhere in the app we use User-Agent

Can we actually just get this from viewer.userAgent?

435 ↗(On Diff #24189)

What happens if the user logs in as a staff user, but then logs out and logs back in as a non-staff? (Or vice versa?) Seems like it can get messy... is there any way to determine this on the client instead?

444 ↗(On Diff #24189)

Can you explain in more detail why you need this to be determined on the keyserver and passed in from there? Why can't we decide this on the client?

keyserver/src/utils/validation-utils.js
182–185 ↗(On Diff #24189)

Can we use browser-detect, which we already use elsewhere in our codebase?

This revision now requires changes to proceed.Mar 27 2023, 4:09 PM

move logic to client-side

Thanks, @ashoat. You identified a severe issue with doing it on the keyserver. Moving logic to client-side

web/database/utils/constants.js
11 ↗(On Diff #24551)

win 11 is still detected as 11 due to the old kernel version (issue).

I didn't include older windows versions as they're not officially supported by Microsoft anymore.

16–23 ↗(On Diff #24551)

based on caniuse

web/database/utils/constants.js
16 ↗(On Diff #24551)

this is possible:

import { Browser } from 'detect-browser';

...

export const DB_SUPPORTED_BROWSERS: $ReadOnlyArray<Browser> = [

but to do that we need to add flow types for detect-browser

ashoat added inline comments.
web/database/utils/constants.js
11 ↗(On Diff #24551)

@kamil meant "detected as 10" here I think (based on linked issue)

web/database/utils/db-utils.js
37–38 ↗(On Diff #24551)

I would replace this with:

if (!isDev && (!currentLoggedInUserID || !isStaff(currentLoggedInUserID))) {
  return false;
}
40–43 ↗(On Diff #24551)

And then this could be:

return DB_SUPPORTED_OS.includes(browser.os)
  && DB_SUPPORTED_BROWSERS.includes(browser.name);
This revision is now accepted and ready to land.Apr 3 2023, 1:57 PM
web/database/utils/constants.js
11 ↗(On Diff #24551)

yeah, I had "detected as 10" in mind, sorry for confusion