ad.find always returns group membership

[SelfDevTV]

Hi guys,

Problem:

When you search with the method find it will resolve the group members internally, but returns the search results without the resolved members. In my oppinion it should not resolve it in the first place when no options with includeMembership has been provided. But right now it does it internally (you can only find out if you enable logging like stated in the docs) but it's returning the correct result. This is consuming very much ressources, imagine finding 100 groups with 200 users each. But you only want the groups without the users.

Why did I encounter this problem? When I searched for specific groups in my company, the server crashed. It was just a too big search, since it found about 50 groups and some of these groups have groups inside it with 1000's of users.

OLD ERROR / initial Issue:

I have a basic api route that looks like this:

app.get("/api/test/search",  (req, res) => {
  ad.find("cn=*all_2217*", (err, results) => {
    console.log("in test find");
    if (err) {
      console.log("error from test find", err);
      res.status(401).send("error");
      return;
    }

    res.send(results);
  });
});

When I go to this route with "activedirectory2" installed I get this error:

ConnectionError: 1__ldap://dcserver.company.root.local closed

But it also logs the results before. So it somehow resolves like this: ==> It logs undefined, so it doesn't resolve, but I get the response on the client, I see all the groups and users! After that the Server crashed and I can't make another call. Weird O.o

• It finds the results and sends it. But then somehow it also runs the error line and sends the error too.

