-
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
[LABS] feat: adapter that wraps strategy #3056
Conversation
41acfec
to
561f6b5
Compare
} | ||
|
||
function verify(username: string, password: string, cb: Function) { | ||
process.nextTick(() => { |
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.
Why do we need to wrap it into process.nextTick()
? The users.find
is already async.
|
||
// FOR REVIWERS: THIS ACCEPTANCE TEST IS FOR `PassportAdapter` | ||
|
||
import { authenticate, AuthenticateFn, AuthenticationBindings, AuthenticationComponent, AuthenticationStrategy, UserProfile } from '@loopback/authentication'; |
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.
The code does not follow prettier
formatting style.
labs/passport-adapter/README.md
Outdated
convertToAuthStrategy(basic: BasicStrategy): AuthenticationStrategy { | ||
return new StrategyAdapter(basic, AUTH_STRATEGY_NAME); | ||
} | ||
} |
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.
What about the following idea?
-
Have one
PassportAuthenticationStrategy
provider to be registered with LB4 authentication strategies extension point. -
The
PassportAuthenticationStrategy
supportspassport:<name>
-
Inside
PassportAuthenticationStrategy
, we expose another extension point to register all passport strategies (for example, with a tagpassport.strategy
).- Creates one instance of
passport
- Discovers all passport strategy extensions
- Calls
passport.use()
to register all passport strategies
- Creates one instance of
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.
@raymondfeng Having 2 extensions kind of falls back to the initial implementation in the draft PR #2822 (which unfortunately was squashed and cannot be recovered...)
The reason why I wrapped the passport strategy directly to a common implementation of AuthenticationStrategy
is explained in `dbc51ce#r280376757
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.
And I am able to perform an authentication by invoking passport.authenticate()
, but not quite sure about the benefit of doing it (compared with invoking strategy.authenticate()
), and describing the type of a passport
instance is very problematic, see explanation
By saying that I didn't mean we have to define the passport
in any function's parameter, but in case we have to do that, we need figure out a solution for the type issue.
labs/passport-adapter/LICENSE
Outdated
@@ -0,0 +1,25 @@ | |||
Copyright (c) IBM Corp. 2017,2019. All Rights Reserved. |
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.
Would it be better to name the package as authentication-passport
?
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.
+1 to use authentication-passport
labs/passport-adapter/LICENSE
Outdated
@@ -0,0 +1,25 @@ | |||
Copyright (c) IBM Corp. 2017,2019. All Rights Reserved. | |||
Node module: @loopback/authentication |
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.
Change the module name here.
labs/passport-adapter/README.md
Outdated
.tag({ | ||
[CoreTags.EXTENSION_FOR]: | ||
AuthenticationBindings.AUTHENTICATION_STRATEGY_EXTENSION_POINT_NAME, | ||
}); |
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.
Maybe we should have a sugar method such as usePassportStrategy()
here.
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.
Yes we have it in https://github.com/strongloop/loopback-next/blob/master/packages/authentication/src/types.ts#L66
(my code was copied from the out of date draft.)
labs/passport-adapter/README.md
Outdated
import {StrategyAdapter} from `@loopback/passport-adapter`; | ||
import {AuthenticationStrategy} from '@loopback/authentication'; | ||
|
||
class PassportBasicAuthProvider implements Provider<AuthenticationStrategy> { |
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.
Another option is to use StrategyAdapter
as the base class.
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.
@raymondfeng Hmm is there a big difference between using the provider or using a class that extends StrategyAdapter
?
labs/passport-adapter/README.md
Outdated
import {AuthenticationStrategy} from '@loopback/authentication'; | ||
|
||
class PassportBasicAuthProvider implements Provider<AuthenticationStrategy> { | ||
constructor(@inject(VERIFY) verifyFn: BasicVerifyFunction); |
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.
Where is VERIFY
constant defined?
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.
oops...
labs/passport-adapter/README.md
Outdated
namespace: | ||
AuthenticationBindings.AUTHENTICATION_STRATEGY_EXTENSION_POINT_NAME, | ||
}, | ||
); |
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 as not very elegant. Can we find a way how to simplify this registration step? For example, can we leverage @extension
or @bind
decorators to specify the extension point name in the code where we are defining PassportBasicAuthProvider
.
@raymondfeng you are the author of addExtension
and the extension-point/extension implementation. What is the recommended way for registering extensions? Are we missing any new helpers in @loopback/core
to simplify this process?
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.
My code is out of date we do have that sugar function :)
@@ -0,0 +1,25 @@ | |||
Copyright (c) IBM Corp. 2017,2019. All Rights Reserved. |
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.
It should be 2019
.
@@ -23,7 +23,7 @@ async function markNonLabsPackagesPrivate() { | |||
const dir = path.relative(pkg.rootPath, pkg.location); | |||
if (dir.startsWith('labs')) continue; | |||
const data = readJsonFile(pkg.manifestLocation); | |||
let modified = !data.private; | |||
const modified = !data.private; |
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 update the base branch with this change first?
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.
good catch! updated in #3150
@@ -0,0 +1,185 @@ | |||
# Passport Strategy Adapter | |||
|
|||
_Important: We suggest users understand LoopBack's authentication system[Link |
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.
We strongly recommend that users ...
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.
And I updated the link as well since #2977 get merged
TBD](some loopback.io link) before using this module_ | ||
|
||
This is an adapter module created for plugging in | ||
[`passport`](https://www.npmjs.com/package/passport) base strategies to the |
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.
base -> based
## Installation | ||
|
||
```sh | ||
npm i @loopback/passport-adapter --save |
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.
@loopback/authentication-passport
b7ab68f
to
b862643
Compare
45a7f34
to
c3cf0a3
Compare
1a4af5b
to
ec4a4f6
Compare
5dd90f5
to
117545a
Compare
ec4a4f6
to
638cd4d
Compare
117545a
to
3f5ca61
Compare
local tests pass
|
638cd4d
to
b80934a
Compare
3f5ca61
to
ee204de
Compare
|
||
## Usage | ||
|
||
### Simply Usage |
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.
Simple Usage or Basic Use
Create an instance of the passport strategy |
Taking the basic strategy exported from passport-http as an example, first create an instance of the basic strategy with your verify function |
For reviewers: the decreased code coverage is complaining about the see report in https://coveralls.io/builds/24001707 |
This is an adapter module created for plugging in | ||
[`passport`](https://www.npmjs.com/package/passport) base strategies to the | ||
authentication system in `@loopback/[email protected]`. | ||
|
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 see two comments I added today, June 14 outside this review.
It's the similar configuration as you do when adding a strategy to a `passport` | ||
by calling `passport.use()`. | ||
|
||
But don't provide any passport specific options since we don't support the |
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.
But don't provide any passport specific options since we don't support the session manager.
Can you give an example of what you mean here? And what happens if that passport strategy requires a session manager? Will it not work?
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 doubled checked the usage of passport-http
, and realize the passport specific options are only provided when the strategy is invoked by passport.authenticate()
, like https://github.com/jaredhanson/passport-http#authenticate-requests.
So, since we don't involve passport
in this PR, we don't introduce that concern either. I will remove "But don't provide any passport specific options since we don't support the session manager."
// Give the strategy a name | ||
// You'd better define your strategy name as a constant, like | ||
// `const AUTH_STRATEGY_NAME = 'basic'`. | ||
// So that you will decorate the APIs later with the same name. |
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.
// So that you will decorate the APIs later with the same name.
// You will need to decorate the APIs later with the same name
import {AuthenticationBindings} from '@loopback/authentication'; | ||
|
||
app | ||
.bind('authentication.strategies.basicAuthStrategy') |
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.
.bind('authentication.strategies.basicAuthStrategy')
Is there a constant that can be used from @authenticate to form the first two segments of this string?
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.
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 guess the class name of the strategy here
is appended as the last segment of the string?
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.
@emonddr The key itself could be any name, it is the tag that makes the strategy recognized by the extension point, and the name
property defined in the strategy class that needs to match the name in decorator @authenticate('basic')
I don't have a strong opinion on using a string or leveraging the constant in https://github.com/strongloop/loopback-next/blob/master/packages/authentication/src/keys.ts#L108, I would respect your preference.
|
||
### With Provider | ||
|
||
If you need to inject stuff (e.g. the verify function) when configure the |
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.
If you need to inject stuff (e.g. the verify function) when configure the
strategy, you may want to provide your strategy as a provider.
If you need to inject stuff (e.g. the verify function) when configuring the
strategy, you may want to supply your strategy as a provider.
If you need to inject stuff (e.g. the verify function) when configure the | ||
strategy, you may want to provide your strategy as a provider. | ||
|
||
_Note: If you are not familiar with LoopBack provider, check the documentations |
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.
Note: If you are not familiar with LoopBack provider, check the documentations
in
Note: If you are not familiar with LoopBack providerS, check the documentation
in
// Applies the `StrategyAdapter` to the configured basic strategy instance. | ||
// You'd better define your strategy name as a constant, like | ||
// `const AUTH_STRATEGY_NAME = 'basic'` | ||
// So that you will decorate the APIs later with the same name |
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.
// So that you will decorate the APIs later with the same name
// You will decorate the APIs later with the same name
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.
Got some comments, but looks good to me overall 👍
first create an instance of the basic strategy with your `verify` function. | ||
|
||
```ts | ||
function verify(username: string, password: string, cb: Function) { |
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.
nitpick: would it help to show the import statement here?
@inject('authentication.basic.verify') verifyFn: BasicVerifyFunction, | ||
); | ||
value(): AuthenticationStrategy { | ||
const basicStrategy = this.configuredBasicStrategy(verify); |
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.
this.configuredBasicStrategy(verify);
-> this.configuredBasicStrategy(verifyFn);
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.
verifyFn
is a type, verify
is the real function.
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.
Oh...where do we get verify
from? I thought we are passing the injected variable here https://github.com/strongloop/loopback-next/pull/3056/files#diff-ac3170e38ad0d9561109e8d873e5e467R122. I thought verifyFn
is the function and BasicVerifyFunction
is the type.
// `const AUTH_STRATEGY_NAME = 'basic'` | ||
// So that you will decorate the APIs later with the same name | ||
convertToAuthStrategy(basic: BasicStrategy): AuthenticationStrategy { | ||
return new StrategyAdapter(basic, AUTH_STRATEGY_NAME); |
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 show where AUTH_STRATEGY_NAME
is declared or imported?
} | ||
|
||
app.bind('authentication.basic.verify').to(verify); | ||
registerAuthenticationStrategy(app, PassportBasicAuthProvider); |
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 include registerAuthenticationStrategy
and PassportBasicAuthProvider
imports at the top for this line?
7034146
to
e861966
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 👍
// This file is licensed under the MIT License. | ||
// License text available at https://opensource.org/licenses/MIT | ||
|
||
// FOR REVIWERS: THIS UNIT TEST IS FOR `StrategyAdapter` |
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.
nitpick: REVIWERS
-> REVIEWERS
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.
oh..another good catch, that comment should be moved too!
e861966
to
62cb56e
Compare
Connect to #2311
This PR contains 2 potential solutions(you will find TWO adapter files in
src
+ TWO unit tests + TWO acceptance tests) for converting the passport based strategies to the one defined in@loopback/authentication
:BasicStrategy
inpassport-http
: the 1st commitpassport.authenticate()
worksLet's discuss which one do we want and discard the unwanted one. If both are good, I will extract one of them to a new package.
Update:
We decided to take the simple approach: wrapping the strategy, so please ignore the 2nd proposal above ^
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 👈