Skip to content

Loading…

bcrypt is an optional dependency and allow anonymous login #13

Open
wants to merge 2 commits into from

6 participants

@sokra

two little changes I needed to use this great lib.

@vesse vesse added a commit to vesse/node-ldapauth-fork that referenced this pull request
@vesse vesse bcrypt as optional dependency
Cherry-picked a commit by @sokra from trentm/node-ldapauth#13. This
probably fixes trentm/node-ldapauth#9 as well when cache is not needed.
772b08e
@cstephe

not sure about the bcrypt, but the anonymous login fix would be very helpful to me. I can submit as a separate change set... @vesse Is there a reason not to remove that check?

@sokra

@vesse picked the commit for another repo which has already many fixes including the anonymous login. You can switch from node-ldapauth to node-ldapauth-fork... I've done so...

@cstephe

fantastic, keep up the good work!

@stryju

not really sure that you are using optionalDependencies correctly...

If a dependency can be used, but you would like npm to proceed if it cannot be found or fails to install, then you may put it in the optionalDependencies hash. This is a map of package name to version or url, just like the dependencies hash. The difference is that failure is tolerated.

It is still your program's responsibility to handle the lack of the dependency.

source


so, you would have to check if bcrypt is present (probably with try { ... } catch() { ... })

@stryju stryju referenced this pull request in vesse/node-ldapauth-fork
Closed

potential issue with optionalDependency @ bcrypt #5

@sokra

ldapauth only requires bcrypt if you pass cache: true. So if you don't use the caching feature you can omit it. If you use the cache and bcrypt isn't loaded it throws an exception...

@trentm
Owner

@vesse Just saw all your great work on node-ldapauth-fork. What would you think about merging your changes into this repo, and me giving you commit and publish rights for this repo and its npm module?

@vesse

@trentm Thanks, that would be great!

@solomongifford

Whats the status on the merge?

@vesse vesse added a commit to vesse/node-ldapauth-fork that referenced this pull request
@vesse vesse bcrypt as optional dependency
Cherry-picked a commit by @sokra from trentm/node-ldapauth#13. This
probably fixes trentm/node-ldapauth#9 as well when cache is not needed.
f5495b9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 11, 2013
  1. @sokra

    bcrypt is an optional dependency

    sokra committed
  2. @sokra

    allow anonymous login

    sokra committed
Showing with 9 additions and 4 deletions.
  1. +6 −3 lib/ldapauth.js
  2. +3 −1 package.json
View
9 lib/ldapauth.js
@@ -13,7 +13,6 @@
*/
var assert = require('assert');
-var bcrypt = require('bcrypt');
var ldap = require('ldapjs');
var debug = console.warn;
var format = require('util').format;
@@ -53,7 +52,6 @@ var format = require('util').format;
function LdapAuth(opts) {
this.opts = opts;
assert.ok(opts.url);
- assert.ok(opts.adminDn);
assert.ok(opts.searchBase);
assert.ok(opts.searchFilter);
@@ -81,7 +79,10 @@ function LdapAuth(opts) {
this._adminBound = false;
this._userClient = ldap.createClient(clientOpts);
- this._salt = bcrypt.genSaltSync();
+ if (opts.cache) {
+ var bcrypt = require('bcrypt');
+ this._salt = bcrypt.genSaltSync();
+ }
}
@@ -180,6 +181,7 @@ LdapAuth.prototype.authenticate = function (username, password, callback) {
if (self.opts.cache) {
// Check cache. 'cached' is `{password: <hashed-password>, user: <user>}`.
var cached = self.userCache.get(username);
+ var bcrypt = require('bcrypt');
if (cached && bcrypt.compareSync(password, cached.password)) {
return callback(null, cached.user)
}
@@ -198,6 +200,7 @@ LdapAuth.prototype.authenticate = function (username, password, callback) {
return callback(err);
}
if (self.opts.cache) {
+ var bcrypt = require('bcrypt');
bcrypt.hash(password, self._salt, function (err, hash) {
self.userCache.set(username, {password: hash, user: user});
return callback(null, user);
View
4 package.json
@@ -16,7 +16,9 @@
"engines": ["node >=0.8.0"],
"dependencies": {
"ldapjs": "0.6.3",
- "bcrypt": "0.7.5",
"lru-cache": "2.0.4"
+ },
+ "optionalDependencies": {
+ "bcrypt": "0.7.5"
}
}
Something went wrong with that request. Please try again.