From c50b98da723dab9a35ddb2cde0258d141cf92495 Mon Sep 17 00:00:00 2001 From: Drew Blessing 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 cd4d778e5082..92bac149313d 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 000000000000..e4071e22e5e0 --- /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 a0a8f88584c3..f06c4d4ecf22 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -213,22 +213,9 @@ 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 8b38cfaefb60..7b05290e5cc4 100644 --- a/lib/gitlab/ldap/adapter.rb +++ b/lib/gitlab/ldap/adapter.rb @@ -89,9 +89,7 @@ def user_options(field, value, limit) 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 bad683c65113..4745311402ce 100644 --- a/lib/gitlab/ldap/authentication.rb +++ b/lib/gitlab/ldap/authentication.rb @@ -54,11 +54,9 @@ def user_filter(login) # 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 6ea069d26df2..de52ef3fc657 100644 --- a/lib/gitlab/ldap/config.rb +++ b/lib/gitlab/ldap/config.rb @@ -13,7 +13,7 @@ def self.servers 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 @@ def enabled? 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 @@ def user_filter 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 @@ def has_auth? 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 @@ def auth_options } } 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 f5ebe7030836..1a6803e01c32 100644 --- a/spec/lib/gitlab/ldap/config_spec.rb +++ b/spec/lib/gitlab/ldap/config_spec.rb @@ -19,6 +19,87 @@ 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