gheeres / node-activedirectory

ActiveDirectory is an Node.js ldapjs client for authN (authentication) and authZ (authorization) for Microsoft Active Directory with range retrieval support for large Active Directory installations.
MIT License
534 stars 146 forks source link

Broken paging #122

Open stemail23 opened 8 years ago

stemail23 commented 8 years ago

It looks like v1.0.0 of ldapjs breaks the paging implementation implemented in activedirectory. Because the activedirectory package.json dependencies are too permissive, a vanilla install of activedirectory will install ldapjs 1.0.0, rather than the working 0.7.1.

I recommend changing the package.json dependencies declarations to e.g: "ldapjs": "0.7.x" rather than "ldapjs": ">= 0.7.1" to avoid breaking changes in major version increments of dependencies breaking activedirectory.

stemail23 commented 8 years ago

…or even better: include an npm-shrinkwrap.json file to completely lock down the versions of all dependencies.

gheeres commented 8 years ago

...or even best: update the ActiveDirectory library to support the changes for ldapjs 1.0. :)

In the meantime, it looks like you can send an opts object with your query to enable the automatic paging:

var opts = { paged: true };
ad.findUsers(opts, function(err, user) {
});

Note: I looks like the ldapjs paging documentation might be out of sync with the actual code for the paging support.

Let me know if that works for you.

stemail23 commented 8 years ago

Well yes, that would fix it too ;) … and moving forwards that's obviously the best approach. If it were me though, I'd quickly release a patch version of activedirectory (0.7.3) to resolve the problem with the existing codebase, but modified package.json. As it stands right now, anybody installing version 0.7.2 from npm will be installing a broken version of activedirectory. (I'll submit a pull request)

Unfortunately setting paged: true doesn't work by the way, because activedirectory still adds the pagedResultControl and ldapjs complains with Error: redundant pagedResultControl.

In principle, I'm not in favour of using >= in npm dependencies, as it doesn't make sense if everybody respects semver (which they should!). The developer of any of your dependencies can quite legitimately make breaking interface changes and increment the major version. So there are no guarantees that any version of a dependency with a different major version will work with your component. Consequently my preference is to always lock the major version of dependencies (and in practice I generally lock the minor version too, because there are plenty of cases where developers don't respect semver and make breaking changes in minor version increments).

Thanks for the component by the way: it works great once I got to the bottom of it and backed out to ldapjs 0.7.1.

Cheers, Steven

shelagh-lewins-ucl commented 5 years ago

Version 0.7.2 still seems to be the latest available version: https://www.npmjs.com/package/activedirectory

Is there a way to get the fix for the problem with Error: redundant pagedResultControl? I am seeing this problem

Many thanks for the great component.

stemail23 commented 5 years ago

This library appears to have been deprecated in favour of https://www.npmjs.com/package/activedirectory2