(Note: I uploaded the patch with a wrong "Base Diff Path"... I don't know how to fix it.)


There is a serious bug in the current LDAP auth backend: it is possible to authenticate as any user by entering an empty password ! 

This is because the password is used in a bind request, but in LDAP a bind request with an empty password is equivalent to an anonymous bind request, and most LDAP servers allow anonymous bind requests.

But more generally speaking, the design of the LDAP backend is flawed and inflexible. For example with a standard Windows domain controller, a typical DN might look like:
  CN=John Smith,OU=engineering,DC=company,DC=com

So if the following User Mask is specified in Review Board's LDAP settings:
  CN=%s,OU=engineering,DC=company,DC=com

Endusers are forced to type their full name ("John Smith") as their username, instead of the more practical SAM Account Name (which could be "jsmith" -- Windows domain controllers store it in the sAMAccountName attribute).

The right way to implement LDAP authentication is to do what lighttpd's mod_auth does for example (see http://redmine.lighttpd.net/wiki/lighttpd/Docs:ModAuth).  Basically when an enduser enters <enduser-login> and <enduser-passwd>, Review Board should:

(1) Immediately fail the authentication if <enduser-passwd> is empty.
(2) Bind anonymously, or with a given DN/password (<bind-DN> and <bind-passwd>).
(3) Find the enduser's DN <enduser-DN> by doing a SUBTREE search of a <base-DN> (eg. "OU=engineering,DC=company,DC=com") with a filter <filter> built from <enduser-login> (eg. "sAMAccountName=<enduser-login>").
(4) Retrieve <enduser-DN> from the SUBTREE search.
(5) Attempt to bind with <enduser-DN> and <enduser-passwd> to verify the validity of the auth credentials.

So a Review Board administrator needs to specify up to 4 LDAP parameters in addition to the LDAP server:

  <bind-DN>     (optional)
  <bind-passwd> (optional)
  <base-DN>     (typically something like OU=foo,DC=company,DC=com)
  <filter>      (typically sAMAccountName=%s for Windows-based domain controllers)

I have written a patch to implement all of this correctly. I have repurposed the 3 existing parameters as follow: 

  Anonymous User Mask:     <bind-DN>
  Anonymous User Password: <bind-passwd>
  User Mask:               <filter>

I don't know Django that well so I have hard-coded <base-DN> in my patch, but ultimately it should be added to the LDAP configuration page /admin/settings/general/. And the description of these settings needs to be updated as well. 

I use this patch on a Review Board server, the LDAP server is a Windows 2003 DC, and it seems to work well.

Let me know what you think.

-marc

Tested on Review Board SVN r1541, with a Windows 2003 DC as the LDAP server.