From 83f104e3ef789a66b8ec015125b8db25a8c3c833 Mon Sep 17 00:00:00 2001
From: Eddie Maldonado <emaldonado@mbta.com>
Date: Wed, 21 Feb 2024 16:15:38 -0500
Subject: [PATCH] fix: store logout URL in user session, not guardian claims
 (#2408)

* fix: store logout URL in user session, not guardian claims

* fixup! fix: store logout URL in user session, not guardian claims
---
 lib/skate_web/controllers/auth_controller.ex  | 23 +++++++++-------
 .../controllers/auth_controller_test.exs      | 27 +++++++++++++++++++
 2 files changed, 40 insertions(+), 10 deletions(-)

diff --git a/lib/skate_web/controllers/auth_controller.ex b/lib/skate_web/controllers/auth_controller.ex
index b38aff0a8..c38b43ce8 100644
--- a/lib/skate_web/controllers/auth_controller.ex
+++ b/lib/skate_web/controllers/auth_controller.ex
@@ -23,9 +23,10 @@ defmodule SkateWeb.AuthController do
       |> Guardian.Plug.sign_in(
         AuthManager,
         %{id: user_id},
-        %{groups: groups, sign_out_url: sign_out_url(auth)},
+        %{groups: groups},
         ttl: {1, :hour}
       )
+      |> put_session(:sign_out_url, sign_out_url(auth))
       |> redirect(to: ~p"/")
     else
       send_resp(conn, :forbidden, "forbidden")
@@ -90,20 +91,22 @@ defmodule SkateWeb.AuthController do
 
   @spec logout(Plug.Conn.t(), map()) :: Plug.Conn.t()
   def logout(conn, %{"provider" => "keycloak"}) do
-    case Guardian.Plug.current_claims(conn) do
-      %{"sign_out_url" => sign_out_url} when not is_nil(sign_out_url) ->
-        conn
-        |> session_cleanup()
-        |> redirect(external: sign_out_url)
+    claims = Guardian.Plug.current_claims(conn)
 
+    sign_out_url = get_session(conn, :sign_out_url) || Map.get(claims, "sign_out_url")
+
+    if is_nil(sign_out_url) do
       # The router makes sure we can't call `/auth/:provider/callback`
       # unless we have a session.
       # So the potential `nil` from `current_claims` and the potential map with
       # `sign_out_url=nil` can be handled the same
-      _ ->
-        conn
-        |> session_cleanup()
-        |> redirect(to: "/")
+      conn
+      |> session_cleanup()
+      |> redirect(to: "/")
+    else
+      conn
+      |> session_cleanup()
+      |> redirect(external: sign_out_url)
     end
   end
 
diff --git a/test/skate_web/controllers/auth_controller_test.exs b/test/skate_web/controllers/auth_controller_test.exs
index 9045853f4..b22d47661 100644
--- a/test/skate_web/controllers/auth_controller_test.exs
+++ b/test/skate_web/controllers/auth_controller_test.exs
@@ -3,6 +3,8 @@ defmodule SkateWeb.AuthControllerTest do
 
   import Test.Support.Helpers
 
+  import Skate.Factory
+
   alias Skate.Settings.User
 
   describe "GET /auth/:provider" do
@@ -129,6 +131,31 @@ defmodule SkateWeb.AuthControllerTest do
       assert redirected_to(conn) == redirect_url
     end
 
+    test "falls back to `sign_out_url` from Guardian claims", %{conn: conn} do
+      redirect_url = "redirect.url.localhost"
+
+      user = insert(:user)
+
+      resource = %{id: user.id}
+
+      reassign_env(:skate, :logout_url_fn, fn _, _ ->
+        {:ok, redirect_url}
+      end)
+
+      conn =
+        conn
+        |> init_test_session(%{})
+        |> Guardian.Plug.sign_in(SkateWeb.AuthManager, resource, %{sign_out_url: redirect_url})
+
+      assert Guardian.Plug.authenticated?(conn)
+
+      conn = get(conn, ~p"/auth/keycloak/logout")
+
+      refute Guardian.Plug.authenticated?(conn)
+
+      assert redirected_to(conn) == redirect_url
+    end
+
     @tag :authenticated
     test "clears guardian session", %{conn: conn} do
       assert Guardian.Plug.authenticated?(conn)