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

Fire Failed events #154

Merged
merged 2 commits into from
Nov 21, 2020
Merged

Fire Failed events #154

merged 2 commits into from
Nov 21, 2020

Conversation

rodrigopedra
Copy link
Contributor

This PR attempts to address the issue described in #145

That issue states the Failed event is not being dispatched when a user provides faulty credentials. From my assessment this happens when 2-Factor Auth is enabled, or when the developer uses the Fortify::$authenticateUsingCallback feature.

Reason I found is the Failed event is only dispatched inside the \Illuminate\Auth\SessionGuard@attempt method after a failed login attempt.

As the RedirectIfTwoFactorAuthenticatable action is executed before the AttemptToAuthenticate in the default Fortify's login pipeline, when a user provides faulty credentials the \Illuminate\Auth\SessionGuard@attempt is not executed, and thus the Failed event never gets dispatched.

I added the code to dispatch the Failed event in 3 spots:

  1. From the AttemptToAuthenticate@handleUsingCustomCallback method, as it skips using the \Illuminate\Auth\SessionGuard@attempt, but later uses the guard's login method, so I thought in increased consistency.
  2. From the RedirectIfTwoFactorAuthenticatable@validateCredentials inside the first if clause, when the developer configured a Fortify::$authenticateUsingCallback.
  3. From the RedirectIfTwoFactorAuthenticatable@validateCredentials inside the return callback, executed when the developer did NOT configured a Fortify::$authenticateUsingCallback.

In the RedirectIfTwoFactorAuthenticatable action I considered dispatching the Failed event from within the throwFailedAuthenticationException method. But I decided to keep it separated to increase consistency between the RedirectIfTwoFactorAuthenticatable and AttemptToAuthenticate actions code.

Let me know if you prefer to have it consolidated inside that method.

As there no tests to assess the other events dispatched by Fortify (Registered, PasswordReset, Lockout, and Verified events), I did not add tests for checking if the Failed event gets dispatched.

@taylorotwell taylorotwell merged commit 66323c4 into laravel:1.x Nov 21, 2020
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.

None yet

2 participants