Skip to content
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

Multiple LDAP Server Configurations Support #13

Closed
wants to merge 61 commits into from
Closed

Multiple LDAP Server Configurations Support #13

wants to merge 61 commits into from

Conversation

addnab
Copy link

@addnab addnab commented Jul 9, 2014

This is to help with scenarios where you'd want the user who is getting authenticated to decide which LDAP Server they'd like to authenticate with.

The Multiple LDAP Server configurations can be stored in a Database and the required configuration to authenticate with can be fetched based on the user's request.

var getLDAPConfiguration = function(req, callback) {
    //Fetch the required configuration from DB
    var opts = fetchConfigFromDB(req.body.ldapServerId); 
    callback(null, opts);
  });
};

var LdapStrategy = require('passport-ldapauth').Strategy;

passport.use(new LdapStrategy(getLDAPConfiguration,
  function(user, done) {
    ...
    return done(null, user);
  }
));

Vesa Poikajärvi and others added 30 commits April 13, 2013 16:56
Check arguments, set default options, and instantiate LdapAuth.
Added initial Express and ldapjs servers. Do not work yet.
First authentication implementation. LDAP test server does not work yet.
Simple LDAP server now allows everyone to access but only two users to
bind, and can return only one user record. With these we are able to
test that return values of ldapauth are handled somewhat correctly.
Always requiring applications to provide a verify callback does not
really make sense for LDAP because users are authenticated (and
authorized) from LDAP. Thus made it optional.

Also added test for reading credentials from user defined keys.
As much as I hate it when rm -fr node_modules; npm install produces
different outcome on different runs due to unfixed dependencies decided
not to fix them. Since this module is just a wrapper it will likely work
nicely even if ldapauth API changes, and passport developers have used
~0.1.0 in other modules as well so likely that will not break either.
Added test that verifies request is passed to verify callback if
configured to do so.
Previous version was using a rather old version of ldapauth
There is an issue in ldapjs[1] which causes unhandled error when the
underlying socket gets closed due to inactivity. Create a new ldapauth
instance on every authentication request to address this issue.

[1] ldapjs/node-ldapjs#127
CamelCasing adminDn so it is consistent with node-ldapauth's property.
Compilation of a dependency module fails now on 0.11, stop testing on it
for now.
vesse and others added 22 commits August 21, 2013 14:16
This is usefull for example if you want to reconfigure your application
without having to restart your node server.
Pull request #8 did not contain any tests. Add some that verify it works
now, and then get rid of regression caused by the pull request without
breaking desired functionality.
With the possibility to pass a function as options we cannot assume the
given parameter is a verify callback if it's a function, thus removed
the test.
Finally changed to binding scope since the new pull request used binding
as well. Now the scope is handled the same way in all code.
Add possibility give a function as options to LDAP strategy.

Closes #8
@addnab addnab changed the title Multiple LDAP Server Configuration Support Multiple LDAP Server Configurations Support Jul 9, 2014
@@ -158,7 +158,7 @@ Strategy.prototype.authenticate = function(req, options) {
return handleAuthentication.call(this, req, options);
}

this.getOptions(function(err, configuration) {
this.getOptions(req, function(err, configuration) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please see if you could detect given function signature and if the given function only accepts the callback print a deprecated-message and call without the req. Also, please include tests for both one and two parameters. This way the change would not be a breaking one and we can then remove the callback only -branch when making a bigger version bump.

@vesse
Copy link
Owner

vesse commented Sep 3, 2014

Terribly sorry for the delay, I didn't want to accept a breaking change and then this just slipped my mind. I added some comments to the commit if you're still interested in getting this accepted.

@mrfamazing
Copy link

I need something similar actually, i like the idea of just being able to change the name of the strategy, if not the leave it the default 'ldapauth', then I can load multiple instances of LdapStrategy with different names for different domains. That would be a simple solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants