-
Notifications
You must be signed in to change notification settings - Fork 122
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: allow passing of object to auth strategy service #371
base: master
Are you sure you want to change the base?
Conversation
@@ -200,7 +200,8 @@ class Server { | |||
|
|||
configurePassport() { | |||
if (this.config.auth && this.config.auth.strategy) { | |||
let { strategy } = require(path.resolve(this.config.auth.strategy.service)); | |||
const service = this.config.auth.strategy.service; |
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.
makes more sense to make it into a separate function
const Strategy = require('passport-http-bearer').Strategy;
configurePassportWithStrategy(strategy) {
if(strategy instanceof passport.Strategy){
passport.use(strategy);
return true;
}
return false;
}
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.
Thanks for checking, I will modify it, but I am not sure I understand entirely. Do you mean something like this? What to do with the return value?
configurePassport() {
if (this.config.auth && this.config.auth.strategy) {
const service = this.config.auth.strategy.service;
let { strategy } = typeof service === 'string' ? require(path.resolve(service)) : service;
this.configurePassportWithStrategy(strategy);
}
// return self for chaining
return this;
}
configurePassportWithStrategy(strategy) {
if (strategy instanceof passport.Strategy) {
passport.use(strategy);
return true;
}
return false;
}
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.
don't update configurePassport(). Just add the new configurePassportWithStrategy() and use that instead of configurePassport().
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.
ok, but I think that will not be very useful - those who may need it will not be able to use the default initialize
, but instead have to do custom initialization. In such case the users can just skip configurePassport()
and call passport.use()
directly.
I copied the code from verifyAndLoadProfiles
which already supports both strings and objects, so this change doesn't break existing behavior, just adds more possibility and unification.
This change will allow passing of object to auth.strategy.service. Similar possibility is already present in profile configuration service, so this change will unify the behaviour.
Reason:
Currently it is not possible to pass object to auth.strategy.service, only a string path which is then loaded using
require
. However, this doesn't work in ES module project where onlyimport
statement is allowed. On the other hand, profile configuration supports either path to a service or the service itself, which means in ES module project the user can load the service usingimport
and pass the loaded object.