From 0b27a554f17126cb0287302021c569f0da39c9fc Mon Sep 17 00:00:00 2001
From: Arie Peterson <arie@greenhost.nl>
Date: Fri, 13 Oct 2023 12:34:05 +0200
Subject: [PATCH] Fix logout to include redirect to hydra logout URL

---
 backend/web/login/login.py       | 83 +++++++++++++-------------------
 backend/web/templates/clear.html |  4 +-
 2 files changed, 35 insertions(+), 52 deletions(-)

diff --git a/backend/web/login/login.py b/backend/web/login/login.py
index 6947038e..177bdd90 100644
--- a/backend/web/login/login.py
+++ b/backend/web/login/login.py
@@ -541,19 +541,28 @@ def get_kratos_cookie():
     return cookie
 
 
-
-@web.route("/prelogout", methods=["GET"])
-def prelogout():
-    """Handles the Hydra OpenID Connect Logout flow
+@web.route("/logout", methods=["GET"])
+def logout():
+    """Handles the Hydra OpenID Connect Logout flow and Kratos logout flow.
 
     Steps:
     1. Hydra's /oauth2/sessions/logout endpoint is called by an application
     2. Hydra calls this endpoint with a `logout_challenge` get parameter
     3. We retrieve the logout request using the challenge
-    4. We accept the Hydra logout request
-    5. We redirect to Hydra to clean-up cookies.
-    6. Hyrda calls back to us with a post logout handle (/logout)
-
+    4. We accept the Hydra logout request. This returns a URL -- let's call it
+    "next redirect" -- that we should redirect the browser to to finish the
+    Hydra logout.
+    5. We create a Kratos logout flow, setting its `return_to` parameter to the
+    "next redirect". This returns a Kratos logout URL.
+    6. We return a small HTML page to the browser, based on the `clear.html`
+    template, which clears dashboard local storage and redirects to the Kratos
+    logout URL.
+    7. The browser follows that redirect, Kratos does its thing and redirects
+    to the "next redirect".
+    8. The browser follows the "next redirect", Hydra does its thing and
+    redirects to the "post-logout URL". We set that to the dashboard root URL
+    by default, but OIDC clients can override it to something else. For
+    example, Nextcloud sets it to the root Nextcloud URL.
      
     Args:
         logout_challenge (string): Reference to a Hydra logout challenge object
@@ -576,7 +585,7 @@ def prelogout():
         current_app.logger.error(
             "Conflict. Logout request with challenge '%s' has been used already.",
             challenge)
-        abort(503)
+        abort(404, "Logout request has been accepted already.")
 
     current_app.logger.info("Logout request hydra, subject %s", logout_request.subject)
 
@@ -585,20 +594,28 @@ def prelogout():
     # browser flow and we can't do both.
     try:
         hydra_return = hydra_admin_api.accept_logout_request(challenge)
+        next_redirect = hydra_return.redirect_to
     except Exception as ex:
-        current_app.logger.info("Error logging out hydra: %s", str(ex))
+        current_app.logger.info("Error accepting hydra logout request: %s", str(ex))
+        next_redirect = DASHBOARD_URL
 
-    # Now start ending the kratos session.
+    # Now end the kratos session.
     kratos_cookie = get_kratos_cookie()
     if not kratos_cookie:
         # No kratos cookie, already logged out from kratos.
         current_app.logger.info("Expected kratos cookie but not found. Redirecting to hydra post-logout");
-        return redirect(hydra_post_logout)
+        # We skip the Kratos logout, but we still need to follow
+        # `next_redirect` -- probably the Hydra logout URL -- and clear
+        # dashboard storage.
+        return render_template("clear.html",
+            url=next_redirect)
     try:
         # Create a Logout URL for Browsers
-        kratos_api_response = \
-            admin_frontend_api.create_browser_logout_flow(
-                cookie=kratos_cookie)
+        current_app.logger.info(f"Creating logout flow, with return_to={next_redirect}")
+        kratos_api_response = admin_frontend_api.create_browser_logout_flow(
+            return_to=next_redirect,
+            cookie=kratos_cookie)
+        current_app.logger.info("Kratos api response to creating logout flow:")
         current_app.logger.info(kratos_api_response)
         return render_template("clear.html",
             url=kratos_api_response.logout_url)
@@ -606,41 +623,7 @@ def prelogout():
         current_app.logger.error("Exception when calling"
             " create_browser_logout_flow: %s\n",
             ex)
-
-    current_app.logger.info("Hydra logout not completed. Redirecting to kratos logout, maybe user removed cookies manually")
-    return redirect("logout")
-
-
-@web.route("/logout", methods=["GET"])
-def logout():
-    """Handles the Kratos Logout flow
-
-    Steps:
-    1. We got here from hyrda
-    2. We retrieve the Kratos cookie from the browser
-    3. We generate a Kratos logout URL
-    4. We redirect to the Kratos logout URIL
-    """
-
-    kratos_cookie = get_kratos_cookie()
-    if not kratos_cookie:
-        # No kratos cookie, already logged out
-        current_app.logger.info("Expected kratos cookie but not found. Redirecting to login");
-        return render_template("clear.html",
-            url="login")
-
-    try:
-        # Create a Logout URL for Browsers
-        kratos_api_response = \
-            admin_frontend_api.create_browser_logout_flow(
-                cookie=kratos_cookie)
-        current_app.logger.info(kratos_api_response)
-    except ory_kratos_client.ApiException as ex:
-        current_app.logger.error("Exception when calling"
-            " create_self_service_logout_flow_url_for_browsers: %s\n",
-            ex)
-    return render_template("clear.html",
-            url=kratos_api_response.logout_url)
+        return redirect(DASHBOARD_URL)
 
 
 if DEMO_INSTANCE:
diff --git a/backend/web/templates/clear.html b/backend/web/templates/clear.html
index c1a37ddb..e7f06a77 100644
--- a/backend/web/templates/clear.html
+++ b/backend/web/templates/clear.html
@@ -6,9 +6,9 @@
     // Wipe the local storage
     localStorage.removeItem("persist:root");
     // Redirect
-    window.location = '{{ url }}';
+    window.location = '{{ url | safe }}';
 </script>
 
-Redirecting ...
+Clearing session data and redirecting...
 
 {% endblock %}
-- 
GitLab