feat: jwt auth #26

jannyHou · 2018-12-11T04:25:29Z

I am going to fix the CI. It wouldn't block the review, the tests all pass on my local



  35 passing (1s)
  3 pending

connect to loopbackio/loopback-next#1997

This PR implements the JWT authentication strategy for verifying requests:

A JWT strategy class
- includes an authenticate method that retrieves the token from request and then restores the user profile by decoding the token.
An auth action provider
- executes the jwtStrategy.authenticate() function.
A strategy resolver provider
- returns a JWTStrategy instance if an endpoint is decorated with @authentication('jwt')
Two endpoints in the user controller
- users/login
  - jwt token will be generated using user's email and id as payload
- users/me
  - returns the login user

jannyHou · 2019-01-02T19:20:23Z

src/controllers/user.controller.ts

@@ -91,7 +93,7 @@ export class UserController {
  }

  @post('/users/login')
-  async login(@requestBody() user: User): Promise<User> {
+  async login(@requestBody() user: User): Promise<string | undefined> {


We need a type filter to describe the credentials needed for login.

The filtered type would be

type Credentials = { username: string, // consider adding `Password` and `Email` as built-in loopback types password: Password, email: Email }

Or

type Credentials = { identity: string | Email password: Password }

bajtos · 2019-01-03T15:59:41Z

src/authentication-strategies/JWT.strategy.ts

+      }
+    }
+
+    return Promise.reject('Token not found!');


When the client did not provide any token, perhaps because it did not know this URL requires authentication, the server should return 401 Unauthorized response with WWW-Authenticate header field.

In Passport, the strategy would call this.fail() API, see e.g. how passport-jwt uses it here: https://github.com/themikenicholson/passport-jwt/blob/f2044371bc9c85f22fbd30e8e58267a42dcd26ac/lib/strategy.js#L96

In @loopback/authentication, the strategy adapter is implementing fails as follows. I think this is not entirely correct implementation, e.g. I don't see any WWW-Authenticate header field being set.

https://github.com/strongloop/loopback-next/blob/aacfcf7b1ee8f6550b91d1fa3c84a6e4fbfc9a36/packages/authentication/src/strategy-adapter.ts#L50-L53

// add failure state handler to strategy instance strategy.fail = function(challenge: string) { reject(new HttpErrors.Unauthorized(challenge)); };

bajtos

Great progress!

src/authentication-strategies/JWT.strategy.ts

bajtos · 2019-01-10T12:41:59Z

src/authentication-strategies/JWT.strategy.ts

+    if (token) {
+      try {
+        const decoded = await verifyAsync(token, SECRET);
+        return Promise.resolve(_.pick(decoded, ['id', 'email']));


I think you should pick name too, since that's a property described in UserProfileSchema below?

@bajtos The picked properties are a little bit tricky.

The type UserProfile only has id as required, which makes sense. email and name are optional properties.

The User model doesn't have a name property but has firstName and lastName instead, we could compose a name property by joining them, but given the fact that name is optional, is it necessary to compose one?

This is tricky indeed.

I think we should make UserProfile more flexible to allow the applications to choose what fields they want to include. That's probably out of scope of this spike though.

I would assume that UserProfile's name property is meant either as a handle or a display name. If we have firstName and lastName properties in our domain model, then I propose to use firstName as the profile name.

@raymondfeng what do you think?

I would assume that UserProfile's name property is meant either as a handle or a display name. If we have firstName and lastName properties in our domain model, then I propose to use firstName as the profile name.

I am good with doing it in this PR, and for the discussion about the flexibility, I created a new issue for it, let's continue the discussion there: loopbackio/loopback-next#2246

bajtos · 2019-01-10T12:46:00Z

src/controllers/user.controller.ts

+    // Check if user exists
+    try {
+      const foundUser = await this.userRepository.findOne({
+        where: {email: credentials.email, password: credentials.password},


Uff uff, storing password in plain text is a huge security vulnerability. I recommend to use scrypt algorithm and store the hash only. Note that scrypt is built into Node.js from version 10, see https://nodejs.org/dist/latest-v10.x/docs/api/crypto.html#crypto_crypto_scrypt_password_salt_keylen_options_callback Since we are still supporting Node.js 8, we cannot use that built-in API and have to use a 3rd party module like https://www.npmjs.com/package/scrypt.

Historically (LoopBack 1.x/2x./3.x), we were using bcrypt instead.

@bajtos We changed the login API to return the token in a json payload.

I recommend to use scrypt algorithm and store the hash only. Note that scrypt is built into Node.js from version 10,

Hmm, we didn't include the password in the payload that's used to generate the token, so I think we don't need scrypt here.

Or did you mean we shouldn't store the password directly in the database but the hash of it instead? That's probably out of the scope of this PR, but good point, and I can create another PR to fix it.

Or did you mean we shouldn't store the password directly in the database but the hash of it instead?

Yes, I was talking about how to store the password in the database.

Storing the password plain-text is a huge security vulnerability. IMO, a reference implementation of authentication that stores passwords in plain-text is severely undermining our credibility. I would not trust any advice from somebody who is not aware of such basic security measures.

See also loopbackio/loopback-next#1996, where I proposed few more improvements to make passwords less likely to leak.

src/controllers/user.controller.ts

test/acceptance/user.controller.acceptance.ts

bajtos · 2019-01-10T12:53:25Z

test/acceptance/user.controller.acceptance.ts

+      }
+      // MongoDB returns an id object we need to convert to string
+      // since the REST API returns a string for the id property.
+      newUser.id = newUser.id.toString();


Have you considered using toJSON helper from @loopback/testlab instead? See https://github.com/strongloop/loopback-next/tree/master/packages/testlab#toJSON

const me = _.pick(toJSON(newUser), ['id', 'email']));

This comment does not seem to be addressed, PTAL.

(A detail not preventing this PR from being approved.)

@bajtos Hmm we already switched to toJSON, could you elaborate more about your expected implementation? thanks.

Can we remove this line now? I believe that toJSON will take care of converting id from ObjectID to a string.

test/acceptance/user.controller.acceptance.ts

src/controllers/user.controller.ts

bajtos · 2019-01-14T15:25:59Z

src/repositories/user.repository.ts

+  async login(credentials: Credentials) {
+    // Validate Email
+    if (!isemail.validate(credentials.email)) {
+      throw new HttpErrors.UnprocessableEntity('invalid email');


Ah, I see there is a catch in what I proposed.

The Repository layer should not be coupled with HTTP/REST specifics. Instead of using HttpErrors.UnprocessableEntity and signal the particular error case via statusCode property, we should use just err.code here and have this code translated to transport specific means elsewhere. For example, gRPC may want to use a different mechanism to signal malformed request payload.

You can find the current translation table in @loopback/rest here:

https://github.com/strongloop/loopback-next/blob/e0f75ace756d8b5479ad5b66e9b2803e4b419eef/packages/rest/src/providers/reject.provider.ts#L12-L16

// TODO(bajtos) Make this mapping configurable at RestServer level, // allow apps and extensions to contribute additional mappings. const codeToStatusCodeMap: {[key: string]: number} = { ENTITY_NOT_FOUND: 404, };

Unfortunately, we don't yet implemented API allowing extensions to contribute custom mappings. It's something we want to eventually provide, but there was no urgent need so far.

I am proposing the following approach here in this spike:

Make sure that all authorization-related errors come with an error code, e.g. UNPROCESSABLE_ENTITY.

For this spike, set statusCode too (by calling one of HttpErrors factories)

Add a comment to remind us that this solution is only temporary, and we need to make codeToStatusCodeMap customizable by extensions. Create a user story for this task, it will become a part of this spike's outcome.

See also loopbackio/loopback-next@7a56bad, loopbackio/loopback-next#1658 and loopbackio/loopback-next#1469

@bajtos I found a really weird thing, the Error interface in @types/node doesn't have the code property, it only has an optional property stack. Which means by let err = new Error('Wrong Credentials!'), you cannot give err an error code. I could define a custom Error class that extend Error but if having to do that, I would rather leveraging the HttpErrors class from rest to create the error instance.

Next I am going to:

Create a PR in @types to add the NodeJs Error properties like code in its Error interface.

Create a new story to discuss 1. how to contribute a custom mapping in the reject action. 2. how to provide the mapping in a component. 3. update this example repo or the authentication module to add the mapping for the unauthorized error.

Create a PR in @types to add the NodeJs Error properties like code in its Error interface.

+1

Until that happens, you can use the following workaround:

const err = new HttpErrors.UnprocessableEntity('invalid email'); Object.assign(err, {code: 'INVALID_EMAIL'}); throw err;

Create a new story to discuss 1. how to contribute a custom mapping in the reject action. 2. how to provide the mapping in a component. 3. update this example repo or the authentication module to add the mapping for the unauthorized error.

Awesome 👍

This is a warning sign that login should be refactored out of the repo.

src/repositories/user.repository.ts

test/acceptance/user.controller.acceptance.ts

bajtos

Looks mostly good 👍

src/authentication-strategies/JWT.strategy.ts

bajtos · 2019-01-15T08:28:21Z

src/controllers/user.controller.ts


 const hashAsync = promisify(hash);

+// TODO(jannyHou): This should be moved to @loopback/authentication
+const UserProfileSchema = {


We need to find a way allowing applications to add additional properties to user profile.

I would also prefer to leverage @model and @property decorators together with the existing code converting LB4 models to JSON/OpenAPI schema, instead of maintaining the schema manually.

@model class UserProfile extends Model { @property({type: 'string', required: true}) id: string; @property() email: string; @property() name: string; }

Feel free to leave such changes out of scope of this spike and create a new user story to follow up on this area.

There will be a refactor PR in @loopback/authentication coming soon, this change will definitely be part of it :)

bajtos · 2019-01-22T13:58:22Z

I like all the discussions while to be honest since we start with implementing the jwt authentication in this example repository, and in this PR we gathered enough information about what to do next, I would like to land this PR soon(I understand it's not perfect but endless discussion wouldn't help move forward either IMO ☹️ )

I understand your frustration.

Let's take a look at #1997 description, emphasis is mine:

The goal of this step is to show a reference implementation in our Shopping App and prepare ground for the actual auth-related work in the next iterations.
(...)
Ideally, there should be a documentation and/or a blog-post and/or a reference implementation to make it easier for LB4 users to implement similar functionality in their project.

What I would like to see as the outcome of this pull request, is a reference implementation that we are happy to recommend to all LB4 users as the right way of dealing with authentication.

Personally, I would prefer to revert the commit switching to passport-jwt:

I believe that using passport strategies with LoopBack 4 has been solved long time ago by @loopback/authentication, no need to show that again.
I see more benefits in showing people how to implement an authentication strategy without Passport, because that's not documented yet and I am not sure if@loopback/authentication actually supports that in the current version?
Plus the concerns I expressed about introducing more dependencies in my earlier comments.

IIUC, your concern is the module jsonwebtoken and passport-jwt get out of sync later in the future? Module passport-jwt doesn't provide the sign function, that's why we have to invoke jwt directly when generate the access token.

Your concern is valid while I don't have a perfect solution to sync those two dependencies. And I think that should be a concern for all the passport-jwt users too.

I assumed that both passport-jwt and our code implementing token creation (signing) eventually call the same underlying module (jsonwebtoken or jwt, I don't know), and my primary concern was about getting into a state where each path (token creation vs. token verification & parsing) end up calling a different version of JTW implementation.

Let's consider two situations.

Situation 1

A new, more secure signing algorithm is introduced.
Our login implementation uses this new algorithm to sign the token created.
passport-jwt is using an older version of JWT implementation that does not support the new signing algorithm yet.

If our authentication layer was calling JWT implementation directly, bypassing any additional abstractions like passport-jwt, then this situation would never happen.

Situation 2

A security vulnerability was found, for example in jsonwebtoken (a dependency of passport-js), or maybe researchers found a way how to compromise a certain signing algorithm and thus the algorithm is no longer considered as secure. Let's say the fix of this vulnerability requires a semver-major release of jsonwebtoken.

How long will it take until passport-js is updated to use the new version of jsonwebtoken that is not vulnerable? According to npmjs.org, the last version of passport-js was published 10 months ago. See also the discussion in mikenicholson/passport-jwt#151, it looks like this project has a single maintainer only.

Anyhow, this is just me. What do other @strongloop/loopback-maintainers think, especially @raymondfeng? Are they happy with recommending the current version of this pull request as "The Right Way"?

bajtos · 2019-01-22T14:06:32Z

src/utils/user.authentication.ts

+  credentials: Credentials,
+): Promise<string> {
+  const foundUser = await userRepository.findOne({
+    where: {email: credentials.email, password: credentials.password},


Please see the discussion in #26 (comment) where I am explaining why storing passwords in plain-text is a security vulnerability.

To me, this is a blocker preventing landing of this pull request.

On the second thought, I see that this problem was not introduced by your pull request, it was already there on master, so maybe it's alright to leave the fix out of scope of this pull request.

I am proposing the following approach:

Create a new issue in this repository to keep track of this problem and allow us to plan the work in one of the next milestones.

Here in this pull request, add a prominent code comment to point out the problem and refer people to the GitHub issue for further discussion.

@bajtos I am ok to address this concern when waiting for team's agreement.(Or in a separate PR, it depends).

@bajtos Given a look of loopbackio/loopback-next#1996 I take back my word :( Let's address it in a separate story.

Actually, the scope of loopbackio/loopback-next#1996 is different. When I was creating that issue, I did not realize that we have a bigger problem in the current implementation!

As I see it, we have two changes to make:

MUST HAVE: hash the passwords before they are stored in the database. I think we need to create a new issue for this?

SHOULD HAVE: move credentials (password, salt?) from User model to a new model. This is covered by Refactoring: Customer credentials loopback-next#1996.

Please don't forget to add a comment to this place in code to warn users about not using this code as a reference implementation.

bajtos · 2019-01-22T14:10:40Z

test/acceptance/user.controller.acceptance.ts

+      }
+      // MongoDB returns an id object we need to convert to string
+      // since the REST API returns a string for the id property.
+      newUser.id = newUser.id.toString();


This comment does not seem to be addressed, PTAL.

(A detail not preventing this PR from being approved.)

jannyHou · 2019-01-22T17:16:52Z

@bajtos I reverted to the original implementation, let's focus on one solution in this PR :)

Some background of why I switched to passport-jwt: I saw an issue in loopback-next asking about how to implement another passport strategy and decorate endpoints with it, gave a quick research I found a community module compatible with passport, but we don't have a documentation of how to plugin such modules with our existing @loopback/authentication.
Considering that the community module passport-jwt is maturer than our implementation and people needs guide for plugin different passport strategies, I changed the implementation.
Even though this PR doesn't leverage passport based strategies, I still suggest we add some tutorial/documentation about how to plugin them, maybe do it as part of "Make registering different strategies in StrategyResolverProvider extensible."

raymondfeng · 2019-01-22T22:54:34Z

@raymondfeng are we in agreement then to use the following design based on @jannyHou's comment above?

Yes. In addition to controller and repository, there should be a few classes (services - probably need to find a good name here) that can be bound to Context and use DI.

hacksparrow · 2019-01-23T13:21:38Z

jannyHou · 2019-01-23T14:15:14Z

@hacksparrow Good catch, we should. Added them in the last commit.

bajtos

Looks mostly good. I don't want to block further progress, feel free to address my comments in a follow-up pull request.

bajtos · 2019-01-24T13:27:54Z

src/authentication-strategies/JWT.strategy.ts

+    }
+
+    try {
+      const decoded = await verifyAsync(token, SECRET);


Based on our discussion around login, I feel this place should be changed in a similar way too.

JWTStrategy deals with REST-specific aspects only: how to extract the token from the request, how to bind UserProfile as request's current user.

Validation of the token and conversion into UserProfile should be handled by a new utility/service. This can be possibly the same class/file where login is implemented.

@jannyHou I feel this comment should be addressed before landing the patch. If it makes it easier for you, then I am ok to defer this work to a follow-up pull request, as long as the change is made as part of the actual user story this pull request belongs to.

bajtos · 2019-01-24T13:35:04Z

src/utils/user.authentication.ts

+  credentials: Credentials,
+): Promise<string> {
+  const foundUser = await userRepository.findOne({
+    where: {email: credentials.email, password: credentials.password},


Actually, the scope of loopbackio/loopback-next#1996 is different. When I was creating that issue, I did not realize that we have a bigger problem in the current implementation!

As I see it, we have two changes to make:

MUST HAVE: hash the passwords before they are stored in the database. I think we need to create a new issue for this?

SHOULD HAVE: move credentials (password, salt?) from User model to a new model. This is covered by Refactoring: Customer credentials loopback-next#1996.

Please don't forget to add a comment to this place in code to warn users about not using this code as a reference implementation.

bajtos · 2019-01-24T13:36:22Z

test/acceptance/user.controller.acceptance.ts

+      }
+      // MongoDB returns an id object we need to convert to string
+      // since the REST API returns a string for the id property.
+      newUser.id = newUser.id.toString();


Can we remove this line now? I believe that toJSON will take care of converting id from ObjectID to a string.

jannyHou · 2019-01-24T13:54:22Z

I feel this comment should be addressed before landing the patch. If it makes it easier for you, then I am ok to defer this work to a follow-up pull request.

Oops I didn't continue addressing this comment due to switched to passport-jwt(which is now reverted), if you don't mind I will do it in the right next PR.

Actually, the scope of loopbackio/loopback-next#1996 is different. When I was creating that issue, I did not realize that we have a bigger problem in the current implementation!

I will create a new story to discuss it, I tried to store the hash using scrypt as you suggested in

I recommend to use scrypt algorithm and store the hash only.

while my understanding is we need to store the salt in database as well, which means we need to create a new property, and if that's the case I would like to do it in a new issue that explores the possibility of extracting credentials into a standalone model as you said.

Signed-off-by: jannyHou <[email protected]> Co-authored-by: Nora <[email protected]> Co-authored-by: Biniam <[email protected]> Co-authored-by: Janny <[email protected]>

jannyHou · 2019-01-24T14:05:14Z

The follow-up PRs and stories:

(within the scope of #1997) Extract the utility/service functions in JWT.strategy.ts into utility files
~~(within the scope of #1997) Write a tutorial/blog of how to add custom authentication strategy.~~
(in progress) Refactor the UserProfile to be a model [Spike]Make model UserProfile more flexible loopback-next#2246
Hash the password Store hashed password in the login function #35
Make registering different strategies in StrategyResolverProvider extensible. Creating adapter for the plugged in passport strategies loopback-next#2311
Make AuthenticateActionProvider flexible to support both passport based and other strategies. Extension point for registering the authentication strategies loopback-next#2312
Add the logout function by invalidating jwt(I did a quick search while couldn't find a good way other than simply remove the token from the client...) Add logout function to invalidate the accesstoken #36

nabdelgadir force-pushed the jwt/auth branch 2 times, most recently from 08f6eb5 to 1f3473f Compare December 12, 2018 13:48

jannyHou force-pushed the jwt/auth branch from 1f3473f to df37b29 Compare December 19, 2018 15:53

nabdelgadir force-pushed the jwt/auth branch from f3b0a60 to aded6b6 Compare December 20, 2018 14:02

jannyHou commented Jan 2, 2019

View reviewed changes

jannyHou mentioned this pull request Jan 3, 2019

Groundwork: Access tokens, login & logout loopbackio/loopback-next#1997

Closed

bajtos reviewed Jan 3, 2019

View reviewed changes

jannyHou mentioned this pull request Jan 7, 2019

fix: optional auth metadata loopbackio/loopback-next#2218

Merged

7 tasks

jannyHou changed the title ~~[Still in progress]feat: jwt auth~~ feat: jwt auth Jan 7, 2019

jannyHou force-pushed the jwt/auth branch 3 times, most recently from 4e8fdea to e7ddbb8 Compare January 8, 2019 18:30

bajtos reviewed Jan 10, 2019

View reviewed changes

bajtos reviewed Jan 14, 2019

View reviewed changes

nabdelgadir force-pushed the jwt/auth branch 2 times, most recently from a3bc2e5 to 5b6defb Compare January 14, 2019 18:51

jannyHou force-pushed the jwt/auth branch 6 times, most recently from d6963b6 to 0001fe6 Compare January 14, 2019 19:09

nabdelgadir force-pushed the jwt/auth branch from 0001fe6 to 6511423 Compare January 14, 2019 20:17

jannyHou force-pushed the jwt/auth branch from 90516e2 to da1b97f Compare January 14, 2019 22:15

bajtos reviewed Jan 15, 2019

View reviewed changes

jannyHou force-pushed the jwt/auth branch 5 times, most recently from f823a7c to 543f54b Compare January 15, 2019 21:21

jannyHou force-pushed the jwt/auth branch from f3dd801 to 6ac3f39 Compare January 21, 2019 18:02

bajtos suggested changes Jan 22, 2019

View reviewed changes

bajtos mentioned this pull request Jan 22, 2019

Refactoring: Customer credentials loopbackio/loopback-next#1996

Closed

2 tasks

jannyHou force-pushed the jwt/auth branch 2 times, most recently from d5a59d7 to e7b65f7 Compare January 22, 2019 16:36

bajtos mentioned this pull request Jan 22, 2019

Spike: Application service layer with @service decorator (like Java Spring) loopbackio/loopback-next#2239

Closed

jannyHou force-pushed the jwt/auth branch 5 times, most recently from ae23120 to 0d78333 Compare January 22, 2019 22:48

jannyHou force-pushed the jwt/auth branch from b7edc20 to fd906bb Compare January 22, 2019 23:00

jannyHou force-pushed the jwt/auth branch from 58e0013 to a4c19fc Compare January 23, 2019 17:13

bajtos approved these changes Jan 24, 2019

View reviewed changes

feat: jwt auth

6d2c0f2

Signed-off-by: jannyHou <[email protected]> Co-authored-by: Nora <[email protected]> Co-authored-by: Biniam <[email protected]> Co-authored-by: Janny <[email protected]>

jannyHou force-pushed the jwt/auth branch from a4c19fc to 6d2c0f2 Compare January 24, 2019 14:02

jannyHou merged commit 3cbb0a8 into master Jan 24, 2019

jannyHou deleted the jwt/auth branch January 25, 2019 16:21

feat: jwt auth #26

feat: jwt auth #26

Conversation

jannyHou commented Dec 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

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

bajtos commented Jan 22, 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

Choose a reason for hiding this comment

jannyHou commented Jan 22, 2019

raymondfeng commented Jan 22, 2019

hacksparrow commented Jan 23, 2019

jannyHou commented Jan 23, 2019 • edited Loading

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

jannyHou commented Jan 24, 2019

jannyHou commented Jan 24, 2019 • edited Loading

jannyHou commented Dec 11, 2018 •

edited

Loading

jannyHou commented Jan 23, 2019 •

edited

Loading

jannyHou commented Jan 24, 2019 •

edited

Loading