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.
This commit is contained in:
parent
6eeff67c6e
commit
c50b98da72
|
@ -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
|
||||
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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