Authenticate endpoints and validate user state_code in metrics requests #546

terryttsai · 2022-05-04T00:37:44Z

Description of the change

Authenticate endpoints when auth is enabled and validate that user state_code matches the requested tenantId in metrics requests.

This ensures that users with a state_code in one state cannot request data from another state.

Users with a state_code of recidiviz can request data from any state.

Type of change

Bug fix (non-breaking change that fixes an issue)
New feature (non-breaking change that adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Configuration change (adjusts configuration to achieve some end related to functionality, development, performance, or security)

Related issues

Closes #540

Checklists

Development

These boxes should be checked by the submitter prior to merging:

Manual testing against realistic data has been performed locally

Code review

These boxes should be checked by reviewers prior to merging:

This pull request has a descriptive title and information useful to a reviewer
This pull request has been moved out of a Draft state, has no "Work In Progress" label, and has assigned reviewers
Potential security implications or infrastructural changes have been considered, if relevant

…ed, don't verify if auth is disabled

terryttsai · 2022-05-04T00:47:52Z

spotlight-api/README.md

@@ -44,6 +44,8 @@ Expected backend environment variables include:

 - `GOOGLE_APPLICATION_CREDENTIALS` - a relative path pointing to the JSON file containing the credentials of the service account used to communicate with Google Cloud Storage, for metric retrieval.
 - `METRIC_BUCKET` - the name of the Google Cloud Storage bucket where the metrics reside.
+- `AUTH_ENABLED` - whether or not we should require authentication to access our endpoints. Currently only used in staging to make the entire site private. No need to enable this locally unless you are developing or testing something auth-related. If set to `true` then `AUTH0_APP_METADATA_KEY` **must** be set to a supported value.


This means we'll need to change the staging environment to make sure these keys are set

Yea good call, do you know where/how to do this? I'm actually not sure, I forget where the Spotlight backend is hosted. I see something both in Firebase and also GCP.

the backend is in App Engine, there are GAE config files in a shared 1password document that we have to update and then store locally (untracked) in order to deploy.

@colincadams do you know how to set the staging environment variables?

@terryttsai @macfarlandian so is it sufficient to update gae-staging.yaml in public-dashboard-untracked-config-files.zip in 1pass? How do those get pulled in during deploy?

@terryttsai you don't have access to 1pass, right? So I can do this for you

I don't think so, I don't see that file in 1pass

that zip file is the "master copy" but then you have to download it and copy those files into their respective locations in your working directory. (The deployed backend does not actually use the .env file, which is a little confusing, it uses the environment config in the yaml for that environment. The .env is just for development.)

I see. Are spotlight deploys just done manually by whomever wants to then? By running gcloud app deploy gae-staging.yaml --project [staging_project_id]? Or is there a more formal process?

nope that's as formal as it gets

Cool, ok this all makes sense, thanks!

lilidworkin

Looking good to me, thanks for doing the backend component of this too @terryttsai just to be super safe!

lilidworkin · 2022-05-04T16:38:15Z

spotlight-api/README.md

@@ -44,6 +44,8 @@ Expected backend environment variables include:

 - `GOOGLE_APPLICATION_CREDENTIALS` - a relative path pointing to the JSON file containing the credentials of the service account used to communicate with Google Cloud Storage, for metric retrieval.
 - `METRIC_BUCKET` - the name of the Google Cloud Storage bucket where the metrics reside.
+- `AUTH_ENABLED` - whether or not we should require authentication to access our endpoints. Currently only used in staging to make the entire site private. No need to enable this locally unless you are developing or testing something auth-related. If set to `true` then `AUTH0_APP_METADATA_KEY` **must** be set to a supported value.


Yea good call, do you know where/how to do this? I'm actually not sure, I forget where the Spotlight backend is hosted. I see something both in Firebase and also GCP.

lilidworkin · 2022-05-04T16:40:13Z

spotlight-api/routes/api.js

+    stateCode !== tenantId.toLowerCase() &&
+    stateCode !== "recidiviz"
+  ) {
+    res.status(401).json({


I wonder if we should throw different error messages in these different cases:

AUTH0_APP_METADATA_KEY is empty

req.user?.[AUTH0_APP_METADATA_KEY] is empty

req.user?.[AUTH0_APP_METADATA_KEY]?.state_code is empty

req.user?.[AUTH0_APP_METADATA_KEY]?.state_code is set, but not equal to the tenant ID

Might be helpful for debugging?

Maybe I can assert that AUTH0_APP_METADATA_KEY is defined and we'll see the error in the logs if it's not? If any of the rest is empty we know the user app_metadata hasn't been configured properly. We could have different logs though I would want to return the same 401 error to the client.

Yep sounds good, whatever you think is best! Just thought I'd point it out

macfarlandian · 2022-05-04T17:54:07Z

spotlight-api/README.md

@@ -44,6 +44,8 @@ Expected backend environment variables include:

 - `GOOGLE_APPLICATION_CREDENTIALS` - a relative path pointing to the JSON file containing the credentials of the service account used to communicate with Google Cloud Storage, for metric retrieval.
 - `METRIC_BUCKET` - the name of the Google Cloud Storage bucket where the metrics reside.
+- `AUTH_ENABLED` - whether or not we should require authentication to access our endpoints. Currently only used in staging to make the entire site private. No need to enable this locally unless you are developing or testing something auth-related. If set to `true` then `AUTH0_APP_METADATA_KEY` **must** be set to a supported value.


the backend is in App Engine, there are GAE config files in a shared 1password document that we have to update and then store locally (untracked) in order to deploy.

spotlight-api/.env.example

macfarlandian

tested this locally and it looks great! just had a few minor notes

spotlight-api/routes/api.test.js

coveralls · 2022-05-04T22:44:51Z

Pull Request Test Coverage Report for Build 2272685546

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

7 of 7 (100.0%) changed or added relevant lines in 3 files are covered.
No unchanged relevant lines lost coverage.
Overall first build on terry/auth-endpoint-by-user-state-code at 84.797%

Totals
Change from base Build 2247875950:	84.8%
Covered Lines:	2008
Relevant Lines:	2248

💛 - Coveralls

macfarlandian · 2022-05-04T22:46:39Z

spotlight-client/src/metricsApi/fetchMetrics.ts

@@ -30,6 +30,7 @@ type ErrorAPIResponse = { error: string };
 type FetchMetricOptions = {
  metricNames: string[];
  tenantId: TenantId;
+  getToken: () => Promise<string>;


what was the issue here, importing the datastore into this module was causing some kind of dependency resolution problem? I wonder if it might be better solved by moving the API methods into a datastore class rather than drilling this function through all of our content models? (I don't know off the top of my head if that would cause different problems but it seems like it might be cleaner, if it works)

Yeah I was going to ask here, if it would be easy to move the API methods into one of our stores and call it without having to pass it down all these content models?

Because I was getting nasty dependency cycles with trying to import the Datastore in fetchmetrics

I think you should give it a try! IIRC I kept them separate because at the time they didn't really have any dependencies, but there isn't any structural reason why they have to be set up that way

terryttsai added 5 commits May 3, 2022 15:01

Guard metric api behind auth

16f3133

verify matching state code when retrieving metrics when auth is enabl…

03dcea1

…ed, don't verify if auth is disabled

add tests

d739491

fix tests

643f214

fix lint

f6027dd

terryttsai commented May 4, 2022

View reviewed changes

terryttsai requested review from jovergaag, macfarlandian, colincadams and lilidworkin May 4, 2022 00:50

lilidworkin reviewed May 4, 2022

View reviewed changes

macfarlandian reviewed May 4, 2022

View reviewed changes

spotlight-api/routes/api.test.js Outdated Show resolved Hide resolved

remove unneeded env variable and move to constants

823b079

macfarlandian reviewed May 4, 2022

View reviewed changes

Pass in rootStore to fetchMetrics in order to get token in fetch.

552814e

terryttsai force-pushed the terry/auth-endpoint-by-user-state-code branch from efffd4f to 552814e Compare May 5, 2022 00:01

macfarlandian approved these changes May 5, 2022

View reviewed changes

terryttsai merged commit e571ade into main May 5, 2022

terryttsai deleted the terry/auth-endpoint-by-user-state-code branch May 5, 2022 17:18

terryttsai mentioned this pull request May 5, 2022

Update gae.yaml.example with AUTH_ENABLED #563

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Authenticate endpoints and validate user state_code in metrics requests #546

Authenticate endpoints and validate user state_code in metrics requests #546

terryttsai commented May 4, 2022 •

edited by macfarlandian

Loading

terryttsai May 4, 2022

lilidworkin May 4, 2022

macfarlandian May 4, 2022

terryttsai May 4, 2022

lilidworkin May 4, 2022

terryttsai May 4, 2022

macfarlandian May 4, 2022

lilidworkin May 4, 2022

macfarlandian May 4, 2022

lilidworkin May 4, 2022

lilidworkin left a comment

lilidworkin May 4, 2022

lilidworkin May 4, 2022

terryttsai May 4, 2022

lilidworkin May 4, 2022

macfarlandian May 4, 2022

macfarlandian left a comment

coveralls commented May 4, 2022 •

edited

Loading

macfarlandian May 4, 2022

terryttsai May 4, 2022

terryttsai May 4, 2022

macfarlandian May 4, 2022

Authenticate endpoints and validate user state_code in metrics requests #546

Authenticate endpoints and validate user state_code in metrics requests #546

Conversation

terryttsai commented May 4, 2022 • edited by macfarlandian Loading

Description of the change

Type of change

Related issues

Checklists

Development

Code review

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilidworkin 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

macfarlandian left a comment

Choose a reason for hiding this comment

coveralls commented May 4, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2272685546

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

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

terryttsai commented May 4, 2022 •

edited by macfarlandian

Loading

coveralls commented May 4, 2022 •

edited

Loading