diff --git a/.dialyzer_ignore.exs b/.dialyzer_ignore.exs index fac1bc3..d85540b 100644 --- a/.dialyzer_ignore.exs +++ b/.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/ ] diff --git a/lib/ex_saml/sp_handler.ex b/lib/ex_saml/sp_handler.ex index f6064b5..be223b5 100644 --- a/lib/ex_saml/sp_handler.ex +++ b/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,9 +125,10 @@ 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, @@ -122,7 +136,6 @@ defmodule ExSaml.SPHandler do }} 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 diff --git a/test/ex_saml/sp_handler_test.exs b/test/ex_saml/sp_handler_test.exs index 887aad6..46f1c2f 100644 --- a/test/ex_saml/sp_handler_test.exs +++ b/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