Merge branch 'user_filter_auth' into 'master'

Centralized all LDAP config logic in to `Gitlab::LDAP::Config`. We had varying configuration for devise/omniauth and other things. For example, `user_filter` was never taken in to account for devise/omniauth so a user object would always be created, even if the user did not match the user_filter. 

Fixes gitlab-org/gitlab-ce#21195, https://gitlab.com/gitlab-org/gitlab-ce/issues/15396 and gitlab-org/gitlab-ce#13296

See merge request !6606

7 files changed, 150 insertions, 31 deletions

diff --git a/app/helpers/auth_helper.rb b/app/helpers/auth_helper.rb
index cd4d778e508..92bac149313 100644
--- a/app/helpers/auth_helper.rb
+++ b/app/helpers/auth_helper.rb
@@ -3,7 +3,7 @@ module AuthHelper
   FORM_BASED_PROVIDERS = [/\Aldap/, 'crowd'].freeze
 
   def ldap_enabled?
-    Gitlab.config.ldap.enabled
+    Gitlab::LDAP::Config.enabled?
   end
 
   def omniauth_enabled?
diff --git a/changelogs/unreleased/user_filter_auth.yml b/changelogs/unreleased/user_filter_auth.yml
new file mode 100644
index 00000000000..e4071e22e5e
--- /dev/null
+++ b/changelogs/unreleased/user_filter_auth.yml
@@ -0,0 +1,4 @@
+---
+title: Centralize LDAP config/filter logic
+merge_request: 6606
+author: 
diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb
index a0a8f88584c..f06c4d4ecf2 100644
--- a/config/initializers/devise.rb
+++ b/config/initializers/devise.rb
@@ -213,22 +213,9 @@ Devise.setup do |config|
   end
 
   if Gitlab::LDAP::Config.enabled?
-    Gitlab.config.ldap.servers.values.each do |server|
-      if server['allow_username_or_email_login']
-        email_stripping_proc = ->(name) {name.gsub(/@.*\z/, '')}
-      else
-        email_stripping_proc = ->(name) {name}
-      end
-
-      config.omniauth server['provider_name'],
-        host:     server['host'],
-        base:     server['base'],
-        uid:      server['uid'],
-        port:     server['port'],
-        method:   server['method'],
-        bind_dn:  server['bind_dn'],
-        password: server['password'],
-        name_proc: email_stripping_proc
+    Gitlab::LDAP::Config.providers.each do |provider|
+      ldap_config = Gitlab::LDAP::Config.new(provider)
+      config.omniauth(provider, ldap_config.omniauth_options)
     end
   end
 
diff --git a/lib/gitlab/ldap/adapter.rb b/lib/gitlab/ldap/adapter.rb
index 8b38cfaefb6..7b05290e5cc 100644
--- a/lib/gitlab/ldap/adapter.rb
+++ b/lib/gitlab/ldap/adapter.rb
@@ -89,9 +89,7 @@ module Gitlab
       end
 
       def user_filter(filter = nil)
-        if config.user_filter.present?
-          user_filter = Net::LDAP::Filter.construct(config.user_filter)
-        end
+        user_filter = config.constructed_user_filter if config.user_filter.present?
 
         if user_filter && filter
           Net::LDAP::Filter.join(filter, user_filter)
diff --git a/lib/gitlab/ldap/authentication.rb b/lib/gitlab/ldap/authentication.rb
index bad683c6511..4745311402c 100644
--- a/lib/gitlab/ldap/authentication.rb
+++ b/lib/gitlab/ldap/authentication.rb
@@ -54,11 +54,9 @@ module Gitlab
 
         # Apply LDAP user filter if present
         if config.user_filter.present?
-          filter = Net::LDAP::Filter.join(
-            filter,
-            Net::LDAP::Filter.construct(config.user_filter)
-          )
+          filter = Net::LDAP::Filter.join(filter, config.constructed_user_filter)
         end
+
         filter
       end
 
diff --git a/lib/gitlab/ldap/config.rb b/lib/gitlab/ldap/config.rb
index 6ea069d26df..de52ef3fc65 100644
--- a/lib/gitlab/ldap/config.rb
+++ b/lib/gitlab/ldap/config.rb
@@ -13,7 +13,7 @@ module Gitlab
       end
 
       def self.providers
-        servers.map {|server| server['provider_name'] }
+        servers.map { |server| server['provider_name'] }
       end
 
       def self.valid_provider?(provider)
@@ -38,13 +38,31 @@ module Gitlab
       end
 
       def adapter_options
