diff --git a/app/helpers/auth_helper.rb b/app/helpers/auth_helper.rb
index cd4d778e508252b1e3e9b4508ac5af171fa9a120..92bac149313d00d6cefaeffc05cf563e17618aae 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 0000000000000000000000000000000000000000..e4071e22e5e052b5cc821189040e1ada3e47b0ec
--- /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 a0a8f88584c3dc8450daea995dc19df183b42f31..f06c4d4ecf22bac093b8edbfe52bb6a2160e67ca 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 8b38cfaefb6049a7582f28b94fc1d874a163c55a..7b05290e5cc404442cafb29be57525149d767775 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 bad683c65113eca563b09eed69c27703bf4c58ea..4745311402ce0153a39d0de34d7f704ff646c27c 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 6ea069d26df25636634a4eac412c2c64c7b76f20..de52ef3fc657fea039a202b4f3e8dca0700f8811 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 f5ebe70308364fc1ce1e971497a1bac828218af9..1a6803e01c32799ec5e6fe1d8bec8f456c5fdb47 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(