dwightwatson / validating

Automatically validating Eloquent models for Laravel
MIT License
968 stars 76 forks source link

Laravel Logout Functionality Causes User Model Save #46

Closed mikebronner closed 10 years ago

mikebronner commented 10 years ago

Laravel's built-in logout functionality that is part of the Auth system (Auth::logout()), triggers a save on the user model, which inserts the remember_token.

I don't believe there is any way for me to intercept this save, and have to do forceSave instead, so I would propose one of two:

  1. Not to intercept the default save() function of Eloquent, but rather introduce a new validateAndSave() function to allow validation to be explicitly called. I think there are too many risks with intercepting framework methods that may change behavior in the future, or may change behavior for different database layers, i.e. using it with Laravel-MongoDB could prove problematic. (This is my preferred choice.)
  2. Add new pre-defined rulesets login and logout that will be used accordingly, and could also handle the password confirmation issue in the future.
mikebronner commented 10 years ago

Temporary work-around in SessionsController is the following:

    public function destroy($id = null)
    {
        Event::listen('validating.*', function($model) {
            return false;
        });
        Auth::logout();

        return Redirect::home();
    }
dwightwatson commented 10 years ago

Couldn't you hook in to auth.logout? But I don't think I'm understanding exactly - what is the problem with Auth::logout() performing a save? A remember_token isn't going to make your model invalid (assuming you haven't stuffed your rules) so it should always save properly.

  1. I don't believe this is in the best interests of this package - my goal with this was to introduce valid models as the default. I believe that anything that is being saved should be valid to maintain the integrity of your data. Once you make it not the default there is plenty of potential for someone on your team to forget and just do a normal save() or not even realise that a validation trait is being used in the project.
  2. This is a little bit tricky because different authentication modules may use different events or not even fire events at all. We then have to add support for Laravel's Auth, Sentry and whatever other major authentication packages there are for Laravel and maintain it too. Plus I think it is beyond the reach of this package. Validation only and keep clear of everything else.
mikebronner commented 10 years ago

Hi Dwight, thanks for the reply.

  1. I can see the merits of this methodology.
  2. Makes sense.

The reason this is failing for me is because of the password confirmation rule I have set in the 'saving' ruleset:

    protected $rulesets = [
        'saving' => [
            'email' => 'required|unique:users,email',
            'password' => 'required|confirmed',
            'username' => 'required',
        ],
        'sanityCheck' => [
            'email' => 'required',
            'password' => 'required',
            'username' => 'required',
        ],
    ];

And then my UsersController has this to work with it:

        if (strlen(Input::get('password')) > 0) {
            $user->password = Input::get('password');
            $user->password_confirmation = Input::get('password_confirmation');
        } else {
            $user->password_confirmation = $user->password;
        }
        $user->isValidOrFail();
        if (strlen(Input::get('password')) > 0) {
            $user->password = Hash::make( $user->password );
        }
        unset($user->password_confirmation);
        $user->forceSave();

Your isValidOrFail() update is working like a charm, thanks very much for that, by the way. :) I guess maybe this issue is more related to the password confirmation feature, rather than being an issue of its own (issue #19). I'm happy to disable validation in the logout routine until password confirmations get sorted out. :)

Will the following re-enable validations?

        Event::listen('validating.*', function($model) {
            return true;
        });

If so, then I'll add that back in right after the Auth::logout() to minimize security risks, like so:

    public function destroy($id = null)
    {
        Event::listen('validating.*', function($model) {
            return false;
        });
        Auth::logout();
        Event::listen('validating.*', function($model) {
            return true;
        });

        return Redirect::home();
    }
dwightwatson commented 10 years ago

Ah, I see what you mean. This is definitely more related to the other issue, so I'll start looking into the next version.