From ed3dc89980d81e15b57a5352ea260d6ac2c4aec6 Mon Sep 17 00:00:00 2001 From: Nick Stein Date: Wed, 30 Aug 2023 15:34:22 -0400 Subject: [PATCH] feat: block access to admin pages when 2fa is disabled (#667) * feat: block access to admin pages when 2fa is disabled * wip: fix tests --- .../lib/api_web/plugs/require_2factor.ex | 30 ++++++++++ apps/api_web/lib/api_web/router.ex | 1 + .../client_portal/layout/footer.html.heex | 9 ++- .../admin/accounts/key_controller_test.exs | 6 +- .../admin/accounts/user_controller_test.exs | 9 ++- .../admin/session_controller_test.exs | 1 + .../api_web/plugs/require_2factor_test.exs | 55 +++++++++++++++++++ .../test/api_web/plugs/require_admin_test.exs | 2 +- 8 files changed, 107 insertions(+), 6 deletions(-) create mode 100644 apps/api_web/lib/api_web/plugs/require_2factor.ex create mode 100644 apps/api_web/test/api_web/plugs/require_2factor_test.exs diff --git a/apps/api_web/lib/api_web/plugs/require_2factor.ex b/apps/api_web/lib/api_web/plugs/require_2factor.ex new file mode 100644 index 00000000..d839604b --- /dev/null +++ b/apps/api_web/lib/api_web/plugs/require_2factor.ex @@ -0,0 +1,30 @@ +defmodule ApiWeb.Plugs.Require2Factor do + @moduledoc """ + Plug enforcing a user to have 2fa enabled + """ + + import Phoenix.Controller + + def init(opts), do: opts + + def call(conn, _opts) do + conn + |> fetch_user() + |> authenticate(conn) + end + + defp fetch_user(conn) do + conn.assigns[:user] + end + + defp authenticate(%ApiAccounts.User{totp_enabled: true}, conn), do: conn + + defp authenticate(_, conn) do + conn + |> put_flash( + :error, + "Account does not have 2-Factor Authentication enabled. Please enable before performing administrative tasks." + ) + |> redirect(to: ApiWeb.Router.Helpers.user_path(conn, :configure_2fa)) + end +end diff --git a/apps/api_web/lib/api_web/router.ex b/apps/api_web/lib/api_web/router.ex index 2c8306d4..4a0bd3a1 100644 --- a/apps/api_web/lib/api_web/router.ex +++ b/apps/api_web/lib/api_web/router.ex @@ -60,6 +60,7 @@ defmodule ApiWeb.Router do pipeline :admin do plug(ApiWeb.Plugs.RequireAdmin) + plug(ApiWeb.Plugs.Require2Factor) end pipeline :portal_view do diff --git a/apps/api_web/lib/api_web/templates/client_portal/layout/footer.html.heex b/apps/api_web/lib/api_web/templates/client_portal/layout/footer.html.heex index 290947ec..4ca7829c 100644 --- a/apps/api_web/lib/api_web/templates/client_portal/layout/footer.html.heex +++ b/apps/api_web/lib/api_web/templates/client_portal/layout/footer.html.heex @@ -28,8 +28,13 @@
diff --git a/apps/api_web/test/api_web/controllers/admin/accounts/key_controller_test.exs b/apps/api_web/test/api_web/controllers/admin/accounts/key_controller_test.exs index 535f621f..a185562c 100644 --- a/apps/api_web/test/api_web/controllers/admin/accounts/key_controller_test.exs +++ b/apps/api_web/test/api_web/controllers/admin/accounts/key_controller_test.exs @@ -8,7 +8,11 @@ defmodule ApiWeb.Admin.Accounts.KeyControllerTest do on_exit(fn -> ApiAccounts.Dynamo.delete_all_tables() end) - {:ok, user} = ApiAccounts.create_user(%{email: "test@mbta.com", role: "administrator"}) + {:ok, user} = + ApiAccounts.create_user(%{email: "test@mbta.com", role: "administrator", totp_enabled: true}) + + {:ok, user} = ApiAccounts.generate_totp_secret(user) + ApiAccounts.enable_totp(user, NimbleTOTP.verification_code(user.totp_secret_bin)) conn = conn diff --git a/apps/api_web/test/api_web/controllers/admin/accounts/user_controller_test.exs b/apps/api_web/test/api_web/controllers/admin/accounts/user_controller_test.exs index ac13667e..f8a07232 100644 --- a/apps/api_web/test/api_web/controllers/admin/accounts/user_controller_test.exs +++ b/apps/api_web/test/api_web/controllers/admin/accounts/user_controller_test.exs @@ -49,8 +49,13 @@ defmodule ApiWeb.Admin.Accounts.UserControllerTest do on_exit(fn -> ApiAccounts.Dynamo.delete_all_tables() end) - params = %{email: "admin@mbta.com", role: "administrator"} - {:ok, user} = ApiAccounts.create_user(params) + params = %{email: "admin@mbta.com", role: "administrator", totp_enabled: true} + + {:ok, user} = + ApiAccounts.create_user(%{email: "test@mbta.com", role: "administrator", totp_enabled: true}) + + {:ok, user} = ApiAccounts.generate_totp_secret(user) + ApiAccounts.enable_totp(user, NimbleTOTP.verification_code(user.totp_secret_bin)) conn = conn diff --git a/apps/api_web/test/api_web/controllers/admin/session_controller_test.exs b/apps/api_web/test/api_web/controllers/admin/session_controller_test.exs index af1bbdea..7ff056bd 100644 --- a/apps/api_web/test/api_web/controllers/admin/session_controller_test.exs +++ b/apps/api_web/test/api_web/controllers/admin/session_controller_test.exs @@ -7,6 +7,7 @@ defmodule ApiWeb.Admin.SessionControllerTest do @authorized_user_attrs %{ email: "authorized@mbta.com", role: "administrator", + totp_enabled: true, password: @test_password } @unauthorized_user_attrs %{ diff --git a/apps/api_web/test/api_web/plugs/require_2factor_test.exs b/apps/api_web/test/api_web/plugs/require_2factor_test.exs new file mode 100644 index 00000000..c1ac57cf --- /dev/null +++ b/apps/api_web/test/api_web/plugs/require_2factor_test.exs @@ -0,0 +1,55 @@ +defmodule ApiWeb.Plugs.Require2FactorTest do + use ApiWeb.ConnCase, async: true + + setup %{conn: conn} do + conn = + conn + |> conn_with_session() + |> bypass_through(ApiWeb.Router, [:browser, :admin]) + + {:ok, conn: conn} + end + + test "opts" do + assert ApiWeb.Plugs.Require2Factor.init([]) == [] + end + + describe ":require_2factor plug" do + test "gives 404 with no authenicated user", %{conn: conn} do + conn = get(conn, "/") + assert conn.status == 404 + assert html_response(conn, 404) =~ "not found" + end + + test "gives 404 for user without administrator role", %{conn: conn} do + conn = + conn + |> user_with_role(nil, true) + |> get("/") + + assert html_response(conn, 404) =~ "not found" + end + + test "redirects on missing 2fa, but valid admin account", %{conn: conn} do + conn = + conn + |> user_with_role("administrator", false) + |> get("/") + + assert html_response(conn, 302) + end + + test "allows user with administrator role and 2fa to proceed", %{conn: conn} do + conn = + conn + |> user_with_role("administrator", true) + |> get("/") + + refute conn.status + end + end + + defp user_with_role(conn, role, totp_enabled) do + Plug.Conn.assign(conn, :user, %ApiAccounts.User{role: role, totp_enabled: totp_enabled}) + end +end diff --git a/apps/api_web/test/api_web/plugs/require_admin_test.exs b/apps/api_web/test/api_web/plugs/require_admin_test.exs index f69be2f4..1f1d0396 100644 --- a/apps/api_web/test/api_web/plugs/require_admin_test.exs +++ b/apps/api_web/test/api_web/plugs/require_admin_test.exs @@ -41,6 +41,6 @@ defmodule ApiWeb.Plugs.RequireAdminTest do end defp user_with_role(conn, role) do - Plug.Conn.assign(conn, :user, %ApiAccounts.User{role: role}) + Plug.Conn.assign(conn, :user, %ApiAccounts.User{role: role, totp_enabled: true}) end end