Skip to content

Commit

Permalink
feat: block access to admin pages when 2fa is disabled (#667)
Browse files Browse the repository at this point in the history
* feat: block access to admin pages when 2fa is disabled

* wip: fix tests
  • Loading branch information
nlwstein authored Aug 30, 2023
1 parent fa9bf5a commit ed3dc89
Show file tree
Hide file tree
Showing 8 changed files with 107 additions and 6 deletions.
30 changes: 30 additions & 0 deletions apps/api_web/lib/api_web/plugs/require_2factor.ex
Original file line number Diff line number Diff line change
@@ -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
1 change: 1 addition & 0 deletions apps/api_web/lib/api_web/router.ex
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ defmodule ApiWeb.Router do

pipeline :admin do
plug(ApiWeb.Plugs.RequireAdmin)
plug(ApiWeb.Plugs.Require2Factor)
end

pipeline :portal_view do
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,13 @@
<div class="footer-links">
<label>Portal</label>
<ul>
<li><%= link "Login", to: session_path(@conn, :new) %></li>
<li><%= link "Register", to: user_path(@conn, :new) %></li>
<%= if user = @conn.assigns[:user] do %>
<li><%= link user.email, to: user_path(@conn, :show) %></li>
<li><%= link "Logout", to: session_path(@conn, :delete), method: :delete %></li>
<% else %>
<li><%= link "Login", to: session_path(@conn, :new) %></li>
<li><%= link "Register", to: user_path(@conn, :new) %></li>
<% end %>
<li><%= link "Docs", to: "/docs/swagger" %></li>
</ul>
</div>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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: "[email protected]", role: "administrator"})
{:ok, user} =
ApiAccounts.create_user(%{email: "[email protected]", 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,13 @@ defmodule ApiWeb.Admin.Accounts.UserControllerTest do

on_exit(fn -> ApiAccounts.Dynamo.delete_all_tables() end)

params = %{email: "[email protected]", role: "administrator"}
{:ok, user} = ApiAccounts.create_user(params)
params = %{email: "[email protected]", role: "administrator", totp_enabled: true}

{:ok, user} =
ApiAccounts.create_user(%{email: "[email protected]", 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ defmodule ApiWeb.Admin.SessionControllerTest do
@authorized_user_attrs %{
email: "[email protected]",
role: "administrator",
totp_enabled: true,
password: @test_password
}
@unauthorized_user_attrs %{
Expand Down
55 changes: 55 additions & 0 deletions apps/api_web/test/api_web/plugs/require_2factor_test.exs
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion apps/api_web/test/api_web/plugs/require_admin_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -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

0 comments on commit ed3dc89

Please sign in to comment.