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

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,

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,

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

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

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

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|<p class="modal-card-title">OIDC Configuration</p>|
@@ -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|<p class="modal-card-title">SAML Configuration</p>|
       assert html =~ ~s|entityID=&quot;http://localhost:8080/realms/firezone|
+      assert html =~ ~s|<input class="input " id="saml-form_label"|
 
-      html =
+      redirect =
         view
-        |> 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|<p class="modal-card-title">SAML Configuration</p>|
 
-      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()