Multitenant: Add filtering by tenant_id #282

mattbroussard · 2022-05-26T07:08:59Z

Depends on #259

See overall GOST integration plan: https://www.notion.so/usdr/GOST-ARPA-integration-technical-plan-cffc5800c0414660816a0920b959da82#b4dfb0449fc54674ae97f070a6b7b33a

This PR adds filtering and access checks based on the current user's tenant_id in (as far as I know) all places that need it. In most places the tenant ID to filter by comes from the logged in user, via a new req.session.user object populated by requireUser middleware that's used almost everywhere. In a few cases, functions that deal with other types of entities (mainly uploads) get a tenant ID from that object.

Most changes are serverside only and clients are generally not aware of the concept of tenants. My approach was to go through all the DB accessor functions in src/server/db and trace backward to the places that call them. This was a bit tricky in the absence of TypeScript since I had to rely on grep and we do some annoying things like renaming exports and imports.

One notable clientside change is that previously /api/application_settings was called before logging in and thus could not use requireUser to determine a tenant ID to filter by. I've changed this so that this data is not loaded until after login now, and the endpoint requires authentication. Accordingly, the navbar with the period dropdown (which draws from this data) is now hidden before login. There is a slight race condition after logging in since nothing explicitly waits for application_settings to be populated, but the only apparent issue in the gap before it loads is that the period selector is not populated. This doesn't seem like a huge deal, and that gap is very short.

Regarding dead code:

This PR removes routes/files and all code referencing UPLOAD_DIRECTORY. This seemed to be dead (posted about it on Slack here) and was problematic to keep around because it doesn't interact with the database and couldn't be easily tenant-scoped. Also, the implementation of the /api/files/delete endpoint was insecure and could have been used to arbitrarily delete any file on the machine!
I did not attempt for now to add filtering logic in db/subrecipients or routes/subrecipients since there is a concurrent PR Igor/recipients #260 that is removing/revamping these. I had previously made changes to several other now-deleted files that caused some issues when rebasing over Additional UI for upload tracking #249 edit: now that Igor/recipients #260 has merged, I did go back and add tenantId checks to code dealing with the revamped subrecipients model.

For DB seeds, the "dev" ones (which I think may also be used in prod deployments? Staging at least has data that looks like it came from them) still assume a single tenant with ID 0. Some of the "test" seeds add data for multiple tenants so that the tests can verify multitenant behaviors.

In this PR I also added a mechanism for writing API route tests using Supertest (this is conceptually similar to how API route tests work in GOST but imo a bit cleaner and something we might want to move GOST over to eventually). I added a few route tests verifying this works (health & sessions) and a few (exports) for multitenant, and I'd like to add some more, but I'm going to try to put those in a separate PR so this one stops growing and can land sooner.

…e login, and hide navbar on login since it uses some fields from applicationSettings Note during rebase: originially I'd killed `loadApplicationSettings` in favor of `login` calling `doFetch('application_settings')`, but #249 added new logic to `loadApplicationSettings` so had to bring it back during rebase. We still do NOT call it before login in main.js anymore.

…l for debugging)

Rebase note: formerly these functions were in db/index

…lly omitted during interactive rebase

…tests)

…move/revamp

…environment variable

…ities by ID (mainly uploads)

…t on the request object

…stuff

…eleted

ajhyndman

Directionally, this looks great. Feel free to leave some of these TODOs in the code for @igor47 and I to resolve.

Here are my thoughts on how to adapt seeds:

We currently use db seed files primarily for unit tests and local execution. These shouldn't run in any production or staging environments. From that point of view, whatever changes we make to seed files should align to the goal of giving developers a basically usable configuration after running.

One approach could be to set initial users, reporting periods and application settings to the default tenant_id.

src/server/db/reporting-periods.js

src/server/db/subrecipients.js

src/server/db/uploads.js

src/server/routes/exports.js

…nts (new after merge)

…en reworked and tenant_id checks readded in 0f574f8

mattbroussard · 2022-06-10T02:32:47Z

Github seems to have done some confusing thing with my stacked PRs... Since #259 was merged, it says:

Base automatically changed from multitenant-migrations-1 to staging 13 days ago

But then there are commits I've pushed to this branch past multitenant-migrations-1 that are not in staging yet, but they do not show up on the PR page. The changes from them do show up in "Files changed" though.

Specifically:

6faa25d chore: rm TODO comment about missing user arg; the bad callsite was deleted
9f4482b chore: add missing trns to a few getCurrentReportingPeriodID callsites

