-
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(Authentication) enable authentication strategies to contribute OASEnhancer #4693
feat(Authentication) enable authentication strategies to contribute OASEnhancer #4693
Conversation
f2d96e4
to
38cfa7a
Compare
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.
@dougal83 🙇♀ Thank you again for creating a follow-up PR for the security spec. I left a few suggestions.
sorry for the delay again 😅 finished the auth migration PR so I have enough bandwidth to help you apply this feature this month
e8bef12
to
31b5c90
Compare
Restricted to a single |
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.
Thank you @dougal83 for the pull request, I appreciate the effort you are putting into this initiative ❤️
I like that your pull request includes test coverage to verify that security schemas are correctly contributed. What I like even more is that we have two examples of real-world security schemas that we can use when users come asking about examples 👍
I'd like to discuss few aspects of the proposed implementation, see my comments below.
1c39e48
to
e2b2f16
Compare
e2b2f16
to
9ad1ac9
Compare
9ad1ac9
to
59782c3
Compare
59782c3
to
2c57295
Compare
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.
🙇♀ LGTM when CI passes. Thanks @bajtos and @raymondfeng for joining the discussion, extending the enhancer class is much better than having a separate class(my original proposal).
I will refactor https://github.com/strongloop/loopback-next/tree/master/examples/access-control-migration/src/components/jwt-authentication accordingly as well.
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.
I find this new version so much more elegant 👏
Can you please add a tsdoc comment and a unit test for mergeSecuritySchemeToSpec
?
2c57295
to
d6a3e4d
Compare
add asAuthStrategy decorator for AuthenticationStrategy. Decorate and extend AuthenticationStrategy as follows: ```ts @Bind(asAuthStrategy, asSpecEnhancer) class MyAuthenticationStrategy implements AuthenticationStrategy, OASEnhancer { // ... } ``` Bind to application: ```ts this.add(createBindingFromClass(MyAuthenticationStrategy)); ``` Signed-off-by: Douglas McConnachie <[email protected]>
d6a3e4d
to
5535135
Compare
The simplicity of the PR is down to great architectural design by the Loopback Team. It was surprising how little work was required in the end! Keep up the great work! ❤️ |
Extend AuthenticationStrategy to implement OASEnhancer.
Example usage:
App > jwt-strategy.ts:
App > application.ts:
See also #4554
Impl. #3669
Checklist
npm test
passes on your machinepackages/cli
were updatedexamples/*
were updated