Fix logout view #1236
Fix logout view #1236
Conversation
| @@ -37,25 +36,30 @@ def _keycloak_logout_url(self, user): | |||
| qs = urlencode( | |||
| { | |||
| "id_token_hint": id_token, | |||
| "post_logout_redirect_uri": settings.LOGOUT_REDIRECT_URL, | |||
| "post_logout_redirect_uri": self.request.build_absolute_uri( | |||
There was a problem hiding this comment.
I can see this causing issues on a live server since self.request.build_absolute_uri will end up generating a different url depending on if its hit from api.* (which it might if done via js) or the main hostname. I think we need to explicitly generate this from the APP_BASE_URL since redirecting back to api.mitopen.odl.mit.edu leaves the user in a broken state - some functionality does not work as intended from api.*
There was a problem hiding this comment.
The API should only ever be served from api.* domains. I updated the docstring to reflect the redirect path that the browser will take as the user is logged out.
shanbady
added
Waiting on author
and removed
Needs Review
rhysyngsun
force-pushed
the
nl/fix-logout
branch
from
4865935 to
bee9599
Compare
There was a problem hiding this comment.
this should work using the /app route so not making this a blocker but i'll note my main concerns with using self.request.build_absolute_uri:
- it wont work when directly accessing from /logout (has to be done via js) - seems like we are unnecessarily restricting ourselves and sometimes i do go directly to /logout instead of clicking the button.
- Its not clear to me what the /app endpoints supports and seems like we are necessarily making extra roundtrips: mitopen -> api.mitopen/logout ->
- if we need to all of a sudden support redirects elsewhere or back to where the user previously this has the potential to get messy (we'd have to keep track of and pass some "next_url" through 3 request hops)
* Fix logout redirect url * Fix typos and update docstring to be more descriptive
What are the relevant tickets?
Fixes https://github.com/mitodl/hq/issues/4831
Description (What does it do?)
NOTE: I tried to add some tests for this, but they fail because we're using logout via a GET request, which is deprecated and removed in django 5+. I tried to fix that so we could just be done with logout for a good while but that spiraled into CSRF errors and it's not worth that level of effort at this time.