662192 - Make an LDAP fake server for tests

Status: RESOLVED FIXED

(MailNews Core :: Testing Infrastructure, task)

Description

[Joshua Cranmer [:jcranmer]]

LDAP is almost untested right now. Let's make a fake LDAP server.

The problem is that LDAP uses ASN.1 and BER, which means we ultimately are testing a raw binary protocol with a moderately ugly encoding scheme. There is a generic BER encoding/decoding framework exported from libldap60 (ber_read/ber_write does char*-to-BerElement* and ber_scanf/ber_printf can pack/unpack BerElement*). If we use that, it of course relies on the assumption that those methods aren't buggy--but I think I can live with that.

What features are needed:
1. Database read/write (when we get writable LDAP working, naturally)
2. Bind and access controls (both simple and SASL)
3. SSL/StartTLS (when we get them working for fakeserver, we should have them for ldapserver)
4. Search (obviously)
5. Possibly schemas

From a test API, new ldapDaemon(ldif[, auth[, schema]]) is probably sufficient.

Crazy idea:
Specify an option to nsMailServer that allows servers the ability to transcode the event stream. The LDAP daemon would use that transcoder to convert the binary stream to a text-ish protocol that would allow the same handler architecture to be used for LDAP. Then this same architecture could be used to do SSL/StartTLS support, as well as potentially mail-via-proxy.

Comment 1

[Ludovic Hirlimann [:Usul]]

We could also use things like http://pyasn1.sourceforge.net/ and the perls equivallent.

Comment 2

[Joshua Cranmer [:jcranmer]]

While that seems like a nice package, I think I'd prefer to keep things in C/C++ or JS where possible...

Comment 3

[Ludovic Hirlimann [:Usul]]

Removing myslef on all the bugs I'm cced on. Please NI me if you need something on MailNews Core bugs from me.

Comment 4

[Ben Campbell]

Attachment: 662192-fake-ldap-server-1.patch

Comment 5

[Ben Campbell]

There is already mailnews/addrbook/test/LDAPServer.jsm, although that seems to require manual pumping, and I couldn't figure a way to get it working with nsILDAPSyncQuery. So I wrote another one, drawing on LDAPServer.jsm and the various other fake servers scattered about the codebase.

It's pretty basic - supports loading in test data and performing some simple searches. But it's easy to extend, so we should be able to add whatever other LDAP functionality we need as we add more LDAP tests.

It also includes a generic binary server, which might be handy for hacking up fake servers to test other protocols.

Comment 6

[Magnus Melin [:mkmelin]]

Comment on attachment 9189671 [details] [diff] [review]
662192-fake-ldap-server-1.patch

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

Looks pretty ok to me. Let's have Geoff review since he created the other LDAP test server.

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

For documentation, use /**

@@ +10,5 @@
> +  "resource://testing-common/mailnews/Binaryd.jsm"
> +);
> +const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
> +
> +const nsILDAPURL = Ci.nsILDAPURL;

Doesn't look like you use this, and I would avoid declaring these temp variables unnecessarily. They exist in the code base purely since Ci. didn't used to be defined and then it was long to write Components.interfaces. everywhere

@@ +17,5 @@
> +
> +function getLDAPAttributes(urlSpec) {
> +  let url = Services.io.newURI(urlSpec).QueryInterface(Ci.nsILDAPURL);
> +  let ldapquery = Cc[LDAPSyncQueryContractID].createInstance(nsILDAPSyncQuery);
> +  let version = Ci.nsILDAPConnection.VERSION3;

please inline this one

::: mailnews/test/fakeserver/Binaryd.jsm
@@ +61,5 @@
> + *
> + */
> +class BinaryServer {
> +  /**
> +   * @param handlerFn Function to call to handle each new connection.

@param {Function} handlerFn - Function to call......

@@ +62,5 @@
> + */
> +class BinaryServer {
> +  /**
> +   * @param handlerFn Function to call to handle each new connection.
> +   * @param daemon    Object to pass on to the handler, to share state

@param {Object} daemon - Object......

@@ +80,5 @@
> +   * @param port The port to run on (or -1 to pick one automatically).
> +   */
> +  async start(port = -1) {
> +    if (this._listener) {
> +      throw Components.Exception("", Cr.NS_ERROR_ALREADY_INITIALIZED);

I think it's nicer to put a message in these, not just "".

@@ +105,5 @@
> +            // if we get here, assume the error occured because we're
> +            // shutting down, and ignore it.
> +          } else {
> +            // if we get here, something went wrong.
> +            dump("ERROR " + e.toString());

Should the error be let un-caught instead?

@@ +109,5 @@
> +            dump("ERROR " + e.toString());
> +          }
> +        }
> +        conn.close();
> +        delete server._connections[connID];

and these in a finally clause?

Comment 7

[Ben Campbell]

Attachment: 662192-fake-ldap-server-2.patch

Revision mainly to tidy up jsdoc use.

Kind of at the outer limits of my JS knowledge here - in particular my implementation of Connection.read() seems a little off: mixing Promises with async. Suggestions for improvement welcome!

(In reply to Magnus Melin [:mkmelin] from comment #6)

> ::: mailnews/test/fakeserver/Binaryd.jsm
> @@ +105,5 @@
> > +            // if we get here, assume the error occured because we're
> > +            // shutting down, and ignore it.
> > +          } else {
> > +            // if we get here, something went wrong.
> > +            dump("ERROR " + e.toString());
> 
> Should the error be let un-caught instead?
> 
> @@ +109,5 @@
> > +            dump("ERROR " + e.toString());
> > +          }
> > +        }
> > +        conn.close();
> > +        delete server._connections[connID];
> 
> and these in a finally clause?

I'm not sure yet. For a real server, you want it to keep running even if one connection explodes, so I kind of think our fake server should aim for that. But for testing purposes it could be nice if the whole server explodes too :-)
I've left it as is for now. It can be revisited if some tests require a change.

Comment 8

[Geoff Lankow (:darktrojan)]

Comment on attachment 9189777 [details] [diff] [review]
662192-fake-ldap-server-2.patch

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

This is much more in-depth than the one I wrote. :-)

R+, but with a bunch of things to work on that you probably don't need me to check.

::: ldap/xpcom/tests/unit/xpcshell.ini
@@ +1,4 @@
>  [DEFAULT]
>  head =
>  tail =
> +support-files = data/*

You haven't added this file to the patch. But I'd change this line to point to mailnews/addrbook/test/unit/data/ldap_contacts.json anyway.

::: mailnews/test/fakeserver/Binaryd.jsm
@@ +96,5 @@
> +    let socket = new ServerSocket(
> +      port,
> +      true, // loopback only
> +      -1
> +    ); // default number of pending connections

This comment has wandered away from the previous line.

@@ +100,5 @@
> +    ); // default number of pending connections
> +
> +    let server = this;
> +
> +    socket.asyncListen({

Maybe throw in `QueryInterface: ChromeUtils.generateQI(["nsIServerSocketListener"])` here, it's not required but it would help future readers.

An alternative approach would be to make these methods members of BinaryServer and call `socket.asyncListen(this)`, which would remove the need for the `server` variable. It'd help with your TODO in stop() as well.

@@ +104,5 @@
> +    socket.asyncListen({
> +      async onSocketAccepted(socket, transport) {
> +        let connID = server._connID++;
> +        let conn = new Connection(transport);
> +        server._connections[connID] = conn;

Since connections never get referenced by ID outside this function you could make `_connections` a Set and use the add and delete methods. No need for an ID at all then.

::: mailnews/test/fakeserver/Ldapd.jsm
@@ +212,5 @@
> +  }
> +
> +  /**
> +   * Create a new sequence
> +   * @param {number} type - BER type byte

Let's have the types as constants somewhere so that we don't have to remember the values.

@@ +410,5 @@
> +      for (let attr in e.attributes) {
> +        let val = e.attributes[attr];
> +        if (Object.prototype.toString.call(val) !== "[object Array]") {
> +          e.attributes[attr] = [val];
> +        }

for...in is a bit old hat these days. There are potential pitfalls it's easy to forget about. It's *probably* fine here but I'd use `for (let [attr, val] of Object.entries(e.attributes))`.

An easier way to check if a JS variable is an array is `Array.isArray`.

@@ +457,5 @@
> +        };
> +      }
> +      case 2: {
> +        // not
> +        let subFilter = this.buildFilter(ber.children[0]); // one child

Maybe throw if there's not one child? Same below.

Comment 9

[Ben Campbell]

Attachment: 662192-fake-ldap-server-3.patch

(In reply to Geoff Lankow (:darktrojan) from comment #8)

All good stuff and mostly implemented. Exceptions as noted below.

An alternative approach would be to make these methods members of
BinaryServer and call socket.asyncListen(this), which would remove the
need for the server variable. It'd help with your TODO in stop() as well.

I think some of the other fake servers do this, but I wanted to keep the BinaryServer public interface as trivial as possible. I've found lots of cases in C-C where things derive from listeners to handle internal stuff (mainly because C++ is a bit annoying), and so so many times it's really confused me. So I decided to hide the listener, even if it makes the implementation a little more finicky.

• • @param {number} type - BER type byte

Let's have the types as constants somewhere so that we don't have to
remember the values.

I'm not yet sure on the best way to tackle it as there are a few cases where you're really treating them as bitfields (class/sequence/tag). You can make up helper functions, but they tend to end up producing more verbose code which obscures things more than just fiddling bits directly would...
I'll have a go next time I'm digging about in this - At the very least, I'll make sure the basic types have some constants. What's the best way to handle these in JS? Is a module somewhere in C-C you think is a particularly good example of defining constants?

for...in is a bit old hat these days. There are potential pitfalls it's easy
to forget about. It's probably fine here but I'd use for (let [attr, val] of Object.entries(e.attributes)).

Cool - that's exactly the kind of JS stuff I need to learn!

An easier way to check if a JS variable is an array is Array.isArray.

Doh! I discovered that I'd already visited the MDN page for Array.isArray at some point in the past too!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d5affc0fea36
Add a fake LDAP server. r=darktrojan