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

Improve Password EAV Attribute Backend model #1455

Closed
wants to merge 1 commit into from
Closed

Improve Password EAV Attribute Backend model #1455

wants to merge 1 commit into from

Conversation

ihor-sviziev
Copy link
Contributor

  1. In method beforeSave $object->getPassword() can return not a string, so would be great to convert value to string to prevent converting value to string in all places and as result we may have not unexpected behavior behavior in future, when code will be changed. Also == operator would be great replace with === because there we will have two strings, to type converting needed.
  2. In method validate $object->getPassword() and $object->getPasswordConfirm() may be not a string. As example password is string 'myPassword', password confirm is true. 'myPassword' == true Result will be true.
    So those params were converted to string and and compared with ===.
  3. Would be great to define which object type will be received in beforeSave and validate methods to be sure that we got \Magento\Framework\Object and this method contains getPassword and getPasswordConfirm (or __call method, which will process them correct). I didn't changed those methods because they defined in parent class wo type declaration.

1. In method beforeSave $object->getPassword() can return not a string, so would be great to convert value to string to prevent converting value to string in all places and as result we may have not unexpected behavior behavior in future, when code will be changed. Also == operator would be great replace with === because there we will have two strings, to type converting needed.

2. In method validate $object->getPassword() and $object->getPasswordConfirm() may be not a string. As example password is string 'myPasswordConfirm', password confirm is true.

'myPasswordConfirm' == true 

Result will be true.

So those params were converted to string and and compared with ===.

3. Would be great to define which object type will be received in beforeSave and validate methods to be sure that we got \Magento\Framework\Object and this method contains getPassword and getPasswordConfirm (or __call method, which will process them correct). I didn't changed those methods because they defined in parent class wo type declaration.
@vpelipenko vpelipenko added the CS label Jul 13, 2015
@antonkril
Copy link
Contributor

=== operator is an improvement but string conversion looks like an adaptation for poor API. We should make sure getPassword() and getPasswordConfirm() always return string

@ihor-sviziev
Copy link
Contributor Author

@antonkril so you mean that we should add checking that getPassword() and getPasswordConfirm are strings?

Which behavior should be it it's not string?
For beforeSave() method we may throw exception like "The password must be string". What's about validate() method?

@antonkril
Copy link
Contributor

I mean that signature (and PHPDoc) of getPassword and getPasswordConfirm should be recieved as a contract. Such checks are not required. Otherwise we will have to do checks for every method call. And the fact that these methods break the contract is the problem that should be fixed. They should always return strings.

@ihor-sviziev
Copy link
Contributor Author

@antonkril feel free to improve retrieving password and password confirmation 👍

@magento-cicd2
Copy link
Contributor

We have automated a Magento Contributor License Agreement verifier for contributions sent to our GitHub projects.
Please see the CLA agreement in the Pull Request comments.

@ihor-sviziev
Copy link
Contributor Author

@magento-cicd I have signed CLA agreement, but it isn't looks like signed.

@slavvka
Copy link
Member

slavvka commented Dec 28, 2015

@ihor-sviziev, thank you for contributing to Magento 2. Please ensure that all changed code is coverd with unit tests and Travis build is passing (https://travis-ci.org/magento/magento2/builds/91828729)

@KrystynaKabannyk
Copy link

KrystynaKabannyk commented Mar 30, 2016

Hello @ihor-sviziev, are you still interested in this pull request? If yes, could you update the code and cover it by automated tests?

@slavvka
Copy link
Member

slavvka commented Jun 8, 2016

Closing since author didn't cover the code by tests and similar PR is merged #4355

@slavvka slavvka closed this Jun 8, 2016
@ihor-sviziev ihor-sviziev deleted the patch-1 branch June 8, 2016 08:47
@ihor-sviziev
Copy link
Contributor Author

@slavvka @nevvermind thank you for finishing my request. I hope Magento 2 will be more secure with this fix.

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.

6 participants