From c50b98da723dab9a35ddb2cde0258d141cf92495 Mon Sep 17 00:00:00 2001
From: Drew Blessing <drew@gitlab.com>
Date: Fri, 11 Nov 2016 14:44:08 -0600
Subject: [PATCH] Centralize LDAP config/filter logic
Centralize all LDAP config logic in `GitLab::LDAP::Config`. Previously,
some logic was in the Devise initializer and it was not honoring the
`user_filter`. If a user outside the configured `user_filter` signed
in, an account would be created but they would then be denied access.
Now that logic is centralized, the filter is honored and users outside
the filter are never created.
---
app/helpers/auth_helper.rb | 2 +-
changelogs/unreleased/user_filter_auth.yml | 4 ++
config/initializers/devise.rb | 19 +----
lib/gitlab/ldap/adapter.rb | 4 +-
lib/gitlab/ldap/authentication.rb | 6 +-
lib/gitlab/ldap/config.rb | 65 +++++++++++++++--
spec/lib/gitlab/ldap/config_spec.rb | 81 ++++++++++++++++++++++
7 files changed, 150 insertions(+), 31 deletions(-)
create mode 100644 changelogs/unreleased/user_filter_auth.yml
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(
--
GitLab