🐛 Fix / IdP-initiated flow never completes — validate_authresp returns bare :ok

.dialyzer_ignore.exs

@@ -3,5 +3,5 @@
   # Provably unreachable today, kept intentionally so the auth endpoint fails
   # closed (HTTP 403) if a future change introduces a new return shape.
   # See lib/ex_saml/sp_handler.ex for the inline rationale.
-  ~r/lib\/ex_saml\/sp_handler\.ex:95:.*pattern_match_cov/
+  ~r/lib\/ex_saml\/sp_handler\.ex:108:.*pattern_match_cov/
 ]

lib/ex_saml/sp_handler.ex

@@ -57,8 +57,21 @@ defmodule ExSaml.SPHandler do
   @doc """
   Processes the IdP sign-in response and extracts the SAML assertion.
 
-  Returns `{:ok, %{assertion: assertion, nonce: nonce, user_token: token, redirect_uri: uri}}`
-  on success, or `{:error, reason}` on failure.
+  On success returns
+  `{:ok, %{flow: flow, assertion: assertion, nonce: nonce, user_token: token, redirect_uri: uri}}`
+  where:
+
+    * `flow` is `:idp_initiated` or `:sp_initiated` and reflects which SAML flow
+      produced the response (deduced from the assertion's `SubjectConfirmationData`
+      `InResponseTo` — empty means IdP-initiated).
+    * `nonce` is the AuthnRequest-bound SAML nonce for SP-initiated flows, and
+      `nil` for IdP-initiated flows (no AuthnRequest exists in that case, so no
+      nonce is generated; downstream consumers must accept `nil` for the
+      IdP-initiated case).
+
+  On failure returns `{:error, reason}`. Possible reasons include
+  `:idp_initiated_not_allowed`, `:invalid_target_url`, `:invalid_relay_state`,
+  `:invalid_idp_id`, and `:access_denied`.
   """
   # Router-facing clause: matches when the SP router dispatched here with
   # `idp_id` in path params. Performs the full SAML flow AND handles the
@@ -112,17 +125,17 @@ defmodule ExSaml.SPHandler do
     redirect_uri = RelayStateCache.get(relay_state)[:redirect_uri]
 
     with {:ok, assertion} <- Helper.decode_idp_auth_resp(sp, saml_encoding, saml_response),
-         {:ok, nonce} <- validate_authresp(conn, idp_data, assertion, relay_state) do
+         {:ok, flow, nonce} <- validate_authresp(conn, idp_data, assertion, relay_state) do
       {:ok,
        %{
+         flow: flow,
          assertion: %Assertion{assertion | idp_id: idp_id},
          nonce: nonce,
          user_token: user_token,
          redirect_uri: redirect_uri
        }}
     else
       {:error, error} -> {:error, error}
-      _ -> {:error, :access_denied}
     end
   end
 
@@ -169,22 +182,24 @@ defmodule ExSaml.SPHandler do
     get_session(conn, "target_url") || RelayStateCache.get(relay_state)[:target_url] || "/"
   end
 
-  # IDP-initiated flow auth response
-  defp validate_authresp(_conn, idp_data, %{subject: %{in_response_to: ""}}, relay_state) do
+  # IdP-initiated flow auth response. Tagged as `:idp_initiated` with a `nil`
+  # nonce since there is no AuthnRequest to bind a nonce to in this flow.
+  @doc false
+  def validate_authresp(_conn, idp_data, %{subject: %{in_response_to: ""}}, relay_state) do
     cond do
       !idp_data.allow_idp_initiated_flow ->
-        {:error, :idp_first_flow_not_allowed}
+        {:error, :idp_initiated_not_allowed}
 
       idp_data.allowed_target_urls && relay_state not in idp_data.allowed_target_urls ->
         {:error, :invalid_target_url}
 
       true ->
-        :ok
+        {:ok, :idp_initiated, nil}
     end
   end
 
-  # SP-initiated flow auth response
-  defp validate_authresp(conn, %IdpData{id: idp_id}, _assertion, relay_state) do
+  # SP-initiated flow auth response.
+  def validate_authresp(conn, %IdpData{id: idp_id}, _assertion, relay_state) do
     rs_in_session =
       get_session(conn, "relay_state") || RelayStateCache.get(relay_state)[:relay_state]
 
@@ -193,8 +208,6 @@ defmodule ExSaml.SPHandler do
     saml_nonce_in_session =
       get_session(conn, "saml_nonce") || RelayStateCache.get(relay_state)[:saml_nonce]
 
-    # RelayStateCache.delete(relay_state)
-
     cond do
       rs_in_session == nil || rs_in_session != relay_state ->
         {:error, :invalid_relay_state}
@@ -203,7 +216,7 @@ defmodule ExSaml.SPHandler do
         {:error, :invalid_idp_id}
 
       true ->
-        {:ok, saml_nonce_in_session}
+        {:ok, :sp_initiated, saml_nonce_in_session}
     end
   end
 

test/ex_saml/sp_handler_test.exs

@@ -1,13 +1,14 @@
 defmodule ExSaml.SPHandlerTest do
   use ExUnit.Case, async: false
-  import Plug.Test
+
   import Plug.Conn
+  import Plug.Test
 
-  alias ExSaml.SPHandler
+  alias ExSaml.{IdpData, SPHandler, Subject}
 
   # Minimal in-process stub for the cache configured via
   # `config :ex_saml, cache: …`. The real cache is a Nebulex backend, but for
