laravel / fortify

Backend controllers and scaffolding for Laravel authentication.
https://laravel.com/docs/fortify
MIT License
1.61k stars 294 forks source link

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

Closed LeoniePhiline closed 3 years ago

LeoniePhiline commented 3 years ago

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 commented 3 years ago

Thanks for the report! I've sent in a PR here: https://github.com/laravel/fortify/pull/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 commented 3 years ago

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 commented 3 years ago

Made https://github.com/laravel/fortify/pull/206 - let's see if this is welcome. :)