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

Route /user/confirmed-password-status only created if ($enableViews) despite returning JSON. #200

Closed
LeoniePhiline opened this issue Jan 17, 2021 · 3 comments · Fixed by #203
Labels

Comments

@LeoniePhiline
Copy link

  • Fortify Version: v1.7.4
  • Laravel Version: v8.22.1
  • PHP Version: v7.4.13

Description:

The /user/confirmed-password-status route is only created if ($enableViews) in https://github.com/laravel/fortify/blob/1.x/routes/routes.php#L113.

Actually, though, \Laravel\Fortify\Http\Controllers\ConfirmedPasswordStatusController::show() returns JSON - and is really only useful in an SPA (therefore: ! $enableViews).

Of course anyone can create this route themselves, but it looks a bit like the declaration was moved inside the if block accidentally and should be fixed.

    // Password Confirmation...
    if ($enableViews) {
        Route::get('/user/confirm-password', [ConfirmablePasswordController::class, 'show'])
            ->middleware(['auth'])
            ->name('password.confirm');

        // This route looks like it got into this `if` block by accident.
        Route::get('/user/confirmed-password-status', [ConfirmedPasswordStatusController::class, 'show'])
            ->middleware(['auth'])
            ->name('password.confirmation');
    }

    Route::post('/user/confirm-password', [ConfirmablePasswordController::class, 'store'])
        ->middleware(['auth']);

Do you agree or am I getting this wrong?

I can see that https://laravel.com/docs/8.x/fortify does not mention /user/confirmed-password-status, so maybe the feature is just a left-over.
It seems useful to me, though: Writing an SPA i'd want to check if the password confirmation timeout has passed before even asking to re-type the password.
I fail to see why this should be enabled if you are using views. In that case you would much rather redirect to a password confirmation route (GET /user/confirm-password) if needed, before performing the

Fix:

    // Password Confirmation...
    if ($enableViews) {
        Route::get('/user/confirm-password', [ConfirmablePasswordController::class, 'show'])
            ->middleware(['auth'])
            ->name('password.confirm');
    }

    Route::get('/user/confirmed-password-status', [ConfirmedPasswordStatusController::class, 'show'])
        ->middleware(['auth'])
        ->name('password.confirmation');

    Route::post('/user/confirm-password', [ConfirmablePasswordController::class, 'store'])
        ->middleware(['auth']);

Generally, would you like to see pull requests, or have contributers first ask if a PR is desired?


PS:
\Laravel\Fortify\Http\Controllers\ConfirmablePasswordController::store() might want to use \Illuminate\Session\Store::passwordConfirmed() instead of manually calling $request->session()->put('auth.password_confirmed_at', time()).

@driesvints
Copy link
Member

Thanks for the report! I've sent in a PR here: #203

Generally, would you like to see pull requests, or have contributers first ask if a PR is desired?

PRs are always preferred over issues because then we can look at actual code.

@driesvints driesvints added the bug label Jan 19, 2021
@driesvints
Copy link
Member

PS: \Laravel\Fortify\Http\Controllers\ConfirmablePasswordController::store() might want to use \Illuminate\Session\Store::passwordConfirmed() instead of manually calling $request->session()->put('auth.password_confirmed_at', time()).

Just send in a PR if you like, thanks!

@LeoniePhiline
Copy link
Author

Made #206 - let's see if this is welcome. :)

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

Successfully merging a pull request may close this issue.

2 participants