-  # unit-testing `target_url/2` we only need `get/1`. The returned value is
+  # unit-testing the `SPHandler` we only need `get/1`. The returned value is
   # read from the process dictionary so each test can drive its own scenario.
   defmodule StubRelayCache do
     def get(_key), do: Process.get(:stub_relay_cache_value)
@@ -55,6 +56,39 @@ defmodule ExSaml.SPHandlerTest do
     {:ok, conn: conn}
   end
 
+  # Builds a fresh conn with the given session payload populated. Used by tests
+  # that need to drive `validate_authresp/4`'s SP-initiated branch from
+  # session-stored values (relay_state, idp_id, saml_nonce). Independent of the
+  # default `:conn` provided by `setup` because each test needs its own session.
+  defp build_conn(session) do
+    secret = String.duplicate("a", 64)
+
+    opts =
+      Plug.Session.init(
+        store: :cookie,
+        key: "_test_session",
+        signing_salt: "salt",
+        encryption_salt: "esalt"
+      )
+
+    conn =
+      :get
+      |> conn("/")
+      |> init_test_session(session)
+      |> Map.put(:secret_key_base, secret)
+      |> Plug.Session.call(opts)
+      |> fetch_session()
+
+    Enum.reduce(session, conn, fn {k, v}, acc -> put_session(acc, k, v) end)
+  end
+
+  defp idp_initiated_assertion, do: %{subject: %Subject{in_response_to: ""}}
+  defp sp_initiated_assertion, do: %{subject: %Subject{in_response_to: "request-id-42"}}
+
+  # ---------------------------------------------------------------------------
+  # target_url/2
+  # ---------------------------------------------------------------------------
+
   describe "target_url/2" do
     test "returns the session target_url when set", %{conn: conn} do
       Process.put(:stub_relay_cache_value, %{target_url: "/from-cache"})
@@ -86,4 +120,102 @@ defmodule ExSaml.SPHandlerTest do
       assert SPHandler.target_url(conn, "rls") == "/sign-in"
     end
   end
+
+  # ---------------------------------------------------------------------------
+  # validate_authresp/4 — IdP-initiated
+  # ---------------------------------------------------------------------------
+
+  describe "validate_authresp/4 — IdP-initiated" do
+    test "returns {:ok, :idp_initiated, nil} on success — regression test for #24" do
+      idp = %IdpData{allow_idp_initiated_flow: true, allowed_target_urls: nil}
+
+      assert {:ok, :idp_initiated, nil} =
+               SPHandler.validate_authresp(build_conn(%{}), idp, idp_initiated_assertion(), "")
+    end
+
+    test "succeeds when relay_state is one of allowed_target_urls" do
+      idp = %IdpData{
+        allow_idp_initiated_flow: true,
+        allowed_target_urls: ["https://app.example.com/dashboard"]
+      }
+
+      assert {:ok, :idp_initiated, nil} =
+               SPHandler.validate_authresp(
+                 build_conn(%{}),
+                 idp,
+                 idp_initiated_assertion(),
+                 "https://app.example.com/dashboard"
+               )
+    end
+
+    test "returns :idp_initiated_not_allowed when allow_idp_initiated_flow is false" do
+      idp = %IdpData{allow_idp_initiated_flow: false}
+
+      assert {:error, :idp_initiated_not_allowed} =
+               SPHandler.validate_authresp(build_conn(%{}), idp, idp_initiated_assertion(), "")
+    end
+
+    test "returns :invalid_target_url when relay_state not in allowed_target_urls" do
+      idp = %IdpData{
+        allow_idp_initiated_flow: true,
+        allowed_target_urls: ["https://app.example.com/dashboard"]
+      }
+
+      assert {:error, :invalid_target_url} =
+               SPHandler.validate_authresp(
+                 build_conn(%{}),
+                 idp,
+                 idp_initiated_assertion(),
+                 "https://elsewhere.example.com/"
+               )
+    end
+  end
+
+  # ---------------------------------------------------------------------------
+  # validate_authresp/4 — SP-initiated
+  # ---------------------------------------------------------------------------
+
+  describe "validate_authresp/4 — SP-initiated" do
+    test "returns {:ok, :sp_initiated, nonce} on success" do
+      idp = %IdpData{id: "idp-1"}
+
+      conn =
+        build_conn(%{
+          "relay_state" => "rs-abc",
+          "idp_id" => "idp-1",
+          "saml_nonce" => "nonce-xyz"
+        })
+
+      assert {:ok, :sp_initiated, "nonce-xyz"} =
+               SPHandler.validate_authresp(conn, idp, sp_initiated_assertion(), "rs-abc")
+    end
+
+    test "returns :invalid_relay_state when session relay_state does not match" do
+      idp = %IdpData{id: "idp-1"}
+
+      conn =
+        build_conn(%{
+          "relay_state" => "rs-different",
+          "idp_id" => "idp-1",
+          "saml_nonce" => "nonce-xyz"
+        })
+
+      assert {:error, :invalid_relay_state} =
+               SPHandler.validate_authresp(conn, idp, sp_initiated_assertion(), "rs-abc")
+    end
+
+    test "returns :invalid_idp_id when session idp_id does not match" do
+      idp = %IdpData{id: "idp-1"}
+
+      conn =
+        build_conn(%{
+          "relay_state" => "rs-abc",
+          "idp_id" => "idp-other",
+          "saml_nonce" => "nonce-xyz"
+        })
+
+      assert {:error, :invalid_idp_id} =
+               SPHandler.validate_authresp(conn, idp, sp_initiated_assertion(), "rs-abc")
+    end
+  end
 end