-        {
-          host: options['host'],
-          port: options['port'],
-          encryption: encryption
-        }.tap do |options|
-          options.merge!(auth_options) if has_auth?
+        opts = base_options.merge(
+          encryption: encryption,
+        )
+
+        opts.merge!(auth_options) if has_auth?
+
+        opts
+      end
+
+      def omniauth_options
+        opts = base_options.merge(
+          base: base,
+          method: options['method'],
+          filter: omniauth_user_filter,
+          name_proc: name_proc
+        )
+
+        if has_auth?
+          opts.merge!(
+            bind_dn: options['bind_dn'],
+            password: options['password']
+          )
         end
+
+        opts
       end
 
       def base
@@ -68,6 +86,10 @@ module Gitlab
         options['user_filter']
       end
 
+      def constructed_user_filter
+        @constructed_user_filter ||= Net::LDAP::Filter.construct(user_filter)
+      end
+
       def group_base
         options['group_base']
       end
@@ -96,8 +118,27 @@ module Gitlab
         options['password'] || options['bind_dn']
       end
 
+      def allow_username_or_email_login
+        options['allow_username_or_email_login']
+      end
+
+      def name_proc
+        if allow_username_or_email_login
+          Proc.new { |name| name.gsub(/@.*\z/, '') }
+        else
+          Proc.new { |name| name }
+        end
+      end
+
       protected
 
+      def base_options
+        {
+          host: options['host'],
+          port: options['port']
+        }
+      end
+
       def base_config
         Gitlab.config.ldap
       end
@@ -126,6 +167,16 @@ module Gitlab
           }
         }
       end
+
+      def omniauth_user_filter
+        uid_filter = Net::LDAP::Filter.eq(uid, '%{username}')
+
+        if user_filter.present?
+          Net::LDAP::Filter.join(uid_filter, constructed_user_filter).to_s
+        else
+          uid_filter.to_s
+        end
+      end
     end
   end
 end
diff --git a/spec/lib/gitlab/ldap/config_spec.rb b/spec/lib/gitlab/ldap/config_spec.rb
index f5ebe703083..1a6803e01c3 100644
--- a/spec/lib/gitlab/ldap/config_spec.rb
+++ b/spec/lib/gitlab/ldap/config_spec.rb
@@ -19,6 +19,87 @@ describe Gitlab::LDAP::Config, lib: true do
     end
   end
 
+  describe '#adapter_options' do
+    it 'constructs basic options' do
+      stub_ldap_config(
+        options: {
+          'host'    => 'ldap.example.com',
+          'port'    => 386,
+          'method'  => 'plain'
+        }
+      )
+
+      expect(config.adapter_options).to eq(
+        host: 'ldap.example.com',
+        port: 386,
+        encryption: nil
+      )
+    end
+
+    it 'includes authentication options when auth is configured' do
+      stub_ldap_config(
+        options: {
+          'host'      => 'ldap.example.com',
+          'port'      => 686,
+          'method'    => 'ssl',
+          'bind_dn'   => 'uid=admin,dc=example,dc=com',
+          'password'  => 'super_secret'
+        }
+      )
+
+      expect(config.adapter_options).to eq(
+        host: 'ldap.example.com',
+        port: 686,
+        encryption: :simple_tls,
+        auth: {
+          method: :simple,
+          username: 'uid=admin,dc=example,dc=com',
+          password: 'super_secret'
+        }
+      )
+    end
+  end
+
+  describe '#omniauth_options' do
+    it 'constructs basic options' do
+      stub_ldap_config(
+        options: {
+          'host'    => 'ldap.example.com',
+          'port'    => 386,
+          'base'    => 'ou=users,dc=example,dc=com',
+          'method'  => 'plain',
+          'uid'     => 'uid'
+        }
+      )
+
+      expect(config.omniauth_options).to include(
+        host: 'ldap.example.com',
+        port: 386,
+        base: 'ou=users,dc=example,dc=com',
+        method: 'plain',
+        filter: '(uid=%{username})'
+      )
+      expect(config.omniauth_options.keys).not_to include(:bind_dn, :password)
+    end
+
+    it 'includes authentication options when auth is configured' do
+      stub_ldap_config(
+        options: {
+          'uid'         => 'sAMAccountName',
+          'user_filter' => '(memberOf=cn=group1,ou=groups,dc=example,dc=com)',
+          'bind_dn'     => 'uid=admin,dc=example,dc=com',
+          'password'    => 'super_secret'
+        }
+      )
+
+      expect(config.omniauth_options).to include(
+        filter: '(&(sAMAccountName=%{username})(memberOf=cn=group1,ou=groups,dc=example,dc=com))',
+        bind_dn: 'uid=admin,dc=example,dc=com',
+        password: 'super_secret'
+      )
+    end
+  end
+
   describe '#has_auth?' do
     it 'is true when password is set' do
       stub_ldap_config(