1576901 - Add automated tests for Thunderbird querying LDAP and LDAP/SSL servers, and S/MIME certificate retrieval

Status: RESOLVED FIXED

(MailNews Core :: LDAP Integration, enhancement)

Description

[Kai Engert [:KaiE:]]

We need to be able to discover regressions that cause connections to LDAP and LDAP/SSL servers to stop working. We should ensure that the SSL connections include servers that have a certificate that also references an OCSP server.

Comment 1

[Magnus Melin [:mkmelin]]

Geoff added an LDAP mock server in bug 1608304. Could be a building block here.

Comment 2

[Ben Campbell]

Attachment: 1576901-add-ldap-query-test-1.patch

This patch adds a basic unit test for performing an LDAP query (non TLS). Not exactly comprehensive, but it should be a good template for adding tests to cover whatever bugs we encounter from here on.

Comment 3

[Ben Campbell]

Just some notes on SSL/TLS tests:

• The core of my fake LDAP server is handled in mailnews/test/fakeserver/Binaryd.jsm.
• Looks like a there's nice easy tls server example to follow in netwerk/test/unit/test_be_conservative_error_handling.js (see startServer()).
• The listener callback nsILDAPMessageListener::onLDAPError() is where the security errors will be caught. So, it'd be easy to add another PromiseListener (see patch in comment 2) which catches certificate errors.
• Want a test to make sure a bad server cert produces a reasonable error code.
• Want a test to make sure connecting to a server with self-signed cert can be caught and recovered from (by adding an exception and retrying).
• Want a test to make sure we reject un-secured connections, if the user has specified they only want secure connections! (not sure the LDAP code even supports this TBH).

Same goes for the other protocols - IMAP, SMTP, NNTP etc...

Comment 4

[Magnus Melin [:mkmelin]]

Comment on attachment 9197651 [details] [diff] [review]
1576901-add-ldap-query-test-1.patch

Review of attachment 9197651 [details] [diff] [review]:
-----------------------------------------------------------------

::: ldap/xpcom/tests/unit/test_ldapquery.js
@@ +1,3 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +
> +/**

This file should have a license on top.

@@ +18,5 @@
> + * caller can then await the promise attribute.
> + * This base class just rejects all callbacks. Derived classes should
> + * implement the callbacks they need to handle.
> + * Based on the pattern used in PromiseTestUtils.jsm.
> + */

Please add an
```@implements {nsILDAPMessageListener}```

@@ +28,5 @@
> +      this._reject = reject;
> +    });
> +  }
> +  onLDAPMessage(aMessage) {
> +    this._reject(new Error("unexpected onLDAPMessage"));

So the idea is that this method should always be overridden? Maybe add a code comment about that.
Would also be nice to capitalize U in unexpected (here and elsewhere in the patch)

@@ +33,5 @@
> +  }
> +  onLDAPInit() {
> +    this._reject(new Error("unexpected onLDAPInit"));
> +  }
> +  onLDAPError(aStatus, aSecInfo, location) {

would be good not to use the a-notation (meaning _a_rgument) for params in new code. Here it's also inconsistent.

@@ +41,5 @@
> +  }
> +}
> +
> +/**
> + * PromiseInitListener resolves the promise when onLDAPInit is called.

Please add @extends {PromiseListener}
(Hoping we at one point get linting or something to check these.)

Comment 5

[Ben Campbell]

Attachment: 1576901-add-ldap-query-test-2.patch

Take two, with all the suggestions.
Also tweaked the modeline. Turns out this code is actually Javascript rather than C++!

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9197831 [details] [diff] [review]
1576901-add-ldap-query-test-2.patch

Review of attachment 9197831 [details] [diff] [review]:
-----------------------------------------------------------------

::: ldap/xpcom/tests/unit/test_ldapquery.js
@@ +66,5 @@
> +class PromiseBindListener extends PromiseListener {
> +  onLDAPMessage(message) {
> +    if (Ci.nsILDAPErrors.SUCCESS != message.errorCode) {
> +      this._reject(
> +        new Error(`operation failed (LDAP code ${message.errorCode})`)

Capitalize O

Comment 7

[Ben Campbell]

Attachment: 1576901-add-ldap-query-test-3.patch

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/55e3510ef79e
Add a unit test for basic LDAP query. r=mkmelin

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e5b763ddc520
followup to fix linting. rs=eslint DONTBUILD