From d43c1ffe1ae37957af430b20e54227b17358f33e Mon Sep 17 00:00:00 2001 From: Xabier Arbulu Insausti Date: Wed, 25 Sep 2024 13:41:27 +0200 Subject: [PATCH] SAML configurable attr fields (#3011) * Make saml user profile attribute names configurable * Add the configuration in runtime execution * Update tests to avoid concurrency issues --- config/config.exs | 10 ++++++- config/runtime.exs | 8 ++++- lib/trento_web/auth/assent_saml_strategy.ex | 29 +++++++++++++++---- .../auth/assent_saml_strategy_test.exs | 12 ++++++++ .../controllers/session_controller_test.exs | 28 +++++++++++++++--- 5 files changed, 76 insertions(+), 11 deletions(-) diff --git a/config/config.exs b/config/config.exs index e65145eefb..970ea2d443 100644 --- a/config/config.exs +++ b/config/config.exs @@ -88,7 +88,15 @@ config :trento, :pow_assent, user_identities_context: Trento.UserIdentities config :trento, :oidc, enabled: false config :trento, :oauth2, enabled: false -config :trento, :saml, enabled: false + +config :trento, :saml, + enabled: false, + user_profile_attributes: %{ + username_field: "username", + email_field: "email", + first_name_field: "firstName", + last_name_field: "lastName" + } # Agent heartbeat interval. Adding one extra second to the agent 5s interval to avoid glitches config :trento, Trento.Heartbeats, interval: :timer.seconds(6) diff --git a/config/runtime.exs b/config/runtime.exs index c36cf6c6cf..4550dad135 100644 --- a/config/runtime.exs +++ b/config/runtime.exs @@ -259,7 +259,13 @@ if config_env() in [:prod, :demo] do callback_url: "/auth/saml_callback", idp_id: System.get_env("SAML_IDP_ID") || - raise("environment variable SAML_IDP_ID is missing") + raise("environment variable SAML_IDP_ID is missing"), + user_profile_attributes: %{ + username_field: System.get_env("SAML_USERNAME_ATTR_NAME", "username"), + email_field: System.get_env("SAML_EMAIL_ATTR_NAME", "email"), + first_name_field: System.get_env("SAML_FIRSTNAME_ATTR_NAME", "firstName"), + last_name_field: System.get_env("SAML_LASTNAME_ATTR_NAME", "lastName") + } config :trento, :pow_assent, providers: [ diff --git a/lib/trento_web/auth/assent_saml_strategy.ex b/lib/trento_web/auth/assent_saml_strategy.ex index b5421d303a..cfd7cc910b 100644 --- a/lib/trento_web/auth/assent_saml_strategy.ex +++ b/lib/trento_web/auth/assent_saml_strategy.ex @@ -18,7 +18,23 @@ defmodule TrentoWeb.Auth.AssentSamlStrategy do end def callback(_config, %{attributes: attributes}) do - case normalize(attributes) do + %{ + username_field: username_field, + email_field: email_field, + first_name_field: first_name_field, + last_name_field: last_name_field + } = user_profile_attributes() + + expected_attributes = + Map.new(attributes, fn + {^username_field, username} -> {:username, username} + {^email_field, email} -> {:email, email} + {^first_name_field, first_name} -> {:first_name, first_name} + {^last_name_field, last_name} -> {:last_name, last_name} + other -> other + end) + + case normalize(expected_attributes) do {:ok, user} -> {:ok, %{user: user, token: %{}}} @@ -28,10 +44,10 @@ defmodule TrentoWeb.Auth.AssentSamlStrategy do end defp normalize(%{ - "username" => username, - "email" => email, - "firstName" => first_name, - "lastName" => last_name + username: username, + email: email, + first_name: first_name, + last_name: last_name }) do {:ok, %{ @@ -46,4 +62,7 @@ defmodule TrentoWeb.Auth.AssentSamlStrategy do defp normalize(%{}) do {:error, :user_attributes_missing} end + + defp user_profile_attributes, + do: Application.fetch_env!(:trento, :saml)[:user_profile_attributes] end diff --git a/test/trento_web/auth/assent_saml_strategy_test.exs b/test/trento_web/auth/assent_saml_strategy_test.exs index 6713b22e15..ef2c20e044 100644 --- a/test/trento_web/auth/assent_saml_strategy_test.exs +++ b/test/trento_web/auth/assent_saml_strategy_test.exs @@ -13,6 +13,18 @@ defmodule TrentoWeb.Auth.AssentSamlStrategyTest do end describe "callback/2" do + setup do + Application.put_env(:trento, :saml, + enabled: false, + user_profile_attributes: %{ + username_field: "username", + email_field: "email", + first_name_field: "firstName", + last_name_field: "lastName" + } + ) + end + test "should return a normalized user" do username = Faker.Internet.user_name() email = Faker.Internet.email() diff --git a/test/trento_web/controllers/session_controller_test.exs b/test/trento_web/controllers/session_controller_test.exs index fe106c86db..e33fdea511 100644 --- a/test/trento_web/controllers/session_controller_test.exs +++ b/test/trento_web/controllers/session_controller_test.exs @@ -638,6 +638,30 @@ defmodule TrentoWeb.SessionControllerTest do |> json_response(401) |> assert_schema("Unauthorized", api_spec) end + end + + describe "saml_callback endpoint" do + setup %{conn: conn} = context do + Samly.State.init(Samly.State.ETS) + + Application.put_env(:trento, :saml, + enabled: false, + user_profile_attributes: %{ + username_field: "username", + email_field: "email", + first_name_field: "firstName", + last_name_field: "lastName" + } + ) + + conn = + conn + |> Plug.Conn.put_private(:plug_session, %{}) + |> Plug.Conn.put_private(:plug_session_fetch, :done) + |> Pow.Plug.put_config(otp_app: :trento) + + Map.put(context, :conn, conn) + end test "should return the credentials when the saml callback flow is completed without errors and the user does not exist on trento", %{conn: conn, api_spec: api_spec} do @@ -650,8 +674,6 @@ defmodule TrentoWeb.SessionControllerTest do end ) - Samly.State.init(Samly.State.ETS) - username = Faker.Internet.user_name() not_on_or_after = @@ -710,8 +732,6 @@ defmodule TrentoWeb.SessionControllerTest do test "should return unauthorized in saml callback flow when user attributes are missing", %{conn: conn, api_spec: api_spec} do - Samly.State.init(Samly.State.ETS) - not_on_or_after = DateTime.utc_now() |> DateTime.add(8, :hour)