From 931029b2ac11c58fc2cddf6d13635a9deeece59b Mon Sep 17 00:00:00 2001 From: Jamil Date: Fri, 20 Jan 2023 07:16:23 -0800 Subject: [PATCH] Better outbound email config parsing (#1346) This also refactors local auth routes to protect them using Plug. --- .../lib/fz_http/configurations/mailer.ex | 34 ++++++++++++++ apps/fz_http/lib/fz_http/mailer.ex | 35 --------------- apps/fz_http/lib/fz_http/telemetry.ex | 8 +--- .../controllers/auth_controller.ex | 24 ++-------- apps/fz_http/lib/fz_http_web/mailer.ex | 44 +++++++++++++++++++ .../plug/require_local_authentication.ex | 16 +++++++ apps/fz_http/lib/fz_http_web/router.ex | 37 +++++++++++----- .../fz_http/configurations/mailer_test.exs | 39 ++++++++++++++++ apps/fz_http/test/fz_http/telemetry_test.exs | 18 +++++--- .../controllers/auth_controller_test.exs | 29 +++++++++++- apps/fz_http/test/fz_http_web/mailer_test.exs | 18 +++++--- config/runtime.exs | 24 +++++++--- 12 files changed, 233 insertions(+), 93 deletions(-) create mode 100644 apps/fz_http/lib/fz_http/configurations/mailer.ex delete mode 100644 apps/fz_http/lib/fz_http/mailer.ex create mode 100644 apps/fz_http/lib/fz_http_web/mailer.ex create mode 100644 apps/fz_http/lib/fz_http_web/plug/require_local_authentication.ex create mode 100644 apps/fz_http/test/fz_http/configurations/mailer_test.exs diff --git a/apps/fz_http/lib/fz_http/configurations/mailer.ex b/apps/fz_http/lib/fz_http/configurations/mailer.ex new file mode 100644 index 000000000..3cb15a6c0 --- /dev/null +++ b/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 diff --git a/apps/fz_http/lib/fz_http/mailer.ex b/apps/fz_http/lib/fz_http/mailer.ex deleted file mode 100644 index 79cc4c1a9..000000000 --- a/apps/fz_http/lib/fz_http/mailer.ex +++ /dev/null @@ -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 diff --git a/apps/fz_http/lib/fz_http/telemetry.ex b/apps/fz_http/lib/fz_http/telemetry.ex index 73c21ae54..25346eb64 100644 --- a/apps/fz_http/lib/fz_http/telemetry.ex +++ b/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} -> diff --git a/apps/fz_http/lib/fz_http_web/controllers/auth_controller.ex b/apps/fz_http/lib/fz_http_web/controllers/auth_controller.ex index 7902ebb6a..0fe134075 100644 --- a/apps/fz_http/lib/fz_http_web/controllers/auth_controller.ex +++ b/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) diff --git a/apps/fz_http/lib/fz_http_web/mailer.ex b/apps/fz_http/lib/fz_http_web/mailer.ex new file mode 100644 index 000000000..0bfaff3f6 --- /dev/null +++ b/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 diff --git a/apps/fz_http/lib/fz_http_web/plug/require_local_authentication.ex b/apps/fz_http/lib/fz_http_web/plug/require_local_authentication.ex new file mode 100644 index 000000000..22cf31087 --- /dev/null +++ b/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 diff --git a/apps/fz_http/lib/fz_http_web/router.ex b/apps/fz_http/lib/fz_http_web/router.ex index b5e8c54b4..71c53a71d 100644 --- a/apps/fz_http/lib/fz_http_web/router.ex +++ b/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 diff --git a/apps/fz_http/test/fz_http/configurations/mailer_test.exs b/apps/fz_http/test/fz_http/configurations/mailer_test.exs new file mode 100644 index 000000000..b0d3a0f88 --- /dev/null +++ b/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 diff --git a/apps/fz_http/test/fz_http/telemetry_test.exs b/apps/fz_http/test/fz_http/telemetry_test.exs index eae3d1617..6a86601c1 100644 --- a/apps/fz_http/test/fz_http/telemetry_test.exs +++ b/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() diff --git a/apps/fz_http/test/fz_http_web/controllers/auth_controller_test.exs b/apps/fz_http/test/fz_http_web/controllers/auth_controller_test.exs index f5425380e..2a9be74a6 100644 --- a/apps/fz_http/test/fz_http_web/controllers/auth_controller_test.exs +++ b/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 diff --git a/apps/fz_http/test/fz_http_web/mailer_test.exs b/apps/fz_http/test/fz_http_web/mailer_test.exs index a3ebb691e..d3d330b0d 100644 --- a/apps/fz_http/test/fz_http_web/mailer_test.exs +++ b/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" ] diff --git a/config/runtime.exs b/config/runtime.exs index 4298f372f..8bf6988e7 100644 --- a/config/runtime.exs +++ b/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 =