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,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
 

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)