From 98c4ff5487e97d246fd128a708f5dce92d91302c Mon Sep 17 00:00:00 2001 From: Laura Beatris <48022589+LauraBeatris@users.noreply.github.com> Date: Sun, 24 Mar 2024 18:47:23 -0300 Subject: [PATCH] Refactor SAML structs with separate groups of attributes --- lib/saml.ex | 4 +- lib/saml/request/request.ex | 30 +++-- lib/saml/response/response.ex | 105 +++++++++++++++++- test/saml/request_test.exs | 26 ++--- test/saml/response_test.exs | 12 +- .../saml/saml_request_missing_element.xml | 9 ++ 6 files changed, 147 insertions(+), 39 deletions(-) create mode 100644 test/support/saml/saml_request_missing_element.xml diff --git a/lib/saml.ex b/lib/saml.ex index 3c0ec99..7aca3e6 100644 --- a/lib/saml.ex +++ b/lib/saml.ex @@ -32,7 +32,7 @@ defmodule ShinAuth.SAML do if valid_xml?(saml_request) do case DataSchema.to_struct(saml_request, Request) do {:ok, parsed_saml_request} -> {:ok, parsed_saml_request} - {:error, _} = error -> error + _ -> {:error, error} end else {:error, error} @@ -63,7 +63,7 @@ defmodule ShinAuth.SAML do if valid_xml?(saml_response) do case DataSchema.to_struct(saml_response, Response) do {:ok, parsed_saml_response} -> {:ok, parsed_saml_response} - {:error, _} = error -> error + _ -> {:error, error} end else {:error, error} diff --git a/lib/saml/request/request.ex b/lib/saml/request/request.ex index e9623b8..41d1dc6 100644 --- a/lib/saml/request/request.ex +++ b/lib/saml/request/request.ex @@ -7,6 +7,20 @@ defmodule ShinAuth.SAML.Request do alias ShinAuth.SAML.Request.Utils + @type t :: + {:common, ShinAuth.SAML.Request.Common.t()} + + @data_accessor ShinAuth.SAML.XMLHandler + data_schema(has_one: {:common, "/samlp:AuthnRequest", ShinAuth.SAML.Request.Common}) +end + +defmodule ShinAuth.SAML.Request.Common do + @moduledoc false + + import DataSchema, only: [data_schema: 1] + + alias ShinAuth.SAML.Request.Utils + @type t :: {:id, String.t()} | {:version, String.t()} @@ -16,19 +30,13 @@ defmodule ShinAuth.SAML.Request do @data_accessor ShinAuth.SAML.XMLHandler data_schema( - field: {:id, "/samlp:AuthnRequest/@ID", &{:ok, Utils.maybe_to_string(&1)}, optional: false}, + field: {:id, "./@ID", &{:ok, Utils.maybe_to_string(&1)}, optional: false}, + field: {:version, "./@Version", &{:ok, Utils.maybe_to_string(&1)}, optional: false}, field: - {:version, "/samlp:AuthnRequest/@Version", &{:ok, Utils.maybe_to_string(&1)}, - optional: false}, - field: - {:assertion_consumer_service_url, "/samlp:AuthnRequest/@AssertionConsumerServiceURL", + {:assertion_consumer_service_url, "./@AssertionConsumerServiceURL", &{:ok, Utils.maybe_to_string(&1)}, optional: false}, - field: - {:issuer, "/samlp:AuthnRequest/saml:Issuer/text()", &{:ok, Utils.maybe_to_string(&1)}, - optional: false}, - field: - {:issue_instant, "/samlp:AuthnRequest/@IssueInstant", &{:ok, Utils.maybe_to_string(&1)}, - optional: false} + field: {:issuer, "./saml:Issuer/text()", &{:ok, Utils.maybe_to_string(&1)}, optional: false}, + field: {:issue_instant, "./@IssueInstant", &{:ok, Utils.maybe_to_string(&1)}, optional: false} ) end diff --git a/lib/saml/response/response.ex b/lib/saml/response/response.ex index 88de5a3..57c6d9a 100644 --- a/lib/saml/response/response.ex +++ b/lib/saml/response/response.ex @@ -3,31 +3,124 @@ defmodule ShinAuth.SAML.Response do Parsed XML struct of a SAML response """ + @type t :: + {:common, ShinAuth.SAML.Response.Common.t()} + | {:conditions, ShinAuth.SAML.Response.Conditions.t()} + + import DataSchema, only: [data_schema: 1] + + alias ShinAuth.SAML.Response.Utils + + @data_accessor ShinAuth.SAML.XMLHandler + data_schema( + has_one: {:common, "/saml2p:Response", ShinAuth.SAML.Response.Common}, + has_one: {:status, "/saml2p:Response/saml2p:Status", ShinAuth.SAML.Response.Status}, + has_one: + {:conditions, "/saml2p:Response/saml2:Assertion/saml2:Conditions", + ShinAuth.SAML.Response.Conditions}, + has_many: + {:attributes, "/saml2p:Response/saml2:Assertion/saml2:AttributeStatement/saml2:Attribute", + ShinAuth.SAML.Response.Attribute} + ) +end + +defmodule ShinAuth.SAML.Response.Common do + @moduledoc false + + @type t :: + {:id, String.t()} + | {:version, String.t()} + | {:destination, String.t()} + | {:issuer, String.t()} + | {:issue_instant, String.t()} + import DataSchema, only: [data_schema: 1] alias ShinAuth.SAML.Response.Utils @data_accessor ShinAuth.SAML.XMLHandler data_schema( - field: {:id, "/saml2p:Response/@ID", &{:ok, Utils.maybe_to_string(&1)}, optional: false}, + field: {:id, "./@ID", &{:ok, Utils.maybe_to_string(&1)}, optional: false}, + field: {:version, "./@Version", &{:ok, Utils.maybe_to_string(&1)}, optional: false}, + field: {:destination, "./@Destination", &{:ok, Utils.maybe_to_string(&1)}, optional: false}, field: - {:version, "/saml2p:Response/@Version", &{:ok, Utils.maybe_to_string(&1)}, optional: false}, + {:issuer, "./saml2p:Issuer/text()", &{:ok, Utils.maybe_to_string(&1)}, optional: false}, + field: {:issue_instant, "./@IssueInstant", &{:ok, Utils.maybe_to_string(&1)}, optional: false} + ) +end + +defmodule ShinAuth.SAML.Response.Conditions do + @moduledoc false + + @type t :: + {:not_before, String.t()} + | {:not_on_or_after, String.t()} + + import DataSchema, only: [data_schema: 1] + + alias ShinAuth.SAML.Response.Utils + + @data_accessor ShinAuth.SAML.XMLHandler + data_schema( + field: {:not_before, "./@NotBefore", &{:ok, Utils.maybe_to_string(&1)}, optional: false}, field: - {:destination, "/saml2p:Response/@Destination", &{:ok, Utils.maybe_to_string(&1)}, - optional: false}, + {:not_on_or_after, "./@NotOnOrAfter", &{:ok, Utils.maybe_to_string(&1)}, optional: false} + ) +end + +defmodule ShinAuth.SAML.Response.Status do + @moduledoc false + + @type t :: + {:status, :failure, :successful} + | {:status_code, String.t()} + + import DataSchema, only: [data_schema: 1] + + alias ShinAuth.SAML.Response.Utils + + @data_accessor ShinAuth.SAML.XMLHandler + data_schema( field: - {:issuer, "/saml2p:Response/saml2p:Issuer/text()", &{:ok, Utils.maybe_to_string(&1)}, + {:status, "./saml2p:StatusCode/@Value", &{:ok, Utils.map_status_code_value(&1)}, optional: false}, field: - {:issue_instant, "/saml2p:Response/@IssueInstant", &{:ok, Utils.maybe_to_string(&1)}, + {:status_code, "./saml2p:StatusCode/@Value", &{:ok, Utils.maybe_to_string(&1)}, optional: false} ) end +defmodule ShinAuth.SAML.Response.Attribute do + @moduledoc false + + @type t :: + {:name, String.t()} + | {:value, String.t()} + + import DataSchema, only: [data_schema: 1] + + alias ShinAuth.SAML.Response.Utils + + @data_accessor ShinAuth.SAML.XMLHandler + data_schema( + field: {:name, "./@Name", &{:ok, Utils.maybe_to_string(&1)}, optional: false}, + field: + {:value, "./saml2:AttributeValue/text()", &{:ok, to_string(&1) |> String.trim()}, + optional: true} + ) +end + defmodule ShinAuth.SAML.Response.Utils do @moduledoc false def maybe_to_string(""), do: nil def maybe_to_string(nil), do: nil def maybe_to_string(value), do: to_string(value) |> String.trim() + + def map_status_code_value(value) do + case value do + "urn:oasis:names:tc:SAML:2.0:status:Success" -> :success + _ -> :failure + end + end end diff --git a/test/saml/request_test.exs b/test/saml/request_test.exs index 678d444..d434f41 100644 --- a/test/saml/request_test.exs +++ b/test/saml/request_test.exs @@ -30,17 +30,11 @@ defmodule ShinAuth.SAML.RequestTest do context "when missing required element" do it "returns error" do - saml_request_mock = """ - - - - - """ - {:error, - %DataSchema.Errors{ - errors: [issuer: "Field was required but value supplied is considered empty"] - }} = SAML.decode_saml_request(saml_request_mock) + %Request.Error{ + tag: :malformed_saml_request, + message: "Verify if the SAML request is structured correctly by the Service Provider." + }} = SAML.decode_saml_request(get_xml("saml_request_missing_element")) end end end @@ -49,11 +43,13 @@ defmodule ShinAuth.SAML.RequestTest do it "returns parsed request struct" do {:ok, %Request{ - id: "_123", - version: "2.0", - issue_instant: "2023-09-27T17:20:42.746Z", - issuer: "https://example.com/123", - assertion_consumer_service_url: "https://auth.example.com/sso/saml/acs/123" + common: %{ + id: "_123", + version: "2.0", + issue_instant: "2023-09-27T17:20:42.746Z", + issuer: "https://example.com/123", + assertion_consumer_service_url: "https://auth.example.com/sso/saml/acs/123" + } }} = SAML.decode_saml_request(get_xml("valid_saml_request")) end end diff --git a/test/saml/response_test.exs b/test/saml/response_test.exs index 06f81da..c8eabad 100644 --- a/test/saml/response_test.exs +++ b/test/saml/response_test.exs @@ -46,11 +46,13 @@ defmodule ShinAuth.SAML.ResponseTest do it "returns parsed response struct" do {:ok, %Response{ - id: "_123", - version: "2.0", - destination: "https://api.example.com/sso/saml/acs/123", - issuer: "https://example.com/1234/issuer/1234", - issue_instant: "2024-03-23T20:56:56.768Z" + common: %{ + id: "_123", + version: "2.0", + destination: "https://api.example.com/sso/saml/acs/123", + issuer: "https://example.com/1234/issuer/1234", + issue_instant: "2024-03-23T20:56:56.768Z" + } }} = SAML.decode_saml_response(get_xml("valid_saml_response")) end end diff --git a/test/support/saml/saml_request_missing_element.xml b/test/support/saml/saml_request_missing_element.xml new file mode 100644 index 0000000..92bd236 --- /dev/null +++ b/test/support/saml/saml_request_missing_element.xml @@ -0,0 +1,9 @@ + + + +