From 547b3bbf65befa3055c3341293ae55b09ca2c7e5 Mon Sep 17 00:00:00 2001 From: Jamil Date: Wed, 18 Jan 2023 12:44:57 -0800 Subject: [PATCH] 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 Co-authored-by: Andrew Dryga --- apps/fz_http/lib/fz_http/config.ex | 19 ++++++++++++------- apps/fz_http/lib/fz_http/saml/start_proxy.ex | 10 +++++----- apps/fz_http/test/fz_http/devices_test.exs | 4 ++-- apps/fz_http/test/fz_http/rules_test.exs | 4 ++-- apps/fz_http/test/fz_http/telemetry_test.exs | 12 ++++++------ .../fz_http_web/acceptance/admin_test.exs | 15 +++++++++++++++ .../controllers/auth_controller_test.exs | 2 +- .../live/setting_live/security_test.exs | 4 ++-- .../fz_http_web/plug/path_prefix_test.exs | 10 +++++----- 9 files changed, 50 insertions(+), 30 deletions(-) diff --git a/apps/fz_http/lib/fz_http/config.ex b/apps/fz_http/lib/fz_http/config.ex index da29549b1..123b00842 100644 --- a/apps/fz_http/lib/fz_http/config.ex +++ b/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 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 abf93ed41..10f1ae92e 100644 --- a/apps/fz_http/lib/fz_http/saml/start_proxy.ex +++ b/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 diff --git a/apps/fz_http/test/fz_http/devices_test.exs b/apps/fz_http/test/fz_http/devices_test.exs index bbc8939df..d443b3414 100644 --- a/apps/fz_http/test/fz_http/devices_test.exs +++ b/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) diff --git a/apps/fz_http/test/fz_http/rules_test.exs b/apps/fz_http/test/fz_http/rules_test.exs index 056192a83..6c10d49a1 100644 --- a/apps/fz_http/test/fz_http/rules_test.exs +++ b/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 diff --git a/apps/fz_http/test/fz_http/telemetry_test.exs b/apps/fz_http/test/fz_http/telemetry_test.exs index 911e9e559..eae3d1617 100644 --- a/apps/fz_http/test/fz_http/telemetry_test.exs +++ b/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() 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 dbef633bd..39dc0864e 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 @@ -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 diff --git a/apps/fz_http/test/fz_http_web/controllers/auth_controller_test.exs b/apps/fz_http/test/fz_http_web/controllers/auth_controller_test.exs index 72c30bf44..f5425380e 100644 --- a/apps/fz_http/test/fz_http_web/controllers/auth_controller_test.exs +++ b/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" }) 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 27d4d59cb..18460297e 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 @@ -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"], diff --git a/apps/fz_http/test/fz_http_web/plug/path_prefix_test.exs b/apps/fz_http/test/fz_http_web/plug/path_prefix_test.exs index 98afa98a5..3b06eca18 100644 --- a/apps/fz_http/test/fz_http_web/plug/path_prefix_test.exs +++ b/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, [])