Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: create jwt auth service #33

Merged
merged 1 commit into from
Feb 14, 2019
Merged

feat: create jwt auth service #33

merged 1 commit into from
Feb 14, 2019

Conversation

jannyHou
Copy link
Contributor

@jannyHou jannyHou commented Jan 25, 2019

A improvement PR after #32.
(I was trying to create this PR directly as the refactor for #26 , while considering the amount of effort and the time left in January, I decide to do the service refactor in this separate PR.)

  • Turns the JWT related util functions into a service, which could be injected to controller using @inject.

  • organizes JWT related providers/services/strategies into a component that easy to mount to an application. Not sure is it still needed.

value(): ValueOrPromise<JWTAuthenticationService> {
return new JWTAuthenticationService(this.userRepository, this.jwt_secret);
}
}
Copy link
Contributor

@raymondfeng raymondfeng Jan 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not necessary to have JWTAuthenticationServiceProvider as JWTAuthenticationService class can receive injections to create instances. You can simply do the following:

export class JWTAuthenticationService {
  constructor(
    @repository(UserRepository) public userRepository: UserRepository,
    @inject('JWT.authentication.secret') protected jwt_secret: string,
  ) {}
}

And

this.bind(JWTAuthenticationBindings.SERVICE).toClass(JWTAuthenticationService);

tsconfig.json Outdated
"lib": ["es2018", "dom", "esnext.asynciterable"]
"lib": ["es2018", "dom", "esnext.asynciterable"],
"compilorOptions": {
"experimentalDecorators": true,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it required? I thought this project inherits from the base tsconfig which has "experimentalDecorators": true,.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raymondfeng My vscode doesn't recognize the config in the extended file, it complains experimental decorator feature missing and has red line for all the decorators. So I changed the file. But they should be removed in this PR.

@raymondfeng
Copy link
Contributor

Please fix the CI failure. IIRC, it's related to the version of oas-validator.

> @loopback/[email protected] test:ci /Users/travis/build/strongloop/loopback4-example-shopping
> lb-mocha --allow-console-logs "dist/test"
Error: Cannot find module 'call-me-maybe'
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:581:15)
    at Function.Module._load (internal/modules/cjs/loader.js:507:25)
    at Module.require (internal/modules/cjs/loader.js:637:17)
    at require (internal/modules/cjs/helpers.js:22:18)
    at Object.<anonymous> (/Users/travis/build/strongloop/loopback4-example-shopping/node_modules/@loopback/testlab/node_modules/oas-validator/index.js:11:15)

*/
export type HashPassword = (
password: string,
rounds: number,
Copy link
Member

@bajtos bajtos Feb 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, consumers of HashPassword should not need to understand specifics of the hashing function used, e.g. number of rounds. Typically, we want to use a different number of rounds when running in test vs. in production. Consumers of HashPassword do not have enough information to be able to pick the right number of rounds.

I am proposing the following API:

export type HashPassword = (password: string) => Promise<string>;

And the following usage:

// in production
app.bind(OtherServicesBindings.HASH_PASSWORD)
  .to(pwd => hasPassword(pwd, 10));

// in test
app.bind(OtherServicesBindings.HASH_PASSWORD)
  .to(pwd => hasPassword(pwd, 4));

Having wrote that, I think we should go one step further. As I see it, the function for computing password hash is tightly coupled with the function for comparing user-provided password with the stored hash, and therefore both functions should be part of the same "service".

interface PasswordHasher {
  hash(password: string): Promise<string>;
  compare(password: string): Promise<boolean>;
}

class BcryptHasher implements PasswordHasher {
  constructor(
    @inject('BcryptHasher.rounds')
    private readonly rounds: number
  ) {}
  // ...
}

app.bind('services.PasswordHasher').toClass(BcryptHasher);

// usage in production
app.bind('BcryptHasher.rounds').to(10);
// usage in tests
app.bind('BcryptHasher.rounds').to(4);

Later in the future we should leverage loopbackio/loopback-next#2259 to control this configuration option.

Feel free to leave this second part (service for hash+compare) for a follow-up pull request.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bajtos

IMO, consumers of HashPassword should not need to understand specifics of the hashing function used, e.g. number of rounds.

Having wrote that, I think we should go one step further. As I see it, the function for computing password hash is tightly coupled with the function for comparing user-provided password with the stored hash, and therefore both functions should be part of the same "service".

Both good points 👍 Do you mind if I address both of them in the next PR as a refactor specific for password hashing? (within the same story #40)

I would like to have this PR focus on the JWT related services first. Then improve the password hash in a second one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough 👍

@jannyHou jannyHou merged commit 1ffc806 into master Feb 14, 2019
@jannyHou jannyHou mentioned this pull request Feb 15, 2019
@bajtos bajtos deleted the auth/refactor branch February 15, 2019 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants