Update Account DNS settings UI (#2120)

Why:

* The previous Account DNS Settings page was only a static page. This
commit enables the form on the page to actually save and update the DNS
settings for a given account.

elixir/apps/api/test/api/client/channel_test.exs

@@ -4,7 +4,11 @@ defmodule API.Client.ChannelTest do
 
   setup do
     account = Fixtures.Accounts.create_account()
-    Fixtures.Config.upsert_configuration(account: account, clients_upstream_dns: ["1.1.1.1"])
+
+    Fixtures.Config.upsert_configuration(
+      account: account,
+      clients_upstream_dns: [%{address: "1.1.1.1"}]
+    )
 
     actor_group = Fixtures.Actors.create_group(account: account)
     actor = Fixtures.Actors.create_actor(type: :account_admin_user, account: account)
@@ -134,7 +138,7 @@ defmodule API.Client.ChannelTest do
                ipv4: client.ipv4,
                ipv6: client.ipv6,
                upstream_dns: [
-                 %Postgrex.INET{address: {1, 1, 1, 1}}
+                 %Domain.Config.Configuration.ClientsUpstreamDNS{address: "1.1.1.1"}
                ]
              }
     end

elixir/apps/domain/lib/domain/config/configuration.ex

@@ -3,7 +3,9 @@ defmodule Domain.Config.Configuration do
   alias Domain.Config.Logo
 
   schema "configurations" do
-    field :clients_upstream_dns, {:array, :string}, default: []
+    embeds_many :clients_upstream_dns, ClientsUpstreamDNS, on_replace: :delete, primary_key: false do
+      field :address, :string
+    end
 
     embeds_one :logo, Logo, on_replace: :delete
 

elixir/apps/domain/lib/domain/config/configuration/changeset.ex

@@ -2,14 +2,17 @@ defmodule Domain.Config.Configuration.Changeset do
   use Domain, :changeset
   import Domain.Config, only: [config_changeset: 2]
 
-  @fields ~w[clients_upstream_dns]a
+  @fields ~w[clients_upstream_dns logo]a
 
   def changeset(configuration, attrs) do
     changeset =
       configuration
-      |> cast(attrs, @fields)
+      |> cast(attrs, [])
       |> cast_embed(:logo)
