PhilanthropyDataCommons / service

A project for collecting and serving public information associated with grant applications
GNU Affero General Public License v3.0
8 stars 2 forks source link

Decouple authorization and authentication #871

Closed slifty closed 5 months ago

slifty commented 5 months ago

This PR changes authentication so that it is both a universal step AND the act of checking authentication is not in itself an act of authorization. It also adds adds a simple authorization middleware which verifies that the request is associated with a valid authentication object.

Functionally this should not result in any changes to the API (so no impact on version / changelog) -- however it should allow for support for users in future!

Resolves #870 Related to #710

codecov[bot] commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 87.82%. Comparing base (9939440) to head (e7e681f).

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #871 +/- ## ========================================== + Coverage 87.50% 87.82% +0.32% ========================================== Files 104 106 +2 Lines 1424 1445 +21 Branches 175 177 +2 ========================================== + Hits 1246 1269 +23 + Misses 163 161 -2 Partials 15 15 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

slifty commented 5 months ago

Approval (and vacation zone) appreciated but just to clarify for this concern:

If authorization middleware is split from authentication middleware, we could imagine a (bad) situation where authorization middleware says "OK, all good" with a bad token

The only thing populating the auth variable is the jwt middleware, so under this current code base that won't be any more possible than it was before (basically, the jwt middleware is the thing that would be duped by a bad token). If the jwt middleware doesn't run, or doesn't recognize a valid token, then I believe auth wouldn't be populated (I'm actually going to add a test for this, to make sure I'm understanding the jwt middleware properlty).

If the jwt middleware somehow gets moved or not run for an endpoint (right now it is universally run), then auth would not be populated regardless of a good, bad, or nonexistant token being passed.

Does that make sense?

slifty commented 5 months ago

Added better tests! (that was... quite a journey -- fun fact, if you want to test proper jwt parsing you need to make the test an integration test!).

The tests as written were passing, but that's because the jwt stuff was all happening asynchronously / wasn't aligned with the test, and there was no positive test case 😬

Hooray for better tests!