refactor: bcrypt hash #51

jannyHou · 2019-02-15T03:59:00Z

A follow up PR for #33

connect to #40

Description

Refactor bcrypt hashing related functions into a service.
Decouple the password hashing service from JWT service.

Details see discussion in #33 (comment)

bajtos

I am confused about the architecture of the tests and why are they calling the hasher function directly. Feel free to leave this discussion for later, after the PR is landed.

I have few comments about the new bindings, PTAL.

bajtos · 2019-02-15T07:11:55Z

src/keys.ts

-    'services.hash_password',
+export namespace BcryptHasherBindings {
+  export const BCRYPT_HASHER = BindingKey.create<BcryptHasher>(
+    'services.bcrypt.hasher',


In dependency inversion, the bindings are usually defined via interfaces, to allow consumers to provide any implementation matching the required contract.

I am proposing to change this binding to something like

export const PASSWORD_HASHER = BindingKey.create<PasswordHasher>( 'services.password-hasher' );

bajtos · 2019-02-15T07:12:57Z

src/services/hash.password.bcryptjs.ts

+
+export interface PasswordHasher<T = string> {
+  hashPassword(password: T): Promise<T>;
+  comparePassword(provided_pass: T, stored_pass: T): Promise<boolean>;


We use camelCase, not snake_case.

Suggested change

comparePassword(provided_pass: T, stored_pass: T): Promise<boolean>;

comparePassword(providedPass: T, storedPass: T): Promise<boolean>;

bajtos · 2019-02-15T07:13:11Z

src/services/hash.password.bcryptjs.ts

+
+  async comparePassword(
+    provided_pass: string,
+    stored_pass: string,


bajtos · 2019-02-15T07:15:02Z

src/keys.ts

+  export const BCRYPT_HASHER = BindingKey.create<BcryptHasher>(
+    'services.bcrypt.hasher',
+  );
+  export const ROUND = BindingKey.create<number>(


Suggested change

export const ROUND = BindingKey.create<number>(

export const ROUNDS = BindingKey.create<number>(

(plural "rounds", not a singular "round")

bajtos · 2019-02-15T07:15:50Z

test/acceptance/user.controller.acceptance.ts


    before('create new user', async () => {
+      bcrypt_hasher = new BcryptHasher(4);


Since this is acceptance-level test, behavior should be configured mostly at app level via bindings.

I think you should change BcryptHasherBindings.ROUNDS in one of the early hooks to ensure that all tests are using less rounds to hash password when creating new users.

bajtos · 2019-02-15T07:17:25Z

test/acceptance/user.controller.acceptance.ts

      plainPassword = user.password;
-      user.password = await hashPassword(user.password, 4);
-      jwt_auth_service = new JWTAuthenticationService(userRepo, JWT_SECRET);
+      user.password = await bcrypt_hasher.hashPassword(user.password);


I am confused about this. Why is it necessary to hash the password here? Shouldn't UserRepository take care of hashing the password? At the moment, it's too easy to forget to hash the password before calling userRepo.create and end up with an invalid user record.

@bajtos The reason is the "hashing password" logic is handled in the user controller, NOT user repository.
When we create the user by making client request POST /Users, the controller will execute the hashing logic to make sure the hashed password is stored.
While when we call userRepo.create(user) directly, as what we did in the acceptance test, we need to hash the plain password first then store it.

bajtos · 2019-02-15T07:19:30Z

test/unit/utils.authentication.unit.ts

@@ -84,14 +85,19 @@ describe('authentication utilities', () => {
  });

  async function createUser() {
-    user.password = await hashPassword(user.password, 4);
+    bcrypt_hasher = new BcryptHasher(4);
+    user.password = await bcrypt_hasher.hashPassword(user.password);


Similarly here. I would expect that the logic of password hashing should be implemented in a shared piece of code that's called both by runtime parts (repositories/controllers/services) and by tests.

Maybe I am missing something?

@bajtos See #51 (comment) ^

jannyHou · 2019-02-19T17:45:13Z

@bajtos Thank you for the review.

In dependency inversion, the bindings are usually defined via interfaces, to allow consumers to provide any implementation matching the required contract.

True I realized the implemented class is over used in most places, so I replaced it with interface PasswordHasher

And regarding your concern about the test case, see my comment in #51 (comment).

PTAL thanks.

b-admike · 2019-02-20T19:32:14Z

src/services/hash.password.bcryptjs.ts

+    storedPass: string,
+  ): Promise<boolean> {
+    const passwordIsMatched = await compare(providedPass, storedPass);
+    return passwordIsMatched;


nitpick: can we do return await compare(providedPass, storedPass) here?

@b-admike Sorry forgot to apply this change, I will remember to do it when refactor this example to use new authentication module.

b-admike · 2019-02-20T19:33:25Z

src/controllers/user.controller.ts

  ) {}

  @post('/users')
  async create(@requestBody() user: User): Promise<User> {
    validateCredentials(_.pick(user, ['email', 'password']));
-    user.password = await this.hashPassword(user.password, 10);


Do we want to set the rounds binding key to 10 for this change?

@bajtos This rounds is set at the application level as 10 :) see https://github.com/strongloop/loopback4-example-shopping/pull/51/files#diff-8f42631f8d7d8cbaf47074d3ea5536d1R65

b-admike · 2019-02-20T19:34:42Z

src/keys.ts

-    'services.hash_password',
+export namespace PasswordHasherBindings {
+  export const PASSWORD_HASHER = BindingKey.create<PasswordHasher>(
+    'services.password.hasher',


nitpick: Is it possible to rename services.password.hasher to services.hasher.password so that it's in the namespace hasher like services.hasher.round below?

The difference is services.password.hasher maps to the hasher class, while services.hasher.rounds maps to a class member.

Maybe I should rename services.password.hasher to be services.hasher, does it make more sense?

Your proposal sounds great, but feel free to ignore.

b-admike

LGTM; feel free to wait for @bajtos as he might have more comments.

Signed-off-by: jannyHou <[email protected]>

bajtos

LGTM 👍

jannyHou force-pushed the refactor/bcrypt-hash branch 2 times, most recently from 71e9754 to 671d8cb Compare February 15, 2019 04:03

bajtos reviewed Feb 15, 2019

View reviewed changes

jannyHou force-pushed the refactor/bcrypt-hash branch 2 times, most recently from d433eb2 to 9920931 Compare February 19, 2019 17:12

b-admike reviewed Feb 20, 2019

View reviewed changes

b-admike approved these changes Feb 21, 2019

View reviewed changes

refactor: bcrypt hash

7e9095a

Signed-off-by: jannyHou <[email protected]>

jannyHou force-pushed the refactor/bcrypt-hash branch from 9920931 to 7e9095a Compare February 21, 2019 17:36

jannyHou merged commit 2860083 into master Feb 21, 2019

bajtos reviewed Feb 26, 2019

View reviewed changes

bajtos deleted the refactor/bcrypt-hash branch February 26, 2019 09:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: bcrypt hash #51

refactor: bcrypt hash #51

jannyHou commented Feb 15, 2019 •

edited

Loading

bajtos left a comment

bajtos Feb 15, 2019

bajtos Feb 15, 2019

bajtos Feb 15, 2019

bajtos Feb 15, 2019

bajtos Feb 15, 2019

bajtos Feb 15, 2019

jannyHou Feb 19, 2019

bajtos Feb 15, 2019

jannyHou Feb 19, 2019

jannyHou commented Feb 19, 2019

b-admike Feb 20, 2019

jannyHou Feb 26, 2019

b-admike Feb 20, 2019

jannyHou Feb 20, 2019

b-admike Feb 20, 2019

jannyHou Feb 20, 2019 •

edited

Loading

b-admike Feb 21, 2019

b-admike left a comment

bajtos left a comment

	comparePassword(provided_pass: T, stored_pass: T): Promise<boolean>;
	comparePassword(providedPass: T, storedPass: T): Promise<boolean>;

	export const ROUND = BindingKey.create<number>(
	export const ROUNDS = BindingKey.create<number>(


		before('create new user', async () => {
		bcrypt_hasher = new BcryptHasher(4);

refactor: bcrypt hash #51

refactor: bcrypt hash #51

Conversation

jannyHou commented Feb 15, 2019 • edited Loading

Description

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou commented Feb 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannyHou Feb 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

b-admike left a comment

Choose a reason for hiding this comment

bajtos left a comment

Choose a reason for hiding this comment

jannyHou commented Feb 15, 2019 •

edited

Loading

jannyHou Feb 20, 2019 •

edited

Loading