From 4e2a62af71bd1fd9dbb64ab3b2f13286f0342598 Mon Sep 17 00:00:00 2001 From: Andrew Dryga Date: Thu, 26 Jan 2023 14:21:59 -0600 Subject: [PATCH] Fix bugs with OIDC and SAML forms (#1383) 1. When OIDC/SAML is deleted the state of the LV socket becomes invalid and basically makes it looks like everything was deleted 2. When there is more than one OIDC/SAML config trying to delete it leads to a crash 3. Updates of OIDC/SAML were messy and hard to follow, they are reworked and more tests are added around them Closes #1382 --- .../configuration/openid_connect_provider.ex | 8 +- .../configuration/saml_identity_provider.ex | 8 +- .../live/setting_live/oidc_form_component.ex | 76 +++++----- .../live/setting_live/saml_form_component.ex | 74 +++++----- .../live/setting_live/security_live.ex | 18 ++- .../live/setting_live/security_test.exs | 132 +++++++++++------- 6 files changed, 175 insertions(+), 141 deletions(-) diff --git a/apps/fz_http/lib/fz_http/configurations/configuration/openid_connect_provider.ex b/apps/fz_http/lib/fz_http/configurations/configuration/openid_connect_provider.ex index c503e70c7..15da21664 100644 --- a/apps/fz_http/lib/fz_http/configurations/configuration/openid_connect_provider.ex +++ b/apps/fz_http/lib/fz_http/configurations/configuration/openid_connect_provider.ex @@ -26,10 +26,14 @@ defmodule FzHttp.Configurations.Configuration.OpenIDConnectProvider do field :auto_create_users, :boolean end - def changeset(struct \\ %__MODULE__{}, data) do + def create_changeset(attrs) do + changeset(%__MODULE__{}, attrs) + end + + def changeset(struct, attrs) do struct |> cast( - data, + attrs, [ :id, :label, 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 2f3ca5113..a17e7cf6a 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 @@ -24,9 +24,13 @@ defmodule FzHttp.Configurations.Configuration.SAMLIdentityProvider do field :auto_create_users, :boolean end - def changeset(struct \\ %__MODULE__{}, data) do + def create_changeset(attrs) do + changeset(%__MODULE__{}, attrs) + end + + def changeset(struct, attrs) do struct - |> cast(data, [ + |> cast(attrs, [ :id, :label, :base_url, diff --git a/apps/fz_http/lib/fz_http_web/live/setting_live/oidc_form_component.ex b/apps/fz_http/lib/fz_http_web/live/setting_live/oidc_form_component.ex index 4dc9880b7..cb24d5af0 100644 --- a/apps/fz_http/lib/fz_http_web/live/setting_live/oidc_form_component.ex +++ b/apps/fz_http/lib/fz_http_web/live/setting_live/oidc_form_component.ex @@ -3,6 +3,7 @@ defmodule FzHttpWeb.SettingLive.OIDCFormComponent do Form for OIDC configs """ use FzHttpWeb, :live_component + alias FzHttp.Configurations def render(assigns) do ~H""" @@ -174,55 +175,50 @@ defmodule FzHttpWeb.SettingLive.OIDCFormComponent do assigns.providers |> Map.get(assigns.provider_id, %{}) |> Map.put(:id, assigns.provider_id) - |> FzHttp.Configurations.Configuration.OpenIDConnectProvider.changeset() + |> Configurations.Configuration.OpenIDConnectProvider.create_changeset() - {:ok, - socket - |> assign(assigns) - |> assign(:external_url, FzHttp.Config.fetch_env!(:fz_http, :external_url)) - |> assign(:changeset, changeset)} + socket = + socket + |> assign(assigns) + |> assign(:external_url, FzHttp.Config.fetch_env!(:fz_http, :external_url)) + |> assign(:changeset, changeset) + + {:ok, socket} end def handle_event("save", %{"open_id_connect_provider" => params}, socket) do - changeset = - params - |> FzHttp.Configurations.Configuration.OpenIDConnectProvider.changeset() - |> render_changeset_errors() + create_changeset = Configurations.Configuration.OpenIDConnectProvider.create_changeset(params) - update = - case changeset do - %{valid?: true} -> - {:ok, _} = - changeset - |> Ecto.Changeset.apply_changes() - |> Map.from_struct() - |> Map.new(fn {k, v} -> {to_string(k), v} end) - |> then(fn data -> - id = Map.get(data, "id") + if create_changeset.valid? do + new_attrs = + create_changeset + |> Ecto.Changeset.apply_changes() + |> Map.from_struct() - %{ - openid_connect_providers: - socket.assigns.providers - |> Map.delete(socket.assigns.provider_id) - |> Map.put(id, data) - |> Map.values() - } - end) - |> FzHttp.Configurations.update_configuration() + new_attrs = + socket.assigns.providers + |> Map.get(socket.assigns.provider_id, %{}) + |> Map.merge(new_attrs) - _ -> - {:error, changeset} - end + providers = + socket.assigns.providers + |> Map.delete(socket.assigns.provider_id) + |> Map.values() - case update do - {:ok, _config} -> - {:noreply, - socket - |> put_flash(:info, "Updated successfully.") - |> redirect(to: socket.assigns.return_to)} + {:ok, _changeset} = + FzHttp.Configurations.update_configuration(%{ + openid_connect_providers: providers ++ [new_attrs] + }) - {:error, changeset} -> - {:noreply, assign(socket, :changeset, changeset)} + socket = + socket + |> put_flash(:info, "Updated successfully.") + |> redirect(to: socket.assigns.return_to) + + {:noreply, socket} + else + socket = assign(socket, :changeset, render_changeset_errors(create_changeset)) + {:noreply, socket} 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 8bfeb06e6..748237407 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 @@ -3,6 +3,7 @@ defmodule FzHttpWeb.SettingLive.SAMLFormComponent do Form for SAML configs """ use FzHttpWeb, :live_component + alias FzHttp.Configurations def render(assigns) do ~H""" @@ -196,54 +197,49 @@ defmodule FzHttpWeb.SettingLive.SAMLFormComponent do assigns.providers |> Map.get(assigns.provider_id, %{}) |> Map.merge(%{id: assigns.provider_id}) - |> FzHttp.Configurations.Configuration.SAMLIdentityProvider.changeset() + |> Configurations.Configuration.SAMLIdentityProvider.create_changeset() - {:ok, - socket - |> assign(assigns) - |> assign(:changeset, changeset)} + socket = + socket + |> assign(assigns) + |> assign(:changeset, changeset) + + {:ok, socket} end def handle_event("save", %{"saml_identity_provider" => params}, socket) do - changeset = - params - |> FzHttp.Configurations.Configuration.SAMLIdentityProvider.changeset() - |> render_changeset_errors() + create_changeset = Configurations.Configuration.SAMLIdentityProvider.create_changeset(params) - update = - case changeset do - %{valid?: true} -> - {:ok, _} = - changeset - |> Ecto.Changeset.apply_changes() - |> Map.from_struct() - |> Map.new(fn {k, v} -> {to_string(k), v} end) - |> then(fn data -> - id = Map.get(data, "id") + if create_changeset.valid? do + new_attrs = + create_changeset + |> Ecto.Changeset.apply_changes() + |> Map.from_struct() - %{ - saml_identity_providers: - socket.assigns.providers - |> Map.delete(socket.assigns.provider_id) - |> Map.put(id, data) - |> Map.values() - } - end) - |> FzHttp.Configurations.update_configuration() + new_attrs = + socket.assigns.providers + |> Map.get(socket.assigns.provider_id, %{}) + |> Map.merge(new_attrs) - _ -> - {:error, changeset} - end + providers = + socket.assigns.providers + |> Map.delete(socket.assigns.provider_id) + |> Map.values() - case update do - {:ok, _config} -> - {:noreply, - socket - |> put_flash(:info, "Updated successfully.") - |> redirect(to: socket.assigns.return_to)} + {:ok, _changeset} = + FzHttp.Configurations.update_configuration(%{ + saml_identity_providers: providers ++ [new_attrs] + }) - {:error, changeset} -> - {:noreply, assign(socket, :changeset, changeset)} + socket = + socket + |> put_flash(:info, "Updated successfully.") + |> redirect(to: socket.assigns.return_to) + + {:noreply, socket} + else + socket = assign(socket, :changeset, render_changeset_errors(create_changeset)) + {:noreply, socket} end end end diff --git a/apps/fz_http/lib/fz_http_web/live/setting_live/security_live.ex b/apps/fz_http/lib/fz_http_web/live/setting_live/security_live.ex index a4ea31e42..9f692b643 100644 --- a/apps/fz_http/lib/fz_http_web/live/setting_live/security_live.ex +++ b/apps/fz_http/lib/fz_http_web/live/setting_live/security_live.ex @@ -87,16 +87,20 @@ defmodule FzHttpWeb.SettingLive.Security do field_key = Map.fetch!(@types, type) providers = - socket.assigns.config_changeset - |> get_in([Access.key!(:data), Access.key!(field_key)]) + socket.assigns.config_changeset.data + |> Map.fetch!(field_key) |> Enum.reject(&(&1.id == key)) + |> Enum.map(&Map.from_struct/1) - {:ok, conf} = Configurations.update_configuration(%{field_key => providers}) + {:ok, configuration} = Configurations.update_configuration(%{field_key => providers}) - {:noreply, - socket - |> assign(String.to_existing_atom("#{type}_configs"), get_in(conf, [Access.key!(field_key)])) - |> assign(:config_changeset, change(conf))} + socket = + socket + |> assign(:config_changeset, Configurations.change_configuration(configuration)) + |> assign(:oidc_configs, map_providers(configuration.openid_connect_providers)) + |> assign(:saml_configs, map_providers(configuration.saml_identity_providers)) + + {:noreply, socket} end @hour 3_600 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 39142add1..3db2af86b 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 @@ -1,9 +1,9 @@ defmodule FzHttpWeb.SettingLive.SecurityTest do use FzHttpWeb.ConnCase, async: true - alias FzHttp.Configurations alias FzHttpWeb.SettingLive.Security import FzHttp.SAMLIdentityProviderFixtures + import FzHttp.ConfigurationsFixtures describe "authenticated mount" do test "loads the active sessions table", %{admin_conn: conn} do @@ -52,8 +52,6 @@ defmodule FzHttpWeb.SettingLive.SecurityTest do end describe "toggles" do - import FzHttp.ConfigurationsFixtures - setup %{conf_key: key, conf_val: val} do FzHttp.Configurations.put!(key, val) {:ok, path: ~p"/settings/security"} @@ -95,8 +93,6 @@ defmodule FzHttpWeb.SettingLive.SecurityTest do end describe "oidc configuration" do - import FzHttp.ConfigurationsFixtures - setup %{admin_conn: conn} do configuration(%{ openid_connect_providers: [ @@ -108,6 +104,15 @@ defmodule FzHttpWeb.SettingLive.SecurityTest do "discovery_document_uri" => "https://common.auth0.com/.well-known/openid-configuration", "auto_create_users" => false + }, + %{ + "id" => "test2", + "label" => "test2", + "client_id" => "foo2", + "client_secret" => "bar2", + "discovery_document_uri" => + "https://common.auth0.com/.well-known/openid-configuration", + "auto_create_users" => false } ], saml_identity_providers: [] @@ -130,7 +135,7 @@ defmodule FzHttpWeb.SettingLive.SecurityTest do test "click edit button", %{view: view} do html = view - |> element("a", "Edit") + |> element("a[href=\"/settings/security/oidc/test/edit\"]", "Edit") |> render_click() assert html =~ ~s|

OIDC Configuration

| @@ -139,35 +144,44 @@ defmodule FzHttpWeb.SettingLive.SecurityTest do test "validate", %{view: view} do view - |> element("a", "Edit") + |> element("a[href=\"/settings/security/oidc/test/edit\"]", "Edit") |> render_click() return = view |> form("#oidc-form") - |> render_submit(%{"label" => "updated"}) + |> render_submit(%{ + open_id_connect_provider: %{ + label: "updated" + } + }) assert {:error, {:redirect, _}} = return - assert FzHttp.Configurations.get!(:openid_connect_providers) == [ - %FzHttp.Configurations.Configuration.OpenIDConnectProvider{ - id: "test", - label: "test123", - scope: "openid email profile", - response_type: "code", - client_id: "foo", - client_secret: "bar", - discovery_document_uri: - "https://common.auth0.com/.well-known/openid-configuration", - redirect_uri: nil, - auto_create_users: false - } - ] + assert %FzHttp.Configurations.Configuration.OpenIDConnectProvider{ + id: "test", + label: "updated", + scope: "openid email profile", + response_type: "code", + client_id: "foo", + client_secret: "bar", + discovery_document_uri: + "https://common.auth0.com/.well-known/openid-configuration", + redirect_uri: nil, + auto_create_users: false + } in FzHttp.Configurations.get!(:openid_connect_providers) end test "delete", %{view: view} do view - |> element("button", "Delete") + |> element("button[phx-value-key=\"test\"]", "Delete") + |> render_click() + + openid_connect_providers = FzHttp.Configurations.get!(:openid_connect_providers) + assert Enum.map(openid_connect_providers, & &1.id) == ["test2"] + + view + |> element("button[phx-value-key=\"test2\"]", "Delete") |> render_click() assert FzHttp.Configurations.get!(:openid_connect_providers) == [] @@ -175,13 +189,14 @@ defmodule FzHttpWeb.SettingLive.SecurityTest do end describe "saml configuration" do - import FzHttp.ConfigurationsFixtures - setup %{admin_conn: conn} do # Security views use the DB config, not cached config, so update DB here for testing + saml_attrs1 = saml_attrs() + saml_attrs2 = saml_attrs() |> Map.put("id", "test2") |> Map.put("label", "test2") + configuration(%{ openid_connect_providers: [], - saml_identity_providers: [saml_attrs()] + saml_identity_providers: [saml_attrs1, saml_attrs2] }) path = ~p"/settings/security" @@ -227,7 +242,7 @@ defmodule FzHttpWeb.SettingLive.SecurityTest do saml_identity_providers = FzHttp.Configurations.get!(:saml_identity_providers) - assert length(saml_identity_providers) == 2 + assert length(saml_identity_providers) == 3 assert %FzHttp.Configurations.Configuration.SAMLIdentityProvider{ auto_create_users: false, @@ -246,33 +261,43 @@ defmodule FzHttpWeb.SettingLive.SecurityTest do test "edit", %{view: view} do html = view - |> element("a", "Edit") + |> element("a[href=\"/settings/security/saml/test/edit\"]", "Edit") |> render_click() assert html =~ ~s|

SAML Configuration

| assert html =~ ~s|entityID="http://localhost:8080/realms/firezone| + assert html =~ ~s| form("#saml-form", %{ - saml_identity_provider: %{ - label: "just-changed" + |> form("#saml-form") + |> render_submit(%{ + "saml_identity_provider" => %{ + id: "new_id", + label: "new_label", + base_url: "http://example.com/realms/firezone", + metadata: saml_attrs()["metadata"] } }) - |> render_submit() - assert html =~ "value=\"just-changed\"" + assert {:error, {:redirect, %{flash: _, to: "/settings/security"}}} = redirect - # XXX this test fails, figure out why - # assert [saml_identity_provider] = FzHttp.Configurations.get!(:saml_identity_providers) - # assert saml_identity_provider.label == "changed" + assert saml_identity_provider = + FzHttp.Configurations.get!(:saml_identity_providers) + |> Enum.find(fn saml_identity_provider -> + saml_identity_provider.id == "new_id" + end) + + assert saml_identity_provider.id == "new_id" + assert saml_identity_provider.label == "new_label" + assert saml_identity_provider.base_url == "http://example.com/realms/firezone" end test "validate", %{view: view} do attrs = saml_attrs() view - |> element("a", "Edit") + |> element("a[href=\"/settings/security/saml/test/edit\"]", "Edit") |> render_click() html = @@ -283,22 +308,27 @@ defmodule FzHttpWeb.SettingLive.SecurityTest do # stays on the modal assert html =~ ~s|

SAML Configuration

| - assert FzHttp.Configurations.get!(:saml_identity_providers) == [ - %FzHttp.Configurations.Configuration.SAMLIdentityProvider{ - auto_create_users: true, - base_url: "#{FzHttp.Config.fetch_env!(:fz_http, :external_url)}/auth/saml", - id: attrs["id"], - label: attrs["label"], - metadata: attrs["metadata"], - sign_metadata: true, - sign_requests: true, - signed_assertion_in_resp: true, - signed_envelopes_in_resp: true - } - ] + assert %FzHttp.Configurations.Configuration.SAMLIdentityProvider{ + auto_create_users: true, + base_url: "#{FzHttp.Config.fetch_env!(:fz_http, :external_url)}/auth/saml", + id: attrs["id"], + label: attrs["label"], + metadata: attrs["metadata"], + sign_metadata: true, + sign_requests: true, + signed_assertion_in_resp: true, + signed_envelopes_in_resp: true + } in FzHttp.Configurations.get!(:saml_identity_providers) end test "delete", %{view: view} do + view + |> element("button[phx-value-key=\"test\"]", "Delete") + |> render_click() + + saml_identity_providers = FzHttp.Configurations.get!(:saml_identity_providers) + assert Enum.map(saml_identity_providers, & &1.id) == ["test2"] + view |> element("button", "Delete") |> render_click()