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
This commit is contained in:
commit
2a90b69a8d
|
@ -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?
|
||||
|
|
|
@ -0,0 +1,4 @@
|
|||
---
|
||||
title: Centralize LDAP config/filter logic
|
||||
merge_request: 6606
|
||||
author:
|
|
@ -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
|
||||
|
||||
|
|
|
@ -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)
|
||||
|
|
|
@ -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
|
||||
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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(
|
||||
|
|
Loading…
Reference in New Issue