add register endpoint #53
Conversation
There was a problem hiding this comment.
I've stolen package-lock.json from my branch to fix this. I do not have enough expertise to know how. Does @shangzhel know how?
Also may @shangzhel confirm what I've observed from bcrypt documentation?
server/src/api/register.ts
Outdated
| // generate a new user document in the user collection | ||
| await this.Users.create({ | ||
| username, | ||
| password, |
There was a problem hiding this comment.
Your password here needs to be hashed a second time.
You want to use hashSync from bcrypt with a genSaltSync to get the salt.
Or you can use the async version.
Also use the login endpoint to verify your endpoint instead of looking at the database
There was a problem hiding this comment.
@chomosuke will this work when zhangzhe's login use this V
There was a problem hiding this comment.
Yes, that's exactly why i'm asking you to hash it again
There was a problem hiding this comment.
If you don't hash it again, it won't work
server/src/__tests__/api/register.ts
Outdated
| @@ -50,7 +50,6 @@ describe('register unit tests', () => { | |||
| expect(res.sendStatus).toBeCalledTimes(1); | |||
| // check whether a new user is created in the db | |||
| expect(router.Users.create).toBeCalledTimes(1); | |||
| expect(router.Users.create).toBeCalledWith({ username, password }); | |||
There was a problem hiding this comment.
You can still check whether the hash is correct by using compareSync with bcrypt, as I've shown you in the screenshot of my last review.
Or as shangzhe has done in his login endpoint.
server/src/api/register.ts
Outdated
| // ensure there is no existing identical username | ||
| if (users.length === 0) { | ||
| // generate a new user document in the user collection | ||
| const hashedPw = hashSync(password, genSaltSync()); |
There was a problem hiding this comment.
Use the async bcrypt functions hash and genSalt, and await them. No sense in blocking when it's not necessary nor inconvenient.
server/src/__tests__/api/register.ts
Outdated
| expect(router.Users.create).toBeCalledTimes(1); | ||
|
|
||
| const firstParameter = ( | ||
| router.Users.create as jest.MockedFunction<typeof router.Users.create> |
There was a problem hiding this comment.
You can avoid this type cast by declaring create as a local variable then assigning it to the mock Users object.
|
Just gonna merge it because it's no longer WIP. |
close #28