Better outbound email config parsing (#1346)

This also refactors local auth routes to protect them using Plug.

apps/fz_http/lib/fz_http/configurations/mailer.ex

@@ -0,0 +1,34 @@
+defmodule FzHttp.Configurations.Mailer do
+  @moduledoc """
+  A non-persisted schema to validate email configs on boot.
+  XXX: Consider persisting this to make outbound email configurable via API.
+  """
+  use Ecto.Schema
+  import Ecto.Changeset
+
+  embedded_schema do
+    field :from, :string
+    field :provider, :string
+    field :configs, :map
+  end
+
+  def changeset(attrs) do
+    %__MODULE__{}
+    |> cast(attrs, [:from, :provider, :configs])
+    |> validate_required([:from, :provider, :configs])
+    |> validate_format(:from, ~r/@/)
+    |> validate_provider_in_configs()
+  end
+
+  defp validate_provider_in_configs(
+         %Ecto.Changeset{
+           changes: %{provider: provider, configs: configs}
+         } = changeset
+       )
+       when not is_map_key(configs, provider) do
+    changeset
+    |> add_error(:provider, "must exist in configs")
+  end
+
+  defp validate_provider_in_configs(changeset), do: changeset
+end

apps/fz_http/lib/fz_http/mailer.ex

@@ -1,35 +0,0 @@
-defmodule FzHttpWeb.Mailer do
-  @moduledoc """
-  Outbound Email Sender.
-  """
-
-  use Swoosh.Mailer, otp_app: :fz_http
-
-  alias Swoosh.{Adapters, Email}
-
-  @provider_mapping %{
-    "smtp" => Adapters.SMTP,
-    "mailgun" => Adapters.Mailgun,
-    "mandrill" => Adapters.Mandrill,
-    "sendgrid" => Adapters.Sendgrid,
-    "post_mark" => Adapters.Postmark,
-    "sendmail" => Adapters.Sendmail
-  }
-
-  def default_email do
-    Email.new()
-    |> Email.from(FzHttp.Config.fetch_env!(:fz_http, FzHttpWeb.Mailer)[:from_email])
-  end
-
-  def configs_for(provider) do
-    adapter = Map.fetch!(@provider_mapping, provider)
-
-    mailer_configs =
-      System.fetch_env!("OUTBOUND_EMAIL_CONFIGS")
-      |> Jason.decode!()
-      |> Map.fetch!(provider)
-      |> Enum.map(fn {k, v} -> {String.to_atom(k), v} end)
-
-    [adapter: adapter] ++ mailer_configs
-  end
-end

apps/fz_http/lib/fz_http/telemetry.ex

@@ -112,7 +112,7 @@ defmodule FzHttp.Telemetry do
           FzHttp.Configurations.get!(:allow_unprivileged_device_configuration),
         local_authentication: FzHttp.Configurations.get!(:local_auth_enabled),
         disable_vpn_on_oidc_error: FzHttp.Configurations.get!(:disable_vpn_on_oidc_error),
-        outbound_email: outbound_email?(),
+        outbound_email: FzHttpWeb.Mailer.active?(),
         external_database:
           external_database?(Map.new(FzHttp.Config.fetch_env!(:fz_http, FzHttp.Repo))),
         logo_type: FzHttp.Configurations.logo_type(FzHttp.Configurations.get!(:logo))
@@ -161,12 +161,6 @@ defmodule FzHttp.Telemetry do
     host != "localhost" && host != "127.0.0.1"
   end
 
-  defp outbound_email? do
-    from_email = FzHttp.Config.fetch_env!(:fz_http, FzHttpWeb.Mailer)[:from_email]
-
-    !is_nil(from_email)
-  end
-
   defp os_type do
     case :os.type() do
       {:unix, type} ->

apps/fz_http/lib/fz_http_web/controllers/auth_controller.ex

@@ -11,8 +11,6 @@ defmodule FzHttpWeb.AuthController do
   alias FzHttpWeb.UserFromAuth
   require Logger
 
-  @local_auth_providers [:identity, :magic_link]
-
   # Uncomment when Helpers.callback_url/1 is fixed
   # alias Ueberauth.Strategy.Helpers
 
@@ -36,7 +34,7 @@ defmodule FzHttpWeb.AuthController do
   def callback(%{assigns: %{ueberauth_auth: auth}} = conn, _params) do
     case UserFromAuth.find_or_create(auth) do
       {:ok, user} ->
-        maybe_sign_in(conn, user, auth)
+        do_sign_in(conn, user, auth)
 
       {:error, reason} when reason in [:not_found, :invalid_credentials] ->
         conn
@@ -59,7 +57,7 @@ defmodule FzHttpWeb.AuthController do
 
     with {:ok, user} <-
            UserFromAuth.find_or_create(:saml, idp, %{"email" => assertion.subject.name}) do
-      maybe_sign_in(conn, user, %{provider: idp})
+      do_sign_in(conn, user, %{provider: idp})
     end
   end
 
@@ -81,7 +79,7 @@ defmodule FzHttpWeb.AuthController do
 
           conn
           |> put_session("id_token", tokens["id_token"])
-          |> maybe_sign_in(user, %{provider: provider_id})
+          |> do_sign_in(user, %{provider: provider_id})
 
         {:error, reason} ->
           conn
@@ -143,7 +141,7 @@ defmodule FzHttpWeb.AuthController do
   def magic_sign_in(conn, %{"user_id" => user_id, "token" => token}) do
     with {:ok, user} <- Users.fetch_user_by_id(user_id),
          {:ok, _user} <- Users.consume_sign_in_token(user, token) do
-      maybe_sign_in(conn, user, %{provider: :magic_link})
+      do_sign_in(conn, user, %{provider: :magic_link})
     else
       {:error, _reason} ->
         conn
@@ -181,20 +179,6 @@ defmodule FzHttpWeb.AuthController do
     end
   end
 
-  defp maybe_sign_in(conn, user, %{provider: provider_key} = auth)
-       when is_atom(provider_key) and provider_key in @local_auth_providers do
-    if FzHttp.Configurations.get!(:local_auth_enabled) do
-      do_sign_in(conn, user, auth)
-    else
-      conn
-      |> put_resp_content_type("text/plain")
-      |> send_resp(401, "Local auth disabled")
-      |> halt()
-    end
-  end
-
-  defp maybe_sign_in(conn, user, auth), do: do_sign_in(conn, user, auth)
-
   defp do_sign_in(conn, user, auth) do
     conn
     |> Authentication.sign_in(user, auth)

apps/fz_http/lib/fz_http_web/mailer.ex

@@ -0,0 +1,44 @@
+defmodule FzHttpWeb.Mailer do
+  @moduledoc """
+  Outbound Email Sender.
+  """
+
+  use Swoosh.Mailer, otp_app: :fz_http
+
+  alias Swoosh.{Adapters, Email}
+
+  @provider_mapping %{
+    "smtp" => Adapters.SMTP,
+    "mailgun" => Adapters.Mailgun,
+    "mandrill" => Adapters.Mandrill,
+    "sendgrid" => Adapters.Sendgrid,
+    "post_mark" => Adapters.Postmark,
+    "sendmail" => Adapters.Sendmail
+  }
+
+  def active? do
+    mailer_config = FzHttp.Config.fetch_env!(:fz_http, FzHttpWeb.Mailer)
+    mailer_config[:from_email] && mailer_config[:adapter]
+  end
+
+  def default_email do
+    # Fail hard if email not configured
+    from_email =
+      FzHttp.Config.fetch_env!(:fz_http, FzHttpWeb.Mailer)
+      |> Keyword.fetch!(:from_email)
+
+    Email.new()
+    |> Email.from(from_email)
+  end
+
+  def from_configuration(%FzHttp.Configurations.Mailer{} = mailer) do
+    from_email = mailer.from
+    config = Map.fetch!(mailer.configs, mailer.provider)
+    adapter = Map.fetch!(@provider_mapping, mailer.provider)
+
+    [
+      from_email: from_email,
+      adapter: adapter
+    ] ++ Enum.map(config, fn {k, v} -> {String.to_atom(k), v} end)
+  end
+end

apps/fz_http/lib/fz_http_web/plug/require_local_authentication.ex

@@ -0,0 +1,16 @@
+defmodule FzHttpWeb.Plug.RequireLocalAuthentication do
+  use FzHttpWeb, :controller
+
+  def init(opts), do: opts
+
+  def call(conn, _opts) do
+    if FzHttp.Configurations.get!(:local_auth_enabled) do
+      conn
+    else
+      conn
+      |> put_resp_content_type("text/plain")
+      |> send_resp(404, "Local auth disabled")
+      |> halt()
+    end
+  end
+end

apps/fz_http/lib/fz_http_web/router.ex

@@ -36,7 +36,6 @@ defmodule FzHttpWeb.Router do
   end
 
   pipeline :require_unauthenticated do
-    plug FzHttpWeb.Plug.Authorization, :test
     plug Guardian.Plug.EnsureNotAuthenticated
   end
 
@@ -44,19 +43,43 @@ defmodule FzHttpWeb.Router do
     plug FzHttpWeb.Auth.HTML.Pipeline
   end
 
+  pipeline :require_local_auth do
+    plug FzHttpWeb.Plug.RequireLocalAuthentication
+  end
+
   pipeline :samly do
     plug :fetch_session
     plug FzHttpWeb.Plug.SamlyTargetUrl
   end
 
-  # Ueberauth routes
-  scope "/auth", FzHttpWeb do
+  # OIDC auth routes
+  scope "/auth/oidc", FzHttpWeb do
     pipe_through [
       :browser,
       :html_auth,
       :require_unauthenticated
     ]
 
+    get "/:provider/callback", AuthController, :callback, as: :auth_oidc
+    get "/:provider", AuthController, :redirect_oidc_auth_uri, as: :auth_oidc
+  end
+
+  # SAML auth routes
+  scope "/auth/saml" do
+    pipe_through :samly
+
+    forward "/", Samly.Router
+  end
+
+  # Local auth routes
+  scope "/auth", FzHttpWeb do
+    pipe_through [
+      :browser,
+      :html_auth,
+      :require_unauthenticated,
+      :require_local_auth
+    ]
+
     get "/reset_password", AuthController, :reset_password
     post "/magic_link", AuthController, :magic_link
     get "/magic/:user_id/:token", AuthController, :magic_sign_in
@@ -64,14 +87,6 @@ defmodule FzHttpWeb.Router do
     get "/:provider", AuthController, :request
     get "/:provider/callback", AuthController, :callback
     post "/:provider/callback", AuthController, :callback
-    get "/oidc/:provider/callback", AuthController, :callback, as: :auth_oidc
-    get "/oidc/:provider", AuthController, :redirect_oidc_auth_uri, as: :auth_oidc
-  end
-
-  scope "/auth/saml" do
-    pipe_through :samly
-
-    forward "/", Samly.Router
   end
 
   # Unauthenticated routes

apps/fz_http/test/fz_http/configurations/mailer_test.exs

@@ -0,0 +1,39 @@
+defmodule FzHttp.Configurations.MailerTest do
+  use ExUnit.Case, async: true
+
+  alias FzHttp.Configurations.Mailer
+
+  describe "changeset/1" do
+    test "adds errors for required fields" do
+      changeset = Mailer.changeset(%{})
+
+      assert changeset.errors[:from] == {"can't be blank", [validation: :required]}
+      assert changeset.errors[:provider] == {"can't be blank", [validation: :required]}
+      assert changeset.errors[:configs] == {"can't be blank", [validation: :required]}
+    end
+
+    test "adds error for invalid from address" do
+      changeset = Mailer.changeset(%{"from" => "invalid"})
+
+      assert changeset.errors[:from] == {"has invalid format", [validation: :format]}
+    end
+
+    test "adds error when provider is not in configs" do
+      changeset =
+        Mailer.changeset(%{"from" => "foobar@localhost", "provider" => "smtp", "configs" => %{}})
+
+      assert changeset.errors[:provider] == {"must exist in configs", []}
+    end
+
+    test "doesn't add errors when attrs is valid" do
+      changeset =
+        Mailer.changeset(%{
+          "from" => "foobar@localhost",
+          "provider" => "smtp",
+          "configs" => %{"smtp" => %{}}
+        })
+
+      assert changeset.errors == []
+    end
+  end
+end

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.put_env(FzHttp.Repo, hostname: "localhost")
+      FzHttp.Config.put_env(:fz_http, FzHttp.Repo, hostname: "localhost")
 
       ping_data = Telemetry.ping_data()
 
@@ -123,7 +123,7 @@ defmodule FzHttp.TelemetryTest do
     end
 
     test "local url" do
-      FzHttp.Config.put_env(FzHttp.Repo, url: "postgres://127.0.0.1")
+      FzHttp.Config.put_env(:fz_http, 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.put_env(FzHttp.Repo, hostname: "firezone.dev")
+      FzHttp.Config.put_env(:fz_http, 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.put_env(FzHttp.Repo, url: "postgres://firezone.dev")
+      FzHttp.Config.put_env(:fz_http, FzHttp.Repo, url: "postgres://firezone.dev")
 
       ping_data = Telemetry.ping_data()
 
@@ -149,7 +149,10 @@ defmodule FzHttp.TelemetryTest do
 
   describe "email" do
     test "outbound set" do
-      FzHttp.Config.put_env(FzHttpWeb.Mailer, from_email: "test@firezone.dev")
+      FzHttp.Config.put_env(:fz_http, FzHttpWeb.Mailer,
+        adapter: Swoosh.Adapters.NoopAdapter,
+        from_email: "test@firezone.dev"
+      )
 
       ping_data = Telemetry.ping_data()
 
@@ -157,7 +160,10 @@ defmodule FzHttp.TelemetryTest do
     end
 
     test "outbound unset" do
-      FzHttp.Config.put_env(FzHttpWeb.Mailer, from_email: nil)
+      FzHttp.Config.put_env(:fz_http, FzHttpWeb.Mailer,
+        adapter: SwooshAdapters.NoopAdapter,
+        from_email: nil
+      )
 
       ping_data = Telemetry.ping_data()
 

apps/fz_http/test/fz_http_web/controllers/auth_controller_test.exs

@@ -65,6 +65,22 @@ defmodule FzHttpWeb.AuthControllerTest do
       assert redirected_to(get(conn, ~p"/auth/identity/callback")) == ~p"/"
     end
 
+    test "GET /auth/identity omits forgot password link when local_auth disabled", %{
+      unauthed_conn: conn
+    } do
+      FzHttp.Configurations.put!(:local_auth_enabled, false)
+      test_conn = get(conn, ~p"/auth/identity")
+
+      assert text_response(test_conn, 404) == "Local auth disabled"
+    end
+
+    test "when local_auth is disabled responds with 404", %{unauthed_conn: conn} do
+      FzHttp.Configurations.put!(:local_auth_enabled, false)
+      test_conn = post(conn, ~p"/auth/identity/callback", %{})
+
+      assert text_response(test_conn, 404) == "Local auth disabled"
+    end
+
     test "invalid email", %{unauthed_conn: conn} do
       params = %{
         "email" => "invalid@test",
@@ -114,7 +130,16 @@ defmodule FzHttpWeb.AuthControllerTest do
       FzHttp.Configurations.put!(:local_auth_enabled, false)
 
       test_conn = post(conn, ~p"/auth/identity/callback", params)
-      assert text_response(test_conn, 401) == "Local auth disabled"
+      assert text_response(test_conn, 404) == "Local auth disabled"
+    end
+  end
+
+  describe "GET /auth/reset_password" do
+    test "protects route when local_auth is disabled", %{unauthed_conn: conn} do
+      FzHttp.Configurations.put!(:local_auth_enabled, false)
+      test_conn = get(conn, ~p"/auth/reset_password")
+
+      assert text_response(test_conn, 404) == "Local auth disabled"
     end
   end
 
@@ -285,7 +310,7 @@ defmodule FzHttpWeb.AuthControllerTest do
       FzHttp.Configurations.put!(:local_auth_enabled, false)
 
       test_conn = get(conn, ~p"/auth/magic/#{user.id}/#{user.sign_in_token}")
-      assert text_response(test_conn, 401) == "Local auth disabled"
+      assert text_response(test_conn, 404) == "Local auth disabled"
     end
   end
 

apps/fz_http/test/fz_http_web/mailer_test.exs

@@ -8,13 +8,19 @@ defmodule FzHttpWeb.MailerTest do
     assert Mailer.default_email().from == {"", "test@firez.one"}
   end
 
-  test "configs_for provider" do
-    System.put_env(
-      "OUTBOUND_EMAIL_CONFIGS",
-      Jason.encode!(%{"smtp" => %{"config_key" => "config_value"}})
-    )
+  test "from_configuration/1" do
+    attrs = %{
+      "from" => "foo@localhost",
+      "provider" => "smtp",
+      "configs" => %{"smtp" => %{"config_key" => "config_value"}}
+    }
 
-    assert Mailer.configs_for("smtp") == [
+    mailer =
+      FzHttp.Configurations.Mailer.changeset(attrs)
+      |> Ecto.Changeset.apply_changes()
+
+    assert Mailer.from_configuration(mailer) == [
+             from_email: "foo@localhost",
              adapter: Swoosh.Adapters.SMTP,
              config_key: "config_value"
            ]

config/runtime.exs

@@ -7,6 +7,8 @@ import Config
 
 alias FzCommon.{CLI, FzInteger, FzString, FzKernelVersion, FzNet}
 
+require Logger
+
 # external_url is important, so fail fast here if we can't parse
 {:ok, external_url} =
   if config_env() == :prod do
@@ -89,14 +91,24 @@ if config_env() == :prod do
   cookie_secure = FzString.to_boolean(System.get_env("SECURE_COOKIES", "true"))
 
   # Outbound Email
-  from_email = System.get_env("OUTBOUND_EMAIL_FROM")
+  outbound_config_env = System.get_env("OUTBOUND_EMAIL_CONFIGS", "{}")
 
-  if from_email do
-    provider = System.get_env("OUTBOUND_EMAIL_PROVIDER", "sendmail")
+  with {:ok, configs} <- Jason.decode(outbound_config_env) do
+    mailer =
+      FzHttp.Configurations.Mailer.changeset(%{
+        "from" => System.get_env("OUTBOUND_EMAIL_FROM"),
+        "provider" => System.get_env("OUTBOUND_EMAIL_PROVIDER", "sendmail"),
+        "configs" => configs
+      })
 
-    config :fz_http,
-           FzHttpWeb.Mailer,
-           [from_email: from_email] ++ FzHttpWeb.Mailer.configs_for(provider)
+    if mailer.valid? do
+      config :fz_http, FzHttpWeb.Mailer, FzHttpWeb.Mailer.from_configuration(mailer)
+    else
+      Logger.warn("Outbound email not configured. Disabling! Details: #{mailer.errors}")
+    end
+  else
+    {:error, error} ->
+      raise "OUTBOUND_EMAIL_CONFIGS not a valid JSON-encoded string. Error: #{error}"
   end
 
   max_devices_per_user =