fix: hash the password #37
Conversation
jannyHou
force-pushed
the
fix/hashpass
branch
2 times, most recently
from
e84f4f2 to
6d993b0
Compare
src/utils/user.authentication.ts
Outdated
| @@ -18,11 +20,21 @@ export async function getAccessTokenForUser( | |||
| userRepository: UserRepository, | |||
| credentials: Credentials, | |||
| ): Promise<string> { | |||
| console.log('getAccessTokenForUser', credentials.password); | |||
| before('create new user', async () => { | ||
| plainPassword = user.password; | ||
| // Salt + Hash Password | ||
| user.password = await hashAsync(user.password, 10); |
There was a problem hiding this comment.
Should it be the responsibility of the controller/repository to hash passwords?
There was a problem hiding this comment.
@jannyHou and I were talking about a hash utility function which can be called from the user repo or controller. I think it makes sense for the repository to call it when storing user passwords in the backend, but I see how that can be delegated to a controller too.
There was a problem hiding this comment.
@raymondfeng @b-admike My opinion is unless we define a specific repository interface for model User and add a function for the hashing purpose, I tend to put the logic in controller. And as @b-admike said, we can extract it into utility/service that can be called from the controller.
Like what we have in authentication utils.
There was a problem hiding this comment.
There was a problem hiding this comment.
@jannyHou Please do extract this code into utility/service that can be called from multiple places. For example tests.
src/utils/user.authentication.ts
Outdated
| }); | ||
| if (!foundUser) { | ||
| throw new HttpErrors.Unauthorized('Wrong credentials!'); | ||
| throw new HttpErrors.Unauthorized( |
There was a problem hiding this comment.
Should we throw a 404 here or would that be a security risk?
There was a problem hiding this comment.
@b-admike Good point, I think 404 makes more sense.
src/utils/user.authentication.ts
Outdated
| foundUser.password, | ||
| ); | ||
| if (!passwordMatched) { | ||
| throw new HttpErrors.Unauthorized('The credential is not correct!'); |
There was a problem hiding this comment.
nitpick: I would rephrase this to The credentials are not correct.
jannyHou
force-pushed
the
fix/hashpass
branch
from
43e1aa2 to
057e018
Compare
src/utils/user.authentication.ts
Outdated
| @@ -20,21 +20,21 @@ export async function getAccessTokenForUser( | |||
| userRepository: UserRepository, | |||
| credentials: Credentials, | |||
| ): Promise<string> { | |||
| console.log('getAccessTokenForUser', credentials.password); | |||
| console.log(credentials.email); | |||
There was a problem hiding this comment.
Can we remove the console.log altogether here?
There was a problem hiding this comment.
good catch removed and squashed into one commit.
Signed-off-by: jannyHou <[email protected]>
jannyHou
force-pushed
the
fix/hashpass
branch
from
5f20500 to
b0cd66d
Compare
| }); | ||
|
|
||
| async function createUser() { | ||
| user.password = await hashAsync(user.password, 10); |
There was a problem hiding this comment.
@jannyHou
In the test suite, please use the smallest number that's still supported by bcrypt - we use 4 in LoopBack 3.x
The higher the number is, the longer it takes to compute the hash. Slow hash computation is good in production as it makes brute-force attacks difficult, but also impractical in tests because it can significantly slow down the test suite. We have been through this in LoopBack 3.x, let's not repeat that mistake again.
There was a problem hiding this comment.
@bajtos Thanks for sharing the experience and explain the reason behind using the smallest number!
In the test suite, please use the smallest number that's still supported by bcrypt
Will do it in the next PR that extracts hash functions into utils.
There was a problem hiding this comment.
Description
Fix the accesstoken generation function by checking the password instead of sending the plain password.
A follow up PR for #26
And connect to the first acceptance criteria in #35