-      |> trim_change(:clients_upstream_dns)
+      |> cast_embed(
+        :clients_upstream_dns,
+        with: &clients_upstream_dns_changeset/2
+      )
 
     Enum.reduce(@fields, changeset, fn field, changeset ->
       config_changeset(changeset, field)
@@ -17,6 +20,23 @@ defmodule Domain.Config.Configuration.Changeset do
     |> ensure_no_overridden_changes(configuration.account_id)
   end
 
+  def clients_upstream_dns_changeset(
+        dns_config \\ %Domain.Config.Configuration.ClientsUpstreamDNS{},
+        attrs
+      ) do
+    Ecto.Changeset.cast(
+      dns_config,
+      attrs,
+      [:address]
+    )
+    |> validate_required(:address)
+    |> trim_change(:address)
+    |> Domain.Validator.validate_one_of(:address, [
+      &Domain.Validator.validate_fqdn/2,
+      &Domain.Validator.validate_uri(&1, &2, schemes: ["https"])
+    ])
+  end
+
   defp ensure_no_overridden_changes(changeset, account_id) do
     changed_keys = Map.keys(changeset.changes)
 

elixir/apps/domain/lib/domain/config/definitions.ex

@@ -443,24 +443,20 @@ defmodule Domain.Config.Definitions do
   @doc """
   Comma-separated list of upstream DNS servers to use for clients.
 
-  It can be either an IP address or a FQDN if you intend to use a DNS-over-TLS server.
+  It can be one of the following:
+    - IP address
+    - FQDN if you intend to use a DNS-over-TLS server
+    - URI if you intent to use a DNS-over-HTTPS server
 
   Leave this blank to omit the `DNS` section from generated configs,
   which will make clients use default system-provided DNS even when VPN session is active.
   """
   defconfig(
     :clients_upstream_dns,
-    {:array, ",", {:one_of, [Types.IP, :string]}, validate_unique: true},
+    {:json_array, {:embed, Domain.Config.Configuration.ClientsUpstreamDNS},
+     validate_unique: true},
     default: [],
-    changeset: fn
-      Types.IP, changeset, _key ->
-        changeset
-
-      :string, changeset, key ->
-        changeset
-        |> Domain.Validator.trim_change(key)
-        |> Domain.Validator.validate_fqdn(key)
-    end
+    changeset: {Domain.Config.Configuration.Changeset, :clients_upstream_dns_changeset, []}
   )
 
   ##############################################

elixir/apps/domain/lib/domain/validator.ex

@@ -34,7 +34,7 @@ defmodule Domain.Validator do
       case URI.new(value) do
         {:ok, %URI{} = uri} ->
           cond do
-            uri.host == nil ->
+            uri.host == nil or uri.host == "" ->
               [{field, "does not contain a scheme or a host"}]
 
             uri.scheme == nil ->
@@ -80,6 +80,25 @@ defmodule Domain.Validator do
     end
   end
 
+  def validate_one_of(changeset, field, validators) do
+    validate_change(changeset, field, fn current_field, _value ->
+      orig_errors = Enum.filter(changeset.errors, &(elem(&1, 0) == current_field))
+
+      Enum.reduce_while(validators, [], fn validator, errors ->
+        validated_cs = validator.(changeset, current_field)
+
+        new_errors =
+          Enum.filter(validated_cs.errors, &(elem(&1, 0) == current_field)) -- orig_errors
+
+        if Enum.empty?(new_errors) do
+          {:halt, new_errors}
+        else
+          {:cont, new_errors ++ errors}
+        end
+      end)
+    end)
+  end
+
   def validate_no_duplicates(changeset, field) when is_atom(field) do
     validate_change(changeset, field, fn _current_field, list when is_list(list) ->
       list

elixir/apps/domain/priv/repo/migrations/20230926211345_update_clients_upstream_dns_column_type.exs

@@ -0,0 +1,11 @@
+defmodule Domain.Repo.Migrations.UpdateClientsUpstreamDnsColumnType do
+  use Ecto.Migration
+
+  def change do
+    execute("ALTER TABLE configurations DROP COLUMN clients_upstream_dns;")
+
+    alter table("configurations") do
+      add(:clients_upstream_dns, {:array, :map}, default: [], null: false)
+    end
+  end
+end

elixir/apps/domain/test/domain/config_test.exs

@@ -92,7 +92,9 @@ defmodule Domain.ConfigTest do
     test "returns source and config values", %{account: account} do
       assert fetch_resolved_configs!(account.id, [:clients_upstream_dns, :clients_upstream_dns]) ==
                %{
-                 clients_upstream_dns: [%Postgrex.INET{address: {1, 1, 1, 1}, netmask: nil}]
+                 clients_upstream_dns: [
+                   %Domain.Config.Configuration.ClientsUpstreamDNS{address: "1.1.1.1"}
+                 ]
                }
     end
 
@@ -141,7 +143,7 @@ defmodule Domain.ConfigTest do
                %{
                  clients_upstream_dns:
                    {{:db, :clients_upstream_dns},
-                    [%Postgrex.INET{address: {1, 1, 1, 1}, netmask: nil}]}
+                    [%Domain.Config.Configuration.ClientsUpstreamDNS{address: "1.1.1.1"}]}
                }
     end
 
@@ -442,26 +444,25 @@ defmodule Domain.ConfigTest do
       config = get_account_config_by_account_id(account.id)
 
       attrs = %{
-        clients_upstream_dns: ["!!!"]
+        clients_upstream_dns: [%{address: "!!!"}]
       }
 
       assert {:error, changeset} = update_config(config, attrs)
 
       assert errors_on(changeset) == %{
                clients_upstream_dns: [
-                 "!!! is not a valid FQDN",
-                 "must be one of: Elixir.Domain.Types.IP, string"
+                 %{address: ["does not contain a scheme or a host", "!!! is not a valid FQDN"]}
                ]
              }
     end
 
     test "returns error when trying to change overridden value", %{account: account} do
-      put_system_env_override(:clients_upstream_dns, ["1.2.3.4"])
+      put_system_env_override(:clients_upstream_dns, [%{address: "1.2.3.4"}])
 
       config = get_account_config_by_account_id(account.id)
 
       attrs = %{
-        clients_upstream_dns: ["4.1.2.3"]
+        clients_upstream_dns: [%{address: "4.1.2.3"}]
       }
 
       assert {:error, changeset} = update_config(config, attrs)
@@ -478,27 +479,39 @@ defmodule Domain.ConfigTest do
       config = get_account_config_by_account_id(account.id)
 
       attrs = %{
-        clients_upstream_dns: ["   foobar.com", "google.com   "]
+        clients_upstream_dns: [%{address: "   foobar.com"}, %{address: "google.com   "}]
       }
 
       assert {:ok, config} = update_config(config, attrs)
-      assert config.clients_upstream_dns == ["foobar.com", "google.com"]
+
+      assert config.clients_upstream_dns == [
+               %Domain.Config.Configuration.ClientsUpstreamDNS{address: "foobar.com"},
+               %Domain.Config.Configuration.ClientsUpstreamDNS{address: "google.com"}
+             ]
     end
 
     test "changes database config value when it did not exist", %{account: account} do
       config = get_account_config_by_account_id(account.id)
-      attrs = %{clients_upstream_dns: ["foobar.com", "google.com"]}
+      attrs = %{clients_upstream_dns: [%{address: "foobar.com"}, %{address: "google.com"}]}
       assert {:ok, config} = update_config(config, attrs)
-      assert config.clients_upstream_dns == attrs.clients_upstream_dns
+
+      assert config.clients_upstream_dns == [
+               %Domain.Config.Configuration.ClientsUpstreamDNS{address: "foobar.com"},
+               %Domain.Config.Configuration.ClientsUpstreamDNS{address: "google.com"}
+             ]
     end
 
     test "changes database config value when it existed", %{account: account} do
       Fixtures.Config.upsert_configuration(account: account)
 
       config = get_account_config_by_account_id(account.id)
-      attrs = %{clients_upstream_dns: ["foobar.com", "google.com"]}
+      attrs = %{clients_upstream_dns: [%{address: "foobar.com"}, %{address: "google.com"}]}
       assert {:ok, config} = update_config(config, attrs)
-      assert config.clients_upstream_dns == attrs.clients_upstream_dns
+
+      assert config.clients_upstream_dns == [
+               %Domain.Config.Configuration.ClientsUpstreamDNS{address: "foobar.com"},
+               %Domain.Config.Configuration.ClientsUpstreamDNS{address: "google.com"}
+             ]
     end
   end
 end

elixir/apps/domain/test/support/fixtures/config.ex

@@ -4,7 +4,7 @@ defmodule Domain.Fixtures.Config do
 
   def configuration_attrs(attrs \\ %{}) do
     Enum.into(attrs, %{
-      clients_upstream_dns: ["1.1.1.1"]
+      clients_upstream_dns: [%{address: "1.1.1.1"}]
     })
   end
 

elixir/apps/web/lib/web/components/core_components.ex

@@ -291,7 +291,7 @@ defmodule Web.CoreComponents do
   attr :id, :string, default: "flash", doc: "the optional id of flash container"
   attr :flash, :map, default: %{}, doc: "the map of flash messages to display"
   attr :title, :string, default: nil
-  attr :kind, :atom, values: [:info, :error], doc: "used for styling and flash lookup"
+  attr :kind, :atom, values: [:success, :info, :error], doc: "used for styling and flash lookup"
   attr :rest, :global, doc: "the arbitrary HTML attributes to add to the flash container"
   attr :style, :string, default: "pill"
 
@@ -304,6 +304,7 @@ defmodule Web.CoreComponents do
       id={@id}
       class={[
         "p-4 text-sm flash-#{@kind}",
+        @kind == :success && "text-green-800 bg-green-50 dark:bg-gray-800 dark:text-green-400",
         @kind == :info && "text-yellow-800 bg-yellow-50 dark:bg-gray-800 dark:text-yellow-300",
         @kind == :error && "text-red-800 bg-red-50 dark:bg-gray-800 dark:text-red-400",
         @style != "wide" && "mb-4 rounded-lg"

elixir/apps/web/lib/web/live/settings/dns.ex

@@ -1,7 +1,24 @@
 defmodule Web.Settings.DNS do
   use Web, :live_view
+  alias Domain.Config
+
+  def mount(_params, _session, socket) do
+    {:ok, config} = Config.fetch_account_config(socket.assigns.subject)
+
+    form =
+      Config.change_account_config(config, %{})
+      |> add_new_server()
+      |> to_form()
+
+    socket = assign(socket, config: config, form: form)
+
+    {:ok, socket}
+  end
 
   def render(assigns) do
+    assigns =
+      assign(assigns, :errors, translate_errors(assigns.form.errors, :clients_upstream_dns))
+
     ~H"""
     <.breadcrumbs account={@account}>
       <.breadcrumb path={~p"/#{@account}/settings/dns"}>DNS Settings</.breadcrumb>
@@ -29,52 +46,114 @@ defmodule Web.Settings.DNS do
     </p>
     <section class="bg-white dark:bg-gray-900">
       <div class="max-w-2xl px-4 py-8 mx-auto lg:py-16">
+        <.flash kind={:success} flash={@flash} phx-click="lv:clear-flash" />
         <h2 class="mb-4 text-xl font-bold text-gray-900 dark:text-white">Client DNS</h2>
-        <form action="#">
+        <p class="mb-4 text-slate-500">
+          DNS servers will be used in the order they are listed below.
+        </p>
+
+        <.form for={@form} phx-submit={:submit} phx-change={:change}>
           <div class="grid gap-4 mb-4 sm:grid-cols-1 sm:gap-6 sm:mb-6">
             <div>
-              <label
-                for="resolver"
-                class="block mb-2 text-sm font-medium text-gray-900 dark:text-white"
-              >
-                Resolver
-              </label>
-              <select
-                id="resolver"
-                class="bg-gray-50 border border-gray-300 text-gray-900 text-sm rounded-lg focus:ring-blue-500 focus:border-blue-500 block w-full p-2.5 dark:bg-gray-700 dark:border-gray-600 dark:placeholder-gray-400 dark:text-white dark:focus:ring-blue-500 dark:focus:border-blue-500"
-              >
-                <option>System default</option>
-                <option selected>Custom</option>
-              </select>
+              <.inputs_for :let={dns} field={@form[:clients_upstream_dns]}>
+                <div class="mb-4">
+                  <.input label="Address" field={dns[:address]} placeholder="DNS Server Address" />
+                </div>
+              </.inputs_for>
             </div>
-            <div>
-              <.label for="resolver-address">
-                Address
-              </.label>
-              <input
-                type="text"
-                name="address"
-                id="resolver-address"
-                class="bg-gray-50 border border-gray-300 text-gray-900 text-sm rounded-lg focus:ring-primary-600 focus:border-primary-600 block w-full p-2.5 dark:bg-gray-700 dark:border-gray-600 dark:placeholder-gray-400 dark:text-white dark:focus:ring-primary-500 dark:focus:border-primary-500"
-                value="https://doh.familyshield.opendns.com/dns-query"
-                required
-              />
-              <p id="address-explanation" class="mt-2 text-xs text-gray-500 dark:text-gray-400">
-                IP addresses, FQDNs, and DNS-over-HTTPS (DoH) addresses are supported.
-              </p>
-            </div>
-          </div>
-          <div class="flex items-center space-x-4">
-            <button
-              type="submit"
-              class="text-white bg-primary-700 hover:bg-primary-800 focus:ring-4 focus:outline-none focus:ring-primary-300 font-medium rounded-lg text-sm px-5 py-2.5 text-center dark:bg-primary-600 dark:hover:bg-primary-700 dark:focus:ring-primary-800"
-            >
+            <.error :for={msg <- @errors} data-validation-error-for="clients_upstream_dns">
+              <%= msg %>
+            </.error>
+            <.button>
               Save
-            </button>
+            </.button>
           </div>
-        </form>
+        </.form>
       </div>
     </section>
     """
   end
+
+  def handle_event("change", %{"configuration" => config_params}, socket) do
+    form =
+      Config.change_account_config(socket.assigns.config, config_params)
+      |> filter_errors()
+      |> Map.put(:action, :validate)
+      |> to_form()
+
+    socket = assign(socket, form: form)
+    {:noreply, socket}
+  end
+
+  def handle_event("submit", %{"configuration" => config_params}, socket) do
+    attrs = remove_empty_servers(config_params)
+
+    with {:ok, new_config} <-
+           Domain.Config.update_config(socket.assigns.config, attrs, socket.assigns.subject) do
+      form =
+        Config.change_account_config(new_config, %{})
+        |> add_new_server()
+        |> to_form()
+
+      socket = assign(socket, config: new_config, form: form)
+      {:noreply, socket}
+    else
+      {:error, changeset} ->
+        form = to_form(changeset)
+        socket = assign(socket, form: form)
+        {:noreply, socket}
+    end
+  end
+
+  defp remove_errors(changeset, field, message) do
+    errors =
+      Enum.filter(changeset.errors, fn
+        {^field, {^message, _}} -> false
+        {_, _} -> true
+      end)
+
+    %{changeset | errors: errors}
+  end
+
+  defp filter_errors(%{changes: %{clients_upstream_dns: clients_upstream_dns}} = changeset) do
+    filtered_cs =
+      changeset
+      |> remove_errors(:clients_upstream_dns, "address can't be blank")
+
+    filtered_dns_cs =
+      clients_upstream_dns
+      |> Enum.map(fn changeset ->
+        remove_errors(changeset, :address, "can't be blank")
+      end)
+
+    %{filtered_cs | changes: %{clients_upstream_dns: filtered_dns_cs}}
+  end
+
+  defp filter_errors(changeset) do
+    changeset
+  end
+
+  defp remove_empty_servers(config) do
+    servers =
+      config["clients_upstream_dns"]
+      |> Enum.reduce(%{}, fn {key, value}, acc ->
+        case value["address"] do
+          nil -> acc
+          "" -> acc
+          _ -> Map.put(acc, key, value)
+        end
+      end)
+
+    %{"clients_upstream_dns" => servers}
+  end
+
+  defp add_new_server(changeset) do
+    existing_servers = Ecto.Changeset.get_embed(changeset, :clients_upstream_dns)
+
+    Ecto.Changeset.put_embed(
+      changeset,
+      :clients_upstream_dns,
+      existing_servers ++ [%{address: ""}]
+    )
+  end
 end

elixir/apps/web/test/web/live/settings/dns/index_test.exs

@@ -0,0 +1,181 @@
+defmodule Web.Live.Settings.DNS.IndexTest do
+  use Web.ConnCase, async: true
+
+  setup do
+    Domain.Config.put_system_env_override(:outbound_email_adapter, Swoosh.Adapters.Postmark)
+
+    account = Fixtures.Accounts.create_account()
+    identity = Fixtures.Auth.create_identity(account: account, actor: [type: :account_admin_user])
+
+    %{
+      account: account,
+      identity: identity
+    }
+  end
+
+  test "redirects to sign in page for unauthorized user", %{account: account, conn: conn} do
+    assert live(conn, ~p"/#{account}/settings/dns") ==
+             {:error,
+              {:redirect,
+               %{
+                 to: ~p"/#{account}",
+                 flash: %{"error" => "You must log in to access this page."}
+               }}}
+  end
+
+  test "renders breadcrumbs item", %{
+    account: account,
+    identity: identity,
+    conn: conn
+  } do
+    {:ok, _lv, html} =
+      conn
+      |> authorize_conn(identity)
+      |> live(~p"/#{account}/settings/dns")
+
+    assert item = Floki.find(html, "[aria-label='Breadcrumb']")
+    breadcrumbs = String.trim(Floki.text(item))
+    assert breadcrumbs =~ "DNS Settings"
+  end
+
+  test "renders form", %{
+    account: account,
+    identity: identity,
+    conn: conn
+  } do
+    {:ok, lv, _html} =
+      conn
+      |> authorize_conn(identity)
+      |> live(~p"/#{account}/settings/dns")
+
+    form = lv |> form("form")
+
+    assert find_inputs(form) == [
+             "configuration[clients_upstream_dns][0][_persistent_id]",
+             "configuration[clients_upstream_dns][0][address]"
+           ]
+  end
+
+  test "saves custom DNS server address", %{
+    account: account,
+    identity: identity,
+    conn: conn
+  } do
+    attrs = %{configuration: %{clients_upstream_dns: %{"0" => %{address: "8.8.8.8"}}}}
+
+    {:ok, lv, _html} =
+      conn
+      |> authorize_conn(identity)
+      |> live(~p"/#{account}/settings/dns")
+
+    lv
+    |> form("form", attrs)
+    |> render_submit()
+
+    assert lv
+           |> form("form")
+           |> find_inputs() == [
+             "configuration[clients_upstream_dns][0][_persistent_id]",
+             "configuration[clients_upstream_dns][0][address]",
+             "configuration[clients_upstream_dns][1][_persistent_id]",
+             "configuration[clients_upstream_dns][1][address]"
+           ]
+  end
+
+  test "removes blank entries upon save", %{
+    account: account,
+    identity: identity,
+    conn: conn
+  } do
+    attrs = %{
+      configuration: %{
+        clients_upstream_dns: %{"0" => %{address: "8.8.8.8"}}
+      }
+    }
+
+    {:ok, lv, _html} =
+      conn
+      |> authorize_conn(identity)
+      |> live(~p"/#{account}/settings/dns")
+
+    lv
+    |> form("form", attrs)
+    |> render_submit()
+
+    assert lv
+           |> form("form")
+           |> find_inputs() == [
+             "configuration[clients_upstream_dns][0][_persistent_id]",
+             "configuration[clients_upstream_dns][0][address]",
+             "configuration[clients_upstream_dns][1][_persistent_id]",
+             "configuration[clients_upstream_dns][1][address]"
+           ]
+
+    empty_attrs = %{
+      configuration: %{
+        clients_upstream_dns: %{"0" => %{address: ""}}
+      }
+    }
+
+    lv |> form("form", empty_attrs) |> render_submit()
+
+    assert lv
+           |> form("form")
+           |> find_inputs() == [
+             "configuration[clients_upstream_dns][0][_persistent_id]",
+             "configuration[clients_upstream_dns][0][address]"
+           ]
+  end
+
+  test "warns when duplicates found", %{
+    account: account,
+    identity: identity,
+    conn: conn
+  } do
+    addr = %{address: "8.8.8.8"}
+
+    attrs = %{
+      configuration: %{
+        clients_upstream_dns: %{"0" => addr}
+      }
+    }
+
+    {:ok, lv, _html} =
+      conn
+      |> authorize_conn(identity)
+      |> live(~p"/#{account}/settings/dns")
+
+    lv
+    |> form("form", attrs)
+    |> render_submit()
+
+    assert lv
+           |> form("form", %{configuration: %{clients_upstream_dns: %{"1" => addr}}})
+           |> render_change() =~ "should not contain duplicates"
+  end
+
+  test "does not display 'cannot be empty' error message", %{
+    account: account,
+    identity: identity,
+    conn: conn
+  } do
+    attrs = %{
+      configuration: %{
+        clients_upstream_dns: %{"0" => %{address: "8.8.8.8"}}
+      }
+    }
+
+    {:ok, lv, _html} =
+      conn
+      |> authorize_conn(identity)
+      |> live(~p"/#{account}/settings/dns")
+
+    lv
+    |> form("form", attrs)
+    |> render_submit()
+
+    refute lv
+           |> form("form", %{configuration: %{clients_upstream_dns: %{"0" => %{address: ""}}}})
+           |> render_change() =~ "can't be blank"
+  end
+end