Skip to content

Conversation

pushpak1300
Copy link
Member

@pushpak1300 pushpak1300 commented Aug 26, 2025

This MR adds Two-Factor Authentication (2FA) functionality to the React Starter Kit using Laravel Fortify's built-in support for 2FA

🔒 Backend Changes

We are now leveraging Fortify for handling two-factor authentication. Since Fortify provides additional security features out of the box, this MR also replaces our custom implementation of the password confirmation page with Fortify’s native confirm password functionality.

Previously: Custom password confirmation
Now: Native Fortify confirm-password implementation

🖥️ Frontend/UI Updates

The UI has been updated to handle all the two-factor settings based on Fortify's configuration. The following Fortify features are enabled:

Features::twoFactorAuthentication([
    'confirm' => true,
    'confirmPassword' => true,
    // 'window' => 0
]),

Demo

https://www.loom.com/share/6bf836eccae84778a511916a9d02bb59?sid=efb1fac2-7482-40ec-82e6-31f0b3fa77a8

Co-authored-by: Tony Lea [email protected]
Thanks to #101

@ludo237
Copy link

ludo237 commented Aug 26, 2025

I think this is better be its own starter kit

# Conflicts:
#	routes/auth.php
#	tests/Feature/Auth/AuthenticationTest.php
#	tests/Feature/Auth/PasswordConfirmationTest.php
@pushpak1300 pushpak1300 marked this pull request as ready for review September 1, 2025 09:28
@pushpak1300 pushpak1300 marked this pull request as draft September 1, 2025 11:42
@pushpak1300 pushpak1300 marked this pull request as ready for review September 1, 2025 14:19
@pushpak1300 pushpak1300 marked this pull request as draft September 3, 2025 04:05
@pushpak1300
Copy link
Member Author

Waiting until we fix this bug in the Inertia <Form> component inertiajs/inertia#2558, as it will clean up a few unnecessary parts.

@OliverSpeak
Copy link

Why is this using Fortify? I don't see how it makes sense to use part of that package when the rest of the authentication is already handled by open code.

@Diddyy
Copy link

Diddyy commented Sep 3, 2025

Waiting until we fix this bug in the Inertia <Form> component inertiajs/inertia#2558, as it will clean up a few unnecessary parts.

Its been merged!

@pushpak1300
Copy link
Member Author

Its been merged!

Hey It's needs to be tagged with new release in order to use it. I'm tracking it and will push changes once tagged.

@ludo237
Copy link

ludo237 commented Sep 3, 2025

Why is this using Fortify? I don't see how it makes sense to use part of that package when the rest of the authentication is already handled by open code.

Don't bother they won't listen. Just wait for a maintainer to close this

@pushpak1300 pushpak1300 marked this pull request as ready for review September 3, 2025 15:32
@tnylea
Copy link
Contributor

tnylea commented Sep 4, 2025

Thanks for the shout out @pushpak1300 😁

Excited to see this released 👍

@joetannenbaum
Copy link
Contributor

Why is this using Fortify? I don't see how it makes sense to use part of that package when the rest of the authentication is already handled by open code.

We decided to use Fortify because it's battle tested and the likelihood of anyone changing the underlying code (not the views, which are open code) are very slim. We did not want to burden the user with maintaining this code, we instead opted for a composer update for bug fixes as the come up.

@joetannenbaum
Copy link
Contributor

Why is this using Fortify? I don't see how it makes sense to use part of that package when the rest of the authentication is already handled by open code.

Don't bother they won't listen. Just wait for a maintainer to close this

Sorry you feel this way. This PR will be merged once it's ready.

Copy link
Contributor

@joetannenbaum joetannenbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did a first pass here, I haven't run it locally, just looked at the code. I'll spin it up in the next round and we'll keep refining 👍

Copy link
Contributor

@joetannenbaum joetannenbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly minor stuff. The "negative first" comments are mostly just about reducing cognitive overhead when looking at code. There are times when negative-first makes the most sense, but in most cases here I would argue that it's clearer to read when going from true to false.

The exception being, in my book, that if the statements of one of the conditions are vastly shorter than the other, that one should go first so it's easier to track the if statement through to the end.

@Diddyy
Copy link

Diddyy commented Sep 9, 2025

I'm loving following these updates, @joetannenbaum is helping me learn, I really enjoy watching this process in public.
Looking forward to the release guys! Great work :D


setQrCodeSvg(svg);
} catch (error) {
console.error('Failed to fetch QR code:', error);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of console.error can we actually float these messages up to the UI somehow so that we display them to the user? We shouldn't be console.erroring at this level.

Even just error(s) state that gets returned from the hook or something

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