Details

Reviewers

Commits

Summary

Logic based on detect-browser

Test Plan

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

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