Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: block access to admin pages when 2fa is disabled #667

Merged
merged 2 commits into from
Aug 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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