Commit ae84eaeb authored by Francisco Javier López's avatar Francisco Javier López Committed by Douwe Maan

Add better LDAP connection handling

parent 5d16fc67
......@@ -229,10 +229,6 @@ class ApplicationController < ActionController::Base
@event_filter ||= EventFilter.new(filters)
end
def gitlab_ldap_access(&block)
Gitlab::Auth::LDAP::Access.open { |access| yield(access) }
end
# JSON for infinite scroll via Pager object
def pager_json(partial, count, locals = {})
html = render_to_string(
......
---
title: Add better LDAP connection handling
merge_request: 18039
author:
type: fixed
......@@ -52,6 +52,8 @@ module Gitlab
block_user(user, 'does not exist anymore')
false
end
rescue LDAPConnectionError
false
end
def adapter
......
......@@ -2,6 +2,9 @@ module Gitlab
module Auth
module LDAP
class Adapter
SEARCH_RETRY_FACTOR = [1, 1, 2, 3].freeze
MAX_SEARCH_RETRIES = Rails.env.test? ? 1 : SEARCH_RETRY_FACTOR.size.freeze
attr_reader :provider, :ldap
def self.open(provider, &block)
......@@ -16,7 +19,7 @@ module Gitlab
def initialize(provider, ldap = nil)
@provider = provider
@ldap = ldap || Net::LDAP.new(config.adapter_options)
@ldap = ldap || renew_connection_adapter
end
def config
......@@ -47,8 +50,10 @@ module Gitlab
end
def ldap_search(*args)
retries ||= 0
# Net::LDAP's `time` argument doesn't work. Use Ruby `Timeout` instead.
Timeout.timeout(config.timeout) do
Timeout.timeout(timeout_time(retries)) do
results = ldap.search(*args)
if results.nil?
......@@ -63,16 +68,26 @@ module Gitlab
results
end
end
rescue Net::LDAP::Error => error
Rails.logger.warn("LDAP search raised exception #{error.class}: #{error.message}")
[]
rescue Timeout::Error
Rails.logger.warn("LDAP search timed out after #{config.timeout} seconds")
[]
rescue Net::LDAP::Error, Timeout::Error => error
retries += 1
error_message = connection_error_message(error)
Rails.logger.warn(error_message)
if retries < MAX_SEARCH_RETRIES
renew_connection_adapter
retry
else
raise LDAPConnectionError, error_message
end
end
private
def timeout_time(retry_number)
SEARCH_RETRY_FACTOR[retry_number] * config.timeout
end
def user_options(fields, value, limit)
options = {
attributes: Gitlab::Auth::LDAP::Person.ldap_attributes(config),
......@@ -104,6 +119,18 @@ module Gitlab
filter
end
end
def connection_error_message(exception)
if exception.is_a?(Timeout::Error)
"LDAP search timed out after #{config.timeout} seconds"
else
"LDAP search raised exception #{exception.class}: #{exception.message}"
end
end
def renew_connection_adapter
@ldap = Net::LDAP.new(config.adapter_options)
end
end
end
end
......
module Gitlab
module Auth
module LDAP
LDAPConnectionError = Class.new(StandardError)
end
end
end
......@@ -124,6 +124,9 @@ module Gitlab
Gitlab::Auth::LDAP::Person.find_by_uid(auth_hash.uid, adapter) ||
Gitlab::Auth::LDAP::Person.find_by_email(auth_hash.uid, adapter) ||
Gitlab::Auth::LDAP::Person.find_by_dn(auth_hash.uid, adapter)
rescue Gitlab::Auth::LDAP::LDAPConnectionError
nil
end
def ldap_config
......
require 'spec_helper'
describe Gitlab::Auth::LDAP::Access do
include LdapHelpers
let(:access) { described_class.new user }
let(:user) { create(:omniauth_user) }
......@@ -32,8 +34,10 @@ describe Gitlab::Auth::LDAP::Access do
end
context 'when the user is found' do
let(:ldap_user) { Gitlab::Auth::LDAP::Person.new(Net::LDAP::Entry.new, 'ldapmain') }
before do
allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(:ldap_user)
allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(ldap_user)
end
context 'and the user is disabled via active directory' do
......@@ -120,6 +124,22 @@ describe Gitlab::Auth::LDAP::Access do
end
end
end
context 'when the connection fails' do
before do
raise_ldap_connection_error
end
it 'does not block the user' do
access.allowed?
expect(user.ldap_blocked?).to be_falsey
end
it 'denies access' do
expect(access.allowed?).to be_falsey
end
end
end
describe '#block_user' do
......
......@@ -124,16 +124,36 @@ describe Gitlab::Auth::LDAP::Adapter do
context "when the search raises an LDAP exception" do
before do
allow(adapter).to receive(:renew_connection_adapter).and_return(ldap)
allow(ldap).to receive(:search) { raise Net::LDAP::Error, "some error" }
allow(Rails.logger).to receive(:warn)
end
it { is_expected.to eq [] }
context 'retries the operation' do
before do
stub_const("#{described_class}::MAX_SEARCH_RETRIES", 3)
end
it 'as many times as MAX_SEARCH_RETRIES' do
expect(ldap).to receive(:search).exactly(3).times
expect { subject }.to raise_error(Gitlab::Auth::LDAP::LDAPConnectionError)
end
context 'when no more retries' do
before do
stub_const("#{described_class}::MAX_SEARCH_RETRIES", 1)
end
it 'logs the error' do
subject
expect(Rails.logger).to have_received(:warn).with(
"LDAP search raised exception Net::LDAP::Error: some error")
it 'raises the exception' do
expect { subject }.to raise_error(Gitlab::Auth::LDAP::LDAPConnectionError)
end
it 'logs the error' do
expect { subject }.to raise_error(Gitlab::Auth::LDAP::LDAPConnectionError)
expect(Rails.logger).to have_received(:warn).with(
"LDAP search raised exception Net::LDAP::Error: some error")
end
end
end
end
end
......
require 'spec_helper'
describe Gitlab::Auth::OAuth::User do
include LdapHelpers
let(:oauth_user) { described_class.new(auth_hash) }
let(:gl_user) { oauth_user.gl_user }
let(:uid) { 'my-uid' }
......@@ -38,10 +40,6 @@ describe Gitlab::Auth::OAuth::User do
end
describe '#save' do
def stub_ldap_config(messages)
allow(Gitlab::Auth::LDAP::Config).to receive_messages(messages)
end
let(:provider) { 'twitter' }
describe 'when account exists on server' do
......@@ -269,20 +267,47 @@ describe Gitlab::Auth::OAuth::User do
end
context 'when an LDAP person is not found by uid' do
it 'tries to find an LDAP person by DN and adds the omniauth identity to the user' do
it 'tries to find an LDAP person by email and adds the omniauth identity to the user' do
allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_uid).and_return(nil)
allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(ldap_user)
allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_email).and_return(ldap_user)
oauth_user.save
identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
expect(identities_as_hash).to match_array(result_identities(dn, uid))
end
context 'when also not found by email' do
it 'tries to find an LDAP person by DN and adds the omniauth identity to the user' do
allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_uid).and_return(nil)
allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_email).and_return(nil)
allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(ldap_user)
oauth_user.save
identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
expect(identities_as_hash).to match_array(result_identities(dn, uid))
end
end
end
def result_identities(dn, uid)
[
{ provider: 'ldapmain', extern_uid: dn },
{ provider: 'twitter', extern_uid: uid }
]
end
context 'when there is an LDAP connection error' do
before do
raise_ldap_connection_error
end
it 'does not save the identity' do
oauth_user.save
identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
expect(identities_as_hash)
.to match_array(
[
{ provider: 'ldapmain', extern_uid: dn },
{ provider: 'twitter', extern_uid: uid }
]
)
expect(identities_as_hash).to match_array([{ provider: 'twitter', extern_uid: uid }])
end
end
end
......@@ -739,4 +764,19 @@ describe Gitlab::Auth::OAuth::User do
expect(oauth_user.find_user).to eql gl_user
end
end
describe '#find_ldap_person' do
context 'when LDAP connection fails' do
before do
raise_ldap_connection_error
end
it 'returns nil' do
adapter = Gitlab::Auth::LDAP::Adapter.new('ldapmain')
hash = OmniAuth::AuthHash.new(uid: 'whatever', provider: 'ldapmain')
expect(oauth_user.send(:find_ldap_person, hash, adapter)).to be_nil
end
end
end
end
......@@ -41,4 +41,9 @@ module LdapHelpers
entry
end
def raise_ldap_connection_error
allow_any_instance_of(Gitlab::Auth::LDAP::Adapter)
.to receive(:ldap_search).and_raise(Gitlab::Auth::LDAP::LDAPConnectionError)
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment