From c8090f80170741b164f606fc708dfc130ca3fb45 Mon Sep 17 00:00:00 2001 From: bmanifold Date: Mon, 2 Oct 2023 16:19:48 -0400 Subject: [PATCH] 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. --- .../apps/api/test/api/client/channel_test.exs | 8 +- .../domain/lib/domain/config/configuration.ex | 4 +- .../domain/config/configuration/changeset.ex | 26 ++- .../domain/lib/domain/config/definitions.ex | 18 +- elixir/apps/domain/lib/domain/validator.ex | 21 +- ...pdate_clients_upstream_dns_column_type.exs | 11 ++ .../apps/domain/test/domain/config_test.exs | 39 ++-- .../domain/test/support/fixtures/config.ex | 2 +- .../web/lib/web/components/core_components.ex | 3 +- elixir/apps/web/lib/web/live/settings/dns.ex | 155 +++++++++++---- .../test/web/live/settings/dns/index_test.exs | 181 ++++++++++++++++++ 11 files changed, 397 insertions(+), 71 deletions(-) create mode 100644 elixir/apps/domain/priv/repo/migrations/20230926211345_update_clients_upstream_dns_column_type.exs create mode 100644 elixir/apps/web/test/web/live/settings/dns/index_test.exs diff --git a/elixir/apps/api/test/api/client/channel_test.exs b/elixir/apps/api/test/api/client/channel_test.exs index 6fae9b9f6..24c3c0fee 100644 --- a/elixir/apps/api/test/api/client/channel_test.exs +++ b/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 diff --git a/elixir/apps/domain/lib/domain/config/configuration.ex b/elixir/apps/domain/lib/domain/config/configuration.ex index c3e59802d..79cd49760 100644 --- a/elixir/apps/domain/lib/domain/config/configuration.ex +++ b/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 diff --git a/elixir/apps/domain/lib/domain/config/configuration/changeset.ex b/elixir/apps/domain/lib/domain/config/configuration/changeset.ex index 17f9a74de..35b029298 100644 --- a/elixir/apps/domain/lib/domain/config/configuration/changeset.ex +++ b/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) diff --git a/elixir/apps/domain/lib/domain/config/definitions.ex b/elixir/apps/domain/lib/domain/config/definitions.ex index 08cef280f..f7169eb9e 100644 --- a/elixir/apps/domain/lib/domain/config/definitions.ex +++ b/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, []} ) ############################################## diff --git a/elixir/apps/domain/lib/domain/validator.ex b/elixir/apps/domain/lib/domain/validator.ex index e6272acaa..d2887fa80 100644 --- a/elixir/apps/domain/lib/domain/validator.ex +++ b/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 diff --git a/elixir/apps/domain/priv/repo/migrations/20230926211345_update_clients_upstream_dns_column_type.exs b/elixir/apps/domain/priv/repo/migrations/20230926211345_update_clients_upstream_dns_column_type.exs new file mode 100644 index 000000000..90199cd4e --- /dev/null +++ b/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 diff --git a/elixir/apps/domain/test/domain/config_test.exs b/elixir/apps/domain/test/domain/config_test.exs index 99eba13c8..da6741ba9 100644 --- a/elixir/apps/domain/test/domain/config_test.exs +++ b/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 diff --git a/elixir/apps/domain/test/support/fixtures/config.ex b/elixir/apps/domain/test/support/fixtures/config.ex index f463d4081..a6cf1a67d 100644 --- a/elixir/apps/domain/test/support/fixtures/config.ex +++ b/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 diff --git a/elixir/apps/web/lib/web/components/core_components.ex b/elixir/apps/web/lib/web/components/core_components.ex index 5edd161f5..1b420e1dd 100644 --- a/elixir/apps/web/lib/web/components/core_components.ex +++ b/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" diff --git a/elixir/apps/web/lib/web/live/settings/dns.ex b/elixir/apps/web/lib/web/live/settings/dns.ex index ded4c65aa..6eb7cffdc 100644 --- a/elixir/apps/web/lib/web/live/settings/dns.ex +++ b/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 @@ -29,52 +46,114 @@ defmodule Web.Settings.DNS do

+ <.flash kind={:success} flash={@flash} phx-click="lv:clear-flash" />

Client DNS

-
+

+ DNS servers will be used in the order they are listed below. +

+ + <.form for={@form} phx-submit={:submit} phx-change={:change}>
- - + <.inputs_for :let={dns} field={@form[:clients_upstream_dns]}> +
+ <.input label="Address" field={dns[:address]} placeholder="DNS Server Address" /> +
+
-
- <.label for="resolver-address"> - Address - - -

- IP addresses, FQDNs, and DNS-over-HTTPS (DoH) addresses are supported. -

-
-
-
- +
-
+
""" 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 diff --git a/elixir/apps/web/test/web/live/settings/dns/index_test.exs b/elixir/apps/web/test/web/live/settings/dns/index_test.exs new file mode 100644 index 000000000..20b145357 --- /dev/null +++ b/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