Password Confirmed Not Matching

[mikebronner]

Passwords are always fickle, especially when working with hashed passwords.

I am hashing my password using the following in my Users model:

    public function setPasswordAttribute($password)
    {
        $this->attributes['password'] = Hash::make($password);
    }

When I validate using the following ruleset:

    protected $fillable = [
        'email',
        'password'
    ];

    protected $rules = [
        'creating' => [
            'email' => 'required|email|unique:users,email',
            'password' => 'required|confirmed'
        ]
    ];

I always get passwords don't match. I assume this is because of two things:

1. The password_confirmation field is not saved in the model.
2. The password is already hashed before the validation begins.

Soooo, the big question is: how do you recommend dealing with password validation? :) Thanks!

[decnorton]

It's probably because you're hashing the password in the mutator. Try using model events to Hash the password before saving:

protected $passwordAttributes = ['password'];

public static function boot()
{
    parent::boot();

    static::saving(function ($model) {
        // Prepare attributes
        foreach ($model->attributes as $key => $value)
        {
            // Remove any confirmation fields
            if (ends_with($key, '_confirmation'))
            {
                array_forget($model->attributes, $key);
                continue;
            }

            // Check if this one of our password attributes and if it's been changed.
            if (in_array($key, $model->passwordAttributes) && $value != $model->getOriginal($key))
            {
                // Hash it
                $model->attributes[$key] = Hash::make($value);
                continue;
            }
        }
    });
}

[decnorton]

The above won't actually work as the saving event always seems to be called before the ValidatingObserver. We'll have to wait until #9 is ready!

[dwightwatson]

This is an interesting one. Because password_confirmation is not in your $fillable array, it never makes it into the model attributes and thus doesn't get into the Validator. Plus, putting password_confirmation in $fillable means you just compare the hash with the plaintext password.

I'm not a huge fan of hooking into the saving event to hash a password either as I feel that belongs in a mutator.

I'm going to sit on this one and have a think about it for now as it would be nice to be able to handle this, open to any ideas on a simple solution!

[jenssegers]

@decnorton In my project I actually made a validation trait that is almost exactly the same as yours, and I had the same problem with passwords as @mikebronner. I did not really find a solution for this yet.

I will replace my own code with your package and see if I can find something helpful.

[dalabarge]

Stay tuned, there is a new package coming early next week that will solve this problem and add new functionality to @dwightwatson's package. In the meantime you'll have to validate manually before hashing then validate more loosely after hashing.

[dwightwatson]

Which package are you referring to?

On 18 Jun 2014, at 3:55 pm, Daniel LaBarge notifications@github.com wrote:

Stay tuned, there is a new package coming early next week that will solve this problem and add new functionality to @dwightwatson's package. In the meantime you'll have to validate manually before hashing then validate more loosely after hashing.

— Reply to this email directly or view it on GitHub.

[dalabarge]

It's a package that is being open-sourced next week by a team I'm working with. It solves several common Laravel model manipulations using traits and extends upon your own Validating model trait work. I'll be doing some PRs for changes needed prior to release so if you can review those quickly when I do that would be great – it'll help release these new packages sooner.

[dwightwatson]

Yeah sure, no worries. I will be on a plane back to Sydney on the 23rd (Monday) but will take a look as soon as I'm at the gate! Look forward to seeing what you've come up with :)

Sent from my iPhone

On 18 Jun 2014, at 4:02 pm, Daniel LaBarge notifications@github.com wrote:

It's a package that is being open-sourced next week by a team I'm working with. It solves several common Laravel model manipulations using traits and extends upon your own Validating model trait work. I'll be doing some PRs for changes needed prior to release so if you can review those quickly when I do that would be great – it'll help release these new packages sooner.

— Reply to this email directly or view it on GitHub.

[tbbuck]

Seems to me that there are two separate problems to be overcome:

1. How to handle mutators
2. How to handle "external" values that must be validated, e.g. confirmations

I can think of a simple-yet-hacky way, and I don't think it'd be the end of the world given that these are relatively rare occasions:

$model->setPreMutatedValues([
    'password' => Input::get('password')
]);

And

$model->setExternalValues([
    'password_confirmation' => Input::get('password_confirmation')
]);

Merge the model's attributes into an array along with the above, pass them to the Validator::make() call, and job's a good 'un.

This would all be so much simpler if model attributes were only ever set via form submission, grrr! ;-)

[dalabarge]

@mikebronner this package should help you out: https://github.com/esensi/model. You can consume the PurgingModelTrait individually on your existing model or you can extend the Model which includes @dwightwatson's ValidatingTrait internally as ValidatingModelTrait. You don't even have to do anything since _confirmation fields are purged automatically once used.

Let me know if it works out or if you continue to have issues with validation and purging.

[dwightwatson]

@dalabarge won't PurgingModelTrait just remove *_confirmation fields before it gets to the validation and then any fields with confirmed rules will fail?

One idea is that when building the Validator I could merge the model's attributes with any input values that end in _confirmation. That way they won't make it into the model but they will be included for validation.

This would be a step in the right direction, though it wouldn't solve the password issue just yet (as the password will be hashed by the mutator and then compared against the original, unhashed, confirmation password). Like @tbbuck said, I think this would solve the second problem but not the first.

Any thoughts on this?

[dalabarge]

Out of office till Monday for the holidays here but long and short is that Purging waits till after Validating (see esensi/model docs) to actually purge. If it did it automatically then we would just use the fillable property to do that. So this should work as needed and nicely in combination with HashingModelTrait for passwords too.

On Jul 2, 2014, at 6:57 PM, Dwight Watson notifications@github.com wrote:

@dalabarge won't PurgingModelTrait just remove *_confirmation fields before it gets to the validation and then any fields with confirmed rules will fail?

One idea is that when building the Validator I could merge the model's attributes with any input values that end in _confirmation. That way they won't make it into the model but they will be included for validation.

This would be a step in the right direction, though it wouldn't solve the password issue just yet (as the password will be hashed by the mutator and then compared against the original, unhashed, confirmation password). Like @tbbuck said, I think this would solve the second problem but not the first.

Any thoughts on this?

— Reply to this email directly or view it on GitHub.

[mikebronner]