…flicts from #314, #317)

…rom #332)

…st during git merge

…and reporting_periods

…ntually removing the default value from the column

igor47

left comments of varying pertinence throughout the diff here, but nothing is blocking. maybe just go through my comments and make sure nothing jumps out at you?

igor47 · 2022-06-24T20:01:11Z

seeds/06_migrate_from_source.js

@@ -1,22 +1,27 @@
 require('dotenv').config()

-exports.seed = async function (knex) {
+// NOTE(mbroussard): unclear to me if this seed will be used again, but I'm updating it to minimally


it probably won't get used again; we should maybe just drop this file, and also the migration script in scripts esp now that we have a blueprint

OK. I'm probably not going to do that in this PR, but if you'd like to kill it separately, go ahead 👍

src/components/Navigation.vue

igor47 · 2022-06-24T20:03:59Z

src/main.js

-  } else {
-    await store.dispatch('updateApplicationSettings')


(Answered above and in PR description)

igor47 · 2022-06-24T20:07:16Z

src/server/db/agencies.js

@@ -6,8 +6,13 @@ function baseQuery (trns) {
    .select('*')
 }

-function agencies (trns = knex) {
+function agencies (tenantId, trns = knex) {
+  if (tenantId === undefined) {


why do we explicitly test for undefined ?

Because the default tenant ID is 0 which is falsey, so if (tenantId) is not enough.

src/server/services/validation-rules.js

igor47 · 2022-06-24T21:01:59Z

src/server/routes/agencies.js

@@ -33,10 +34,19 @@ router.post('/', requireAdminUser, async function (req, res, next) {

  try {
    if (agencyInfo.id) {
+      const existingAgency = await agencyById(agencyInfo.id)
+      if (!existingAgency || existingAgency.tenant_id !== req.session.user.tenant_id) {


huh, how would the second condition ever happen?

Someone could do a request passing in an agency ID that is valid but refers to an agency of a different tenant.

igor47 · 2022-06-24T21:03:38Z

src/server/routes/exports.js

@@ -4,11 +4,17 @@ const _ = require('lodash')



don't think the exports routes are in-use

Well, the code exists still and it theoretically could be called, so it needs to obey multitenant rules. If you want to kill it after this PR merges, that's cool 👍

Hmm, it does seem in use: the main "Download Treasury Report" button on the dashboard links to /api/exports

src/server/routes/configuration.js

src/server/routes/uploads.js

…from #348)

…that fails since we removed the projects seed

…an equivalent one

mattbroussard added 15 commits May 25, 2022 13:50

feat: add req.session.user

e346a45

feat: add tenant_id to db/settings methods & callers (minus tests)

cc9b01c

fix: (drive-by) misspelled state.viewPeriodID in store

31098af

fix: null vs undefined in currentReportingPeriodIDCache

780be73

chore: add express-async-errors dep (not related to this PR but usefu…

8c577c6

…l for debugging)

fix: tenant_id, not tenantId in db query

122bc82

fix: fix tests for application_settings changes (no new tests yet)

b53cec6

feat: add tenant_id to db/users methods & callers (minus tests)

fb8b260

Rebase note: formerly these functions were in db/index

feat: add tenant_id to db/agencies methods & callers (minus tests)

cc45cbc

feat: add tenant_id to db/audit-report methods & callers (minus tests)

e49705f

fix: commit merge conflict fixes to generate-arpa-report.js accidenta…

8695719

…lly omitted during interactive rebase

feat: add tenant_id to db/reporting-periods methods & callers (minus …

80ce88e

…tests)

feat: add tenant_id to db/uploads methods & callers (minus tests)

44b8b36

chore: TODO comments in subrecipient-related files since #260 will re…

9348c92

…move/revamp

mattbroussard mentioned this pull request May 26, 2022

Add tsconfig and @typescript-eslint/no-floating-promises lint rule #294

Merged

mattbroussard added 6 commits May 26, 2022 17:25

chore: delete routes/files and anything referencing UPLOAD_DIRECTORY …

facfd47

…environment variable

chore: add tenant access checks in routes that deal with specific ent…

1000ac7

…ities by ID (mainly uploads)

chore: get rid of redundant getUser calls now that requireUser puts i…

3f395d8

…t on the request object

chore: oops, forgot to kill an export when removing UPLOAD_DIRECTORY …

f6d1485

…stuff

chore: add missing trns to a few getCurrentReportingPeriodID callsites

9f4482b

chore: rm TODO comment about missing user arg; the bad callsite was d…

6faa25d

…eleted

Base automatically changed from multitenant-migrations-1 to staging May 27, 2022 21:39

mattbroussard mentioned this pull request May 27, 2022

Multitenant: initial DB migration #259

Merged

mattbroussard changed the title ~~[WIP] Multitenant: Add filtering by tenant_id~~ Multitenant: Add filtering by tenant_id May 27, 2022

mattbroussard requested review from igor47 and ajhyndman May 27, 2022 22:02

mattbroussard marked this pull request as ready for review May 27, 2022 22:03

ajhyndman reviewed Jun 8, 2022

View reviewed changes

Merge branch 'staging' into multitenant-filtering-1

90e47fd

mattbroussard added 2 commits June 9, 2022 19:19

chore: add tenant_id filtering + access checks for db/arpa_subrecipie…

0f574f8

…nts (new after merge)

chore: rm TODO comment in routes/subrecipients since that file has be…

3b88138

…en reworked and tenant_id checks readded in 0f574f8

mattbroussard added 4 commits June 15, 2022 15:01

Merge branch 'staging' into multitenant-filtering-1 (fixing merge con…

4455716

…flicts from #314, #317)

fix: ambiguous tenant_id join in listUsers

7c28c1b

chore: add basic runner for route tests

89bac80

chore: add ability to simulate login cookie in tests

f0c9d5e

This was referenced Jun 17, 2022

Duplicate key errors inserting sub-recipients #337

Closed

Reporting period dropdown menu assumes reporting period IDs are sequential #336

Closed

mattbroussard added 9 commits June 21, 2022 10:19

fix: don't depend on reporting_period.id being sequential (fixes #336)

d56906c

Merge branch 'staging' into multitenant-filtering-1 (most conflicts f…

b4f57fa

…rom #332)

chore: fix tests after git merge

662a822

chore: reapply reporting periods dropdown sorting fix accidentally lo…

9534cb5

…st during git merge

chore: don't show raw period ID in UI

02dcb1f

chore: update seeds and existing unit tests for application_settings …

c0371ab

…and reporting_periods

chore: remove some stale todos

6d51272

chore: /api/exports tenant check + test

7b0bbd4

chore: lint

10e4f54

mattbroussard requested a review from ajhyndman June 23, 2022 22:40

chore: add tenantId explicitly to db seed so they don't fail when eve…

0e87234

…ntually removing the default value from the column

mattbroussard mentioned this pull request Jun 24, 2022

Multitenant: Migration to remove default value of tenantId #354

Merged

igor47 approved these changes Jun 24, 2022

View reviewed changes

mattbroussard added 7 commits June 24, 2022 14:09

Merge branch 'staging' into multitenant-filtering-1 (Fixes conflicts …

3a107ba

…from #348)

chore: kill a getUser callsite

65b9b6a

chore: kill routes/configuration accidentally brought back during merge

03841c7

chore: kill seeds/06_projects, table is unused

d432ca0

chore: add requiredArgument helper

ef8c56f

chore: kill 09_period_summaries seed; it is also unused and has a FK …

0244922

…that fails since we removed the projects seed

chore: rm redundant loggedIn check in Navigation since 4f08dcb added …

c474a8f

…an equivalent one

mattbroussard merged commit a19ec28 into staging Jun 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multitenant: Add filtering by tenant_id #282

Multitenant: Add filtering by tenant_id #282

mattbroussard commented May 26, 2022 •

edited

Loading

ajhyndman left a comment •

edited

Loading

mattbroussard commented Jun 10, 2022

igor47 left a comment

igor47 Jun 24, 2022

mattbroussard Jun 24, 2022

igor47 Jun 24, 2022

mattbroussard Jun 24, 2022 •

edited

Loading

igor47 Jun 24, 2022

mattbroussard Jun 24, 2022

igor47 Jun 24, 2022

mattbroussard Jun 24, 2022

igor47 Jun 24, 2022

mattbroussard Jun 24, 2022

mattbroussard Jun 24, 2022

Multitenant: Add filtering by tenant_id #282

Multitenant: Add filtering by tenant_id #282

Conversation

mattbroussard commented May 26, 2022 • edited Loading

ajhyndman left a comment • edited Loading

Choose a reason for hiding this comment

mattbroussard commented Jun 10, 2022

igor47 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattbroussard Jun 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattbroussard commented May 26, 2022 •

edited

Loading

ajhyndman left a comment •

edited

Loading

mattbroussard Jun 24, 2022 •

edited

Loading