Add support for Multi Factor Authentication using TOTP by pulkit110 · Pull Request #2229 · nerves-hub/nerves_hub_web

pulkit110 · 2025-07-24T09:32:52Z

This PR allows users to configure multi factor authentication using the TOTP method using NimbleTOTP under the hood.

Here are some screenshots:

User Account:

Login:

joshk · 2025-07-25T00:50:46Z

Hi @sapandiwakar and @pulkit110

Thanks for the PR!

I've had a quick look over it and things look very good. I'll give it a full review shortly.

pulkit110 · 2025-08-15T04:21:47Z

Hi @joshk, thanks for the quick look! Let me know if you need anything from me to help get this ready.

joshk

This is very cool, and heading in a good direction.

I still need to try this out locally for a full review.

One question I have is how this will impact API auth?

joshk · 2025-08-16T21:44:36Z

+    field(:secret, :binary)
+    field(:code, :string, virtual: true)
+
+    belongs_to(:user, NervesHub.Accounts.User)


could you please alias NervesHub.Accounts.User and then change this to just User

joshk · 2025-08-16T21:45:45Z

+      |> validate_required([:code])
+      |> validate_format(:code, ~r/^\d{6}$/, message: "should be a 6 digit number")
+
+    code = Ecto.Changeset.get_field(changeset, :code)


since you have imported Ecto.Changeset, you can use get_field without the module

joshk · 2025-08-16T21:45:54Z

+    code = Ecto.Changeset.get_field(changeset, :code)
+
+    if changeset.valid? and not valid_totp?(totp, code) do
+      Ecto.Changeset.add_error(changeset, :code, "invalid code")


same as above

joshk · 2025-08-16T21:49:35Z

@@ -0,0 +1,96 @@
+defmodule NervesHub.MFA do


I think we should move this to NervesHub.Accounts.MFA

joshk · 2025-08-16T22:10:26Z

+  The MFA context.
+  """
+
+  import Ecto.Query, warn: false


I don't think the warn: false is needed

joshk · 2025-08-16T22:21:14Z

+  """
+  def regenerate_user_totp_backup_codes(totp) do
+    {:ok, updated_totp} =
+      Repo.transaction(fn ->


transaction is no longer recommended for use, transact is the preferred option.
(I also realize that we use transaction everywhere)

joshk · 2025-08-16T22:27:35Z

+    user_id = get_session(conn, :mfa_user_id)
+
+    if user_id do
+      form = Phoenix.Component.to_form(%{}, as: "mfa")


I believe you can just use to_form

Thanks, but it doesn't seem to work without it - controllers don't have a Phoenix.Component import. It is also being used here:

nerves_hub_web/lib/nerves_hub_web/controllers/session_controller.ex

Line 12 in 4c6d828

form = Phoenix.Component.to_form(%{}, as: "User")

Ah, ok, thanks for checking.

joshk · 2025-08-16T22:37:25Z

+      timestamps()
+    end
+
+    create index(:user_totps, [:user_id])


should this be a unique index?

joshk · 2025-08-16T22:40:59Z

@@ -0,0 +1,95 @@
+defmodule NervesHubWeb.MFAControllerTest do


We use https://hexdocs.pm/phoenix_test/PhoenixTest.html where possible. Could you please look at our other controller tests, like OAuthControllerTest, switch to this setup.

I just noticed that some of your view tests do use PhoenixTest, is there a reason you aren't using it here?

We noticed you were using PhoenixTest for the view tests, but the controller tests had already been prepared earlier. We'll update them.

Awesome, thank you!

joshk · 2025-08-16T22:50:40Z

+    user_totp = MFA.get_user_totp(user)
+
+    if user_totp do
+      MFA.delete_user_totp(user_totp)


Since delete_user_totp uses Repo.delete! under the hood, if there is an error it will bubble up and cause the LiveView to crash, and not displaying any sort of error message to the user.

I would suggest not using the ! variety of delete, and handle this use case.

joshk · 2025-08-16T23:45:45Z

Oh, and final review comment, could we update the MFA UI to use a more PIN style UI, eg. https://preline.co/docs/pin-input.html

CR feedback

fix credo warnings

Other minor changes related to MFA

pulkit110 · 2025-08-18T11:46:01Z

Thanks for the detailed review, @joshk. We’ve addressed all your review points except the last one (MFA UI using a more PIN-style UI), since we have a few questions:

Please correct me if I’m wrong, but it seems the project has both the legacy Bootstrap-based UI and the new Tailwind-based UI. However, the user settings are still only in Bootstrap for now. Is it okay to use the PIN-style UI only in the Tailwind-based views (login flow)?
So far, we’ve avoided adding external libraries on the UI side because we weren’t sure if that would be acceptable. Is it okay to include preline, as you suggested? I think the UI library will be important to support a good UX with the PIN UI (pasting OTPs, auto-advancing to the next input, auto-back on backspace, etc.).

joshk · 2025-08-18T20:50:45Z

Hey @pulkit110

Thanks for your awesome work on the PR and addressing our feedback.

1. Please correct me if I’m wrong, but it seems the project has both the legacy Bootstrap-based UI and the new Tailwind-based UI. However, the user settings are still only in Bootstrap for now. Is it okay to use the PIN-style UI only in the Tailwind-based views (login flow)?

Yep, that is more than perfect. We will be switching the other UI over soon, but its the login flow that I care most about.

2. So far, we’ve avoided adding external libraries on the UI side because we weren’t sure if that would be acceptable. Is it okay to include `preline`, as you suggested? I think the UI library will be important to support a good UX with the PIN UI (pasting OTPs, auto-advancing to the next input, auto-back on backspace, etc.).

Although I was using preline as an example, it was more for the UI than a recommendation to include the library. I'm hesitant to include preline if its just for this one thing. Are you able to look into 'copying' some of the sugar it provides and LiveView-ize it? You can then comment here on the pros and cons of this approach and we can decide on the best path forward.

You are a ⭐

pulkit110 · 2025-08-19T10:55:57Z

Thanks for your thoughtful feedback and kind words @joshk.
We’ve updated the PR to use a PIN input without adding any dependencies.

Here are some screenshots:

joshk · 2025-08-20T01:22:11Z

@sapandiwakar I'll give this a full review shortly, but I wanted to say that the UI change is awesome!

One question we still need to address is what this means for API/CLI auth?

pulkit110 · 2025-08-20T02:53:40Z

Ah, yes. Forgot to answer that. MFA is not enforced on API side. I think that's the standard practice as well when token based with is used. Please feel free to let us know if you'd like to change something on this side.

pulkit110 force-pushed the main branch from f417ba8 to 4c6d828 Compare July 24, 2025 10:21

joshk self-assigned this Jul 25, 2025

joshk requested changes Aug 16, 2025

View reviewed changes

sapandiwakar and others added 4 commits August 18, 2025 12:45

Add TOTP and allow users to configure MFA

9e9e52b

CR feedback

Support authentication with MFA enabled

eba3ed5

fix credo warnings

Add words for cspell

e047235

Refactor(MFA): Integrate MFA into Accounts context based on PR comments

ae1e789

Other minor changes related to MFA

pulkit110 force-pushed the main branch from 658f60a to ae1e789 Compare August 18, 2025 11:08

Migrate mfa_controller_test to PhoenixTest

bff9eaa

Use pin style inputs for MFA on login page

b915107

sapandiwakar force-pushed the main branch from 63b2af4 to b915107 Compare August 19, 2025 10:56

Uh oh!

Conversation

pulkit110 commented Jul 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joshk commented Jul 25, 2025

Uh oh!

pulkit110 commented Aug 15, 2025

Uh oh!

joshk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joshk commented Aug 16, 2025

Uh oh!

pulkit110 commented Aug 18, 2025

Uh oh!

joshk commented Aug 18, 2025

Uh oh!

pulkit110 commented Aug 19, 2025

Uh oh!

joshk commented Aug 20, 2025

Uh oh!

pulkit110 commented Aug 20, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pulkit110 commented Jul 24, 2025 •

edited

Loading