Don't override SAML config booleans (#1333)

Fixes #1321 

Also updates a couple tests to read from `EXTERNAL_URL` instead of
`http://localhost:4002` (these were failing in my env because I have a
different value set)

Signed-off-by: Jamil <jamilbk@users.noreply.github.com>
Co-authored-by: Andrew Dryga <andrew@dryga.com>

apps/fz_http/lib/fz_http/config.ex

@@ -5,11 +5,11 @@ defmodule FzHttp.Config do
   """
 
   if Mix.env() != :test do
-    def maybe_put_env_override(_key, _value), do: :ok
-    def fetch_env!(app, key), do: Application.fetch_env!(app, key)
+    defdelegate put_env(app, key, value), to: Application
+    defdelegate fetch_env!(app, key), to: Application
   else
-    def maybe_put_env_override(key, value) do
-      _ = Process.put(key, value)
+    def put_env(app \\ :fz_http, key, value) do
+      Process.put(key_function(app, key), value)
       :ok
     end
 
@@ -25,9 +25,11 @@ defmodule FzHttp.Config do
     def fetch_env!(app, key) do
       application_env = Application.fetch_env!(app, key)
 
-      with :error <- fetch_process_value(key),
-           :error <- fetch_process_value(get_last_pid_from_pdict_list(:"$ancestors"), key),
-           :error <- fetch_process_value(get_last_pid_from_pdict_list(:"$callers"), key) do
+      pdict_key = key_function(app, key)
+
+      with :error <- fetch_process_value(pdict_key),
+           :error <- fetch_process_value(get_last_pid_from_pdict_list(:"$ancestors"), pdict_key),
+           :error <- fetch_process_value(get_last_pid_from_pdict_list(:"$callers"), pdict_key) do
         application_env
       else
         {:ok, override} -> override
@@ -66,5 +68,8 @@ defmodule FzHttp.Config do
         List.last(values)
       end
     end
+
+    # String.to_atom/1 is only used in test env
+    defp key_function(app, key), do: String.to_atom("#{app}-#{key}")
   end
 end

apps/fz_http/lib/fz_http/saml/start_proxy.ex

@@ -52,10 +52,10 @@ defmodule FzHttp.SAML.StartProxy do
           sp_id: "firezone",
           metadata: provider.metadata,
           base_url: provider.base_url || Path.join(external_url, "/auth/saml"),
-          sign_requests: provider.sign_requests || true,
-          sign_metadata: provider.sign_metadata || true,
-          signed_assertion_in_resp: provider.signed_assertion_in_resp || true,
-          signed_envelopes_in_resp: provider.signed_envelopes_in_resp || true
+          sign_requests: provider.sign_requests,
+          sign_metadata: provider.sign_metadata,
+          signed_assertion_in_resp: provider.signed_assertion_in_resp,
+          signed_envelopes_in_resp: provider.signed_envelopes_in_resp
         }
       end)
 
@@ -63,7 +63,7 @@ defmodule FzHttp.SAML.StartProxy do
   end
 
   def refresh(samly_configs) do
-    Application.put_env(:samly, Samly.Provider, samly_configs)
+    FzHttp.Config.put_env(:samly, Samly.Provider, samly_configs)
     Samly.Provider.refresh_providers()
   end
 

apps/fz_http/test/fz_http/devices_test.exs

@@ -56,7 +56,7 @@ defmodule FzHttp.DevicesTest do
     }
 
     test "prevents creating more than max_devices_per_user", %{device: device} do
-      FzHttp.Config.maybe_put_env_override(:max_devices_per_user, 1)
+      FzHttp.Config.put_env(:max_devices_per_user, 1)
 
       assert {:error,
               %Ecto.Changeset{
@@ -85,7 +85,7 @@ defmodule FzHttp.DevicesTest do
     test "returns error when device IP can't be assigned due to CIDR pool exhaustion", %{
       device: device
     } do
-      FzHttp.Config.maybe_put_env_override(:wireguard_ipv4_network, "10.3.2.0/30")
+      FzHttp.Config.put_env(:wireguard_ipv4_network, "10.3.2.0/30")
       attrs = %{@device_attrs | ipv4: nil, ipv6: nil, user_id: device.user_id}
 
       assert {:ok, _device} = Devices.create_device(attrs)

apps/fz_http/test/fz_http/rules_test.exs

@@ -4,8 +4,8 @@ defmodule FzHttp.RulesTest do
   alias FzHttp.Rules
 
   setup do
-    FzHttp.Config.maybe_put_env_override(:wireguard_ipv4_network, "100.64.0.0/10")
-    FzHttp.Config.maybe_put_env_override(:wireguard_ipv6_network, "fd00::0/106")
+    FzHttp.Config.put_env(:wireguard_ipv4_network, "100.64.0.0/10")
+    FzHttp.Config.put_env(:wireguard_ipv6_network, "fd00::0/106")
 
     :ok
   end

apps/fz_http/test/fz_http/telemetry_test.exs

@@ -115,7 +115,7 @@ defmodule FzHttp.TelemetryTest do
 
   describe "database" do
     test "local hostname" do
-      FzHttp.Config.maybe_put_env_override(FzHttp.Repo, hostname: "localhost")
+      FzHttp.Config.put_env(FzHttp.Repo, hostname: "localhost")
 
       ping_data = Telemetry.ping_data()
 
@@ -123,7 +123,7 @@ defmodule FzHttp.TelemetryTest do
     end
 
     test "local url" do
-      FzHttp.Config.maybe_put_env_override(FzHttp.Repo, url: "postgres://127.0.0.1")
+      FzHttp.Config.put_env(FzHttp.Repo, url: "postgres://127.0.0.1")
 
       ping_data = Telemetry.ping_data()
 
@@ -131,7 +131,7 @@ defmodule FzHttp.TelemetryTest do
     end
 
     test "external hostname" do
-      FzHttp.Config.maybe_put_env_override(FzHttp.Repo, hostname: "firezone.dev")
+      FzHttp.Config.put_env(FzHttp.Repo, hostname: "firezone.dev")
 
       ping_data = Telemetry.ping_data()
 
@@ -139,7 +139,7 @@ defmodule FzHttp.TelemetryTest do
     end
 
     test "external url" do
-      FzHttp.Config.maybe_put_env_override(FzHttp.Repo, url: "postgres://firezone.dev")
+      FzHttp.Config.put_env(FzHttp.Repo, url: "postgres://firezone.dev")
 
       ping_data = Telemetry.ping_data()
 
@@ -149,7 +149,7 @@ defmodule FzHttp.TelemetryTest do
 
   describe "email" do
     test "outbound set" do
-      FzHttp.Config.maybe_put_env_override(FzHttpWeb.Mailer, from_email: "test@firezone.dev")
+      FzHttp.Config.put_env(FzHttpWeb.Mailer, from_email: "test@firezone.dev")
 
       ping_data = Telemetry.ping_data()
 
@@ -157,7 +157,7 @@ defmodule FzHttp.TelemetryTest do
     end
 
     test "outbound unset" do
-      FzHttp.Config.maybe_put_env_override(FzHttpWeb.Mailer, from_email: nil)
+      FzHttp.Config.put_env(FzHttpWeb.Mailer, from_email: nil)
 
       ping_data = Telemetry.ping_data()
 

apps/fz_http/test/fz_http_web/acceptance/admin_test.exs

@@ -494,6 +494,21 @@ defmodule FzHttpWeb.Acceptance.AdminTest do
                  auto_create_users: true
                }
 
+      assert FzHttp.Config.fetch_env!(:samly, Samly.Provider) == [
+               identity_providers: [
+                 %{
+                   base_url: "http://localhost:4002/autX/saml#foo",
+                   id: "foo-bar-buz",
+                   metadata: saml_metadata,
+                   sign_metadata: false,
+                   sign_requests: false,
+                   signed_assertion_in_resp: false,
+                   signed_envelopes_in_resp: false,
+                   sp_id: "firezone"
+                 }
+               ]
+             ]
+
       # Edit
       session =
         session

apps/fz_http/test/fz_http_web/controllers/auth_controller_test.exs

@@ -295,7 +295,7 @@ defmodule FzHttpWeb.AuthControllerTest do
       query =
         URI.encode_query(%{
           "id_token_hint" => "abc",
-          "post_logout_redirect_uri" => "http://localhost:4002/",
+          "post_logout_redirect_uri" => FzHttp.Config.fetch_env!(:fz_http, :external_url) <> "/",
           "client_id" => "okta-client-id"
         })
 

apps/fz_http/test/fz_http_web/live/setting_live/security_test.exs

@@ -231,8 +231,8 @@ defmodule FzHttpWeb.SettingLive.SecurityTest do
 
       assert %FzHttp.Configurations.Configuration.SAMLIdentityProvider{
                auto_create_users: false,
-               # XXX this field would be nil if we don't "guess" the url when we load the record in StartServer
-               base_url: "http://localhost:4002/auth/saml",
+               # XXX this field would be nil if we don't "guess" the url when we load the record in StartProxy
+               base_url: "#{FzHttp.Config.fetch_env!(:fz_http, :external_url)}/auth/saml",
                id: "FAKEID",
                label: "FOO",
                metadata: attrs["metadata"],

apps/fz_http/test/fz_http_web/plug/path_prefix_test.exs

@@ -11,17 +11,17 @@ defmodule FzHttpWeb.Plug.PathPrefixTest do
 
   describe "call/2" do
     test "does nothing when path prefix is not configured" do
-      FzHttp.Config.maybe_put_env_override(:path_prefix, nil)
+      FzHttp.Config.put_env(:path_prefix, nil)
       conn = conn(:get, "/")
       assert call(conn, []) == conn
 
-      FzHttp.Config.maybe_put_env_override(:path_prefix, "/")
+      FzHttp.Config.put_env(:path_prefix, "/")
       conn = conn(:get, "/foo")
       assert call(conn, []) == conn
     end
 
     test "removes prefix from conn.request_path" do
-      FzHttp.Config.maybe_put_env_override(:path_prefix, "/vpn/")
+      FzHttp.Config.put_env(:path_prefix, "/vpn/")
       conn = conn(:get, "/vpn/foo")
       assert returned_conn = call(conn, [])
       assert returned_conn.request_path == "/foo"
@@ -30,7 +30,7 @@ defmodule FzHttpWeb.Plug.PathPrefixTest do
     end
 
     test "removes prefix from conn.path_info" do
-      FzHttp.Config.maybe_put_env_override(:path_prefix, "/vpn/")
+      FzHttp.Config.put_env(:path_prefix, "/vpn/")
       conn = conn(:get, "/vpn/foo")
       assert returned_conn = call(conn, [])
       assert returned_conn.path_info == ["foo"]
@@ -39,7 +39,7 @@ defmodule FzHttpWeb.Plug.PathPrefixTest do
     end
 
     test "redirects users from not prefixed path" do
-      FzHttp.Config.maybe_put_env_override(:path_prefix, "/vpn/")
+      FzHttp.Config.put_env(:path_prefix, "/vpn/")
 
       conn = conn(:get, "/foo")
       assert returned_conn = call(conn, [])