User CRUD Tests saving hashed password

[mleanos]

While running the server tests, I'm periodically running into an issue with some of the User CRUD tests. I would get a model validation error on the password field. Each time, the validation was complaining about the "may not contain sequences of three or more..". I would re-run the tests and may, or may not, run into this issue again.

Today, I decided to dig a bit deeper into this. What it looks like to me is the following tests are attempting to re-save the hashed password. Once I realized this, the issue appearing only periodically made sense. The hashed password isn't always going to contain 3 or more consecutive characters.

We fixed pretty much the same issue with the User model's pre('save') method. But I think we now have to add that same fix to the pre('validate') method using this.isModified('password')

This is only happening with these tests, since they re-save the user...
https://github.com/meanjs/mean/blob/master/modules/users/tests/server/user.server.routes.tests.js#L129-L265

@lirantal Would you agree with the suggested fix? I ask you because I know you're working directly with the User CRUD tests right now. You may have the most insight at this time.

@jloveland WDYT?

[mleanos]

Screen shot of the error

[lirantal]

@mleanos I can definitely confirm this bug is right.
I'm wondering if just checking for the modified password is a patch or do we have a bigger problem with the password generating again due to the owasp requirements?

[mleanos]

@lirantal If you see where the tests are originating from, it's not interacting with the password generator at all. I'm pretty sure the issue is that in the case of the User Crud tests, the user is saved in the beforeEach hook. Then again, with the tests that modify the user object & then save it again. This will cause the User model validate pre hook to fire, and the password is now in the hashed format. Thus it MAY contain 3 or more consecutive characters. This is why we're only seeing this issue with these particular tests.

https://github.com/meanjs/mean/blob/master/modules/users/tests/server/user.server.routes.tests.js#L33

https://github.com/meanjs/mean/blob/master/modules/users/tests/server/user.server.routes.tests.js#L140

I also wonder if adding the check for the password isModified is just a patch. But I think it would need to be addressed in the pre 'validate' hook of the user model; OR we need to be mindful of the validation firing on each save; thus, we would adjust our strategy with the saving here in these tests (or anywhere else we have similar logic).

[lirantal]

Ahh ok, got it.
The isModified is the exact same way we handled it in the .pre save hook so we should stick with that for consistency. It means we test the password field only if it's really changed, meaning a new password is there. Last question on this - in general, are we sure we're actually running the validation test on the pre-hashed password field and not on the hashed password field ?

[mleanos]

@lirantal What do you mean "in general"? Can you provide an example, or reference?

From what I see, we're testing the strength of the password after user input. In respect to mongoose, the pre('validate') method runs before the pre('save'); whatever state the password is in at that time, will be what the pre('validate') uses.

Adding the isModified check to the validation method, does seem like a little bit of a patch, but the alternative is to take careful consideration of the state of the User object, when we're interacting with it; like when saving. Unless we have more robust model validation abstracted away from Mongoose, it still comes down to individual implementation.

Regardless, we should have the .isModified check. Perhaps, we can also pay more attention to the state of the User object.

[lirantal]

So it's somewhat related to the discussion we had back then about whether to use the isModified check or not and I don't think it's a patch, it's part of the model's responsibility.

[jloveland]

I agree that we should have the model take care of making sure the password has been changed before sending to the password validation. I do think we need to evaluate how we test. Do we have a test that explicitly looks for this issue? If not, we should.

I also think that when our individual tests should start with a clean state (i.e. not leverage previous test's user information). Some of the test I have written do not follow this strategy, so I think a PR to fix that is in order.

WDYT?

[lirantal]

Yes one of the user tests in the model has a test for changing a password and confirming that a new hash is generated. Maybe we can enhance it with another test for pre-validations? maybe there's already one, I'm not sure. We need to take another look at the tests in the user model.

The tests do start with a clean slate (although some are dependent on the other so they don't clean) Take a look at them