Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
184 changes: 113 additions & 71 deletions README.md

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion lib/samly/auth_handler.ex
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ defmodule Samly.AuthHandler do
)

_ ->
conn |> send_resp(403, "access_denied")
Helper.handle_error_response(conn, :no_active_session, 403, "access_denied")
end

# rescue
Expand Down
21 changes: 21 additions & 0 deletions lib/samly/helper.ex
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,27 @@ defmodule Samly.Helper do
Map.get(idps, idp_id)
end

def handle_error_response(%Plug.Conn{} = conn, error, status, body) do
conn
|> Plug.Conn.put_private(:samly_error, error)
|> run_error_pipeline()
|> case do
%Plug.Conn{halted: true} = conn -> conn
conn -> conn |> Plug.Conn.send_resp(status, body) |> Plug.Conn.halt()
end
end

def run_error_pipeline(%Plug.Conn{} = conn) do
case Application.get_env(:samly, :on_error_pipeline) do
nil ->
conn

pipeline ->
conn = pipeline.call(conn, [])
if conn.state in [:sent, :chunked, :set_chunked], do: %{conn | halted: true}, else: conn
end
end

@spec get_metadata_uri(nil | binary, binary) :: nil | charlist
def get_metadata_uri(nil, _idp_id), do: nil

Expand Down
4 changes: 3 additions & 1 deletion lib/samly/idp_data.ex
Original file line number Diff line number Diff line change
Expand Up @@ -383,7 +383,9 @@ defmodule Samly.IdpData do
idp_signs_assertions: idp_data.signed_assertion_in_resp,
trusted_fingerprints: idp_data.fingerprints,
metadata_uri: Helper.get_metadata_uri(idp_data.base_url, path_segment_idp_id),
consume_uri: idp_data.custom_recipient_url || Helper.get_consume_uri(idp_data.base_url, path_segment_idp_id),
consume_uri:
idp_data.custom_recipient_url ||
Helper.get_consume_uri(idp_data.base_url, path_segment_idp_id),
logout_uri: Helper.get_logout_uri(idp_data.base_url, path_segment_idp_id),
entity_id: sp_entity_id
)
Expand Down
1 change: 1 addition & 0 deletions lib/samly/provider.ex
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ defmodule Samly.Provider do
identity_providers =
Samly.IdpData.load_providers(opts[:identity_providers] || [], service_providers)

Application.put_env(:samly, :on_error_pipeline, opts[:on_error_pipeline])
Application.put_env(:samly, :service_providers, service_providers)
Application.put_env(:samly, :identity_providers, identity_providers)

Expand Down
9 changes: 7 additions & 2 deletions lib/samly/router_util.ex
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ defmodule Samly.RouterUtil do
if idp do
conn |> Conn.put_private(:samly_idp, idp)
else
conn |> Conn.send_resp(403, "invalid_request unknown IdP") |> Conn.halt()
Helper.handle_error_response(conn, :unknown_idp, 403, "invalid_request unknown IdP")
end
end

Expand All @@ -43,7 +43,12 @@ defmodule Samly.RouterUtil do
"[Samly] target_url must be x-www-form-urlencoded: #{inspect(conn.params["target_url"])}"
)

conn |> Conn.send_resp(400, "target_url must be x-www-form-urlencoded") |> Conn.halt()
Helper.handle_error_response(
conn,
:invalid_target_url_encoding,
400,
"target_url must be x-www-form-urlencoded"
)
end
end

Expand Down
56 changes: 46 additions & 10 deletions lib/samly/sp_handler.ex
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,12 @@ defmodule Samly.SPHandler do

def consume_signin_response(conn) do
%IdpData{id: idp_id} = idp = conn.private[:samly_idp]
%IdpData{pre_session_create_pipeline: pipeline, esaml_sp_rec: sp_rec} = idp

%IdpData{
pre_session_create_pipeline: pipeline,
esaml_sp_rec: sp_rec
} = idp

sp = ensure_sp_uris_set(sp_rec, conn)

saml_encoding = conn.body_params["SAMLEncoding"]
Expand All @@ -53,17 +58,42 @@ defmodule Samly.SPHandler do
|> put_session_new("samly_assertion_key", assertion_key)
|> redirect(302, target_url)
else
{:halted, conn} -> conn
{:halted, conn} ->
conn

{:error, reason} ->
case idp do
%IdpData{debug_mode: true} ->
{_, assertion_or_error} = Helper.decode_idp_auth_resp(sp, saml_encoding, saml_response)

conn
|> put_private(:samly_error, reason)
|> put_private(:samly_assertion, assertion_or_error)
|> then(fn conn ->
if idp.debug_mode do
put_private(conn, :samly_saml_response, saml_response)
else
conn
|> put_resp_header("content-type", "text/html")
|> send_resp(403, "<html><body><div><h1>access_denied</h1><p><b>Error:</b><br /><pre><code>#{inspect(reason)}</code></pre></p><p><b>Raw Response:</b><br /><pre><code>#{saml_response}</code></pre></p></div></body></html")
_ ->
conn |> send_resp(403, "access_denied #{inspect(reason)}")
end
end)
|> Helper.run_error_pipeline()
|> case do
%Conn{halted: true} = conn ->
conn

Comment on lines +77 to +81
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This block reruns the on-error pipeline + halted check that Helper.handle_error_response/4 already centralizes, so any future changes to pipeline/response flow require editing the helper and this branch separately; can we delegate the debug-HTML branch to that helper (e.g. by passing a custom responder) instead of copying the pipeline/case logic?

Finding type: Code Dedup and Conventions


  • Apply fix with Baz

conn ->
if idp.debug_mode do
conn
|> put_resp_header("content-type", "text/html")
|> send_resp(
403,
"<html><body><div><h1>access_denied</h1><p><b>Error:</b><br /><pre><code>#{inspect(reason)}</code></pre></p><p><b>Raw Response:</b><br /><pre><code>#{saml_response}</code></pre></p></div></body></html>"
)
else
send_resp(conn, 403, "access_denied")
end
end
_ -> conn |> send_resp(403, "access_denied")

_ ->
Helper.handle_error_response(conn, :access_denied, 403, "access_denied")
end

# rescue
Expand Down Expand Up @@ -151,7 +181,13 @@ defmodule Samly.SPHandler do
|> configure_session(drop: true)
|> redirect(302, target_url)
else
error -> conn |> send_resp(403, "invalid_request #{inspect(error)}")
error ->
Helper.handle_error_response(
conn,
{:invalid_logout_response, error},
403,
"invalid_request #{inspect(error)}"
)
end

# rescue
Expand Down
3 changes: 2 additions & 1 deletion lib/samly/sp_router.ex
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ defmodule Samly.SPRouter do
cond do
conn.params["SAMLResponse"] != nil -> Samly.SPHandler.handle_logout_response(conn)
conn.params["SAMLRequest"] != nil -> Samly.SPHandler.handle_logout_request(conn)
true -> conn |> send_resp(403, "invalid_request")
true ->
Samly.Helper.handle_error_response(conn, :invalid_request, 403, "invalid_request")
end
end

Expand Down