Improve password logic

[mikehaertl]

The way this behavior works now, you hand out the hashed password to the user during an update. If the password content equals the hashed content, the password is not changed.

I don't particularly like the idea of sending pw hashes to the user. And it makes the behavior logic unneccessarly complex, as you have to save the original DB value in afterFind().

I'd find it much better, if the behavior used a dedicated property for new passwords from forms:

class X extends CActiveRecord
{
    // Used in forms to let users enter a *new* password
    public $newPassword;
    //...
}

The DB column (e.g. password) is never exposed to the user, only newPassword is. You can think of it as a one-way road for new passwords into our DB. Only if newPassword is not empty, a new hashed password is written to password.

This would simplify the behavior code quite a bit - and make it more transparent to the user, in case he wants to add additional validation to the newPassword column.

[githubjeka]

Store passwords in its original form is bad.

[mikehaertl]

@githubjeka I didn't say that it should store passwords in original form. Please read carefully:

Only if newPassword is not empty, a new hashed password is written to password.

[githubjeka]

I'm really trying to understand what you need, but did not understand how and what you want to improve in behavior. I read:

And it makes the behavior logic unneccessarly complex, as you have to save the original DB value in afterFind().

[mikehaertl]

Not sure how to put it simpler, so maybe have a look at the current code. After record was loaded it has to save the current crypted password (=="original DB value") into a private variable. It needs to do this, so that it can find out, if the password has changed.

That would be obsolete with my proposed solution. Basic mechanism:

class MyRecord  extends CActiveRecord
{
    public $newPassword; // Not a DB column!!!
    public function rules()
    {
        return array(
            array('newPassword','safe'), // no rule for "password"!!
        }
    }
    public function beforeSave()
    {
        if(!empty($this->newPassword))
            // "password" is the crypted DB password. It's never exposed to the user and only
            // ever changed, if a new password was requested.
            $this->password = hash($this->newPassword);
    }
}

The above only shows the basic concept. It's easy to integrate into the behavior.

[githubjeka]

Hmm.. Which scenario uses the newpassword? (login, registration, changePassword) I use safe rule for password only in login scenario and i use $user->verifyPassword($this->password) for valid it, because password cannot be changed where else.

This code in behavior:

public function beforeSave($event)
    {
        $password = $event->sender->{$this->passwordAttribute};
        if ($password != $this->_hashedPassword && $password != "") {
            $this->changePasswordInternal($password);
        } elseif ($password == "" && $this->_hashedPassword != "") {
            $event->sender->{$this->passwordAttribute} = $this->_hashedPassword;
        }
    }

Sorry, but I still do not understand what you want to hide, delete, disable?

[mikehaertl]

It's used everywhere in forms! password is never ever used in any form anymore! password is a DB-only field, which contains the hashed password.

If it's still unclear, please do me a favour and copy my above code example and try it out. It's a very simple and proven mechanism.

UPDATE: Maybe it becomes easier to understand, if we call the DB column crypt_pw and the class property password:

• password: Not a DB column, only used in forms, to let users enter the clear text password. Never stored to DB!
• crypt_pw: What is stored in DB. Only ever changed, in beforeSave(), if password is not empty. Also used for login, to compare hash($this->password)===$this->crypt_pw.

[githubjeka]

And I also do. But this is associated with the logic of my app, at what here behavior?

[phpnode]

@mikehaertl i agree with you, this was a poor design decision. It would have been better to have a hashedPassword field in the db and implement password as a virtual property.

[githubjeka]

:+1:

[phpnode]

@githubjeka the problem with the current design is that if you e.g. load a model in a controller, and want to display a password input field in a view, you have to manually blank the current password in the model before rendering the view, otherwise you end up displaying the hashed password in the password field. Then if the user clicks save it overwrites their existing password with a hashed version of their password, so they can no longer login. Having to do $model->passsword = null; before every view render is very fragile. We should fix this, but it will be a breaking change.

[githubjeka]

I do not know whether to make such changes. It really clarifies the use of the code, but it will be really critical changes. As for me, I do not use the password field anywhere, and everywhere use newPassword and for me this is normal. If we do that, we will have to explain what you need to create a field in the table, other than the native password.

Although I am for having to make this clear, but you will finally make detailed instructions on how to use :)

[githubjeka]

This change will only use $password instead of $newPassword. Or will it give another profit?

[phpnode]

what we probably need to do is tag the current code as v1.0.0, fix this, and tag it as a new version, e.g. v1.1.0 - however it will break for anyone currently using composer and dev-master to install it. I'll have a think about this, let's leave it as it is for now as anyone currently using this is having to work around the issue any way.

[crisu83]

I will look this when I have time unless this was already done?

[phpnode]

@crisu83 it wasn't done yet so that'd be really appreciated, many thanks!