passport
passport copied to clipboard
jaredhanson
Dynamic Configuration
This is a work in progress sketch of dynamic configuration of passport strategies, to get high-level feedback from @jaredhanson on the design before committing to writing tests.
Two changes are made, first, the standard authenticate middleware is altered so that it may be called with an instance of the Strategy class or subclass. So you could write middleware like:
app.post('/login',
function authWrap(req, res, next) {
var lstrategy = new LocalStrategy(function(username, password, done) {
var db = dbs[req.params.directory];
db.users.verify(username, password, function(err, user) {
if (err) { return next(err); }
if (!user) { return done(null, false); }
return done(null, user);
});
}
return passport.authenticate(lstrategy)(req, res, next);
},
function(req, res) {
res.redirect('/');
});
The second change is the introduction of an authenticateWith middleware, that acts as syntactic sugar for the above approach. With it, you can use a cleaner model like:
function config(req, done) {
var lstrategy = new LocalStrategy(function(username, password, done) {
var db = dbs[req.params.directory];
db.users.verify(username, password, function(err, user) {
if (err) { return next(err); }
if (!user) { return done(null, false); }
return done(null, user);
});
}
return done(null, lstrategy);
}
app.post('/login',
passport.authenticateWith(config),
function(req, res) {
res.redirect('/');
});
The new configurator callback can be of either of the following definitions:
function configurator(req, options, done) {};
function configurator(req, done) {};
It can also return a string or array of strings to use strategies loaded with passport.use, e.g. if you dynamically want to limit the valid strategies on a route.
Additionally, this approach would obsolete the unmerged branch https://github.com/jaredhanson/passport/pull/379.
Coverage decreased (-4.5%) to 94.4% when pulling 2475f7997185727acee134055f9d29ccbb02ed44 on sjudson:dynamic-config into fc0fdc804fb5dbf99c510b6b43fa05dded9f4f48 on jaredhanson:master.
Coverage decreased (-4.5%) to 94.4% when pulling 2475f7997185727acee134055f9d29ccbb02ed44 on sjudson:dynamic-config into fc0fdc804fb5dbf99c510b6b43fa05dded9f4f48 on jaredhanson:master.
Coverage decreased (-4.2%) to 94.652% when pulling 2199cd6c9b420c9304d4c838a106507459e43974 on sjudson:dynamic-config into fc0fdc804fb5dbf99c510b6b43fa05dded9f4f48 on jaredhanson:master.
Coverage decreased (-0.7%) to 98.172% when pulling 8bf521cbd3479af01cbc776e7dde5961639843eb on sjudson:dynamic-config into 2327a36e7c005ccc7134ad157b2f258b57aa0912 on jaredhanson:master.
Coverage decreased (-4.2%) to 94.652% when pulling e879e5380b90ea7f493a1034748adda130d581cc on sjudson:dynamic-config into 821a474342b1ae900849911b5c3d3ccc4ef5ab86 on jaredhanson:master.
I have a related use-case. I don't need dynamic configuration but I do need multiple static SAML configurations. Currently passport-saml hardcoded the strategy name saml, which seems to complicate that effort.
@markstos - You should be able to get around that by customizing the name in the passport.use call. So, e.g.,
passport.use('SAMLIdP1', new SamlStrategy({
issuer: 'SAMLIdP1'
}, function(profile, done) {
...
}));
passport.use('SAMLIdP2', new SamlStrategy({
issuer: 'SAMLIdP2'
}, function(profile, done) {
...
}));
app.post('/callback/SAMLIdP1', passport.authenticate('SAMLIdP1'));
app.post('/callback/SAMLIdP2', passport.authenticate('SAMLIdP2'));
The strategy name is just defaulted to: https://github.com/jaredhanson/passport/blob/master/lib/authenticator.js#L54
@sjudson Thanks for the reply. Where is the two-argument syntax for passport.use documented? The docs I found are here: http://www.passportjs.org/docs/configure/ and don't provide any indication of that a name can be passed before the strategy, or how it might be used.
I can't speak to the documentation, or the lack thereof. But the code is clear that names of strategies can be overwritten by a first parameter in the passport.use() call, and that the new name may be used to reference the strategy for passport.authenticate() calls.
@sjudson if you create a PR against https://github.com/passport-next/passport this will get looked into.
@jaredhanson - Removing the WIP indicator, as I've now added tests. They cover the happy paths through the authenticateWith middleware, as it's designed to tightly wrap the authenticate middleware in the error cases, and so the tests for the latter should cover the tests for the former. Let me know if you want additional test cases included, however.
@sjudson as per previous comment, this isn't maintained you'll want to make a PR against https://github.com/passport-next/passport
@rwky - I will be merging this PR. I would appreciate if you would refrain from saying this isn't maintained. I may not merge things at the rate you desire, but that is different from not maintaining it. Thanks.
@jaredhanson I apologise if I've offended you. However this is the first response from you've I've seen on a passport-* repo in sometime. Several haven't had a commit this year and are starting to break (passport-facebook has been broken for a while). Since you'd not responded to anyone I had to assume you've stopped maintaining all the repos.
If you're planning on continuing to maintain passport-* I'm happy to merge the passport-next repos into yours as long as the repos become part of an organisation and at least two people have admin/npm access.
Update: I've published [email protected] to npm, which includes the ability to pass Strategy instances directly to passport.authenticate(). This version was based on a smaller PR #739. I consider this use case to be effectively addressed by the new version.
More details: https://medium.com/passportjs/authenticate-using-strategy-instances-49e58d96ec8c
(Note: I'm going to keep this PR open, because there are additional enhancements made here that are not in the smaller PR. I think the use cases are covered, however, so if there's no feedback to the contrary in the next few months, I may close this then.)