From 9efdfa10ff1492fbb738be31d67a0932217390ee Mon Sep 17 00:00:00 2001 From: Jamil Date: Fri, 20 Jan 2023 10:00:10 -0800 Subject: [PATCH] Fix SAML restart and remove test env code path (#1350) This codepath was being skipped in the `test` env, which is no longer necessary. This caused a runtime error that failed to show up in tests because the codepath was being skipped. Refs #1341 --- apps/fz_http/lib/fz_http/configurations.ex | 4 +-- .../configuration/saml_identity_provider.ex | 11 +++++++ apps/fz_http/lib/fz_http/saml/start_proxy.ex | 32 +++++-------------- .../live/setting_live/saml_form_component.ex | 8 +---- .../fz_http_web/acceptance/admin_test.exs | 1 + .../live/setting_live/security_test.exs | 2 +- docker-compose.yml | 1 + 7 files changed, 25 insertions(+), 34 deletions(-) diff --git a/apps/fz_http/lib/fz_http/configurations.ex b/apps/fz_http/lib/fz_http/configurations.ex index d5e847828..d3cd79717 100644 --- a/apps/fz_http/lib/fz_http/configurations.ex +++ b/apps/fz_http/lib/fz_http/configurations.ex @@ -40,7 +40,7 @@ defmodule FzHttp.Configurations do |> Configuration.changeset(%{key => val}) |> Repo.update!() - FzHttp.SAML.StartProxy.restart() + FzHttp.SAML.StartProxy.refresh(configuration.saml_identity_providers) configuration end @@ -69,7 +69,7 @@ defmodule FzHttp.Configurations do def update_configuration(%Configuration{} = config \\ get_configuration!(), attrs) do case Repo.update(Configuration.changeset(config, attrs)) do {:ok, configuration} -> - FzHttp.SAML.StartProxy.restart() + FzHttp.SAML.StartProxy.refresh(configuration.saml_identity_providers) {:ok, configuration} diff --git a/apps/fz_http/lib/fz_http/configurations/configuration/saml_identity_provider.ex b/apps/fz_http/lib/fz_http/configurations/configuration/saml_identity_provider.ex index 5fab12e57..6af14b2a0 100644 --- a/apps/fz_http/lib/fz_http/configurations/configuration/saml_identity_provider.ex +++ b/apps/fz_http/lib/fz_http/configurations/configuration/saml_identity_provider.ex @@ -31,12 +31,14 @@ defmodule FzHttp.Configurations.Configuration.SAMLIdentityProvider do :signed_envelopes_in_resp, :auto_create_users ]) + |> gen_default_base_url() |> validate_required([ :id, :label, :metadata, :auto_create_users ]) + |> FzHttp.Validator.validate_uri(:base_url) |> validate_metadata() end @@ -52,4 +54,13 @@ defmodule FzHttp.Configurations.Configuration.SAMLIdentityProvider do end end) end + + defp gen_default_base_url(changeset) do + default_base_url = + FzHttp.Config.fetch_env!(:fz_http, :external_url) + |> Path.join("/auth/saml") + + base_url = get_change(changeset, :base_url, default_base_url) + put_change(changeset, :base_url, base_url) + end end diff --git a/apps/fz_http/lib/fz_http/saml/start_proxy.ex b/apps/fz_http/lib/fz_http/saml/start_proxy.ex index 10f1ae92e..5c8388cc8 100644 --- a/apps/fz_http/lib/fz_http/saml/start_proxy.ex +++ b/apps/fz_http/lib/fz_http/saml/start_proxy.ex @@ -12,13 +12,9 @@ defmodule FzHttp.SAML.StartProxy do end def start_link(_) do - providers = FzHttp.Configurations.get!(:saml_identity_providers) samly = Samly.Provider.start_link() - FzHttp.Config.fetch_env!(:samly, Samly.Provider) - |> set_service_provider() - |> set_identity_providers(providers) - |> refresh() + refresh() samly end @@ -40,18 +36,14 @@ defmodule FzHttp.SAML.StartProxy do end def set_identity_providers(samly_configs, providers) do - external_url = FzHttp.Config.fetch_env!(:fz_http, :external_url) - identity_providers = providers |> Enum.map(fn provider -> - # XXX We should not set default values here, instead they should be part - # of the changeset and always valid in database %{ id: provider.id, sp_id: "firezone", metadata: provider.metadata, - base_url: provider.base_url || Path.join(external_url, "/auth/saml"), + base_url: provider.base_url, sign_requests: provider.sign_requests, sign_metadata: provider.sign_metadata, signed_assertion_in_resp: provider.signed_assertion_in_resp, @@ -62,21 +54,13 @@ defmodule FzHttp.SAML.StartProxy do Keyword.put(samly_configs, :identity_providers, identity_providers) end - def refresh(samly_configs) do + def refresh(providers \\ FzHttp.Configurations.get!(:saml_identity_providers)) do + samly_configs = + FzHttp.Config.fetch_env!(:samly, Samly.Provider) + |> set_service_provider() + |> set_identity_providers(providers) + FzHttp.Config.put_env(:samly, Samly.Provider, samly_configs) Samly.Provider.refresh_providers() end - - # XXX: This should be removed when the configurations singleton record is removed. - # - # Needed to prevent the test suite from recursively restarting this module as - # it put!()'s mock data - if Mix.env() == :test do - def restart, do: :ignore - else - def restart do - :ok = Supervisor.terminate_child(FzHttp.Supervisor, __MODULE__) - Supervisor.restart_child(FzHttp.Supervisor, __MODULE__) - end - end end diff --git a/apps/fz_http/lib/fz_http_web/live/setting_live/saml_form_component.ex b/apps/fz_http/lib/fz_http_web/live/setting_live/saml_form_component.ex index 6f4178d66..7dab855e1 100644 --- a/apps/fz_http/lib/fz_http_web/live/setting_live/saml_form_component.ex +++ b/apps/fz_http/lib/fz_http_web/live/setting_live/saml_form_component.ex @@ -190,16 +190,10 @@ defmodule FzHttpWeb.SettingLive.SAMLFormComponent do end def update(assigns, socket) do - external_url = FzHttp.Config.fetch_env!(:fz_http, :external_url) - changeset = assigns.providers |> Map.get(assigns.provider_id, %{}) - |> Map.merge(%{ - id: assigns.provider_id, - # XXX this should be part of changeset itself - base_url: Path.join(external_url, "/auth/saml") - }) + |> Map.merge(%{id: assigns.provider_id}) |> FzHttp.Configurations.Configuration.SAMLIdentityProvider.changeset() {:ok, diff --git a/apps/fz_http/test/fz_http_web/acceptance/admin_test.exs b/apps/fz_http/test/fz_http_web/acceptance/admin_test.exs index 39dc0864e..1570f7708 100644 --- a/apps/fz_http/test/fz_http_web/acceptance/admin_test.exs +++ b/apps/fz_http/test/fz_http_web/acceptance/admin_test.exs @@ -478,6 +478,7 @@ defmodule FzHttpWeb.Acceptance.AdminTest do |> assert_el(Query.text("Updated successfully.")) |> assert_el(Query.text("foo-bar-buz")) |> assert_el(Query.text("Sneaky ID")) + |> assert_el(Query.text("http://localhost:4002/autX/saml#foo")) assert [saml_identity_provider] = FzHttp.Configurations.get!(:saml_identity_providers) diff --git a/apps/fz_http/test/fz_http_web/live/setting_live/security_test.exs b/apps/fz_http/test/fz_http_web/live/setting_live/security_test.exs index 18460297e..39142add1 100644 --- a/apps/fz_http/test/fz_http_web/live/setting_live/security_test.exs +++ b/apps/fz_http/test/fz_http_web/live/setting_live/security_test.exs @@ -286,7 +286,7 @@ defmodule FzHttpWeb.SettingLive.SecurityTest do assert FzHttp.Configurations.get!(:saml_identity_providers) == [ %FzHttp.Configurations.Configuration.SAMLIdentityProvider{ auto_create_users: true, - base_url: nil, + base_url: "#{FzHttp.Config.fetch_env!(:fz_http, :external_url)}/auth/saml", id: attrs["id"], label: attrs["label"], metadata: attrs["metadata"], diff --git a/docker-compose.yml b/docker-compose.yml index 9547e0e22..6988e1227 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -35,6 +35,7 @@ services: ports: - 51820:51820/udp environment: + EXTERNAL_URL: ${EXTERNAL_URL:-https://localhost} LOCAL_AUTH_ENABLED: 'true' FZ_WALL_CLI_MODULE: FzWall.CLI.Live cap_add: