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

Add 2FA to the phx.gen.auth generator #5859

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

RobinBoers
Copy link

@RobinBoers RobinBoers commented Jul 1, 2024

Hi! I’ve been working on adding 2FA to the phx.gen.auth generator and wondered whether I could maybe contribute my changes back to the upstream. I already posted about it on the Elixir forum a few weeks back, but I didn't get much feedback there.

My fork currently has:

  • Scannable QR code on the settings page (can be used with any authenticator app).
  • Codes can only be used once (as required by the spec).
  • Checks account for differences in time between devices and slow typing, by also allowing 30 secs before and after expiration.

Feedback is greatly appreciated! I’m looking for feedback on code quality, proper style and security. In particular, I have a few questions:

  • Is the way the User module currently uses the Accounts module okay?
  • I’m manually Base64 {en,de}coding the secret before storing it using Ecto. Is there a better way to do this?
  • Is the way I’ve implemented this secure? To me it seems pretty solid, but I’m not a security expert.
  • I noticed the hashing library in the generator is dynamic. Is it desirable to also make the TOTP and QR-code generator libraries dynamic like that?

I haven't written any tests yet, but am willing to do that if it's required to get it merged.

Edit:

Here's a demo repo generated like this:

cd installer
mix phx.new dev_app --dev
cd dev_app
mix phx.gen.auth Accounts User users

Edit 2

@dvic said I should post screenshots, so here's a few:

Screenshot 2024-07-01 at 11 35 15
Screenshot 2024-07-01 at 11 37 00

@janwillemvd
Copy link
Contributor

Nice work @RobinBoers!

@wtcross
Copy link

wtcross commented Sep 20, 2024

Great work. I think it would be best to make this feature optional and disabled by default. It would be nice to have a flag (ex: --with-totp) on the phx.gen.auth task.

@wtcross
Copy link

wtcross commented Sep 20, 2024

One more bit of feedback. TOTP is probably what most people want, but HOTP is valid in a lot of scenarios. I'd use "totp" in file names and references to the implementation instead of "second-factor". This great code you have written could be one of many factors (not just 2nd).

@wtcross
Copy link

wtcross commented Sep 20, 2024

If this PR is not accepted into Phoenix (I think it's a good idea to accept it) my suggestion would be to make this its own repo. I suspect it would still see adoption and it doesn't seem very tightly coupled with the rest of the phx.gen.auth code.

@mughees936
Copy link

This is awesome! I was actually about to start working on something similar myself, so it’s great to see you've already put on some good work. I’d love to help out if I can!

Just a few quick thoughts on your questions:

  • User module using Accounts seems like a good idea to keep the logic in the Accounts context. It keeps things clean and fits the Phoenix pattern, so I’d say stick with that.

  • What you're doing for encoding/decoding Base64 makes sense! One suggestion though—you could create a custom Ecto type to handle the Base64 encoding/decoding automatically, just to keep things neat and avoid manual handling.

  • Sticking to one solid library for now is probably the best approach unless there’s a strong use case for more flexibility.

I’d be happy to pitch in. Let me know if I can help—this would be a cool feature to get merged!

@RobinBoers
Copy link
Author

RobinBoers commented Sep 21, 2024

Thanks for the feedback guys! I've renamed all references to second_factor, otp and 2fa, to totp, as you suggested. I also moved the base64 {en,de}coding to a custom Ecto type (I couldn't find an existing implementation, so I wrote my own).

I fully agree that this should be behind a feature flag, but I first wanted to know if the 2FA implementation was wanted and whether it was technically sound. It might take me a while to figure out how to add it, because the phx.gen.auth generator is pretty complicated. Probably somewhere next week.

@mughees936 Thanks for the offer! I'll let you know if I need help with something.

@ponychicken
Copy link
Contributor

Since passkeys are now quickly getting adopted in a variety of browser, I'm wondering if that would not be the more future proof approach...

@RobinBoers
Copy link
Author

Took a bit longer than expected, but I moved everything behind a feature flag. I couldn't figure out how to add a CLI argument, so it's only a prompt right now. Next would be tests I think, but I'm not very good at writing those. Help appreciated!

@shiroyasha
Copy link

@RobinBoers wow, this is awesome! I was just looking around for a solution to add 2fa to our app and people from ElixirForums sent me this way. This could be just what we need.

Do you need any extra help testing it out, or feedback about usage? I'm happy to help out in any way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants