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.

Sign up for GitHub

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

Fix logout view #1236

Merged
merged 2 commits into from
Jul 10, 2024
+19 −7
Merged

Fix logout view #1236

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7bcb78e
Fix logout redirect url
rhysyngsun Jul 9, 2024
bee9599
Fix typos and update docstring to be more descriptive
rhysyngsun Jul 10, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 19 additions & 7 deletions authentication/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@

from django.conf import settings
from django.contrib.auth import views
from django.http import Http404
from django.shortcuts import redirect
from social_django.utils import load_strategy

Expand Down Expand Up @@ -37,25 +36,38 @@ 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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.*

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

settings.LOGOUT_REDIRECT_URL
),
}
)
return f"{settings.KEYCLOAK_BASE_URL}/realms/{settings.KEYCLOAK_REALM_NAME}/protocol/openid-connect/logout?{qs}" # noqa: E501

return (
f"{settings.KEYCLOAK_BASE_URL}/realms/"
f"{settings.KEYCLOAK_REALM_NAME}/protocol/openid-connect/logout"
f"?{qs}"
)

def get(
self,
request,
*args, # noqa: ARG002
**kwargs, # noqa: ARG002
): # pylint:disable=unused-argument
rhysyngsun marked this conversation as resolved.
Show resolved Hide resolved
):
"""
GET endpoint for loggin a user out.
Raises 404 if the user is not included in the request.

The logout redirect path the user follows is:

- api.example.com/logout (this view)
- keycloak.example.com/realms/REALM/protocol/openid-connect/logout
- api.example.com/app (see main/urls.py)
- app.example.com

"""
user = getattr(request, "user", None)
if user and user.is_authenticated:
super().get(request)
return redirect(self._keycloak_logout_url(user))
else:
msg = "Not currently logged in."
raise Http404(msg)
return redirect("/app")
Loading