Skip to content
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

Update middleware.py to not set oidc_login_next for AJAX 403 redirects #364

Closed
wants to merge 8 commits into from

Conversation

drwonky
Copy link

@drwonky drwonky commented Jul 31, 2020

Moved the oidc_login_next to below the AJAX return, this means AJAX requests will be redirected to the LOGIN_REDIRECT_URL instead of the API endpoint that generated the session refresh.

Current behavior is to redirect AJAX 403 responses to the API endpoint, this results in the API endpoint response being displayed in the browser main window.

As noted, redirects with XHR requests are extremely finicky, in most cases a simple 302 redirect results in a CORS denial. So the only way to get a session refresh is to do a window.location.href redirect, the 403 becomes a "simple" 302 redirect (using CORS terminology for simple vs XHR requests).

Moved the oidc_login_next to below the AJAX return, this means AJAX requests will be redirected to the LOGIN_REDIRECT_URL instead of the API endpoint that generated the session refresh.
@codecov-commenter
Copy link

codecov-commenter commented Jul 31, 2020

Codecov Report

Merging #364 into master will decrease coverage by 0.17%.
The diff coverage is 75.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #364      +/-   ##
==========================================
- Coverage   89.47%   89.30%   -0.18%     
==========================================
  Files           7        7              
  Lines         456      458       +2     
==========================================
+ Hits          408      409       +1     
- Misses         48       49       +1     
Impacted Files Coverage Δ
mozilla_django_oidc/middleware.py 91.37% <75.00%> (-1.48%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 56773c1...9673822. Read the comment docs.

@akatsoulas
Copy link
Collaborator

@davidjb thanks for this PR. Could you please update the commit message to something more descriptive?

@davidjb
Copy link
Contributor

davidjb commented Jul 31, 2020

@akatsoulas It's not my PR, but I'd definitely appreciate a review of mine over at #350 😄

@akatsoulas
Copy link
Collaborator

Yep you are right :) Autocomplete mix up. I will have a look at the PR

@akatsoulas
Copy link
Collaborator

@drwonky Could you update the commit message to something more descriptive?

@drwonky drwonky changed the title Update middleware.py Update middleware.py to not set oidc_login_next for AJAX 403 redirects Jul 31, 2020
drwonky and others added 2 commits July 31, 2020 12:47
Moved the oidc_login_next to below the AJAX return, this means AJAX requests will be redirected to the LOGIN_REDIRECT_URL instead of the API endpoint that generated the session refresh.
@drwonky
Copy link
Author

drwonky commented Jul 31, 2020

Hi, I amended the commit and changed the message, please let me know if it is acceptable. Sorry for the commit spam, but I made the original change online through github, so I had to clone it offline and amend it there.

drwonky added 3 commits July 31, 2020 16:19
Added a special case that allows AJAX queries to refresh the session
on demand.  AJAX queries simply need to add the
header 'X-Refresh-OIDC-Token' to the request and it will reset the
session expiration so it generates a reauth redirect.
This allows SPAs and other Javascript driven applications
to proactively control session refresh.
Added a special case that allows AJAX queries to refresh the session
on demand.  AJAX queries simply need to add the
header 'X-Refresh-OIDC-Token' to the request and it will reset the
session expiration so it generates a reauth redirect.
This allows SPAs and other Javascript driven applications
to proactively control session refresh.
@drwonky
Copy link
Author

drwonky commented Jul 31, 2020

Hi, sorry about the way this pull request is going, it's a bit of a mess because I'm getting used to the way Github does things.

If you want to cherry pick my 403 commit, that's fine.

My latest commit is a really simple hack to the middleware that allows AJAX clients to proactively refresh session tokens.

The client merely needs to add the header X-Refresh-OIDC-Token to the request and it will cause the middleware to reset the oidc_id_token_expiration to now, which will trigger the token reauth logic.

This allows SPAs and Javascript based applications to control when they will be interrupted for a session refresh, so the control flow won't be surrendered to an inopportune redirect.

Some might see this as a hack, and it is, but it's also an elegant and simple solution to a forced session refresh. Now you don't have to force a logout to refresh your token. The middleware does what it is supposed to, but now the behavior can be influenced by the client.

The value of the header is inconsequential, merely the presence of the header is required.

If you want a use case, let's say when you authenticate for the first time, the browser knows how long the session will last, then it periodically checks the remaining time and informs the user the session token will timeout at a specific time. The app can then allow the user to continue working or perform a session token refresh then, avoiding interruption.

As with the 403 patch, this is no different, you get redirected to the LOGIN_REDIRECT_URL.

@drwonky
Copy link
Author

drwonky commented Aug 4, 2020

Hi there, since this PR seems to have stalled, would you like me to close this and redo the edits as branches and open 2 new pull requests? I don't know if you want my second pull request, since that implementation might be not be desired in your grand design.

@akatsoulas
Copy link
Collaborator

@drwonky yes that would be the ideal scenario. Let's get two PRs and we can discuss the second fix separately from the oidc_login_next. Thank you for this patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants