Add LDAP auth tests (#864)

rickbergfalk · web-flow · commit c328cec7950f · 2020-10-26T11:27:03.000-05:00
* Add ldap service to testing setup

* Remove open admin registration from ldap auth

* Add ldap tests

* test name consistencty

* Add documentation for LDAP roles
diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml
@@ -52,6 +52,9 @@ jobs:
       - name: Start redis
         run: docker-compose -f server/docker-compose.yml up -d redis &
 
+      - name: Start ldap
+        run: docker-compose -f server/docker-compose.yml up -d ldap &
+
       - name: Start service for backend server
         if: ${{ matrix.backendtype }}
         run: docker-compose -f server/docker-compose.yml up -d ${{ matrix.backendtype }} &
diff --git a/DEVELOPER-GUIDE.md b/DEVELOPER-GUIDE.md
@@ -123,8 +123,14 @@ Tests are located under `server/test` directory, and use `mocha` as a test runne
 cd server
 # if dependencies have not yet been installed (build.sh will do this)
 npm ci
+
 # If test fixture has not yet been generated (build.sh will do this)
 node generate-test-db-fixture.js
+
+# Start backend services if desired. (-d runs in background)
+docker-compose up -d redis
+docker-compose up -d ldap
+
 # Run tests
 npm test
 
diff --git a/docs/authentication.md b/docs/authentication.md
@@ -131,6 +131,10 @@ SQLPad users do not need to be added ahead of time, and may be created on the fl
 
 ## LDAP (Experimental)
 
+?> Available as of 5.8.0
+
+!> LDAP does not honor "open admin registration" for initial log in. Use `SQLPAD_ADMIN` to set initial admin email address if necessary
+
 LDAP-based authentication can be enabled by setting the necessary environment variables:
 
 - `SQLPAD_LDAP_AUTH_ENABLED` - Set to TRUE if LDAP enable, FALSE if LDAP disable.
@@ -145,7 +149,9 @@ LDAP-based authentication can be enabled by setting the necessary environment va
 - `SQLPAD_LDAP_ROLE_EDITOR_FILTER` - LDAP filter used to determine if a user should be assigned SQLPad editor role
 - `SQLPAD_LDAP_DEFAULT_ROLE`- Default role for users that do not match LDAP role filters. May be either `admin`, `editor`, `denied`, or empty. If `denied` or empty, a user _must_ match an LDAP role filter to be admitted into SQLPad, unless they are previously created as a SQLPad user in advanced.
 
-To assign roles via LDAP-RBAC, you may specify a profile attribute and value to look to for a particular role.
+To assign roles via LDAP-RBAC, you may specify additional LDAP user filters to ensure the user fits a particular role or group.
+
+?> Roles assigned via LDAP will sync on every login.
 
 For example, if your LDAP implementation supports `memberOf`, you may decide to use group DN values. In this case two groups are needed, one for editors and one for admins.
 
diff --git a/server/auth-strategies/ldap.js b/server/auth-strategies/ldap.js
@@ -212,10 +212,7 @@ function enableLdap(config) {
             }
           }
 
-          let [openAdminRegistration, user] = await Promise.all([
-            models.users.adminRegistrationOpen(),
-            models.users.findOneByEmail(email),
-          ]);
+          let user = await models.users.findOneByEmail(email);
 
           if (user) {
             if (user.disabled) {
@@ -234,21 +231,16 @@ function enableLdap(config) {
             return done(null, user);
           }
 
-          // At this point user was not found, and a decision needs to be made to either create or reject the user
-
-          // If openAdminRegistration is enabled, this is the initial user
-          // The users role should be admin
-          if (openAdminRegistration) {
-            role = 'admin';
-          }
+          // At this point user was not found
+          // A decision needs to be made to either create or reject the user
 
           // If role is still not set, and default is provided, use that
           if (!role) {
             role = ldapDefaultRole;
           }
 
-          // Finally, if role is set, and open admin registration or ldap auto sign up, create user
-          if (role && (openAdminRegistration || config.get('ldapAutoSignUp'))) {
+          // If role is set and ldap auto sign up is turned on, create user
+          if (role && config.get('ldapAutoSignUp')) {
             appLog.debug(`adding user ${profileUsername} to role ${role}`);
             const newUser = await models.users.create({
               email,
diff --git a/server/docker-compose.yml b/server/docker-compose.yml
@@ -5,6 +5,12 @@ services:
     ports:
       - 6379:6379
 
+  ldap:
+    image: rroemhild/test-openldap
+    ports:
+      - 389:389
+    privileged: true
+
   mssql:
     image: 'mcr.microsoft.com/mssql/server:2017-latest'
     hostname: 'mssql'
diff --git a/server/test/auth/ldap.js b/server/test/auth/ldap.js
diff --git a/server/test/utils.js b/server/test/utils.js
