fortify icon indicating copy to clipboard operation
fortify copied to clipboard

RecoveryCodeReplaced dispatched twice when two-factor.login receive a valid recovery code

Open joke2k opened this issue 11 months ago • 5 comments

Fortify Version

1.25.3

Laravel Version

10.48.27

PHP Version

8.1

Database Driver & Version

No response

Description

The TwoFactorAuthenticatedSessionController dispatches a RecoveryCodeReplaced event https://github.com/laravel/fortify/blob/17112764d9f6a81a002bddfc3e305e73f3d19b85/src/Http/Controllers/TwoFactorAuthenticatedSessionController.php#L57-L64

but User::replaceRecoveryCode already dispatches the exact same event https://github.com/laravel/fortify/blob/17112764d9f6a81a002bddfc3e305e73f3d19b85/src/TwoFactorAuthenticatable.php#L47-L57

I’m not sure where it would make the most sense to keep the dispatch, but I think it’s reasonable to avoid triggering it twice.

Steps To Reproduce

public function test_two_factor_challenge_with_recovery_code_dispatches_RecoveryCodeReplaced_twice()
    {
        Event::fake();

        $user = UserWithTwoFactor::forceCreate([
            'name' => 'Taylor Otwell',
            'email' => '[email protected]',
            'password' => bcrypt('secret'),
            'two_factor_recovery_codes' => encrypt(json_encode(['invalid-code', 'valid-code'])),
        ]);

        $response = $this->withSession([
            'login.id' => $user->id,
            'login.remember' => false,
        ])->withoutExceptionHandling()->post('/two-factor-challenge', [
            'recovery_code' => 'valid-code',
        ]);

        Event::assertDispatchedTimes(RecoveryCodeReplaced::class, 2);
    }

joke2k avatar Feb 05 '25 15:02 joke2k

@joke2k Can I try to resolve this one ?

developerluanramos avatar Feb 05 '25 19:02 developerluanramos

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

github-actions[bot] avatar Feb 06 '25 00:02 github-actions[bot]

@developerluanramos sure you can consider that recently the dispatch was added to the Authenticable trait by this PR https://github.com/laravel/fortify/pull/564 and all other package events are raised within action classes.

Considering this I would remove revert that PR completely.

joke2k avatar Feb 06 '25 14:02 joke2k

Same issue. Trying to listen for the event to notify my user that a recovery code was replaced. But, the email is being sent twice as the event is dispatched twice.

Unless I am completely missing something I would assume that the event should be raised once and via the controller not the trait.

But any solution to this would be appreciated.

aman-evoenergyio avatar Mar 13 '25 02:03 aman-evoenergyio

TwoFactorAuthenticatedSessionController triggers the event after calling TwoFactorAuthenticatable::replaceRecoveryCode, which ALSO triggers the event.

Arguably, only the latter should be triggering the event, and not TwoFactorAuthenticatedSessionController.

On the other hand, @joke2k makes a good point about consistency, so perhaps reverting #564 would be the proper move.

In either case, there is now a PR to fix it one way, and an option to revert to fix it the other way. Please choose one. Thanks!

coolAlias avatar Nov 06 '25 20:11 coolAlias