openclimatefix / pv-site-api

Site specific API for PV forecasting
5 stars 8 forks source link

Add minimal authentication to the API #82

Closed simlmx closed 1 year ago

simlmx commented 1 year ago

Best reviewed commit by commit

The main purpose of this pull request is to add basic route protection. I only protect /sites* but we can trivially protect more routes as we wish. All that authentication logic is in the 3rd commit of this pull request.

To make my life easier, I did quite a big refactoring of the way we define the app. This is in the first commit. The big rationale behind the changes is to remove global variables. Python API frameworks have a tendency to use global variables everywhere (app, db, etc.) and it makes testing harder because we end up with a bunch of variables that are defined at import-time, and not at run-time, and so that are harder to change in tests. To summarize, instead of having:

app = FastAPI()

app.get('/route')
def route():
    pass

now we have:

def _add_routes(app):
    app.get('/route')
    def route():
        pass

def create_app():
    app = FastAPI()

    _add_routes(app)

    return app    

In the second commit, I organize all the environment variable into a single Config object. This way we can define this object once and then inject it where needed, like in the create_app function.


You can view those two first commits as a Request For Comments. If y'all don't like them I can always revert them: they were less critical than I thought for the specific case of authentication, but I still think that they make the code clearer and better organized. Let me know what you think!

codecov[bot] commented 1 year ago

Codecov Report

Merging #82 (a54df37) into main (c505c55) will decrease coverage by 4.02%. The diff coverage is 84.79%.

@@            Coverage Diff             @@
##             main      #82      +/-   ##
==========================================
- Coverage   93.80%   89.78%   -4.02%     
==========================================
  Files           9       12       +3     
  Lines         371      421      +50     
==========================================
+ Hits          348      378      +30     
- Misses         23       43      +20     
Impacted Files Coverage Δ
pv_site_api/main.py 0.00% <0.00%> (-92.46%) :arrow_down:
pv_site_api/session.py 0.00% <0.00%> (-37.50%) :arrow_down:
pv_site_api/auth.py 90.47% <90.47%> (ø)
pv_site_api/app.py 90.85% <90.85%> (ø)
pv_site_api/__init__.py 100.00% <100.00%> (ø)
pv_site_api/config.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

simlmx commented 1 year ago

Reopened in #83 without the dependency injection fixes. Those can always be fixed later.