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

@@ -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}
 

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

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

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,

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)
 

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"],

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: