-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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: add jwt auth extension #5046
Conversation
@@ -0,0 +1,25 @@ | |||
Copyright (c) IBM Corp. 2019. All Rights Reserved. | |||
Node module: @loopback/extension-health |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use loopback-next/packages/cli/bin/cli-main copyright
to fix file headers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Next time, try to use #4984. It will take care of all the project configuration/file headers.
auth -", you can search for it to find all the related code you need to enable | ||
the entire jwt authentication in a LoopBack 4 application._ | ||
|
||
## To Be Done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Future work
?
|
||
## Usage | ||
|
||
This module contains an example LoopBack 4 application in folder `fixtures`. It |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a user, I think I would be more interested to see how I can use this extension from ground zero, e.g. I have a todo application, how this extension can be used. If User and UserCredentials are the pre-requisite for using this extension, it might be good to include there.
I think it's better to include the models, so that users only need to have minimal setup in order to use this extension. But at the same time, we have the flexibility to extend the models for customization. Is it possible?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhmlau Good question. The user service requires User and UserCredential's persistence. Therefore including the user service in the component means the corresponding repositories and datasource are also needed:
- User model
- UserCredential model
- User repository
- UserCredential repository
- DbDatasource
But usually a LoopBack app has its own datasource, that's why I find it hard to include the user service.
But at the same time, we have the flexibility to extend the models for customization
It's not only about the models, but repositories and datasource are also involved...WTDY?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we abstract out the implementation of the UserService and TokenService? For example, if the UserService is actually calling out other service to verify the credentials rather than validating against a database entry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dhmlau I extracted the repository and model into component.
Now the app can mount the component then inject datasource and call its services in controller 👏
From an application security perspective, a JWT Bearer token is usually (but not always) just one half of the Bearer-token authentication strategy. The other half is the refresh token. Generally, access tokens have a very short life, and refresh tokens have a longer one. Will this extension account for a refresh token as well, or is it specific to the bearer authorization token? |
Thank you very helpful suggestion. This PR is a start of the extension, in the future we can support the refresh functionality. There is a demo PR for it in loopbackio/loopback4-example-shopping#537, and the token service interface has Would you like to create a new issue to track this feature(add token refresh to the jwt extension)? |
@jannyHou this package is coming with a model definition of its own. How would users use the functionality but still use their own user model (which could be drastically different from the one that's included)? In my opinion, the package should not contain any model or repositories, they should be provided to the package via dependency injection. For testing purposes they could be under the |
@hacksparrow Thank you for the review and bringing this question again, interesting:
The user service file is coupled with user model/repository, therefore moving the user model out of the module would also remove the user services, which is not good. My original implementation in the 1st commit EXCLUDED the user service+model+repository:
While after discussing with @dhmlau in #5046 (comment), and seeing a similar authorization extension created in https://github.com/ckoliber/loopback-acl-extension, we think it's better to include the user service+model+repository as prototype implementation, and only inject the datasource from application. It's also easy for people to quickly try the extension. Developers can still provide their own User model, just like extending the default User model in lb3. And given the original version, it's not hard to replace the default one, I can write additional doc to show how to replace the User model. WDYT? |
@jannyHou, IIUC, I think @hacksparrow and I are on the same page. My views in #5046 (comment) and #5046 (comment) were to have minimum implementation of the TokenService and UserService as possible. But since i'm not familiar with the implementation, I wasn't sure what's possible and what's not. The bottomline is that we should be clear in README that what's required to use the extension and how to customize it. We could possibly provide the default implementation, and let the users customize. |
@jannyHou please show how users can use the extension with their own models and repositories, that'll give us a better insight into the DX. |
"what's required to use the extension" is already provided in the README.md file, I didn't get this part: "and how to customize it". Ok now I understand what to add. Thx. |
this.bind(UserServiceBindings.USER_REPOSITORY).toClass( | ||
UserRepository, | ||
), | ||
this.bind(UserServiceBindings.USER_CREDENTIALS_REPOSITORY).toClass( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a code comment that the value of TokenServiceBindings
can be changed too if required. This is something many will definitely want to change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hacksparrow I added a new section called "customization" and it has "customizing model" as the sub section, see https://github.com/strongloop/loopback-next/pull/5046/files#diff-b7edcb305a3224321ad8aea0c9573fb8R181
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix https://travis-ci.com/github/strongloop/loopback-next/jobs/320563646.
I'm not sure what's the best way to make a disclaimer that the default user service is experimental or reference implementation.
@raymondfeng I added a section explaining this is a prototype extension, developers can customize any element, see https://github.com/strongloop/loopback-next/blob/77f730f15843c487920fd2e2e09be2321615804d/extensions/authentication-jwt/README.md#customization, and "customizing user" is the sub-section of it. |
That's good. I'm wondering if we can further improve it as follows.
|
Sure, PR coming. |
Implements #4903
Extract the jwt authentication component into a standalone extension.
Considering the user service relies on user + userCredential model and their presistency layer(repository+datasource), I decide to leave them out of the component.
Checklist
👉 Read and sign the CLA (Contributor License Agreement) 👈
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated👉 Check out how to submit a PR 👈