Thanks, I will try and play around with this. (Haven't had a chance yet.) From what you say it might compliment the Validating package completely and there would be little or no extra work needed.

[dwightwatson]

Possibly a step in the right direction, the current version sitting on develop will include any input that ends in _confirmation in it's validation for when you use confirmed rules. This input does not need to be passed through to the model.

[barryvdh]

I'm also having this issue, when using

public function setPasswordAttribute($password)
{
    $this->attributes['password'] = Hash::make($password);
}

Perhaps the confirmations should be checked with the value from the Input, instead of the model?

[dalabarge]

@barryvdh setPasswordAtAttribute would hash the input when $model->password = 'password123' so then the ValidatingObserver would compare it's hashed value against the password_confirmation input. This is why it isn't working. We solved this in esensi/model package by making sure that the hashing occurs only after validation (and the import order of the traits is important to ensure this). Comparing against the Input isn't really appropriate (from what I understand) since it might not be the value on the password: it's an assumption that cannot be confirmed (no pun intended). The only way I know that you could solve this is to:

• validate when setting the password
• listen to the saving event or custom validating event to inject the hashing logic after validating
• hash both password and password_confirmation using a mutator

In my opinion making the model Input aware is not a very SOLID choice and so I'm not actually a fan of the automatic handling of _confirmation fields in that way primarily for the reason that it doesn't respect mutators which might be needed. Though for _confirmation fields it's a nice convenience. The esensi/model package used a PurgingModelTrait to handle these common problems but in a non-Input aware way: so you have to set the $model->password_confirmation manually or by hydrating with $model->fill(Input::all()) or better $model->fill(Input::only($model->getFillable())).

[decnorton]

Modified the snippet in my first comment to get it working for me. Haven't come across any pitfalls or issues yet. It's relying on the fact that validating.passed is only fired by ValidatingObserver, meaning it's come from a model event as opposed to a user calling $model->isValid().

Perhaps something similar could be applied in ValidatingObserver::performValidation.

protected $passwordAttributes = ['password'];

public static function boot()
{
    parent::boot();

    Event::listen('validating.passed', function ($model)
    {
        if (is_a($model, __CLASS__))
        {
            // Prepare attributes
            foreach ($model->attributes as $key => $value)
            {
                // Remove any confirmation fields
                if (ends_with($key, '_confirmation'))
                {
                    array_forget($model->attributes, $key);
                    continue;
                }

                // Check if this one of our password attributes and if it's been changed.
                if (in_array($key, $model->passwordAttributes) && $value != $model->getOriginal($key))
                {
                    // Hash it
                    $model->attributes[$key] = Hash::make($value);
                    continue;
                }
            }
        }
    });
}

[dalabarge]

@decnorton just spit-balling here as I haven't tested your code but wouldn't your hashing get called every time validation is passed? So you manually check validation – your value is hashed. Then you save the model – your hashed value is hashed. So now you have double hashing going on. So it would seem to me that you need to check to see if your value has already been hashed: not just that it's not like the original value.

Aside: This package is called Validating and while I think Mike's and your code solves problems, is it really something that is aligned with this package's purpose? Only @dwightwatson can answer that definitively. I'm already using production-ready and tested hashing in the HashingModelTrait (of https://github.com/esensi/model) – it would be a shame to split efforts here when esensi/model consumes watson/validating which is already doing a great job with validation while esensi/model is also doing hashing and encrypting and more advanced purging. Not to mention users of Sentry will find conflict with some of these behaviors and there's no real way to opt out cleanly like there is by simply not including a dedicated trait. So maybe devs should just evaluate why HashingModelTrait doesn't fit the bill and instead submit an issue or PR on that package the team over there I can care for it?

[decnorton]

At the moment validating.passed is only fired by ValidatingObserver, and not when isValid() is called. I'm aware that's likely to change in the future, but at the moment it's working for me.

I think hashing isn't necessarily suited to this package, but as long as we can intercept an event between the time that validation has passed and the save is performed I'm happy. Perhaps add the model event intercepted by ValidatingObserver to any validating.* events?

[dalabarge]

@decnorton good observation (pun intended)! Apparently isValid() calls performValidation() on the model and not the observer (which also has the same method name which confused me at first read). So you would be correct in identifying that hashing will only occur after validation.passed during a write operation such as save() and not when manually called from isValid(). That said, have you tested two writes in a row? I haven't looked at Eloquent::save() but does it update the original property with the new attributes so as to prep for a new isDirty() check on the second save()? If it does you'll get double hashing again so the safe thing to do is actually check if the value is hashed so as to not double hash your password.

[asmerkin]

What about this?? Has it been solved?

[phazei]

@decnorton It seems that the passed observer might actually be being called twice. It seems, at least in 0.9.* that when saving a model, it does validation first with the saving ruleset, when validation.saving then validation.passed is called, and then on the validation.creating, then validation.passed again.

I solved the issue by setting the password back to plain text if the validating event wasn't passed or skipped:

    Event::listen('validating.*', function($model) {

        if (in_array(Event::firing(), ["validating.passed", "validating.skipped"]) ) {

            if ($model->isDirty('password')) {
                //TODO: potentially use Hash::needsRehash() to check is it's already been hashed?

                $model->hashedOriginal['password'] = $model->password;
                $model->password = Hash::make($model->password);

            }

        } elseif (isset($model->hashedOriginal['password'])) {
            //if it's been hashed already, set it back to its text value on all other validation events
            $model->password = $model->hashedOriginal['password'];
        }

    });

EDIT:

Updated to work with dev-develop and new events:

public function getHashable()
{
    return $this->hashable ?: [];
}

public static function bootHashingTrait() {
    //parent::boot();

    Event::listen('eloquent.validating.*', function($model) {

        $hashable = $model->getHashable();

        foreach ($hashable as $hashAttrib) {
            if (isset($model->hashedOriginal[$hashAttrib])) {
                //if it's been hashed already, set it back to its text value on all other validation events
                $model->$hashAttrib = $model->hashedOriginal[$hashAttrib];
            }
        }
    });

    Event::listen('eloquent.validated.*', function($model) {

        $hashable = $model->getHashable();

        if (in_array(explode(":",Event::firing())[0], ["eloquent.validated.passed", "eloquent.validated.skipped"]) ) {

            foreach ($hashable as $hashAttrib) {
                if ($model->isDirty($hashAttrib)) {
                    //TODO: potentially use Hash::needsRehash() to check is it's already been hashed?

                    $model->hashedOriginal[$hashAttrib] = $model->$hashAttrib;
                    $model->$hashAttrib = Hash::make($model->$hashAttrib);

                }
            }
        }
    });
}

[phazei]

Damn... so, just realized there are a couple a problems with that.

First, if I have the listener on one model, and another model is saved, it will also trigger the listener and crash because getHashable isn't set. Fixed with a method_exists check.

Second, if the listener is on 2 models, it adds two listeners for everything. And worse, when the listener for model A is triggered for model B, it crashes since apparently from there $model->hashedOriginal is protected and not visible. Not entirely sure how to fix that. I think I'll need to be very specific with the event name and listen on "eloquent.validating.(saving|updating|creating): {ModelName}" and "eloquent.validat.(passed|skipped): {ModelName}" explicity.

Any better ideas? I'm very new to Laravel, so could easily be missing something.

EDIT: this works:

Event::listen(['eloquent.validating.*: ' . static::class], function($model) {

[dwightwatson]

@phazel what are you using to do your automated model hashing? Your event listener could either check that the model is an instance of a certain class, has a certain trait or even that a given method exists on the model before calling it.

[phazel]

@phazei :)

[dwightwatson]

Apologies! Thanks @phazel

[dwightwatson]

Just a quick update on this issue, I think it's now in the best interests of this package going forward to not worry about validating attributes that are related to forms. The FormRequest that comes in Laravel 4.3 will be much better suited to handling confirmed and other form-specific validation rules and this package will be best suited to ensuring that your model's data is always persisted as valid.