After the fist error there is a second in the console (don't get why, I just send the error and return):

Cannot set headers after they are sent to the client

Full Error:

When I use the other package "activedirectory" this works fine, but I can't use this package because in this I have the bug that was resolved with 2.0 "Fix illegal escaping".

Whats funny too is when the search term is another one like "cn=admins" it works and I don't get this error, although I find many more groups and users then with "cn=all_2217".

Any ideas why this api route first sends the response and then also sends the 401, and why I get the ldap error from my dc server? I tried several dc servers that have ldap same error. With exact same setup it works on the little brother "activedirectory".

I also tried to npm install activedirectory2@next but that doesn't help.

Thx so much and btw: Awesome Library!!

[jsumners]

The snippet of code you posted looks fine. Have you tried enabling trace level logging? https://github.com/jsumners/node-activedirectory#opts

[SelfDevTV]

Hi @jsumners thx for the reply. I can't install pino. I tried with normal pino and also pino-http but I get several errors when I try to npm install.

Is there any other logger package I can use for example node-logger ?

[jsumners]

You can use any logger that supports the Log4j-style interface. But pino should work just fine.

[SelfDevTV]

I needed to install windows build tools. Now the logger is working and I might have an idea why it's not working. When I see the output of the filter from find "2217_all" it finds the groups AND also the members. Some of these groups have well over 1000 members. Is there a way to disable it, so it doesnt search for members too?

My config looks like this:

attributes: {
    user: [
      "distinguishedName",
      "userPrincipalName",
      "sAMAccountName",
      "mail",
      "lockoutTime",
      "whenCreated",
      "pwdLastSet",
      "userAccountControl",
      "employeeID",
      "sn",
      "givenName",
      "initials",
      "cn",
      "displayName",
      "comment",
      "description",
      "companyUCID",
      "companyUserType",
      "companyResponsible",
      "division",
      "company",
    ],
    group: [
      "companyUCID",
      "companyUserType",
      "companyResponsible",
      "distinguishedName",
      "objectCategory",
      "cn",
      "description",
    ],

Here is a screenshot of a log that works because it's just one group, but you can see that it resolves the members, which it shouldn't right (look at member array this is a huge array with all members)?

This would explain the dc disconnect since it's simple too huge. With "all_2217*" it finds about 30 groups with nested groups inside and I think more then 10.000 users combined. I want to just find the groups without resolving members.

And when I click on a group I make another api call to: getUsersForGroup

Thx so much for your help!

[jsumners]

The find function is a generic search function. You will need to narrow your query if you don't want to return so many results.

[SelfDevTV]

Yeah but it's ok to find 100 users and 100 groups. But why is it also resolving the members of the group(s)?
Edit: With findGroups it does the same except that it doesn't resolve the members. So I will search for Users and for Groups create a custom object and I return this. I think this will work. Still I don't know why find should resolve the members too.

Thx for your work @jsumners :)

[jsumners]

Because you told it to --

node-activedirectory/lib/components/find.js

Lines 138 to 151 in 7b0cf44

	// get user group memberships if desired
	if (utils.includeGroupMembershipFor(this.opts, 'group')) {
	ad.getGroupMembershipForDN(this.opts, group.dn, (err, groups) => {
	if (err) {
	return cb(err)
	}
	group.groups = groups
	ad.emit('group', group)
	cb()
	})
	} else {
	ad.emit('group', group)
	cb()
	}

[SelfDevTV]

But where in my code have I told the find method to incloudeGroupMembership?
Here is my whole config object:

var config = {
  url: "ldap://mydc.mycompany.root.local",
  baseDN: "DC=company,DC=root,DC=local",
  logging: log,
  username: "myuser",
  password: 'mypassword',
  attributes: {
    user: [
      "distinguishedName",
      "userPrincipalName",
      "sAMAccountName",
      "mail",
      "lockoutTime",
      "whenCreated",
      "pwdLastSet",
      "userAccountControl",
      "employeeID",
      "sn",
      "givenName",
      "initials",
      "cn",
      "displayName",
      "comment",
      "description",
      "companyUCID",
      "companyUserType",
      "companyResponsible",
      "division",
      "company",
    ],
    group: [
      "companyUCID",
      "companyUserType",
      "companyResponsible",
      "distinguishedName",
      "objectCategory",
      "cn",
      "description",
    ],
  },
};

And the test route:

app.get("/api/test/search",  (req, res) => {
  ad.find("cn=*all_2217*", (err, results) => {
    console.log("in test find");
    if (err) {
      console.log("error from test find", err);
      res.status(401).send("error");
      return;
    }

    res.send(results);
  });
});

[jsumners]

Maybe there is a bug. If so, a PR to fix it is welcome. Please remember to include unit tests.

[SelfDevTV]

function includeGroupMembershipFor (opts, name) {
  const lowerCaseName = name.toLowerCase()
  return (opts.includeMembership || []).some((i) => {
    const j = i.toLowerCase()
    return j === 'all' || j === lowerCaseName
  })
}

Never returns false, I think this function is faulty.
Hmm [].some returns false, and it's an empty array when there is no options provided.
Need to dive deeper.
I might have a look if I find the time. But if someone else would like to try it be my guest 👍
Can you open the issue again plz?

[SelfDevTV]

@jsumners Is there some kind of Contribute guide or guidelines for this repo? For example is there some kind of mock server? I'm a bit lost, would be my first issue on github I try to solve :D
The scripts in package.json are just for testing.

[jsumners]

This repo follows typical Node.js community module conventions. You can run npm test to run the unit tests. Those unit tests are within the test/ directory.

[SelfDevTV]

Ok, sorry one more question. How to start the mock server? In the tests/mockServer direcotry in index.js you export the server.listen function but I don't know where the entry file is and where I can start it with node file.js

[jsumners]

The tests automatically start it and query against it.

[SelfDevTV]

Ahhh okay so I make changes to the find function that I think will solve the problem and then I write a test against it. Then I run the suite. I think I get it.

[jsumners]

It would be better to write a failing test first, but, yes, that is the way.

[SelfDevTV]

t.test('should not include groups/membership groups and users if opts.includeMembership[] has not been provided', t => {
    const query = settings.queries[0]
    const opts = {
      
      filter: query.query
    }
    t.context.ad.find(opts, function (err, results) {
      t.error(err)
      t.type(results, 'object')

      results.users.forEach((user) => {
        t.notOk(user.groups)
      })

      results.groups.forEach((group) => {
        t.notOk(group.groups)
      })

      results.other.forEach((other) => {
        t.notOk(other.groups)
      })

      t.end()
    })
  })

My assumption was that this will fail, but it passes. So on the mock server it appears to be working. Maybe anyone that sees this could take a look? Or is my test faulty? I think testing this function won't bring much, since it is working and it returns just the result without the resolved members.

It's just in the log. So it resolves it somewhere. Need to dive deeper

Edit: the closest Ive gotten is here:

node-activedirectory/lib/components/utilities.js

Line 102 in 7b0cf44

function getRequiredLdapAttributesForGroup (opts) {

I logged it but this also does not add the member attribute to it when options is empty. I really have no idea where to look. You have any ideas or starting points for me?

[tfrancois]

Maybe I'm a little late to the party, but isn't it simpler just to add the includeMemberships: [] to the opts object? I think by default memberships are returned for users by design (and maybe also for groups), and that is why you're seeing this extra info. Advise if that makes sense or if a PR is truly needed so as to understand if this is truly a bug or not. Thank you.