admin:user:change-password validate

[pocallaghan]

This is a question rather than a bug.

As the code coverage is borderline and I've never really done any unit testing, I thought I'd take a look at creating some tests for admin:user:change-password (the first command I came across without any). Whilst reading through the code to try and decide how to test it, I noticed that it calls validate after loading the model. I'm curious if there's a specific reason behind this (seems like there should be a test case against the reason, but I can't work out why it would ever not validate)?

[tkn98]

I don't know to which line of code you relate with your question but it's common in tests to check preconditions so that it is clear what the test expects to work. From your description the validate method could just be such a check.

A well written test should however have that clear in the fail-message (often- if not always - the last parameter of the assertion method in PHPUnit) so that other developers can see that it's a precondition assertion.

[pocallaghan]

The line in question has nothing to do with the test itself, but rather the command. In /Users/pocallaghan/Sites/github/pocallaghan/n98-magerun/src/N98/Magento/Command/Admin/User/ChangePasswordCommand.php line 51.

    $user = $this->getUserModel()->loadByUsername($username);
    // ...
    $result = $user->validate();
    if (is_array($result)) {
        throw new \Exception(implode(PHP_EOL, $result));
    }
    $user->setPassword($password);
    $user->save();

The validate function on Mage_Admin_Model_User validates that the username, first name and last name are not empty and that it's a valid email, but this is called against a user just loaded from the database. If it was called after changing the password, and the command set 'new_password', thus causing the new password to be validated, then I could understanding it and write a test for both scenarios, but as it stands the command doesn't validate after changing the password and allows you to set passwords that Magento core doesn't allow (i.e. less that 7 chars, without a number).

[steverobbins]

Looking at an example of how passwords are reset I think this could have been a mistake (or copypasta code).

In the example, they use setNewPassword() rather than setPassword(), and validate() is called after the set which triggers the password validation you're talking about.

I think the $user->validate(); should either be removed and this will be treated like a force password update, or use setNewPassword() and move the validation just above the save.

Maybe @cmuench can weigh in on what the intended design was when he wrote it.

[tkn98]

Just reading through here again. One way to simply deal with this would be writing a test that shows that the currrent way of doing in magerun has a flaw. The test should be red then.

Then the code should be fixed so that the test becomes green.

As long as the test tests for the right thing you can be sure that you did the right code.

This would also be a good example of how a unit-test actually works.

[steverobbins]

I still wonder if this was intended as a super-user feature.

For instance, when I'm told "Your password has expired" I go clear out enterprise_admin_passwords so I don't have to change it.

This could be a way of setting your password to whatever you want it to be, regardless of Magento's opinion of whether or not it's secure enough.

[tkn98]

This could be a way of setting your password to whatever you want it to be, regardless of Magento's opinion of whether or not it's secure enough.

Sounds like a valid option for a tool like magerun when you use it in development.