-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add optional Auth0 integration #229
Conversation
Pull Request Test Coverage Report for Build 309550846
💛 - Coveralls |
Pull Request Test Coverage Report for Build 311716257
💛 - Coveralls |
If by this you mean that the login form is not hosted at a recidiviz.org subdomain yet and is hosted at a subdomain of auth0.com, then we do want to use a recidiviz.org subdomain for consistency and control.
Can you point me to the library we're using in pulse and the library you're now using here so I can compare?
We do need to do email verification of some kind. Otherwise, random people can sign up for accounts using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a fairly elegant approach! The only thing that's not perfect is that we have to have two separate places where we check if auth is enabled or not (the provider and the wall) but that's not terrible. Well done!
Just a few questions.
spotlight-client/README.md
Outdated
Expected environment variables include: | ||
|
||
- `REACT_APP_AUTH_ENABLED` - set to `true` or `false` to toggle Auth0 protection per environment. 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. | ||
- `REACT_APP_AUTH_ENV` - a string indicating the "auth environment" used to point to the correct Auth0 tenant. Either "development" (which also covers staging) or "production". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this true and relevant? I thought we only had one auth environment for this app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes you are right ... I adapted this from the pulse readme and was a little unsure of how to handle the production case. But after sleeping on it, it seems pretty obvious to me that the docs should just reflect the current state of things (just the one environment) and not worry about being future-proof. I'll update this!
spotlight-client/src/App.test.tsx
Outdated
}; | ||
}); | ||
|
||
test("require auth", async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing test 🚀
@@ -0,0 +1,55 @@ | |||
// Recidiviz - a data platform for criminal justice reform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Was this code copied from Auth0 or was it entirely hand-written? Just curious!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not copied, although the logic is simplistic enough that it probably substantially resembles what's in the docs for this library
@jessex re: the Auth0 libraries: here we are using auth0-react, which as far as I can tell is a React wrapper around @auth0/auth0-spa-js, the more generic JS library we are using in pulse (and covid). It essentially replaces the I am also not in love with the double environment check but I am hoping the tests will be decently effective at catching that; one of both of those top-level As far as the additional Auth0 config is concerned ... I'll keep working on that and update here as needed. |
Fascinating! Yeah it appears that auth0-react is newish and was created after our first uses of Auth0 in our frontend. Would be good to standardize on this not long from now after this app has proven its worth. |
all comments up until this point should be addressed now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work, @macfarlandian!
@macfarlandian This looks great! Great test coverage. As far as standardizing on an auth package, I agree that we should move towards this, especially considering I've been stuck in an IE11 localhost authentication hole for a full day. I'm wondering if the auth0-react package has tidied some of that up? I do a spike to try to install this package on pulse on Monday and see if it clears anything up. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🌮
I had a flash of inspiration this afternoon about reducing the number of magical incantations required to prevent this auth solution from breaking. In the latest commit I pushed:
Tests continued to pass without modification (I only had to change some import paths) so ... that's good at least? I am curious whether you all actually see this as an improvement, though, so I will defer merging until after the weekend to let you weigh in. (The thing that allowed this new approach was the surprising discovery that the auth0 hook does not actually blow up if you call it without a Provider; that feels maybe a little hacky to me but could potentially be improved if this new overall pattern is to our liking) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do think this is an improvement! Centralizing complexity into a single location to be reasoned about, with as few instances of moving parts as possible, is worthwhile.
Description of the change
Adds support for an Auth0 integration that puts the entire site behind an auth requirement. The feature can be toggled on and off with environment variables. This includes the necessary Auth0 configuration to support this in staging, but there is no staging environment that is actually set up yet, so we can harden this as needed without any risk in the meantime.
Matters of note:
pulse-dashboard
of adding Auth0 rule and hook functions to the repo even though they are not actually used by the application. I did keep them out of thesrc
directory in an attempt to mitigate that ambiguity. I don't think any of the rules we have applied elsewhere are really needed here, so there aren't actually any, but I did replicate the domain filtering hook, which in the actual Auth0 settings only includesrecidiviz.org
. (I wasn't sure whether we definitely needed to require email verification; pulse-dashboard uses it but the COVID site does not. Question for @jessex whether that is a blocker here for compliance or other reasons.)Type of change
Related issues
Checklists
Development
These boxes should be checked by the submitter prior to merging:
Code review
These boxes should be checked by reviewers prior to merging: