-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
base: main
Are you sure you want to change the base?
Conversation
Nice work @RobinBoers! |
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: |
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). |
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 |
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:
I’d be happy to pitch in. Let me know if I can help—this would be a cool feature to get merged! |
Thanks for the feedback guys! I've renamed all references to 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 @mughees936 Thanks for the offer! I'll let you know if I need help with something. |
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... |
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! |
@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. |
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:
Feedback is greatly appreciated! I’m looking for feedback on code quality, proper style and security. In particular, I have a few questions:
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:
Edit 2
@dvic said I should post screenshots, so here's a few: