LDAP improvements:

- Better logging
  - Better error messages
  - Add unit tests
  - Clean up the setup tool for LDAP

data/users.py

@@ -11,12 +11,23 @@ from util.validation import generate_valid_usernames
 from data import model
 
 logger = logging.getLogger(__name__)
+if os.environ.get('LDAP_DEBUG') == '1':
+  logger.setLevel(logging.DEBUG)
 
+  ch = logging.StreamHandler()
+  ch.setLevel(logging.DEBUG)
+
+  logger.addHandler(ch)
 
 class DatabaseUsers(object):
   def verify_user(self, username_or_email, password):
     """ Simply delegate to the model implementation. """
-    return model.verify_user(username_or_email, password)
+    result = model.verify_user(username_or_email, password)
+    if not result:
+      return (None, 'Invalid Username or Password')
+
+    return (result, None)
+
 
   def user_exists(self, username):
     return model.get_user(username) is not None
@@ -30,9 +41,10 @@ class LDAPConnection(object):
     self._conn = None
 
   def __enter__(self):
-    trace_level = 2 if os.environ.get('LDAP_DEBUG') else 0
+    trace_level = 2 if os.environ.get('LDAP_DEBUG') == '1' else 0
     self._conn = ldap.initialize(self._ldap_uri, trace_level=trace_level)
     self._conn.simple_bind_s(self._user_dn, self._user_pw)
+
     return self._conn
 
   def __exit__(self, exc_type, value, tb):
@@ -55,7 +67,7 @@ class LDAPUsers(object):
       query = u'(|({0}={2})({1}={2}))'.format(self._uid_attr, self._email_attr,
                                               username_or_email)
 
-      logger.debug('Conducting user search: %s => %s', user_search_dn, query)
+      logger.debug('Conducting user search: %s under %s', query, user_search_dn)
       try:
         user = conn.search_s(user_search_dn, ldap.SCOPE_SUBTREE, query.encode('utf-8'))
       except ldap.LDAPError:
@@ -75,12 +87,12 @@ class LDAPUsers(object):
 
     # Make sure that even if the server supports anonymous binds, we don't allow it
     if not password:
-      return None
+      return (None, 'Anonymous binding not allowed')
 
     found_user = self._ldap_user_search(username_or_email)
 
     if found_user is None:
-      return None
+      return (None, 'Username not found')
 
     found_dn, found_response = found_user
 
@@ -91,9 +103,15 @@ class LDAPUsers(object):
         pass
     except ldap.INVALID_CREDENTIALS:
       logger.exception('Invalid LDAP credentials')
-      return None
+      return (None, 'Invalid password')
 
     # Now check if we have a federated login for this user
+    if not found_response.get(self._uid_attr):
+      return (None, 'Missing uid field "%s" in user record' % self._uid_attr)
+
+    if not found_response.get(self._email_attr):
+      return (None, 'Missing mail field "%s" in user record' % self._email_attr)
+
     username = found_response[self._uid_attr][0].decode('utf-8')
     email = found_response[self._email_attr][0]
     db_user = model.verify_federated_login('ldap', username)
@@ -107,7 +125,7 @@ class LDAPUsers(object):
 
       if not valid_username:
         logger.error('Unable to pick a username for user: %s', username)
-        return None
+        return (None, 'Unable to pick a username. Please report this to your administrator.')
 
       db_user = model.create_federated_user(valid_username, email, 'ldap', username,
                                             set_password_notification=False)
@@ -116,7 +134,7 @@ class LDAPUsers(object):
       db_user.email = email
       db_user.save()
 
-    return db_user
+    return (db_user, None)
 
   def user_exists(self, username):
     found_user = self._ldap_user_search(username)
@@ -225,12 +243,7 @@ class UserAuthentication(object):
       else:
         password = decrypted
 
-    result = self.state.verify_user(username_or_email, password)
+    return self.state.verify_user(username_or_email, password)
-    if result:
-      return (result, '')
-    else:
-      return (result, 'Invalid password.')
-
 
   def __getattr__(self, name):
     return getattr(self.state, name, None)

requirements-nover.txt

@@ -51,3 +51,4 @@ cachetools
 mock
 psutil
 stringscore
+mockldap

requirements.txt

@@ -37,6 +37,7 @@ jsonschema==2.4.0
 marisa-trie==0.7
 mixpanel-py==3.2.1
 mock==1.0.1
+mockldap==0.2.4
 paramiko==1.15.2
 peewee==2.4.7
 psutil==2.2.1

static/directives/config/config-setup-tool.html

@@ -310,12 +310,12 @@
           </p>
         </div>
 
-        <div class="alert alert-warning" ng-if="config.AUTHENTICATION_TYPE == 'LDAP' && !config.FEATURE_REQUIRE_ENCRYPTED_BASIC_AUTH">
+        <div class="co-alert co-alert-warning" ng-if="config.AUTHENTICATION_TYPE == 'LDAP' && !config.FEATURE_REQUIRE_ENCRYPTED_BASIC_AUTH">
           It is <strong>highly recommended</strong> to require encrypted client passwords. LDAP passwords used in the Docker client will be stored in <strong>plaintext</strong>!
           <a href="javascript:void(0)" ng-click="config.FEATURE_REQUIRE_ENCRYPTED_BASIC_AUTH = true">Enable this requirement now</a>.
         </div>
 
-        <div class="alert alert-success" ng-if="config.AUTHENTICATION_TYPE == 'LDAP' && config.FEATURE_REQUIRE_ENCRYPTED_BASIC_AUTH">
+        <div class="co-alert co-alert-success" ng-if="config.AUTHENTICATION_TYPE == 'LDAP' && config.FEATURE_REQUIRE_ENCRYPTED_BASIC_AUTH">
           Note: The "Require Encrypted Client Passwords" feature is currently enabled which will
           prevent LDAP passwords from being saved as plaintext by the Docker client.
         </div>
@@ -343,29 +343,76 @@
               </div>
             </td>
           </tr>
-          <tr>
-            <td>Administrator DN:</td>
-            <td><span class="config-string-field" binding="config.LDAP_ADMIN_DN"></span></td>
-          </tr>
           <tr>
             <td>Base DN:</td>
-            <td><span class="config-list-field" item-title="DN" binding="config.LDAP_BASE_DN"></span></td>
+            <td>
+              <span class="config-list-field" item-title="DN" binding="config.LDAP_BASE_DN"></span>
+              <div class="help-text">
+              A list of Distinguished Name pieces which forms the base path for
+              looking up all LDAP records.
+              </div>
+              <div class="help-text">
+                Example: [dc=my,dc=domain,dc=com]
+              </div>
+            </td>
           </tr>
           <tr>
-            <td>Administrator Password:</td>
+            <td>User Relative DN:</td>
-            <td><span class="config-string-field" binding="config.LDAP_ADMIN_PASSWD"></span></td>
+            <td>
+              <span class="config-list-field" item-title="RDN" binding="config.LDAP_USER_RDN"></span>
+              <div class="help-text">
+              A list of Distinguished Name pieces which forms the base path for
+              looking up all user LDAP records, relative to the Base DN defined above.
+              </div>
+              <div class="help-text">
+                Example: [ou=employees]
+              </div>
+            </td>
           </tr>
           <tr>
-            <td>E-mail Attribute:</td>
+            <td>Administrator DN:</td>
-            <td><span class="config-string-field" binding="config.LDAP_EMAIL_ATTR"></span></td>
+            <td><span class="config-string-field" binding="config.LDAP_ADMIN_DN"></span>
+              <div class="help-text">
+              The Distinguished Name for the Administrator account. This account must be able to login and view the records for all user accounts.
+              </div>
+              <div class="help-text">
+                Example: uid=admin,ou=employees,dc=my,dc=domain,dc=com
+              </div>
+            </td>
+          </tr>
+          <tr>
+            <td>Administrator DN Password:</td>
+            <td>
+              <div class="co-alert co-alert-warning">
+                Note: This will be stored in
+                <strong>plaintext</strong> inside the config.yaml, so setting up a dedicated account or using
+                <a href="http://tools.ietf.org/id/draft-stroeder-hashed-userpassword-values-01.html" target="_blank">a password hash</a> is <strong>highly</strong> recommended.
+              </div>
+              <span class="config-string-field" binding="config.LDAP_ADMIN_PASSWD"></span>
+              <div class="help-text">
+               The password for the Administrator DN.
+              </div>
+            </td>
           </tr>
           <tr>
             <td>UID Attribute:</td>
-            <td><span class="config-string-field" binding="config.LDAP_UID_ATTR"></span></td>
+            <td>
+              <span class="config-string-field" binding="config.LDAP_UID_ATTR" default-value="uid"></span>
+              <div class="help-text">
+               The name of the property field in your LDAP user records that stores your
+               users' username. Typically "uid".
+              </div>
+            </td>
           </tr>
           <tr>
-            <td>User RDN:</td>
+            <td>Mail Attribute:</td>
-            <td><span class="config-list-field" item-title="RDN" binding="config.LDAP_USER_RDN"></span></td>
+            <td>
+              <span class="config-string-field" binding="config.LDAP_EMAIL_ATTR" default-value="mail"></span>
+              <div class="help-text">
+               The name of the property field in your LDAP user records that stores your
+               users' e-mail address(es). Typically "mail".
+              </div>
+            </td>
           </tr>
         </table>
       </div>

test/test_ldap.py

@@ -0,0 +1,83 @@
+import unittest
+
+from app import app
+from initdb import setup_database_for_testing, finished_database_for_testing
+from data import model
+from data.users import LDAPUsers
+
+from mockldap import MockLdap
+
+class TestLDAP(unittest.TestCase):
+  def setUp(self):
+    setup_database_for_testing(self)
+    self.app = app.test_client()
+    self.ctx = app.test_request_context()
+    self.ctx.__enter__()
+
+    self.mockldap = MockLdap({
+      'dc=quay,dc=io': {'dc': ['quay', 'io']},
+      'ou=employees,dc=quay,dc=io': {
+        'dc': ['quay', 'io'],
+        'ou': 'employees'
+      },
+      'uid=testy,ou=employees,dc=quay,dc=io': {
+        'dc': ['quay', 'io'],
+        'ou': 'employees',
+        'uid': 'testy',
+        'userPassword': ['password']
+      },
+      'uid=someuser,ou=employees,dc=quay,dc=io': {
+        'dc': ['quay', 'io'],
+        'ou': 'employees',
+        'uid': ['someuser'],
+        'userPassword': ['somepass'],
+        'mail': ['foo@bar.com']
+      },
+      'uid=nomail,ou=employees,dc=quay,dc=io': {
+        'dc': ['quay', 'io'],
+        'ou': 'employees',
+        'uid': ['nomail'],
+        'userPassword': ['somepass']
+      }
+    })
+
+    self.mockldap.start()
+
+  def tearDown(self):
+    self.mockldap.stop()
+    finished_database_for_testing(self)
+    self.ctx.__exit__(True, None, None)
+
+  def test_login(self):
+    base_dn = ['dc=quay', 'dc=io']
+    admin_dn = 'uid=testy,ou=employees,dc=quay,dc=io'
+    admin_passwd = 'password'
+    user_rdn = ['ou=employees']
+    uid_attr = 'uid'
+    email_attr = 'mail'
+
+    ldap = LDAPUsers('ldap://localhost', base_dn, admin_dn, admin_passwd, user_rdn,
+                     uid_attr, email_attr)
+
+    (response, _) = ldap.verify_user('someuser', 'somepass')
+    self.assertEquals(response.username, 'someuser')
+
+  def test_missing_mail(self):
+    base_dn = ['dc=quay', 'dc=io']
+    admin_dn = 'uid=testy,ou=employees,dc=quay,dc=io'
+    admin_passwd = 'password'
+    user_rdn = ['ou=employees']
+    uid_attr = 'uid'
+    email_attr = 'mail'
+
+    ldap = LDAPUsers('ldap://localhost', base_dn, admin_dn, admin_passwd, user_rdn,
+                     uid_attr, email_attr)
+
+    (response, err_msg) = ldap.verify_user('nomail', 'somepass')
+    self.assertIsNone(response)
+    self.assertEquals('Missing mail field "mail" in user record', err_msg)
+
+
+if __name__ == '__main__':
+  unittest.main()
+