Update oidc middleware with two new feature flags that introspect and allow posting#1054
Update oidc middleware with two new feature flags that introspect and allow posting#1054MelissaAutumn wants to merge 4 commits into
Conversation
davinotdavid
left a comment
davinotdavid
left a comment
There was a problem hiding this comment.
Tested locally with the test steps in the PR description and can verify that removing the session in Keycloak and refreshing the page punts the user to the login screen, nice!
However, if I don't refresh the page but continue to try to click around / trigger some requests, they fail the requests instead of punting the user to the login screen. Is that expected?
Not a super realistic scenario but for example if you:
- Add a user to the allow list and go through sign-up / FTUE
- Stay in the ToS page and in another tab / window / browser revoke access
- As the user in ToS page, click on accept or decline and see it failing and not punting the user to login
Repeating the process but in the Paddle checkout page:
- Add a user to the allow list and go through sign-up / FTUE
- Go through ToS and stay in the Paddle checkout page
- In another tab / window / browser revoke access of said user
- As the user in Paddle checkout page, click on the UserAvatar and then Support and then click Thunderbird Pro to go back to the Paddle checkout. See that it fails to load and doesn't punt the user to login
| # POST OIDC's token introspect route to see if a token is actually active. | ||
| WAFFLE_FLAG_INTROSPECT_TOKEN_PER_REQUEST = 'auth-introspect-token-per-request' | ||
|
|
||
| # Allow auth middleware to try and refresh an inactive access token with an active refresh token during non-get requests | ||
| WAFFLE_FLAG_ALLOW_POST_REAUTH = 'auth-allow-post-reauth' |
There was a problem hiding this comment.
I think it would be nice to add a section in README for the flags, also I just realized that we don't have an entry for MFA either (which I believe is multi-factor-authentication)
There was a problem hiding this comment.
Good point, I'll add a section in docs because readme is getting crowded.
| # is_redirectable = request.method == 'GET' | ||
| default_response = HttpResponseForbidden() | ||
| xhr_response_json = {'error': 'the authentication session has expired'} | ||
| if prompt_reauth: | ||
| if prompt_reauth: # and is_redirectable: |
There was a problem hiding this comment.
Is the is_redirectable a left over for debugging here? If so I think we should remove it to reduce confusion
|
Thanks for testing, and yeah we should handle those. Will need to setup useFetch and have a default 302 redirect follower for that. Not too hard, but may end up with a larger change overall. |
MelissaAutumn
force-pushed
the
features/oidc-middleware-should-introspect-token
branch
from
00a22a8 to
d6a9113
Compare
… allow posting * `auth-introspect-token-per-request` forces the middleware to introspect the token every request * `auth-allow-post-reauth` allows the middleware to attempt to reauth on non-get requests
…d json response for json requests.
… 403 auth errors.
MelissaAutumn
force-pushed
the
features/oidc-middleware-should-introspect-token
branch
from
d6a9113 to
8ba5ec1
Compare
|
Okay it turns out there was a bug where the refresh token was never used unless it's invalid lol. I fixed that flipped condition, so now access tokens should silently refresh correctly. I've introduced a useFetch (much like in appointment) and sprinkled it around api calls that require auth. It's called useAuthfulFetch but I'm willing to rename it if anyone has a better suggestion. I wanted to convey that this function is for authed api calls only. If the access token and refresh token are invalid (session invalidation iirc) then the user will be punted to the login screen via the fetch error function. In the future I think we should block page access with a modal and text explaining that they need to sign back in, but that feels like a services-ui pattern and we'll need to borrow some designers I think. |
|
will go through this in the morning |
3 participants
Fixes #380
Fixes #1053
auth-introspect-token-per-requestforces the middleware to introspect the token every requestauth-allow-post-reauthallows the middleware to attempt to reauth on non-get requestsI've updated the middleware acquired from a mozilla-django-oidc PR to allow refreshing tokens on non-GET requests and making the middleware introspect the access/refresh token. These are both gated behind the above feature flags.
To test add the feature flags listed above via django admin panel and set them to true. With a short-lived access token you should hopefully experience a better experience without being bumped to the login screen. Additionally if you invalidate your token (via password reset) it should pick up on it.
An easy way to test this is to remove the active session for the currently logged in user on keycloak's admin panel. (http://keycloak:8999/admin/master/console/#/tbpro/users/). On a refresh that user should be punted to the login screen with a "refresh token is not active" debug log entry listed in the console.
This code is kinda too complex imo, suggestions for streamlining while keeping it secure are welcome. I wanted to let us roll these changes back easily if needed, so that's why they're waffled.