fix: modifyDn length fix

[JohnTJohnston]

This should resolve #740.

[UziTech]

Can you write a test for this?

[JohnTJohnston]

This should repro testing against AD. In my case, the new superior was 0x86 bytes long, and wireshark ldap disector flagged the length as illegal. Refer to "BER Lengths" in https://ldap.com/ldapv3-wire-protocol-reference-asn1-ber/

const cn = "cn=existing user";
const oldDn = cn + ",ou=users,dc=aCompany,dc=com";
const newDn = cn + ",someLongNewSuperiorGreaterThan0x80Lengh";

client.modifyDN(oldDn, newDn, (err) => {
    if (err) console.log(err);
    else console.log("ok");
});

Running above will log "ok", but the object is not moved.

[jsumners]

We need at least an integration test in the same vein as

node-ldapjs/test-integration/client/issues.test.js

Lines 12 to 41 in 91de94d

	tap.test('modifyDN with long name (issue #480)', t => {
	const longStr = 'a292979f2c86d513d48bbb9786b564b3c5228146e5ba46f404724e322544a7304a2b1049168803a5485e2d57a544c6a0d860af91330acb77e5907a9e601ad1227e80e0dc50abe963b47a004f2c90f570450d0e920d15436fdc771e3bdac0487a9735473ed3a79361d1778d7e53a7fb0e5f01f97a75ef05837d1d5496fc86968ff47fcb64'
	const targetDN = 'cn=Turanga Leela,ou=people,dc=planetexpress,dc=com'
	const client = ldapjs.createClient({ url: baseURL })
	client.bind('cn=admin,dc=planetexpress,dc=com', 'GoodNewsEveryone', bindHandler)
	function bindHandler (err) {
	t.error(err)
	client.modifyDN(
	targetDN,
	`cn=${longStr},ou=people,dc=planetexpress,dc=com`,
	modifyHandler
	)
	}
	function modifyHandler (err, res) {
	t.error(err)
	t.ok(res)
	t.equal(res.status, 0)
	client.modifyDN(
	`cn=${longStr},ou=people,dc=planetexpress,dc=com`,
	targetDN,
	(err) => {
	t.error(err)
	client.unbind(t.end)
	}
	)
	}
	})

[jsumners]

We need at least an integration test in the same vein as

node-ldapjs/test-integration/client/issues.test.js

Lines 12 to 41 in 91de94d

	tap.test('modifyDN with long name (issue #480)', t => {
	const longStr = 'a292979f2c86d513d48bbb9786b564b3c5228146e5ba46f404724e322544a7304a2b1049168803a5485e2d57a544c6a0d860af91330acb77e5907a9e601ad1227e80e0dc50abe963b47a004f2c90f570450d0e920d15436fdc771e3bdac0487a9735473ed3a79361d1778d7e53a7fb0e5f01f97a75ef05837d1d5496fc86968ff47fcb64'
	const targetDN = 'cn=Turanga Leela,ou=people,dc=planetexpress,dc=com'
	const client = ldapjs.createClient({ url: baseURL })
	client.bind('cn=admin,dc=planetexpress,dc=com', 'GoodNewsEveryone', bindHandler)
	function bindHandler (err) {
	t.error(err)
	client.modifyDN(
	targetDN,
	`cn=${longStr},ou=people,dc=planetexpress,dc=com`,
	modifyHandler
	)
	}
	function modifyHandler (err, res) {
	t.error(err)
	t.ok(res)
	t.equal(res.status, 0)
	client.modifyDN(
	`cn=${longStr},ou=people,dc=planetexpress,dc=com`,
	targetDN,
	(err) => {
	t.error(err)
	client.unbind(t.end)
	}
	)
	}
	})

[JohnTJohnston]

That won't work. The new superior must be long. A long RDN will not repro the problem. So the test can only be written if your test directory has an existing long superior, which in my case was an OU.

[JohnTJohnston]

Perhaps I'm missing the point of your unit/integration tests. Is it essential to reproduce a failure prior to the fix? Is it possible for unit tests to get access to the output BER stream to confirm that the length was inserted correctly?

[jsumners]

That won't work. The new superior must be long. A long RDN will not repro the problem. So the test can only be written if your test directory has an existing long superior, which in my case was an OU.

I am showing an example of a test for a similar problem. One would need to be written to cover this problem.

Perhaps I'm missing the point of your unit/integration tests. Is it essential to reproduce a failure prior to the fix? Is it possible for unit tests to get access to the output BER stream to confirm that the length was inserted correctly?

Yes. It is essential. Otherwise we have no proof that the code does what it is purported to do. It also helps prevent changes in the future that would re-introduce the problem.

[UziTech]

LGTM

[jsumners]

Fantastic! Everything checks out locally. Thank you for providing the test.

Looks good to me.

[jsumners]

⚠️ This issue has been locked due to age. If you have encountered a recent
problem that seems to be covered by this issue, please open a new issue.

Please include a minimal reproducible example
when opening a new issue.