Add Auth0 authentication to all routes #83
Conversation
for more information, see https://pre-commit.ci
…api into issue/2-auth0
for more information, see https://pre-commit.ci
Codecov Report
@@ Coverage Diff @@
## main #83 +/- ##
==========================================
- Coverage 95.66% 95.36% -0.31%
==========================================
Files 17 18 +1
Lines 508 539 +31
==========================================
+ Hits 486 514 +28
- Misses 22 25 +3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
requirements.txt
Outdated
| geopandas | ||
| aiofiles | ||
| pytest-cov | ||
| fastapi-auth0 |
There was a problem hiding this comment.
Hm, I'm not a big fan of introducing this dependency: fastapi-auth0.
- maintained by someone not affiliated with Auth0
- last release in 12/'21 (>4 months ago)
- current stable version is
0.3.0which seems very early - 96 stars on GitHub is very low
Seeing as authentication is critical, paired with the points above, I'd be keen not to introduce this dependency into our production app.
I'd suggest that instead we follow the official Auth0 guide here. What do you think?
There was a problem hiding this comment.
ok- I can give that ago, perhaps Ill do on a different branch incase its too hard
There was a problem hiding this comment.
The guide there only does Bearing token ID, and doesnt do 0Auth with google login. I find the Google log in partiruclar useful.
it was acknowledged by Auth0 though - https://community.auth0.com/t/using-auth0-with-fastapi/58764
i'm happy to pin the version to 0.3.0, so there are not security leaks in the future.
I can also copy out the code from that repo - its only 200 lines. This seems the wrong way to do it though
src/gsp.py
Outdated
| @router.get( | ||
| "/forecast/one_gsp/{gsp_id}", | ||
| response_model=Forecast, | ||
| dependencies=[Depends(get_auth_implicit_scheme())], |
There was a problem hiding this comment.
By explicitly requiring auth on every endpoint it's very easy for us to create an endpoint where we forget about this requirement. If possible, I'd prefer to implement the auth as a middleware. We need to require auth on every single endpoint anyways, so if we implement it as a middleware that would be satisfied.
There was a problem hiding this comment.
I take your point, we may miss one.
Ill have a go at putting as middleware,
if we wanted to leave some things public, thenI dont think we should do this. But I think its fine putting everything private
There was a problem hiding this comment.
Althought actually we might want to have one end point not authenticed, so we can easily do a quick check to see if they api is up and running
src/gsp.py
Outdated
| """ | ||
|
|
||
| logger.info(f"Get truth values for gsp id {gsp_id} and regime {regime}") | ||
| logger.info(f"Get truth values for gsp id {gsp_id} and regime {regime} for {user}") |
There was a problem hiding this comment.
What is user here?
We don't want to be logging PII (Personally Identifiable Information), so maybe, if this user includes a full name and/or email, we could be logging the user id instead?
There was a problem hiding this comment.
I'm not sure, perhaps once reworked, ill have a look.
I was thinking it would print out the 'full name'/ 'component name', so we have a little bit of an idea who is calling the api in the logs
src/gsp.py
Outdated
| """ | ||
|
|
||
| logger.info("Getting all GSP boundaries") | ||
| logger.info(f"Getting all GSP boundaries for {user}") |
There was a problem hiding this comment.
same as above and more below
src/main.py
Outdated
|
|
||
| app.include_router(gsp_router, prefix=f"{v0_route}/gsp") | ||
| # app.include_router(pv_router, prefix=f"{v0_route}/pv") | ||
| app.include_router(pv_router, prefix=f"{v0_route}/pv") |
There was a problem hiding this comment.
As discussed with @JackKelly on Slack previously: We should never be exposing this data, no matter if it's behind authentication or not. We're not the data owners and neither do we have the right to publish it, according to our license agreement.
While the authentication is nice, as it won't make the data publicly available, we are not allowed to redistribute this raw data to our customers in any way.
src/pv.py
Outdated
| @@ -1,42 +1,50 @@ | |||
| """ Expose PV data """ | |||
There was a problem hiding this comment.
see comment above, this should be deleted
Co-authored-by: Flo <6052785+flowirtz@users.noreply.github.com>
Co-authored-by: Flo <6052785+flowirtz@users.noreply.github.com>
for more information, see https://pre-commit.ci
…api into issue/2-auth0
for more information, see https://pre-commit.ci
…auth0 # Conflicts: # src/gsp.py # src/main.py # src/tests/test_gsp.py
for more information, see https://pre-commit.ci
…api into issue/2-auth0
Issue/auth0 speed up docker
for more information, see https://pre-commit.ci
…api into issue/2-auth0 # Conflicts: # script/fake_data.py
for more information, see https://pre-commit.ci
… & format with black
Pull Request
Description
TODO need to update nowcasting APP to get bearer token
Fixes #2 and #130
How Has This Been Tested?
unittests
tested locally
Yes
If your changes affect data processing, have you plotted any changes? i.e. have you done a quick sanity check?
Checklist: