laravel / jetstream

Tailwind scaffolding for the Laravel framework.
https://jetstream.laravel.com
MIT License
3.95k stars 809 forks source link

Logout Other Browser Sessions does not observe custom Fortify logic for passwords #780

Closed wlius-support3 closed 3 years ago

wlius-support3 commented 3 years ago

Description:

When trying to use the Logout Other Browser Sessions function, I discovered that both the LogoutOtherBrowserSessionsForm class, and the Illuminate\Auth\SessionGuard assume the Hash::check($password, Auth::user()->password) function call is sufficient for checking that a user's password is accurate, despite Laravel Fortify allowing custom logic for authenticating and confirming passwords.

Steps To Reproduce:

In the App\Providers\FortifyServiceProvider, set a custom password confirmer/authenticator:

Fortify::authenticateUsing(function (Request $request) {
    $user = User::where('email', $request->email)->first();

    $pepper = '1';

    if ($user &&
        Hash::check($pepper.$request->password, $user->password)) {
        return $user;
    }
});
Fortify::confirmPasswordsUsing(function (User $user, $password) {
    $user = User::where('email', $user->email)->first();

    $pepper = '1';

    if ($user &&
        Hash::check($pepper.$password, $user->password)) {
        return $user;
    }
});

In App\Actions\Fortify\CreateNewUser, set custom logic for storing passwords:

public function create(array $input)
{
    Validator::make($input, [
        'name' => ['required', 'string', 'max:255'],
        'email' => ['required', 'string', 'email', 'max:255', 'unique:users'],
        'password' => $this->passwordRules(),
    ])->validate();

    $pepper = '1';

    return DB::transaction(function () use ($input, $pepper) {
        return tap(User::create([
            'name' => $input['name'],
            'email' => $input['email'],
            'password' => Hash::make($pepper.$input['password']),
        ]), function (User $user) {
            $this->createTeam($user);
        });
    });
}

Then create a user, log in, and attempt to log out other browsers.

Why this happens:

The issue comes from the function logoutOtherBrowserSessions(StatefulGuard $guard) from Laravel\Jetstream\Http\Livewire\LogoutOtherBrowserSessionsForm class assuming that just a regular password Hash check will confirm the user's password. See these lines of the function:

if (! Hash::check($this->password, Auth::user()->password)) {
    throw ValidationException::withMessages([
        'password' => [__('This password does not match our records.')],
    ]);
}

At first I thought you could just change the if statement to use the ConfirmPassword class, like so:

if (! app(ConfirmPassword::class)($guard, Auth::user(), $this->password)) {

However, the issue then comes up again when calling:

$guard->logoutOtherDevices($this->password);

This is because the same assumption about how passwords are stored is built into Illuminate\Auth\SessionGuard

How to resolve:

Update the logic in logoutOtherBrowserSessions(StatefulGuard $guard) from the Laravel\Jetstream\Http\Livewire\LogoutOtherBrowserSessionsForm class and Illuminate\Auth\SessionGuard's logoutOtherDevices($password) to observe the custom logic for passwords defined in the App\Providers\FortifyServiceProvider.

driesvints commented 3 years ago

I'll talk this over with the team.

taylorotwell commented 3 years ago

Feel free to send in a PR if you wish.

rginnow commented 3 years ago

Unless I'm missing something obvious, I'd also like to add that this doesn't allow for customization of the name of the password field - for instance, my app uses the term passphrase in my DB.

I can override it in my User model by saying:

/**
     * Get the password for the user.
     *
     * @return string
     */
    public function getAuthPassword()
    {
        return $this->passphrase;
    }

but this function doesn't seem to get used anywhere else.

Is there any other option so I don't have to go change each instance of Auth::user()->password or $user->password?

driesvints commented 3 years ago

@wlius-support3 so I've been investigating this and you can actually fully customize everything at the moment. You can override the session guard class and register your own one in config/auth.php and you can overwrite the LogoutOtherBrowserSessionsForm in your AppServiceProvider's boot method:

use App\Http\Livewire\ LogoutOtherBrowserSessionsForm;

/**
 * Bootstrap any application services.
 *
 * @return void
 */
public function boot()
{
    Livewire::component('profile.logout-other-browser-sessions-form', LogoutOtherBrowserSessionsForm::class);
}

Then you may overwrite the methods that don't work for you and implement any behavior your like. Ideally I agree that the LogoutOtherBrowserSessionsForm class and the session guard would be a bit "smarter" but I'm not sure what the best path for that is. We're always open to PR's so feel free to attempt one if you like.

@rginnow Auth::user()->password can probably indeed be swapped out with Auth::user()-> getAuthPassword(). As for the $this->password calls in the Livewire components, those will need to be adjusted and you can pass the second parameter for the ->logoutOtherDevices call to adjust it to your custom key.

wlius-support3 commented 3 years ago

@driesvints Thank you. That was very helpful.

I was able to take your suggestions and make the changes needed in my app to get everything working. I will post them here in case anyone in the future finds this thread:

In App\Providers\AuthServiceProvider (perhaps it belongs in the AppServiceProvider instead, but it seems to work either way), I added the following lines to the boot() method:

Auth::extend(
    'sessionExtended',
    function ($app) {
        $provider = new EloquentUserProvider($app['hash'], config('auth.providers.users.model'));
        $sessionGuardExtended = new SessionGuardExtended('sessionExtended', $provider, app()->make('session.store'), request());
        if (method_exists($sessionGuardExtended, 'setCookieJar')) {
            $sessionGuardExtended->setCookieJar($this->app['cookie']);
        }

        if (method_exists($sessionGuardExtended, 'setDispatcher')) {
            $sessionGuardExtended->setDispatcher($this->app['events']);
        }

        if (method_exists($sessionGuardExtended, 'setRequest')) {
            $sessionGuardExtended->setRequest($this->app->refresh('request', $sessionGuardExtended, 'setRequest'));
        }

        return $sessionGuardExtended;
    }
);

Then I changed my config/auth.php to set the guards array like so (to use this new sessionExtended driver):

'guards' => [
    'web' => [
        'driver' => 'sessionExtended',
        'provider' => 'users',
    ],
    // ...

The following class is the App\Auth\SessionGuardExtended class that overrides the rehashUserPassword method:

<?php

namespace App\Auth;

use App\Actions\Fortify\ResetUserPassword;
use Illuminate\Auth\SessionGuard;
use Illuminate\Support\Facades\Auth;
use InvalidArgumentException;
use Laravel\Fortify\Actions\ConfirmPassword;

class SessionGuardExtended extends SessionGuard
{
    /**
     * Rehash the current user's password.
     *
     * @param  string  $password
     * @param  string  $attribute
     * @return bool|null
     *
     * @throws \InvalidArgumentException
     */
    protected function rehashUserPassword($password, $attribute)
    {
        if (! app(ConfirmPassword::class)($this, Auth::user(), $password)) {
            throw new InvalidArgumentException('The given password does not match the current password.');
        }

        $reset_password = new ResetUserPassword();

        return $reset_password->reset(Auth::user(), [
            'password' => $password,
            'password_confirmation' => $password,
        ]);
    }
}

As Dries suggested, I also updated the App\Providers\AppServiceProvider and added the following to the boot method:

Livewire::component('profile.logout-other-browser-sessions-form', LogoutOtherBrowserSessionsForm::class);

And I made the following class, App\Http\Livewire\Override\LogoutOtherBrowserSessionsForm to override the original:

<?php

namespace App\Http\Livewire\Override;

use Illuminate\Contracts\Auth\StatefulGuard;
use Illuminate\Support\Facades\Auth;
use Illuminate\Validation\ValidationException;
use Laravel\Fortify\Actions\ConfirmPassword;
use Laravel\Jetstream\Http\Livewire\LogoutOtherBrowserSessionsForm as JetstreamLogoutOtherBrowsers;

class LogoutOtherBrowserSessionsForm extends JetstreamLogoutOtherBrowsers
{

    /**
     * Logout from other browser sessions.
     *
     * @param  \Illuminate\Contracts\Auth\StatefulGuard  $guard
     * @return void
     */
    public function logoutOtherBrowserSessions(StatefulGuard $guard)
    {
        $this->resetErrorBag();

        if (! app(ConfirmPassword::class)($guard, Auth::user(), $this->password)) {
            throw ValidationException::withMessages([
                'password' => [__('This password does not match our records.')],
            ]);
        }

        $guard->logoutOtherDevices($this->password);

        $this->deleteOtherSessionRecords();

        $this->confirmingLogout = false;

        $this->emit('loggedOut');
    }
}