Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #13833 #13844

Closed
wants to merge 1 commit into from
Closed

fix #13833 #13844

wants to merge 1 commit into from

Conversation

lynicidn
Copy link
Contributor

@lynicidn lynicidn commented Mar 23, 2017

fix break logic

Q A
Is bugfix? no
New feature? yes
Breaks BC? yes (for validator out model object)
Tests pass? not tested

fix break logic
@samdark
Copy link
Member

samdark commented Mar 23, 2017

Would you please explain what is the intent of these changes?

@lynicidn
Copy link
Contributor Author

lynicidn commented Mar 23, 2017

now without this pr:

class UserLogin extends Model
{
    public $login;
    public $password;
    /**
     * @var UserValidator
     */
    private $userValidator;

    public function __construct(array $config = [], UserValidator $userValidator)
    {
        parent::__construct($config);
        $this->userValidator = $userValidator;
    }

    /**
     * @inheritdoc
     */
    public function rules()
    {
        return [
            [['login', 'password'], 'required'],
            ['login', [$this->userValidator, 'validateLoginExist'], 'params' => $this],
            ['password', [$this->userValidator, 'validatePassword'], 'params' => $this],
        ];
    }
}
class UserValidator extends Object
{
    public $passwordError;
    public $loginExistError;
    public $loginUniqueError;

    /**
     * @var UserManager
     */
    private $manager;
    /**
     * @var Security
     */
    private $security;

    public function __construct(array $config = [], UserManager $manager, Security $security)
    {
        parent::__construct($config);
        $this->manager = $manager;
        $this->security = $security;
    }

    public function init()
    {
        parent::init();
        if (!$this->passwordError) {
            $this->passwordError = \Yii::t('user/validator', 'Incorrect password.');
        }
        if (!$this->loginExistError) {
            $this->loginExistError = \Yii::t('user/validator', 'Incorrect login.');
        }
        if (!$this->loginUniqueError) {
            $this->loginUniqueError = \Yii::t('user/validator', 'Login already exist.');
        }
    }

/*
current signature ($attribute, $params = [])
sugest signature ($model, $attribute, $params = []) that not need set context(model) to params
*/
    function validateLoginExist($attribute, UserLogin $model)
    {
        $login = $model->login;
        if (!$this->manager->existByLogin($login)) {
            $model->addError($attribute, $this->loginExistError);
        }
    }

    function validateLoginUnique($attribute, UserLogin $model)
    {
        $login = $model->login;
        if ($this->manager->existByLogin($login)) {
            $model->addError($attribute, $this->loginUniqueError);
        }
    }

    function validatePassword($attribute, UserLogin $model)
    {
        $login = $model->login;
        $password = $model->password;
        $user = $this->manager->findByLogin($login);
        if (!$user || !$this->security->validatePassword($password, $user->password_hash)) {
            $model->addError($attribute, $this->passwordError);
        }
    }
}

if apply this pr rules changed to (not need params):

    ['login', [$this->userValidator, 'validateLoginExist']],
    ['password', [$this->userValidator, 'validatePassword']],

and handler signature changed to:

validateXXX(UserLogin $model, $attribute, $params = [])

@lynicidn
Copy link
Contributor Author

Вообщем сейчас разрешена, как строка (это инлайн валидатор в этой же модели) или массив (возможно уже не инлайн, если вне модели). Код не запрещает передавать даже closure. Только в случае с callable контекст то не передается, и приходится его сувать в параметры, хотя логично, если метод вне модели - передавать туда модель, потому что валидируем то ее

@samdark samdark added this to the 2.1.0 milestone Mar 23, 2017
@samdark
Copy link
Member

samdark commented Mar 23, 2017

OK. That's valid but since backwards compatibility is broken, it's something to consider for 2.1.

@CyberPunkCodes
Copy link

It is generally discouraged to let a user know whether or not the login (either username or email) is invalid. That helps password hackers and makes brute forcing easier.

@samdark samdark closed this Apr 23, 2019
@samdark
Copy link
Member

samdark commented Apr 23, 2019

Won't be merged into 2.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants